feat(harness): publish exact trees through typed push - #2203
coreplane-switchboard[bot] wants to merge 3 commits into
Conversation
![coreplane-switchboard[bot]](https://avatars.githubusercontent.com/u/264571901?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: The typed push path can abort valid shadow-mode ship runs and does not safely recover or restore publication receipts.
Warning
Changes requested · head 57dfbc9 · 4 findings: 4 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Shadow mode rejects every successful legacy ship publication | src/core/ship/coordinator.ts:1776 |
| major | F2 Ambiguous push success is recorded as a permanent refusal without reconciliation | src/core/runEffects.ts:223 |
| major | F3 Resume restores the oldest successful publication receipt | src/core/dispatch/runLoop.ts:991 |
| major | F4 The changed-set gate ignores repository-declared verification commands | src/core/runEffectsGit.ts:91 |
Full review
F1 — High confidence. harness.effects defaults to shadow, where legacy pushes remain authoritative and typed receipts intentionally do not publish pushed_head by: "runner". This unconditional check rejects the resulting by: "push" record, so a successful ship coding round opens its PR and then aborts before review. Keep accepting matching legacy publication evidence while shadow mode is active, or carry the rollout mode into the coordinator decision.
F2 — High confidence. If git push updates the remote but its response is lost, perform() throws and the code immediately persists transport_refused. Every later call with that effectId returns the refusal without checking the destination, leaving a successfully published branch without its receipt. Reconcile the destination ref and SHA after an ambiguous transport failure before recording a refusal.
F3 — High confidence. Object.values(...).find() restores the first successful push receipt, not the latest. After a run publishes more than once and resumes, the stale SHA fails the PR post-step’s publication comparison; because a publication exists, the legacy remote-equality fallback is also disabled. Restore the newest applicable receipt by occurredAt or persisted sequence.
F4 — High confidence. The adapter claims to run repository-declared changed-set gates but instead assumes root-level Vitest, TypeScript, Prettier, hygiene, and specs commands. An onboarded Python repository—or a Node monorepo whose tests and tsconfig live in a workspace—cannot publish when effects are enabled, regardless of its configured resident verification commands. Resolve the repository’s declared changed-set gate configuration and execute that closed command set instead of synthesizing Switchboard-specific commands.
There was a problem hiding this comment.
Changes requested: The typed push path can lose or misclassify effect receipts, and its new runner publication records fail the durable record validator.
Warning
Changes requested · head 57dfbc9 · 5 findings: 4 major, 1 minor
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — run-history.md item 2: runner push records are rejected | src/core/runRecord.ts:1043 |
| major | F2 Spec contradiction — harness.md item 12a: the envelope is not persisted before resolve | src/core/dispatch/runLoop.ts:1015 |
| major | F3 Ambiguous push success is made a terminal refusal without reconciliation | src/core/runEffects.ts:223 |
| major | F4 An effectId can be reused for a different command and receive the old receipt | src/core/runEffects.ts:164 |
| minor | F5 Resume restores the first publication receipt instead of the current one | src/core/dispatch/runLoop.ts:991 |
Full review
F1 — pushed_head events with by: "runner" are now emitted and folded into RunRecord.pushed, but the structural validator accepts only "push" and "salvage". Such records become unreadable through validated stores and conflict with run-history item 2. Accept "runner" and add a round-trip validator regression.
F2 — persistEnvelope only mutates memory and queues the void ledgerRun.setState() call; it proceeds immediately, and runs without a ledger proceed with no durable write at all. A crash between publication and the queued state flush loses the envelope needed to reconcile the effect, contrary to harness item 12a’s persist-before-resolve guarantee. Publication must fail closed until durable persistence is acknowledged.
F3 — Any exception from perform, including a successful git push followed by a lost response or failed ls-remote, becomes a recorded transport_refused. That refusal is authoritative on retry, so the remotely published branch can never be reconciled into a receipt and downstream PR/ship handling aborts. Reconcile refs/heads/<branch> against the intended SHA after ambiguous transport failure before recording a refusal.
F4 — The envelope is overwritten before the prior result is read, and the prior result is validated only by effectId and effect kind. Reusing an ID for a different head, base, or branch therefore returns the old success as if the new command ran. Persist the first envelope immutably and reject any retry whose complete command differs.
F5 — On resume, Object.values(effectState.results).find(...) selects the earliest successful publication. If a run published, made more changes, published again, and restarted before post-processing, the stale receipt fails the current-head check and suppresses PR handling despite the later valid receipt. Restore the latest receipt matching the admitted branch and current head.
57dfbc9 to
52d3d1d
Compare
|
Re-review requested at 52d3d1d — both arms' findings at 57dfbc9 addressed in one round: shadow mode keeps today's publication evidence for ship readiness and only |
There was a problem hiding this comment.
Changes requested: The typed push path remains bypassable from child bash and can persist false terminal refusals around ambiguous publication outcomes.
Warning
Changes requested · head 52d3d1d · 3 findings: 3 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — docs/reference/specs/routing-and-config.md item 34: child bash can still publish outside the typed effect | src/core/harness/pi/toolRules.ts:216 |
| major | F2 A receipt write failure can turn a successful push into a durable refusal | src/core/runEffects.ts:224 |
| major | F3 A failed reconciliation read permanently poisons the effect id | src/core/runEffects.ts:229 |
Full review
F1 — High confidence. Effects-on still blocks pushes by parsing shell text, while writable resident worktrees retain GitHub credentials. A command such as G=git; "$G" push origin HEAD:<branch> bypasses potentialChildPush() and publishes directly, violating the typed seam. Remove publication credentials from child-executable commands and expose credentialed publication only through a runner-owned operation.
F2 — High confidence. The inner try includes both perform() and succeed(), so a recordResult() failure after a successful push is treated as a transport failure. If reconciliation finds the pushed SHA but the second receipt write also fails, the outer catch can subsequently persist transport_refused, leaving the remote updated with a durable refusal. Catch only publication transport failures; receipt-persistence failures must remain unrecorded and propagate for recovery.
F3 — High confidence. Reconciliation errors are converted to undefined, making an unavailable remote indistinguishable from a confirmed mismatch. The resulting transport_refused receipt becomes authoritative for that stable effect ID, so every retry returns the refusal without reconciling again—even if the original push succeeded. Persist refusal only after a successful reconciliation proves the destination differs; leave failed reconciliation retryable.
There was a problem hiding this comment.
Changes requested: The typed push cutover has four paths that can strand or erase an otherwise valid publication.
Warning
Changes requested · head 52d3d1d · 4 findings: 4 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — docs/reference/specs/harness.md item 12a: coding runs without a ledger receive no typed push seam | src/core/dispatch/runLoop.ts:1038 |
| major | F2 Default-bound coding runs cannot publish a newly created feature branch | src/core/runEffectsGit.ts:110 |
| major | F3 An unavailable reconciliation is cached forever as a transport refusal | src/core/runEffects.ts:229 |
| major | F4 Reusing an effect id overwrites its successful receipt | src/core/runEffects.ts:184 |
Full review
F1: With harness.effects: on and run history disabled, ledgerRun is absent, so the push tool receives no effects capability while shell pushes are still refused. Every publication becomes impossible. Either reject this configuration or provide the durable effect store that item 12a promises for every coding harness.
F2: A coding run initially attached to the default branch commonly creates a feature branch before publishing. Passing that initial branch as admittedBranch makes authorization reject the new branch, even though the legacy guard explicitly permits any non-protected branch in this case. Preserve strict matching for explicitly named and PR-bound refs, but allow a default-bound run’s validated run-owned branch.
F3: If git push may have succeeded but the following ls-remote reconciliation also fails transiently, the catch converts the unavailable read into a durable transport_refused. The same effectId then returns that refusal forever without reconciling again, potentially leaving a published remote ref with no receipt. Distinguish an unavailable reconciliation from a confirmed mismatch and keep the effect retryable under the same ID.
F4: A different command reusing an ID calls refuse(), whose recordResult() replaces the result stored under that ID. If the original command already succeeded, this erases its authoritative receipt; subsequent correct retries return the reuse refusal, and restart recovery can no longer find the publication. Return or audit the reuse refusal without replacing the original result.
52d3d1d to
12dadbd
Compare
12dadbd to
a70626b
Compare
|
Re-review requested at a70626b — all six majors from the second review pair are addressed in one architectural round: effects-on removes the child’s publication credential, receipt persistence is outside transport handling, unavailable reconciliation stays retryable, effect-id reuse preserves the standing result, effects-on requires durable history, and default-bound branch authority is distinct from named/PR refs. CI is green at this head; dispositions and the refreshed description are submitted. |
|
Re-review requested at a70626b — the round-2 findings at 52d3d1d addressed: under |
There was a problem hiding this comment.
Changes requested: Effects-on is not fail-closed for cold executors and can be enabled without the durable ledger it requires.
Warning
Changes requested · head a70626b · 2 findings: 2 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — docs/reference/specs/routing-and-config.md item 34: cold children retain write credentials | src/execution/factory.ts:1171 |
| major | F2 Spec contradiction — docs/reference/specs/routing-and-config.md item 34: effects-on accepts configurations with no durable effect ledger | src/config/validate.ts:326 |
Full review
F1 (major, high confidence): perThreadCheckout() still derives a write-scoped credential from the coding profile, and the local, E2B, and Cloudflare executors expose it to child commands. An effects-on run that falls back from a resident—or uses a non-resident repository—can therefore bypass the advisory bash detector with an obfuscated real push. These executors also lack publishGit, so the typed push cannot succeed. Effects-on must provide read-only child credentials and a runner-owned publication transport for every supported backend, or refuse unsupported backends before execution.
F2 (major, high confidence): Validation only checks that a runHistory block exists. store: file deliberately creates no RunLedger, and a Worker configuration without its bearer also degrades to no ledger. In either case effects-on passes startup validation but cannot persist envelopes through setStateDurable; resident children simultaneously lose shell publication, making publishing impossible. Validate the effective durable ledger capability—including store type and credential availability—at deployment/startup rather than checking section presence.
There was a problem hiding this comment.
Changes requested: Effects-on publication is not yet fail-closed across fallback, startup, and restart paths.
Warning
Changes requested · head a70626b · 4 findings: 3 major, 1 minor
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — docs/reference/specs/routing-and-config.md item 34: effects-on cold sandboxes bypass runner-only publication | src/execution/factory.ts:582 |
| major | F2 Spec contradiction — docs/reference/specs/routing-and-config.md item 34: effects-on accepts run history that produces no durable ledger | src/config/validate.ts:326 |
| major | F3 Default-bound runner receipts are not restored after restart | src/core/dispatch/runLoop.ts:1029 |
| minor | F4 Effect implementation paths are outside spec coverage | src/core/runEffects.ts:1 |
Full review
F1 (high confidence): When a repository is not onboarded or resident attachment fails, this path creates the ordinary per-thread executor without an effects-only credential mode. The child therefore retains the legacy ability to push, while that executor lacks both publishGit and the resident-provided verification commands. Under harness.effects: on, either provision an equivalent read-scoped typed-effect backend or fail closed instead of falling back.
F2 (high confidence): Checking only for a runHistory section does not prove a durable ledger exists. For example, a configured worker store whose bearer is absent is downgraded to history-off during startup, leaving effects-on runs without a durable effect capability after shell publication has been disabled. Startup and deploy validation must require the effective ledger, including its credential, rather than merely the config block.
F3 (high confidence): publicationBranch comes from the resident’s original sticky binding. A default-bound run can legitimately create and publish a feature branch while that binding remains the default branch. If the process dies after persisting the effect result but before emitting its runner publication event, resume compares the feature-branch receipt against the default binding, fails to restore it, and can record only legacy push evidence that effects-on ship readiness rejects. Restore against the current checked-out branch and head, while retaining strict authority checks for named bindings.
F4 (high confidence): specs:coverage reports src/core/runEffects.ts, src/core/runEffectsGit.ts, and src/tools/push.ts as uncovered. Add these implementation paths to the appropriate changed spec headers so future changed-spec and contradiction checks include the typed-effect behavior.
|
Parked for the morning after three review rounds (57dfbc9 → 52d3d1d → a70626b; CI green at this head). Round 3's two arms both requested changes, and three majors have survived since round 1 in one form or another: (1) under |
a70626b to
b205d53
Compare
|
Re-review requested at The seam stays one shared implementation; there is no second behaviour for residents. What is new is a capability each executor declares: it keeps every repository write credential out of the child's environment, and it gives the runner a way to publish that workspace's work. The resident declares it and satisfies it today. An executor that does not declare it cannot serve a write run while Also in this round: Stated cost, which belongs in the record of this change: until a sandbox adapter exists, a repository that is not onboarded, or one whose resident attach fails, cannot do write work under |
There was a problem hiding this comment.
Changes requested: The typed push path cannot recover one successful publication failure mode, and shadow mode does not satisfy its no-ledger contract.
Warning
Changes requested · head b205d53 · 2 findings: 1 major, 1 minor
| Severity | Finding | Where |
|---|---|---|
| major | F1 A lost receipt turns a successful publication into a permanent stale-head refusal | src/core/runEffects.ts:210 |
| minor | F2 Spec contradiction — docs/reference/specs/harness.md item 12a: shadow mode records nothing without a ledger | src/core/dispatch/runLoop.ts:1047 |
Full review
F1 — If publication succeeds but recordResult fails, the workspace and remote are already at the rebased commit while the durable envelope still contains the pre-rebase expectedHead. Retrying the same effect then hits stale_head before reconciliation and durably records a refusal, leaving a real push without the runner receipt required for review. Recover an envelope with no result by reconciling its destination before the stale-head check, and add a retry regression after receipt persistence failure.
F2 — effects is bound only when ledgerRun exists. With the default shadow configuration and no run-history store, the advertised push tool therefore returns “runner effects are unavailable” and records none of the resolve/authorize/fence/gate decision promised by harness item 12a. Bind a non-durable recording implementation for shadow mode, or update the configuration contract to require a ledger there too.
There was a problem hiding this comment.
Changes requested: Typed publication cannot succeed on the resident and its two recovery paths can persist false refusals.
Warning
Changes requested · head b205d53 · 3 findings: 3 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Spec contradiction — harness.md item 12a: clean resident worktrees are always refused as dirty | src/core/runEffectsGit.ts:74 |
| major | F2 Spec contradiction — harness.md item 12a: receipt-write recovery records stale_head after a successful push | src/core/runEffects.ts:210 |
| major | F3 Spec contradiction — harness.md item 12a: unavailable reconciliation becomes a durable transport refusal | src/core/runEffectsGit.ts:84 |
Full review
F1 — High confidence. ResidentExecutor.exec() renders successful empty output as "(no output)", but facts() recognizes only "" as a clean status. Therefore every clean resident checkout is classified as dirty and effects-on publication stops before rebase, gates, or push. Preserve raw empty output for this probe or use a status command with an explicit machine-readable clean marker.
F2 — High confidence. Publication rebases the checkout before pushing. If the push succeeds but receipt persistence fails, retrying the same envelope compares the rebased HEAD with the original pre-rebase expectedHead and permanently records stale_head. Reconcile an unrecorded effect against the prepared/published SHA before applying the initial-head guard, which requires durably retaining that prepared intent.
F3 — High confidence. Executor.exec() represents ordinary git failures as exit N: text, so a network or authentication failure from git ls-remote becomes undefined, indistinguishable from a successful missing/mismatched ref. After an ambiguous push, that result is persisted as transport_refused, preventing another attempt even when the push may have landed. Reconciliation must distinguish a successful ref read from command failure and return reconciliation_unavailable for the latter.
Co-Authored-By: coreplane-switchboard[bot] <318072483+coreplane-switchboard[bot]@users.noreply.github.com>
Co-Authored-By: coreplane-switchboard[bot] <318072483+coreplane-switchboard[bot]@users.noreply.github.com>
Co-Authored-By: coreplane-switchboard[bot] <318072483+coreplane-switchboard[bot]@users.noreply.github.com>
b205d53 to
f30141b
Compare
|
Re-review requested at The worktree probe no longer depends on empty output. Also in this round: the prepared intent is retained durably and an envelope with no result reconciles its destination before the initial-head guard, so a push that succeeded while receipt persistence failed recovers its receipt instead of durably recording |
There was a problem hiding this comment.
Changes requested: The typed publication path still trusts child-mutable git configuration, and restart recovery can lose a durable feature-branch receipt.
Warning
Changes requested · head f30141b · 2 findings: 2 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Privileged publication follows the child-controlled origin |
deploy/cloudflare-resident/worker.ts:7214 |
| major | F2 Spec contradiction — docs/reference/specs/harness.md item 12a: resume can ignore the durable published ref | src/core/dispatch/runLoop.ts:1042 |
Full review
F1 (major, high confidence): The privileged route pushes to origin, whose configuration remains writable by the child. A background process or repository gate can change origin after validation, causing root-owned Git to follow an unvalidated endpoint or execute a child-controlled local receive-pack command with access to the one-shot credential. Pass the validated canonical endpoint through publishGit and push directly to it without consulting worktree remote configuration.
F2 (major, high confidence): The effect receipt becomes durable before the pushedBranch state patch. If the process dies between those writes, a resumed default-bound run selects the original binding, such as main, and rejects the durable receipt for its published feature branch. Retrying the effect returns the prior result without restoring runnerPublication, so ship coordination can abort despite a successful publication. Derive candidate branches directly from durable successful receipts and match them against the current head.
There was a problem hiding this comment.
Changes requested: Effects-on removes the credential that the existing automatic checkpoint path still needs, so interrupted coding work can be lost.
Warning
Changes requested · head f30141b · 1 finding: 1 major
| Severity | Finding | Where |
|---|---|---|
| major | F1 Effects-on disables automatic salvage checkpoints | src/core/dispatch/runLoop.ts:843 |
Full review
F1 — High confidence. With harness.effects: on, resident worktrees receive only a read-scoped GitHub credential, but budget, completion, provider-failure, and compaction salvage still call salvageBudgetPush, which performs a legacy git push. That push cannot authenticate, so the checkpoint fails and resident cleanup can discard the interrupted work. Route mechanical salvage through a runner-owned publication capability and add effects-on regression coverage for these checkpoint paths.
Parked after six review rounds, for a person to decideHeads: The three findings still open are not the same kind of thing as the earlier rounds, and that is why this stops here rather than taking a seventh. 1. Privileged publication follows a child-controlled remote. 2. Turning the seam on disables the safety net. Under 3. A resumed run can reject its own durable receipt. The effect receipt becomes durable before the The reading that mattersRounds one through four were implementation defects and each was fixed. Round five was the feature not working at all: a clean worktree was classified dirty, so typed push never published, and the suite passed because the fake executors did not match the real one's output. Round six is a different class again: two of the three findings are consequences of enabling the seam rather than mistakes inside it. Salvage and the privileged remote are both things the design has to answer for, and neither was in the plan's unit one. That is a scope question, not a code question, so it belongs to a person. The choices as I see them: extend unit one to cover the salvage path and the canonical remote, which makes it materially bigger than the plan describes; or split those two out as their own unit and land what is here once the security finding is closed, accepting that No further rounds are being posted. Issue #2197 is being addressed separately and independently by #2221, which works under today's default configuration and does not depend on this. |
Closed to be redrawn: publication is privilegedSix heads, twelve review verdicts, thirty findings, no approval. Closing rather than landing and splitting, on the maintainer's decision, with the reason stated plainly: the goal is to land a change that is collapsed, elegant, clean and understandable, not to arrive there incrementally. Nothing here is lost — the branch stands, and the thirty findings below become the acceptance criteria of the redrawn unit rather than defects to be rediscovered by it. Why this one did not convergeRounds one to four were ordinary implementation defects, each fixed. Round five was the feature not working at all: The unit was scoped as "push owns the exact tree and publication receipt". What the findings keep saying is that the real subject is larger and simpler to state: publication is privileged. Everything that reached the remote had to be in scope from the start. What the redrawn unit must cover
One coherent change, one flag, and no intermediate state in which the flag cannot be switched. The invariant behind all of it: no code path publishes except through the seam, enforced rather than described. Acceptance criteria, from the thirty findings raised hereTwenty-seven major, three minor, deduplicated by subject. Credential boundary — cold executors must not retain write credentials; child bash must not publish outside the typed effect by any spelling; a clean resident worktree must be recognised as clean through the real executor's output contract; privileged publication must not follow a child-writable remote. Ambiguity and recovery — an ambiguous push success must reconcile before any refusal is recorded; an unavailable reconciliation must stay retryable rather than becoming a durable transport refusal; a failed reconciliation read must not poison the effect id; a receipt-write failure after a successful push must not become a durable refusal or a Identity and persistence — the envelope must be persisted before resolve; an effect id reused for a different command must be refused without overwriting the original receipt; runner push records must pass the durable record validator; the effect implementation paths must be inside spec coverage. Configuration — the flag must be refused where no durable ledger exists; shadow must record what the contract says it records; shadow must not reject a successful legacy publication; a default-bound run must be able to publish its own validated feature branch. Gates — the changed-set gate must resolve the repository's declared verification commands rather than assuming this repository's script names, which is issue #2211 and is the same defect seen from the runner side. RelatedIssue #2197, which unit one was meant to fix, is addressed independently by #2221 and works under today's default configuration, so it does not wait on this. Issue #2211 covers the gate-resolution finding on the child side. The plan's unit one is to be rewritten rather than amended. |
1 participant
Adds a runner-owned typed push that rebases, gates and lease-publishes one exact clean tree. Ship readiness now trusts only the runner receipt, closing shell-shape and stale-gate ambiguity.
Why: U1 implements accepted record 0074. It establishes the port and receipt ledger that U2–U4 build on.
Where to look
Feedback wanted: Please focus on exact-tree fencing around gate execution and whether the shadow-to-on receipt migration is sufficiently fail-closed.
Risk: 1,368 changed lines across the port, adapter, rollout and readiness. A split was considered, but would leave two authoritative publishers; rollback is harness.effects=shadow or reverting this PR.
Verified: 724 focused tests, scoped tsc, Prettier, hygiene, specs and docs checks passed; user-message, vocabulary and title gates passed. The deployed live receipt remains human-gated.
Decisions (3)
Validation (8 criteria)
For agents
Branch was created from and rebased onto origin/main at 9f6516d before push. Review the new effect lifecycle first, then runLoop wiring and coordinator readiness. No generated files were hand-edited. Focused repro: npx vitest run src/core/runEffects.test.ts src/core/runEffectsGit.test.ts src/core/harness/pi/toolRules.test.ts src/core/ship/coordinator.test.ts.
Requested by @justinhelmer in slack:C0BRRHKFLCB
🤖 Generated with Claude Code