Skip to content

Write the checkpoint that status, resume and handoff already read - #255

Merged
Antawari merged 3 commits into
mainfrom
catrina/checkpoints-need-a-producer
Jul 28, 2026
Merged

Write the checkpoint that status, resume and handoff already read#255
Antawari merged 3 commits into
mainfrom
catrina/checkpoints-need-a-producer

Conversation

@Antawari

@Antawari Antawari commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

The defect, verified in this lane before building

SessionStore.save had no caller anywhere in src/bonfire/, and the pipeline loop had no checkpoint write site. bonfire status, bonfire resume and bonfire handoff all read that artifact. Three shipped verbs, no producer.

Verified with my own greps rather than on the inherited report:

  • grep -rn "SessionStore" src/ — 3 readers (status, resume, handoff), 0 writers.
  • grep -rn "\.save(" src/ — exactly one hit, store.py:85, inside SessionStore.save itself. Nothing calls it.
  • grep -n "checkpoint" src/bonfire/engine/pipeline.py — 3 hits, all docstring/comment. composition.py — zero.

Then reproduced empirically: a real 2-stage run through build_default_engine with the SDK transport faked, BONFIRE_CHECKPOINT_DIR pointed at an empty dir.

run success: True  session: 5ac41494edef  stages: ['scout','warrior']  cost: 0.22
checkpoint files on disk: []
SessionStore.latest(): None

Everything in the brief reproduced. Two claims I want to correct in passing, both mine, neither a contradiction of the brief:

  • The claim is stronger than "a run that gets partway through". Even a fully successful run left nothing.
  • src/bonfire/engine/__init__.py and docs/architecture.md both documented this as intended design ("The engine does not write checkpoints between stages; callers persist a PipelineResult"). So it was a documented extension surface with no in-tree caller, not an oversight nobody had noticed. I corrected the package docstring; docs/architecture.md is another lane's this round and is now stale (see Owed).

Where the checkpoint is now written

PipelineEngine takes an optional checkpoint_sink: CheckpointSink | None. At every stage-group boundary it hands the passed stages to that sink. build_default_engine wires it to the same SessionStore the three verbs read, so one directory is resolved by one rule (BONFIRE_CHECKPOINT_DIR, then ~/.bonfire/checkpoints) rather than two resolutions agreeing by coincidence.

Per stage group, not once at the end — checked against the three things the brief asked about:

  • Cost. A local JSON dump against a stage that just made a billed LLM dispatch. Not measurable.
  • Atomicity. Unchanged, and it is the repo's existing helper: CheckpointManager.save writes a .tmp and os.replaces it, through safe_write_text with its symlink refusals. This matters more now, not less: the write now happens repeatedly while the run is alive, so an interruption mid-write is an ordinary outcome rather than a freak one. It leaves either the previous checkpoint or the new one, never a lying record.
  • Concurrent runs. Each run writes {session_id}.json, so two runs never collide on a file. But latest() picks max-timestamp across the directory, so a second concurrent run does shadow the first for status/resume. That is pre-existing behaviour of latest(), not introduced here, and I did not change it.

Written before the budget check, not after. That group was dispatched and paid for whether or not the next line halts the run; a halt that discarded the record would bill it again on the next attempt.

Only gate-passed stages are ever recorded. stages_done at the call site holds only stages that cleared their gates. Recording a failed stage would be worse than recording nothing, because resume skips whatever the checkpoint names — a failed stage written as done is a stage that silently never runs again.

What the three verbs do with it, run as real separate processes (not CliRunner):

A 2-stage debug run halted by the budget check after scout, then three real
bonfire processes reading only the file it left:

$ bonfire status                     $ bonfire resume
Session 917e3d103d2e                 Resuming session 917e3d103d2e (debug).
  Workflow: debug                      Completed: 1 stage(s) - $0.11 spent.
  Stage:    1 / 2 stages               Remaining: 1 stage(s): warrior
  Cost:     $0.11                      Re-entering the pipeline at the first
  Saved:    2026-07-28 14:35:32Z       remaining stage. Live dispatch requires
                                       ANTHROPIC_API_KEY.
$ bonfire handoff
# Session Handoff - 917e3d103d2e
- **Workflow:** debug
- **Task:** ship the refactor
- **Cost so far:** $0.11
- **Stages completed:** 1
## Completed stages     - scout
## Remaining stages     - warrior
## How to resume
Run `bonfire resume` to re-enter 'debug' from this checkpoint (session 917e3d103d2e).

Every one of those three printed an absence-of-state line before this PR.

