fix: stop archive-on-merge opening duplicate PRs for the same change

[maryia-deriv]

Closes #141.

What happened

On deriv-blox, three merges within a minute (#404, #367, #319) produced three identical archive PRs (#411, #412, #413), all archiving the same change. (deriv-blox runs an old local copy of this workflow — PR-numbered branches, diff-based detection — but this PR hardens the shared reusable workflow against every cause listed in the issue.)

The fix, mapped to the issue's suggestions

1. Skip when the merged PR's head branch starts with archive-on-merge/ — already on master; unchanged.

2. Skip changes an open archive-on-merge/* PR already covers — new coverage check in the script: before archiving anything, it paginates the repo's open PRs, keeps those headed by archive-on-merge/*, and reads their file lists. An archive PR's diff deletes openspec/changes/<name>/…, so the same extraction used on a merged PR's files recovers exactly what it covers; covered changes are skipped with a log line naming the covering PR. The workflow's same-branch gh pr list check stays as a backstop.

3. Per-repo concurrency group — the group is now archive-on-merge-${{ github.repository }} (was keyed per PR), so back-to-back merges serialize and cannot race.

Why detection is now repo-wide

The old per-PR concurrency comment warned that a repo-wide group makes GitHub drop intermediate pending runs, silently losing those merges' archives. That hazard is real, so this PR removes its precondition: candidates are now every change on the base branch whose tasks.md is fully checked, not only the ones the merged PR touched. A dropped pending run then loses nothing — the surviving run archives everything complete, including what the dropped run would have covered.

Side benefits:

• A change whose post-merge tasks get checked in a later, unrelated commit is now archived by the next merge (previously it needed a PR touching its directory again).
• A closed-unmerged archive PR no longer strands a completed change — the next merge re-opens it.

The merged PR's API file list is still fetched, but only scopes the remaining-tasks report: incomplete changes the PR didn't touch stay silent, so green-run logs don't fill up with other changes' backlogs. The zero-tasks-parsed warning stays unconditional, since repo-wide detection will archive such a change on any merge.

Verification

• node --test — 63 pass (8 new: listRepoChangeNames, open-PR filtering/pagination/errors, coverage union, prefix pin).
• End-to-end smoke test of main() against a stub GitHub API + stub openspec CLI:
  • complete + uncovered change → archived, changes=done-change;
  • complete + covered change → already covered by open archive PR #99 — skipping, never archived;
  • incomplete PR-touched change → three-channel report, not archived; incomplete untouched change → silent;
  • all complete changes covered → archived=false, openspec never invoked, no PR step.
• YAML validated; no new untrusted-input interpolation in the workflow.

Made with Cursor

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

[github-actions]

Model: kimi-k3

🤖 Kimi PR Review Complete

📋 Initial Review

Summary

This PR fixes #141 (duplicate archive PRs from back-to-back merges) with three mutually reinforcing changes: repo-wide candidate detection via listRepoChangeNames, a coverage check that skips changes already covered by open archive-on-merge/* PRs (fetchOpenArchivePullRequests / fetchCoveredChangeNames), and a per-repository concurrency group in the reusable workflow. The implementation is careful and well-documented — input validation, retry semantics, kebab-case allowlists, and the canonical-slug reasoning all hold up, and the new helpers are unit-tested. The main gaps are that the coverage check trusts open PRs from forks, and the new orchestration in main() (the heart of the fix) has no automated test.

Recommendation: APPROVE (with suggestions — no blockers)

---

🔴 Critical Issues (BLOCK MERGE)

None.

🟠 High Priority Issues

None.

🟡 Medium Priority Issues

🟡 1. Coverage check trusts open PRs from forks — any fork PR can suppress archiving indefinitely — .github/actions/archive_on_merge/archive-on-merge.js:519-528

Details

Severity	File	Lines
MEDIUM	.github/actions/archive_on_merge/archive-on-merge.js	519-528

❌ Problematic Code:

for (const pr of batch) {
  const headRef =
    pr && pr.head && typeof pr.head.ref === "string" ? pr.head.ref : null;
  if (
    headRef &&
    headRef.startsWith(ARCHIVE_BRANCH_PREFIX) &&
    Number.isInteger(pr.number)
  ) {
    pulls.push({ number: pr.number, headRef });
  }
}

📋 Issue: GET /repos/{repo}/pulls?state=open returns PRs from forks too, and the filter only checks pr.head.ref. Anyone who can open a PR against a consumer repo (i.e. anyone, for a public repo) can open a PR from a fork branch named archive-on-merge/<change> whose diff deletes openspec/changes/<change>/…. fetchCoveredChangeNames will then extract <change> from that PR's file list and main() will skip archiving it on every subsequent merge ('…' is already covered by open archive PR #N — skipping), for as long as the junk PR stays open. This is exactly the failure mode the workflow's own if: skip avoided mattering: repo-wide detection heals a skipped run on the next merge, but a covered change is suppressed on every merge.

⚠️ Impact: On a public consumer repo, an external party (or an accidental fork branch with the magic prefix) can silently and persistently disable auto-archiving for a chosen change. The only signal is a log line in a green run, which — as this file's own comments note — is nobody's first stop. Not exploitable for code execution, but it defeats the automation this PR exists to protect.

✅ Fix:

for (const pr of batch) {
  const headRef =
    pr && pr.head && typeof pr.head.ref === "string" ? pr.head.ref : null;
  // Only same-repo PRs count: a fork branch named archive-on-merge/* must not
  // suppress archiving. head.repo is null for deleted forks — treat as foreign.
  const headRepo = pr && pr.head && pr.head.repo;
  const sameRepo =
    headRepo &&
    typeof headRepo.full_name === "string" &&
    headRepo.full_name.toLowerCase() === repo.toLowerCase();
  if (
    headRef &&
    sameRepo &&
    headRef.startsWith(ARCHIVE_BRANCH_PREFIX) &&
    Number.isInteger(pr.number)
  ) {
    pulls.push({ number: pr.number, headRef });
  }
}

plus a test case:

test("fetchOpenArchivePullRequests ignores archive branches on forks", async (t) => {
  const apiUrl = await stubApi(t, () => [
    { number: 5, head: { ref: "archive-on-merge/a", repo: { full_name: "o/r" } } },
    { number: 6, head: { ref: "archive-on-merge/b", repo: { full_name: "evil/r" } } },
    { number: 7, head: { ref: "archive-on-merge/c", repo: null } }, // deleted fork
  ]);
  const pulls = await fetchOpenArchivePullRequests({ apiUrl, repo: "o/r", token: "tok" });
  assert.deepStrictEqual(pulls, [{ number: 5, headRef: "archive-on-merge/a" }]);
});

💡 Explanation: Archive PRs are only ever opened from same-repo branches by the workflow (git push origin HEAD:refs/heads/$BRANCH), so restricting coverage to head.repo.full_name === repo loses nothing and closes the suppression vector. The existing test stub (openPr) will need repo: { full_name: "o/r" } added to stay representative.

🟡 2. No automated test for the new main() orchestration — the core of the #141 fix is verified only by a manual smoke test — .github/actions/archive_on_merge/archive-on-merge.js:799-884

Details

Severity	File	Lines
MEDIUM	.github/actions/archive_on_merge/archive-on-merge.js	799-884

❌ Problematic Code:

let candidates = complete;
if (candidates.length > 0) {
  const covered = await fetchCoveredChangeNames({ apiUrl, repo, token });
  candidates = candidates.filter((name) => {
    const coveringPr = covered.get(name);
    if (coveringPr !== undefined) {
      console.log(
        `'${name}' is already covered by open archive PR #${coveringPr} — skipping.`
      );
      return false;
    }
    return true;
  });
}

📋 Issue: The 8 new tests cover the new helpers (listRepoChangeNames, open-PR filtering/pagination/errors, coverage union, prefix pin), but nothing exercises how main() wires them together: repo-wide candidate pool → completeness census → coverage filter → archived=false and no openspec invocation when everything is covered; and the prTouched.includes(name) gating that keeps incomplete-but-untouched changes silent. The PR description confirms this path was verified with a one-off manual smoke test ("End-to-end smoke test of main() against a stub GitHub API + stub openspec CLI") — that verification evaporates after merge and won't catch a future regression (e.g. someone moving the coverage filter after the archive loop, or flipping the prTouched condition). This repo otherwise holds its logic to a pinned-by-test standard ("Built here rather than inline so its shape is pinned by a test").

⚠️ Impact: A regression in the exact behavior this PR adds — duplicate-PR prevention — would ship green. The scenario matrix from the manual smoke test (covered → skipped, all covered → archived=false and openspec never invoked, incomplete untouched → silent) is precisely what CI should assert.

✅ Fix: Add main()-level tests in archive-on-merge.test.js reusing the existing stubApi/stubRepo helpers, with GITHUB_WORKSPACE pointed at a stub repo, PR_NUMBER/TARGET_REPOSITORY/GITHUB_TOKEN/GITHUB_OUTPUT env stubs, and a stub openspec on PATH (a shell script echoing {"archive":{...}}). Assert at minimum:

// complete + covered change → skip log line, openspec never invoked,
// GITHUB_OUTPUT gets archived=false
// incomplete change the PR did not touch → no report/notice/summary output
// incomplete change the PR touched → report + notice + summary written

💡 Explanation: The helpers are already dependency-injected (apiUrl, ROOT via GITHUB_WORKSPACE), so main() is testable without refactoring; the missing piece is only the env/PATH stubbing. Also consider pinning the MAX_OPEN_PR_PAGES truncation warning (archive-on-merge.js:536-540) the same way the 3000-file warning is pinned by hitting the API's file ceiling warns rather than truncating silently.

🟢 Low Priority Issues

🟢 3. Composite-action callers get no run serialization, so the coverage check alone leaves a duplicate-PR race — .github/actions/archive_on_merge/action.yml:30-46

Details

Severity	File	Lines
LOW	.github/actions/archive_on_merge/action.yml	30-46

❌ Problematic Code:

runs:
  using: composite
  steps:
    - name: Detect + archive completed changes
      id: archive
      shell: bash
      ...
      run: node "${{ github.action_path }}/archive-on-merge.js"

📋 Issue: The duplicate-PR fix is three-layered, but two of the layers — the per-repository concurrency group and the canonical-branch gh pr list backstop — live in the reusable workflow, not the composite action. A caller using this action directly (which README.md still supports with "Prefer the reusable workflow" as soft guidance) who opens archive PRs in their own workflow can still have two runs race: both pass the coverage check before either opens its PR. The coverage check narrows the window but, unlike the reusable workflow, nothing closes it.

⚠️ Impact: Composite-action users can still hit #141-style duplicate archive PRs despite running the fixed script. Low likelihood (the reusable workflow is the documented, preferred path and the action itself never opens PRs), but the gap is currently undocumented.

✅ Fix: Document the requirement where composite users will see it — in action.yml's description and the action README.md usage section:

> **Note:** The action serializes nothing itself. If you use it directly rather
> than via the reusable workflow, wrap the calling job in a per-repository
> concurrency group (`group: archive-on-merge-${{ github.repository }}`,
> `cancel-in-progress: false`) or back-to-back merges can still race the
> coverage check and open duplicate archive PRs.

💡 Explanation: The composite action can't own the concurrency group (that's job-level), so the only fix available at this layer is making the caller's obligation explicit.

---

Summary Table

Priority	Count	Categories
🔴 Critical	0	—
🟠 High	0	—
🟡 Medium	2	Fork-PR trust in coverage check; missing main() test coverage
🟢 Low	1	Undocumented serialization requirement for composite-action use

---

Recommendations

1. Filter the open-PR coverage list to same-repo PRs (pr.head.repo.full_name === repo) so a fork branch named archive-on-merge/* cannot suppress archiving — small change, closes the one real trust hole in the new check.
2. Promote the manual main() smoke test from the PR description into archive-on-merge.test.js so the fix's core behavior is pinned in CI.
3. Note the per-repo concurrency requirement in the composite action's docs for non-workflow callers.

Everything else — repo-wide detection making the per-repo concurrency drop safe, sorted names for canonical slugs, the unconditional zero-tasks-parsed warning, and the retry/timeout handling on the new endpoints — is sound and consistent with the PR description's claims.

---

Auto Fix Claude Reviews

Action	Open Dashboard