Skip to content

Commit d0a46f0

Browse files
mchwangclaude
andauthored
F6 (part): run the pre-merge already-fixed check through GhAlreadyFixedGateway (#89)
* F6 (part): run the pre-merge already-fixed check through GhAlreadyFixedGateway GhMergeGateway's own REST-timeline check counted every same-repository PR that mentioned the issue and returned unknown for any cross-repository reference. The merge now runs the pre-PR check with the same rules (decided 2026-09-30): a cross-reference counts only when it would close the issue; with a non-default base, a PR here into that base that references the issue also counts; manual links, the closed state and new base-branch commits count. This PR stays the task's own PR, excluded only while open. The scan starts at the PR's base commit, and the check stops early enough for its gh processes to settle before the inspection deadline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Close local review round 2 on F6 merge check: run it alongside the rule reads - Start the already-fixed check as soon as the PR is read, in parallel with the rule reads; stop and await it if those reads fail. A fresh inspection fell from about 3.4 s to 2.1 s on cli/cli, leaving the check its time. - Move CHECK_SETTLE_MS next to the grace periods it is built from. - Require an injected check to name the merge repository. - Show no already-fixed blocker once this PR has merged. - Blocker text: "refers to it" also covers a reference into a non-default base. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Close local review round 3 on F6 merge check: say why it blocks - Carry the check's matches (closes, PRs, short commit SHAs; at most five, never a commit message) or its unknown reason into the merge blocker. A long-lived PR whose base is more than 250 commits behind now says so. - Validate the merge repository with the shared REPOSITORY pattern. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Close local review round 4 on F6 merge check: share the merge-read fake in tests Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Close Copilot review round 6 on F6 merge check: shorten a closing commit's SHA A close by a commit reached the blocker as "commit <40 hex>", against the documented short-SHA detail. describeMatches now shortens a full SHA in the close description to 12 characters. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Close Copilot review round 7 on F6 merge check: refuse answers after an abort Copilot (summary only) noted that a check answer arriving after the early stop was still used, and that rule reads answering after an abort still produced a state. Reproduced both with fake-timer tests. A stopped check is now unknown whatever it answers, and the inspection rejects with the abort's reason once its reads end. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Close Copilot review round 8 on F6 merge check: no late start, exact SHA shortening Copilot (summary only) found two cases, both reproduced with tests: - A PR read that ended past the stop point still started the check, whose processes could then settle after the inspection deadline. With no settle time left, the check is now not started and is unknown. - The closer SHA shortening matched any 40-hex token, so a repository name of 40 hex characters was cut. Only the "commit <sha>" form is shortened. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Close Copilot review round 10 on F6 merge check: require the base head A clear or found answer without a valid baseHead was accepted, though clear removes the merge blocker. It now counts as unknown ("The check returned an invalid result."). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Close Copilot review round 12 on F6 merge check: keep the first abort reason 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. It now rethrows the combined signal's reason, which is whichever abort came first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
1 parent f4b8336 commit d0a46f0

5 files changed

Lines changed: 402 additions & 195 deletions

File tree

‎docs/implementation/guarded-merge.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,16 @@ Derived planted-review configurations clear the source GitHub binding because th
88

99
Merging blocks when any plan item is unreviewed or stale, an attributed file is outside that item's declared plan scope, an Ambiguous or Unplanned segment remains, a current change request is open, or a `cmd:` acceptance check lacks a passing result for the current head. Approval does not override an out-of-scope file; the plan must be amended. Historical change requests from an earlier revision or snapshot remain visible but do not block the current revision.
1010

11-
The GitHub adapter reads the current PR base/head, effective branch rulesets, classic branch protection, required check contexts and app identities, and issue cross-references. It ignores review records that are not required checks. Cross-referenced PRs are read in one GraphQL request; repository identity is preserved with each PR number, and cross-repository references fail closed. More than 100 references also fail closed as unknown. Malformed pagination or PR references, partial referenced-PR records, rules without a type, malformed protection objects, missing or malformed check-app identities, and inconsistent check status/conclusion pairs fail closed, while an explicit `null` remains an unbound check. Display status is cached for five seconds and the combined inspection has a 12-second deadline. Each of the two sequential merge-validation inspections has a six-second deadline, keeping their combined validation budget below the 15-second serving request budget. A deadline aborts the underlying GitHub CLI process before releasing the shared in-flight request. An unreadable or incomplete rule source, a pending/missing/failing check, another open or merged PR for the issue, a conflict, or a closed PR blocks merging.
11+
The GitHub adapter reads the current PR base/head, effective branch rulesets, classic branch protection, required check contexts and app identities, and issue cross-references. It ignores review records that are not required checks. The already-fixed check is the pre-PR check, `GhAlreadyFixedGateway` in `github/already-fixed.ts`, with the same rules (see “The check” in `pull-request-opening.md`): a closed issue, another open or merged PR that would close the issue or is linked manually, or a new commit on the base branch that mentions the issue. A PR that only mentions the issue does not count, in this repository or another; when the PR's base is not the default branch, a PR in this repository into that same base that references the issue counts too (decided 2026-09-30). The merge passes this PR as the task's own PR, matched by repository and number: it is excluded while open and counts once merged. Unlike the pre-PR check, the merge does not know the task's earlier PRs, so another open PR of the same task still counts. The commit scan starts at the PR's base commit on its base branch. Merging requires that commit to equal the reviewed snapshot's base, which is the base the pre-PR check scanned from. The merge passes no own commits, because the task's commits reach the base branch only by this merge. Any result the check cannot complete is unknown and blocks merging, including a base branch name outside the pattern the check accepts. The check starts as soon as the PR is read, alongside the rule reads, and stops 0.65 seconds before the inspection's deadline (`MERGE_CHECK_SETTLE_MS` in `github/merge.ts`: both grace periods of the merge's `gh` runner, which the check also uses, plus a margin), so its processes settle before the inspection ends; a check stopped that way is unknown. If the rule reads fail, the check is stopped and awaited before the inspection rejects. When the PR read ends too late to leave that much time, the check is not started and is unknown. A check answer that arrives after the check was stopped is unknown, and an inspection whose caller aborted or whose deadline passed rejects even when its reads answer late. An injected check must name the merge repository. The blocker names what the check found (at most five closes, PRs and commits, by repository, number, state and short SHA, with a closing commit's SHA shortened too; never a commit message) or why it could not finish. Once this PR has merged, the check counts it as the fix, so the coordinator shows only the `pr-state` blocker, not an already-fixed one. Malformed pagination, rules without a type, malformed protection objects, missing or malformed check-app identities, and inconsistent check status/conclusion pairs fail closed, while an explicit `null` remains an unbound check. Display status is cached for five seconds and the combined inspection has a 12-second deadline. Each of the two sequential merge-validation inspections has a six-second deadline, keeping their combined validation budget below the 15-second serving request budget. A deadline aborts the underlying GitHub CLI process before releasing the shared in-flight request. A caller's own abort during the already-fixed check waits for its processes like any other `gh` call of the gateway, up to 0.4 seconds. An unreadable or incomplete rule source, a pending/missing/failing check, an already-fixed match or unknown result, a conflict, or a closed PR blocks merging.
1212

1313
Automatic merging also requires strict required status checks as a server-enforced current-base policy. A client-side fetch cannot supply that guarantee. Merge-queue branches were blocked in this increment because `gh pr merge` can report a successful enqueue before the pull request is merged. Queue support has since landed (#24; see `merge-queue.md`): a queue-capable adapter tracks each attempt until GitHub confirms the merge, removal or failure, and queue rules count as the server-side current-base guard. An adapter without queue inspection still fails closed with a `merge-queue` blocker. The coordinator performs the complete gate twice without using the display cache, verifies the same base/head pair, then re-reads the local review generation immediately before invoking `gh pr merge --match-head-commit` with literal argv. Each `gh` process gets only the allowlisted variables in `github/gh-env.ts`, plus fixed settings that turn off prompts, the pager, colour and update checks. No other server variables are passed on. A timeout or abort sends the `gh` process SIGTERM, then SIGKILL after a quarter of a second, and the call returns only after that process has exited (or 0.15 s after it exits, if a process it started keeps its output open). So a merge click aborted at its 14-second deadline settles by 14.4 s, inside the 14.5-second shutdown drain and below the 15-second request budget. Stopping `gh` cannot recall a merge request it had already sent: GitHub can still apply it, so a cancelled merge is reported as unknown and its attempt stays in flight, with the action disabled. A direct attempt is settled only when GitHub shows the pull request merged; a queued attempt also settles when GitHub reports it removed or failed (`merge-queue.md`). The adapter invalidates its pre-action cache before the command and after either success or refusal. A generation guard prevents inspections started before or during the command from restoring stale cache entries afterward. After command success, the server returns “merge submitted” even if its best-effort status reload fails, and the browser keeps the action disabled until Refresh confirms GitHub state. GitHub's command refusal text is returned to the local UI, and a failed attempt also remains disabled until Refresh loads fresh state. There is no “merge anyway” path around missing atomic protection. Demo mode never constructs the coordinator, including when a gateway is injected.
1414

1515
Shutdown sets a terminal admission flag before inspecting active work and checks it again after partial request bodies are received. It then stops HTTP admission, aborts and awaits an active merge CLI process, drains requests, and closes the review service. A request admitted before shutdown cannot start a new merge afterward, and a cancelled command cannot outlive the local state that authorized it.
1616

1717
## Deferred runner work
1818

19-
This slice does not mutate Git history or run plan commands. If the base moved, the gate sends the task back to review. A plan item with a `cmd:` acceptance check remains blocked because the current review service has no trusted container result. Issue #22 tracks rebasing through the ledger, recomputing review state, running commands in the container, and persisting results against the exact head.
19+
This slice does not mutate Git history or run plan commands. If the base moved, the gate sends the task back to review. A plan item with a `cmd:` acceptance check remains blocked because the current review service has no trusted container result. Issue #22 tracks rebasing through the ledger, recomputing review state, running commands in the container, and persisting results against the exact head. The already-fixed commit scan starts at the PR's current base commit; once a rebase moves that base, the scan must still cover the base-branch commits the rebase brought under the PR since the last check.
2020

2121
## Validation
2222

23-
Unit and integration tests cover local review blockers; current and stale base/head pairs; PR state and mergeability; known, unknown, empty, pending, and failing check requirements; app-bound and malformed app identities; server base protection; merge-queue refusal for adapters without queue inspection; runtime merge-method validation; already-fixed results; a requirement changing between validation passes; deadline cancellation; post-action cache invalidation; and the exact `gh` merge argv. Browser coverage proves blockers are visible, the ready action uses the reviewed head, and duplicate clicks start only one merge.
23+
Unit and integration tests cover local review blockers; current and stale base/head pairs; PR state and mergeability; known, unknown, empty, pending, and failing check requirements; app-bound and malformed app identities; server base protection; merge-queue refusal for adapters without queue inspection; runtime merge-method validation; the already-fixed check run through `GhAlreadyFixedGateway` (closing-only cross-references, the non-default-base rule, manual links, the closed state, base-branch commits, this PR excluded only while open, the merge inputs, fail-closed results and caller cancellation); a requirement changing between validation passes; deadline cancellation; post-action cache invalidation; and the exact `gh` merge argv. Browser coverage proves blockers are visible, the ready action uses the reviewed head, and duplicate clicks start only one merge.

‎docs/implementation/pull-request-opening.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ The check matches when any of these is true:
3333
| Another open or merged PR links to the issue. | Cross-reference events that would close the issue (`willCloseTarget`: a closing keyword such as `Fixes #12`). GitHub closes issues only from PRs into the default branch, so `willCloseTarget` is false for every PR into another branch; when the task's base is not the default branch, a PR in this repository into that same base that references the issue counts too (decided 2026-09-30). Manual links: "connected" and "disconnected" events replayed in order. Both sides of a manual link are read, because which side GitHub reports as the subject depends on where the link was made; the linked PR is the side that is a PR, and a link between two PRs or to an unknown type makes the check `unknown`. | The task's own open PRs, matched by repository and number (its own merged PR is a match). Closed, unmerged PRs. A manual link whose latest event is a disconnect. A PR that only mentions the issue, in this repository or another (decided 2026-09-30: on cli/cli a third of open issues had such mentions, mostly merged PRs in unrelated repositories). |
3434
| A new commit on the base branch mentions the issue. | The commits from the task's base to the current base branch head. | Own commits. `#123` when the issue is `#12`. `other/repo#12`. A token inside a URL path or a longer path-like token (`https://example.com/GH-12`, `mirror/owner/repo#12`). |
3535

36+
The pre-merge check (`GhMergeGateway`, see `guarded-merge.md`) runs this same check, with the task's PR as its only own PR and the PR's base commit as the task base.
37+
3638
A commit mentions the issue with `#12`, `GH-12`, `owner/repo#12`, or the issue URL. A PR in another repository that would close the issue, or is linked manually, counts as a match. It is not excluded by number, because its number belongs to another repository. GitHub turns `willCloseTarget` false once the issue is closed, so a merged PR that closed the issue is reported through the closed state instead.
3739

3840
An abandoned opening's PR has no recorded number, so the check cannot exclude it by number. The main path looks the branch up first and adds a visible PR's number to the own PRs. If GitHub's PR list still does not show that PR after the 10-minute settle time but the issue timeline already does, the check counts the task's own PR as another PR, and the task moves to possibly already fixed. That fails closed: a person sees the PR and continues.
@@ -123,7 +125,6 @@ Some repositories do not support draft PRs (for example private repositories on
123125
- **Push.** `BranchPusher` is injected. The real push needs the runner's commits in a runner-owned host repository, fetched from the bundles `commitTaskChanges` returns (#66, #87).
124126
- **Continue from possibly already fixed.** The Continue and Cancel actions are user actions for a later slice.
125127
- **Close the draft on cancel.** The design closes the draft PR when a person cancels a needs-human task. The PR record is kept for that.
126-
- **The pre-merge check.** `GhMergeGateway` keeps its own check for now. F6 moves it onto this module. The merge check treats a PR in another repository as `unknown`; this check treats it as a match.
127128

128129
## Tests
129130

0 commit comments

Comments
 (0)