Skip to content

fix(dispatcher): resume ended pipelines from their threads - #2324

Merged
justinhelmer merged 1 commit into
mainfrom
plan/fix-https-github-com-cor-8b8f2c/u1
Sep 23, 2026
Merged

justinhelmer merged 1 commit into
mainfrom
plan/fix-https-github-com-cor-8b8f2c/u1

Conversation

@coreplane-switchboard

@coreplane-switchboard coreplane-switchboard Bot commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

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

  1. Durable remaining-budget calculation Subtracts elapsed wall time and completed review rounds from persisted caps, failing closed on incomplete or contradictory receipts. ⚠ Bad arithmetic can replenish or prematurely exhaust a pipeline.
  2. Deterministic continuation door Resolves the durable ended owner, including a concurrent winner, and bypasses the operator before any model or command runs. ⚠ A missed owner could execute substitute work before admission.
  3. Authority and generated-task identity fences Requires owner authority plus durable thread, plan, repository, and branch identity before continuation state can run. ⚠ A loose fence could resume another requester's pipeline.
  4. Mandatory pull-request continuation inputs Requires a recorded pull request and expected head before GitHub is read or the original task can be reissued. ⚠ An optional input here would bypass the remote-head gate.
  5. Attributable remote-head receipt Combines one branch-ref read with the PR payload and emits a receipt only when repository, branch, and full SHA agree. ⚠ Fallback data must never masquerade as verified remote state.
  6. Exact durable-head enforcement Requires open same-repository PR facts and an attributable receipt equal to the unit branch and persisted head. ⚠ Failing open could run superseded or unreviewed work.
  7. Production-path continuation truth table Covers missing durable PR/head inputs alongside unknown, stale, moved, mismatched, and exact GitHub states with no bypass.

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)
  • Require a recorded pull request before continuation. Letting a branch-only unit resume was rejected because no PR/ref/durable-head agreement can be proven without durable PR identity. Missing PR or expected head now blocks before GitHub I/O and replacement work.
  • Unify existence and tip into one read. Keeping separate requests was rejected because a successful existence response could lend authority to a later failed tip lookup. One typed ref result owns missing, unverified, or verified-with-SHA state.
  • Require PR/ref agreement. Falling back to the PR payload or tolerating temporary PR/ref lag was rejected for continuation: neither proves the expected branch currently points at the durable head. Disagreement blocks until GitHub converges.
  • Require durable SHA equality. Treating any verified SHA on the expected branch as current was rejected because a force-push or later commit changes the work identity. The receipt must equal persisted lastPush.
  • Bypass the operator for deterministic continuation. Folding operator output was rejected because the provider and read tools had already run before deterministic gates. An ended owner fixes the operation: authorize and resume the durable task or name the blocker.
  • Recognize the historical owner behind a live winner. A concurrent continuation can put a live row ahead of the ended ship row. Rechecking finished rows with the same cached unit read identifies that origin while admission folds duplicates.
  • Derive remainders only from durable receipts. Reparsing the original budget directive was rejected because it recreates spent capacity. Persisted caps, unit start/end times, and review boundaries survive restarts.
  • Allow policy to tighten but never replenish. The resumed runner takes the minimum of current policy and its durable remainder. Ignoring current policy could exceed a lowered boundary; preferring it could restore consumed capacity.
Validation (7 criteria)
Criterion Proof
Missing durable pull-request identity cannot bypass exact-head verification Red: focused production matrix — 1 failed/13 passed because missing PR produced no blocker. Green: the same command — 14 passed, including missing PR and expected head with zero GitHub I/O.
Remote truth table, owner routing, budgets, caps, and duplicate fencing npx vitest run on dispatcher, operator, ship, thread, and GitHub-pulls tests — 5 files, 718 passed after rebase.
Changed TypeScript compiles NODE_OPTIONS=--max-old-space-size=6144 npx tsc --noEmit -p tsconfig.json — passed after rebase.
All 13 changed files retain repository formatting npx prettier --check on the 13 final changed files — passed after rebase.
Specs and public-tree hygiene remain valid npm run hygiene:check and npm run specs:check — passed; 51 specs and 5,839 proof references checked.
Every changed source path remains spec-covered npm run specs:coverage -- --changed origin/main...HEAD --test-guard --require — passed; no verification removed without its spec.
Pull-request title follows the changelog rule 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.pr from 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. fetchPullRequestFacts still uses one typed ref read and emits verifiedHead only 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

Comment thread src/core/dispatcher.ts Fixed

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-https-github-com-cor-8b8f2c/u1 branch from ad5c319 to a3d00bd Compare September 23, 2026 17:00
@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-https-github-com-cor-8b8f2c/u1 branch from a3d00bd to 3ac35ae Compare September 23, 2026 17:14
@coreplane-switchboard coreplane-switchboard Bot changed the title fix(dispatcher): resume ended pipelines from owning threads fix(dispatcher): preserve budgets when resuming ended pipelines Sep 23, 2026
@justinhelmer

Copy link
Copy Markdown
Contributor

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.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-https-github-com-cor-8b8f2c/u1 branch from 3ac35ae to 0ad7461 Compare September 23, 2026 17:37
@justinhelmer

Copy link
Copy Markdown
Contributor

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.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-https-github-com-cor-8b8f2c/u1 branch from 0ad7461 to 99e1b6e Compare September 23, 2026 17:56
@justinhelmer

Copy link
Copy Markdown
Contributor

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.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-https-github-com-cor-8b8f2c/u1 branch from 99e1b6e to c2fa4fc Compare September 23, 2026 18:12
@coreplane-switchboard coreplane-switchboard Bot changed the title fix(dispatcher): preserve budgets when resuming ended pipelines fix(dispatcher): resume ended pipelines from their threads Sep 23, 2026
@justinhelmer

Copy link
Copy Markdown
Contributor

Re-review requested at c2fa4fc — fixed F1 by requiring headBranchExists === true as well as exact persisted-head equality; absent, false, undefined, unreadable, moved, or mismatched remote state now blocks before operator/model, commands, or pipeline creation, with focused production-path proofs. All current-head CI is green; the 11:17 AM PDT audit reports zero unresolved inline threads and no current-head findings, with only exact-head approval pending.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-https-github-com-cor-8b8f2c/u1 branch from c2fa4fc to e9f04b1 Compare September 23, 2026 18:35
@justinhelmer

Copy link
Copy Markdown
Contributor

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.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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>
@coreplane-switchboard
coreplane-switchboard Bot force-pushed the plan/fix-https-github-com-cor-8b8f2c/u1 branch from e9f04b1 to 5d799e5 Compare September 23, 2026 18:51
@justinhelmer

Copy link
Copy Markdown
Contributor

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.

@coreplane-switchboard coreplane-switchboard Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@github-actions github-actions Bot 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.

Auto-approved: coreplane-switchboard[bot] reviewed this PR and posted an LGTM verdict (see its review). This repository opted in through its REVIEW_BOT_LOGIN and REVIEW_BOT_ID variables.

@justinhelmer
justinhelmer merged commit 438f898 into main Sep 23, 2026
30 checks passed
@justinhelmer
justinhelmer deleted the plan/fix-https-github-com-cor-8b8f2c/u1 branch September 23, 2026 19:14
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