[JacobStephens2]

Summary

Custom runner manifest + mapped scripts/skills
  → SHA-256 frozen Arm + Git provenance
  → unattended request.json → committed HEAD + result.json
  → code Graders + Pair evidence

Either Arm accepts --*-runner on explicit process suites. Two Custom runners need no thirdshift checkout, build cache or Harness. Setup format 13 records nullable commit/Harness plus runner identity; malformed results, unexplained exits and committed excluded paths fail one Trial.

Provider sandbox adapters, Console start parity and session recording retain their sibling-ticket scope (#2189, #2190, #2191). record_command is reserved as null until #2191.

Closes #2188

Evidence

Execution evidence

• Before: cargo test --test eval_arms custom_runners_freeze_dirty_sources_and_home_trees_without_building -- --exact failed with expected a full lowercase commit object ID: .../custom.json.
  After: the same command passed; all 11 Arm-preparation tests pass.
• Before: cargo test --test experiments custom_runner::a_self_reported_void_cannot_hide_committed_excluded_paths -- --exact --nocapture returned void instead of failed, retrying three times.
  After: the same command passed; committed excluded paths fail once.
• Before: cargo test --test experiments custom_runner::nonzero_exit_without_outer_void_evidence_fails_despite_a_runner_void_claim -- --exact --nocapture returned void instead of failed.
  After: the same command passed, retaining exit 7 and stderr.
• Before: cargo test --test experiments custom_runner::custom_runner_can_use_the_host_prepared_rust_toolchain -- --exact --nocapture failed with prepared Rust toolchain is not available in this HOME.
  After: the same command passed through the shared toolchain proxies.
• Before: cargo test --test experiments custom_runner::custom_runner_reports_ignored_uncommitted_scratch_in_trial_diagnostics -- --exact --nocapture failed because .scratch/private.txt was absent from diagnostics.
  After: the same command passed; committed-HEAD grading still passes.
• Before: cargo test --test experiments custom_runner::custom_runner_preserves_empty_mapped_folders_in_bundle_and_home -- --exact failed its mapped-directory assertion.
  After: the same command passed, including directory SHA-256 entries.
• Before: cargo test --test experiments experiment_records_both_arms_the_bar_logs_stages_and_each_trial -- --exact stored a hash instead of the entered candidate reference.
  After: the same command and all five Experiment naming tests passed.
• Final validation: CI passed on f140e259, including all four Linux Rust partitions, their exact-coverage gate, site and Console checks. The Console job passed on its failed-job re-run; its exact local testcase also passed. Headless CARGO_BUILD_JOBS=4 NEXTEST_TEST_THREADS=3 RUST_TEST_THREADS=3 scripts/affected-tests refs/remotes/origin/issue-1468 </dev/null passed 69 site tests and 205 Console tests; the duplicate local Rust run was stopped after successful CI covered the same commit, following docs/testing.md. All 15 Custom runner tests, the native Rust-pin test and cargo clippy --all-targets -- -D warnings passed locally.

Seams under test

• thirdshift evals prepare-arms: frozen manifests/files, provenance, confined targets and build independence.
• thirdshift experiment ... --json: mixed Arms, committed-HEAD grading, Pair evidence, protocol failure classification, outcomes, identity/resume and frozen-input verification.

Visual evidence

Paired Chromium captures cover the Console Experiment page and the homepage, architecture, workflow and generated manual pages at 1440×900 and 390×844. Secret-Gist publication failed with HTTP 404 because the available GitHub token lacks the gist scope; the captures remain local in the Session workspace.

Flow	Desktop Before → After	Mobile Before → After
Console Experiment	/tmp/before-desktop.OwT7p5.png → /tmp/after-desktop.dRwGv9.png	/tmp/before-mobile.An4vRN.png → /tmp/after-mobile.zxHbdR.png
Homepage Experiments	/tmp/before-home-desktop.ozS2Pv.png → /tmp/after-home-desktop.NrK6S2.png	/tmp/before-home-mobile.rdTYTX.png → /tmp/after-home-mobile.YAPBe4.png
Architecture	/tmp/before-architecture-desktop.ZxCYWf.png → /tmp/after-architecture-desktop.9NknHc.png	/tmp/before-architecture-mobile.bmrX2o.png → /tmp/after-architecture-mobile.qhDmlv.png
Workflow	/tmp/before-workflow-desktop.wBvO0m.png → /tmp/after-workflow-desktop.EIrd00.png	/tmp/before-workflow-mobile.VMibeH.png → /tmp/after-workflow-mobile.eUnFZ1.png
Manual Experiments	/tmp/before-manual-desktop.g00Elt.png → /tmp/after-manual-desktop.9s7A7A.png	/tmp/before-manual-mobile.CKT1Wy.png → /tmp/after-manual-mobile.HK5RtX.png

Review coverage

Independent Standards and Spec reviews used refs/remotes/origin/issue-1468 (6e650c4f2f84111c74e0c3e87e5ae786e590d9ce) as the fixed point. Fresh reviews and their follow-ups covered final head f140e259. Cumulative findings: Standards 1, Spec 4, all fixed. Every behavioral finding's exact exercising command failed before repair and passed afterward; the tests are retained.

The Standards axis intentionally left site/docs/index.html unread as generated source. It reviewed docs/guide.md and site/generate-docs.mjs, independently passed npm run docs:check --prefix site, and inspected the rendered desktop/mobile manual captures. Spec directly read the generated changed hunks. Standards accounts for all 38 changed paths as 37 read plus one confirmed generated exclusion; Spec read all 38. No other unread file or repair hunk remains.

Unaddressed findings

Standards

None.

Spec

None. The nonzero-exit rule follows #2188's explicit acceptance criterion requiring outer Void evidence.

Security

None.

Security review outcome

No unaddressed introduced findings.

Merge Danger

Door: two-way

Format 13 changes the result contract for Experiment consumers. Custom runner provider isolation and recording follow in #2189/#2191 on the parent feature branch.

Blast Radius: Experiments

Built with codex codex-cli 0.167.2 · gpt-6.1-sol · xhigh

[JacobStephens2]

Review run. The branch moved; no fixes were pushed. Proposed fixes remain in the local worktree. Spec axis used the pull request body alone; no closing issue was linked.

Review of PR #2194 (fixed point 6e650c4f, Spec: issue #2188)

The review fixed one Spec bug and four Standards findings. Final validation then exposed a regression in the PR's own code: a native Candidate's candidate_ref showed the resolved commit, breaking 4 experiments tests. I fixed that too. After both fix commits, scripts/affected-tests passes in full.

The review input file had no text from #2188, only the PR body. I fetched #2188's title, body and labels with gh issue view 2188 --json title,body,labels, without comments, and used that as the Spec.

Standards

No hard violations of a documented standard. All eight findings are judgement calls:

1. Repeated Switches / Primitive Obsession: whether an Arm is a runner or native is spread across Option fields and placeholder values, with about 27 runner.is_some()/is_none() checks (src/evals/experiment.rs, arms.rs, options.rs, execution.rs). The reviewer suggested an ArmKind enum.
2. Silent default: Setup::choice now falls back to Claude (src/evals/experiment.rs:192), and Custom runner Arms get a made-up Claude Choice for the Trial Monitor.
3. Duplicated Code: Runner repeats Process's freeze, verify and install logic (arms/runner.rs:125-173 vs arms/frozen.rs).
4. Duplicated Code: the Custom runner failure message is built in three places (custom.rs and two sites in execution.rs).
5. Mysterious Name:
   • executor()/reference() put a runner name under "Harness" or commit labels.
   • arms::Runner clashes with trial::Runner.
   • runner_result and process_setups are unclear.
6. Duplicated Code: the same "all tasks are process tasks" guard appears twice in model_grader.rs.
7. Soft documented guidance: docs/code-navigation.md has no row for Custom runner (docs/agents/domain.md; the code-navigation header says "Keep these pointers current").
8. Duplicated Code in tests: the force-add-scratch runner body appears twice in tests/experiments/custom_runner.rs.

Files read: CODING_STANDARDS.md, AGENTS.md, .agents/skills/codebase-design/SKILL.md, docs/agents/domain.md, docs/code-navigation.md, docs/testing.md, src/evals/experiment/trial.rs, tests/experiments/process.rs, and all 37 changed files other than site/docs/index.html.

Confirmed exclusion: site/docs/index.html is generated output. The reviewer checked its source docs/guide.md (the generator site/generate-docs.mjs is unchanged), and npm run docs:check --prefix site exited 0. The #experiments anchor the new links point to exists.

Unread changed files: none.

Spec

Every requirement in #2188 is implemented, and there is no meaningful scope creep. Two findings:

1. A Custom-runner Baseline is labelled "main (merge base)". At src/evals/experiment.rs:511, baseline_ref falls back to "main (merge base)" even though no merge base is computed for a Custom runner Baseline. This conflicts with the spec's "key accurately off the Custom runner".
   • Test: cargo test --test experiments custom_runner::a_custom_runner_baseline_is_not_described_as_the_main_merge_base
   • The reviewer also noted, as a cosmetic aside, that the default name for two different runners embeds both full 64-character digests.
2. The defaulted HEAD Baseline isn't pinned for resume. With a native Baseline and a Custom runner Candidate, --baseline defaults to HEAD, and resume_command keeps the unpinned argv. After any new commit, resuming starts a new Experiment instead.
   • Test: cargo test --test experiments custom_runner::candidate_runner_resume_keeps_the_defaulted_head_baseline_across_commits

Files read: all 38 changed files, including site/docs/index.html, inspected directly. Supporting files: .review-logs/issue-2188.json, site/generate-docs.mjs, src/evals/experiment/grading/process.rs, src/evals/experiment/pairs.rs, src/evals/format.rs.

Exclusions: none. Unread changed files: none.

What I fixed

Commit d60125a6:

• Spec 1: I ran the test as written and it failed (left: "main (merge base)", right: "fixture"). A Custom runner Baseline now records its runner reference. The test is kept and passes.
• Standards 4: one shared custom::failure_cause / recorded_failure_cause now builds the message at all three sites.
• Standards 6: a single needs_grader helper replaces the two guards.
• Standards 7: added a "Custom runner, frozen Arm preparation" row to docs/code-navigation.md.
• Standards 8: a shared force_committed_scratch() test helper.

Commit 6ef16cff, found by validation rather than by either reviewer:

• The PR set candidate_ref to the resolved commit for native Candidates, where it used to keep the ref the user gave. Four experiments tests failed on this, for example "Release d4295…" instead of "Release candidate".
• Only a Custom runner Candidate now uses its runner reference.

Unaddressed findings

Standards

• 1 (ArmKind enum): declined. It would restructure the serialized format-13 Setup, Arm and Requested types, which is too large and risky for a review fix. The current behaviour is covered by format-13 tests.
• 2 (Claude fallback in choice):
  • Declined on native Arms: Requested::new (src/evals/arms.rs:33) already rejects unknown Harness names, so they can't silently run as Claude.
  • Declined on runner Arms: the Monitor only reads trial-*-review*.jsonl session logs (execution/monitor.rs:29-35). Custom runners only run process suites, which never write those logs.
• 3 (Runner/Process freeze duplication): declined. The two install paths have different targets (bundle or mapped HOME versus the process bundle), so sharing them is a refactor beyond this review. Judgement call.
• 5 (naming): declined. The renames are judgement-call churn across the Console, cost and pairwise code. arms::Runner and trial::Runner live in separate modules.

Spec

• 2 (unpinned HEAD Baseline on resume): I ran the test as written and it failed: the second run tried to build a new Baseline Arm at the new commit. Freeze Custom runner manifests and run Custom-runner Trials on process suites #2188 explicitly asks for "--baseline defaults to HEAD". Omitted --baseline (merge base) and HEAD commit arguments already resolve fresh on resume today, so pinning would change resume semantics for every Experiment. That is a call for the Spec or the Day shift. I filed Defaulted HEAD Baseline of a Candidate-runner Experiment is not pinned for resume #2195 (needs-triage only) and removed the test.
• 1, cosmetic aside (full digests in the default name): not changed. It is display-only, and the reviewer only suggested it.

Review coverage

No changed file is left unread on either axis. site/docs/index.html is excluded on the Standards axis with the evidence above; the Spec reviewer read it directly. The reports are in .thirdshift-review-Bl28dJ/standards.md and spec.md. summary.json is {"standards":8,"spec":2}.

Validation

Command	Result
npm run docs:check --prefix site (after npm ci --prefix site)	exit 0
cargo test --test experiments custom_runner:: (with both Spec reproducers)	exit 101: both reproducers failed as reported
same, after the fixes	exit 0, 13 passed
cargo clippy --all-targets -- -D warnings	exit 0
scripts/affected-tests refs/remotes/origin/issue-1468, first run	exit 1: Console tests failed because console/node_modules was missing (a setup gap)
same, second run (after npm ci --prefix console)	exit 1: site, build and Console passed; cargo test failed 4 experiments tests on the candidate_ref regression, and the test binaries after experiments did not run
cargo test --test experiments, after commit 6ef16cff	exit 0, 219 passed
scripts/affected-tests refs/remotes/origin/issue-1468, final run	exit 0: site, cargo build --locked, Console and full cargo test all passed

The extra command list was empty. Validation logs are in .review-logs/ and .affected-tests/.

Per axis: Standards raised 8 findings (worst: the Option-based runner/native split, #1); Spec raised 2 (worst: the wrong baseline_ref for a Custom runner Baseline, now fixed).

Security

No unaddressed introduced findings.

Delivery failed

the branch moved during Review run