fix(cli): surface unknown liveness in resolve/delete/archive (win32) - #39
Conversation
|
Warning Review limit reached
Next review available in: 44 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 (6)
WalkthroughAdds tri-state engine liveness handling across runs, CLI commands, and TUI cleanup paths, with unknown states now warning instead of failing in several flows. Tests were updated to cover alive, dead, and unknown outcomes. ChangesTri-state liveness propagation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 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 |
🤖 Augment PR SummarySummary: Updates the CLI to use a tri-state engine liveness probe on Windows so a live-but-unreadable engine pid reports Changes:
Technical Notes: POSIX behavior is intended to remain unchanged; the new 🤖 Was this summary useful? React with 👍 or 👎 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/automator/runs.py (1)
180-187: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBlind
except Exceptionalso swallowsProcessHostErrormisconfiguration.The
trywraps bothget_process_host()and.liveness_of(...). AProcessHostErrorfromget_process_host()(e.g. an invalidBMAD_AUTO_PROCESS_HOSToverride) is a hard misconfiguration, not a flaky per-pid probe failure — but it's silently downgraded to"unknown"here, which could hide a broken deployment for a long time (every liveness check just looks "unverifiable"). Ruff also flags this as BLE001 (blind except).Consider narrowing the catch to the probe call only, or letting
ProcessHostErrorpropagate.♻️ Proposed narrowing
def probe_liveness(pid: int, identity: float | None) -> str: + host = get_process_host() # let ProcessHostError (misconfiguration) propagate try: - return get_process_host().liveness_of(pid, identity) - except Exception: + return host.liveness_of(pid, identity) + except Exception: # noqa: BLE001 - deliberately fail-open to 'unknown', see docstring return "unknown"🤖 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 `@src/automator/runs.py` around lines 180 - 187, The probe_liveness helper currently catches every Exception around both get_process_host() and liveness_of(), which hides ProcessHostError misconfiguration as "unknown". Narrow the try/except in probe_liveness so get_process_host() is called outside the blanket handler, and only the actual liveness_of(pid, identity) probe is downgraded to "unknown"; alternatively, let ProcessHostError propagate unchanged while preserving the tri-state behavior for transient probe failures.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@src/automator/runs.py`:
- Around line 180-187: The probe_liveness helper currently catches every
Exception around both get_process_host() and liveness_of(), which hides
ProcessHostError misconfiguration as "unknown". Narrow the try/except in
probe_liveness so get_process_host() is called outside the blanket handler, and
only the actual liveness_of(pid, identity) probe is downgraded to "unknown";
alternatively, let ProcessHostError propagate unchanged while preserving the
tri-state behavior for transient probe failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65118ce4-5e83-45be-9402-04ddf5079def
📒 Files selected for processing (6)
src/automator/cli.pysrc/automator/runs.pysrc/automator/tui/data.pytests/test_cli.pytests/test_runs.pytests/test_tui_data.py
resolve now refuses on a live-but-unverifiable pid (tri-state `unknown`), closing the double-drive hole; delete/archive warn that the pid is unverifiable before cleanup but still proceed (`unknown` must never block cleanup). Adds runs.engine_liveness / probe_liveness (wrapping ProcessHost.liveness_of) so the CLI reads tri-state liveness without a TUI import, and routes tui.data.liveness through the shared probe. POSIX unaffected. Fixes bmad-code-org#38
ba22162 to
7619ead
Compare
|
Thanks for the reviews — addressed in the latest push:
CI is green across all lanes (incl. Windows 3.11–3.14). |
…verable resolve: split the single != "dead" gate. A provably-live engine always blocks (--force never bypasses it); 'unknown' blocks unless the operator vouches with the new --force. Without an escape hatch a squatted pid with an unreadable identity would lock resolve out forever: `stop` cannot verify or clear an unverifiable pid (stop_run's alive_and_ours guard reads it as not-ours and falls back to mark-stopped without touching engine.pid), so the old "stop it first" remedy looped back to the same refusal. The TUI blocks this case with no override, so CLI --force is the only unknown-resolve escape on any surface. resume: gate cmd_resume (not _resume_paused_run — that helper is also resolve's already-gated re-arm path). 'alive' blocks outright: resume rewrites engine.pid and kills the agent session, double-driving a live run; the pre-existing hole made the resolve gate trivially bypassable. 'unknown' warns but proceeds — resume is the recovery path that rewrites engine.pid, so it must stay usable when liveness is unverifiable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… pids in cleanup/clean Pre-PR, tui.data.liveness had get_process_host() inside its try, so a ProcessHostError (bad BMAD_AUTO_PROCESS_HOST) read as 'unknown'. probe_liveness deliberately propagates it — right for CLI decision paths (main()'s backstop turns it into a clean error), wrong for display: the dashboard poll worker has no except and Textual's exit_on_error takes the whole app down on the first tick, and `list` fails outright where it used to render UNKNOWN rows. data.liveness now catches ProcessHostError and degrades to 'unknown'; the runs layer keeps failing loud. cleanup/clean kept collapsing 'unknown' to not-alive with no warning while delete/archive gained one: prunable_sessions now returns the unknown subset (classification unchanged — unknown never blocks cleanup) so cmd_cleanup and the TUI cleanup worker can flag possibly-live sessions before killing them, and cmd_clean warns per reclaimed run whose liveness is unverifiable. Residual accepted gap: window-only ctl prune stays on the boolean gate with no per-window warning (documented inline) — the session-level warning covers the operator surface. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The two probes encoded the identical decision table by hand (pid<=0, identity-None degrade, identity match, reused/unreadable), and the test fake carried a third copy — a drift risk where the binary path (stop, prune, reclaim) and the tri-state path (resolve/delete/archive gates, TUI) could silently disagree about the same engine. alive_and_ours is now exactly liveness_of(pid, identity) == "alive" (provably equivalent on all branches: gone/reused/unreadable were already not-ours), and _FakeHost subclasses ProcessHost so both derivations are inherited rather than mirrored. Also drops a stale engine_alive comment in the delete test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@dracic thanks — this is a solid implementation of #38, and the review confirmed it does exactly what the issue asked with no POSIX behavior change (the full suite is green on Linux). I ran a deep review pass over it and, rather than round-trip a list of change requests, pushed three commits to your branch (maintainer edits are enabled) addressing what the review surfaced. Summary: 502c5e8 — tri-state resolve/resume gates. Two issues with the
58d17b2 — misconfig degrade + cleanup warnings. d9f5dbe — dedup. Full suite: 1314 passed, 1 skipped; trunk clean. Happy to walk through any of it — and if you disagree with a direction (the |
…win32 CI)
alive_and_ours now routes through liveness_of, which probes is_alive when
identity reads None. test_alive_and_ours_matches_only_same_identity stubbed
only identity, so on the native-Windows lanes the real
PosixProcessHost.is_alive fixture hit os.kill(4242, 0) → WinError 87
(production is unaffected: win32 selects WindowsProcessHost/psutil). Stub
is_alive like the adjacent liveness_of test does and assert not-ours for
both sub-branches — gone, and live-but-unreadable ('unknown' must never
read as ours on the strict path).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/automator/cli.py (1)
857-877: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGood centralization — single liveness sample drives both warn and block.
_stop_or_block_live_engineavoids a warn/block split-brain by samplingengine_livenessonce and branching off the same value, which is a clean fix for the identity-flip race the docstring calls out.One gap: I don't see a test exercising the
force=True+alivepath whereruns.stop_runraisesStopRunError/ProcessHostError(only the happy-path force test is present intests/test_cli.py). Since this helper is now shared by bothdeleteandarchive, a regression here silently breaks both commands' force-stop error handling.🧪 Suggested test to add coverage for the stop-error path
def test_delete_force_stop_error_propagates(tmp_path, monkeypatch, capsys): from automator import runs monkeypatch.setattr(runs, "engine_liveness", lambda _rd: "alive") def _raise(_rd): raise runs.StopRunError("boom") monkeypatch.setattr(runs, "stop_run", _raise) run_dir = _make_run_with_state(tmp_path, "r1") assert cli.main(["delete", "--project", str(tmp_path), "r1", "--force"]) == 1 assert "boom" in capsys.readouterr().err assert run_dir.exists()🤖 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 `@src/automator/cli.py` around lines 857 - 877, The shared liveness helper `_stop_or_block_live_engine` is not covered for the `force=True` path when `runs.stop_run` fails. Add a test in `tests/test_cli.py` that stubs `runs.engine_liveness` to return `"alive"` and makes `runs.stop_run` raise `runs.StopRunError` (and/or `ProcessHostError`), then assert `cli.main(["delete", ..., "--force"])` returns 1 and the error is printed. This should exercise the shared helper used by both `delete` and `archive` so the stop-error handling remains correct.
🤖 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.
Nitpick comments:
In `@src/automator/cli.py`:
- Around line 857-877: The shared liveness helper `_stop_or_block_live_engine`
is not covered for the `force=True` path when `runs.stop_run` fails. Add a test
in `tests/test_cli.py` that stubs `runs.engine_liveness` to return `"alive"` and
makes `runs.stop_run` raise `runs.StopRunError` (and/or `ProcessHostError`),
then assert `cli.main(["delete", ..., "--force"])` returns 1 and the error is
printed. This should exercise the shared helper used by both `delete` and
`archive` so the stop-error handling remains correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24fc2b8d-9013-481f-930a-c51cd4d78fc3
📒 Files selected for processing (11)
src/automator/cli.pysrc/automator/process_host.pysrc/automator/runs.pysrc/automator/tui/app.pysrc/automator/tui/data.pysrc/automator/tui/launch.pytests/test_cleanup.pytests/test_cli.pytests/test_runs.pytests/test_tui_app.pytests/test_tui_data.py
✅ Files skipped from review due to trivial changes (1)
- src/automator/tui/launch.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/automator/tui/data.py
|
Follow-up: the first CI round failed on all four native-Windows lanes — |
|
augment review |
| if unknown: | ||
| self.call_from_thread( | ||
| self.notify, | ||
| f"{len(unknown)} pruned session(s) had an unverifiable engine pid " |
There was a problem hiding this comment.
src/automator/tui/app.py:566: unknown is sampled from runs.prunable_sessions() before runs.prune_sessions() re-partitions; if liveness changes between calls, this notification can claim “pruned session(s) had …” for sessions that weren’t actually pruned. Consider tying the warning list/count to the sessions actually killed (or wording it as “sessions eligible for pruning had …”).
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
prune_sessions now returns (killed, unknown) from its single internal partition sample, and the TUI cleanup worker drops its separate prunable_sessions pre-sample. Previously a liveness flip between the two samples could make the 'pruned session(s) had an unverifiable engine pid' notification name sessions that were never pruned (or miss ones that were). Same single-sample principle as _stop_or_block_live_engine; cmd_cleanup keeps its pre-prune warning, which is state-descriptive by design and needed for --dry-run. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
_stop_or_block_live_engine's force=True + alive branch returns 1 when stop_run raises, but only the happy-path force was tested; a regression would silently break both commands' force-stop error handling. Covers StopRunError via delete and ProcessHostError via archive. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the two new review comments in caa3eb9 + 8a49cea: caa3eb9 — cleanup unknown-pid warning tied to the pruned partition (augment, 8a49cea — force-stop error-path tests (CodeRabbit, Full suite green locally (1317 passed) and trunk clean. |
|
@CodeRabbit review |
|
✅ Action performedReview finished.
|
| print(f"leaving {len(live)} live session(s) untouched") | ||
| return 0 | ||
| killed = runs.prune_sessions(project) | ||
| killed, _ = runs.prune_sessions(project) |
There was a problem hiding this comment.
src/automator/cli.py:931: cmd_cleanup() warns from a prunable_sessions() sample but then prunes via a fresh prune_sessions() sample, so the “unverifiable pid” warnings (and live count) can drift from what was actually killed/left if liveness changes between calls. Consider deriving the warning set (and any related messaging) from the unknown returned by prune_sessions() on the non---dry-run path so the operator-facing output always matches the performed actions.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
…ition prune_sessions now returns the full (killed, live, unknown) partition and cmd_cleanup routes both paths through a single call (dry_run= suppresses the kill). Previously the unknown warnings and the 'left N live session(s) untouched' count came from a prunable_sessions sample taken before prune_sessions re-partitioned, so a liveness flip between the two samples could report a pruned session as left untouched or kill a newly-unknown session without warning. Completes the single-sample story from the TUI fix; the pre-sample rationale (needed for --dry-run) dissolves now that the dry-run path gets the same partition back. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the augment follow-up in eeb1f87:
Full suite green locally (1317 passed) and trunk clean. |
|
augment review |
What
Routes the CLI's
resolve/delete/archivecommands through a tri-state liveness read so a live-but-unreadable engine pid (unknown) is surfaced instead of silently read as dead — bringing the CLI to parity with the TUI half of the fix.Why
On native Windows a running engine whose identity is unreadable (
psutil.create_time()→ERROR_ACCESS_DENIED) reads as tri-stateunknown. The CLI gated only on the strict booleanruns.engine_alive, which collapsesunknowntoFalse, soresolvecould launch a driver against a possibly-live engine (double-drive) anddelete/archiveproceeded with no warning while the engine may still be writing the run dir. POSIX is unaffected.Fixes #38
How
runs.engine_liveness(plus a sharedprobe_livenessbody) wrappingProcessHost.liveness_of, so the CLI reads tri-state liveness without importing TUI modules;unknownis preserved and an unexpected probe failure degrades tounknown, never a falsedead.resolvenow refuses onaliveorunknown(both possibly-live);delete/archivewarnengine may still be live (unverifiable pid)onunknownbut still proceed — an unverifiable/recycled pid must never block cleanup forever. A single liveness sample drives both the warning and the strict block, so a mid-check identity flip can't fire one without the other.tui.data.livenessthrough the sharedprobe_liveness, keeping one copy of the pid-liveness logic and reading the pid file once.Testing
pytest tests/test_runs.py tests/test_cli.py tests/test_tui_data.py tests/test_process_host.py— added cases for the tri-state read (no-pid→dead, alive, reused→dead, unreadable→unknown, probe-raises→unknown),resolverefusing onunknown,delete/archivewarn-but-proceed onunknown, and the TUI delegation regression. Full suite green (1303 passed; the only failures are a pre-existing skill-sync drift, unrelated to this change).Summary by CodeRabbit
--forceoption toresolve, allowing operators to continue when engine liveness is unverifiable (unknown) while still blocking provably-live engines.delete,archive,clean, and cleanup/TUI flows now warn for unverifiable PIDs and proceed safely when appropriate.resumenow blocks provably-live engines, but warns and recovers when liveness is unverifiable.--forcehandling.