Ruling on re-billing

A resume does not re-dispatch and does not re-bill. run(plan, completed=...) skips the named stages in the DAG and seeds total_cost from their envelopes, so prior spend still counts against plan.budget_usd and cannot be spent twice.

Measured at the transport, not inferred from a total the engine computed: the first leg halts after scout (2 transport calls), the checkpoint is fed back, the second leg makes 2 transport calls for the two remaining stages and ends at $0.33 = 3 x $0.11. scout is never re-dispatched, and its output is carried forward into warrior's prompt, which is what makes skipping it legitimate rather than merely cheaper.

The boundary, stated plainly: bonfire resume computes and reports the re-entry; it does not itself dispatch. That is its existing documented contract and I did not change it. What changed is that it now has real data to act on — before this PR it could only ever print "No session to resume." Making the verb actually re-enter the pipeline is a separate ruling for Anta: it turns a currently free verb into a billing verb, and the dispatch driver (cli/commands/run.py) is outside this lane's partition.

Control rods — both halves

Rod A — the producer. Removed the write site from _run_inner.

FAILED test_a_run_leaves_a_checkpoint_on_disk
FAILED test_a_run_that_halts_partway_records_only_the_stages_that_passed
FAILED test_the_verbs_read_the_checkpoint_from_a_separate_process
FAILED test_resuming_from_the_checkpoint_does_not_re_dispatch_or_re_bill
FAILED test_a_sink_that_cannot_write_does_not_fail_the_run
5 failed, 3 passed

Verbatim reasons:

E  FileNotFoundError: [Errno 2] No such file or directory:
   '/tmp/pytest-of-candyfactory/pytest-293/test_a_run_leaves_a_checkpoint0/checkpoints'
E  FileNotFoundError: [Errno 2] No such file or directory:
   '.../test_a_run_that_halts_partway_0/checkpoints/6c79d66b6465.json'

Rod B — the composition root. Restored A, then removed checkpoint_sink=SessionStore() from build_default_engine. The same four behavioural tests plus the wiring assertion go red. Verbatim, and this is the one that matters:

E  AssertionError: assert '944037e708ba' in 'No active session.\n'
E   +  where '944037e708ba' = PipelineResult(success=False, session_id='944037e708ba', ...)

A real bonfire status process printing the pre-fix message while the run it should describe sits in the same test. This is the half a hand-assembled engine cannot catch — every one of these tests goes through build_default_engine, so unwiring the root is visible. Under Rod B the two tests that must not depend on the write (test_an_engine_with_no_sink_writes_nothing, the Protocol conformance test) stayed green, so the rod discriminates rather than just breaking everything.

Both restores via git checkout — byte-identical by construction, git status clean, 8/8 green after each.

Budget — measured, and three raises taken

Measured cf-file-budget check as an early action, before authoring. Findings at that point: src/bonfire/engine 1 line of headroom, tests/integration 0, tests/unit 1.

Restructured twice to shrink the ask before raising anything:

  1. PipelineResult construction + the durability reasoning moved to SessionStore.save_progresssession/ is unbudgeted and is the layer that owns what a stored record means.
  2. The sink is None guard + error handling moved to write_progress in engine/checkpoint.py — the module that already owns the atomic write and the symlink refusals.

That took pipeline.py from +52 down to +5, and the engine package from +78-with-a-fat-pipeline to +78 spread properly.

entry from to lines
src/bonfire/engine/pipeline.py 989 994 +5
src/bonfire/engine 1903 1981 +78
tests/integration 1363 1702 +339

Committed separately (ab63a61), appended at the end of package_raises, three number changes + three entries, nothing reformatted. Each carries what the lines buy and the alternatives rejected in my own words. Headline rejections:

  • Deleting the 4-line comment to land on exactly 989. That is buying a number with the reasoning for the subtlest ordering decision in the change.
  • Inlining the guard and try/except into _run_inner. Costs zero new lines in pipeline.py — but adds two branches to a function frozen in the complexity snapshot at 23.
  • Putting the Protocol in bonfire/protocols.py. Unbudgeted, would have cost zero — but CLAUDE.md's release gate names "the four @runtime_checkable extension protocols" as a v0.1 trust-triangle item, and quietly making it five edits a documented count from a lane that does not own that doc.
  • Writing from an event consumer in the unbudgeted events package. Not a budget trade — it does not work. StageCompleted carries stage_name, agent_name, duration_seconds, cost_usd and no Envelope, so a consumer cannot populate CheckpointData.completed at all.
  • tests/unit. Deliberately untouched: a second live lane needs that ceiling this round, and hand-building the engine there is the measurement error this defect family is made of.

