fix(dispatcher): resume ended pipelines from their threads - #2324
Conversation
There was a problem hiding this comment.
Changes requested: Continuation restores identity and fencing, but it resets consumed pipeline budgets instead of preserving the remainder.
Warning
Changes requested · head ad5c319 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — thread-admission.md item 9: continuation resets rather than preserves remaining budget | src/core/dispatcher.ts:1088 |
Full review
F1: Re-parsing the original durable request restores its full budget: directive, and the new coordinator attempt receives fresh wall-clock and review-round caps. For example, a 180-minute pipeline aborted after using 170 minutes can receive another 180 minutes on “continue.” Carry the durable remaining wall-clock and round budgets into the new attempt, rejecting continuation when either is exhausted, and add a partial-budget regression.
ad5c319 to
a3d00bd
Compare
a3d00bd to
3ac35ae
Compare
|
Re-review requested at 3ac35ae — continuation now carries only durable remaining wall-clock and review-round budgets; the CodeQL unused-assignment inline comment is fixed, replied to, and resolved; all current-head CI is green. Please verify the full ended-thread continuation, authorization, budget, remote-head, ambiguity, and duplicate-fencing contract. |
There was a problem hiding this comment.
Changes requested: Ended-thread blockers and duplicate attempts still reach the operator before authorization and fencing.
Warning
Changes requested · head 3ac35ae · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — docs/reference/specs/routing-and-config.md item 29: blocked continuations run the operator before authorization and fencing | src/core/dispatcher.ts:824 |
Full review
F1 — With routing.operator: on, operatorStage runs before ended-pipeline authorization, ambiguity/budget/task/head checks, and thread admission. An unauthorized, blocked, or concurrent duplicate reply can therefore spend a provider turn and execute intermediate read tools before being rejected; concurrent replies can each do this before one claims the thread. Move continuation preflight and durable fencing ahead of the operator, or bypass the operator for this deterministic continuation path.
3ac35ae to
0ad7461
Compare
|
Re-review requested at 0ad7461 — fixed F1 by resolving continuation-shaped ended-thread ownership and deterministic authorization/ambiguity/budget/task/head/duplicate blockers before operatorStage; focused regressions prove blocked and concurrent attempts never invoke the model/operator or substitute commands. The scoped gates pass. Current-head CI has one independently proven repeat 5-second memoryWorkerDiagnostics timeout and a still-running memory Worker verify; neither touches this diff and no blind retry has occurred yet. |
There was a problem hiding this comment.
Changes requested: The operator ordering is fixed, but ended-pipeline continuation still does not enforce the required exact PR head.
Warning
Changes requested · head 0ad7461 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — thread-admission.md item 9: continuation accepts a moved PR head | src/core/dispatcher.ts:1154 |
Full review
F1 — The PR check validates only that some SHA exists on the expected branch; it never compares facts.headSha with the unit’s durable lastPush. The regression even records 111…, returns remote 222…, and expects continuation. A branch changed after the pipeline ended can therefore restart coding instead of failing the exact-head fence required by thread-admission item 9. Require a persisted expected head and exact equality before reissuing the pipeline, with a regression that rejects the mismatched SHA without invoking the operator or ship branch.
0ad7461 to
99e1b6e
Compare
|
Re-review requested at 99e1b6e — fixed F1 by requiring the persisted expected PR head and exact equality with fresh remote facts before continuation; moved, missing, or mismatched heads block before operator/model, commands, or pipeline creation. Added the production-path regression. All current-head CI is green and unresolved inline threads are zero. |
There was a problem hiding this comment.
Changes requested: The continuation head fence still fails open when GitHub cannot verify the branch ref directly.
Warning
Changes requested · head 99e1b6e · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Require positive branch-head verification before continuation | src/core/dispatcher.ts:1164 |
Full review
F1 — headBranchExists === undefined currently passes the fence. In production, fetchPullRequestFacts can return that state when direct ref verification fails, then fall back to the potentially stale PR-object SHA. If the branch moved while GitHub still reports the persisted SHA, continuation can start against an unverified head. Require headBranchExists === true and add a regression where the ref lookup is unavailable but the PR payload still carries the expected SHA.
99e1b6e to
c2fa4fc
Compare
|
Re-review requested at c2fa4fc — fixed F1 by requiring |
There was a problem hiding this comment.
Changes requested: The continuation gate still accepts an unverified stale SHA when the branch-tip lookup fails after existence succeeds.
Warning
Changes requested · head c2fa4fc · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Remote-head verification still accepts a stale PR SHA when the ref tip is unreadable | src/core/dispatcher.ts:1164 |
Full review
F1 — Major, high confidence: fetchPullRequestFacts performs separate ref-existence and ref-tip requests. If existence returns true but the subsequent tip request times out, returns malformed data, or fails, it falls back to the potentially stale PR-object SHA. When that stale SHA equals lastPush despite the branch having moved, this gate passes and starts the continuation without a positively verified remote head. Bind existence and SHA to one successful ref read, fail closed when the tip is unreadable, and add a production-path regression for this partial-read failure.
c2fa4fc to
e9f04b1
Compare
|
Re-review requested at e9f04b1 — replaced split existence/tip reads with one attributable PR/ref head receipt and require repo, branch, and SHA to match durable continuation state. The production-path truth table now fails closed for existence/tip missing, unknown, malformed or errored; stale/moved/mismatched PR and branch heads; branch/PR disagreement; and concurrent/repeated replies. Exact verified agreement is the sole continuation path. All current-head CI is green; the 11:40 AM PDT audit reports zero inline threads/current-head findings and only exact-head approval pending. |
There was a problem hiding this comment.
Changes requested: Exact-head safety is still bypassed when an ended generated unit has not recorded a pull request.
Warning
Changes requested · head e9f04b1 · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Continuation without a recorded PR bypasses exact-head verification | src/core/dispatcher.ts:1137 |
Full review
F1: When owner.unit.pr is undefined—such as an abort before the PR was recorded—the entire remote-head gate is skipped and execution proceeds to reissue the pipeline. shipPreflight does not close this gap because its ref lookup is advisory and cannot prove PR/ref/durable-SHA agreement. Require a recorded PR and exact attributable head receipt for every continuation, otherwise fail closed before runShipBranch.
Co-Authored-By: coreplane-switchboard[bot] <318072483+coreplane-switchboard[bot]@users.noreply.github.com>
e9f04b1 to
5d799e5
Compare
|
F1 disposition at 5d799e5: fixed. Continuation now requires both durable recorded PR identity and expected head before any GitHub read or continuation side effect. The production-path matrix proves missing PR, missing head, malformed/failed/unreadable remote receipt, stale/moved/mismatched heads, and repeated/concurrent replies all fail closed before operator/model, substitute command, pipeline, work, review, or PR creation. Focused suites and all current-head CI pass. Exact-head re-review is required; the prior approval/finding state is not being reused. |
There was a problem hiding this comment.
LGTM: The prior fail-open continuation gap is fixed and the current head has no remaining review findings.
Note
Approved · head 5d799e5 · no findings
Full review
The prior fail-open continuation gap is fully resolved at the current head; no findings remain.
Ended generated pipelines resume only when their durable task, remaining budgets, recorded pull request, expected head, and one attributable remote receipt all agree. Missing or contradictory facts now stop before any model, command, or replacement work.
Why: Issue #2265 exposed an ended ship thread accepting an informational command instead of recovering. The fresh review found optional inputs and split ref reads could bypass exact-head safety, extending the RCA after PR #2310 added head reconciliation.
Where to look
Feedback wanted: Please scrutinize the mandatory durable inputs, single-read receipt boundary, and exact PR/ref/durable-head agreement, plus the placement of every blocker before side effects.
Risk: A bad fence could block valid resumes or duplicate stale work. This is a 1,207-line diff; splitting admission, budgets, and remote verification was rejected because a partial continuation path remains unsafe. Rollback is one commit.
Verified: 718 focused tests plus changed-set types, formatting, hygiene, specs, coverage, and title gates passed after rebase. GitHub CI and human re-review remain gating.
Decisions (8)
Validation (7 criteria)
npx vitest runon dispatcher, operator, ship, thread, and GitHub-pulls tests — 5 files, 718 passed after rebase.NODE_OPTIONS=--max-old-space-size=6144 npx tsc --noEmit -p tsconfig.json— passed after rebase.npx prettier --checkon the 13 final changed files — passed after rebase.npm run hygiene:checkandnpm run specs:check— passed; 51 specs and 5,839 proof references checked.npm run specs:coverage -- --changed origin/main...HEAD --test-guard --require— passed; no verification removed without its spec.npm run check:pr-title -- "fix(dispatcher): resume ended pipelines from their threads"— passed.For agents
Rebased the single coherent commit onto origin/main c873c50 and force-pushed full head 5d799e5. The new red proof deletes
owner.unit.prfrom the persisted unit; old code skipped the full remote gate and returned no blocker. The production matrix now starts with missing durable PR and missing expected head, asserts zero GitHub I/O, and retains PR read unknown/error; ref missing/unknown/error/malformed; PR head missing/stale/moved/mismatched; PR/ref disagreement; wrong branch; exact agreement; and verified concurrent repeats.fetchPullRequestFactsstill uses one typed ref read and emitsverifiedHeadonly when the PR object and commit ref agree. Existing authorization, ownership, grants, ambiguity, task, budget, and duplicate fences remain. Current-head CI and human re-review remain gates.Requested by @justinhelmer in slack:C0BRRHKFLCB
🤖 Generated with Claude Code