Skip to content

Show the merge job's step list and let a person cancel the merge alone (D16) - #194

Open
mchwang wants to merge 31 commits into
mainfrom
feat/merge-step-list
Open

mchwang wants to merge 31 commits into
mainfrom
feat/merge-step-list

Conversation

@mchwang

@mchwang mchwang commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Part of closing build step 4 (#22 follow-up): the merge job's full step list and Cancel button (design D16, DT7 step list). Issues #174 and #176 were closed by separate PRs (#197 and #195).

What changes

  • Each merge job records the design's seven steps (Fetch → Rebase → Push → Re-run checks → GitHub checks → Already-fixed check → Merge) with state (pending, running, done, skipped, failed, stopped, unknown) and start/end times, inside its existing merge_jobs JSON record. No schema version change; records saved before this have no steps and show as before.
  • Preparation (PreMergeCoordinator.start) and the guarded merge (MergeCoordinator.merge, onAsking) report steps through display-only callbacks: a failed step write is logged and never stops the job.
  • Cancel merge (POST /api/action {action: "cancel-merge", jobId}) stops the job and its preparation without cancelling the task. The task keeps its status, its PR stays open, and Merge PR can be clicked again. Refused once the job's merge attempt exists (GitHub was asked), and while the task itself is being cancelled. A restart after a cancel settles the job cancelled.
  • The review screen shows the list under the review strip, with icon + word + color per DESIGN.md, elapsed time, and the Cancel button (marked unavailable, aria-disabled, once the Merge step starts).
  • The merge job saves its waiting checks only when they change (it starts with none), so unchanged inspections add no revision and the page's poll backs off.
  • DESIGN.md: the step marks (○ Waiting, ● Running, – Skipped, ! Stopped, ! Unknown) added to the status table and decision log, approved by the user.
  • AGENTS.md: one new rule (Async review UI) for moving focus when a background update hides the focused control.
  • Docs: merge-handoff.md "Step list and Cancel"; architecture, design and README status; build step 4 marked done, with the rest of D16 and DT7 in Build the rest of D16 and DT7: named merge failure banners, Queue-row step, and the blocker list with links #199.

User decisions (2026-10-20): Cancel stops the merge only, not the task; use the design's 7 steps. (2026-10-21): Cancel shows only while the job runs, not "always" as D16 says; the rest of D16 and DT7 goes to follow-up #199; the step marks are added to DESIGN.md; the step list is a bar under the review strip, not a replacement for the header. Each is recorded beside D16 or in DESIGN.md's log.

Validation (head 546f6fc, intermediate checkpoint)

  • Typecheck: pass.
  • test/merge-job.test.ts: 106/106 at 561ed01 (13 new); 16 new tests in total after round 1, all passing (-t cancel 15/15).
  • New test/pre-merge.test.ts tests at 561ed01 (5): pass. Six in total after round 2. The rest of that file timed out locally under a machine load average of ~200 (48 timeouts at 15 s, 2 timing assertions); not yet compared with main under the same load. CI is the gate.
  • New browser tests (4, -g D16): pass. The ended-answer and backed-off-poll tests were each checked to fail with their fix removed.
  • test/merge-job.test.ts: 111/111 at a741b97. Browser -g D16: 9/9 at 546f6fc (the 2 new tests run alone: 2/2); the in-flight-poll and focus tests were each checked to fail without their own fix.
  • CI test at 561ed01 failed only the route test fixed in round 1 (2476 other tests passed, including all of pre-merge).
  • New server route test in test/runner-publishing.test.ts: pass (failed at 561ed01: its fake lacked stopForMergeCancel).
  • Full suite: CI (see each checkpoint below).

Review record

  • Self-review round 1 (author, full diff vs main): fixed a doc comment split in runner/merge.ts; removed an untested doc claim about pushes.
  • Independent review round 1 (fresh general-purpose agent, base main / head 561ed01): 6 findings.
    1. High: route test fake lacked stopForMergeCancel (409). Fixed in the fake. Declined the optional try/finally: the stop calls cannot throw.
    2. Medium: a cancel answered with an ended job stalled the page. Fixed: adopt only running records, poll at once otherwise; browser test added.
    3. Medium-low: untested windows. Added tests: cancel during guarded-merge validation, during the job's first review load, cancel then shutdown; browser test now answers an older record after the cancel.
    4. Low: list can name a step Failed whose own work succeeded, or none. Documented in merge-handoff.md (the banner gives the reason).
    5. Low: ticker rebuilt the list each second; duplicate "Cancelling…" names; Cancel not "always shown". Fixed the ticker (times only) and named the pending button "Cancel requested"; documented when Cancel shows.
    6. Low: test count in this body. Fixed.
  • Independent review round 2 (fresh general-purpose agent, base main / head 6f00799): no correctness or safety defects; round-1 fixes verified. Findings:
    1. Low-medium: after Cancel, a backed-off poll could take up to 30 s to show the outcome. Fixed: the poll key includes the cancel; browser test (checked to fail without it).
    2. Low: restart after a merge cancel and a task cancel used the merge-only reason. Fixed in settleInterruptedMergeJobs; store test.
    3. Low: step word and time were aria-hidden. Fixed: the word and the time are text; only the mark is hidden; focus moves off a hidden Cancel.
    4. Low: docs listed only some no-running-step cases; repeat cancel after the end; clock skew. Documented.
    5. Low: no test of a merge cancel during the push. Added (pre-merge). A fetch-phase cancel is not separately tested (same abort path as the rebase and push tests).
  • Independent review round 3 (fresh general-purpose agent, base main / head 8753af0): no correctness or safety defects; rounds 1–2 fixes verified. Findings, all fixed:
    1. Low: an inspection that answered after a cancel could still block the task. Fixed: the stop wins after the status read; test (checked to fail without the fix).
    2. Low: the immediate poll after an ended cancel answer was skipped for a poll of an older generation. Fixed.
    3. Low: doc said Cancel is disabled once Merge starts, but a failed step write leaves it enabled. Documented (the refusal explains itself).
    4. Nit: unescaped fallback step word. Escaped.
    5. Nits: duplicate comment line, repeated doc sentence. Removed. Declined: a repeat cancel raising revision (harmless; the button is disabled while pending).
  • Independent review round 4 (fresh general-purpose agent, diffed against main's tip 46567ea, merge base aced300 / head 2dd2266): no server-side defects; rounds 1–3 verified. Findings, all fixed:
    1. Low: a cancel answered while a poll was in flight, or with a newer job, could leave a stale running list for up to 30 s. Fixed: any non-adopted answer polls now, or 2 s after the poll in flight; two browser tests.
    2. Low: focus could fall to the page when the whole bar hid. Fixed: the check runs before the early return.
    3. Nit: the Restart row omitted the cancelled case. Fixed.
    4. Nit: the disabled Cancel's reason lived only in title. Fixed: a visible note that also describes the button (a disabled Cancel ignores clicks, so no refusal banner would explain it).
  • Independent review round 5 (fresh general-purpose agent, base main / head a741b97): no server-side defects; rounds 1–4 verified. Findings, all fixed:
    1. Low-medium: an enabled Cancel was described by the hidden "GitHub has been asked" note (a hidden element named by aria-describedby is still read). Fixed: the description is set only while the note shows; test asserts the enabled description.
    2. Low: the focus claim had no test. Added: focus moves to the merge control when Cancel hides, and when the whole bar hides (to Refresh if the merge control hides too).
    3. Nit: focus fell to the page when a failed load hid the merge controls. Fixed: it moves to Refresh.
  • Independent review round 6 (fresh general-purpose agent, base main / head d15f269): no correctness, safety or data-integrity defects; rounds 1–5 verified. Findings, all fixed:
    1. Low: the focus fallback to Refresh (merge control hidden too) had no test. Added; checked to fail when only that branch is broken.
    2. Low: the focus move in a failed load had no test. Added; checked to fail when only that line is removed.
    3. Nit: a failed load left the elapsed-time ticker running. Fixed.
  • Independent review round 7 (fresh general-purpose agent, PR diff against merge base aced300 / head 546f6fc): clean, no findings at any severity; rounds 1–6 verified. Two display-only notes, both already documented (a crash or failed write between beginning the attempt and the Merge step write leaves Merge at Waiting; a blocked job's running step reads Failed).

Before Copilot request 1 (recorded at head 546f6fc, validated head for that request)

Copilot round 1 (head 546f6fc)

Copilot (copilot-pull-request-reviewer) on base aced300 / head 546f6fc: 3 medium findings, all in web/public/app.js. Fixed in 1b4ded6; each thread was answered and resolved.

  1. A job that ended queued lost its step list once queue polling replaced the job-refresh blocker. Fix: endedJobShown on the merge view, kept by queue polling. Regression: the queued-job browser test now has a step list and asserts it after GitHub merged (fails without the fix).
  2. A cancel answer arriving after the poll ended the job forced a queue-only poll that ignored the job, so a newer job was never followed and the ended job's steps and outcome were hidden. Fix: answers already seen change nothing; a newer running job is adopted through withMergeJobObservation; a poll that read an older job is discarded. Regressions: the refusal arrives after the poll saw the job end, carrying the ended job, an older record and a newer job (each fails without its fix).
  3. A refusal could replace a success banner the poll had shown. Fix: the refusal is shown only while the cancelled job is still shown running. Regression as in 2.

After Copilot round 1

  • Independent review round 8 (fresh general-purpose agent, base aced300 / head 1b4ded6): no correctness, safety or data-integrity defects. Each Copilot fix's test was confirmed to fail without it. Local at 1b4ded6: merge-job 111/111, route test 1/1, new pre-merge tests 7/7, browser -g "D16|follows the merge queue a polled job ended in" 13/13, typecheck. Findings, fixed in db6e7eb (docs only):
    1. Low: a doc sentence kept a leftover clause. Rewritten.
    2. Nit: "until Refresh" was too narrow; any review reload clears the list. Fixed.
    3. Low: this body did not record the Copilot round. Recorded above.
  • Independent review round 9 (fresh general-purpose agent, merge base aced300 / head db6e7eb): clean on correctness, safety and data integrity; no high or medium findings. Local at db6e7eb: merge-job 111/111, the 6 new pre-merge tests (filter -t "merge is cancelled|preparation step|command checks skipped|progress callback throws", which also matches 1 existing test: 7/7), route test 1/1, browser 13/13. Findings:
    1. Low: Cancel "always shown" (D16) is not a recorded user decision. The user approved the deviation on 2026-10-21; recorded above.
    2. Nit: settleInterruptedMergeJobs doc comment did not mention the cancelled case. Fixed in 84e6b14 (comment only).
    3. Nit: the new pre-merge test count. Corrected (6 new).
    4. Nit: round 4's base label. Corrected.
    5. Declined: a route-level test for 503 on cancel-merge during shutdown. The route reaches 503 through the server's shared stopping check and ShuttingDownError handler, which other routes' tests cover; cancelMerge's ShuttingDownError is tested at the coordinator.
  • Independent review round 10 (fresh general-purpose agent, merge base aced300 / head 84e6b14): no correctness, safety or data-integrity defects; the declined item judged reasonable. Local at 84e6b14: merge-job 111/111, new pre-merge 7/7 (filter), route 1/1, browser 13/13. Findings, fixed in 9e49746:
    1. Low: the Shutdown row (and the #end comment) did not cover a cancelled merge. Fixed.
    2. Low: Cancel merge was accepted while the task was being cancelled, so the banner promised the PR stays open. Fixed: refused with "The task is being cancelled."; unit test.
    3. Low: this body said Keep no Docker daemon log of agent containers (#165) #190 and Sweep leftover path identity probes in startup recovery (#181) #192 touch only agents/. Corrected above.
    4. Low: no record at the current head; footer line in the middle. Fixed (validation below; footer moved to the end). The review-lesson audit is added before merge.
    5. Nit: no browser test for the refusal banner of an enabled Cancel. Added.
    6. Nit: docs/architecture.md did not name Cancel merge as a screen button. Fixed.
    7. Nit: the pending-cancel flag was page-wide. Now per job.
    8. Nit: D16 did not note the approved amendment. Noted beside D16.
  • Validation at 9e49746 (intermediate checkpoint): merge-job 112/112; browser -g "D16|follows the merge queue a polled job ended in" 14/14; typecheck. CI (test, real-docker) passed on 9e49746.
  • Independent review round 11 (fresh general-purpose agent, merge base aced300 / head 9e49746): no high or medium findings. Local at 9e49746: merge-job 112/112, browser 14/14. Findings, fixed in 40d1563:
    1. Low: a cancel refusal was overwritten by the next poll even when nothing changed, though it is the only explanation of an enabled Cancel. Fixed: kept until the job's record changes; the browser test now waits past a poll (fails without the fix).
    2. Nit: any cancel's answer cleared another job's pending state. Fixed (cleared only for its own job).
    3. Nit: an always-true assertion in the task-cancel refusal test. Fixed.
    4. Nit: the repeat-cancel rule omitted the task-cancel refusal. Fixed.
    5. Nit: this body's change list and checkpoint label. Fixed.
  • Validation at 40d1563 (intermediate checkpoint): merge-job 112/112; browser -g "D16|follows the merge queue a polled job ended in" 14/14; typecheck. CI (test, real-docker) passed on 40d1563.
  • Independent review round 12 (fresh general-purpose agent, merge base aced300 / head 40d1563): no high or medium findings. Local at 40d1563: merge-job 112/112, browser 14/14. Findings, fixed in ac6dd8f:
    1. Low: the held refusal stopped the poll from restoring the banner after a reload or a failed poll rewrote it (blank or stale error until the job's record changed). Fixed: the refusal's text is kept with its key and each poll writes it again; the browser test now fails one poll in between (fails without the fix).
    2. Nit: merge-handoff.md did not describe the held refusal, and its Tests section listed none of this PR's tests. Fixed.
  • Validation at ac6dd8f (intermediate checkpoint): merge-job 112/112; browser -g "D16|follows the merge queue a polled job ended in" 14/14; typecheck. CI (test, real-docker) passed on ac6dd8f.
  • Independent review round 13 (fresh general-purpose agent, merge base aced300 / head ac6dd8f): no code defects. Findings, fixed in 891e6c3:
    1. Medium (docs): the build step 4 status lines this PR rewrites still said Keep the rebase conflict resolver's trust-read timeout over a later stop #174 and App-pinned required checks always read as missing #176 were open; both closed (Read app-pinned required checks from the head's check runs (#176) #195, Keep the rebase conflict resolver's trust-read timeout over a later stop (#174) #197). Fixed after merging main: build step 4 is marked done (checkbox ticked), with architecture.md and README.md updated to match.
    2. Low: the held-refusal test covered a failed poll but not a Refresh. Added (the test's review fake now carries the running job, as a runner server does); a mutation clearing the refusal on Refresh fails it.
    3. Low: this body did not record the base change or a self-review on the latest head, and the lesson audit was pending. Recorded below.

Base integration (2026-10-21)

main moved to e46cb68 (#190, #192, #195, #196, #197). Merged into the branch in 17db340 (no conflicts; shared files: docs/architecture.md, docs/designs/codeboost-plan-indexed-review.md, docs/implementation/merge-handoff.md, test/pre-merge.test.ts, test/runner-publishing.test.ts). Earlier reviews and validation are for the pre-merge pairs; the new pair needs a clean pass.

Self-review round 2 (author, base e46cb68 / head 891e6c3)

Full diff against main re-read, with the merge's shared files. Checked: the merged docs keep #195's and #197's text; status lines now true at merge; README status names the step list. No new issues found.

  • Validation at 891e6c3 (intermediate checkpoint): typecheck; merge-job 112/112; new pre-merge tests 7/7 (filter, 6 new); route test 1/1; browser -g "D16|follows the merge queue a polled job ended in" 14/14. CI (test, real-docker) passed on 891e6c3.

Review-lesson audit (merge gate)

Each finding is covered by an existing rule, captured by the one new rule below, or a one-off with its reason.

New rule (this branch, AGENTS.md, Async review UI; CLAUDE.md has no mirror of that section): "When a background update (a poll, a reload, an answer) hides or removes the control that has keyboard focus, move focus to a named fallback that is still shown and not disabled (an aria-disabled control qualifies) once the update has set every control's final state, and test that focus lands there for each way the control can be hidden. Decide a focusable control's visibility once per update: hiding it and showing it again blurs it." (Amended after rounds 20, 21 and 22.) It captures rounds 4.2, 5.2, 5.3, 6.1 and 6.2. That class recurred, and no existing rule covered it: the existing focus rule covers only a control disabled during a request.

Existing rules cited:

  • [claim] CLAUDE.md, Review readiness: "Treat every behavioural claim … as something to verify … and give it a test that fails if the claim is false."
  • [current] AGENTS.md, Async jobs and polling: "Never apply a background response without proving it is still current". With the required race regression "Poll starts, then a user action completes, then the old poll returns."
  • [backoff] AGENTS.md: "Reset the interval only after a meaningful lifecycle change or explicit user action."
  • [holders] AGENTS.md: "Define the lifecycle states and ownership …" and "Treat … rendered UI as separate state holders. Define how each transitions and settles."
  • [one-part] AGENTS.md, Async review UI: "When polling updates one part of the screen, update only that state."
  • [focus] AGENTS.md, Async review UI: "While a request is in flight, do not disable the control that has keyboard focus … Mark it aria-disabled."
  • [design] DESIGN.md: "Status is always an icon plus a word plus a color"; "Status mark: icon + word + color, with a screen-reader label."
  • [evidence] CLAUDE.md evidence rules: label cited commits; record each round; report state before Copilot; list deferred follow-ups.
  • [gate] CLAUDE.md: "Treat review-lesson extraction as a merge gate … classify every review finding".

By round (one-off reasons in brackets):

  • Self-review round 1: doc comment split, one-off [editorial]; untested push claim, [claim].

  • Round 1: 1 one-off [an internal fake cast to its type missed a new method; not an external tool; CI caught it]; 2 [current]; 3 [claim]; 4 [claim]; 5 one-off [screen-reader churn from a 1 s rebuild, and two controls with one name: accessibility details no rule names], with Cancel visibility a user decision, one-off [settled by the user, recorded beside D16]; 6 [evidence].

  • Round 2: 1 [backoff]; 2 [holders]; 3 [design]; 4 [claim]; 5 [claim].

  • Round 3: 1 [current]; 2 [current]; 3 [claim]; 4 one-off [defensive escaping of a server value]; 5 one-off [editorial; the declined part recorded with its reason].

  • Round 4: 1 [current]; 2 new rule; 3 [claim]; 4 one-off [a disabled control's reason lived only in title, which keyboard and screen-reader users do not reliably get; no rule names it].

  • Round 5: 1 one-off [a hidden element named by aria-describedby is still read: a browser detail no rule names]; 2 new rule; 3 new rule.

  • Round 6: 1 new rule; 2 new rule; 3 [holders] (the ticker on the failed-load path).

  • Copilot round 1: 1 [current] and [one-part]; 2 [current]; 3 [current] and [one-part].

  • Round 7: clean; its two display-only notes were already documented, so nothing to classify.

  • Round 8: 1 one-off [editorial]; 2 [claim]; 3 [evidence].

  • Round 9: 1 one-off [settled by the user; recorded]; 2 [claim]; 3 [evidence]; 4 [evidence]; 5 one-off [declined: the route reaches 503 through shared, already-tested handling].

  • Round 10: 1 [claim]; 2 [holders]; 3 [evidence]; 4 [evidence]; 5 [claim]; 6 [claim]; 7 [current]; 8 one-off [the user decision recorded beside D16].

  • Round 11: 1 [one-part]; 2 [current]; 3 [claim] (an always-true assertion); 4 [claim]; 5 [evidence].

  • Round 12: 1 [one-part] and [current]; 2 [claim].

  • Round 13: 1 [claim]; 2 [claim] (the held refusal across a Refresh had no test), plus CLAUDE.md "A test's setup must leave the state the production path would" (the browser test's server stand-in lacked the running job a runner server's review carries); 3 [evidence].

  • Round 14: 1 [claim]; 2 [gate]; nit 1 one-off [the change list omitted README.md; record completeness]; nit 2 (onAsking wording) [claim].

  • Round 15: 1 one-off [settled by the user: follow-up Build the rest of D16 and DT7: named merge failure banners, Queue-row step, and the blocker list with links #199]; 2 new rule; 3 [gate]; 4 [gate]; 5 [gate]; 6 [evidence]; 7 one-off [settled by the user: marks added to DESIGN.md]; 8 [backoff]; 9 [claim].

  • Round 16: 1 [gate]; 2 [gate]; 3 [claim]; nit 1 [claim]; nit 2 one-off [the N/A mark worded "Skipped" for a step; within the approved step list, recorded in DESIGN.md's log].

  • Round 17: 1 one-off [settled by the user: marks added]; 2 one-off [the change list omitted three changes; record completeness]; 3 [gate]; 4 new rule; 5 one-off [settled by the user: placement]; 6 [claim].

  • Round 18: 1 [claim]; 2 [claim]; 3 [gate]; nit 1 [evidence]; nit 2 one-off [settled by the user; logged in DESIGN.md]; nit 3 [claim]; nit 4 [claim].

  • Round 19: 1 new rule (focus off Merge PR and its blockers button when a reload hides them); 2 [evidence]; nit 3 [claim]; nit 4 [claim]; nit 5 [gate].

  • Round 20: 1 new rule (Merge status hidden and shown again blurred it on each poll); 2 new rule (the Merge PR fallback had no test); 3 one-off [a test fixture's revision note, not an external tool; it now claims only the order]; nit [evidence].

  • Round 21: 1 new rule (as amended: the fallback must be enabled once the render is done); 2 [gate] (the rule did not yet say so; amended); nit [evidence].

  • Round 22: 1 [claim] and new rule (Cancel's check relied on a parent-hide timing the comment contradicted); 2 [claim] (the not-disabled clause had no test; added, defensive); nit 3 [gate] (rule wording); nit 4 new rule (a second write of one control's visibility).

  • Round 23: nits 1–3 [claim] (a split doc comment, "disables" where the button is only aria-disabled, and the blockers button's two labels).

  • Round 24: 1 [claim] (the display-only onAsking guard had no test); 2 [claim] ("done after queued" had no test); nits 1–2 [claim] (wording); nit 3 [current] (a cancel answered as ended left Cancel offered until the poll answered).

  • Round 25: 1 [claim] and [current] (the wait cleared on a stale running answer, so round 24's claim was false on the in-flight path).

  • Round 26: nit 1 [claim] (a retried assertion that passed on the later state); nit 2 [claim] (step times after a restart); nit 3 [claim] (an untested comment sentence).

  • Round 27: 1 [claim] and [current] (the older-record poll guard had no test); 2 [claim] (the shutdown half of "decides nothing"); 3 [claim] (Fetch Failed for review); nit 4 [claim] (the join half); nit 5 [claim] (DESIGN.md Frame).

  • Round 28: 1 [claim] (Fetch's running window on the real preparation path was untested); nit [claim] (the handoff doc's test list).

  • Independent review round 14 (fresh general-purpose agent, base e46cb68 / head 891e6c3): no high or medium issues and no code defects. Findings, fixed in 25e67d2 (docs, comment) and this body:

    1. Low: D16 read as fully built, but its named failure banners and its Queue-row step are not. They are now listed as missing in the design review, and the "built" lines say "step list and Cancel".
    2. Low: the lesson audit was incomplete and miscited. Rewritten above, every finding by round.
    3. Nits: README.md added to the change list; onAsking's comment says "when a store is configured".
  • Independent review round 15 (fresh general-purpose agent, base e46cb68 / head 25e67d2): no code defects. Local at 25e67d2: merge-job 112/112, new pre-merge 7/7 (filter), route 1/1, browser 14/14. Findings, fixed in 8b9c9ad and this body:

    1. Medium: build step 4 marked done while DT7's blocker list and D16's failure banners and Queue-row step were unbuilt and untracked. The user chose a follow-up issue: Build the rest of D16 and DT7: named merge failure banners, Queue-row step, and the blocker list with links #199, cited in the design doc and architecture.md, and in the deferred follow-ups above.
    2. Medium: the audit put the hidden-focus class under a rule that does not cover it. New AGENTS.md rule added (see the audit), [new rule].
    3. Low: an [assert-state] citation dropped the rule's scope. Recited as [claim].
    4. Low: several [design] citations did not match DESIGN.md. Reclassified as one-offs with reasons.
    5. Low: self-review round 1, user decisions and the declined item were not classified. Classified.
    6. Low: CI was recorded as pending at heads where it passed. Updated (546f6fc, 9e49746, 40d1563, ac6dd8f, 891e6c3: test and real-docker passed).
    7. Low: DESIGN.md did not define the ○ Waiting and ● Running marks. The user approved adding them; added to DESIGN.md's status table and decision log. [one-off: settled by the user]
    8. Nit: each unchanged inspection saved the waiting checks again, adding a revision that stopped the page's poll from backing off (and cleared a held refusal). Now saved only when they change; merge-job test added (fails without the fix). [backoff]
    9. Nit: onAsking's comment said "in the same turn as GitHub is asked". Now "the call that asks GitHub to merge", wrapped.
  • Validation at 8b9c9ad (intermediate checkpoint): typecheck; merge-job 113/113. Round 16 ran browser -g "D16|follows the merge queue a polled job ended in|merge-job poll" 17/17 there.

  • Independent review round 16 (fresh general-purpose agent, base e46cb68 / head 8b9c9ad): no code defects; the waiting-checks change checked against every consumer; the shared AGENTS.md/CLAUDE.md sections still identical. Findings, fixed in 03957d6 and this body:

    1. Low: round 15's findings were not all classified. Classified above.
    2. Low: two audit citations were wrong (13.2 is not a focus finding; 4.4 is not covered by [focus]). Corrected.
    3. Low: the build step 4 checklist line did not cite Build the rest of D16 and DT7: named merge failure banners, Queue-row step, and the blocker list with links #199 or name all three missing parts. Fixed.
    4. Nit: the checks-write comment ignored the first save. Reworded.
    5. Nit: "– Skipped" was not in DESIGN.md. Added to the N/A row, with a log entry.
  • Validation at 03957d6 (intermediate checkpoint, from round 17): typecheck; merge-job 113/113; new pre-merge 7/7 (filter); route 1/1; browser -g "D16|follows the merge queue a polled job ended in|merge-job poll" 17/17.

  • Round 16 nit 2 classification stands; see round 17 for the remaining marks.

  • Independent review round 17 (fresh general-purpose agent, base e46cb68 / head 03957d6): no code defects, races or wrong route codes. Findings, fixed in the next commit and this body:

    1. Low: ! Stopped and ! Unknown used warning without a DESIGN.md entry. The user approved them; added to the warning row and the log. [one-off: settled by the user]
    2. Low: "What changes" left out DESIGN.md, the AGENTS.md rule and the waiting-checks change. Added.
    3. Low: two audit citations (13.2, 14 nits). Corrected.
    4. Nit: no focus test for a reload that shows the job ended with the merge control still shown. Added (3/3 with the other reload cases). [new rule]
    5. Nit: the step list's placement (a bar under the review strip) differed from D16 without a recorded decision. The user approved; recorded beside D16. [one-off: settled by the user]
    6. Nit: a status heading's check date. Updated.
  • Validation at 3d975e7 (intermediate checkpoint, from round 18): typecheck; merge-job 113/113; new pre-merge 7/7 (filter); route 1/1; browser 18/18.

  • Independent review round 18 (fresh general-purpose agent, base e46cb68 / head 3d975e7): no correctness, safety, race or route-code defects. Findings, fixed in dc6e6df and this body:

    1. Low: the older-job poll guard had no test that failed without it. Both "newer" tests now wait for the stale poll to land and assert the newer job stays (a mutation removing the guard fails).
    2. Low: "a new step of the list polls quickly again" had no test. Added a poll-delay case where only the list step changes ([2000, 4000, 2000, 4000]; fails without the step in the poll key).
    3. Low: audit citations for audit-rule findings and change-list omissions. Recited ([gate], one-offs), and rounds 17 and 18 added to the list.
    4. Nits: stale body lines corrected; the step bar's placement logged in DESIGN.md; the F6b queue test named in the handoff doc's test list; a test that the list items survive the 1 s timer.
  • Validation at dc6e6df (intermediate checkpoint): typecheck; browser -g "D16|follows the merge queue a polled job ended in|merge-job poll" 19/19 (one earlier run of the same set at load average about 98 failed without naming the test; the rerun passed). merge-job unchanged since 8b9c9ad (113/113). CI: pending.

  • Independent review round 19 (fresh general-purpose agent, base e46cb68 / head dc6e6df): no correctness, safety, data-integrity, race or route-code defects. Findings, fixed in 9883bec and 7e22563:

    1. Low: the focus moves off Merge PR and its blockers button had no test, and a reload that made merging unavailable hid them without moving focus. Fixed: renderMerge reads the focused control before hiding (hiding blurs it at once) and moves focus to Merge PR or Refresh; 4 browser tests (each control × a failed load and an unavailable reload); removing the move fails the unavailable cases.
    2. Low: main moved to 79deeec (Check the planning agent at startup and before planning requests (#164) #193) after the base integration. Merged in 7e22563 (no overlap with this PR's files; Check the planning agent at startup and before planning requests (#164) #193 changed planning code, its tests and docs).
    3. Nit: the redundant first save of the waiting checks. Removed (the job starts, and the merging step leaves it, with none).
    4. Nit: DESIGN.md's step-bar entry said "while a job runs". Widened to "and after it ends until the review reloads".
    5. Nit: round 7 had no audit entry. Added.

Base integration 2 (2026-10-21)

main moved to 79deeec (#193). Merged into the branch in 7e22563 with no conflicts and no shared files.

  • Validation at 7e22563 (intermediate checkpoint): typecheck; merge-job 113/113; browser -g "D16|follows the merge queue a polled job ended in|merge-job poll" 23/23. CI: pending.
  • Independent review round 20 (fresh general-purpose agent, base 79deeec / head 7e22563): one Low-Medium bug, two Lows. Fixed in 5910440:
    1. Low-Medium: renderMerge hid Merge status and then the running-job branch showed it again, so a focused Merge status lost focus on every poll of a running job with no blockers (to Merge PR since 9883bec, to the page before). Fixed: its visibility is decided once, counting a running job; browser test (fails without the fix).
    2. Low: the fallback to Merge PR when only the blockers button hides had no test. Added (a reload that clears the last blocker; fails if the fallback always picks Refresh).
    3. Low: the browser fixture's note on how revisions advance no longer matched production. Reworded to claim only the order.
    4. Nit: this body's waiting-checks wording. Fixed.
  • Validation at 5910440 (intermediate checkpoint): typecheck; browser -g "D16|follows the merge queue a polled job ended in|merge-job poll" 25/25. merge-job unchanged since 9883bec (113/113). CI: test (run 39570641945) and real-docker (run 39570641957) passed.
  • Independent review round 21 (fresh general-purpose agent, base 79deeec / head 5910440): one Low-Medium focus bug, one Low audit gap. Fixed in 188b68d:
    1. Low-Medium: without a runner, the fallback focused Merge PR before its disabled state was updated, so focus fell to the page when a reload cleared the last blocker. Fixed: renderMerge records the focused control, renders the controls and the step list, then picks Merge PR only when shown and enabled, otherwise Refresh (the Cancel fallback uses the same choice). The "clears the last blocker" test now runs with and without a runner; the old order fails the without-runner case.
    2. Low: the new rule did not cover round 20's hide-then-show or a disabled fallback. Amended (see the audit).
    3. Nit: CI at 5910440 recorded. Done.
  • Validation at 188b68d (intermediate checkpoint): typecheck; browser -g "D16|F6b|merge" 81/81. merge-job unchanged (113/113). CI: pending.
  • Independent review round 22 (fresh general-purpose agent, base 79deeec / head 188b68d): two Lows, two nits; no High or Medium. Fixed in ae68419:
    1. Low: Cancel's focus check read document.activeElement after hiding the step bar, working only because hiding a parent blurs at the next frame. Fixed: renderMergeSteps takes the control focused before the update; the comment states both blur timings.
    2. Low: the fallback's "not disabled" clause had no test. Added a defensive browser test (a reload that leaves Merge PR disabled; focus goes to Refresh); it fails without the clause. merge-handoff.md now describes every focus move.
    3. Nit: "enabled" was ambiguous for an aria-disabled Merge PR. Rule and comment now say "not disabled (an aria-disabled control qualifies)".
    4. Nit: Merge status's visibility was still written twice. The running branch's write is removed.
  • Validation at ae68419 (intermediate checkpoint): typecheck; browser -g "D16|F6b|merge" 82/82. merge-job unchanged (113/113). CI: pending.
  • Independent review round 23 (fresh general-purpose agent, base 79deeec / head ae68419): no High, Medium or Low findings; each focus move re-checked by mutation. Three nits, fixed in 69ea536 (comment and docs only):
    1. renderMergeSteps had two doc comments, the first saying "until Refresh". Merged; "until the review reloads".
    2. The docs and this body said the page "disables" Cancel; it is only aria-disabled and keeps focus. Reworded.
    3. The focus sentence named the blockers button only as "Merge status". Now "Review blockers or Merge status".
  • Validation at 69ea536 (intermediate checkpoint): typecheck; route test cancels a running merge job from the merge header 1/1 (the round's filter had missed it). Code unchanged since ae68419 (browser 82/82, merge-job 113/113). CI: pending.
  • Independent review round 24 (fresh general-purpose agent, base 79deeec / head 69ea536): no correctness findings; browser 82/82, merge-job 113/113, route 1/1 there. Findings, fixed in abeeda8:
    1. Low: "an error onAsking throws is ignored" had no test. Added a merge-job test with a throwing callback (GitHub is still asked; fails without the guard).
    2. Low: "Done after queued" for the running step had no test. The queued merge-job test now asserts it (fails without it).
    3. Nit: a comment still said "until Refresh". Reworded.
    4. Nit: a test title and two comments still said "disabled Cancel". Reworded (aria-disabled).
    5. Nit: after a cancel answered as ended, Cancel was offered again until the forced poll answered. It now stays "Cancel requested" and unavailable until then; the in-flight browser test asserts it (fails without the fix). (Corrected in round 25: with a poll already in flight, the wait cleared on that poll's stale answer; see round 25.)
  • Validation at abeeda8 (intermediate checkpoint): typecheck; merge-job 114/114; browser -g "D16|F6b|merge" 82/82. CI: pending.
  • Independent review round 25 (fresh general-purpose agent, base 79deeec / head abeeda8): no High or Medium findings; every set and clear of mergeCancelAwaitingPoll traced; round 24's mutations re-checked. One Low, fixed in 1d9f910:
    1. Low: a poll already in flight answered the cancelled job's older running record and cleared the wait, so Cancel was offered for about 2 s next to "This merge has already ended." Fixed: the wait clears only when a poll does not report that job running. The in-flight test now waits for the held poll's answer and asserts Cancel is still unavailable (fails without the fix).
  • Validation at 1d9f910 (intermediate checkpoint): typecheck; browser -g "D16|F6b|merge" 82/82. merge-job unchanged (114/114). CI: pending.
  • Independent review round 26 (fresh general-purpose agent, base 79deeec / head 1d9f910): no High, Medium or Low findings; every set and clear of the cancel flags traced (Cancel cannot stay stuck); round 25's mutation re-checked. Three nits, fixed in acddd76 (tests and one doc sentence):
    1. The round-25 assertion retried and passed on the later Merged state too. It now reads the in-flight state once (fails without the fix).
    2. A step ended by startup settlement counts the server's downtime in its time. Noted beside the clock-skew note.
    3. "A click that joins or replays another does not call onAsking" had no test. Added a replay test (fails if onAsking runs before the replay). (The join half was tested in round 27.)
  • Validation at acddd76 (intermediate checkpoint): typecheck; merge-job 115/115; the in-flight browser test 1/1. App code unchanged since 1d9f910 (browser 82/82). CI: pending.
  • Independent review round 27 (fresh general-purpose agent, base 79deeec / head acddd76): no correctness, safety, data-integrity, race or route-code defects; three untested claims found by surviving mutations. Fixed in 38f6625 (tests and DESIGN.md only):
    1. Low: the poll guard that discards an older record of the shown job had no test. Added a browser test: a poll in flight answers revision 3 after a cancel's 200 answer (revision 4) was adopted; Cancel, the control and the banner keep the cancel (fails without the guard).
    2. Low: "an inspection that answers after … shutdown decides nothing" was tested only for a cancel. Added a merge-job test: shutdown during an inspection that then reports a failing check; the job stays running and the task in review (fails without the check).
    3. Low: "Fetch is Failed when the head moved" had no job-level test. Added a merge-job test (fails if review-required maps to Stopped).
    4. Nit: the join half of the onAsking sentence had no test. Added (a resend joining a click in progress; fails if a join calls it).
    5. Nit: DESIGN.md's Frame did not name the step bar. Added.
  • Validation at 38f6625 (intermediate checkpoint): typecheck; merge-job 118/118; browser -g "D16|F6b|merge" 83/83. App and runner code unchanged since 1d9f910. CI: pending.
  • Independent review round 28 (fresh general-purpose agent, base 79deeec / head 38f6625): no defects, races or wrong route codes; 31 mutations killed. One Low and one nit, fixed in 8da81cf (a test and a doc line):
    1. Low: "Fetch is Failed when the head moved" was tested only with the fake preparation. The real-path head-moved test in test/pre-merge.test.ts now asserts the progress events are exactly fetch:running (fails if Fetch is marked done before the check).
    2. Nit: the handoff doc's test list lacked round 27's tests. Added.
  • Validation at 8da81cf (intermediate checkpoint): typecheck; the head-moved pre-merge test 1/1. Code unchanged since 1d9f910 (merge-job 118/118 and browser 83/83 at 38f6625). CI: pending.
  • Independent review round 29: pending. Copilot re-request: after a clean independent pass on the new pair.

🤖 Generated with Claude Code

mchwang and others added 7 commits October 20, 2026 18:29
…e (D16)

Merge PR's job now records the design's seven steps (Fetch, Rebase, Push,
Re-run checks, GitHub checks, Already-fixed check, Merge) with each step's
state and start and end times, in its merge_jobs record. Preparation and
the guarded merge report their steps through display-only callbacks.

The review screen shows the list under the review strip with a Cancel
merge button. Cancelling stops the job and its preparation without
cancelling the task: the task keeps its status, its pull request stays
open, and Merge PR may be clicked again. It is refused once GitHub has
been asked to merge. A restart after a cancel settles the job cancelled.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Give the server test's fake preparation stopForMergeCancel; poll at once
when a cancel is answered with a job that ended; rewrite only the elapsed
times each second; name the pending Cancel 'Cancel requested'; test a
cancel during the guarded merge's validation, during the job's first
review load and before shutdown, and older poll records after a cancel;
document where the list says a job stopped and when Cancel shows.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Poll quickly again after a cancel, so its outcome shows within seconds of
a backed-off poll; settle a restart after both cancels as a cancelled
task; let screen readers read each step's word and time as text and
move focus off a hidden Cancel; test a merge cancel during the push;
document every case where no step reads where the job stopped, repeat
cancels and clock skew.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Let a stop win over an inspection that answers after it, poll at once
after an ended cancel answer unless a poll of this generation is in
flight, escape a step's fallback word, and say that Cancel explains
itself when the Merge step's write failed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Poll again within 2 s when a cancel is answered with an ended or newer
job while a poll is on its way; move focus off Cancel whenever the bar
or the button is hidden; write why a disabled Cancel cannot stop the
merge beside it; give the restart row its cancelled case.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Describe Cancel by its note only while the note is shown, test that focus
moves off a hidden Cancel (to Refresh when the merge control hides too),
and keep focus on the page when a failed load hides the merge controls.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Test that focus on Cancel moves to Refresh when a reload hides the merge
control or fails, and stop the elapsed-time ticker when a failed load
hides the step bar.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Cancellation-response races and queue polling can leave stale outcomes or hide completed merge progress.

3 open issues
What changed in this PR

Adds D16 merge progress and merge-only cancellation to the existing background merge workflow, advancing build step 4.

Changes:

  • Persists seven merge steps with states and timestamps.
  • Adds cancellation that leaves the task and pull request open.
  • Adds progress UI, lifecycle regressions, and documentation.
File Description
web/​server.ts Adds the cancel-merge endpoint.
web/​public/​style.css Styles merge steps and cancellation controls.
web/​public/​index.html Adds the step list and Cancel button.
web/​public/​app.js Renders progress and handles cancellation responses.
test/​runner-publishing.test.ts Tests the cancellation endpoint.
test/​pre-merge.test.ts Tests progress callbacks and preparation cancellation.
test/​merge-job.test.ts Tests step persistence, cancellation, and recovery.
test/​browser/​review.spec.ts Tests progress, polling, cancellation, and focus.
runner/​store.ts Persists steps and cancellation state.
runner/​pre-merge.ts Reports progress and supports merge-only stopping.
runner/​merge.ts Reports when guarded merge submission begins.
runner/​merge-job.ts Coordinates progress and merge-only cancellation.
docs/​implementation/​merge-handoff.md Documents progress and cancellation behavior.
docs/​designs/​codeboost-plan-indexed-review.md Updates D16 delivery status.
docs/​architecture.md Updates merge workflow capabilities.

Review effort: Balanced

Comment thread web/public/app.js Outdated
Comment thread web/public/app.js Outdated
Comment thread web/public/app.js Outdated
mchwang and others added 9 commits October 20, 2026 20:13
Keep the step list of a job that ended queued through the queue polls;
ignore a cancel answer the poll already showed, follow a newer running
job it carries and discard polls of older jobs; show a refusal only
while the cancelled job is still shown running.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…194 review round 8)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…w round 9)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refuse a merge-only cancel while the task is being cancelled, keep the
pending-cancel flag per job, update the Shutdown row and the #end
comment for a cancelled merge, name Cancel merge in the architecture
overview, note the approved D16 amendment beside D16, and test the
refusal banner for an enabled Cancel.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
 review round 11)

Also clear only this cancel's own pending state, check the task-cancel
state the refusal test relies on, and qualify the repeat-cancel rule.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… tests (#194 review round 12)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…review round 13)

Merges main (#195, #196, #197) and updates the status lines this PR
rewrites; tests that a held cancel refusal also survives Refresh.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…onAsking (#194 review round 14)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mchwang and others added 8 commits October 20, 2026 23:23
Add the step list's Waiting and Running marks to DESIGN.md (approved),
an AGENTS.md rule for focus when a background update hides the focused
control, cite #199 for the rest of D16 and DT7, save the waiting checks
only when they change, and correct the onAsking comment.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…omment, and add Skipped to DESIGN.md (#194 review round 16)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s placement, and test focus when a reload shows the job ended (#194 review round 17)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nd the list surviving the timer (#194 review round 18)

Also log the step bar's placement in DESIGN.md and name the F6b queue
test in the handoff doc's test list.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…hem, and test each way (#194 review round 19)

Reads the focused control before hiding it, since hiding blurs it at
once. Also drops the redundant first save of the waiting checks and
widens DESIGN.md's step-bar log entry.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…focus on it (#194 review round 20)

Also test the fallback to Merge PR when a reload clears the last
blocker, and word the test fixture's revision note as order only.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…final state (#194 review round 21)

Focus went to a still-disabled Merge PR (without a runner) and fell to
the page. renderMerge now records the focused control, renders, then
picks Merge PR only when shown and enabled, otherwise Refresh. The
AGENTS.md rule now says the fallback must be shown and enabled and a
control's visibility is decided once per update.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mchwang and others added 7 commits October 21, 2026 03:08
…nd test the disabled fallback (#194 review round 22)

Also word the fallback as "not disabled (aria-disabled qualifies)" in the
rule and the comment, drop the second write of Merge status's
visibility, and document every focus move in merge-handoff.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lable, not disabled (#194 review round 23)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nd keep Cancel pending until the poll answers (#194 review round 24)

Also replace the last "disabled Cancel" and "until Refresh" wording.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…b as running (#194 review round 25)

A poll already in flight could answer the job's older running record
and clear the wait early. The in-flight test now checks Cancel after the
held poll's answer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… not call onAsking, and note restart downtime in step times (#194 review round 26)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…own, Fetch failing for review, and a joined click (#194 review round 27)

Also name the merge step bar in DESIGN.md's Frame.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…moved head stops it (#194 review round 28)

Also list round 27's tests in the handoff doc.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants