fix(sweep): re-drive in-flight bundles by identity on resume#112
Conversation
SweepEngine._loop never had the base engine's _finish_inflight pass. Its only recovery arm lived inside _run_bundle, reachable only once a cycle re-derived the bundle key from the *current* triage plan — so a bundle re-armed by `bmad-loop resolve` survived a resume solely because the cached triage.json reloaded and re-emitted the same bundle name. Lose that cache and a fresh triage partitions the ids under new names, silently orphaning the resolution. _finish_inflight_bundles now runs at the top of the loop, before the ledger is read, and re-drives every non-terminal dw* task under its own persisted story_key. Because its ids leave the open set first, validate_triage rejects any fresh plan that references them — a double-drive is unrepresentable. A still-escalated bundle is terminal and stays untouched. The corrupt-triage.json leg of the issue was unreachable: _read_json is a bare json.loads, so a truncated cache crashed the whole run out of _ensure_triage. It now degrades to a fresh triage, as does a cache holding a non-object. Also: _run_bundle's recovery arm is extracted verbatim to _recover_inflight_bundle and kept as its fallback (the plan-matched path is bit-for-bit unchanged); a missing bundle intent file is regenerated from the task; and a bundle surviving to a cycle is journaled + notified, never dropped. Closes #94
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughSweep resume now redrives persisted in-flight bundles by ChangesSweep resume recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Closes #94
The bug
The base
Engine._loopopens with_finish_inflight()(engine.py:1103), which re-drives every non-terminal task by task identity.SweepEngine._loopoverrides_loopwholesale and never did this: its only recovery arm lived inside_run_bundle, reachable only once a cycle re-derived the bundle key from the current triage plan.So a bundle re-armed by
bmad-loop resolve(rearm_escalation→PENDING,rearmed=True) survived a resume only because the cachedtriage.jsonreloaded and re-emitted the same bundle name → the same_bundle_key. Lose that cache and a fresh triage session partitions the open ids under new names, and the re-armed PENDING task is silently orphaned — the human's resolution is dropped.The "corrupt cache" leg was unreachable.
_read_json(sweep.py:30) is a barejson.loads, so a truncatedtriage.jsonraisedJSONDecodeErrorstraight out of_ensure_triage→ escaped_loop→engine.run()'sexcept Exception→ whole-run crash.txt. Only the missing-file leg reached the fresh-triage fallback.The fix
_finish_inflight_bundles()runs at the top of_loop, before the ledger is read, and re-drives every non-terminaldw*task under its own persistedstory_key— never a key recomputed from the currentsweep_cycle, so a cycle-1 re-arm resumed much later still re-drives correctly.Why a double-drive is unrepresentable
_cycletime →_run_bundle's terminal-return.open_now;validate_triage(result, open_now)rejects any fresh plan referencing them. A fresh triage literally cannot re-bundle them.DEFERRED→ the existingfailed_idsfilter drops the fresh bundle.sweep-nothing-open, no triage session at all.A still-escalated, un-rearmed bundle is
terminal(TERMINAL_PHASESincludesESCALATED) and stays untouched — pinned by the existing test attests/test_sweep.py:983and by a new guard test.Also in this PR
_recover_inflight_bundle(task) -> bool—_run_bundle's recovery arm extracted verbatim;_run_bundlekeeps it as its fallback, so the plan-matched path is bit-for-bit unchanged._ensure_triagereload hardening —json.JSONDecodeError/OSError/UnicodeDecodeErrordegrade to a fresh triage. Added anisinstance(cached, dict)guard too: a top-level JSON array would otherwise still crash onrj.get(...)insidevalidate_triage._ensure_bundle_intent— the dev prompt consumes onlytask.bundle_file, andintent.mdpersists in the run dir, so it is reused as-is, never re-parsed. Only when the file is gone is a degraded intent rebuilt from the task;_write_intentre-attaches the verbatim ledger bodies, which the replacement prose names as the contract. The triage session's authored prose is the one unrecoverable piece._warn_stranded_bundles()— belt-and-braces invariant in_cycle: a non-terminaldw*task surviving to a cycle is journaled + notified rather than silently dropped (the issue's no-silent-caps fallback). With the recovery pass in place it should be unreachable.sweep-inflight-redrive,sweep-inflight-stranded,sweep-intent-regenerated.Deliberately out of scope
_resumable_sessionis not adopted for bundles. Base_finish_inflightcontinues a task whose host died in the post-session window by replaying the recorded result (engine.py:1124);_recover_inflight_bundlestill restarts it. Lifting that is a resume-fidelity change of its own and wants its own tests._read_jsonitself is unchanged. Thedecisions.jsonread (sweep.py:~775) keeps its current contract; only_ensure_triage's call site is guarded.Known acceptable drifts
max_bundlesslots under the fresh-triage path.pre_bundle/post_bundlefor a recovered bundle now fire beforepre_sweep_cycle— mirrors base-engine_finish_inflight-before-pick semantics. No test or plugin asserts on the ordering.Testing
2030 passed, 1 skipped; fulltrunk checkclean.7 new tests. Verified they fail without the
sweep.pychange (revertedsweep.pyalone, re-ran): the 6 regression tests fail, whiletest_escalated_unresolved_still_skipped_when_triage_json_lostpasses both before and after — it is a guard, not a regression test.test_rearmed_bundle_redrives_when_triage_json_lost— the headline regression; 2 sessions, no triage.test_fresh_triage_different_bundle_name_no_double_drive— parametrized overmissing/{{{/{}; fresh plan renames the surviving bundle; exactly 5 sessions, each dw id closed once by the bundle that owned it. The invalid-JSON leg pins the no-crash path.test_restore_patch_latch_honored_when_triage_json_lost— restore semantics survive the move out of_run_bundle.test_escalated_unresolved_still_skipped_when_triage_json_lost— escalated bundle untouched; overlapping fresh bundle still dropped viafailed_ids.test_interrupted_bundle_redrives_by_identity_after_triage_loss— non-rearmed mid-dev crash → restart arm,cause="stopped".test_regenerated_intent_when_bundle_file_missing— regenerated file carries the verbatim### DW-1entry and reaches the dev prompt.test_stranded_bundle_task_warns_loudly— the invariant fires for a ghostdw*task, and not for a terminal bundle orsweep-triage.The six existing resume pins named in the design were each traced and stay green; I also diffed the test file to confirm no existing test lost its tail (
common collected test ids: zero disappeared, 7 added).Summary by CodeRabbit