Skip to content

Commit 49f03db

Browse files
mchwangclaude
andauthored
Keep merge stop reasons and GitHub answers through every layer (#171)
Part of #48 (server part). Keeps the first stop reason and gh's own answers through the merge gateways, coordinator and server; answers timeouts before admission as resendable 503s; keeps GitHub's answer after gh exits; answers queue polls from the current attempt; never offers an action from a merge click's pre-command readiness. Also fixes an unhandled rejection in the merge-state inspection, adds two AGENTS.md review lessons, and files #174 as a follow-up. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent d4fe387 commit 49f03db

20 files changed

Lines changed: 1335 additions & 69 deletions

‎AGENTS.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ For features with background jobs, polling, retries, cancellation, or shutdown:
2121
- Back off recurring external-status polling to a bounded cap. Reset the interval only after a meaningful lifecycle change or explicit user action.
2222
- When synchronous work moves behind an await (a worker thread, a subprocess, an async API), list every await it adds and apply the local-generation rule under "Guarded external actions" to each one. A local await must not follow the final remote validation: the last await before an irreversible external action is that validation.
2323
- Work that leaves a serializing path (the event loop, a lock, a single queue) needs its own concurrency bound. Queued work must honour cancellation and deadlines.
24+
- Attach a rejection handler to a promise when it is created whenever anything is awaited before the promise is consumed. A rejection that settles in between reaches Node's process-wide unhandled-rejection handler, which by default exits the process.
2425
- Never compare clock readings across threads or processes. Pass a relative budget and let each side count it on its own monotonic clock.
2526
- A test that runs the server in the test's own process must wait for observable state before acting. Do not rely on the server blocking the test's event loop to order steps.
2627

@@ -50,6 +51,8 @@ Before opening or updating a PR for asynchronous behavior, test every applicable
5051

5152
Every reproduced race requires a failing-before and passing-after regression. Assert both the visible result and the durable state when they can diverge.
5253

54+
A race regression must assert that the interleaving it names actually happened (for example which abort stopped which stage, or that a deadline fired before a release) before it uses the outcome as evidence. Otherwise machine load can turn it into a test of a different order that still passes.
55+
5356
## Review readiness
5457

5558
- Before requesting or re-requesting an automated Copilot review, self-review the full current diff, fix every issue found, and repeat the self-review and fix cycle until a complete pass finds no new issues. Re-run the relevant validation after fixes, then complete the mandatory independent-agent review loop below before requesting Copilot review.

‎docs/implementation/guarded-merge.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,9 +29,9 @@ Merging blocks when any plan item is unreviewed or stale, an attributed file is
2929

3030
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, and every other PR the task records in this repository (#121), as the task's own PRs, matched by repository and number: each is excluded while open and counts once merged, as in the pre-PR check. Without a runner block only this PR is passed. 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.
3131

32-
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.
32+
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 call aborted at its 14-second deadline settles by 14.4 s. *Later (#48, #170):* a click can also be waiting on its issue-trust read, whose stop wait is 0.75 s, or on a review load, whose history worker gets 1 s after the abort before it is terminated; so it settles about 0.75 s after its deadline, or about 1 s or a little more after it when a review load was running, past the 14.5-second shutdown drain (traced, not tested); see [merge-queue.md](merge-queue.md). 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.
3333

34-
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.
34+
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, drains admitted requests, closes the pre-merge lifecycle and the Store write gate, and only then aborts and awaits an active merge CLI process (`web/server.ts`), so a refusal gh gave before the click was admitted, if it is handled only after the write gate closed, cannot be saved and answers a resendable 503 with the refusal's text; after admission, settlement keeps its shutdown capability and saves GitHub's refusal (#48). A request admitted before shutdown cannot start a new merge afterward, and a cancelled command cannot outlive the local state that authorized it.
3535

3636
## Deferred runner work
3737

0 commit comments

Comments
 (0)