feat(branch): composed snapshotting, stage-level branching, and bench eval ablate - #1046
feat(branch): composed snapshotting, stage-level branching, and bench eval ablate#1046JeremyJC67 wants to merge 47 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76d7e1bc32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| config = RolloutConfig.from_legacy( | ||
| task_path=task_path, | ||
| agent=request.agent, | ||
| model=model, | ||
| environment=request.sandbox, |
There was a problem hiding this comment.
For a task whose task.md declares benchflow.environment.manifest, this construction leaves environment_manifest=None, whereas the normal evaluation path resolves that manifest in evaluation.py:1240-1254. Consequently bench eval ablate can run the parent and every arm without the task's required image, services, provisioning, or readiness checks, so the reported comparison is for a different environment than a normal evaluation. Resolve the task-document manifest and pass it into this config.
Useful? React with 👍 / 👎.
| stitched_path = write_stitched_trajectory( | ||
| rollout_dir, | ||
| run.path / "trajectory" / "llm_trajectory.jsonl", | ||
| run.exchange_lines, | ||
| router.live_exchanges, | ||
| max_recorded=n_replay, |
There was a problem hiding this comment.
In host-proxy mode, if the agent exits or errors after requesting fewer than the configured max_exchanges, router.n_replayed_exchanges is smaller than n_replay, but this still appends all configured recorded exchanges to the output trajectory. The following usage calculation likewise classifies tokens from responses the agent never received as replayed, while the patched cut_point provenance reports the smaller served count, leaving internally contradictory and unusable experiment artifacts. Use the router's served count for the stitched prefix, usage boundary, and returned recorded count.
Useful? React with 👍 / 👎.
| finally: | ||
| if holder is not None: | ||
| holder.release() |
There was a problem hiding this comment.
When any child raises an exception other than UnscoredChildError—for example an agent connection or verifier failure—_run_children propagates through this finally, and control never reaches write_branch_artifacts below. run_ablation intentionally catches that exception and reports completed, failed, and skipped arms, but the corresponding tree.json and per-child provenance/reward artifacts are absent, so a partially completed experiment loses the evidence needed to audit its reported arms. Persist the partial tree and attached children on this failure path before re-raising.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
76d7e1b to
96b1050
Compare
|
Really nice work! I especially like the fail-closed approach around branching and restore. I ran the live Docker tests locally (
Happy to re-run the live checks after a patch. |
|
Thank you — running it live and reproducing both issues is exactly the review this needed, and both were real. Fixed at 1. Per-test attribution for in-place children ( 2. 3. Partial lineage on a raising child ( Also fixed from the automated review: Gates: targeted branch/ablate suite 340 passed, 4 skipped (+11 regression tests, each confirmed red against the old code); full suite If you're still up for re-running: the two that would confirm your findings end-to-end are Two things I deliberately left open, in case you have an opinion: |
|
Update: the stack reached its intended final form, and the headline demo landed. All four delta axes now execute. Every child is now a first-class run: in-place children synthesize their own The demo: FrontierPhysics's flagship task, real signal. The per-test rows are the physics stages: the mentor skills flip the fragile electrostatics stage (no-skill guessed 2.2 MHz off trap geometry; with-skill ran the bundled FastLap BEM solve and landed on 4.295858 MHz) and both invariant-transport stages, while the stage the base model owns unaided (Coulomb equilibrium) ties. Trajectory evidence confirms the mechanism: the no-skill child never attempted a field solve; the with-skill child read the mentor skills as its first action and followed them. Both children fork from the byte-identical One genuine bug fixed along the way ( Gates at tip: targeted suites green (25 new tests this round), full suite |
|
Thanks for the follow-up. I re-ran the review on The two issues from my original review are resolved: per-test attribution for in-place children is preserved correctly, and branch execution no longer leaks timing/verifier/usage state across arms or back into the parent. I also verified the newer paths around I found one small non-blocking edge case in From my side, the PR is ready to merge, I don’t see anything that needs to be fixed before merging @bingran-you @JeremyJC67 |
Braces, brackets and commas inside a quoted inline-JSON string value are content: the depth-only walk let a string containing close-braces zero the counter and split the spec mid-JSON. Reported in review on benchflow-ai#1046. The config:@<file> comma limit stays and is documented on --arms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
|
Thank you for the re-run — and for independently reproducing the FastLap oracle number; that makes the ion-trap demo a third-party-replicated result rather than my own claim. The @bingran-you — with @Galius5136's independent review, live re-runs and a reproduced oracle on record, both PRs are ready when you are: #1045 (RFC) and this one. Happy to squash the stack or split it per workstream if either reviews easier, and CI just needs a maintainer's approval to run on a first-time contributor's PRs. |
|
@bingran-you quick unblock request: all four CI workflows on this PR (and on #1045) are queued in |
All approved, thanks! |
|
CI attribution, now that the runs are in:
|
|
The |
|
@bingran-you progress summary, so everything is in one place before Friday: Status: complete and green.
What I need from you: the merge call on #1045/#1046 (happy to squash or split per workstream), plus the "Approve and run" click on #1045. Everything else is done. Looking forward to Friday — I'll walk through the ablation table and how it can feed the paper's attribution story. |
bc712d7 to
7d5e21e
Compare
Braces, brackets and commas inside a quoted inline-JSON string value are content: the depth-only walk let a string containing close-braces zero the counter and split the spec mid-JSON. Reported in review on benchflow-ai#1046. The config:@<file> comma limit stays and is documented on --arms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
Exact-head review: request changesReviewed exact head The core Docker path is real: composed snapshot/restore, fresh branch children, config deltas, lineage, reward artifacts, and provider telemetry all executed in an exact-head provider-backed smoke. However, the PR is not approval/merge ready and the advertised feature set is not complete end to end. Blocking findings
Fresh verification at this head
So: substantial working substrate, but request changes before approval. The critical acceptance gates are durable/auditable artifacts, truthful replay divergence evidence, canonical experiment configuration/provenance, and actual end-to-end stage/task wiring. |
…ation axis run_ablation() hand-rolled a reduced RolloutConfig, dropping the normal controls and provenance a plain eval run stamps — the real E2E parent and child configs published task_digest: null and reasoning_effort: null (PR benchflow-ai#1046 review). The request now resolves through the same two stages as `bench eval run`: build_eval_plan (normalized agent/model/effort/sandbox/ usage settings, fail-closed validation) and the newly extracted benchflow.evaluation.task_rollout_config (dataset identity, live-computed task digest, task-declared environment fallback, task source provenance) — with only the ablation-owned fields overlaid on top: the stage-capture request, the pinned no-skill parent, out-dir/job naming, and the resolved environment binding. Evaluation._run_single_task now calls the same task_rollout_config with its learner-path overrides, so the two callers cannot drift. AblationRequest gains reasoning_effort and the CLI gains --reasoning-effort (documented in cli.md); plan-validation failures re-raise as AblationSpecError before the parent run costs anything. Regression tests assert task_digest and reasoning_effort flow from a task fixture into the parent config and into both fresh-child configs, and that a bad effort/sandbox dies with nothing built. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…orting into focused modules The PR benchflow-ai#1046 review flagged ablate.py (1,203→1,320 lines) and rollout_branch.py (1,294→1,320) as monoliths, branch() at cyclomatic 21+ with 75 statements, and _run_children() taking 14 arguments. Behavior- neutral four-way split along the reviewed seams: - branch_policy.py — stage/snapshot policy: layer resolution + capability gating, capture_stage, recorded-stage resolution, and the whole delta-vector gate (validate_deltas, the per-delta stage/layer/parent/ runner preconditions, the fresh-children boundary rule). - branch_transaction.py — the transactional heart: checkpoint_parent, the scoped LinearState capture, and BranchTransaction — one dataclass carrying what _run_children took as 14 arguments, with the per-child restore/run/record/hand-off loop as methods. - branch_children.py — delta execution paths: the in-place default runner and per-child runner selection; the fresh-rollout half stays implemented in branch_skill.py (its import path and run_fresh_child patch seam are pinned by six test files) and is re-exported here. - branch_report.py — the ablation report model, arm/child pairing, per-test mining and attribution, moved out of ablate.py. rollout_branch.py keeps the branch() orchestrator — now cc≈15 / 41 statements (was cc≈30 / 72) — plus the failure-isolated lineage write, which stays because tests patch benchflow.rollout_branch.write_branch_artifacts; the transaction's fresh-runner factory and in-place result writer are injected from rollout_branch's globals for the same reason (tests/test_branch_child_result.py patches them there). ablate.py lands at 884 lines and rollout_branch.py at 441; every previously importable name keeps working via re-exports (rollout_branch re-exports the gates, exceptions, capture_stage, ChildRunner, CHILD_WALL_CLOCK_KEY, _LinearState and the fresh-child API; ablate re-exports the report and attribution API). Full suite identical before and after (6129 passed); no test edited. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
|
@bingran-you Thank you for the exact-head review — the Highlights:
Gates at tip Ready for another exact-head pass whenever you are. |
Updated exact-head re-review: request changesReviewed exact head This update materially fixes the earlier artifact-custody, canonical-config, request-digest, and ablation snapshot-retention defects. The provider-backed Docker workflow now completes end to end, including export, checksum verification, image removal, reload, and execution. However, I still found two P1 auditability defects and two P2 readiness issues. [P1] Workspace digests silently collide for legal filenames
I reproduced this with two different Alpine workspaces containing
That allows materially different workspaces to be recorded as identical, so replay divergence can remain invisible. Please use a NUL-delimited traversal ( [P1] Normal task-declared snapshots are persisted as usable refs after cleanup has deleted themTask config enables On a plain
[P2] Unsupported experiment-wide reasoning settings fail after provisioning and are retried in a childWith Gemini and This is a global request/agent compatibility error, not task-attributable post-boundary failure. Validate it before parent provisioning and do not retry it in branch children. [P2] The explicit structural gate still fails
Documentation and merge readiness
Verification at this head
So the implementation is substantially closer, but I would not merge it until the two P1 provenance/lifetime issues are fixed and the late global-validation path is corrected. |
PR benchflow-ai#1046 second review, P1-A. The workspace-digest pipeline fed filenames through newline-separated find | sort | xargs, so a legal name like 'file with spaces.txt' was word-split into several arguments; the inner sha256sum/stat failed, the trailing | sha256sum succeeded anyway (a pipeline's exit status is its last command's), and two materially different Alpine workspaces (0 bytes vs 37 bytes inside that file) recorded the SAME successful digest. The rebuilt command in workspace_digest_command(): * is null-safe end to end — find -print0 | sort -z | xargs -0, so names with spaces or newlines stay single arguments; * fails closed intrinsically — every stage writes its own file under a mktemp -d dir with its exit status checked by &&, so an inner failure fails the whole exec and the digest is absent-with-reason, never wrong. No pipefail: sandbox exec runs `sh -c` and the smallest images ship busybox ash (construction verified against alpine's busybox); * pins the sort to byte order with LC_ALL=C. For workspaces whose names the old pipeline handled correctly the byte stream reaching the final sha256sum is unchanged, so digest values are stable; pathological names now produce a correct (different) value instead of colliding. WORKSPACE_DIGEST_BASIS is reworded so digests recorded under the broken basis never read as comparable to new ones. tests/test_branch_composed_docker.py's _digest_command — the helper the reviewer's repro was built from — now delegates to the fixed src helper (workspace_digest_command grew the exclude_basename it needed), so the null-safe form lives in exactly one place. Regression tests (red on the old pipeline, exercising a real /bin/sh — never a faked shell): two trees differing only inside 'file with spaces.txt' must digest differently (the reviewer's exact repro; both digests were identical before), and a newline-bearing filename must digest correctly or fail closed, never succeed wrongly (also identical before). GNU digest tools are shimmed from real tools (shasum -a 256, BSD stat -f) on hosts without coreutils; the shell, find, sort, and xargs under test are never shimmed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…ver retry them in children PR benchflow-ai#1046 second review, P2-A. With Gemini and --reasoning-effort high the run built the environment, created the snapshot, installed and connected the agent, and only then hit the ACP effort rejection; because the env-ready snapshot existed, ablation restored a child, reinstalled the agent, and failed the same way before skipping the rest of the branch. A global request/agent compatibility error is not task-attributable and retrying it re-runs the identical doomed configuration. Two layers: Pre-flight (what is knowable statically): * acp/runtime.py grows reasoning_effort_preflight_error(), kept next to the _configure_acp_session dispatch it mirrors: effort is applicable iff the registry declares acp_effort_config_id or the agent is codex-acp (effort rides the model[effort] id, resolved against the live session's catalog — not statically decidable, so codex passes). Non-ACP and unknown/manifest agents are left to their own validation. * build_eval_plan() calls it for the CLI-authoritative agent paths (--tasks-dir / --source-repo, the same gate as the effective_model validation), so `bench eval run` AND `bench eval ablate` (whose resolve_canonical_parent_config goes through the same plan) reject the pairing before any sandbox exists. In ablate that means no Rollout is even constructed (AblationSpecError). Classification (what only the live session can reject): * ACPRequestGlobalError (a RuntimeError, so existing fail-closed callers/tests hold) is raised for the deterministic rejections: no declared effort option, session does not expose the option, and the agent answering set_model/set_config_option with a protocol error. A timeout/transport failure on the same calls stays a plain retryable RuntimeError — it proves nothing about compatibility. * Every message carries REQUEST_GLOBAL_MARKER; classify_error() maps it to the new request_global category ahead of the "acp error" branch (these messages embed agent text that would otherwise classify as retryable acp_error). The category joins RetryConfig's default exclude_categories — the benchflow-ai#917 provider_auth pattern — so eval-run retries never re-attempt it either. * run_ablation() skips branch_at_stage() outright when the parent's error classifies request_global: no child attempts, every arm reports skipped, and report.error says the arms were not attempted because every child would re-run the same rejected configuration. Regression tests (red first): pre-flight rejection before any Rollout exists (fake rollout registry records zero constructions); a request-global parent failure yields no branch_at_stage call, all-skipped arms, and a report that says why; a task-attributable parent failure still forks the arms (RFC §1 behavior pinned against classifier over-match); the gemini rejection is ACPRequestGlobalError and classifies request_global; an agent-rejected option is request-global while a timeout is not; the category is excluded from retries by default; the eval CLI rejects gemini + --reasoning-effort high cleanly at planning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…s on bench eval run with a tested import path PR benchflow-ai#1046 second review, P1-B: a plain Rollout.run() with stage snapshots left three valid-looking bf-snap-* refs in stage_snapshots.json whose images `docker image inspect` could no longer resolve — no ephemeral or export marker, and no retention/import path outside bench eval ablate. Both halves of the finding: Truthful lifetime, always: Rollout.cleanup() now finalizes stage_snapshots.json immediately before the sandbox stop that destroys the committed images (`compose down --rmi all`), stamping each stage entry with the same ephemeral/exported schema the ablate report uses — `ephemeral: true, exported: null` by default, the export record (path, sha256, image id) when retained. The machinery moved from ablate.py into branch_policy (export_stage_snapshot, annotate_stage_snapshot_lifetime, finalize_stage_snapshots); ablate's retain_stage_snapshot delegates to it and now mirrors its annotation into the parent run's stage_snapshots.json, which cleanup preserves instead of clobbering — one schema, no drift between the two artifacts. An externally-owned sandbox is untouched: its images survive, so marking them ephemeral would be the opposite lie. Retention + import on normal evaluation: `bench eval run --keep-snapshots` (threaded EvalCreateRequest -> EvalPlan -> EvaluationConfig -> RolloutConfig.keep_snapshots, including the sharded-worker payload and YAML config) exports each captured stage image to <run_dir>/snapshots/<ref>.tar before cleanup. The export record now carries the image id read from the tar's own manifest, and the new import path — benchflow.snapshot_import.import_stage_snapshots / `bench eval import-snapshots <run-dir>` — verifies the tar's recorded sha256, `docker load`s it, and confirms the recorded ref resolves to the recorded image id, failing closed on ephemeral entries with the flag to re-run with. That makes the documented branch-later workflow real; docs (cli.md, RFC §3.6, task-standard) now state exactly that scope instead of overselling bare refs as restorable. Tests: cleanup marking red-first against the previous engine (bare refs stayed bare); export/sha256/image-id recording; ablate<->plain file consistency; the import path against a fake docker runner (identity mismatch, tampered tar, ephemeral refusal, relocated run dir); flag threading CLI->RolloutConfig and through the worker payload; plus a live T2 commit->export->rmi->import round trip in test_branch_composed_docker.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
|
@bingran-you All four findings from the re-review are addressed at
Gates at tip: full suite Ready for another pass whenever you are — and thank you for the depth of these reviews; the digest repro and the |
`_resolve_layers` — which rejects an empty layer set — ran only on the cursor
arm. On the `at_stage` arm `layers` was *derived* from the recorded
`StageSnapshot`'s refs and taken on trust: a snapshot carrying neither ref
derived the empty set, `_gate_layers` then had nothing to check, and
`_restore_composed(parent, environment=None, sandbox=None)` rolled nothing back
before each child. The fork ran to completion — every child in the world the
previous one left, a V published across them, and provenance recording a clean
stage fork with `snapshot: {environment: null, sandbox: null}`.
Not reachable with the shipped `ManifestEnvironment`, but nothing between the
Environment protocol and `checkpoint_composed` requires the handle a plane
returns to be non-`None`, so a third-party plane reaches it — and it is a
fail-open hole in a design that is otherwise fail-closed everywhere else.
Run the derived set through the same gate, before the disagreement check: an
empty capture is broken however it is described, and `snapshot_layers=set()`
would otherwise "agree" with it and skip the more specific error. The
empty-set diagnostic now reads for a derived set as well as a requested one and
says what an empty checkpoint costs.
The pinning test uses `pre-verify`, not `env-ready`: at `env-ready` the
fresh-children gate already demands the container layer, so the hole is only
reachable at a boundary whose children run in place.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…vioral difference
A measured skills ablation on `lake-warming-attribution` (docker, codex-acp,
gpt-5.4-mini) scored 0.00 in both arms and the tool printed "no difference in
this comparison" — while the skill pack flipped `test_trend_result` to passing
(it prescribes Mann-Kendall/Sen's slope; the no-skill arm ran OLS and missed
p<0.05 by 0.005) and flipped `test_dominant_factor` to failing. Two large,
reproducible, opposite-signed sub-outcomes netted to exactly zero on the binary
reward, and attribution on the scalar alone reported the reward truthfully and
the behavior falsely.
`bench eval ablate` now attributes at both granularities. Each arm's per-test
outcomes are mined from its own branch child's verifier CTRF report through the
parser the eval report's failure lines already use — `_ctrf_tests()` is factored
out of `_ctrf_failure_line()` so there is one reading of "the tests this
verifier reported", and `ctrf_test_outcomes()` takes the whole map rather than
the first failure. `ablation.json` gains a per-arm `tests` map and a top-level
`test_attribution` section; the console gains a second table listing only the
tests whose outcome differs (tying tests are counted, never listed).
The scalar verdict stays, but a tie it can no longer state unqualified: when
two arms tie on reward and their per-test outcomes disagree, the verdict reads
"scalar rewards tie, but N sub-test outcome(s) differ: <names>". Today's
wording survives exactly where it is true — sub-tests observed and tying, or
never observed at all.
Nothing is fabricated when a verifier emits no CTRF report: `tests` stays
`None` (not `{}`), the section reports scalar-only attribution and names the
arms it could not read, and a test named by one arm's report and not the
other's shows as "not reported", never as a failure. Test names sort and no
wall-clock enters the JSON, so the same input still writes a byte-identical
report.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
CI runs `ruff format --check src tests tools`; these eight files were the only ones in the tree that would have been reformatted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…ifacts Per-test attribution worked for a fresh-rollout child and silently degraded to scalar-only for a child that ran in place — reproduced live at --at-stage pre-verify, where the report read "no difference in this comparison" with the per-test data sitting on disk one directory down. Fallout from the artifact isolation in "fix(branch): branch children no longer clobber the parent's artifacts". An in-place child has no rollout directory of its own: it writes through the parent's bind mounts, and the branch engine archives what it wrote under <child>/mounted/. The reader looked only at <child>/verifier/ctrf.json, which only a fresh-rollout child has. branch_artifacts.child_artifact_roots() now names both rollout-shaped roots, best first, so the layout stays owned by the module that creates it instead of being re-derived in ablate.py. The ablation reader tries them in order through the CLI's existing CTRF parser and takes the first that reports outcomes; neither reporting still means tests=None, never a fabricated row. Order is the child's own directory first — a fresh-rollout child can have both, and the copy it downloaded into its run directory is the authoritative one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…dren An in-place branch child continues the *shared* Rollout, so the isolation invariant has to cover what the parent reports, not only where its cursor sits. _rewards was scoped; _timing, _verifier_error and _diagnostics were not — and in a live two-arm pre-verify ablation the parent's timing.json carried its own agent_execution plus both arms' (execute() accumulates into the same dict), while its result.json carried the last child's verifier error. _LinearState now also scopes the result-bearing attributes, audited against everything _build_result() reads: - provably mutated by a child's connect/execute/verify/disconnect: _timing, _verifier_error, _diagnostics, _native_usage_metrics (whose accumulated tokens cleanup() promotes into _usage_metrics — the same accumulation bug as _timing, one field further from the result) and _native_usage_checkpoint; - result-bearing and reachable from a caller-supplied run_child, scoped for the same reason: _error, _export_error, _evolved_skills, _usage_metrics; - audited and deliberately left alone (setup-owned, cleanup-owned, or derived from the parent's own config): _rollout_dir, _rollout_name, _resolved_prompts, _task_skill_policy, _agent_name, _started_at, _terminal_timeout and the _provider_*_cached trio. Capture and restore both deep-copy, because restore_onto() runs once per child: a captured reference would be mutated in place by child k and restore nothing for child k+1 or the parent. Absent attributes stay absent, so Rollout instances built through __new__() in tests are unaffected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
A child failure other than UnscoredChildError — an agent connection drop, a verifier crash — propagates out of _run_children, and control never reached the lineage write at the end of branch(). But the caller often survives it: run_ablation catches exactly that exception and reports the completed, failed and skipped arms. The run therefore published arms with no tree.json and no per-child provenance or reward artifacts behind them — a partially completed experiment losing the evidence its own report is a claim about. The failure path now restores the parent's linear state and writes the same lineage the success path writes, then re-raises. A fork that died mid-way is distinguishable from one that finished by its content rather than its absence: the failing child is in the tree with neither a reward nor an unscored reason, and the parent carries no V. Both paths go through one _write_lineage() helper, which keeps the existing failure isolation: an artifact-write error is logged, never propagated, and on the partial path never replaces the child failure on its way out — a full disk must not turn "agent connection lost" into OSError. The restore on that path is isolated the same way and for the same reason; on the success path a failed restore stays loud, since there is no more important failure to preserve. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…and arms A task binds its own world by declaring benchflow.environment.manifest in task.md — the image, the services the framework starts, the provisioning and the readiness probes. bench eval resolves that per task before building the rollout config; bench eval ablate built its parent config with environment_manifest=None, so the parent and every arm forked from its snapshot ran without the task's declared environment and the report compared a different world than a normal evaluation of the same task does. The resolver moves out of evaluation.py into environment/manifest.py as manifest_from_task_document(), and both commands now call it: one resolution path, so the two cannot drift. The ablation binds it on the parent config, which is also the config a fresh env-ready child is derived from (child_skill_config), so every arm inherits the same manifest; an in-place child runs on the parent rollout itself and therefore in the same world. A declaration that cannot be resolved is fatal (AblationSpecError, before the parent run costs anything) rather than degrading to None: an ablation whose declared environment could not be built is not an ablation that legitimately ran without services. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
In host proxy mode an agent can exit or error after requesting fewer exchanges than the configured cut. The stitched trajectory and the recorded-vs-live token split were still built on the configured K while the cut_point block already reported the router's served count, so such a run left three artifacts that contradicted each other: a trajectory containing recorded responses the agent never received, those responses' tokens billed as replayed, and provenance saying a shorter prefix was replayed. None of that is usable as experiment evidence. Host mode now takes router.n_replayed_exchanges as the single basis for the stitched prefix, the usage split, the cut_point block and ContinueResult's n_recorded. Sandbox proxy mode has no live router on the host — its uploaded recording is truncated to the configured prefix — so it keeps configured accounting and now writes that block explicitly rather than leaving the basis to be inferred. Both modes go through one write_continuation_artifacts() call that takes a single n_recorded and the matching cut_point block, so the invariant is structural instead of a convention repeated at two call sites; the block's existing `accounting` field is what names the basis in the artifact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…nv-ready The config_override BranchDelta field was schema- and provenance-stable but raised BranchDeltaNotSupported. It now executes through the same fresh-child path skill_mode uses: a child forked from the env-ready snapshot runs as its own Rollout over the restored sandbox (use_prebuilt_env), with the delta's overlay deep-merged over the parent's own overlay into the child's RolloutConfig — the child's setup() then applies it through the existing allowlisted machinery (src/benchflow/_utils/config_override.py, benchflow-ai#790): same allowlist, same re-validation, same content addressing in config.json. Gates, all before anything is quiesced: the delta executes only at the env-ready boundary (the state the config governs is consumed by setup(), so any later fork records the override and runs without it), a non-allowlisted key (anything scorer-shaped) fails closed with the run-level allowlist error, and a caller-supplied run_child conflicts. Combinations compose on one child: config_override + skill_mode are both fields on the child config, and config_override + injected_prompt rides the fresh-child prompt path. Provenance: the delta block keeps config_override_sha256 and now records config_override_keys (the sorted allowlisted sections, the benchflow-ai#790 keys record) when the field is set — every other delta keeps its exact recorded shape. bench eval ablate grows the matching arm kind config:<inline-json-or-@file>, parsed by the same loader as the run-level override, allowlist-checked at parse time, with --arms splitting made brace-aware so inline JSON commas stay content. docs/reference/cli.md updated; the docs-drift test stays green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…eady The environment_ref BranchDelta field executes the documented tool-outage perturbation (env0@prod vs env0@outage): the child runs as a fresh rollout over the restored env-ready sandbox and provisions the CHILD manifest's service set over it, gated on readiness before install_agent() — the RFC §3.1/§3.3 service bracketing around the state restore. The design decision, made explicit as the image-vs-services boundary (resolve_environment_ref_delta, one function shared by engine validation, the child runner, and the ablate pre-flight): the env-ready snapshot commits the PARENT's container, and restoring it kills every framework-started service with it. For a manifest pair sharing the same image with owns_lifecycle = false, what the fresh child provisions IS the restored world's service topology, so swapping the manifest is sound. A manifest that changes the image breaks the restore-the-parent-container premise — it needs a rebuild path, which contradicts branching from a snapshot — and fails closed with the typed BranchEnvironmentImageConflict naming both images; an entrypoint-owned lifecycle on either side fails closed too (the framework cannot subtract a service the entrypoint restarts), as do an unresolvable ref and a parent with no bound manifest. All gates fire before anything is quiesced. The provisioning step covers every fresh child of a manifest-bound parent: a zero-delta control arm re-provisions the parent's own manifest, so the baseline of an outage comparison no longer scores a world whose services all died with the container restore. BranchDeltaNotSupported moves to branch_delta.py (re-exported from rollout_branch) so branch_skill can subclass it without a cycle. bench eval ablate grows the matching arm kind env:<registry-ref>; the env arm's content gates run in run_ablation once the parent's manifest is known, still before the parent run costs anything. docs/reference/cli.md and the RFC's §3.3/§5 updated with the boundary; the docs-drift test stays green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
A fresh-rollout child (env-ready) is a Rollout of its own and leaves the
standard config.json/result.json/timing.json/trajectory set for free; an
in-place child (pre-verify/post-verify stage branches, cursor branches)
continued the parent instance and left only provenance.json/reward.json
plus the mounted/ archive — "what happened in this arm" required
cross-reading tree.json.
New benchflow.branch_result closes the gap through the isolation the
engine already provides: the per-child state bracket. Before an in-place
child runs, its result-bearing fields (_timing, _rewards,
_verifier_error, the diagnostics collector, ...) are scoped to zero — so
what they hold afterwards is the child's own, never the parent's showing
through a field the child happened not to write — and after the child
completes, the standard result set is built from that state via the same
_build_rollout_result every rollout uses, before the next restore
discards it. Trajectory, prompts and tool calls are the delta appended
past the fork baseline; fields the child does not genuinely produce
(token usage, run-level error) are null/absent, never inherited; an
engine-recorded unscored child publishes rewards: null, not a leftover
{}. Best-effort by contract: a failed write is logged and never costs
the reward.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…nd environment
bench eval ablate gains --environment-manifest with the run path's exact
semantics — the shared EnvironmentManifestOption (same flag, type and
help as bench eval run), and the same precedence: an explicit manifest
wins and the task-declared benchflow.environment.manifest is not even
consulted, mirroring benchflow.evaluation.
ablation.json now answers the reviewer's two open questions on its own:
* the bound environment is stamped ("environment": manifest name, the
ref exactly as the caller wrote it — flag value, arm spec, or task.md
declaration, never a machine-local resolved path — the manifest's
sha256 content address, and the image it names), for the parent at the
top level and per arm when an env: delta swapped a different manifest
in. New ManifestBinding / load_manifest_binding /
manifest_binding_from_task_document keep that provenance on the
existing loaders, which now delegate — one resolution path, no drift.
* the branched stage's snapshot refs are stamped ("stage_snapshot": the
committed sandbox image ref, the environment snapshot id, and the
captured layers) and printed with the table, so the recorded world can
be restored and re-branched by hand later — the same refs as the
parent run's stage_snapshots.json, no second source of truth.
Stage UX: the post-research rejection now states the working recipe —
await rollout.mark_stage('post-research') at the cut point, then
await rollout.branch_at_stage(...) — instead of a bare pointer at the
Python API; the CLI surface stays unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…catalog model
codex-acp@1.6.0 validates session/set_model strictly against its
built-in catalog: a model with no advertised ``model[effort]`` variant
is rejected outright — bare ids with "Unsupported format of modelId",
suffixed ids with "Unknown model gpt-5.4-mini[medium]" — even when that
exact id is already the session's *current* model. Verified live
2026-08-21 by driving 1.6.0 over ACP stdio on oncology: the catalog now
carries only gpt-5.6-sol/terra/luna, gpt-5.5 and gpt-5.2, so every
``us-openai/gpt-5.4-mini`` rollout dies in _set_acp_model before the
first prompt (hit while running the surface-ion-trap-shuttling
ablation; parent AND first arm both RuntimeError'd at set_model).
The model was never missing: on the LiteLLM route
``apply_codex_provider_config`` already injects the gateway alias via
CODEX_CONFIG, 1.6.0 applies it at startup, and the session advertises
it as currentModelId (probed live: CODEX_CONFIG={"model":
"gpt-5.4-mini"} yields currentModelId gpt-5.4-mini[medium]). The
set_model call was requesting a state that is already in place, through
a method that can no longer express it.
_configure_acp_session now skips session/set_model exactly when (a) the
requested model resolved to no advertised ``model[effort]`` variant —
set_model can only fail — and (b) the session's current model is the
one BenchFlow's own CODEX_CONFIG injected. In-catalog models keep the
62cc7e4 bare→``model[effort]`` mapping and set_model path, a requested
effort is satisfied only if the injected current id carries it, and an
unsatisfiable effort still fails closed via the existing effort step.
Tests: four new cases in test_acp_model_config_dispatch.py pin the
skip, the effort-satisfied skip, the effort fail-closed path, and the
in-catalog non-regression. Full suite green (one pre-existing unrelated
failure); ruff check/format and ty clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
Braces, brackets and commas inside a quoted inline-JSON string value are content: the depth-only walk let a string containing close-braces zero the counter and split the spec mid-JSON. Reported in review on benchflow-ai#1046. The config:@<file> comma limit stays and is documented on --arms. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…ectory The reviewer's P1: MountedArtifacts.release() caught its own move-back failure, logged that the parent's evidence was "still there, not lost" — and then unconditionally rmtree'd the hold directory holding exactly that evidence. hold() and hand_off() failed open the same way: a hold failure let children overwrite the parent's evidence, a hand_off failure let the next child inherit (and destroy) the previous child's files. Custody is audit-critical and now fails closed: - Typed ArtifactCustodyError carries the preserved hold_dir; every failure log and error message names the preserved path. - hold() raises before any child can run over unheld evidence; whatever was already held stays preserved. - release() removes the hold directory only after every held entry was confirmed moved back and nothing remains inside it — a directory whose contents were not confirmed moved is never deleted. - The one deferral keeps the engine's "an artifact error must never replace the real failure" invariant: hand_off() runs inside the child loop's finally, so it records the failure (custody_failures) instead of raising over a child's own exception; raise_pending() surfaces it as soon as the child's outcome is safely recorded, stopping the fork before the next child inherits leaked files. branch() likewise calls release(raising=False) only while a child's exception is unwinding. Red on the pre-fix code: a failed release deleted the hold directory and left no evidence at the canonical paths; a same-shaped run now preserves the hold directory and raises, and the tests in test_rollout_branch.py (custody section) pin both halves plus the no-masking invariant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…d records workspace digests
The reviewer's P1: cut_point_digest hashed the RECORDED request rather
than the actual incoming one — a diverged replay hashed to the recorded
value and looked faithful — while _check_divergence compared only message
counts, so a same-count prompt/content/tool change was invisible. The
RFC §3.5 workspace digest was absent entirely.
(a) Both sides of the cut are digested and named honestly:
served_request_digest (the request the agent ACTUALLY sent) vs
recorded_request_digest, each sha256 over canonical JSON of the
comparable projection {messages, tools} — the fields the recorded
normalized projection and the live HTTP body share; digesting either
side whole would flag every exchange (placeholder model
openai/replay, transport fields). request_digest_basis states the
basis in the artifact.
(b) Divergence is checked per replayed exchange by content digest on
the same basis; every event (exchange index + both digests + the
message counts) is recorded in the router and lands in the served
cut_point block's divergences list. Divergence annotates rather
than aborts by default — replay fidelity is best-effort by design
(RFC §3.5: "recorded, not hidden") and legitimately shifting
content (timestamps, nondeterministic tool output) would otherwise
kill wanted continuations; strict_divergence remains the opt-in
abort and now trips on content, not only counts. The artifact is
truthful either way.
(c) The RFC-promised workspace digest: a reusable
sandbox/workspace_digest.py helper (the find|sort|sha256sum
pipeline the branch docker proofs use, marker-guarded against
merged compose noise) runs once, at the first live-leg request —
the moment the replay-rebuilt workspace is complete and still
live — scheduled onto the run's event loop from the proxy thread
in host mode. When no sandbox is reachable at the cut (sandbox
proxy mode, a run that never crossed, a digest failure) the block
records null with the reason; a digest is never fabricated.
Red first: the same-count content-change tests fail on the pre-fix
router (divergences stayed 0); they now detect and record the event,
count mismatches still trip, and the provenance tests pin both digest
fields, the basis label, the divergence events and the workspace
digest/null-with-reason in configured- and served-basis blocks.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…thfully
The reviewer's P1: run_ablation() cleaned up before serializing
stage_snapshot, so the real E2E report published bf-snap-…-34995d94bcff
while docker image inspect confirmed the image no longer existed — a
recorded handle that read as restorable but resolved to nothing. RFC
§3.6 promised --keep-snapshots + docker save; neither existed.
Both halves, implemented:
(a) bench eval ablate --keep-snapshots (and
AblationRequest.keep_snapshots on the library entry point): before
cleanup — the one window in which the committed image still
exists — DockerSandbox.export_image docker-saves the branched
stage's snapshot image to <out-dir>/snapshots/<ref>.tar, and
ablation.json records the tar's path and streaming sha256 under
stage_snapshot.exported with ephemeral: false. export_image fails
closed (a failed save never leaves a partial tar behind), and a
backend without export support records export_error while the
arms' rewards survive — retention never raises over the result.
(b) Without the flag the report records the handle truthfully:
stage_snapshot gains ephemeral: true, exported: null, so a reader
knows the ref no longer resolves. The snapshot-ref read moved
before cleanup alongside the retention step; cleanup still always
runs.
docs/reference/cli.md gains the flag row (the bidirectional ablate
flag-drift test forces it) and RFC §3.6 now states the ephemeral
recording instead of overpromising.
Red first: on the pre-fix source the retention tests fail —
keep_snapshots does not exist and the stamped stage_snapshot carries no
lifetime marker; green now with the export ordered before cleanup
(pinned via the fake rollout's call log), tar bytes + sha256 recorded,
and the ephemeral default asserted in report and JSON.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…ation axis run_ablation() hand-rolled a reduced RolloutConfig, dropping the normal controls and provenance a plain eval run stamps — the real E2E parent and child configs published task_digest: null and reasoning_effort: null (PR benchflow-ai#1046 review). The request now resolves through the same two stages as `bench eval run`: build_eval_plan (normalized agent/model/effort/sandbox/ usage settings, fail-closed validation) and the newly extracted benchflow.evaluation.task_rollout_config (dataset identity, live-computed task digest, task-declared environment fallback, task source provenance) — with only the ablation-owned fields overlaid on top: the stage-capture request, the pinned no-skill parent, out-dir/job naming, and the resolved environment binding. Evaluation._run_single_task now calls the same task_rollout_config with its learner-path overrides, so the two callers cannot drift. AblationRequest gains reasoning_effort and the CLI gains --reasoning-effort (documented in cli.md); plan-validation failures re-raise as AblationSpecError before the parent run costs anything. Regression tests assert task_digest and reasoning_effort flow from a task fixture into the parent config and into both fresh-child configs, and that a bad effort/sandbox dies with nothing built. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…orting into focused modules The PR benchflow-ai#1046 review flagged ablate.py (1,203→1,320 lines) and rollout_branch.py (1,294→1,320) as monoliths, branch() at cyclomatic 21+ with 75 statements, and _run_children() taking 14 arguments. Behavior- neutral four-way split along the reviewed seams: - branch_policy.py — stage/snapshot policy: layer resolution + capability gating, capture_stage, recorded-stage resolution, and the whole delta-vector gate (validate_deltas, the per-delta stage/layer/parent/ runner preconditions, the fresh-children boundary rule). - branch_transaction.py — the transactional heart: checkpoint_parent, the scoped LinearState capture, and BranchTransaction — one dataclass carrying what _run_children took as 14 arguments, with the per-child restore/run/record/hand-off loop as methods. - branch_children.py — delta execution paths: the in-place default runner and per-child runner selection; the fresh-rollout half stays implemented in branch_skill.py (its import path and run_fresh_child patch seam are pinned by six test files) and is re-exported here. - branch_report.py — the ablation report model, arm/child pairing, per-test mining and attribution, moved out of ablate.py. rollout_branch.py keeps the branch() orchestrator — now cc≈15 / 41 statements (was cc≈30 / 72) — plus the failure-isolated lineage write, which stays because tests patch benchflow.rollout_branch.write_branch_artifacts; the transaction's fresh-runner factory and in-place result writer are injected from rollout_branch's globals for the same reason (tests/test_branch_child_result.py patches them there). ablate.py lands at 884 lines and rollout_branch.py at 441; every previously importable name keeps working via re-exports (rollout_branch re-exports the gates, exceptions, capture_stage, ChildRunner, CHILD_WALL_CLOCK_KEY, _LinearState and the fresh-child API; ablate re-exports the report and attribution API). Full suite identical before and after (6129 passed); no test edited. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
The PR advertised stage-tagged replay cuts (RFC §3.5) but nothing at run time recorded which LLM exchange a stage boundary closed on, so a stage name could never resolve to a replay prefix. Now every stage capture — lifecycle-auto or Rollout.mark_stage() — records the completed-exchange index of the moment: - LiteLLMProcess.live_exchange_count(): drains the live-capture tail to the gateway log's end (bounded) and counts completed exchanges on the same per-record basis as llm_trajectory.jsonl, answering None instead of a stale lower bound when the tail cannot catch up or capture never started. Polls are now serialized behind a lock so an on-demand drain cannot interleave with the capture loop mid-poll and double-append. - contracts.planes.LiveUsageGateway grows the accessor, statically asserted by the providers plane as with live_usage_tokens. - branch_policy.capture_stage reads it duck-typed (the branch engine, like the kernel, cannot import the providers plane), records it into StageSnapshot.meta, and branch_lineage serializes it as exchanges_completed per stage in stage_snapshots.json — the stage→exchange-index data continue-runs read for --cut-stage. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
bench eval continue exposed only numeric --max-exchanges; the SDK's stage_tags dict existed but nothing read the indices a run actually recorded, so the advertised stage-tagged cut (RFC §3.5) was not usable from the CLI. Now: - RunFolder loads stage_snapshots.json (stage_registry, plus recorded_stages / stage_exchange_tags views) — tolerant at load time, strict at resolution time. - orchestrator.stage_tags_from_run() resolves a --cut-stage request against the recorded registry with typed ReplayCutPointError failures: no recorded stages, an unrecorded stage (lists what was recorded), a stage recorded without an index (exchanges_completed null), or a stage that closed before the first exchange. An explicit SDK stage_tags mapping still overrides the registry. - bench eval continue grows --cut-stage <name>, mutually exclusive with --max-exchanges; the resolved stage keeps landing as branch_stage in the cut_point provenance block. - docs: cli.md continue section + continue-runs.md cut-points now describe the recorded-registry resolution instead of calling run-time stage tags a follow-on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
bench eval ablate rejected --at-stage post-research outright: the stage
is a mid-execute() cut point only Rollout.mark_stage() can record, and
the command drives the lifecycle, not the agent's planning. The new
--mark-research-end-on <path> flag supplies the concrete, testable
trigger: a workspace file (e.g. /app/PLAN.md, the FrontierPhysics
convention) whose first appearance IS the research→execution boundary.
- watch_research_end() runs beside execute() and polls the sandbox with
a cheap `test -e` (2s cadence, plus one final check when the agent
quiesces) and calls mark_stage('post-research') the first time the
file exists. Tradeoff stated loudly in the docs and docstrings: with
no per-LLM-exchange hook available from outside the agent process,
the capture lands within one poll of the file appearing and may
include up to that much post-plan agent work; the exchange index
recorded with the mark is exact for the capture moment.
- validate_arms_for_stage gains research_end_marker: post-research now
passes pre-flight with the trigger, still fails closed without it
(the rejection teaches both the flag and the SDK path), and the
marker on any other stage is rejected.
- run_ablation reports "the marker never appeared" and skips the arms
instead of forking a stage that was never captured.
- cli.md documents the new flag, the corrected --at-stage row, and the
post-research fork semantics (fresh agent session over the restored
workspace; use continue --cut-stage to rebuild agent memory instead).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…pports it task/prompts.py still failed `branch_execution: forked-snapshot` closed as "not implemented" even though the Environment/sandbox snapshot branch engine ships. The gate is lifted the way the engine actually supports the value: - compile_document_user_runtime accepts forked-snapshot (still requires branchable: true) and compiles it to a stage-capture request: CompiledUserRuntime.snapshot_stages — the auto-capturable boundaries by default, or an explicit benchflow.nudges.branch_stages list validated against the branch-stage taxonomy (declaring post-research says the harness will mark_stage() it). branch_stages without forked-snapshot, unknown stages, and non-list shapes fail closed. - RolloutConfig adopts the task's request when it adopts the document user (run-level snapshot_stages wins outright; adopted layers default to the container layer, plus environment iff a plane is bound) — so a plain evaluation of a forked-snapshot task leaves stage_snapshots.json with exchange indices behind, ready for bench eval ablate, branch_at_stage(), or bench eval continue --cut-stage. - validate_task_runtime_support fails forked-snapshot closed on backends whose sandboxes cannot take container snapshots, via a new registry-level supports_container_snapshot fact (CONTAINER_SNAPSHOT_PROVIDERS); the runtime supports_snapshot gate remains the final authority (Daytona DinD). - task-standard and the rollout-branching RFC now describe the shipped behavior instead of "fails closed until integrated"; the RFC's §3.2 and §3.5 wording matches the recorded stage→exchange-index cut flow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
PR benchflow-ai#1046 second review, P1-A. The workspace-digest pipeline fed filenames through newline-separated find | sort | xargs, so a legal name like 'file with spaces.txt' was word-split into several arguments; the inner sha256sum/stat failed, the trailing | sha256sum succeeded anyway (a pipeline's exit status is its last command's), and two materially different Alpine workspaces (0 bytes vs 37 bytes inside that file) recorded the SAME successful digest. The rebuilt command in workspace_digest_command(): * is null-safe end to end — find -print0 | sort -z | xargs -0, so names with spaces or newlines stay single arguments; * fails closed intrinsically — every stage writes its own file under a mktemp -d dir with its exit status checked by &&, so an inner failure fails the whole exec and the digest is absent-with-reason, never wrong. No pipefail: sandbox exec runs `sh -c` and the smallest images ship busybox ash (construction verified against alpine's busybox); * pins the sort to byte order with LC_ALL=C. For workspaces whose names the old pipeline handled correctly the byte stream reaching the final sha256sum is unchanged, so digest values are stable; pathological names now produce a correct (different) value instead of colliding. WORKSPACE_DIGEST_BASIS is reworded so digests recorded under the broken basis never read as comparable to new ones. tests/test_branch_composed_docker.py's _digest_command — the helper the reviewer's repro was built from — now delegates to the fixed src helper (workspace_digest_command grew the exclude_basename it needed), so the null-safe form lives in exactly one place. Regression tests (red on the old pipeline, exercising a real /bin/sh — never a faked shell): two trees differing only inside 'file with spaces.txt' must digest differently (the reviewer's exact repro; both digests were identical before), and a newline-bearing filename must digest correctly or fail closed, never succeed wrongly (also identical before). GNU digest tools are shimmed from real tools (shasum -a 256, BSD stat -f) on hosts without coreutils; the shell, find, sort, and xargs under test are never shimmed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…ver retry them in children PR benchflow-ai#1046 second review, P2-A. With Gemini and --reasoning-effort high the run built the environment, created the snapshot, installed and connected the agent, and only then hit the ACP effort rejection; because the env-ready snapshot existed, ablation restored a child, reinstalled the agent, and failed the same way before skipping the rest of the branch. A global request/agent compatibility error is not task-attributable and retrying it re-runs the identical doomed configuration. Two layers: Pre-flight (what is knowable statically): * acp/runtime.py grows reasoning_effort_preflight_error(), kept next to the _configure_acp_session dispatch it mirrors: effort is applicable iff the registry declares acp_effort_config_id or the agent is codex-acp (effort rides the model[effort] id, resolved against the live session's catalog — not statically decidable, so codex passes). Non-ACP and unknown/manifest agents are left to their own validation. * build_eval_plan() calls it for the CLI-authoritative agent paths (--tasks-dir / --source-repo, the same gate as the effective_model validation), so `bench eval run` AND `bench eval ablate` (whose resolve_canonical_parent_config goes through the same plan) reject the pairing before any sandbox exists. In ablate that means no Rollout is even constructed (AblationSpecError). Classification (what only the live session can reject): * ACPRequestGlobalError (a RuntimeError, so existing fail-closed callers/tests hold) is raised for the deterministic rejections: no declared effort option, session does not expose the option, and the agent answering set_model/set_config_option with a protocol error. A timeout/transport failure on the same calls stays a plain retryable RuntimeError — it proves nothing about compatibility. * Every message carries REQUEST_GLOBAL_MARKER; classify_error() maps it to the new request_global category ahead of the "acp error" branch (these messages embed agent text that would otherwise classify as retryable acp_error). The category joins RetryConfig's default exclude_categories — the benchflow-ai#917 provider_auth pattern — so eval-run retries never re-attempt it either. * run_ablation() skips branch_at_stage() outright when the parent's error classifies request_global: no child attempts, every arm reports skipped, and report.error says the arms were not attempted because every child would re-run the same rejected configuration. Regression tests (red first): pre-flight rejection before any Rollout exists (fake rollout registry records zero constructions); a request-global parent failure yields no branch_at_stage call, all-skipped arms, and a report that says why; a task-attributable parent failure still forks the arms (RFC §1 behavior pinned against classifier over-match); the gemini rejection is ACPRequestGlobalError and classifies request_global; an agent-rejected option is request-global while a timeout is not; the category is excluded from retries by default; the eval CLI rejects gemini + --reasoning-effort high cleanly at planning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…s on bench eval run with a tested import path PR benchflow-ai#1046 second review, P1-B: a plain Rollout.run() with stage snapshots left three valid-looking bf-snap-* refs in stage_snapshots.json whose images `docker image inspect` could no longer resolve — no ephemeral or export marker, and no retention/import path outside bench eval ablate. Both halves of the finding: Truthful lifetime, always: Rollout.cleanup() now finalizes stage_snapshots.json immediately before the sandbox stop that destroys the committed images (`compose down --rmi all`), stamping each stage entry with the same ephemeral/exported schema the ablate report uses — `ephemeral: true, exported: null` by default, the export record (path, sha256, image id) when retained. The machinery moved from ablate.py into branch_policy (export_stage_snapshot, annotate_stage_snapshot_lifetime, finalize_stage_snapshots); ablate's retain_stage_snapshot delegates to it and now mirrors its annotation into the parent run's stage_snapshots.json, which cleanup preserves instead of clobbering — one schema, no drift between the two artifacts. An externally-owned sandbox is untouched: its images survive, so marking them ephemeral would be the opposite lie. Retention + import on normal evaluation: `bench eval run --keep-snapshots` (threaded EvalCreateRequest -> EvalPlan -> EvaluationConfig -> RolloutConfig.keep_snapshots, including the sharded-worker payload and YAML config) exports each captured stage image to <run_dir>/snapshots/<ref>.tar before cleanup. The export record now carries the image id read from the tar's own manifest, and the new import path — benchflow.snapshot_import.import_stage_snapshots / `bench eval import-snapshots <run-dir>` — verifies the tar's recorded sha256, `docker load`s it, and confirms the recorded ref resolves to the recorded image id, failing closed on ephemeral entries with the flag to re-run with. That makes the documented branch-later workflow real; docs (cli.md, RFC §3.6, task-standard) now state exactly that scope instead of overselling bare refs as restorable. Tests: cleanup marking red-first against the previous engine (bare refs stayed bare); export/sha256/image-id recording; ablate<->plain file consistency; the import path against a fake docker runner (identity mismatch, tampered tar, ephemeral refusal, relocated run dir); flag threading CLI->RolloutConfig and through the worker payload; plus a live T2 commit->export->rmi->import round trip in test_branch_composed_docker.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…ion decomposed under the structural gate The second exact-head review's P2-B: ablate.py had grown past the 1,000-line gate (1,036) and run_ablation() measured C901 14 / 16 branches / 63 statements. Behavior-neutral split, no test edits: * benchflow.ablate_arms (new, 481 lines) — the pre-flight half: everything decidable from the request alone. Arm parsing (parse_arm/parse_arms/_split_arm_specs, the arm-kind constants and AblationArm), stage/arm validation (validate_arms_for_stage, CAPTURABLE_STAGES), the fail-closed task and environment-binding resolvers, and the ablation error hierarchy beside its earliest raisers. benchflow.ablate re-exports every moved name, so external callers and tests keep their import paths. * benchflow.ablate (674 lines) — orchestration only. run_ablation() is decomposed into named phase functions with the same failure-isolation semantics, one per leg: _preflight_environment_arms (env-arm content gates + stamps, before the parent run costs anything), _run_parent (unchanged), _branch_into_arms (the request-global and research-end-marker skip gates, branch errors returned not raised), _retain_and_cleanup (retention strictly before the cleanup that destroys the image; cleanup always runs), _finalize_report (outcomes, attribution, failure-isolated result materialization). run_ablation() now measures C901 2 / 2 branches / 19 statements. * Report assembly already lived in benchflow.branch_report; its docstring and TYPE_CHECKING import now point at ablate_arms as AblationArm's definition site. Full suite identical before/after (targeted set 441 passed 4 skipped on both sides; full 6190 passed). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
…tion and snapshot lifetimes The second exact-head review's documentation-truth pass: * docs/architecture.md — the Branch lifecycle no longer claims the engine is environment-snapshot-only: the checkpoint composes the layers the fork requests (`snapshot_layers`; environment first on checkpoint, container first on restore), `require_sandbox_snapshot` is described with its actual check-only semantics, and the closing paragraph now says container + environment composition is implemented while agent-session composition remains future work. * docs/rollout-branching-rfc.md — the §2 status column tells the present truth (the engine composes layers; container snapshots are called when requested; env-state is the default layer, not the sole restore point; replay cuts at --max-exchanges/--cut-stage; the variation axes execute per child), the architecture.md gap quote is dated to drafting time with what has shipped since, §3.1 uses the implemented `snapshot_layers` spelling, §3.4 stops saying the tree "today" dies in memory, and §5 no longer lists `post-research` as unreachable — `--mark-research-end-on` supplies the mark from the CLI (§3.2 already said so). * docs/continue-runs.md — "a finished run keeps nothing of the container" is scoped by the phase-2 lifetime truth (ephemeral refs in stage_snapshots.json unless --keep-snapshots exported them), and the Limitations bullet no longer undersells divergence detection as a message-count check: the host replay proxy compares per-exchange content digests; the in-sandbox proxy is count-only. No overclaims: agent-session snapshotting stays documented as not implemented, and every statement matches the shipped code paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DZggU3vNYzr3hfDZJUE7jX
8fc8404 to
72cb8cd
Compare
|
@bingran-you rebased onto current The rebase had exactly one conflict, in Gates at Everything from your second review is in (custody fails closed, digests null-safe and fail-closed, request-global settings validated before provisioning and never retried in children, Ready for a merge call whenever you have a moment — and #1045 (docs-only) has never had its CI run; it needs one "Approve and run" click. Also, for planning: does the 7 September author-list line apply to infra tickets the same way it does to task PRs? |
3 participants
Summary
Implementation of #1045's RFC (read that first): composed environment + container checkpoints, stage-boundary snapshots, per-child branch deltas with lineage artifacts, a replay cut-point, and
bench eval ablate— infrastructure for attributing a rollout's failure to a stage and for running ablations as controlled comparisons rather than independent re-runs.Commit-by-commit, each independently reviewable:
docs(branch)feat(branch): compose sandbox + environment checkpointscheckpoint_composed/restore_composed;snapshot_layers(default{"environment"}= today's behavior). The engine gated onsupports_snapshotbut never calledSandbox.snapshot(); now it composes both layers in the documented order, and sandbox-only branching lets stateless environments branch at allfeat(branch): per-child deltas + lineage artifactsBranchDelta, content-addressed like #790;tree.json, per-child artifact dirs,source_provenance kind="benchflow-branch". Branched runs previously left no artifactsfeat(continue): replay cut-point--max-exchanges: replay the first K recorded exchanges then go live; stage-tagged cuts; served-vs-configured accountingfeat(branch): stage-boundary snapshot policyRolloutConfig.snapshot_stages, default empty = zero overhead);mark_stage(),branch_at_stage(),stage_snapshots.jsonfeat(branch): execute skill_mode deltasenv-readysnapshot runs as a fresh rollout over the restored sandbox and re-runs skill deployment under the switched modefeat(cli): bench eval ablateablation.jsonfeat(ablate): per-test attributionfix(...)Test plan
pytest tests/test_branch*.py tests/test_ablate_cli.py tests/test_sandbox_snapshot_contract.py tests/continue_run/ tests/test_cli_docs_drift.py tests/test_cli_live_progress.py -q→371 passed, 4 skipped6156 passed, 94 skipped, 0 failedruff check ./ty check src/→ All checks passed-m live, real daemon) →5 passed: zero-delta round trip at the container layer (+ negative control), composed sandbox+sqlite round trip, a PASS→FAIL→PASS reward-proxy invariant, restored containers keep their bind mounts and host-visible writescodex-acp/us-openai/gpt-5.4-mini, docker), cross-checked per arm against each child's ownverifier/reward.txtandverifier/ctrf.jsonon the hostWhat the measurements taught us
Running this for real, rather than against fakes, changed the design twice.
1. A scalar tie can hide a large behavioral difference. On a two-test task the skill pack flipped one sub-test and broke the other, netting exactly zero on the reward — so the tool printed "no difference in this comparison" while the arms behaved very differently. Mechanism verified, not inferred: the with-skill arm used the pack's prescribed Mann-Kendall/Sen's-slope method, the no-skill arm fell back to OLS and reproduced
p = 0.05479775402692069bit-for-bit across four runs and two models.bench eval ablatenow mines per-test outcomes from each child's CTRF report (reusing_failure_evidence's parser, not a second one) and reports the tests whose outcome differs:Arms with no per-test data degrade to scalar-only attribution and say so; a test named by only one arm reports
null, never "failed".2. Branch children reproduce plain rollouts faithfully. The branched
no-skillchild reproduced the plainno-skillrollout's per-test outcomes and that 17-significant-figure p-value; the branchedwith-skillchild reproduced its per-test signature and pack usage. Measured cost: the branch route ran 3 rollouts in 656 s vs 1175 s for 2 sequential plain rollouts (1.8× on wall clock against a sequential baseline), at essentially identical token spend — it buys a byte-identical starting world and one environment build, not cheaper tokens. Both numbers are measured, not asserted.Bugs the real runs caught that the fakes could not
Each lives exactly in the seam where a test double replaces the real thing:
COPY skillsinto a Dockerfile that is never built for a child adopting a restored sandbox, anddeploy_skillstreated that line as proof the pack was in the image.0.00—DockerSandbox.restore()re-created the container without the original bind mounts, verifier outputs were never downloaded, and the child-runner turned "no reward" into a real-looking0.0. The command printed a confident attribution verdict computed from numbers that were never collected, exit code 0. Now: unscored ⇒ arm error,reward: null, no verdict, exit 1.restore()did not reproduce the container — it now inspects the live container before removing it and replays binds/volumes/tmpfs, network and cpu/memory; "are verifier outputs mounted" is a live query of the daemon instead of a stale attribute, failing toward downloading; and the remaining fail-open path (container id unresolvable) now raises instead of silently producing a mountless container.verifier/. No reported number was wrong, but the evidence was.no-skillarm could silently measurewith-skill— if the parent itself ranwith-skill, the pack is baked into the image, so restoring that snapshot and "turning skills off" changed nothing while the report kept the label. Skill deltas now fail closed unless the parent's own mode isno-skill(the one mode that provably bakes nothing).Plus, from adversarial review: a stale branch
valuecould survive on a node when V is undefined; the stage-branch path skipped the non-empty-layer check the cursor path enforces.Known limitations, stated rather than left to be found
(Updated for the current head; the earlier version of this section predated the review-response commits and contradicted them.)
skill_mode,injected_prompt,config_override, and the service-level slice ofenvironment_ref); image-changing environment deltas fail closed by design, with a typed error explaining why.result.json/timing.json/prompts.jsonset (fresh children natively; in-place children synthesized from their own captured state).--keep-snapshotsexports sandbox-layer images only; environment-layer sqlite snapshots live inside the container and are not separately exported. The per-stage lifetime annotation covers the whole entry honestly.n_skill_invocationsis adapter-blind (it counts an ACP eventcodex-acpnever emits), so it reads 0 even for arms that demonstrably read the pack. The ablate report deliberately does not surface it; a real fix is wider than this PR.bench eval ablate: image builds share thebf__<task>:latesttag across skill modes, so a concurrent pair races. The fail-closed skill check caught this as a contaminated run rather than mis-measuring it. Worth a per-config tag; not attempted here.Happy to split this into the individual workstream PRs if that reviews better. Tracks FrontierPhysics#73.
Response to the exact-head review (2026-08-28)
All six findings are addressed in nine commits on this branch (
87f8efc7..f796f966), each pinned by tests that were verified red on the pre-fix tip:ArtifactCustodyError, hold dir preserved on any failure,rmtreeonly after every entry is confirmed moved back; a custody failure never masks a child's own exception (87f8efc7)divergence_events,strict_divergencenow trips on content; RFC workspace digest implemented (workspace_digest.py, recorded with basis ornull+reason) (07d4b090)--keep-snapshotsexports the image viadocker saveto<out-dir>/snapshots/<ref>.tar(path+sha256 recorded) before cleanup; without the flag the report recordsephemeral: true, exported: null; RFC §3.6 wording corrected (59b02d17)run_ablationresolves the same per-task assemblybench eval runuses (extractedtask_rollout_config()) and overlays only the ablation axis; parent and child configs now carry non-nulltask_digest,reasoning_effort, source identity, prompts, usage settings — pinned by tests (df0d4360)branch_policy.py/branch_transaction.py/branch_children.py/branch_report.py;branch()≈15 cc / 41 statements,_run_childrensuccessor takes one params object; behavior-neutral (identical full-suite results before/after, zero test edits) (a3db4448)5c891c20);bench eval continue --cut-stage <name>resolves recorded stages (d02502c5);bench eval ablate --mark-research-end-on <path>capturespost-researchwhen the marker file (e.g.PLAN.md) appears and branches there (d40c3743); task authoring acceptsbranch_execution: forked-snapshotand compiles it to a stage-capture request (f796f966)Gates at tip
f796f966: targeted371 passed, 4 skipped; full suite6156 passed, 0 failed;ruff check/ruff format --check/ty check src/clean. Docker live suite at this exact tip:5 passed in 104.85s(real daemon, docker 29.6.0).Response to the second exact-head re-review (2026-08-29)
find -print0 | sort -z | xargs -0) and fail-closed (per-stage status checked; verified against busybox ash) — your exact repro is now a red-first test: two trees differing insidefile with spaces.txtmust digest differently, and a newline filename must never succeed wrongly (ae311174)stage_snapshots.jsonmarking every refephemeral: trueunless exported;--keep-snapshotsadded tobench eval run(same export machinery), and a tested import path exists —bench eval import-snapshots <run-dir>verifies the tar sha256,docker loads it, and checks the loaded image id against the recorded one, failing closed on any mismatch (e22b6d45)build_eval_planrejects unsupported reasoning effort before any provisioning; deterministic ACP rejections classify asrequest_global(the #917 non-retryable pattern), joining retry exclusions, andrun_ablationskips all children when the parent failed request-globally (e5ed14c8)ablate.py674 lines (+ablate_arms.py481);run_ablationpassesruff --select C901,PLR0912,PLR0915clean; behavior-neutral (identical suite results before/after) (7ff1a632)architecture.mdnow states composed container+environment checkpoints are implemented (agent-session remains future work); RFC §3.6 and this PR's Known-limitations section updated to match the code (8fc8404f)Gates at the new tip: full suite
6190 passed, 0 failed; docker live suite6 passed(grew by one: a live proof that a restored-from-tar image id matches the recorded one);ruff/ format /tyclean.