[rominf]

Why

rocm services stop <id> --yes printed "Local server stopped" and exited 0 based only on which tool ran, even when the verified stop could not confirm the server's processes had exited. In that case the CLI had in fact kept the record's PIDs, its stop request and the endpoint key, so the message described a stop that did not happen, and a script checking the exit status moved on as if the server were down. rocm's own stop_server answer hard-coded "status": "stopped" and dropped the verdict the stop had already computed. rocmd's twin of the same code reports this case as stop_unconfirmed (#499).

What changes

	Before	After
rocm services stop, stop not confirmed	Local server stopped on stdout, exit 0	Local server stop not confirmed on stderr, exit 1, with what happened to each recorded process and what was kept
rocm services stop, stop confirmed	Local server stopped, exit 0	unchanged
stop_server answer (rocm sandbox-run)	"status": "stopped" always	stopped or stop_unconfirmed, taken from the stop's verdict; still exits 0 with the verdict in the JSON, as rocmd sandbox-tool does
Ctrl-C in attached rocm serve, stop not confirmed	reported as stopped, exit 0	the same report on stderr, non-zero exit, nothing signalled on its behalf
Ctrl-C in attached rocm serve, the stop itself errors	signals the child it launched, exit 0	unchanged, except that the stop's error and any failure of that signal now go to stderr and are audited at warn/error

The stop report carries the verdict (stopped), each PID's outcome (pid_outcomes, using rocmd's labels), whether the record changed while the stop ran (record_changed) and whether a usable endpoint key was kept (endpoint_key_kept). The printed lines are derived from those fields, so the message cannot disagree with what the stop wrote to disk:

• a process that, or a process under it, did not exit after it was signaled;
• a process whose identity could not be verified as the server's: nothing is signalled, and a ps -p command to check on it is printed;
• a record that changed while the stop ran: a note pointing at rocm services;
• a container engine whose own stop failed (ROCMAI-554: add engines/aim Docker-based AIM serving engine #708): that failure as the reason;
• kept: names the record, and the endpoint key only when a usable one is on disk.

The report deliberately names no command to run the stop again. terminate_verified reports a dead recorded root as already gone without checking the tree under it, so a second stop could confirm, delete the key and print "stopped" while a process under that root still runs. rocmd's comments and MCP error text no longer present a repeated stop as recovery either.

README, the services stop and serve --help text, docs/testing.md and docs/manual-testing.md describe the new output, the exit codes and which JSON fields are authoritative.

Verification

• Unit tests assert the message together with the state: exit status or error, the record's PIDs and stop marker, the endpoint key on disk, the audit level and message. They cover the timed-out, unverified, mixed, changed-record, keyless and container-engine cases, and the attached Ctrl-C decision. Each was checked to fail with its production part reverted.
• Scenarios service-stop-03, -04 and -05 (mock lane, Linux) drive a real rocm services stop through the e2e-test-hooks seam ROCM_E2E_STOP_OUTCOME (timed out, unverified, keyless). They assert stderr, exit 1, the kept record, marker and key, and that no stop command is named. They fail on a binary built without the seam, which shows they go through it. service-stop-01 and -02 are unchanged and still pass.
• cargo test --workspace --all-targets, cargo test -p xtask, clippy -D warnings with and without e2e-test-hooks, cargo fmt --check, a Windows-target cargo check -D warnings (x86_64-pc-windows-gnu), feature_naming and the e2e build pass.
• Not covered by a scenario: the attached rocm serve Ctrl-C stop. Attaching needs a real rocm serve launch with an engine and a runtime the mock lane does not have; the attached_ctrl_c_ unit tests cover everything after the stop answers.
• Not shown end to end: that an unverified PID is never signalled. The seam replaces terminate_verified, so the scenarios prove the report and the state, not the signalling.

Scope

Follow-ups, not in this change:

• terminate_verified treats a dead recorded root as already gone without checking the processes under it. That predates this change, and fixing it is what would make suggesting a retry safe again.
• rocm remote stop, and the cleanup of a part-failed rocm remote serve, relay the remote's unconfirmed-stop report verbatim, including check lines meant for the remote machine. Their non-zero exit also needs documenting, with a docker-lane scenario.
• A seam inside the identity read, plus a rocm-core unit test for the indeterminate path, so that "an unverified PID is never signalled" is tested.
• The dashboard collapses the stop-failure envelope, so an unconfirmed stop there is not reported distinctly.

rocm services stop now exits non-zero when the stop is not confirmed. Callers that treated exit 0 as "stopped", including #299's uninstall flow if it relies on that, will see the failure, which is the intent.

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No row refers to this behaviour.)
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. (No new subcommand; the stop rendering lives in services_text.rs.)
• Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state — not only the wording — per AGENTS.md §3.

