You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Keep merge stop reasons and GitHub answers through every layer (#48) - #171
Part of #48 (server part; #48 stays open until the browser fixes land). The two browser polling defects will follow in a separate PR stacked on F6b (#161), because F6b rewrites scheduleMergePoll / pollMergeQueue in web/public/app.js.
Reproduce Copilot's summary-only concerns on PR #46 about the merge lifecycle before changing code, and record evidence for any that cannot be reproduced.
Findings
Eight defects were reproduced. Cache invalidation was found correct.
#
Defect
Fix
1
Gateway reads (queue watermark, queue inspection, merge-state inspection, the issue gateway's bounded read) reported a later caller abort instead of their own earlier timeout, and replaced a failure gh had already reported
Keep the first abort reason; pass gh's own exit (CommandFailed, ghAnswered()) through
2
The coordinator (#merge, #pollQueue) and readIssueAccess replaced a gateway's first stop reason with their own later abort
Keep a timeout (timedOut()) or gh answer that came first (AGENTS.md: "Preserve the original timeout, cancellation, and shutdown reason through every layer")
3
A timeout before admission was saved as the click's definite refusal
RemoteTimeout (github/timeout.ts); answered as a resendable 503 and never saved; an outcome saved meanwhile under the same click is still replayed; the runner-action trust gate does the same
4
An abort or gh's own timeout that fired after gh's exit replaced GitHub's answer; the gateway labelled a refusal unknown
runWithInput ignores stop requests after exit; a refusal counts when gh answered
5
A failed queue inspection answered with the superseded attempt
Answer from the current attempt; attach the error only to the attempt it read
6
The merge click's answer reported pre-command readiness after a submission
MergeCoordinator.statusAfterMerge, sharing #withAttempt with status()
7–8
Browser: duplicate poll chains let an old poll's error or answer overwrite newer state
Separate PR on F6b
—
Cache invalidation
Correct; test/merge-status-cache.test.ts is evidence
The rules are written up in docs/implementation/merge-queue.md, "Stop reasons and answers (#48)", including what is traced but not tested.
Also corrected: two older sentences in runner-lifecycle.md said a planning GitHub read failure (502) is not recorded; the code saves it and test/planning-production.test.ts checks the replay.
Regression tests for the parallel merge-state inspection stages (test/merge-stop-reasons.test.ts, parallelStages()): a stage's gh exit stops the others, and the inspection waits for every stage and reports that exit, also when its deadline fires while a stopped rule read settles.
Older bug found by those tests, fixed:#inspectNow attached the already-fixed check's rejection handler only after the other stages settled. A check that a failed stage stopped, and that settled first, raised a process-wide unhandled rejection. Node's default is to exit on that. The handler is now attached when the check starts.
After a caller's abort, the already-fixed check's own earlier gh failure stays an unknown blocker. The inspection discards it for the abort, so the click can be resent. Before, it was saved as the click's refusal with raw gh text.
A click's saved outcome is always checked before answering a stop, so an outcome another coordinator saved meanwhile is the answer even after a timeout and this click's own abort (6a6da0f).
The runner-action trust gate keeps a trust read's own timeout through shutdown, and still returns an outcome saved meanwhile; tests for a click aborted before its replay and for output past maxBuffer while the pipes drain (c3a5b3d).
Merge preparation and publish admission: a trust read that timed out, or that gh answered, before shutdown is saved or recorded as failed with that answer, not as the shutdown (tests in ebf520e).
A merge click's answer never offers an action from its pre-command GitHub readiness: if a queue poll ended the attempt while the server reloaded the review, it asks for a refresh (0056e48, e967af3; regression test).
The issue board's trust toggle, planning issue reads, and the already-fixed check's timeout blocker keep their current behaviour (documented).
Tests
New: test/merge-stop-reasons.test.ts, test/merge-queue-poll.test.ts, test/merge-status-cache.test.ts. Changed: test/runner-start.test.ts, test/pre-merge.test.ts and test/runner-publishing.test.ts, plus a comment in test/gh-env.test.ts. Each fix was break-checked: reverting it makes its test fail. The timing-dependent tests assert the interleaving they rely on, so under heavy load they fail rather than pass on a different order.
126b07f merges #172, which changes only browser files (web/public/*, test/browser/*) that this PR does not touch. Since ec135ba, the only server or runner change is MergeCoordinator.statusAfterMerge (0056e48, e967af3), which no Docker test exercises; the Docker results below are from ec135ba.
agent-gate 30/30, agent-network 21/21, agent-planning 1/1, agent-question 2/2 (1 skipped), runner-workspace 3/3, agent-container 151/151 (1 skipped). agent-supervisor 44/45: the one failure ("Could not confirm that allocationId is unused") passed when rerun alone.
Docker gate overall
Pass, with the one agent-supervisor flake noted above. A first agent-container run reported "Failed to establish ownership of task container" during cleanup. The dev machine's Docker then held about 6,700 leftover test-fixture volumes and 1,000 containers from killed runs. After removing those (test runner labels only), agent-container passed cleanly.
Review record
Round
Reviewer
Base / head
Result
1–64
Independent agent (fresh subagent each round, full diff)
Found the unhandled rejection (66), test setups not matching production (65, 66, 72), tests that did not assert their interleaving (72, 73, 75, 78), the already-fixed check's own gh failure surfacing after an abort (70), and wording. All fixed and break-checked.
1 inline finding: untested pre-aborted replay. 2 summary-only findings: the runner trust gate let shutdown replace an earlier timeout; maxBuffer during the pipe drain was untested. All fixed with tests in c3a5b3d.
1 finding: merge preparation and publish admission lacked stop-reason tests. Added in ebf520e (preparation layer and publish through the server's trust read), each break-checked.
0 open findings; summary named Docker validation and the review-lesson audit as incomplete. Audit added (ec135ba); Docker re-run recorded under Validation.
No code issues; PR-body wording (Copilot 5's Docker concern unclassified; a non-review item in the audit; unnumbered Copilot rows). Fixed in the PR body.
1 finding: the merge answer could offer a retry from pre-command readiness after a poll ended the attempt. Fixed in 0056e48 with a regression test that failed before.
0 open findings; Copilot 7's finding marked resolved.
Review-lesson audit
Rounds 1–64 ran before the PR was opened, on the uncommitted tree. Their findings were of the same kinds as the classes below (stop-reason precedence, saved versus resendable outcomes, test setups, doc and comment claims) and are covered by the same rules; they are not listed one by one.
"Attach a rejection handler to a promise when it is created whenever anything is awaited before the promise is consumed" (Async jobs and polling). Captures round 66: the already-fixed check's handler was attached only after the other inspection stages settled, so an early rejection was unhandled.
"A race regression must assert that the interleaving it names actually happened" (Required race regressions). Captures rounds 72, 73, 75, 78 and 84: tests that could pass on a different order under load. (Round 72 also had a setup finding, classified below.) The existing rule "A validation fixture for a summary-only concern must assert the disputed intermediate representation …" covers only summary-only concerns.
Covered by existing rules:
A later abort replacing a gateway's, coordinator's or server's own stop reason (the Investigate summary-only merge lifecycle concerns #48 defects; Copilot 3's runner trust gate): AGENTS.md "Preserve the original timeout, cancellation, and shutdown reason through every layer".
A timeout before admission saved as a refusal; Copilot 2's saved outcome skipped after a timeout plus own abort: "look up its saved outcome before any other guard … Only a passing, nothing-applied outcome … stays resendable".
An abort after gh's exit replacing GitHub's answer; maxBuffer during the pipe drain: race "An abort error fires, then subprocess close arrives later", plus the stop-reason rule.
A failed queue inspection answering with a superseded attempt: "Never apply a background response without proving it is still current".
The merge answer showing pre-command readiness (the Investigate summary-only merge lifecycle concerns #48 defect, and Copilot 7's ended-attempt case): "Treat a successful external command as the transition it actually performed", and "Once an irreversible external command succeeds … keep repeat controls disabled".
After the merge with Run review history work in an abortable worker thread #170: the queue snapshot read before the new await (round 82), and the settle bound changing (round 80): "When synchronous work moves behind an await … list every await it adds", and "Count the subprocess shutdown grace periods … inside that overall deadline". The re-review follows "A parent merge, rebase, restack or base change invalidates prior review".
Test setups that did not match production (fake error types, the check's stop timer, waiting for gh before shutdown, test timeouts and timings, round 80's test inspections running review loads): "A test's setup must leave the state the production path would".
A test's database read racing shutdown: "must wait for observable state before acting".
Weak or vacuous assertions, the cache test's resolution order, missing tests (Copilots 1, 3 (the pre-aborted replay) and 4): "Every reproduced race requires a failing-before and passing-after regression", and the claim rule in Review readiness.
Inaccurate doc and comment claims (timing bounds, shutdown order, what is saved, test-coverage labels, round 81's doc gaps): "Treat every behavioural claim … as something to verify".
Rounds 87, 89–93, 95, 97 and 98, wording and audit-text findings only (punctuation, coverage wording, gaps and structure in this section, the Tests list and the change summary): "Treat every behavioural claim … as something to verify", and "Treat review-lesson extraction as a merge gate".
Copilot 5's and 6's summaries (21578cc, ec135ba), which named incomplete gates: the review-lesson audit is covered by "Treat review-lesson extraction as a merge gate" (this section is that audit); Docker validation by "the required gate must eventually pass or remain explicitly blocked" (see Validation).
The Docker hang during validation: "record the observed failure separately from product failures" (infrastructure, not product).
Round 70, the already-fixed check's own gh failure after an abort kept as gh's answer: specific to a check that turns failures into blockers; recorded in the #alreadyFixed comment.
Reproduces the summary-only merge-lifecycle concerns from PR #46 and
fixes the six server-side defects:
- Gateway reads keep their own first timeout and gh's own answer
instead of a later caller abort (github/merge.ts, github/issues.ts).
- The coordinator and readIssueAccess keep a timeout or gh answer that
came before their own abort (runner/merge.ts, web/server.ts).
- A timeout before admission answers a resendable 503 instead of being
saved as the click's refusal (RemoteTimeout in github/timeout.ts).
- An abort after gh's exit no longer replaces GitHub's answer
(github/run-with-input.ts), and a refusal stays definite.
- A failed queue inspection no longer reports a superseded attempt.
- The merge click's answer reports the attempt, not pre-command
readiness (MergeCoordinator.statusAfterMerge).
Cache invalidation was found correct; test/merge-status-cache.test.ts
is the evidence. The two browser polling defects go to a separate PR
stacked on F6b.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…#48)
Answers Copilot's review on #171:
- Regression tests for the parallel merge-state inspection stages: a
stage's gh exit stops the others, and the inspection waits for every
stage and reports that exit, including when its deadline fires while
a stopped rule read settles.
- The tests exposed an older bug: the already-fixed check's rejection
handler was attached only after the other stages settled, so a check
that settled first raised a process-wide unhandled rejection. The
handler is now attached when the check starts.
- After a caller's abort, the check's own earlier gh failure stays an
unknown blocker that the inspection discards for the abort, instead of
being saved as the click's refusal with raw gh text.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolves runner/merge.ts #replay (keeps #170's async load, shutdown abort
and tracked replays, plus #48's aborted-click answer and the click's own
signal on its fresh read) and web/server.ts (#170's async loadReview with
#48's statusAfterMerge). Adapts #48's new tests to the async review
service; one test now waits for the click's gh call before shutdown, as
production does.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ctions
Since #170 a click can be in a review load when its deadline fires, and
the load's history worker gets 1 s before it is terminated, so a click
can settle about 1 s or a little more after its deadline, not 0.75 s.
The merge-queue poll tests read the recorded snapshot from the Store
instead of running a review load inside every mocked inspection.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Since #170 the review load awaits a worker; reading the queue snapshot
before it could pair an older queue state with a newer attempt status.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot review on #171: when a read's own timeout was followed by this
coordinator's abort, the #merge catch skipped the replay check, so an
outcome another coordinator had saved under the same click was answered
as a resendable 503. The replay is now always checked; for an aborted
click it reads nothing from GitHub. Only recording a new refusal stays
limited to gh's answers.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Handle the timeout before the shutdown guard. If the issue read times out and shutdown starts while gh is still settling, stopping/signal.aborted is true here, so the earlier RemoteTimeout is replaced with ShuttingDownError and the concurrent saved-outcome lookup is skipped. Move this branch above the shutdown check and add the corresponding controlled interleaving regression; timedOut(error) already proves the read's timeout won.
Test max-buffer failure during post-exit pipe draining
github/run-with-input.ts:29
The changed exit-order behavior also claims that output-limit failures arriving during post-exit pipe draining still win, but the existing max-buffer test only overflows while the child is running. Add a regression where the child exits, an inherited writer exceeds maxBuffer during the pipe grace period, and the promise rejects with the limit error; this verifies that the new exited guard remains limited to timeout/abort handling.
…48)
Copilot's third review on #171:
- The runner-action trust gate checked for shutdown before the timeout
branch, so a trust read whose own timeout fired just before shutdown
answered "shutting down" and skipped an outcome saved meanwhile. The
timeout branch now runs first; nothing is admitted either way.
- Regression tests for paths that were traced but untested: a click
aborted before its replay answers the saved success with a refresh
blocker, without loading the review or reading GitHub; and output
past maxBuffer while the pipes drain after gh exited still fails the
command.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot's fourth review on #171: the shared trust-read change also
reaches merge preparation and publish admission, which had no tests.
- Merge preparation: a trust read that timed out, or that gh answered,
before shutdown settles and saves the preparation as failed with that
answer, not "Server shutdown."
- Publishing: the same read during publish admission is recorded as
failed with that answer, not as stopped.
Both run against the error the server's readIssueAccess builds, and
fail if the server, the preparation or the publisher reports the abort
instead. The runner-start shutdown test now shows non-admission through
a distinct saved answer rather than a database read racing shutdown.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Attach a promise's rejection handler when it is created whenever
anything is awaited before it is consumed (the already-fixed check's
unhandled rejection).
- A race regression must assert the interleaving it names before using
the outcome as evidence.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Shared cancellation and durable merge behavior need final human review, with a status-response correction outstanding and exact-head CI reported pending.
Copilot review on #171 (126b07f): if a queue poll ended the attempt
while the server reloaded the review after a successful enqueue, the
merge answer combined the ended attempt with GitHub readiness read
before the command and could report ready with a retry action. The
answer now offers no action in that case and asks for a refresh, so a
retry is offered only from a fresh GitHub read. Regression test added;
it fails on the previous code.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolves runner/pre-merge.ts and test/pre-merge.test.ts against #170's
async review loads:
- #pushRewrite takes a ReviewView; the refresh after a moved branch uses
#170's #refresh signature, rechecking the push binding before it
records the refreshed pair.
- The review reload after a successful push now awaits a history
worker, so the push binding is checked again after it: a review edit
during that reload is refused rather than adopted, and no command
check runs under stale approvals (new regression; it fails without
the check).
- F6a's tests read the review asynchronously, use reviewState() in
synchronous hooks, and count the push's authorization in #170's
final-load tests.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflicts resolved: github/merge.ts keeps F6b's #alreadyFixed(target)
with #171's early rejection handler; runner/merge.ts and the tests keep
both sides' imports and fixture options; web/server.ts keeps #170's
async loadReview with F6b's mergeView.
Ported F6b to #170's async review loads:
- MergeJobCoordinator.start is async. A saved click still replays
first; then the review loads under the request's signal (a load
stopped by shutdown decides nothing: 503); after it, a resend of the
same click replays and shutdown refuses, and the token check, every
admission check and beginMergeJob run in one turn.
- The job's own loads await, and the required-check wait lets
MergeCoordinator.status load under the job's signal.
- Tests await the review service and the job's admission; one wait
limit is raised for the worker loads, and two fixtures order their
steps on observed state rather than fixed delays.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #48 (server part; #48 stays open until the browser fixes land). The two browser polling defects will follow in a separate PR stacked on F6b (#161), because F6b rewrites
scheduleMergePoll/pollMergeQueueinweb/public/app.js.What #48 asked
Reproduce Copilot's summary-only concerns on PR #46 about the merge lifecycle before changing code, and record evidence for any that cannot be reproduced.
Findings
Eight defects were reproduced. Cache invalidation was found correct.
CommandFailed,ghAnswered()) through#merge,#pollQueue) andreadIssueAccessreplaced a gateway's first stop reason with their own later aborttimedOut()) or gh answer that came first (AGENTS.md: "Preserve the original timeout, cancellation, and shutdown reason through every layer")RemoteTimeout(github/timeout.ts); answered as a resendable 503 and never saved; an outcome saved meanwhile under the same click is still replayed; the runner-action trust gate does the sameunknownrunWithInputignores stop requests after exit; a refusal counts when gh answeredMergeCoordinator.statusAfterMerge, sharing#withAttemptwithstatus()test/merge-status-cache.test.tsis evidenceThe rules are written up in
docs/implementation/merge-queue.md, "Stop reasons and answers (#48)", including what is traced but not tested.Also corrected: two older sentences in
runner-lifecycle.mdsaid a planning GitHub read failure (502) is not recorded; the code saves it andtest/planning-production.test.tschecks the replay.After Copilot's reviews (8e49642 onward)
test/merge-stop-reasons.test.ts,parallelStages()): a stage's gh exit stops the others, and the inspection waits for every stage and reports that exit, also when its deadline fires while a stopped rule read settles.#inspectNowattached the already-fixed check's rejection handler only after the other stages settled. A check that a failed stage stopped, and that settled first, raised a process-wide unhandled rejection. Node's default is to exit on that. The handler is now attached when the check starts.mainat 99e58a9 (Run review history work in an abortable worker thread #170, async review loads).#replaykeeps Run review history work in an abortable worker thread #170's shutdown abort and tracking, plus this PR's handling of the click's own signal. The merge answer reads its queue snapshot after the review load. The click's settle bound is restated: a review load's history worker gets 1 s after the abort, so a click can settle about 1 s after its 14 s deadline.maxBufferwhile the pipes drain (c3a5b3d).AGENTS.mdand follow-up Keep the rebase conflict resolver's trust-read timeout over a later stop #174 linked (ec135ba).mainagain at d4fe387 (Keep input made before the first review load, and explain a refused plan import #172, browser files only; no conflicts and no interaction with this PR) as 126b07f.Follow-ups (not in this PR)
runner/rebase.tsreports a pending cancel, shutdown or rebase deadline in place of the resolver's own timeout. Unchanged here (F5/F6 code); filed as Keep the rebase conflict resolver's trust-read timeout over a later stop #174.Tests
New:
test/merge-stop-reasons.test.ts,test/merge-queue-poll.test.ts,test/merge-status-cache.test.ts. Changed:test/runner-start.test.ts,test/pre-merge.test.tsandtest/runner-publishing.test.ts, plus a comment intest/gh-env.test.ts. Each fix was break-checked: reverting it makes its test fail. The timing-dependent tests assert the interleaving they rely on, so under heavy load they fail rather than pass on a different order.Validation (head 91da232, base d4fe387)
126b07f merges #172, which changes only browser files (
web/public/*,test/browser/*) that this PR does not touch. Since ec135ba, the only server or runner change isMergeCoordinator.statusAfterMerge(0056e48, e967af3), which no Docker test exercises; the Docker results below are from ec135ba.tsc --noEmit)testReview record
Closes #48(now "Part of #48")mainand this round. Fixed in the PR body.Review-lesson audit
Rounds 1–64 ran before the PR was opened, on the uncommitted tree. Their findings were of the same kinds as the classes below (stop-reason precedence, saved versus resendable outcomes, test setups, doc and comment claims) and are covered by the same rules; they are not listed one by one.
New rules in this branch (
AGENTS.md, ec135ba):Covered by existing rules:
One-off:
#alreadyFixedcomment.🤖 Generated with Claude Code