Skip to content

fix(runtime): record final-report producer selections in the run instead of querying smithers - #1183

Merged
aviggiano merged 6 commits into
mainfrom
claude/w27-final-report-selection-record
Sep 30, 2026
Merged

aviggiano merged 6 commits into
mainfrom
claude/w27-final-report-selection-record

Conversation

@aviggiano

@aviggiano aviggiano commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

The owner approved this PR's security-posture change: after a controller restart the verifier's reference for agent_execution comes from a run-directory record instead of the Smithers database, which changes the source but not the boundary (a same-UID agent could forge either) and retires #585's claim, with no migration for runs whose producer ran under an older template.

Problem

On current main, a final-report producer retry or verifier that runs in a restarted controller (resume, quota park, supervisor relaunch, crash) cannot rebuild report.json#run_metadata.agent_execution unless a smithers executable happens to be on the operator's own PATH. When none is:

  • each producer retry fails before its agent runs, on every remaining attempt, until the report node runs out of retries, and the run has no report;
  • a verifier fails the same way. Since fix: a reworded finding no longer discards the final report #1204 the run then publishes the agent's report unchecked (PARTIAL), because unverified-report-inputs.ts accepts a node that its verifier failed (read in the code, not exercised here), and the run ends FAILED.
Task failed: node:report after 3 attempts: artifact-contract failure: Smithers report-producer authority is unavailable after 0ms against a 180000ms budget
  cause: Executable not found in $PATH: "smithers" (ENOENT)