[nowycondro]

🔴 Automated review · pr-review-watcher · df8fc99

Review — no blocking findings

Full review of the whole change (base 6ab12083…head df8fc992), split by file-coupling into five groups and fanned out as independent workers; only one returned in time (see What this covered for what that means for the rest).
No ticket claimed closed by this change. It cites #499 (rocmd's earlier stop-identity-verification fix), #708 (a container-engine stop failure) and #299 (the uninstall flow) as background/related context only — not as issues this PR resolves.

Blocking

None

Non-blocking

• Stale comment overstates what pid_outcomes contains — apps/rocmd/src/mcp.rs:802-803
  Unchanged by this diff, but sits immediately beside wording this PR did edit, in the same comment block. It says pid_outcomes in the structured result "names the PID that could not be confirmed," but pid_outcomes (built in apps/rocmd/src/service.rs around line 100-109) is a list of every recorded PID with its role and outcome, not only the unconfirmed one.
  Confidence 80 · mechanical · Fix: reword to "pid_outcomes names every recorded PID's outcome, including which one could not be confirmed."

Decisions for the author

• finish_attached_stop's "stop failed, and the direct signal also failed" branch still exits 0 — tradeoff
  In apps/rocm/src/service_launch.rs, once this PR lands, two of the three branches of the attached rocm serve Ctrl-C stop correctly exit non-zero on an unconfirmed/uninformative outcome. The third — the managed stop call itself errored, and the direct fallback signal to the child also failed — still returns Ok(()) (exit 0), same as before this PR. That's pre-existing behavior, not changed here, and the PR's own description names it explicitly ("exit 0, as it always has here"), so this isn't a defect introduced by this diff. Flagging only so the choice is a confirmed one: nothing was actually stopped and the direct kill failed too, yet the command now sits as the one path in this three-way match that still reports success.

Positive signals

• rocm sandbox-run stop_server and rocmd sandbox-tool stop_server deliberately keep exiting 0 with the verdict carried in JSON (status: stop_unconfirmed) — verified in main.rs's InternalSandboxRun handler, which prints the raw JSON unconditionally — while only the human-facing rocm services stop and the attached-serve Ctrl-C path were changed to a non-zero exit. That's the right place to draw that line: it fixes the problem for a human or an exit-code-checking script without breaking the documented JSON-consumer contract.
• The chat/assistant stop_server tool call (apps/rocm/src/chat.rs) maps straight onto rocm services stop <id> --yes as a subprocess rather than re-implementing its own success/failure interpretation, so it inherits this fix for free with no separate change needed.
• The ROCM_E2E_STOP_OUTCOME feature-gated test seam gives genuine black-box e2e coverage (service-stop-03/-04/-05) of outcomes that cannot otherwise be staged from outside the binary (a process surviving SIGKILL, an unreadable /proc entry), rather than leaving that half unit-tested only.

Deployment notes

• rocm services stop (and Ctrl-C during an attached rocm serve) now exits non-zero when the stop is not confirmed, where it previously always exited 0. Any script or automation that checks only the exit code and not the printed text will now see a failure here — intended per the PR, which names #299's uninstall flow as a possible example, but worth flagging to anyone rolling this out.
• The stop_server sandbox-tool/MCP JSON answer's status field now takes stopped or stop_unconfirmed instead of always stopped. A consumer keying success off status == "stopped" rather than result.stopped should confirm it handles the new value on the rocm CLI side, as it should already on the rocmd daemon side since #499.

What this covered

