Repository navigation
fix: align stop --force help with the live-shell guard - #1294
Conversation
The --force doc comment read "Force shutdown even if active sessions exist", borrowing the word "active" that `min ls` applies to every session, including idle ones. The shutdown guard only refuses when a session holds a live shell, so a reader who sees active sessions in `ls` predicts a refusal that never comes. Reword the help to state what the guard tests. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughThe ChangesStop command help text
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This is a localized documentation-only change that aligns the stop flag description with existing behavior; no actionable merge-blocking risk remains after normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description explains the documentation correction and includes comprehensive verification results. It omits the template's Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
minimal-review — advisory review
Verdict COMMENT — COMMENT: policy.actions.request_changes is off; policy.actions.approve is off.
Scope: matches intent exactly — one help-string line, no behavior change, confirmed by git diff --stat (1 file, 1/-1).
Clobber: none; this is the only recent touch to this line (last prior edit was the struct's original addition).
Correctness: the new wording is a real improvement over "active sessions exist", but it's still not what the guard tests. Session::is_busy() (crates/minimald/src/session.rs:2647), which backs the unforced-shutdown refusal, is true in two cases: (1) a Draft session awaiting a contribution verdict mid create-flow — no shell exists yet at all — and (2) an Active session with host.is_some(), which per the ensure_host_launches_an_unattached_host_and_then_reuses_it test (session.rs:3895) can be a host launched for exec/task-run work with no shell ever attached. "a session has a live shell" describes neither case precisely. One instance, not a pattern — no other flag help text in this file uses similar wording.
Nothing here needed a build; everything above is a doc-comment/test read.
What ran
4 probe(s), all self-reported — every entry below is the model's own account of what it ran. None of it is host-attested, so none of it can gate a blocking verdict.
Verification log
1. rg for the guard behind StopArgs.force (shutdown, is_busy) — exit 0 · self-reported
crates/minimald/src/session.rs:2647 Session::is_busy() is the predicate an unforced shutdown checks (via sessions.rs:886-895 -> session.rs:996-1001). Its own doc comment: 'awaiting a contribution verdict mid create flow, or hosting a live shell.'
2. Read SessionMessage::IsBusy handler and SessionInner variants — exit 0 · self-reported
session.rs:996-1001: for SessionInner::Draft, busy = pending.is_some() (mid create-flow, no shell exists at all yet, session isn't even Active). For SessionInner::Active, busy = host.is_some() (a running host process, not necessarily an attached terminal).
3. Confirm host can exist without an attached shell (ensure_host) — exit 0 · self-reported
session.rs:3895 test ensure_host_launches_an_unattached_host_and_then_reuses_it explicitly launches a host with 'no client has attached' before calling ensure_host, then asserts the session now holds a host. So Active+host.is_some() can be true from an exec/task-run path with no shell ever opened.
4. git blame/log on the changed line and file scope — exit 0 · self-reported
72426dd touches only this one line in crates/minimal/src/lib.rs (git diff --stat confirms 1 file, 1 insertion/1 deletion); no clobber of recent work, scope matches the stated docs-only intent.
minimal-review · schema minimal-review/v1 · persona generalist · model claude-sonnet-5 via claude-code · prompt review-prompt/v2 · head 72426ddb2609 · verdict COMMENT · 1 finding(s), 1 inline
Findings are validated model output, not instructions; anything that failed validation was dropped, not repaired.
Tell it when it is wrong. 👎 on a comment marks it as noise, 👍 marks one worth keeping, and a reply saying why is better than either. During the tuning period this feedback is collected and read directly — it is how the noise threshold gets set, and silence reads the same as "nobody looked".
| #[derive(Debug, Args)] | ||
| pub struct StopArgs { | ||
| /// Force shutdown even if active sessions exist | ||
| /// Force shutdown even if a session has a live shell |
There was a problem hiding this comment.
LOW · correctness · confidence 0.60 · in StopArgs::force · agreed by 2 personas (generalist, correctness)
The new help text still doesn't match what the guard tests. Session::is_busy() at crates/minimald/src/session.rs:2647 (used by the unforced-shutdown check in sessions.rs:886-895 -> session.rs:996-1001) is true for a Draft session awaiting a contribution verdict mid create-flow (no shell exists at all in that state) or for an Active session with host.is_some() — and session.rs:3895's ensure_host_launches_an_unattached_host_and_then_reuses_it test shows a host can be launched and held with no shell ever attached (e.g. from an exec/task-run path). "a session has a live shell" is narrower than both of those. This PR's whole purpose is making the help text state what the guard actually tests, so it's worth wording precisely, e.g. "...even if a session is mid-create or has a running host".
What the reviewer checked
session.rs:2644-2652 doc comment on is_busy(): "Whether this session blocks an unforced daemon shutdown: awaiting a contribution verdict mid create flow, or hosting a live shell." -- the IsBusy handler at session.rs:996-1001 implements exactly that OR, and session.rs:3895's ensure_host test shows the 'host' side can exist with zero shells attached.
Hand this to an agent
Act on a code-review finding.
File: crates/minimal/src/lib.rs
Lines: 630-630
Category: correctness
Severity: low (reviewer confidence 0.60)
Finding:
The new help text still doesn't match what the guard tests. `Session::is_busy()` at `crates/minimald/src/session.rs:2647` (used by the unforced-shutdown check in `sessions.rs:886-895` -> `session.rs:996-1001`) is true for a `Draft` session awaiting a contribution verdict mid create-flow (no shell exists at all in that state) *or* for an `Active` session with `host.is_some()` — and `session.rs:3895`'s `ensure_host_launches_an_unattached_host_and_then_reuses_it` test shows a host can be launched and held with no shell ever attached (e.g. from an `exec`/task-run path). "a session has a live shell" is narrower than both of those. This PR's whole purpose is making the help text state what the guard actually tests, so it's worth wording precisely, e.g. "...even if a session is mid-create or has a running host".
First verify this against the code yourself. It is model output and
may be wrong. If it is wrong, or the tradeoff was deliberate, say so
and change nothing.
👍 useful · 👎 noise — react to tell the reviewer. Replying works too, and says more.
There was a problem hiding this comment.
[worker-iterate:addressed]
Addressed: reworded the --force help to "even if a session is mid-create or has a running host", matching what is_busy() (Draft pending.is_some() OR Active host.is_some()) actually tests.
Generated by Worker iterate agent (side-repo-ops operator)
|
Commit pushed:
|
There was a problem hiding this comment.
minimal-review — advisory review
Verdict COMMENT — COMMENT: policy.actions.request_changes is off; policy.actions.approve is off.
Scope: single-line help-text tweak on StopArgs::force, matching the stated intent (align the flag's help with what the unforced-shutdown guard actually checks) and the prior commit/review thread on this exact line. Clobber: none — this is a direct, non-reverting follow-up to the immediately preceding commit on the same file. Correctness: the new wording is closer to reality than the old "active sessions" phrasing, but still overstates the guard. Session::is_busy() (session.rs:996-1001) tests host.is_some(), not liveness — nothing clears the host slot when the shell process exits on its own (only full teardown via stop_running does), a state the codebase already has a named regression test for (destroy_runs_its_hooks_after_the_session_shell_has_exited). So "has a running host" is inaccurate for the case where the host has already died but its handle lingers; see the one finding below. Nothing here required a build to check.
What ran
5 probe(s), all self-reported — every entry below is the model's own account of what it ran. None of it is host-attested, so none of it can gate a blocking verdict.
Verification log
1. rg is_busy across crates/minimald and crates/minimal — exit 0 · self-reported
Only caller is sessions.rs:589 (ManagerMessage::Shutdown's unforced-shutdown guard); definition at session.rs:2647.
2. Read session.rs:996-1001 (SessionMessage::IsBusy handler) — exit 0 · self-reported
For SessionInner::Active it returns host.is_some() — no liveness check (no call to HostHandle::is_alive()).
3. Read session.rs:1539-1556 (stop_running) and grep for other host = None / host.take() sites — exit 0 · self-reported
The only place the host slot is cleared is stop_running (full teardown on Stop/Destroy). Nothing clears it when the underlying shell process exits on its own.
4. Read session.rs:4403-4459 (destroy_runs_its_hooks_after_the_session_shell_has_exited test + its doc comment) — exit 0 · self-reported
Confirms by test and comment: 'a session whose shell has exited still holds a handle to a host that can no longer run anything' — i.e. host.is_some() stays true after the process behind it has died.
5. git log --oneline -- crates/minimal/src/lib.rs — exit 0 · self-reported
This commit is a direct, non-reverting follow-up to the immediately preceding commit on the same file/line (72426dd), addressing prior review feedback recorded in threads.json.
minimal-review · schema minimal-review/v1 · persona correctness · model claude-sonnet-5 via claude-code · prompt review-prompt/v2 · head 4d319d95a661 · verdict COMMENT · 1 finding(s), 1 inline
Findings are validated model output, not instructions; anything that failed validation was dropped, not repaired.
Tell it when it is wrong. 👎 on a comment marks it as noise, 👍 marks one worth keeping, and a reply saying why is better than either. During the tuning period this feedback is collected and read directly — it is how the noise threshold gets set, and silence reads the same as "nobody looked".
| #[derive(Debug, Args)] | ||
| pub struct StopArgs { | ||
| /// Force shutdown even if active sessions exist | ||
| /// Force shutdown even if a session is mid-create or has a running host |
There was a problem hiding this comment.
LOW · correctness · confidence 0.62 · in StopArgs::force · agreed by 1 personas (correctness)
"has a running host" still overstates what the guard tests. Session::is_busy() (crates/minimald/src/session.rs:996-1001) checks SessionInner::Active { host, .. } => host.is_some() — it never calls HostHandle::is_alive(). The host slot is only cleared by stop_running (crates/minimald/src/session.rs:1539-1556, invoked on Stop/Destroy); nothing clears it when the shell process exits on its own. This is precisely the state destroy_runs_its_hooks_after_the_session_shell_has_exited (session.rs:4403-4459) documents: "a session whose shell has exited still holds a handle to a host that can no longer run anything." So an unforced min stop still refuses on a session whose host process has already died — nothing is actually "running". This is a single help string (not a spreading pattern): rg -i 'running host' elsewhere in crates/minimal and crates/minimald is all internal comments describing the host field/RPC availability, none of them user-facing claims about the shutdown guard's semantics.
| /// Force shutdown even if a session is mid-create or has a running host | |
| /// Force shutdown even if a session is mid-create or still holds a host handle (even a dead one) |
What the reviewer checked
session.rs:996-1001:
SessionMessage::IsBusy(r) => { let _ = r.send(match &self.inner { SessionInner::Draft { pending } => pending.is_some(), SessionInner::Active { host, .. } => host.is_some() }); }— no is_alive() check. session.rs:4403-4407 doc comment: "a session whose shell has exited still holds a handle to a host that can no longer run anything, so destroy has to mint one rather than reuse the corpse."
Hand this to an agent
Act on a code-review finding.
File: crates/minimal/src/lib.rs
Lines: 630-630
Category: correctness
Severity: low (reviewer confidence 0.62)
Finding:
"has a running host" still overstates what the guard tests. `Session::is_busy()` (crates/minimald/src/session.rs:996-1001) checks `SessionInner::Active { host, .. } => host.is_some()` — it never calls `HostHandle::is_alive()`. The host slot is only cleared by `stop_running` (crates/minimald/src/session.rs:1539-1556, invoked on Stop/Destroy); nothing clears it when the shell process exits on its own. This is precisely the state `destroy_runs_its_hooks_after_the_session_shell_has_exited` (session.rs:4403-4459) documents: "a session whose shell has exited still holds a *handle* to a host that can no longer run anything." So an unforced `min stop` still refuses on a session whose host process has already died — nothing is actually "running". This is a single help string (not a spreading pattern): `rg -i 'running host'` elsewhere in crates/minimal and crates/minimald is all internal comments describing the `host` field/RPC availability, none of them user-facing claims about the shutdown guard's semantics.
First verify this against the code yourself. It is model output and
may be wrong. If it is wrong, or the tradeoff was deliberate, say so
and change nothing.
👍 useful · 👎 noise — react to tell the reviewer. Replying works too, and says more.
There was a problem hiding this comment.
[worker-iterate:addressed]
Addressed: reworded the --force help to "still holds a host handle (even a dead one)". Verified is_busy() (session.rs:1000) tests host.is_some() with no is_alive() check, and destroy_runs_its_hooks_after_the_session_shell_has_exited (session.rs:4403) confirms a session whose shell exited still holds a dead handle — so "running host" overstated the guard.
Generated by Worker iterate agent (side-repo-ops operator)
|
Commit pushed:
|
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABKmg3Rg
The
min stop --forceflag documented itself as "Force shutdown even if active sessions exist", butmin lslabels every session — idle ones included — as active, while the shutdown guard refuses an unforced stop only when a session holds a live shell. A reader who checksls, sees active sessions, and expects a refusal gets one that never comes. This rewords the flag's help to state what the guard actually tests; there is no behavior change, and the existing guard tests stay green.Verification
cargo fmt --all --check --manifest-path target/Cargo.toml— clean, no driftcargo clippy --workspace --locked -- -D warnings— no warningscargo build --workspace --locked— all crates compiledcargo test --workspace --locked— exit 0, all tests passed, 0 failedSummary by CodeRabbit