This reproduces with the pinned Smithers (0.35.0): with main's workflow.tsx (previous round) and with the stacked base's (this round; #1201 keeps the shell-out), both variants of smithers-report-retry.integration.test.ts fail once a stub smithers that exits 97 is first on the detached engine's PATH (table under Verification). command -v smithers is empty on this host.

Overlap with #1201. This branch is stacked on #1201, which merges first. #1201 writes a <run>/trusted-bin/smithers shim so the template's bare smithers resolves, and cites #1143 point 1, so on top of it the ENOENT no longer reproduces in production. What this PR adds there is that the report path no longer spawns a runner subprocess (a --full-output read of up to 64 MiB under a 180 s budget, which #1026's contention forced). Instead it reads a file of about 30 bytes per attempt that the workflow wrote itself. See Merge notes.

Root cause

Checked on main in the previous round (a46a4960; #1229, which main has since added, does not touch these paths). On this stack, #1201 changes the third and fifth points: its shim puts a smithers first on the engine's PATH, and the integration test used that shim.

  • The generated workflow keeps the chain rung of each report-producer attempt in process-local maps. A restart loses them.
  • To rebuild them, the producer's attempt > 1 path and the verifier ran execFileSync("smithers", ["node", …, "--full-output"]) with a bare command name. This is the template's only bare smithers call.
  • The engine's PATH comes from composeSmithersCommandPath (smithers.ts). It is the run's trusted-bin, which on main holds only the ultrafuzz launcher, followed by the operator's PATH with target-local directories (such as .smithers/node_modules/.bin) removed. The controller launches its runner by absolute path, and nothing adds a directory that contains a smithers executable to PATH.
  • The query runs only once the in-memory cache is empty, which is after a restart.
  • The only real-Smithers test of this path prepended node_modules/.bin to the engine's PATH, and pnpm test prepends it too. So CI never ran the query without a smithers on PATH.

Change

In packages/runtime/src/templates/smithers/workflows/workflow.tsx (+54/−98):

  • Record: in each report-producer attempt, before the agent runs, the workflow writes the executed selections [{attempt, chainIndex}, …] with writeFileDurable to <realpath(runRoot)>/smithers/final-report-selections/<attempt-id>.json.
  • Read back when the process-local cache is empty:
    • Producer, attempt > 1: takes the recorded entries for earlier attempts. If Smithers dispatches the same attempt number again, the new entry replaces the old one. resume --retry-failed and --reset-node both do exactly that for a failed verifier: they issue timetravel --node-id <producer> without --attempt (smithers.ts), which resets the producer's latest attempt. The engine's nextAttemptNumber counts only attempts that are not marked reset-cancelled, so that attempt number is dispatched again. A failed preflight never reaches generate, so it is never recorded; the old code skipped those attempts too.
    • Producer, attempt 1: starts a new history, so it neither reads the record nor fails on an unreadable one; it replaces it.
    • Verifier: the producing attempt is the last recorded selection. A missing record throws report producer selection was never recorded; run `ultrafuzz resume <run-id> --refresh-controller --reset-node <node>` to rerun the producer, with the producer's workflow node ID filled in (node:final-report in the packaged topologies).
  • Recovery hint: --reset-node on the producer resets its latest attempt and its dependents. Smithers' timetravel resets dependents unless given --no-deps, and the --reset-node path passes none. Smithers counts as dependents the nodes whose attempts started after the target attempt. In every packaged topology, every node except __finish__ (which depends on it) is upstream of final-report, so for the report producer that is its verifier. The first version of this PR named --retry-failed, which resets every failed or stalled task (smithers.ts loops over smithersFailedTasks), so in a best-effort campaign with failed strategy tasks it would also have rerun each of them. --refresh-controller stays in the hint: without it, a run launched before this release goes back to its persisted workflow, which does not write the record.
  • Malformed record: the error names the file to delete (smithers/final-report-selections/<attempt-id>.json in the run directory) and the same rerun command. Only an outside edit can produce such a file, since writeFileDurable writes a temp file and renames it. Without the deletion, every re-dispatched attempt above 1 reads it again and fails.
  • Precedence: controller memory outranks edits made after that controller read the record, so a verifier in the producer's own controller uses memory, not the record. A producer attempt dispatched after a restart starts from the record.
  • Validation: the reader checks the record's shape. finalReportAgentExecution still checks attempt order and chain bounds, so the same rules are not written twice.
  • Deleted: readFinalReportSmithersAuthority, priorFinalReportAgentSelections, the 180 s SMITHERS_REPORT_PRODUCER_AUTHORITY_TIMEOUT_MS budget, and the workflow's imports of inspectSmithersAttemptAgentSelection and reconcileSmithersAttemptAgentSelection.
    • The runtime package still exports both. Host-side workflow-sync.ts uses inspectSmithersAttemptAgentSelection.
    • Workflows rendered before this change import both from the installed runtime, so a plain resume of an in-flight run keeps its old code path.
    • After this PR, reconcileSmithersAttemptAgentSelection has no caller in the repository except its own tests (see Risk).
  • Unchanged: the in-process checks for "selection changed within an attempt" and "attempt moved behind its history". refactor!: remove per-node cloud execution (execution.mode = "cloud") #1197 removed per-node cloud workers, so every producer attempt writes the record and the verifier has no single-rung cloud shortcut.

Docs and CHANGELOG:

This PR reverses #585's test "a non-Codex fallback cannot forge final-report producer authority through the run filesystem" and deletes it. That property was never a real boundary:

  • Agents run as the same user as the controller. The Claude and DeepSeek agent templates set permissionMode: "bypassPermissions" and OpenCode sets yolo: true, so an unsandboxed agent can edit the Smithers database (<target>/smithers.db) or state.json.
  • The record is easier to edit than the database: it is a JSON array of about 30 bytes at a documented path, while the database is SQLite rows, with a WAL, that the live engine holds open. That lowers the effort of forging (a prompt-injected agent needs one echo), not the boundary.
  • trusted-cli.ts already says the host is "reproducible execution evidence, not a same-UID sandbox boundary".
  • The database and the new record are both outside the agent's worktree (<run>/workspaces/<attempt-id>) and its declared artifact roots. The Codex template sets sandbox: "workspace-write" and the workflow passes addDir: [task.artifactDir, ...dependencyArtifactDirs]. From reading the code, a sandboxed Codex agent is therefore not given either location as a writable root, unless the project lives under /tmp or $TMPDIR. Codex's workspace-write leaves both writable by default, and codex.tsx sets no exclusion. run.output_dir must be project-local, so the record and the database are always exposed together. I did not verify the sandbox experimentally.

The other half of that deleted test, that controller memory outranks the on-disk source, still holds for edits made after the controller read the record, and report-retry-history.test.ts checks it. A producer attempt dispatched after a restart fills memory from the record. So an earlier attempt's agent that rewrote the record before the restart (for example to erase its own failure before a quota park) has that history carried into the prompt authority, and the in-process verifier accepts it. A security review confirmed this with a probe against the extracted helpers; it is the same #585 property.

Deliberately not built (and why)

Verification

command -v smithers is empty on this host. This round ran on the stacked head c9b278a7 after pnpm install --frozen-lockfile, pnpm -w build and a clean dist-test rebuild. Everything after it is from earlier rounds, before this stacked rebase, and says which head it ran on where the earlier description did. Since then the report path changed only in dropping the local-mode guard and the single-rung cloud verifier shortcut, which #1197's removal of execution from task specs required.

This round

Discriminating evidence. I wrote the stacked base's workflow.tsx (#1201's 5689f7f3, which still shells out) into the worktree, ran this PR's two test files against it, and restored it. The previous round got the same failures with origin/main's workflow.tsx.

Test base workflow.tsx (#1201) this branch
smithers-report-retry.integration.test.ts fallback variant (real Smithers 0.35.0; a stub smithers that exits 97 is first on PATH) fail: Smithers report-producer authority is unavailable after 1ms against a 180000ms budget, caused by Command failed: smithers node node:report … with the stub's bare smithers resolved from PATH and "status":97 pass (10.2 s)
same, quota-park variant fail: same pass (9.6 s)
report-retry-history.test.ts: a restarted producer and a restarted verifier rebuild agent_execution from the run, and controller memory outranks the record fail: Smithers report-producer authority is unavailable after 0ms …, caused by ReferenceError: execFileSync is not defined (the harness injects no execFileSync) pass
… a re-dispatched attempt number replaces its own entry, on a different rung fail: same pass
… a single-rung chain keeps quota-exempt retries across restarts fail: same pass
… a missing or malformed record fails the report instead of inventing a producer fail: the never-recorded assertion gets Smithers report-producer authority is unavailable after 0ms …, with the same cause pass
… ordinary in-process retries keep their history and reject a backwards attempt pass (unchanged behaviour) pass

Mutation checks, each applied to workflow.tsx, run, and then reverted:

  • Putting back the task.execution.mode reads that refactor!: remove per-node cloud execution (execution.mode = "cloud") #1197 made unsafe (the local guard on the read and the write, and the single-rung cloud shortcut in the verifier) fails all five history tests with TypeError: Cannot read properties of undefined (reading 'mode'), because the fixtures no longer carry execution.
  • Putting back only the write guard fails both integration variants: node:report failed 3 consecutive attempts with an identical error: undefined is not an object (evaluating 'task.execution.mode').
  • Before the fixture change, both files hard-coded execution: { mode: "local" }, so such a leftover would have passed them and only the cli-e2e lane would have caught it.

What I ran on the stacked head:

  • report-retry-history.test.js: 5/5. The cloud test is deleted (see Rebase notes).
  • smithers-report-retry.integration.test.js: 2/2.
  • 17 other runtime test files that read the template or cover report authority: 258/258.
    • generated-workflow-verifier, generated-workflow-render, generated-workflow-footprint
    • invariant-suite-{ancestor-order,handoff-durability,enumeration-overflow}
    • workspace-patch-{supersede,replay}, workspace-preparation-lifecycle, stale-workspace-cleanup-overflow
    • dynamic-lifecycle, dynamic-workflow, pinned-submodules
    • terminal-report-projection, task-workflow-identity, workflow-dependency-policy, smithers-attempt-authority
    • cloud-worker-handoff, which the previous round also ran, is gone with refactor!: remove per-node cloud execution (execution.mode = "cloud") #1197.
  • The three other real-Smithers integration files that run template code, smithers-{preparation-race,dependency-skip,resume-reopen}.integration: 9/9.
  • runtime.test.ts with --test-name-pattern for the six resume tests named under the targeted recovery below: 6/6.
  • packages/cli/test/e2e/campaign-resume.test.ts, the whole cli-e2e lane (pnpm --filter @ultrafuzz/cli test:e2e runs only this file): 1/1 on this head (273 s). Its final-report node declares report@3 and nonempty-markdown@1 and succeeds on attempt 1, so the run goes through the now unconditional record write and the in-memory verifier, and the report ends available, complete and verified. It does not reach the restart read, which the integration test covers.
  • The whole workflow.tsx transpiled with Bun's tsx loader, the loader the engine uses.
  • The CI gates, all exit 0:
    • pnpm -w format:check
    • pnpm -w lint
    • CI=1 ESLINT_PLUGIN_DIFF_COMMIT=origin/claude/v10-install-based-controller pnpm -w lint:strict:ci
    • pnpm -w knip after pnpm -w build, and again with every dist removed
    • pnpm --filter @ultrafuzz/runtime typecheck
    • node scripts/docs-check.mjs
  • pnpm typecheck covers only src/**/*.ts, so it does not type-check workflow.tsx. ESLint checks the template for undefined names, and the template's runtime coverage comes from the Bun-run integration tests and the e2e run.

Not run this round: the rest of runtime.test.ts, the CLI unit suite, the scratch recovery and scratch e2e variants below, and a live campaign with real model agents.

Earlier rounds

The targeted recovery against real Smithers 0.35.0 (previous round, on 3a1d3430). This is a scratch variant of smithers-report-retry.integration.test.ts and is not committed.

  1. The producer failed on attempt 1 and succeeded on attempt 2. The record read [{"attempt":1,"chainIndex":0},{"attempt":2,"chainIndex":1}]. I deleted it and resumed.
  2. The restarted verifier failed the run with report producer selection was never recorded; run `ultrafuzz resume <run-id> --refresh-controller --reset-node node:report` to rerun the producer.
  3. I sent the timetravel that resume --reset-node node:report sends for a producer that is not itself failed: --node-id node:report --no-vcs --force, with no --iteration. It reset ["node:report","verify:report"] and nothing else.
  4. On resume, Smithers dispatched producer attempt 2 again on rung 1 and the record became [{"attempt":2,"chainIndex":1}]. After one more resume the verifier passed, and the run finished. The four phases that log (three producer attempts and the passing verifier) ran in four distinct controller pids. The rerun's agent_execution lists no failed attempts: attempt 1 went with the deleted record, the kind of gap the docs paragraph now names.

The ultrafuzz side of --refresh-controller --reset-node comes from runtime.test.ts. an unobserved failed occurrence stays in the attempt ledger across a stopped refresh passes resetNode and refreshController together, and it passes along with these tests:

start-run.ts passes the path rendered by --refresh-controller into the same runSmithersLifecycleCommand call that carries resetNode. The round before that ran the same recovery through the timetravel that --retry-failed issues (--iteration 0, no --no-deps), with the same result.

Mutation checks, each applied to workflow.tsx, run, and then reverted:

  • On 3a1d3430, each of these fails only the malformed-record test:
    • dropping the attempt > 1 guard, after which a first attempt fails on an unreadable record instead of replacing it;
    • the hint naming task.attemptId instead of task.smithersNodeId;
    • the hint keeping --retry-failed;
    • dropping the hint from the shape-check throw;
    • dropping the hint from the parse-failure throw.
  • The round before: the verifier ignores controller memory and always reads the record, and only the first history test fails, on controller memory outranks the run record. A re-dispatched attempt keeps its old recorded rung, and only the re-dispatch test fails.

End to end through the ultrafuzz CLI, with a producer retry (two rounds back; since then, the code it ran changed only in error text and, in this round, in dropping the local-mode guard). This is a scratch variant of packages/cli/test/e2e/campaign-resume.test.ts and is not committed. The only differences are that the stub codex holds final-report instead of summarize, plus checks on the record and on the report. Each command runs as its own process on the pinned engine: init, run, a SIGKILL of the engine, the supervisor's relaunch, a SIGKILL of the whole controller, then resume, status, report and stats.

  • Relaunch: the relaunched engine dispatched producer attempt 2 on rung 1 with an empty cache. The record then read [{"attempt":1,"chainIndex":0},{"attempt":2,"chainIndex":1}], and the agent started. On main, that attempt would shell out to the bare smithers before its agent starts. That comes from the code; I did not run this variant against main.
  • Resume: resume started a fresh controller, which dispatched attempt 3 on rung 2, again with an empty cache, and the attempt completed.
    • The record read [{"attempt":1,"chainIndex":0},{"attempt":2,"chainIndex":1},{"attempt":3,"chainIndex":2}].
    • The published report's agent_execution lists failed attempts 1 and 2 and producer attempt 3.
    • status reports the run succeeded, with the report available, complete and verified, and every check in the original test passed (406 s).
  • Not covered here: the verifier ran in the same controller as attempt 3, so it used controller memory. The verifier's read after a restart is covered by the integration test.

The integration test keeps its existing assertions:

  • failed_attempts lists attempt [1], and the producer is attempt 2.
  • Chain indexes are [0,1] for the fallback variant and [0,0] for the quota variant.
  • The verifier does not rewrite the report bytes.
  • The three phases run in three distinct controller pids.

The detached engine's PATH starts with a stub smithers that prints to stderr and exits 97, and the fixture's own pause call uses the runner's absolute path.

Deleted tests that pinned the removed implementation:

  • the loadFinalReportAgentExecutionAuthority harness with a fake execFileSync
  • the feat(runtime): add error-agnostic agent retries #585 forge test (its memory-precedence half is now checked in report-retry-history.test.ts)
  • the single-rung cloud smithersReads() === 0 test
  • the 180 s budget test
  • three source-regex asserts: the reconcileSmithersAttemptAgentSelection(task, authority.authorityDetail call, the detail.ok === true && … envelope unwrap, and the absence of an agent-execution / execution.json record

What I ran on 3a1d3430 (on main a46a4960):

  • report-retry-history.test.js: 6/6
  • smithers-report-retry.integration.test.js: 2/2
  • 17 other runtime test files that read the template or cover report authority: 274/274.
    • generated-workflow-verifier (116) and cloud-worker-handoff (14)
    • invariant-suite-{ancestor-order,handoff-durability,enumeration-overflow}
    • workspace-patch-{supersede,replay}, workspace-preparation-lifecycle, stale-workspace-cleanup-overflow
    • dynamic-lifecycle (19), dynamic-workflow, generated-workflow-footprint, pinned-submodules
    • terminal-report-projection, task-workflow-identity, workflow-dependency-policy, smithers-attempt-authority
  • The three other real-Smithers integration files that run template code, smithers-{preparation-race,dependency-skip,resume-reopen}.integration: 9/9
  • runtime.test.ts with --test-name-pattern for the six resume tests above: 6/6
  • The whole workflow.tsx transpiled with Bun's tsx loader, the loader the engine uses.
  • The CI gates, all exit 0:
    • npx prettier --check on the changed files, and pnpm -w format:check
    • pnpm -w lint
    • pnpm -w knip before the build (every dist removed) and after pnpm -w build
    • CI=1 ESLINT_PLUGIN_DIFF_COMMIT=origin/main pnpm -w lint:strict:ci
    • pnpm --filter @ultrafuzz/runtime typecheck
    • node scripts/docs-check.mjs

Not run on 3a1d3430:

  • the rest of runtime.test.ts and the CLI unit suite;
  • the cli-e2e lane on 3a1d3430. A review ran campaign-resume.test.ts on 594d6b87: 1/1, 309 s. Its report producer succeeds on attempt 1, so it does not reach the lines that round changed.
  • a live campaign with real model agents.

Risk / compatibility

  • Security posture (owner-approved). After a restart, the verifier's reference value for agent_execution comes from a run-directory file, not the Smithers database. A same-UID unsandboxed agent could forge either one, so this changes the source, not the boundary, although the file takes less effort to forge. It retires feat(runtime): add error-agnostic agent retries #585's claim that the run filesystem cannot be used to forge producer provenance. Controller memory outranks edits made after that controller read the record; a producer attempt dispatched after a restart starts from the record.
  • In-flight runs (no migration, per owner policy). Plain resume runs the workflow persisted at launch (start-run.ts reads run.json#workflow.path), and nothing on the resume path rewrites that pointer. --refresh-controller renders into .smithers/continuations/<uuid>/ and passes that path only to its own lifecycle call. So existing runs keep the old code, including the shell-out. Only resume --refresh-controller renders this template, and a later plain resume goes back to the launch workflow.
  • Resets and provenance. After a reset reuses attempt numbers, a post-reset attempt that fails before recording (for example in preflight) leaves the pre-reset entry for its number in place. Later attempts then report that entry in failed_attempts. This affects provenance only; the run does not stop. The docs paragraph now states this gap and the refresh gap. Greptile raised the same gap; it is declined here (see Rebase notes).
  • New I/O. Each report-producer attempt makes one small fsync'd write, about 30 bytes per recorded attempt. The read is capped at 1 MiB, roughly 30,000 recorded attempts.
  • Follow-ups, once no supported in-flight run can be on a pre-change template:

Merge notes

Rebase notes

Stacked rebase onto #1201 (5689f7f3). This PR's six commits moved from main a46a4960 onto origin/claude/v10-install-based-controller 5689f7f3: #1201's five commits on #1197's fourteen, on main 538b6188, which adds #1229. The pre-rebase head was 3a1d3430, and the new head is c9b278a7. This PR changes no lockfile, patch or manifest, so pnpm install --frozen-lockfile was enough. Commits 2 and 3 applied unchanged (git range-diff shows =). The conflicts, and how each was resolved:

Changes made during the rebase that no conflict forced:

  • Fixtures. I deleted execution from the history test's Task type and taskFixture (with its mode option), and the execution: { mode: "local" } line from the integration fixture's task. A leftover task.execution read now fails both files (mutation check under Verification).
  • Tests. I deleted "a cloud worker keeps no run record and its single-rung verifier needs none", which cannot pass once every attempt records. I also renamed "a local single-rung report keeps quota-exempt physical retries across restarts" to drop "local".
  • Docs. Commit 4 no longer adds "local" or the sentence "A cloud worker, which currently runs a single-rung chain once, keeps no record." The paragraph says "Before each producer attempt starts its agent".
  • CHANGELOG entry. It now says "which each producer attempt writes" instead of "which each local producer attempt writes". "Instead of shelling out to a bare smithers node, which Ultrafuzz never put on the controller PATH" became "instead of running smithers node as a subprocess (a --full-output read of up to 64 MiB under a 180 s budget)". The sentence that started "Unless the operator's own PATH had a smithers" is gone, since feat(runtime)!: run lifecycle commands from the pnpm-patched install instead of per-command npm installs (#921 step 1) #1201 fixes that failure and claims it.
  • Commit messages. I reworded commits 1, 4, 5 and 6 where they described cloud mode or a controller PATH without a runner. Commit 4's subject lost "and qualify upgrade claims", because what is left of that commit changes only the template.
  • This description. I updated the line references to the stacked tree: start-run.ts:583 and :630, smithers.ts:8023, workflow.tsx:418.

Greptile (review of 3a1d3430):

  • P2, workflow.tsx:3188 (now line 2236), "Stale failed-attempt history": declined. The observation is right. After a reset that reuses attempt numbers, a new-round attempt that fails before recording (in preflight, say) leaves the old round's entry for its number, and later attempts list it in agent_execution.failed_attempts. It affects provenance only: the producer's prompt and the verifier read the same record, so the run does not stop. The code comment, the docs paragraph and Risk ("Resets and provenance") already state it. A fix needs a host-side hook into the reset path to clear the record, which "Deliberately not built" explains.

Earlier rebase. Rebased onto origin/main a46a4960, 52 commits past the old base b6dd1da9. The pre-rebase head was cef55b10 and the rebased head is 594d6b87. Four of its five commits were kept, and one CHANGELOG commit was added. The review fixes are one commit on top of 594d6b87.

  • packages/runtime/test/generated-workflow-verifier.test.ts: this PR deletes three tests: the feat(runtime): add error-agnostic agent retries #585 forge test, the single-rung cloud restart test and the 180 s budget test. test(runtime): delete vacuous and source-text tests, keep behavioural coverage #1203 deleted the neighbouring source-text test generated retries do not inspect or inject previous failure text. I kept both deletions and dropped the closing brace the two hunks shared. The loadFinalReportAgentExecutionAuthority harness and the three source-regex asserts applied cleanly.
  • CHANGELOG.md: the entry that the docs commit added under Other changes conflicted with main's new entries. The branch's last commit (chore: move the changelog entry to the consolidated release notes) had removed that entry again. I kept main's file through the rebase, dropped that commit, and added one entry at the top of ### Breaking changes in a new commit.
  • No conflicts: workflow.tsx, report-retry-history.test.ts, smithers-report-retry.integration.test.ts and docs/reference/artifacts-reports.md applied cleanly.
    • At 594d6b87, every changed line in these files, and in the verifier test, was identical to the pre-rebase branch.
    • The template hunks sit at main's lines 3119–3270, plus two import lines; the docs paragraph is at line 281.
  • Install: no lockfile or patch changes, so pnpm install --frozen-lockfile was enough.

Refs #1143, #585

🤖 Generated with Claude Code

RetriggerConfidence Score: 4/5

The PR does not appear safe to merge while the previously reported cloud cleanup defect remains outstanding.

Fix All in Claude CodeFindings

  1. P1 Cloud resources survive cleanup ▶
  2. P2 Stale failed-attempt history ▶
Fix with agent prompt
### Issue 1
packages/runtime/src/clean.ts:undefined-85
When an operator cleans a run launched with per-node Modal execution, `cleanRun` now deletes the local run directory without terminating its sandboxes or deleting its volume. Those remote resources can remain billable, while the deleted `plan.json` held the Modal app information needed to locate them for manual cleanup.

### Issue 2
packages/runtime/src/templates/smithers/workflows/workflow.tsx:2235-2237
If a reset reuses an attempt number and that attempt fails in preflight, it never updates the record. The next attempt keeps the old entry through this filter, so `agent_execution.failed_attempts` can name a model that did not run in the current round. This is a non-blocking provenance inaccuracy that can mislead readers comparing model runs.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

The PR records final-report producer selections in the run directory so a restarted controller can rebuild agent_execution without spawning smithers. It adds recovery guidance and tests, and documents the changed provenance and security posture. No changes were made since the previous review.

Diagram
sequenceDiagram
  participant P as Report producer
  participant R as Run-directory record
  participant V as Verifier
  P->>R: Write selected attempt and chain rung
  P->>P: Run agent and remember selection
  alt Same controller
    V->>V: Compare report with remembered selection
  else Restarted controller
    V->>R: Read recorded selections
    V->>V: Compare report with reconstructed execution
  end
Loading

Reviews (3) · Last reviewed commit: "fix(runtime): name a targeted recovery f..."

@aviggiano
aviggiano force-pushed the claude/w27-final-report-selection-record branch from cef55b1 to 594d6b8 Compare September 30, 2026 08:00
@aviggiano
aviggiano marked this pull request as ready for review September 30, 2026 09:56
@aviggiano
aviggiano requested a review from a team as a code owner September 30, 2026 09:56
Comment on lines +3185 to 3188
const previous =
cached ??
(local && attempt > 1 ? readFinalReportSelections(task).filter((selection) => selection.attempt < attempt) : []);
const selections = [...previous, { attempt, chainIndex }];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Stale failed-attempt history If a reset reuses an attempt number and that attempt fails in preflight, it never updates the record. The next attempt keeps the old entry through this filter, so agent_execution.failed_attempts can name a model that did not run in the current round. This is a non-blocking provenance inaccuracy that can mislead readers comparing model runs.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/runtime/src/templates/smithers/workflows/workflow.tsx
Line: 3185-3188

Comment:
**Stale failed-attempt history** If a reset reuses an attempt number and that attempt fails in preflight, it never updates the record. The next attempt keeps the old entry through this filter, so `agent_execution.failed_attempts` can name a model that did not run in the current round. This is a non-blocking provenance inaccuracy that can mislead readers comparing model runs.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code

@aviggiano
aviggiano force-pushed the claude/w27-final-report-selection-record branch from 3a1d343 to c9b278a Compare September 30, 2026 14:09
if (cloudCleanup !== undefined) {
return runtimeFailure([cloudCleanup]);
}
try {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Cloud resources survive cleanup When an operator cleans a run launched with per-node Modal execution, cleanRun now deletes the local run directory without terminating its sandboxes or deleting its volume. Those remote resources can remain billable, while the deleted plan.json held the Modal app information needed to locate them for manual cleanup.

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/runtime/src/clean.ts
Line: 85

Comment:
**Cloud resources survive cleanup** When an operator cleans a run launched with per-node Modal execution, `cleanRun` now deletes the local run directory without terminating its sandboxes or deleting its volume. Those remote resources can remain billable, while the deleted `plan.json` held the Modal app information needed to locate them for manual cleanup.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code

aviggiano and others added 6 commits September 30, 2026 15:31
…ead of querying smithers

After a controller restart (resume, quota park, supervisor relaunch) the
generated workflow's process-local record of which chain rung each
report-producer attempt used is gone. The producer's retry prompt and the
verifier both rebuilt it by running a bare `smithers node ... --full-output`
from inside the workflow: a runner subprocess on the finalizer path that
reads up to 64 MiB under a 180 s budget. Wherever the controller PATH
resolved no `smithers`, every such attempt failed with "Smithers
report-producer authority is unavailable" (cause: ENOENT) until the report
node ran out of retries (#1143). The only real-Smithers test of this path
gave the detached engine a `smithers` on its PATH.

Record each executed selection {attempt, chainIndex} with writeFileDurable
in <run>/smithers/final-report-selections/<attempt-id>.json before the agent
runs, and read it back when the process-local cache is empty. A
re-dispatched attempt number replaces its own entry;
finalReportAgentExecution keeps validating attempt order and chain bounds.
This deletes readFinalReportSmithersAuthority,
priorFinalReportAgentSelections, the 180 s
SMITHERS_REPORT_PRODUCER_AUTHORITY_TIMEOUT_MS budget and the workflow's use
of the Smithers attempt reconcilers. The runtime exports stay: host-side
workflow-sync uses inspectSmithersAttemptAgentSelection, and workflows
rendered before this change import both.

Per-node cloud execution is gone (#1197) and rendered task specs no longer
carry `execution`, so every producer attempt writes the record and the
verifier has no single-rung cloud shortcut. The history and integration
fixtures carry no `execution` either, so a leftover `task.execution` read
in the template fails both.

This reverses #585's "a non-Codex fallback cannot forge final-report
producer authority through the run filesystem" test. That property was not
a real boundary: an unsandboxed agent running as the same user can edit the
Smithers database as easily as a run-directory file, and trusted-cli.ts
already states the host is not a same-UID sandbox boundary. Both the
database and the new record sit outside the agent's worktree and declared
artifact directories.

Tests: the real-Smithers restart test now runs the detached engine with a
PATH that resolves no `smithers` (and calls pause by absolute path); with
the previous template both variants fail with ENOENT, with this change both
pass. report-retry-history.test.ts is rewritten to drive the extracted
helpers across simulated restarts against a temporary run directory. The
tests that pinned the CLI query (fake execFileSync, read counts, the budget,
source regexes) are deleted.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
The artifacts reference said the verifier compares agent_execution with
"independently persisted Smithers attempt authority ... without trusting a
model-writable file". It now compares with controller memory or, after a
restart, the run directory's selection record; say where that record lives
and that, like the Smithers database, it is host evidence rather than a
boundary against an unsandboxed same-user agent.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
…on a bare smithers

Review follow-ups for the final-report selection record tests:

- The first history test now also checks that a verifier in the producer's
  own controller returns what that controller observed after the record is
  rewritten. The #585 forge test that covered controller-memory precedence
  was deleted with the Smithers query; without this, a verifier that always
  re-read the record passed every test.
- The re-dispatch test now dispatches attempt 2 again on a different rung,
  so it tells replacing the recorded entry apart from keeping it. A real
  Smithers timetravel of a failed report verifier's producer re-dispatches
  the latest attempt number, which is the case this models.
- The real-Smithers integration test no longer filters PATH entries that
  hold a `smithers`. That filter also dropped `bun` on hosts where a global
  `smithers` sits beside it, and the node_modules/.bin shim then failed with
  `exec: bun: not found`. A stub that exits 97 now goes first on the
  detached engine's PATH instead, so a bare `smithers` call fails even on a
  host that has one.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
A run launched before the selection record existed keeps its persisted
workflow on plain `resume`; only `resume --refresh-controller` renders the
recording template. If that refresh lands after the report producer
succeeded, the verifier finds no record and fails. The error now says to run
`ultrafuzz resume <run-id> --refresh-controller --retry-failed`, which resets
the verifier's producer and reruns it under the current controller, so the
rerun writes the record.

A comment notes that after a reset reuses attempt numbers, an attempt that
fails before recording leaves the old round's entry for its number in place.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ng change

New entries now go straight into CHANGELOG.md under Unreleased, so the
entry this branch had moved to consolidated release notes comes back. It
sits under Breaking changes rather than Other changes: a run whose report
producer succeeded under an earlier release's workflow and that is resumed
with --refresh-controller before its verifier runs now fails that verifier
until `resume --refresh-controller --retry-failed` reruns the producer, and
the reference docs retire #585's claim that the run filesystem cannot be
used to forge the producer. Main already files changes without a `!` whose
only break is to older runs there (#1176, #1193).

The entry also says that plain `resume` keeps the workflow persisted at
launch, and that the recovered producer's failed_attempts omits the
attempts from before the refresh. It describes what the replaced query cost
(a runner subprocess with a 64 MiB read under a 180 s budget) rather than a
missing `smithers` on PATH, since the run's `trusted-bin/smithers` shim now
gives the controller PATH a runner on every host, and it says every producer
attempt writes the record, since per-node cloud workers are gone.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ort record

The verifier's "never recorded" error told operators to run
`resume --refresh-controller --retry-failed`, but `--retry-failed` resets
every failed or stalled task, so in a best-effort campaign it would also
rerun each failed strategy task. Both record errors now name
`resume <run-id> --refresh-controller --reset-node <producer node>`, which
resets the producer's latest attempt and its dependents, here its verifier.
A scratch run against the pinned Smithers confirmed that this timetravel
resets exactly node:report and verify:report and that the re-dispatched
attempt rewrites the record.

A malformed record blocked every re-dispatched attempt above 1 without
naming a way out; its error now names the file to delete. The history test
now asserts both errors' exact text and pins that a first attempt replaces
an unreadable record instead of failing on it.

The docs paragraph states the two cases where failed_attempts is inexact
and that a Codex workspace-write agent can reach the record when the
project lives under /tmp or $TMPDIR. The CHANGELOG entry names the mixed
plain and refreshed resume failure with its recovery. The integration
test's poison-stub comment no longer describes the controller PATH, which
the run's trusted-bin/smithers shim now gives a runner.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@aviggiano
aviggiano force-pushed the claude/w27-final-report-selection-record branch from c9b278a to 1fde8c7 Compare September 30, 2026 15:31
@aviggiano
aviggiano merged commit 39f4a18 into main Sep 30, 2026
15 of 16 checks passed
@aviggiano
aviggiano deleted the claude/w27-final-report-selection-record branch September 30, 2026 15:31
aviggiano added a commit that referenced this pull request Sep 30, 2026
…shim

Problem: a Breaking-changes entry announced that launch, resume, replay and
fork no longer write <run>/trusted-bin/smithers. No release wrote it: #1201
added it after v0.1.2, and this branch already rewrote #1201's entry without
it. Measured against v0.1.2, nothing the entry lists changes: replay and fork
never refused an unpatched install, and a run launched by an earlier release
already found its persisted workflow's bare `smithers` only on the
operator's PATH.

Change: remove the entry. Its one piece of advice moves to the #1183 entry,
which already explains that plain resume keeps running the persisted
workflow: such a run still runs `smithers node` in a restarted controller,
so continue it with `resume --refresh-controller`. The #1183 entry already
gives the --reset-node form for a report producer that has succeeded.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
aviggiano added a commit that referenced this pull request Sep 30, 2026
Problem: #1201 wrote <run>/trusted-bin/smithers at launch, resume, replay
and fork so that the generated workflow's bare `smithers node` call
(#1143) found a runner. #1183 removed that call: final-report producer
retries and verifiers now read the selections each producer attempt
records in the run. The current template spawns only git, bash and the
run's `ultrafuzz` launcher, and every command the runtime spawns runs an
executable it bound by path, never `smithers` from PATH.
smithers-report-retry.integration.test.ts puts a failing `smithers` first
on the engine PATH around the production report code and still passes.
The shim therefore served only workflows persisted by earlier releases,
and it put an engine CLI first on every task's PATH. It also made replay
and fork bind the installed runner, which they otherwise never run: they
run the run's sealed engine.

Change: delete writeTrustedSmithersShim and its three callers. When resume
cannot re-verify the trusted CLI, it again keeps an existing run launcher
first on PATH itself, which the shim had done as a side effect; the
existing test for that path covers it. Launch still binds the installed
runner before creating a run, because resume runs it; the comments and
the doctor summaries now say launch and resume. The capability and
native-continuation tests drop their shim assertions, and a resume test
now asserts that trusted-bin holds only the ultrafuzz launcher. The docs
no longer say that the shim serves the workflow's smithers calls or that
tasks can drive their run through it. The CHANGELOG entry for #1201,
which has not been released, drops its shim claims, and a new entry
records the removal.

BREAKING CHANGE: tasks no longer find a `smithers` CLI that Ultrafuzz
provides on PATH. A run launched by an earlier release and continued by
plain `resume` still runs its persisted workflow, whose bare `smithers`
call in a restarted controller finds a runner only on the operator's own
PATH, as before #1201; continue such a run with
`resume --refresh-controller`.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
aviggiano added a commit that referenced this pull request Sep 30, 2026
…shim

Problem: a Breaking-changes entry announced that launch, resume, replay and
fork no longer write <run>/trusted-bin/smithers. No release wrote it: #1201
added it after v0.1.2, and this branch already rewrote #1201's entry without
it. Measured against v0.1.2, nothing the entry lists changes: replay and fork
never refused an unpatched install, and a run launched by an earlier release
already found its persisted workflow's bare `smithers` only on the
operator's PATH.

Change: remove the entry. Its one piece of advice moves to the #1183 entry,
which already explains that plain resume keeps running the persisted
workflow: such a run still runs `smithers node` in a restarted controller,
so continue it with `resume --refresh-controller`. The #1183 entry already
gives the --reset-node form for a report producer that has succeeded.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
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.

1 participant