Base 6ab12083e7dd7901eb54e6a4a5b0fd362f4d8eb8…head df8fc9920e556160aa5262dd34c67bbf74f6a564 (PR #799's current head, fetched read-only into a local pr-799 ref and never checked out).

The change was split by file-coupling into five groups and fanned out as five independent background workers. Only one — apps/rocmd/src/{mcp,service,test_support}.rs — returned within the available time; it found no findings scoring ≥70, having verified the diff there is comment/string-only, checked the new wording against the unchanged logic it describes, and compared rocmd's stop contract against rocm's for semantic drift (none found).

The other four groups' workers (plus further sub-workers some of them had spawned) did not return in time and were stopped before completion. Per this method's contingency for a worker that does not return, those four passes were instead run directly by this reviewing context, which is not an independent pass the way the fan-out is meant to produce, and should be weighed accordingly:

• apps/rocm/src/services_text.rs (the core verdict/rendering logic, ~2838 lines, ~914 diff lines) — read in full, including every new/changed test.
• apps/rocm/src/main.rs (targeted: the touched regions, the ServicesCommand::Stop enum/help text, run_approved_service_action, the new finish_service_action, and every pre-existing test referencing them — not the file's ~9500 unrelated lines cover-to-cover) and apps/rocm/src/service_launch.rs (read in full, including every new/changed test).
• tests/e2e-cucumber/{features/service_stop.feature, tests/e2e/service_stop_steps.rs, tests/e2e.rs} — read in full.
• README.md, docs/manual-testing.md, docs/testing.md — read in full and cross-checked against the rendering code and tests they describe.

Design review and beyond-diff impact analysis were also done directly by this context: traced that rocm sandbox-run/rocmd sandbox-tool's JSON-exit-0 contract is deliberately preserved, that the chat/assistant tool call delegates to the same CLI path rather than re-implementing it, and that the dash-TUI/automation-proposal consumers are explicitly named as out of scope in the PR's own "Scope" section rather than a silent gap.

CI at the reviewed head: all required checks green, including build-and-test, Test (affected crates), clippy, E2E tests, E2E consolidated report, windows-build-and-test, Commit signatures + sign-off. Two GPU-hardware lanes (MI300X, W7900) were still queued and two others (Strix Halo, Ubuntu, release/nightly) were cancelled at review time — neither failed, but their result for this exact head could not be confirmed.

Not reconciled against any PR discussion: this PR has zero review comments and zero prior reviews as of the head reviewed. Did not independently run cargo test/cargo build --features e2e-test-hooks here — relied on the green CI run plus direct code tracing instead.

[juhovainio]

I found no blocking issues. The root cause is fixed at the shared layer: stop_server now takes its status from the stop's own stopped verdict, and both rocm services stop and the attached Ctrl-C path fail on stop_unconfirmed. The printed lines come from the same fields the stop wrote to disk, and the unit tests plus scenarios service-stop-03/04/05 assert the message together with the resulting state. README, --help, docs/testing.md and docs/manual-testing.md agree with the code.

Non-blocking:

1. stop_failure_summary in the e2e harness has no test of its own (inline).
2. In finish_attached_stop, a stderr write failure returns before the audit event is recorded (inline).
3. An unconfirmed stop now exits 1 instead of 0. Worth confirming that other callers that treat exit 0 as stopped (the #299 uninstall flow, any other run_internal_sandbox_tool caller) handle stop_unconfirmed.

This is a read of the diff plus the CI results at review time; W7900 E2E was still pending, and the Strix Halo Ubuntu nightly/release jobs were cancelled, which looks like runner contention.

[nowycondro]

🤖 Automated review (Claude Code) · 6d48a44

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — no blocking findings

DEGRADED REVIEW (single context; see What this covered).

Full review of the whole change. No ticket claimed closed by this change; it cites #499 (rocmd's earlier stop-identity-verification fix), #708 (a container-engine stop failure) and #299 (the uninstall flow) as background/related context only, which I independently confirmed via git log (#499, #708 are real prior commits in this area) rather than taking the citations at face value.

Blocking

None

Non-blocking

• Stale comment overstates what pid_outcomes contains — apps/rocmd/src/mcp.rs:803
  Says pid_outcomes in the structured result "names the PID that could not be confirmed," but pid_outcomes (built in apps/rocmd/src/service.rs, mapping every entry in stops unfiltered) is a list of every recorded PID with its role and outcome, not only the unconfirmed one. This file is byte-identical to the commit reviewed earlier in this PR's history, and I independently re-derived the same defect by reading service.rs's construction of pid_outcomes against the claim.
  Confidence 85 · mechanical · Fix: reword to "pid_outcomes names every recorded PID's outcome, including which one(s) could not be confirmed."

Decisions for the author

• finish_attached_stop's "stop failed, and the direct signal also failed" branch still exits 0 — tradeoff
  In apps/rocm/src/service_launch.rs, two of the three branches of the attached rocm serve Ctrl-C stop now correctly exit non-zero on an unconfirmed/uninformative outcome. The third — the managed stop call itself errored, and the direct fallback signal to the child also failed — still returns Ok(()) (exit 0). This is explicitly intentional (the function's own doc comment says "exits 0, as it always has here", and attached_ctrl_c_signals_its_own_child_when_the_stop_itself_fails asserts exactly this), so flagging only so the asymmetry is a confirmed choice.

Positive signals

• The reorder to "audit event recorded, then write to stdout/stderr" in finish_attached_stop's confirmed and error branches is a genuine, well-targeted fix (landed since this PR's previous review round) for a real prior finding, proven by a dedicated BrokenWriter test that would fail if the ordering reverted.
• render_unconfirmed_service_stop's per-PID reasoning is exhaustive against TerminationOutcome::stopped()'s own definition in crates/rocm-core/src/proc_lifecycle.rs (only TimedOut/Unverified make a PID not-stopped, and the renderer handles exactly those two) — traced directly, not assumed.
• The JSON-exit-0 contract for rocm sandbox-run stop_server/rocmd sandbox-tool stop_server is deliberately preserved (InternalSandboxRun's handler in main.rs returns Ok(()) unconditionally) while only the human-facing paths changed to bail; and apps/rocm/src/chat.rs's stop_server tool call maps onto the same CLI subprocess and already treats a non-zero exit as isError, so it inherits the fix for free.

Deployment notes

• rocm services stop (and Ctrl-C during an attached rocm serve) now exits non-zero when the stop is not confirmed; a script checking only the exit code will now see a failure, as intended (the PR names #299's uninstall flow as a possible example).
• The stop_server sandbox-tool/MCP JSON answer's status field now takes stopped or stop_unconfirmed instead of always stopped.
• No CI has yet run (beyond a docs/readthedocs check) against the exact commit being reviewed (6d48a44): get_check_runs returns zero check runs, and every registered workflow run (CI, CodeQL, E2E self-hosted) is tied to the earlier commit df8fc99, which predates the two production-relevant commits made since (the audit-before-write reorder in service_launch.rs, and the tests/e2e-cucumber harness changes). Confirm build-and-test, clippy, E2E tests and windows-build-and-test actually complete on this head before relying on the PR's stated verification matrix for it.

What this covered

DEGRADED REVIEW: I attempted to fan out five file-coupled review groups as independent background workers (services_text.rs+main.rs; service_launch.rs; apps/rocmd/{mcp,service,test_support}.rs; tests/e2e-cucumber/; README.md/docs/.md). Every spawn attempt failed with "Concurrent subagent limit reached" — an environment-wide ceiling already consumed by ~20+ other subagents outside this review's control. Per the method's contingency, I ran every pass directly in this single context instead; this is not an independent fan-out and should be weighed accordingly.

I read, in full: apps/rocm/src/services_text.rs, apps/rocm/src/service_launch.rs (including every new/changed test), apps/rocmd/src/mcp.rs, apps/rocmd/src/service.rs, apps/rocmd/src/test_support.rs, tests/e2e-cucumber/features/service_stop.feature, tests/e2e-cucumber/tests/e2e/service_stop_steps.rs, tests/e2e-cucumber/src/lib.rs, the changed sections of tests/e2e-cucumber/tests/e2e.rs, and the changed sections of README.md/docs/manual-testing.md/docs/testing.md in full context. In apps/rocm/src/main.rs (~11800 lines) I read only the touched regions and immediate callers (ServicesCommand::Stop's doc/help text, run_approved_service_action, finish_service_action, InternalSandboxRun's dispatch), not the file cover-to-cover.

I traced the full JSON-wrapper contract (output/result nesting) end to end across run_internal_sandbox_tool, service_stop_unconfirmed, render_service_action_result, and both call sites; TerminationOutcome's definition against the renderer; chat.rs's subprocess exit-status handling; and confirmed the delta between this PR's previously-reviewed commit (df8fc99) and the current head touches apps/rocm/src/service_launch.rs, tests/e2e-cucumber/src/lib.rs and tests/e2e-cucumber/tests/e2e.rs, reading that delta under the same scrutiny as everything else (it turned out to be a targeted fix for a previously-raised finding, not new risk). Checked CI directly via the GitHub API rather than assuming it.

Did not run: cargo build/cargo test/clippy myself (no Rust toolchain available in this environment) — relied on direct code tracing instead. History pass: spot-checked via git log on the two core files; nothing beyond what's already cited surfaced. Prior-changes pass (review comments on earlier, unrelated changes to these files) did not run — out of budget for a pass with low expected yield. Agent-instruction adherence: checked against AGENTS.md §3, §5 and §6 as directly relevant to this change; not exhaustively re-derived against every line of the file.

Not reconciled against any PR discussion beyond what Step 9 requires: did not read or factor in the review threads/comments on this PR, including the two prior reviews at the old head (df8fc99). The one overlapping non-blocking finding and the one overlapping tradeoff above were independently re-derived from the code, not copied forward.

Base 6ab12083e7dd7901eb54e6a4a5b0fd362f4d8eb8…head 6d48a443f6ace0afe509ef3c58c45c44fd6b4da5.

---

🤖 Generated with Claude Code

[nowycondro]

🤖 Automated review (Claude Code) · d4a8ad5

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — needs work

Full review of the whole change (base e6d2f7c0…head d4a8ad59). No ticket claimed closed by this change; it references #499 (rocmd's earlier stop-identity-verification fix), #708 (a container-engine stop failure) and #299 (the uninstall flow) as background/related context only, not as issues this PR resolves.

Blocking

• CI check E2E tests (Strix Halo, Windows, current) fails at this head — workflow run
  Reconciliation: 3 xfail (failed as expected), 0 XPASS (0 flaky, 0 stale), 1 unexpected failure(s). FAIL: 'runtime-resolve-canonical-release' was expected to pass on this host but FAILED — a regression. That scenario is unrelated to any file this PR touches (it's a ROCm-runtime-resolution scenario, not a service-stop one), so this looks like a pre-existing flake or an unrelated regression on that specific GPU hardware lane rather than something this diff caused — but a failing check is still a blocking finding until resolved, per this repo's own CI-ownership rule (AGENTS.md §12: "a red check is not automatically your code, but it is your responsibility to resolve... it still must end green"). Three other GPU-hardware lanes (W7900, MI300X, Strix Halo Ubuntu release) are still queued at this head and their result could not be confirmed either.
  Confidence 100 · mechanical · Fix: confirm whether runtime-resolve-canonical-release also fails on main independent of this branch (if so, re-run or escalate as a known flake per AGENTS.md §12); if it's specific to this branch, investigate before merging. Re-run the three still-queued GPU lanes once they clear.

• The AttachOutcome::Detach arm's audit-before-print reorder has no test, unlike the identical fix on the Stop arm — apps/rocm/src/service_launch.rs:1097-1112
  This PR reorders record_cli_audit_event ahead of println! in the Detach arm with the comment "Audit before printing: a closed stdout makes println! panic, which must not lose the event" — the same class of bug the PR's second commit fixes for finish_attached_stop's Stop arm (and which that arm now has a dedicated BrokenWriter test for). The Detach arm's println! calls are inline in run_attached_service rather than extracted into an injectable-writer helper, and no test (unit or e2e) exercises this audit-vs-panic ordering for Detach — grepping the whole test suite for "detach" turns up nothing touching this. AGENTS.md §3 requires a regression test for every fix; the exception for impractical-to-test behaviour doesn't apply here since the sibling Stop arm already demonstrates the pattern is practical in this very same file.
  Confidence 78 · mechanical · Fix: factor the Detach arm into a small injectable-writer helper mirroring finish_attached_stop, and add a BrokenWriter-style test proving the audit event survives a closed stdout.

Non-blocking

• Stale comment/error text overstates what pid_outcomes contains — apps/rocmd/src/mcp.rs:798-804
  The (unchanged-by-this-diff, but directly adjacent to wording this PR did edit in the same comment block and user-facing string) text says pid_outcomes "names the PID that could not be confirmed" — singular, implying it is filtered to just the problem PID. In fact pid_outcomes (apps/rocmd/src/service.rs:93-102, confirmed against apps/rocm/src/services_text.rs's equivalent construction too) lists every recorded PID with its own role and outcome — typically two entries (supervisor + engine) unless they alias to the same PID. A caller has to scan every entry, not read off a single named one.
  Confidence 88 · mechanical · Fix: reword to "pid_outcomes names each recorded PID and its own outcome, including which one(s) could not be confirmed."

• Two new tests in services_text.rs don't assert the per-PID line their own name/comment says they verify — apps/rocm/src/services_text.rs (services_stop_notes_a_changed_record_even_when_a_process_timed_out, services_stop_gives_a_mixed_result_a_line_each_and_a_check)
  The first test's comment says the report must show the record-changed note "beside A's own [timed-out] line," but its assertions only check the record-changed note and the no-retry invariant — never the "still running: process 999999999" line. The second test's name promises "a line each," but only asserts the unverified PID's check: line, never the timed-out PID's "still running" line. A regression dropping the timed_out loop in render_unconfirmed_service_stop in just these combined scenarios would pass both tests silently (the underlying per-PID behaviour is separately covered by other, single-outcome tests, so this is a completeness gap in these two tests rather than a true absence of coverage).
  Confidence 78 · mechanical · Fix: add the missing per-PID line assertion to each test so it proves what its own name/comment claims.

Decisions for the author

• finish_attached_stop's "stop itself errored, and the direct signal also failed" branch still exits 0 — tradeoff
  Two of the three branches of the attached rocm serve Ctrl-C stop now correctly exit non-zero on an unconfirmed/uninformative outcome. The third — the managed stop RPC itself erroring, and the fallback direct signal to the child also failing — still returns Ok(()) (exit 0), unchanged from before this PR; the function's own doc comment states this is deliberate ("exits 0, as it always has here"). Flagging only so the asymmetry — nothing was confirmed stopped and the direct kill failed too, yet this is now the one path in the three-way match that still reports success — is a confirmed choice rather than an oversight.
• Two distinct scenarios asserted inside one #[test] fn — non-blocking-improvement
  attached_ctrl_c_signals_its_own_child_when_the_stop_itself_fails (service_launch.rs) tests both "direct signal succeeds" and "direct signal also fails" in one function body. Both still discriminate correctly under mutation, but splitting into two functions would give cleaner failure attribution.
• services_text.rs's module-doc function inventory doesn't list the new helpers — non-blocking-improvement
  The file's own "owns: X, Y, Z" doc-comment convention is otherwise exhaustive but doesn't name stop_report_confirmed, internal_stop_server_value, service_stop_unconfirmed, render_unconfirmed_service_stop, process_check_command, RecordedStopTermination/RecordedPidOutcome, or scripted_stop_outcome. Worth a follow-up touch for consistency with the file's own convention.

Positive signals

• finish_attached_stop's signal_child/out/err injection makes logic previously only reachable via a live attached-serve session fully unit-testable — a good instance of the "pure, injectable helper" pattern AGENTS.md §3 asks for with IO-gated code.
• The new JSON contract (pid_outcomes, record_changed, endpoint_key_kept, engine_stop_unconfirmed, stopped) was traced field-by-field, pointer-by-pointer, from every producer (stop_internal_managed_service_confirmed_with) to every consumer (render_unconfirmed_service_stop, stop_report_confirmed, service_stop_unconfirmed, both CLI and attached-Ctrl-C call sites) with zero drift found anywhere — a meaningfully large surface for a change this size to get exactly right.
• assert_no_retry_line is consistently reused across every unconfirmed-stop test (unit and e2e), systematically guarding the PR's core "never suggest retrying the stop" invariant.
• apps/rocm/src/chat.rs's stop_server tool call maps onto rocm services stop <id> --yes as a subprocess rather than reimplementing success/failure interpretation, so it inherits this fix for free.
• docs/testing.md's distinction between output.result.status (the record's own persisted status, which can read stopped via a liveness refresh even alongside stopped: false) and output.status/result.stopped (this stop's own verdict) is unusually precise about a real, easy-to-conflate subtlety in the code.
• The three new service-stop-03/-04/-05 e2e scenarios give genuine black-box coverage of outcomes (a process surviving SIGKILL, an unreadable /proc entry) that cannot otherwise be staged from outside the binary, via a #[cfg(feature = "e2e-test-hooks")]-gated seam that is cleanly scoped (consumed once per scenario, compiled out of production builds).

Deployment notes

• rocm services stop (and Ctrl-C during an attached rocm serve) now exits non-zero when the stop is not confirmed, where it previously always exited 0. Any script or automation checking only the exit code will now see a failure here — intended, and the PR names #299's uninstall flow as a possible example to check.
• The stop_server sandbox-tool/MCP JSON answer's status field now takes stopped or stop_unconfirmed instead of always stopped, but — deliberately, to preserve the existing JSON-consumer contract — still exits 0 either way. A consumer keying success off status == "stopped" rather than result.stopped should confirm it handles the new value; rocmd's own MCP tool already does (since #499), and this review traced chat.rs's delegation and found it unaffected, but the PR's own "Scope" section already names the dash-TUI's envelope (which collapses this distinction) and rocm remote stop's relay of a remote's report as known, explicitly deferred follow-ups, not silent gaps.
• Three GPU-hardware E2E lanes (W7900, MI300X, Strix Halo Ubuntu release) remain queued and unresolved as of the reviewed head.

What this covered

Full fan-out review of e6d2f7c0d7b9a8051a475dfaca84756e5b97aecf…d4a8ad5903c023927bdcc6c37a0ba46ae6ee6058 (confirmed via git merge-base against origin/main; the PR's own authored content is exactly its two commits — 48ed367d and d4a8ad59 — the PR branch was rebased onto a much-advanced main since the last prior review, which is why that diffstat looks large, but the PR's own commits are unchanged in content). Fetched read-only (refs/remotes/pr/799-fresh) and never checked out.

Split by file-coupling into five groups, each read completely at the PR head plus necessary surrounding context, and fanned out as independent background workers: (1) apps/rocm/src/service_launch.rs's attached-Ctrl-C stop path, (2) apps/rocm/src/services_text.rs + services_cmd.rs + main.rs's core verdict/rendering logic, (3) apps/rocmd/src/{mcp,service,test_support}.rs, (4) tests/e2e-cucumber/*, (5) README.md/docs/manual-testing.md/docs/testing.md. All five groups' first spawn attempt hit this environment's "concurrent subagent limit reached" (four of five failed immediately); per this method's contingency for a worker that doesn't return, all four were re-spawned once and all four succeeded on retry — no group was reviewed non-independently or by this context directly.

Design review and beyond-diff impact analysis: traced that rocm sandbox-run/rocmd sandbox-tool's JSON-exit-0 contract is deliberately preserved; that chat.rs's tool call delegates to the CLI subprocess rather than reimplementing it; that rocmd's pre-existing (#499) contract and comments were consistently updated to stop claiming a retry "finishes the job"; and that the dash-TUI/rocm remote stop consumers are explicitly named as deferred follow-ups in the PR's own "Scope" section rather than a silent gap. CI was checked live via the GitHub API both before and immediately after the fan-out (same result both times, confirming no drift mid-review); see Blocking above for the one failing check found.

Did not run as separate dedicated passes, for budget reasons: a standalone history pass beyond spot-checking the #499/#708 citations against git log (confirmed real), and a standalone prior-review-comments pass. Agent-instruction adherence was checked against the specific AGENTS.md sections each file-group's rubric named (§3, §5, §6), not re-derived exhaustively line-by-line against the whole file. Did not independently run cargo test/cargo build --features e2e-test-hooks (no Rust toolchain in this environment) — relied on direct code tracing plus the live CI result instead.

Claim-check and disclosure: this PR already carries two automated reviews from this same account, both pinned to older, pre-rebase commits (df8fc992, 6d48a443), plus a human APPROVED review from juhovainio and two empty COMMENTED stubs from the author — none pinned to the current head, so none counted as already covering it per this method's narrowing rules, and this review was conducted fresh. Fetching those prior reviews during the mandated claim-check step necessarily exposed their full text to this reviewing context before the fan-out began; the actual code-reading, judgment, and findings above were produced by fresh workers who were never shown that text and who reasoned from the code independently (confirmed by their traced evidence citations) — where a finding here overlaps with a prior review (the pid_outcomes comment, the Decisions for the author tradeoff), it is because independent reasoning reached the same place, not because it was copied forward. This exposure could not be avoided given the task's own mandated claim-check step, and is disclosed here per this method's honesty requirements. Not otherwise reconciled against any PR discussion.

The PR description was read last, after all findings above were settled. Its "Scope" section's three disclosed follow-ups and its verification/testing claims match what this review independently found; no mismatch between the description and the actual diff.

---

🤖 Generated with Claude Code