F6 (part): run the pre-merge already-fixed check through GhAlreadyFixedGateway

[mchwang]

Lane F, part of step F6 (design: "Checking whether the issue is already fixed"). The pre-merge already-fixed check now follows the rules decided on 2026-09-30 for the pre-PR check (#84), by running that check itself.

What changes

• github/merge.ts: GhMergeGateway no longer reads the REST issue timeline. That check counted every same-repository PR that mentioned the issue and returned unknown for any cross-repository reference. It now calls an AlreadyFixedGateway: by default GhAlreadyFixedGateway for the same repository, using the merge gateway's own runner (since Give the merge and issue gh runners the allowlisted environment #86: the allowlisted environment, with 0.25 s and 0.15 s stop waits). The rules are the pre-PR check's:
  • A cross-reference counts only when it would close the issue (willCloseTarget). A PR that only mentions the issue does not count, in this repository or another. A closing PR in another repository is a match, no longer unknown.
  • When the PR's base is not the default branch, a PR in this repository into that same base that references the issue also counts.
  • Manual links (both sides, connect and disconnect replayed in order), the closed state, and new base-branch commits count. The old merge check read none of these.
• The merge-specific exclusion is kept. This PR is passed as the task's own PR, matched by repository and number: excluded while open, a match once merged (AGENTS.md: exclude the subject only in the states the exclusion is for).
• Inputs. The base branch is the PR's baseRefName. The commit scan starts at the PR's baseRefOid: merging requires that commit to equal the reviewed snapshot's base, which the pre-PR check also scanned from. No own commits are passed, because the task's commits reach the base branch only through this merge.
• Timing. The check starts as soon as the PR is read, alongside the rule reads, rather than after them. A fresh inspection on cli/cli fell from about 3.4 s to 2.1 s. The check stops MERGE_CHECK_SETTLE_MS (0.65 s: both of the merge runner's grace periods plus 250 ms, defined in github/merge.ts) before the inspection deadline, so its processes settle inside the 6-second and 12-second inspection budgets. A caller's abort during the check settles within the merge runner's 0.4 s, as Give the merge and issue gh runners the allowlisted environment #86 requires for every gh call of the gateway. A check stopped that way is unknown. If the rule reads fail, the check is stopped and awaited before the inspection rejects. A caller's own abort still propagates as the caller's reason.
• Fails closed. An outcome other than clear or found, or any error, is unknown. An injected check must name the merge repository; one that names another repository, or none, is refused at construction. The merge repository is now validated with the shared REPOSITORY pattern.
• Scope. The only GraphQL query is the pre-PR check's. It reads nothing on ProjectV2 except __typename, so the repo scope is enough.
• runner/merge.ts: the blocker now says why it blocks. A match names what was found: at most five closes, PRs and commits, by repository, number, state and short SHA, never a commit message. An unknown result gives the check's reason, for example a base more than 250 commits behind. Once this PR has merged, only the pr-state blocker is shown: the check would otherwise count the merged PR itself as the fix.
• Docs: docs/implementation/guarded-merge.md describes the new check, its inputs, its deadline and its known gaps. The "pre-merge check" item is removed from the "does not do" list in pull-request-opening.md.

Not in this PR

• Earlier PRs of the task. The merge gateway knows only the configured PR number, so another still-open PR of the same task still blocks merging. The old check did the same. The pre-PR check excludes these PRs using the Store's PR records.

• Base branch names. A name outside the BRANCH pattern in github/validate.ts makes the check unknown, which blocks merging; it fails closed.

• Rebase (Add pre-merge rebase and head-bound command checks #22). Once a rebase moves the PR's base, the scan must still cover the commits the rebase brought under the PR. This is recorded in the deferred section of guarded-merge.md.

• Inspection timeout. The merge-queue reads (queueWatermark, inspectQueue) still check the caller's signal before the deadline's, so a deadline that fires first can be reported as the caller's abort. This code is unchanged here.

Evidence

Final head d7bc309, after merging main at aafd153 (#86):

• npx tsc --noEmit -p .: clean.
• test/merge.test.ts, test/already-fixed.test.ts, test/publish.test.ts, test/runner-shutdown.test.ts, test/gh-env.test.ts: 363 passed.
• Full local suite: 1351 passed, 27 failed, 2 skipped (1380), on a machine with a load average near 200 from other sessions. None of the failures is in a changed module. 18 were timeouts and the rest Docker contention (a network pool overlapping another container's). Every failing non-Docker file then passed in full when re-run: test/history.test.ts, test/review.test.ts, test/runner-planning-feedback.test.ts, test/runner-shutdown.test.ts and test/store.test.ts (115 of 115). Of the 9 real-Docker failures, two are the supervisor tests that fail on main without this branch, and four passed when re-run. The other three failed again under the same load; on main (f4b8336), "refuses to seed a repository with a chain of in-checkout links that ends outside it, and leaves no storage behind" fails the same way, and the other two ("refuses to seed a repository with a chain that leaves through a target this host lacks…", "returns the adapter handle at once and cancels it during network creation") passed there, so they depend on load. CI (test) passed on this head.
• Old code against the new tests. With the old github/merge.ts, 8 of the new tests fail.
• Mutation checks. Each mutation below was applied alone, and at least one test failed each time:
  • no own-PR exclusion;
  • a wrong scan start;
  • the base branch fixed to main;
  • a swallowed caller abort;
  • a bogus outcome passed through;
  • no repository guard;
  • no early stop before the deadline;
  • the check given a runner other than the merge gateway's (before Give the merge and issue gh runners the allowlisted environment #86: the merge's execFile runner; after: the check's own slower-to-stop runner);
  • the check started after the rule reads instead of alongside them;
  • the check not stopped, or not awaited, when the rule reads fail;
  • an injected check without repository accepted;
  • the already-fixed blocker shown for a merged PR;
  • the found detail, the unknown reason, the five-match cap, the time-out reason, or the coordinator's use of the detail dropped;
  • a closing commit's SHA left at full length;
  • a late check answer used after the early stop, or a state returned after the caller aborted;
  • a repository name of 40 hex characters shortened, or the check started with no settle time left (both found by Copilot and failing before the fix);
  • a clear answer without a valid base head accepted (found by Copilot, failing before the fix);
  • the caller's abort reason replaced by the inspection timeout (found by Copilot, failing before the fix).
• Live, read-only. GhMergeGateway.inspect ran against an open cli/cli PR and five cli/cli issues (two closed, three open). There was no scope error; the closed issues were found and the open ones clear. The check read the PR's base branch trunk and scanned from its baseRefOid. Nothing was written to GitHub.

Review rounds

Round	Source	Head	Findings	Fixed in
1	Independent review agent (fresh context, contract checklist)	uncommitted	10. Medium: the check's SIGTERM-to-SIGKILL and pipe grace could push an inspection about 1.5 s past its deadline. Low: only this PR is excluded, not the task's earlier PRs; the default runner wiring was untested; new blockers for unusual branch names and after this PR merges; ProjectV2 runtime scope not proven live; same-tick deadline resolution; no test or detail for the blocker text; an injected check without repository skips the repository guard; one doc sentence's reasoning; deleted REST tests.	bd070d4 (deadline, runner wiring test, docs)
2	Local /code-review, high effort	bd070d4	5. The check started only after four sequential reads, leaving it about 2 s of a fresh 6 s inspection (measured live); the blocker text did not cover a reference into a non-default base; an injected check without repository skipped the repository guard; the settle time was computed in merge.ts from the check's constants; a merged PR showed an already-fixed blocker.	76c271b
3	Local /code-review, high effort	76c271b	2: the check's reason and matches were dropped before the blocker, so a PR whose base is more than 250 commits behind only said "could not be completed"; the repository pattern duplicated REPOSITORY.	93e77b7
4	Local /code-review, high effort	93e77b7	1: the merge-read fake was copied into five tests.	d391305
5	Local /code-review, high effort	d391305	None.	—
6	Copilot	d391305	1: a close by a commit reached the blocker with the full 40-character SHA, against the documented short-SHA detail.	164f690
7	Copilot (summary only, no inline thread)	164f690	1: an answer that arrived after the check's early stop was still used, and rule reads that answered after an abort still produced a state (merge.ts lines 140 and 162). Reproduced with a fake-timer test.	2eae9d2
8	Copilot (summary only, no inline threads)	2eae9d2	2: the closer SHA shortening also cut a repository name of 40 hex characters; a PR read that ended past the stop point still started the check, whose processes could settle after the deadline. Both reproduced with tests.	8742c6d
9	Local /code-review, high effort	8742c6d	None.	—
10	Copilot	8742c6d	1: a clear or found answer without a valid baseHead was accepted, though clear removes the blocker.	a3ac0b8
11	Copilot	a3ac0b8	None ("Approval recommended").	—
—	Merge of main (#86)	fb80731	Conflict in github/merge.ts. #86 moved the merge's gh calls to the allowlisted runner with 0.25 s and 0.15 s stop waits, and documents that an aborted merge click settles by 14.4 s. The check's own runner waits 1.5 s, so it now uses the merge runner, and the early stop is built from the merge's grace periods. A test fails if the check gets another runner.	fb80731
12	Copilot	fb80731	1: when the caller aborted and the inspection deadline also passed before a stopped gh call settled, inspect() reported the timeout instead of the caller's reason (pre-existing code, reachable now that calls wait for gh to stop).	d7bc309
13	Copilot	d7bc309	None ("Approval recommended").	—

Review-lesson audit

Finding	Classification
Grace periods could overrun the inspection deadline	Covered: AGENTS.md, Guarded external actions, "Count the subprocess shutdown grace periods … inside that overall deadline." Fixed with an early stop (now MERGE_CHECK_SETTLE_MS) and a fake-timer test.
Default runner wiring untested	Covered: AGENTS.md, Review readiness, "A test's setup must leave the state the production path would." Fixed with a test that the default check has its own runner.
Only this PR is excluded	One-off: the merge gateway has no Store access. The old check behaved the same way. Recorded under "Not in this PR".
Unusual base branch names and post-merge blocker	One-off: both fail closed. The branch-name case is documented. The post-merge blocker was removed in round 2.
ProjectV2 runtime scope	One-off: the query matches #84's, which was verified with a default repo-scope token. A refusal would make the check unknown, so it fails closed.
Same-tick deadline resolution	Declined: this existed before the change for every read. status() re-checks the caller signal afterwards.
Blocker text: no test, no detail (rounds 1–3)	One-off: fixed in rounds 2 and 3. The blocker names the matches or the reason, and tests assert the text.
Injected check without repository (rounds 1 and 2)	Declined in round 1 (it matched the publisher's guard), then fixed in round 2: the merge now requires the name.
Doc reasoning for the scan start	One-off: fixed in guarded-merge.md.
Deleted REST tests	One-off: the REST timeline is gone. The equivalent malformed-input cases are covered in test/already-fixed.test.ts, and fail-closed cases through the merge are in test/merge.test.ts.
The check ran after the sequential rule reads (round 2)	Covered: AGENTS.md, Guarded external actions, "Budget a multi-stage validation across all sequential stages." Fixed by running the check alongside the rule reads.
Settle time computed outside the check (round 2)	One-off: the settle time moved next to the grace periods it is built from. After #86 those are the merge runner's, so it is MERGE_CHECK_SETTLE_MS in github/merge.ts.
Repository pattern duplicated (round 3)	One-off: github/validate.ts exists for this; the merge now uses REPOSITORY.
Test fake copied into five tests (round 4)	One-off: shared as mergeReads.
Full SHA in a commit close (round 6)	One-off: the formatter shortened commit matches but not the close description; it now shortens both.
Late answers after an abort (round 7)	Covered: AGENTS.md, Async jobs and polling, "Never apply a background response without proving it is still current." Reproduced and fixed; answered in a PR comment.
SHA shortening too broad (round 8)	One-off: a fix from round 6 matched more than the text it was for; it now matches only the commit <sha> form.
Check started with no settle time left (round 8)	Covered: AGENTS.md, Guarded external actions, "Count the subprocess shutdown grace periods … inside that overall deadline." The round-1 fix bounded the stop, not the start.
Clear answer without its base head (round 10)	Covered: AGENTS.md, Guarded external actions, "For safety-critical API responses, require and validate every requested field before any early return, including terminal-success paths."
Caller reason replaced by the timeout (round 12)	Covered: AGENTS.md, Async jobs and polling, "Preserve the original timeout, cancellation, and shutdown reason through every layer."

No new AGENTS.md rule is needed.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

Closing-commit details expose the full SHA despite the documented short-SHA guarantee.

Review effort: Balanced
Findings: 1

Open (1)

• Shorten closing commit SHA in blocker details · New

What changed in this PR

Routes pre-merge already-fixed checks through the shared GhAlreadyFixedGateway.

Changes:

• Reuses shared already-fixed detection and deadline handling.
• Improves blocker details and merged-PR behavior.
• Expands tests and implementation documentation.

File	Description
github/​merge.ts	Integrates the shared check and formats results.
github/​already-fixed.ts	Exports the settlement budget.
runner/​merge.ts	Provides detailed blockers and suppresses redundant merged-state blockers.
test/​merge.test.ts	Covers integration, timing, cancellation, and result details.
docs/​implementation/​pull-request-opening.md	Documents pre-merge reuse.
docs/​implementation/​guarded-merge.md	Documents behavior, deadlines, and limitations.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Two cancellation races can accept late results or swallow caller aborts.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Shorten closing commit SHA in blocker details

Previously missed (1)

In code that hasn't changed since last review

Recheck abort signals before accepting delayed check responses

github/​merge.ts:140

The early-stop signal is only consulted in the rejection path. If a check resolves after observing or ignoring that abort, its clear/found response is still accepted, contradicting the requirement that a check stopped at CHECK_SETTLE_MS becomes unknown. Re-check both cancellation sources immediately after the await before applying the response.

This issue also appears on line 162 of the same file.

[mchwang]

Copilot review on 164f690 (summary only, no inline thread): "Recheck abort signals before accepting delayed check responses" (github/merge.ts lines 140 and 162).

Reproduced and fixed in 2eae9d2. The new fake-timer test "does not accept a check answer or rule reads that arrive after an abort" failed before the fix. It covers:

• a check that ignores its early stop and answers clear: the result is now unknown, "The check did not finish in time.";
• a caller abort followed by a late clear: the inspection now rejects with the caller's reason;
• rule reads that answer after the caller aborted: the inspection now rejects instead of returning a state with rulesKnown: false.

The caller's abort is checked once, after both the check and the rule reads have ended (signal?.throwIfAborted() in #inspectNow). A second check inside #alreadyFixed was tried and removed, because no test could fail without it: the inspection-level check already covers that case.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Late-started checks can exceed the inspection deadline, and valid hexadecimal repository names can be misreported.

Review effort: Balanced
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Only shorten SHAs in commit closer descriptions

github/​merge.ts:69

This regex shortens any standalone 40-hex token in the whole close description, including a valid owner or repository name (for example owner/aaaaaaaa…#9), so the blocker can report the wrong repository. Limit shortening to the commit <sha> form produced for commit closers.

Avoid starting checks when no settlement window remains

github/​merge.ts:136

If the initial pr view consumes the budget past deadlineAt - CHECK_SETTLE_MS, this clamps the delay to zero but still starts the already-fixed check before the timer callback runs. Its subprocesses can then require the full kill/pipe grace period and settle after the inspection deadline, contradicting the bounded inspection contract. Return unknown without starting the check when no settlement window remains; add a regression where the PR read finishes after that cutoff.

[mchwang]

Copilot review on 2eae9d2 (summary only, no inline threads). Both findings were reproduced and fixed in 8742c6d:

• "Only shorten SHAs in commit closer descriptions" (github/merge.ts:69). The new test "shortens only a closing commit SHA, not a repository name that looks like one" (a closer in owner/aaaa… with 40 as) failed before the fix. Only the commit <sha> form at the start of the close description is shortened now.
• "Avoid starting checks when no settlement window remains" (github/merge.ts:136). The new fake-timer test "does not start the merge check when no time is left for its processes to settle", in which the PR read ends 10 ms past the stop point, failed before the fix. With no settle time left, the check is not started and is unknown: "No time was left to run the check."

[Copilot]

Copilot review overview

🟡 Changes recommended

A malformed injected clear result can bypass the already-fixed blocker without its required base SHA.

Review effort: Balanced
Findings: 1

Open (1)

• Validate base SHA before accepting clear outcome · New

[Copilot]

Copilot review overview

🟢 Approval recommended

The implementation fails closed, preserves cancellation semantics, and includes comprehensive regression coverage.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Validate base SHA before accepting clear outcome

[Copilot]

Copilot review overview

🟡 Changes recommended

Caller cancellation can incorrectly be replaced by an inspection timeout when both signals fire before process settlement.

Review effort: Balanced
Findings: 1

Open (1)

• Preserve caller cancellation reason when inspection timeout also fires · New

[Copilot]

Copilot review overview

🟢 Approval recommended

The implementation fails closed, preserves cancellation semantics, and has focused regression coverage for the new behavior.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Preserve caller cancellation reason when inspection timeout also fires