fix(cli): re-stamp the policy snapshot on resume (#189)

[pbean]

Closes #189.

The bug

RunState.policy_snapshot is stamped at exactly two places, both at run creation: cmd_run and _start_sweep. _resume_paused_run reloads policy.toml, hands it to the rebuilt engine, and never re-stamped — so a resumed run carried two policies:

• Enforcement reads live self.policy: the per-story budget check in _finish_commit, and every value threaded onto a SessionSpec.
• Display reads the snapshot via RunState.cache_read_weight(): the run summary, bmad-loop status, three TUI sites, the session-end journal's tokens_weighted, and the whole policy block of the diagnose bundle — which claims to describe the run that was executed.

Edit limits.cache_read_weight between launch and resume and the run enforces at the new weight while every display reports the old one. The legal range is 0.0–1.0, so that is a silent 10x disagreement at the extremes. A single session-end entry could likewise carry tokens_weighted at the snapshot weight beside budget_weighted at the live one.

Why this shape

The contract was already documented in four places — README.md, docs/FEATURES.md, docs/tui-guide.md, and the TUI settings screen all say "changes apply to new runs and resumes." Enforcement honored it; the snapshot did not. That made this a plain bug rather than a semantics choice.

The invariant now established: policy_snapshot is stamped at every engine-process start — run, sweep, resume — and equals the Policy that process enforces for its whole lifetime. Policy is loaded once per process and frozen, so this is exact.

This does not weaken the #129 invariant that displays read the snapshot and never self.policy. Re-stamping removes the divergence at its source instead of moving displays onto live policy; both invariants now hold at once. Engine.summary's comment and one test docstring cited the resume gap as their justification — that premise is reworded, the conclusion ("do not unify these") stands.

Run scope and mode (source, spec_folder, epic_filter, target_branch, …) stay pinned at launch, so a policy edit still cannot redirect a live run. The snapshot can therefore disagree with those fields; that is correct, and commented as such.

Forensics

run-resume entries gain cache_read_weight (always), policy_changed (bool), and cache_read_weight_was (only when it moved). Re-stamping re-weights the run's whole history, so entries written before the resume would otherwise stop being reconstructible from the newer snapshot — the exact guarantee #129 exists to provide.

Scalars only, never policy keys or values. Journal entries are unsanitized at write time, and diagnostics._scrub_entry scrubs unknown fields with scrub_json, not the key-aware _scrub_policy that reduces plugins.settings to plugin ids and adapter.env/extra_args to counts. Since identifiers legally contain dots, a dotted key-path diff would ship plugins.settings.<plugin>.<setting_name> verbatim into a diagnose dump. Verified empirically: the raw snapshot holds the planted secret, _scrub_policy emits settings: ['tea'], secret absent.

A trap worth reviewing closely

Policy.to_dict() returns tuples for verify.commands, extra_args and plugins.enabled; the persisted snapshot round-trips them back as lists, and () != []. A plain comparison therefore reports "policy changed" on every resume, including one where policy.toml was never touched — the common case. The comparison normalizes through json.dumps(..., sort_keys=True) on both sides, the same transform save_state applies. test_resume_under_an_unchanged_policy_reports_no_change resumes twice to pin it.

Visible behavior changes

• A run resumed across a weight edit re-weights its whole accumulated history, not just post-resume sessions — totals are recomputed from raw counts. This is what the budget has always done, so displays now match enforcement instead of drifting from it.
• A pre-0.8.2 run with no snapshot gets one on its first resume, so it stops displaying at the hardcoded 0.1 default.
• clear_pause() is now durable immediately (the CLI never called save_state at all before; it relied on Engine._save()), so status shows a live run right after resume rather than lingering on PAUSED. write_pid runs before save_state so no observer catches a "not paused + dead pid" window, which tui/data.py classifies as INTERRUPTED.

Testing

• 7 new tests in tests/test_cli.py, each confirmed failing before the fix — the primary one failed assert 0.1 == 0.5, and status reported 260 weighted where the budget judged 660. They cover: the stamp being durable before engine.run(), the whole tree (not just the weight), the sweep arm, the legacy empty-snapshot case, status end to end, and both journal cases.
• Full suite: 2514 passed, 1 skipped, including the subprocess E2E resume tests.
• trunk check (no filter) clean.
• Drove the real diagnose CLI end to end: post-resume it reports the re-stamped policy block, weighted: 660, and the run-resume scalars intact through scrubbing, with a planted secret absent from the dump.

Summary by CodeRabbit

• Bug Fixes

  • Resuming a paused run now refreshes policy settings before execution begins.
  • Weighted token totals across the full run history now match the policy enforced after resume.
  • Status views, the TUI, diagnostics, and run summaries consistently display updated policy weights.
  • Resume records now indicate policy changes and previous/current cache-read weights.
  • Older runs without a saved policy snapshot receive one when first resumed.

• Documentation

  • Clarified policy snapshot behavior and budget calculation updates across user guides.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b568197f-a2e4-4276-b2ba-41a0fe5183fa

📥 Commits

Reviewing files that changed from the base of the PR and between 16e707f and 8f4ba6c.

📒 Files selected for processing (9)

• CHANGELOG.md
• README.md
• docs/FEATURES.md
• docs/tui-guide.md
• src/bmad_loop/cli.py
• src/bmad_loop/engine.py
• src/bmad_loop/model.py
• tests/test_cli.py
• tests/test_engine.py

---

Walkthrough

Resume now re-stamps and persists the current policy before engine startup, records weight changes in the journal, and validates consistent weighted totals across resumed, sweep, legacy, and unchanged-policy runs.

Changes

Resume policy synchronization

Layer / File(s)	Summary
Restamp policy before engine startup src/bmad_loop/cli.py, src/bmad_loop/engine.py, src/bmad_loop/model.py, README.md, docs/..., CHANGELOG.md	Resume records normalized policy changes and old/new weights, updates policy_snapshot, and persists state before starting the engine. Documentation describes restamping and full-history reweighting.
Validate resume state and accounting tests/test_cli.py, tests/test_engine.py	Tests cover timing, full-policy updates, sweep and legacy runs, status totals, journal fields, and unchanged-policy behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ResumeCommand
  participant _resume_paused_run
  participant policy_mod_load
  participant state_json
  participant Engine
  ResumeCommand->>_resume_paused_run: resume paused run
  _resume_paused_run->>policy_mod_load: load current policy.toml
  policy_mod_load-->>_resume_paused_run: policy snapshot and cache_read_weight
  _resume_paused_run->>state_json: persist policy_snapshot and resume metadata
  _resume_paused_run->>Engine: start with updated state and policy

Loading

Suggested reviewers: dracic

Poem

A rabbit hops through policy’s door,
Restamps the weights from floor to floor.
The journal notes what changed today,
While totals bloom in one true way.
Squeak—resume, and run away!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and accurately summarizes the main change: re-stamping the policy snapshot on resume.
Linked Issues check	✅ Passed	The changes re-stamp policy_snapshot on resume, persist it before execution, and add tests/docs covering the display and journal consistency fixes from #189.
Out of Scope Changes check	✅ Passed	The docs, comments, and tests all support the snapshot re-stamping fix; no unrelated code changes are evident.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/restamp-policy-snapshot-on-resume-189

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}