fix(adapters): complete dev sessions only on Stop or window death#53
Merged
pbean merged 8 commits intoJul 4, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCompletion now waits for authoritative stop/death signals, fenced ChangesStale result handling and completion gating
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant GenericAdapter.wait_for_completion
participant tmux window
participant GenericAdapter._final
participant devcontract.strip_auto_run_result
participant Engine._reset_spec_for_repair
participant runs.rearm_escalation
GenericAdapter.wait_for_completion->>tmux window: Stop / window death / idle tick
alt live window with stale result artifact
GenericAdapter.wait_for_completion->>GenericAdapter._final: finalizes with accept_result=false
GenericAdapter._final-->>GenericAdapter.wait_for_completion: stalled
else dead window or authoritative Stop
GenericAdapter.wait_for_completion->>GenericAdapter._final: finalizes with accept_result=true
GenericAdapter._final-->>GenericAdapter.wait_for_completion: completed or crashed
end
Engine._reset_spec_for_repair->>devcontract.strip_auto_run_result: remove stale section
runs.rearm_escalation->>devcontract.strip_auto_run_result: remove stale section
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
A terminal artifact observed while the agent window is still alive is advisory: the idle-tick and grace-expiry shortcuts in wait_for_completion could return completed mid-turn, letting run()'s finally-kill terminate a working agent and hand the engine half-written state. Remove both shortcuts (the window is provably alive at those points), pin the grace-expiry stall verdict via _final(accept_result=False), and strip the stale terminal section when a spec is re-armed for repair so a re-driven spec cannot qualify as a result artifact from its first save. Fixes bmad-code-org#48
dracic
force-pushed
the
fix/dev-idle-result-window-alive
branch
from
940c2f9 to
ca288c7
Compare
rearm_escalation flipped the spec back to ready-for-dev but left the escalated attempt's `## Auto Run Result` section in place, so a resolved re-drive still qualified as a terminal artifact from its first in-session save — the untouched Stop path would then parse the prior attempt's stale outcome (typically `Status: blocked`) as this session's result. Strip the section alongside the status flip, mirroring engine._reset_spec_for_repair on the in-loop repair path, so both re-arm paths are covered. Refs bmad-code-org#48 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t scans find_result_artifact qualified any fresh-mtime spec whose body contained `## Auto Run Result` at column 0 — including a heading quoted inside a fenced code example — so such a spec read as this session's terminal artifact from the agent's first save. parse_auto_run_result likewise picked a fenced copy as the "last" section and truncated detail at fenced `## ` lines (quoted shell comments, log output). Route all three scans (parse, find, strip) through one shared fence-aware heading/boundary helper so a fenced example is never structure and the detectors no longer diverge from the stripper. Closes bmad-code-org#52 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collaborator
|
Thanks for the excellent PR — the issue write-up (#48), the adversarial follow-up (#52), and the test coverage made validation straightforward. Everything checked out: the removed shortcuts sit strictly after the same-tick liveness return so the window is provably alive there, all four profiles emit canonical Stop events, the base adapter never arms the stall grace (so triage/non-dev sessions were never on the removed paths), and window-death still honors the artifact. I pushed two maintainer commits onto the branch (maintainer edits are enabled):
Full suite + trunk green locally on both commits; CI matrix re-running on the new head. 🤖 Generated with Claude Code |
Collaborator
|
augment review |
🤖 Augment PR SummarySummary: Makes dev-session completion authoritative only on turn-end Changes:
Technical Notes: Fence tracking now considers both fence character and run-length (CommonMark-style) to avoid misclassifying nested/shorter fence lines as closes. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
…e rules) The parity count in _fenced treated every ```/~~~ line as a fence toggle, ignoring CommonMark's close rules. A shorter run inside a longer fence, or a different fence char, flips parity early — exposing a fenced (documentation) `## Auto Run Result` heading as a real section. On the strip path that is a destructive misread: legitimate fenced content gets deleted on re-arm/repair. Track the open fence's character and length and close only on a same-char run at least as long as the opener with no trailing info string. All existing fence tests (simple paired 3-backtick fences) are unaffected; adds parse/find/strip tests for the nested-longer-fence and mismatched-char cases plus a closed-then-real positive control. Addresses the Augment review note on PR bmad-code-org#53. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
@CodeRabbit review |
|
Kicking off the review now. ✅ Action performedReview finished.
|
The top-of-tick liveness probe and the grace-expiry stall return sit in the same idle tick with a (small) gap between them. Because that return exits the loop, a hard death (killed without a SessionEnd hook) landing in the gap is never caught by a later top-of-tick probe — and accept_result=False then discards a just-flushed terminal artifact that window death is supposed to honor, feeding the destructive stalled -> RETRY -> rollback path. Re-probe _window_alive once immediately before finalizing: a now-dead window flows through the crash path (which trusts its artifact via accept_result=True), so the outcome only ever upgrades (stall -> crash, or -> completed when the artifact is present). decide_dev/decide_review_session treat crashed and stalled identically, so the artifact-absent case is unchanged. A transport error on the re-probe is not proof of death (mirrors the top-of-tick handling) and falls through to the stall; spec.timeout_s bounds a persistent failure. Narrowly closes the in-tick death race; the broader window-alive reconcile gap remains issue bmad-code-org#61. Addresses the Augment review note on PR bmad-code-org#53. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
augment review |
|
✅ Action performedReview finished.
|
…esult strip A `## Auto Run Result` quoted inside a fence nested under list indentation (4+ absolute leading spaces, past `_FENCE_LINE_RE`'s 0-3-space bound) is co-indented with the fence, so it never matches the column-0-anchored AUTO_RUN_HEADING_RE — nothing to strip. Locks that symmetry so any future leading-space tolerance in the heading regex fails loudly instead of destructively stripping quoted spec prose (reviewer #3522359669 on PR bmad-code-org#53). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
augment review |
reset_spec_status and strip_auto_run_result read the spec unconditionally, so a re-drive/repair against a spec that no longer exists on disk crashed the entry point. On the runs.rearm_escalation path this was reachable directly: verify.set_frontmatter_status already no-ops on a missing file, so the following strip_auto_run_result was the first line to raise; on engine._reset_spec_for_repair the sibling reset_spec_status raised one line earlier. Guard both with the same `if not spec_path.is_file(): return False` used by verify.set_frontmatter_status / read_frontmatter / synthesize_result. Scope is deliberate: only an absent spec is guarded. A present-but-unreadable spec or a failing write still raises — silently skipping the strip after the caller has flipped the frontmatter status would leave the re-opened spec carrying its stale `## Auto Run Result` section, the exact state that makes the re-driven session's first save read as a terminal result (the bug this PR fixes). Addresses Augment review comment 3522384764 on PR bmad-code-org#53. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
augment review |
A bare `##` line is a valid empty CommonMark ATX heading, so `_next_heading_start` (`^##\s`) bounding a section there is correct, not a premature truncation. The `Status:` gate is parsed above the boundary and is untouched; terminal detection keys on `## Auto Run Result`, never a bare `##`, so a surviving empty-heading region can't read as a result artifact. On the destructive strip path this is the safe direction (truncate early -> delete less). Tightening `\s` to a space/tab delimiter would drop empty-heading recognition on both LF and CRLF and run the strip PAST such a heading, deleting more. Locks the intended behavior for parse + strip (reviewer #3522512350 on PR bmad-code-org#53). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
|
augment review |
This was referenced Jul 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #48
GenericAdapter.wait_for_completioncould returncompletedfrom the idle-tick and grace-expiry shortcuts on terminal-artifact presence alone, while the agent window was still alive —run()'sfinally: kill()then terminated the agent mid-turn and the engine post-processed half-written state (feeding the destructive auto-rollback path).Changes:
crashedearlier in the same tick), so the checks could never legitimately complete._finalgains a keyword-onlyaccept_resultflag; the grace-expiry stall verdict passesFalseso an artifact under a live window cannot silently upgradestalledtocompleted. Window-death paths (crashed,SessionEnd) keep the trusting default — death is authoritative.strip_auto_run_result(new, indevcontract) removes the stale## Auto Run Resultsection when the escalation re-arm re-opens a spec, so a re-driven spec no longer qualifies as a terminal artifact from the agent's first mid-session save. The helper is fence-aware (incl. fences indented up to 3 spaces per CommonMark) since deletion is destructive; the read-only heading scans are intentionally untouched (follow-up: [BUG] find_result_artifact / parse_auto_run_result treat a fence-quoted heading as a real terminal section #52).Behavioral notes:
Tests
result_json=None; window death with artifact still completes; strip unit tests incl. fenced/indented-fence edge cases; engine re-arm strips the stale section with the intent body untouched.test_module_skills_syncdrift failures, unrelated.Summary by CodeRabbit
result.jsonno longer overrides an in-progress/stalled verdict while the window is alive; grace expiry and window death are honored correctly.