exemptions.json untouched. tests/unit untouched.

Gates

5685 passed, 3 skipped, 37 xfailed, 20 xpassed   (baseline on 7bddd71: 5677 — +8, exactly this PR's tests)
ruff check           All checks passed!
ruff format --check  375 files already formatted
cf-file-budget       clean
lint-imports         Contracts: 1 kept, 0 broken
complexipy           only the 2 pre-existing reds (bard.py::handle, wizard.py::handle)

_run_inner and _execute_stage stay at their snapshot complexities — the change adds a call, not a branch.

What I did NOT prove

  • No billed dispatch. No box run. The transport is faked at claude_agent_sdk.query throughout.
  • I did not kill a live process mid-write. "Survives the process exiting" is proved by three real bonfire subprocesses reading a file the pytest process wrote, plus the pre-existing atomicity test on tmp+os.replace. I did not SIGKILL a run between the safe_write_text and the os.replace.
  • Concurrent runs are untested. Two runs write distinct files, but latest() shadowing is unproven either way and unchanged by this PR.
  • resume does not dispatch. Proved that its computed inputs, fed to the engine, do not re-bill. Did not prove a CLI-driven resume end to end, because the verb does not do that.
  • status / resume derive the stage total and the remaining list from the registry keyed by plan_name, not from the checkpoint. Found while writing these tests: a plan name whose registered shape differs from what actually ran is graded against the registered shape. Harmless today (names always come from the registry) but it means a workflow whose stages change between run and resume will compute its remainder against the new plan. Not fixed — out of this lane's scope, and it is the registry's contract to settle.

Owed

  • docs/architecture.md lines ~168 and ~219-241 now describe the old behaviour ("PipelineEngine.run() does not write checkpoints", "SessionStore.save exists and has no caller"). docs/ is another lane's this round, so I left it. It needs the correction.
  • CHANGELOG.md untouched by instruction. The line this lane would have written: bonfire run now writes a checkpoint at every stage boundary, so bonfire status, bonfire resume and bonfire handoff report a real run instead of an empty store.

Follow-up: cf-exemptions went red in CI, and why the fix is a re-anchor

CI's gate / gate failed on:

src/bonfire/engine/pipeline.py:158: UNREGISTERED_SUPPRESSION: 'BLE001' ...
src/bonfire/engine/pipeline.py:599: UNREGISTERED_SUPPRESSION: 'BLE001' ...

Neither suppression is mine and neither is new. Both exist on 7bddd71 at lines 154 and 589, and both are already registered in exemptions.json (entries 15 and 16, blessed under the Elegance Law). exemptions.json anchors by line number. My insertions pushed 154 -> 158 and 589 -> 599, so the anchors stopped matching.

The suppressions did not change. The registry lost its anchor. The gate's message ("a self-issued suppression is not an exemption") cannot tell those two cases apart.

Proof: cf-exemptions is OK on bare 7bddd71 and FAILs on this branch on exactly those two rows.

Narrowing them to OSError was tried and rejected on evidence. It makes cf-exemptions pass and turns 10 tests red:

FAILED TestNeverRaise::test_handler_exception_returns_failed_result
FAILED TestNeverRaise::test_gate_exception_does_not_crash_pipeline
FAILED TestPipelineNeverRaises::test_raising_policy_fails_gracefully
FAILED test_wave_11_pipeline_outer_exception_failed_emit.py  (5 tests)
FAILED test_unknown_gate_fails_the_run_instead_of_passing_it
FAILED test_the_engine_the_root_builds_refuses_an_unregistered_gate
E  RuntimeError: handler exploded
E  bonfire.engine.gates.UnknownGateError: Stage 'scout' names gate 'ghost', absent from the gate registry

pipeline.py:158 is the barrier behind the documented contract "PipelineEngine.run() NEVER raises"; :599 wraps handler.handle() where StageHandler is an open-set Protocol. Narrowing them would also re-open the unregistered-gate defect PR #251 landed yesterday. The OSError precedent from #249 is right for new defensive handling around a disk write — and my own checkpoint write already uses except (OSError, ValueError) in write_progress. It is wrong for these two.

Fix: re-anchored by qualified symbol, which the gate accepts (symbol_or_line == str(line) or == suppression.symbol) and which does not drift:

154 -> PipelineEngine.run
589 -> PipelineEngine._execute_stage

Symbols computed with the gate's own _symbol_spans / _enclosing_symbol, not guessed. Both unique in the file, one suppression each, so no symbol swallows a second site.

Count-neutral: 52 entries in, 52 out, frozen_count untouched. Nothing added, nothing raised. The diff is two lines.

Two rods on the re-anchor:

  • Gate still bites — injected a genuinely new # noqa: BLE001 in PipelineEngine._emit: cf-exemptions: FAIL (1 violation(s)). Not defanged.
  • Anchor is drift-proof — inserted 30 lines above both sites (they moved to 188/629): cf-exemptions: OK. A corrected number would have broken again on the next insertion.

The real finding: line-anchored exemptions silently un-register

A registry that anchors suppressions by line number un-registers them whenever anything is inserted above. Two lanes hit this in the same hour on different files (this one on engine/pipeline.py, a sibling on onboard/flow.py and onboard/orchestrator.py). That is a defect in the gate's design, not in either lane's code, and it will keep firing until the remaining 50 numeric anchors become symbols. This PR converts 2 of them.

Why local did not equal CI

Not tool availability — cf-exemptions was installed and working locally the whole time. I hand-picked four gate binaries (cf-file-budget, ruff, lint-imports, complexipy) instead of running the aggregate cf-gate that CI runs, so the fifth gate was never invoked.

Running cf-gate locally is itself not a clean signal: on bare 7bddd71 it reports FAIL - 4 of 11 (cf-import-contract, mypy, complexipy, pytest) because the kit venv has no installed bonfire to resolve. The workable local==CI rule in this setup is: run cf-gate and diff its verdict against bare origin/main, and treat only the delta as yours. Against that baseline this branch's only delta was cf-exemptions, now green.

Antawari and others added 3 commits July 28, 2026 08:29
Three shipped verbs read one artifact and nothing wrote it.
SessionStore.save had no caller anywhere in src/bonfire/ and the
pipeline loop had no checkpoint write site, so a bonfire run that
dispatched stages and spent money was followed by "No active session".

The engine now takes an optional CheckpointSink and hands it the passed
stages at every stage-group boundary, which the composition root wires
to the same SessionStore the three verbs read. Written per group rather
than once on the way out: the value of the record is that it survives a
run which does not reach its end. Written before the budget check, not
after: that group was paid for whether or not the next line halts.

Only gate-passed stages are ever recorded, so a resumed run never skips
a stage that failed. Resume does not re-bill -- run(completed=...) skips
the named stages and seeds their cost, proved here by counting transport
calls rather than re-reading a total the engine computed.

The sink takes facts rather than a PipelineResult: a run in progress has
no result, and building one at the call site would mean the engine
stamping a success value onto a question it has not answered.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three entries appended, each sized to a measurement and none padded.

  src/bonfire/engine/pipeline.py  989 -> 994   (+5)
  src/bonfire/engine              1903 -> 1981 (+78)
  tests/integration               1363 -> 1702 (+339)

tests/integration measured EXACTLY its ceiling on origin/main, so no
pull request could add an integration test at all -- and an integration
test through the composition root is the only shape that catches this
defect family, because every unit test of the run path injects its own
engine factory.

Each entry names what the lines buy and the alternatives rejected. The
engine numbers are what remain after moving the mechanism out of
pipeline.py twice, into session/ and into engine/checkpoint.py.

tests/unit is deliberately untouched: a second lane needs that ceiling
this round, and these tests belong in tests/integration regardless.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two BLE001 suppressions in engine/pipeline.py are pre-existing and
already blessed. exemptions.json anchored them by LINE NUMBER (154, 589).
Inserting the checkpoint write site above them pushed those lines to 158
and 599, so the anchors stopped matching and cf-exemptions reported two
registered exemptions as unregistered.

The suppressions did not change. The registry lost its anchor.

Re-anchored to the enclosing qualified symbols, which the gate accepts
and which do not move when a line is inserted above them:

  154 -> PipelineEngine.run
  589 -> PipelineEngine._execute_stage

Count-neutral: 52 entries in, 52 out, frozen_count untouched. Nothing
added, nothing raised, no suppression written.

Narrowing these to OSError was considered and rejected on evidence: it
turns 10 tests red, including the whole outer-exception parity suite and
the unregistered-gate refusal, because both sites exist precisely to
convert ANY failure into a typed result rather than crash a run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Antawari
Antawari merged commit bb2f7c6 into main Jul 28, 2026
4 checks passed
@Antawari
Antawari deleted the catrina/checkpoints-need-a-producer branch July 28, 2026 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant