fix(win32): show UNKNOWN for live-but-unreadable engine instead of false INTERRUPTED

[dracic]

What

Adds a tri-state ProcessHost.liveness_of (alive/dead/unknown) and routes only the TUI run-status through it, so a running engine whose identity is unreadable on win32 shows UNKNOWN instead of a false INTERRUPTED.

Why

On win32, psutil.create_time() raises ERROR_ACCESS_DENIED for a live process in another session / at a different elevation, so identity() returns None; the identity-aware liveness check then read a running engine as dead, weakening resume/delete guards.

Fixes #33

How

• Added tri-state ProcessHost.liveness_of biased away from false-dead: an unreadable identity on a still-existing pid reads unknown, not dead.
• Kept the shared alive_and_ours strict/fail-safe so the destructive stop_run gate and engine_alive delete/archive/reclaim guards are unchanged — an unverifiable pid is never signalled and never blocks cleanup.
• Wired only tui.data.liveness to the tri-state, and made the launch-conflict / resume double-drive guards treat unknown as possibly-live (_engine_possibly_live: has a pid and liveness != dead).

Testing

pytest tests/test_process_host.py tests/test_tui_app.py tests/test_tui_data.py — added cases for the unreadable-but-live pid (unknown), the strict destructive guards, and the TUI UNKNOWN status / possibly-live prompt paths.

Summary by CodeRabbit

• New Features
  • Added more nuanced run liveness detection, including an “unknown” state when process status can’t be confirmed.
  • Improved launch, resume, and resolve safeguards to treat uncertain runs as potentially active and prompt for confirmation.
• Bug Fixes
  • Reduced false assumptions that a run is definitely dead when its identity can’t be read.
  • Updated warning messages to better reflect uncertainty and prevent accidental double-starts.

[coderabbitai]

Warning

Review limit reached

@dracic, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 41 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d71c5d4f-110d-4239-9dc5-cf089b918c15

📥 Commits

Reviewing files that changed from the base of the PR and between ba3d528 and 6d9774d.

📒 Files selected for processing (2)

• src/automator/tui/app.py
• tests/test_tui_app.py

Walkthrough

Introduces a tri-state liveness_of method on ProcessHost distinguishing alive/dead/unknown outcomes when process identity is unreadable but the process exists. Wires this through tui/data.liveness() and TUI app run-control, resume, and resolve flows via a new _engine_possibly_live helper, softens modal wording, and adds corresponding tests.

Changes

Tri-state liveness detection and propagation

Layer / File(s)	Summary
ProcessHost.liveness_of tri-state helper src/automator/process_host.py, tests/test_process_host.py	Adds liveness_of(pid, identity) returning 'alive'/'dead'/'unknown', treating unreadable-but-existing identity as unknown; clarifies alive_and_ours docstring; adds tests for alive/dead/unknown outcomes and legacy identity=None fallback.
data.liveness delegates to liveness_of src/automator/tui/data.py, tests/test_tui_data.py	liveness() now calls get_process_host().liveness_of() directly instead of mapping alive_and_ours; adds a test verifying an unreadable-identity live PID surfaces as UNKNOWN, not INTERRUPTED.
TUI app run-control uses possibly-live detection src/automator/tui/app.py, src/automator/tui/screens/modals.py, tests/test_runs.py	Adds _engine_possibly_live() treating 'unknown' liveness with a readable PID as possibly live; updates _guarded, action_resume_run, action_resolve_run to use it with adjusted messaging; softens ConfirmResumeModal wording to "may still be live"/"could double-drive"; updates test comment.
TUI app smoke tests for unknown/pidless liveness tests/test_tui_app.py	Adds smoke tests covering unknown-PID runs, legacy pidless-but-alive runs, resume warnings, and resolve refusal, verifying confirmation modals appear and launches are blocked.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant TuiApp
  participant EnginePossiblyLive
  participant Data
  participant ProcessHost
  User->>TuiApp: trigger run/resume/resolve action
  TuiApp->>EnginePossiblyLive: _engine_possibly_live(run_dir)
  EnginePossiblyLive->>Data: liveness(run_dir)
  Data->>ProcessHost: liveness_of(pid, identity)
  ProcessHost-->>Data: alive / dead / unknown
  Data-->>EnginePossiblyLive: status
  EnginePossiblyLive-->>TuiApp: true / false
  TuiApp-->>User: show confirmation modal or warning

Loading

Possibly related PRs

• bmad-code-org/bmad-auto#30: Both PRs modify identity-aware process liveness plumbing in ProcessHost and tui/data.liveness().

Suggested reviewers: pbean

Poem

A pid once dead, or so we thought,
Turned out "unknown" was what we sought,
No more false-dead, no more alarm,
Just tri-state hops, safe from harm,
This bunny thumps in glee! 🐰

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: showing UNKNOWN instead of false INTERRUPTED for live-but-unreadable Windows engines.
Linked Issues check	✅ Passed	The changes implement tri-state liveness for unreadable-but-existing pids and keep destructive checks strict, matching issue #33.
Out of Scope Changes check	✅ Passed	The added launch/resume/resolve guard updates and tests are all directly tied to the liveness-safety fix.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[augmentcode]

🤖 Augment PR Summary

Summary: This PR fixes win32 run-status reporting by distinguishing between a dead engine and a live engine whose identity can’t be read.

Changes:

• Added a tri-state ProcessHost.liveness_of() that returns alive/dead/unknown and biases away from false-dead when identity is unreadable.
• Kept alive_and_ours() as the strict check for destructive operations (signal/cleanup gates).
• Updated TUI liveness to use the tri-state probe so unreadable-but-live engines show UNKNOWN instead of INTERRUPTED.
• Adjusted TUI launch/resume/resolve guards and user-facing wording to treat unknown as “possibly live”.

Tests: Added coverage for the unreadable-identity case, strict vs non-destructive guards, and the TUI UNKNOWN / “may be live” flows.

_{🤖 Was this summary useful? React with 👍 or 👎}

[augmentcode]

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

[Copilot]

Pull request overview

This PR introduces a tri-state engine liveness probe (alive/dead/unknown) to avoid reporting a live-but-unreadable engine as INTERRUPTED on win32 (e.g., psutil.AccessDenied while the PID still exists). The tri-state is routed into the TUI run-status and launch/resume/resolve conflict guards, while destructive lifecycle paths remain strict via alive_and_ours.

Changes:

• Added ProcessHost.liveness_of(pid, identity) -> 'alive'|'dead'|'unknown' and kept alive_and_ours strict for destructive paths.
• Updated TUI liveness/status classification to surface unknown as UNKNOWN instead of INTERRUPTED, and adjusted conflict/resume/resolve guarding to treat unknown as possibly-live.
• Added tests covering the TUI UNKNOWN status and confirmation/warning flows, plus unit tests for liveness_of.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/automator/process_host.py	Adds tri-state liveness_of while preserving strict alive_and_ours for destructive operations.
src/automator/tui/data.py	Switches TUI liveness probing from boolean to tri-state and maps unknown to UNKNOWN status.
src/automator/tui/app.py	Updates TUI conflict/resume/resolve guards to treat unknown as possibly-live.
src/automator/tui/screens/modals.py	Adjusts resume warning text to reflect “may still be live” semantics.
tests/test_process_host.py	Adds coverage for the new tri-state liveness_of behavior.
tests/test_tui_data.py	Verifies unreadable-but-live PID is shown as UNKNOWN (not INTERRUPTED).
tests/test_tui_app.py	Adds UI-flow tests for unknown PID confirmation/warnings and resolve refusal.
tests/test_runs.py	Updates test commentary to clarify strict behavior alignment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/automator/tui/app.py (1)

463-484: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Delete/archive guards still ignore "unknown" liveness — contradicts the stated safety objective.

action_delete_run and action_archive_run still gate solely on data.liveness(run_dir) == "alive". Since "unknown" is not "alive", a run whose engine is live-but-unreadable (the exact win32 scenario this PR is fixing) will not be blocked from deletion/archival — the guard falls through and proceeds. This is the opposite direction of action_stop_run's fail-safe behavior (which refuses to act when unsure).

The PR's own issue objective explicitly calls for this case: "preserve resume/delete/archive/reclaim safety guards so unverifiable processes are not treated as safely dead." As written, unknown is effectively treated as safely dead by these two guards, risking file deletion/archival while the win32 engine may still be writing to run_dir.

Since _engine_possibly_live is already defined in this file and used by the sibling resume/resolve/launch guards, extending it here is low-effort:

🛡️ Proposed fix

     def action_delete_run(self) -> None:
         selected = self._selected_run_dir()
         if selected is None:
             return
         run_id, run_dir = selected
-        if data.liveness(run_dir) == "alive":
+        if _engine_possibly_live(run_dir):
             self.notify(f"run {run_id} is live — stop it first", severity="warning")
             return

     def action_archive_run(self) -> None:
         selected = self._selected_run_dir()
         if selected is None:
             return
         run_id, run_dir = selected
-        if data.liveness(run_dir) == "alive":
+        if _engine_possibly_live(run_dir):
             self.notify(f"run {run_id} is live — stop it first", severity="warning")
             return

Do you want me to open an issue to track extending _engine_possibly_live to the delete/archive/reclaim guards, or is this deliberately deferred to a follow-up in the PR stack?

Also applies to: 496-517

🤖 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/tui/app.py` around lines 463 - 484, The delete/archive guard
logic in action_delete_run (and the matching action_archive_run path) only
blocks when data.liveness(run_dir) is "alive", so "unknown" still falls through
and is treated as safe to delete/archive. Update these guards to use the
existing _engine_possibly_live helper, or otherwise reject both "alive" and
"unknown", so unverifiable runs are handled fail-safe like action_stop_run and
the other safety checks in app.py.

🤖 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/automator/tui/app.py`:
- Around line 156-166: The liveness check in the launch flow is flagging
terminal runs because `discover_runs()` results are passed directly into
`_engine_possibly_live()`. Update the `live` collection logic in `app.py` to
filter out terminal run states such as FINISHED, STOPPED, CRASHED, and
INTERRUPTED before probing liveness, so only non-terminal runs are checked and
shown in the `ConfirmModal`.

---

Outside diff comments:
In `@src/automator/tui/app.py`:
- Around line 463-484: The delete/archive guard logic in action_delete_run (and
the matching action_archive_run path) only blocks when data.liveness(run_dir) is
"alive", so "unknown" still falls through and is treated as safe to
delete/archive. Update these guards to use the existing _engine_possibly_live
helper, or otherwise reject both "alive" and "unknown", so unverifiable runs are
handled fail-safe like action_stop_run and the other safety checks in app.py.

🪄 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: ccde1b94-4118-409a-af5f-59f0034c4429

📥 Commits

Reviewing files that changed from the base of the PR and between b9a7705 and ba3d528.

📒 Files selected for processing (8)

• src/automator/process_host.py
• src/automator/tui/app.py
• src/automator/tui/data.py
• src/automator/tui/screens/modals.py
• tests/test_process_host.py
• tests/test_runs.py
• tests/test_tui_app.py
• tests/test_tui_data.py

[pbean]

Validated against #33 and the review threads — approving. ✅

What I checked:

• #33 intent satisfied. Tri-state liveness_of on the ProcessHost base (composing identity() is None with is_alive()) is functionally equivalent to catching AccessDenied inside identity() on the read path, with zero per-platform edits — arguably cleaner than what the issue proposed, and it inherits into any future host backend for free.
• Keeping runs.engine_alive strict is the right call. The "unknown must not block stop/delete" invariant is real and pre-existing (runs.py docstring), and warn-not-block on the TUI delete/archive confirms satisfies #33's "not treated as safely dead" without letting an unverifiable/recycled pid block cleanup forever. Agree with the 6d9774d resolution.
• Bot threads verified against HEAD, not just replies: the legacy pid-less live-session gap (Augment/Copilot) is genuinely fixed in ba3d528 with test coverage; the TUI delete/archive warning landed in 6d9774d with test coverage.
• POSIX safety: Linux is bit-for-bit unchanged (/proc/<pid>/stat is world-readable, so unknown is unreachable there; recycled pids still read dead via identity mismatch). macOS is strictly improved — a live engine with unreadable psutil create_time previously read false-dead exactly like win32. PosixProcessHost.is_alive already maps EPERM → True, so the composition is correct. Destructive force-kill gating untouched. CI green on py3.11–3.14.

Two non-blocking points:

1. Launch guard probes terminal runs (CodeRabbit's still-open inline nit at app.py _guarded). _engine_possibly_live now runs over every discovered run, including FINISHED/STOPPED/CRASHED, which _classify deliberately never probes — so each launch double-probes live runs, and on win32/macOS a finished run whose pid got recycled by an unreadable process false-prompts "another run may be live" forever. The status already computed by discover_runs avoids both, no second probe needed:

   live = [
       r.run_id
       for r in data.discover_runs(self.project)
       if r.status == data.RUNNING
       or (r.status == data.UNKNOWN and runs.read_pid(r.run_dir) is not None)
   ]

   This preserves your legacy-session coverage (session-alive runs classify as RUNNING) and the pid gate for UNKNOWN. Small enough to fold into this PR if you like, but not blocking.

2. CLI parity — filed as #38. cmd_resolve/cmd_delete/cmd_archive still gate on the strict boolean, so on win32 the CLI can resolve (double-drive) or delete a live-but-unreadable engine with no warning, while the TUI now refuses/warns. Follow-up scoped there (tri-state helper at the runs layer so the CLI doesn't reach into TUI modules); fine to land this PR as-is.

🤖 Generated with Claude Code