feat(adapters): post-kill reconcile rescues finished stalled/timeout sessions#95
feat(adapters): post-kill reconcile rescues finished stalled/timeout sessions#95pbean wants to merge 7 commits into
Conversation
Extract _synth_result/_stories_synth_result returning the full devcontract.SynthResult; _result_json stays a thin dict-returning wrapper so every existing caller and test is unchanged. No behavior change — groundwork for #61, whose post-kill reconcile needs status_consistent, which both call sites currently drop.
…sessions (#61) A session that wrote its terminal spec but lost its final Stop ends stalled (nudge-unresponsive under a live window, artifact advisory per the #48/#53 invariant); with total hook loss (misconfig, events-dir write failure) no event ever arrives, the stall grace never arms, and it exits timeout with no artifact check at all. Either way the finished work was discarded solely because the window was alive to distrust. run()'s finally-kill settles that. New _post_kill_reconcile hook on the adapter base (identity by default) runs after the kill on the normal return path only; GenericDevAdapter overrides it for result-less stalled/timeout verdicts: re-probe liveness (alive or unknowable -> verdict stands; unknown is not dead), then re-run the same read-back a delivered Stop would have run. Rescue requires a self-consistent (status_consistent) successful terminal -- done, or the stories plan-halt leg -- with no escalations; blocked is never rescued (no finished work to preserve). Rescued results carry post_kill_reconciled=true and still face the engine's full deterministic verify downstream. Covers dev and review uniformly (both roles run GenericDevAdapter under bmad-dev-auto). The gate is deliberately stricter than the crash path's accept-any-terminal; crashed/completed verdicts already read their artifact at verdict time and are untouched.
A rescued session is otherwise indistinguishable from a live completion in the journal; append session-rescued-post-kill when the adapter's result carries the post_kill_reconciled marker, so a human debugging a run can see the stall/timeout was rescued rather than never happening.
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughThis PR adds post-kill reconciliation for stalled and timeout sessions, hardens terminal artifact parsing against unreadable UTF-8, records rescued sessions in the engine, and expands coverage for rescue, liveness, and decode-failure paths. ChangesPost-kill reconcile feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/adapters/generic.py`:
- Around line 606-608: The post-kill reconcile path in `_post_kill_reconcile()`
should not let unreadable terminal artifacts crash the session. Wrap the
`_synth_result(handle, spec, wait=False)` call in the `GenericAdapter` flow so
`UnicodeDecodeError` and `OSError` are caught, and on failure return the
original `result` just like the existing `sr is None` / inconsistent-status
branch. Keep the fix localized to the `_post_kill_reconcile` / `_synth_result`
call site so corrupt artifacts preserve the original verdict instead of raising.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f042b815-73bf-4b3a-8bc8-6ae08d8b030a
📒 Files selected for processing (6)
src/bmad_loop/adapters/base.pysrc/bmad_loop/adapters/generic.pysrc/bmad_loop/engine.pytests/test_engine.pytests/test_generic_tmux.pytests/test_stories_e2e.py
…#61) The rescue hook re-reads the on-disk spec immediately after run()'s finally-kill. That is precisely the moment a spec the CLI was mid-write is truncated -- possibly through a multi-byte UTF-8 sequence -- so a decode or I/O fault on that read is the expected case, not an anomaly. Nothing contained it. `adapter.run()` is called bare from `_run_session`, and no frame between it and `Engine.run()`'s broad `except Exception` catches: an escaping read error marked the whole RUN crashed, wrote crash.txt, left the task in DEV_RUNNING with no SessionRecord (both `record_session` and `_save` sit after the raise), and abandoned every remaining story. A best-effort rescue must never make things worse than the verdict it was handed. Guard the read-back call and keep the original stalled/timeout verdict, like every other keep-verdict branch. Both exception types are named because UnicodeDecodeError is a ValueError, not an OSError -- the same trap verify.read_frontmatter already documents. Covered on all three read-back legs: the mtime-scan `spec-*.md` (raises from find_result_artifact), the name-matched `bmad-dev-auto-result-*.md` fallback marker (raises from synthesize_result, and is what a `timeout` verdict hits having read nothing at all), and stories mode. Plus a monkeypatched test that pins the guard itself, independent of devcontract's internals.
…ad-back The call-site guard in _post_kill_reconcile contains the rescue hook, but the same two reads are reached from the Stop and crash paths via _result_json, where an unreadable artifact still crashed the run. Fix the source. Two distinct sites, both reachable from a spec truncated mid-write: - find_result_artifact reads each candidate to check for a real (non-fenced) terminal section, guarded by `except OSError`. UnicodeDecodeError is a ValueError, so a torn multi-byte sequence sailed straight through -- the exact trap verify.read_frontmatter's comment already warns about. A candidate that cannot be shown to carry a terminal section does not qualify; skip it like any other unreadable file. - synthesize_result's second read was unguarded entirely. It is reached with a file the finder never read: the `bmad-dev-auto-result-*.md` fallback marker is matched by NAME. Route it through a helper that degrades to "". An unreadable spec carries no parseable result section, so it now reads exactly like a spec that has not terminated yet -- the caller waits, nudges, or keeps its verdict. The repair path keeps the opposite contract on purpose: reset_spec_status and strip_auto_run_result still raise, because silently skipping a rewrite leaves the spec in a state the caller believes it fixed.
pbean
force-pushed
the
feat/61-post-kill-reconcile
branch
from
3eeb337 to
92b73c3
Compare
|
Addressing the Out of Scope Changes pre-merge warning. It flagged two things; they resolved differently. The The decode hardening was genuinely unscoped, and now has a scope of its own. Split out as #96 and declared via
I did not split One more thing, filed rather than fixed: #97. No @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -5
git show d07b9a4 --stat 2>/dev/null | head -30
git show d07b9a4 2>/dev/null | head -60Length of output: 1996
The scope resolution here is sound:
Triggering the review now. ✅ Action performedReview finished.
|
|
@coderabbitai full review (The pre-merge Out of Scope Changes block still carries its pre- |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 13 minutes. |
1 participant
Fixes #61. Fixes #96. Refs #48, #53, #97.
Scope
This PR carries two fixes. The second was surfaced by review of the first, and is tracked separately as #96.
Stopwas lost loses its work. Everything under "Problem" and "Approach" below.They are coupled, which is why they ship together.
_post_kill_reconcileis the one code path that reads a spec immediately afterrun()'sfinally: self.kill(handle)— precisely the moment a spec the CLI was mid-write is truncated, possibly through a multi-byte UTF-8 sequence. The new hook therefore makes a latent decode crash a routine, expected fault rather than an anomaly. Two commits, at two layers:7b2af13contains it at the call site:_synth_resultis wrapped inexcept (OSError, UnicodeDecodeError) -> return result. The rescue keeps its verdict on any read fault. This is the load-bearing invariant — a best-effort rescue must never escalate a clean stall/timeout into an exception — and it does not depend on any distant module's catch clause.1f8918efixes the source (A terminal artifact truncated mid-write crashes the whole run, not just the session #96):find_result_artifactskips an undecodable candidate (it cannot be shown to carry a terminal section), andsynthesize_result's bareread_textdegrades to"". This also closes the pre-existing crash on the Stop/crash paths, which is the failure mode that actually occurs here.Left deliberately unfixed and filed as #97:
verify.read_frontmatterguardsUnicodeDecodeErrorbut notOSError, so a permission fault or a TOCTOU delete still escapes on the Stop path. See the Judgment calls below.Problem
A dev/review session that genuinely finished (terminal spec on disk) but whose final Stop hook event was lost, and which ignores stall nudges, ends
stalledwithresult_json=None— and nothing ever re-reads the on-disk spec after the window dies. The reconcile seam (_reconcile_generic_terminal_status) is completed-gated and structurally unreachable;decide_devroutes the stall to RETRY →_rollback_or_pause, pausing or discarding finished work.Scope finding beyond the issue text: the grace-expiry stall only arms inside the Stop handler, so total hook loss (hook misconfiguration, events-dir write failure — the failure modes #61 names as the likely tail) never produces
stalledat all: it idles totimeout, which performs no artifact check whatsoever. The fix therefore covers bothstalledandtimeout.Approach: adapter-side post-kill hook (not the engine-side sketch in #61)
The re-scan needs adapter-only state — the launch floor (
SessionHandle.launched_ns), rebased artifact dirs, and the stories-mode dispatch — none of which reaches the engine (SessionResultcarries none of it). So:CodingCLIAdapter.run()now passes the result through a_post_kill_reconcilehook after the finally-kill (identity by default; never runs on the exception path).GenericDevAdapteroverrides it for result-lessstalled/timeoutverdicts: re-probe liveness — alive (kill silently failed) orMultiplexerError(unknown ≠ dead, tri-state per fix(win32): show UNKNOWN for live-but-unreadable engine instead of false INTERRUPTED #36/fix(cli): surface unknown liveness in resolve/delete/archive (win32) #39) keeps the verdict — then, on a provably dead window, re-run the exact read-back a delivered Stop would have run.status_consistentsynthesis, no escalations, and a successful terminal —done, or the stories plan-halt leg.This preserves the #48/#53 invariant (an artifact under a live window stays advisory; trust is re-established only by window death), covers dev and review uniformly (both roles run
GenericDevAdapterunder bmad-dev-auto), and the engine's completed path (reconcile → state sync → verify) runs unchanged — a bogus rescue still faces full deterministic verify and degrades into an ordinary verify-failed retry.Rescued results carry
post_kill_reconciled: true, and the engine journalssession-rescued-post-killso a rescue is distinguishable from a live completion in forensics.Judgment calls (flagging for review)
timeoutincluded — widens Hardening: reconcile seam is unreachable for stalled sessions — a finished-but-lost-Stop session cannot be recovered as completed #61's literal "stalled only" wording; justified by the arming analysis above.blockednever rescued — no finished work to preserve; blocked-plus-nudge-unresponsive is weak evidence; the default passive path (rollback_on_failure off) already pauses with the tree intact.status_consistentsemantics: "no active disagreement" — blank frontmatter + prosedonerescues (exactly what a delivered Stop would have synthesized; the engine reconcile repairs the lag). Pinned by a test.post_kill_reconciled, journal kindsession-rescued-post-kill.crasheduntouched — itsaccept_result=Truescan already ran at verdict time.read_frontmatterleft raising onOSError(read_frontmatter: an OSError on the Stop path escapes eight status gates #97) — widening it would make a genuine disk/permission fault degrade silently to "status unknown → clean retry" across ~12 status gates inverify,engine,sweep, andstories, trading loudness for resilience. A missing status and an unreadable disk are different conditions; only the first is safe to treat as "not finished yet".verify.pyis untouched by this PR,stories.resolve_story_speckeeps its ownexcept (OSError, UnicodeDecodeError)(still load-bearing), and the post-kill hook is guarded against it regardless.reset_spec_statusandstrip_auto_run_resultkeep the opposite contract from the read-back: silently skipping a rewrite the caller believes succeeded would leave the re-opened spec carrying its stale terminal section, the exact state that makes the re-driven session's first save read as a result.Commits
refactor(adapters): read-back returns fullSynthResult(both call sites previously droppedstatus_consistent);_result_jsonstays a dict-returning wrapper — zero behavior change.feat(adapters):run()restructure + hook + override + 16 adapter tests, including a real-tmux E2E of the total-hook-loss → timeout → rescue path.feat(engine): journal breadcrumb + engine-level flow-through test.fix(adapters)(7b2af13):_post_kill_reconcilenever raises on a corrupt artifact — the call-site guard.fix(devcontract)(1f8918e): unreadable artifacts degrade to no-result on the read-back — the source fix (A terminal artifact truncated mid-write crashes the whole run, not just the session #96).docs(changelog): post-kill rescue + corrupt-artifact hardening.Testing
run(), exception path never reconciles, and a real-tmux end-to-end rescue.bmad-dev-auto-result-*.mdfallback-marker leg, on the stories leg, and the_post_kill_reconcilekeep-verdict guard.test_dev_grace_result_does_not_complete_while_window_alive(the fix(adapters): complete dev sessions only on Stop or window death #53 invariant test) intentionally unchanged — the live-window stall verdict still stands at verdict time.Engine.run(): pre-fix the run endscrashed=True/crash_error="UnicodeDecodeError: ... 0xff"with the task pinned inDEV_RUNNINGand noSessionRecord; post-fixcrashed=False, taskDEFERREDvia the ordinary retry-exhausted path, session records persisted.trunk checkclean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
session-rescued-post-kill) for rescued runs.Tests