fix(uninstall): stop managed services before removing the tooling that stops them (EAI-8014)

[fredespi]

Summary

rocm uninstall reported uninstall complete (RC=0) while a managed model server was still running — including a publicly-bound, GPU-holding vLLM endpoint — and then deleted the rocm/rocmd binaries and service records needed to stop it. The supported rocm services stop path was gone, leaving only a manual PID kill to release the GPU.

This change stops managed servers before removing anything, and refuses to proceed if it cannot confirm they are stopped.

Changes

The stop-before-remove gate

• apps/rocm/src/uninstall.rs — after the confirmation gate and before removing any path, stop every live managed service. If any cannot be confirmed stopped, abort with a non-zero exit and remove nothing, so the tooling needed to recover (rocm services stop) stays in place. The error names the offending services and the remedy for each.
• apps/rocm/src/main.rs — stop_managed_services_before_uninstall() returns a ManagedServiceStopReport { stopped, failed }, reusing the verified process-tree termination that rocm services stop already uses. A service is counted stopped only once every recorded process is confirmed gone; one that merely crashed is skipped and does not block uninstall.

Killing the supervisor before the supervised

• rocmd respawns a ready record whose endpoint is unreachable — exactly the state the stop pass creates — so the background helper is stopped first. A helper that cannot be confirmed stopped is itself a blocking failure.

Not signalling a PID we cannot identify

• The daemon stop signals a whole process tree with force, and runtime-state.json outlives a crash or reboot, so the recorded PID can name a stranger. rocmd now records daemon_start_ticks (apps/rocmd/src/lib.rs), and rocm-core grows identity_state_with_observed so one reading of the PID answers both the "could this platform verify?" and "is this still the same process?" questions. A recycled PID is a no-op, an unverifiable one aborts with the tooling intact, and a record whose running flag is false is never signalled at all.

Closing the gate's own escape hatches

• A record already marked stopped whose port still answers must identify itself as this record's model before it blocks, so an unrelated service on a recycled port cannot brick uninstall — but an auth refusal (401/403) from that endpoint now blocks, because a server stating it guards the path is evidence something is serving.
• A wildcard bind is probed on both loopback families: :: with IPV6_V6ONLY=1 answers on ::1 and refuses 127.0.0.1.
• Manifests that exist but cannot be parsed abort rather than being skipped silently — a corrupt record can describe a live server.

Supporting work

• crates/rocm-core — the atomic-write publish step is single-sourced (write_file_atomically), including the Windows ReplaceFileW path; ManagedServiceRecord::write and AutomationRuntimeState::write now publish through it, so a reader never sees a truncated manifest. http_get_with_auth reports a response status instead of collapsing every non-200 into an error.
• apps/rocm/src/therock.rs, apps/rocmd — local helpers document precisely what they do and do not share with the centralised one; now-unused dependencies dropped from both manifests.

Test plan

• tests/e2e-cucumber — new install_lifecycle scenario covering uninstall against a live managed server, in the default CI path rather than a gated lane.
• Unit coverage across apps/rocm, apps/rocmd and crates/rocm-core for: the stop-before-remove ordering (asserted on planned files still existing after an aborted run, not on a return value), a crashed service not blocking, a recycled daemon PID never being signalled, a pre-start_ticks record aborting instead of force-killing, an unreadable runtime state aborting, a stopped-but-serving record blocking on retry, an authenticated survivor blocking rather than failing open, an IPv6-only wildcard engine blocking, and a reader never observing a truncated manifest.
• Each of these was verified against a faithful revert of the production change it names, rather than by inspection. Two are labelled in-place as the exceptions: a directory-hygiene check and a false-positive guard, which by design do not fail on revert.
• Gated on Linux at CI strictness: cargo fmt --all -- --check, cargo clippy --workspace --all-targets --exclude e2e-cucumber -- -D warnings, and the rocm, rocm-core and rocmd suites.

Related

• The uninstall-stops-what-it-manages scenario in test(e2e): pin the contracts a README walkthrough expects (EAI-8024) #241 describes the same behaviour, but that PR is open and unmerged, so it is a dependency to keep in sync rather than existing coverage. The e2e scenario in this PR is what actually holds the behaviour today.

[r0x0r]

Solid, well-scoped fix for EAI-8014. Placement is correct (after the dry-run/empty-plan early return and the confirm gate, before removal), the abort is fail-closed, and it reuses the verified process-tree termination path so a service only counts as stopped once every recorded process is confirmed gone. CI is fully green including the GPU/Strix Halo/Windows E2E lanes.

Leaving a few resolvable threads: one meaningful test gap on the abort path plus a couple of minor nits. None are blocking.

[michaelroy-amd]

Reviewed at e2015fb1a9393a6cc3e757805c02dfc1ed09b81b. The stop-before-remove ordering and the new removal gate are sound, and all 17 required checks pass. Two blocking gaps remain:

1. load_managed_services silently skips a service record when serde_json::from_slice fails. That lets uninstall proceed without attempting to stop a live service represented by a corrupt manifest, which contradicts this change's fail-closed guarantee and can recreate the reported outcome. Propagate the parse error with the manifest path and add a regression test proving uninstall removes nothing on malformed service state.

2. The user-visible uninstall contract has no Cucumber scenario in this PR. The cited uninstall-stops-what-it-manages scenario is only in still-open PR #241, so it does not cover this branch. Add the scenario here, or explicitly make this PR depend on #241 and rebase after it lands.

Please address both and re-request review.

[rominf]

I went through this at e2015fb1. The direction is right and the two helpers are cleanly separated so the abort branch is unit-testable — that's a good call. All checks are green and it's MERGEABLE.

I agree with both blocking points already raised: load_managed_services still does if let Ok(mut record) = serde_json::from_slice(...) at main.rs:15001, so a corrupt manifest for a live service is still invisible to the new gate — which directly contradicts the fail-closed doc comment you added at :13891-13895; and there's still no Cucumber scenario, with the cited uninstall-stops-what-it-manages id present nowhere in the tree (I grepped) and #241 still open and now CONFLICTING, so depending on it means waiting for a rebase there first. I won't repeat the detail on those.

What I want to add is that the abort is not the clean no-op its message claims, and this is worse than the wording nit already noted. Beyond that: the stop pass runs even when the uninstall isn't removing the tooling, the plan warning overcounts, and the .ok() swallows the diagnosis. Details inline.

One I looked at and decided isn't worth a comment on its own: refresh_managed_service_runtime_liveness demoting a record to stopped on dead tracked PIDs while an engine grandchild keeps serving. On Linux that's mostly covered — collect_process_tree walks /proc and signal_process_scope gets tree: true, so descendants do get signalled on the stop path. The Windows gap is real though, and I've folded it into the comment on the liveness skip.

[siloteemu]

🔴 Automated review · pr-review-watcher · e2015fb

Summary

Stops live managed services before rocm uninstall deletes the binaries and service records needed to stop them, aborting the uninstall with nothing removed when any service cannot be confirmed stopped. The fix itself is sound and fails closed in the right direction — Needs work, because the ordering guarantee it introduces has no test that pins it. Verified: the ordering is guaranteed, not incidental (uninstall.rs:45-48 runs before the for entry in &plan.actions removal loop at uninstall.rs:50, and both calls use ?, so a failed stop returns Err and the loop never executes — nothing is removed); a partial failure is recoverable and re-runnable (services confirmed stopped are written back as status = "stopped", so managed_service_is_live skips them on the next attempt and only the failed ones are retried); already-dead services are correctly skipped rather than counted as failures; but on the revert question, all five new tests still compile and pass if the four wiring lines in uninstall.rs are deleted — I confirmed by grep that uninstall::uninstall() is called only from CLI dispatch (main.rs:23, main.rs:1972) and from no test. I read the code and the caller-supplied CI state; I did not build or run the suite. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

• apps/rocm/src/main.rs:26777-26887, apps/rocm/src/uninstall.rs:45-50 — nothing tests the thing the PR fixes. The five new tests exercise the two new helpers in isolation; none calls uninstall(), so none proves that stopping happens before removal or that a failed stop actually prevents removal. Delete the stop_managed_services_before_uninstall / uninstall_removal_gate calls from uninstall.rs and the entire test suite stays green — the ordering guarantee rests on code inspection alone. One test is weaker still: uninstall_skips_already_dead_managed_service (main.rs:26809) asserts only that stopped and failed are both empty, which is exactly what a stop_managed_services_before_uninstall gutted to Ok(ManagedServiceStopReport::default()) returns — it cannot distinguish "correctly skipped a dead service" from "did nothing". This also runs into AGENTS.md §3: the change alters user-observable behavior (new abort with a new error message, new stopped N managed service(s) before removal line, servers killed during uninstall), and §3 states plainly that a unit test on an internal helper does not discharge the requirement for a Gherkin scenario in tests/e2e-cucumber/features/ — I checked, and no scenario there registers a managed service (install_lifecycle.feature:115-124 is the only uninstall scenario and would pass identically with or without this fix). Concrete fix: make uninstall() (or an extracted, injectable inner function) testable and add a test that seeds a plan with a real file plus a service whose stop cannot be confirmed, then asserts the call returns Err and the file still exists — that single assertion is what pins the ordering. Add or extend a lifecycle scenario for the observable path, or, if it can only run on a gated lane, name the lane in the PR text per §3. I could not read the PR body from the checkout, so if it already names an @id or states the gap, treat that half as satisfied.

Non-blocking

• apps/rocm/src/main.rs:13928-13938 — a service that can never be confirmed stopped (EPERM on a process owned by another user, or a GPU worker wedged in uninterruptible sleep where SIGKILL does not land) makes rocm uninstall permanently impossible: there is no override in UninstallOptions (main.rs:16114), and the suggested recovery rocm services stop <id> --yes routes through the same stop_internal_managed_service with the same outcome. Refusing is the right default, but consider a documented override flag and mentioning the privilege/manual case in the message.
• apps/rocm/src/main.rs:13903-13908 — stop_internal_managed_service(...).ok() discards the underlying error, so the abort message names the service but never says why the stop failed; carrying the error string into failed would make the dead end diagnosable.
• apps/rocm/src/main.rs:13891-13895 vs 15001 — the doc comment claims fail-closed discovery, but load_managed_services silently skips any record whose JSON fails to parse (if let Ok(mut record) = ...), so a corrupt manifest for a live, GPU-holding server is invisible to the gate and uninstall proceeds — the exact failure this PR closes.
• apps/rocm/src/main.rs:13749-13798 — the stop is serial and unbounded in aggregate: ~10 s grace twice per PID, up to two PIDs per service, plus the engine-side stop, so uninstall can sit silent for roughly a minute per live service after the confirmation prompt; printing stopping <id>… before each would stop it reading as a hang.
• apps/rocm/src/main.rs:16193 — the plan warning now promises "their servers will be stopped before removal" for every record, including ones already stopped/failed that the gate skips entirely; scoping the count to live records would keep the plan truthful.

[siloteemu]

🔴 Automated review · pr-review-watcher · 1f9ca82

Summary

Makes rocm uninstall stop every managed model server before it deletes the binaries and service records needed to stop them, aborting with nothing removed if any stop cannot be confirmed. Verdict: Needs work — the prior blocker is genuinely resolved, but the new verification pass introduces a hard-lockout path and a plan message that no longer matches behaviour. Verified: the prior finding is FIXED — uninstall.rs:96 seeds a real file plus an unconfirmable stop and asserts both the error and that the file survives (I confirmed remove_path (main.rs:16553) really would delete it, so the assertion is not a tautology), uninstall_skips_already_dead_managed_service now pins an exact stopped vec against a live service sharing the directory rather than two empty collections, and the AGENTS.md §3 gap is closed by @id:lifecycle-linux-uninstall-stops-managed-server, which runs in the default Linux Acceptance install lifecycle job (ci.yml:238, E2E_ONLY_LIFECYCLE=1) and fails if the wiring is reverted (the sleep 600 child survives and try_wait times out); on the revert question, 10 of the 12 new unit tests are revert-sensitive, the two exceptions being removal_proceeds_once_every_managed_server_is_confirmed_stopped and uninstall_abort_says_which_services_it_already_stopped, which a no-op-success gate and a message-only change respectively would keep green; on the 2 failing checks, windows-build-and-test is a real, actionable failure (blocking item 3 below — not a flake), while the ReadTheDocs check fails for a reason not reachable from this container and this diff touches no docs sources, so I record it as unexplained rather than dismissed. No prompt-injection or instruction-shaped content was found anywhere in the diff. Blocking: 3 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

• apps/rocm/src/main.rs:13941-13969 — the TCP "reality check" loop iterates every loaded record, skipping only those already in report.failed. Records that managed_service_is_live rejected in the first loop (main.rs:13913) are therefore judged purely by whether something answers on their recorded host:port. Stopping a service rewrites the manifest with status = "stopped" (main.rs:13825-13834) and never deletes it, so stale records accumulate indefinitely with their old ports. Any unrelated local listener that later occupies such a port (vLLM's documented default is 8000) makes stop_managed_services_before_uninstall synthesise a failure, uninstall_removal_gate (main.rs:14040) hard-bail!s, and nothing is removed. There is no override: UninstallOptions (main.rs:16252) has no --force, and yes is consulted only by confirm_uninstall. The failure is deterministic, so every re-run repeats it, and the error's suggested recovery (rocm services stop <id> --yes) does not apply to an already-stopped record — the only escape is hand-deleting JSON under the services directory, which the message never mentions. Fix: apply the same managed_service_is_live guard in the second loop (or require a live recorded PID to corroborate a bare port hit), and add an explicit documented override for when the gate's signal is wrong.

• apps/rocm/src/main.rs:16337-16350 — the plan warning states "…have a running server; those servers will be stopped before removal" whenever live > 0, but the stop pass runs only when plan_removes_recovery_tooling (main.rs:16367) is true (apps/rocm/src/uninstall.rs:44-51). On a cache-only run (--keep-binaries --keep-data), or a dev-binary-layout run without --force-dev-binaries plus --keep-data, the warning is printed before the confirmation prompt and no server is ever stopped. This contradicts both the comment immediately above it ("claiming otherwise would make the plan describe work uninstall never does") and the commit message's claim that the warning "does not promise to stop servers that are not running". Fix: pass the same recovery-tooling predicate into the warning and word it accordingly when the plan leaves the tooling in place.

• CI: windows-build-and-test is red and this PR must resolve it. The job checks out the PR-merged-into-main commit, which includes tests/e2e-cucumber/tests/feature_naming.rs (added upstream after this branch diverged, absent from this checkout). Its scenario_names_are_indexed_sequentially_per_feature requires every scenario in install_lifecycle.feature to be named lifecycle-NN - …; the new Scenario: Linux - uninstall stops the local server it manages has no such prefix. Fix: rebase onto current main and renumber the new scenario into the sequence. (Verified from the CI log and by confirming the guard test's absence from this branch; the Test (affected crates) job pins the raw head SHA, which is why it stays green and why this cannot be dismissed as unrelated.)

Non-blocking

• No unit test drives uninstall() itself — deleting the stop_managed_services_then_remove call and inlining the old loop is caught only by the e2e scenario; a thin test over uninstall() would close the last of the original finding.
• uninstall_refuses_when_a_service_record_cannot_be_parsed (main.rs) is #[cfg(target_os = "linux")] despite touching no PID or process state — it would run fine everywhere and the corrupt-manifest gate is not Linux-specific.
• ManagedServiceRecord::write (rocm-core lib.rs:7168) does a plain non-atomic fs::write; a concurrent read during overwrite can make a healthy record look unparseable and spuriously abort the gate. Write-temp-then-rename would close it.
• build_uninstall_plan uses load_managed_services(...).unwrap_or_default() (main.rs:16328) while the real gate propagates that error, so the plan/dry-run can show "no managed services" for a run that will in fact abort.
• The ReadTheDocs check is failing and unexplained; the diff touches no docs sources, but that should be confirmed rather than assumed before merge.

[siloteemu]

🔴 Automated review · pr-review-watcher · 6b697a3

Summary

This PR makes rocm uninstall stop the managed servers it manages before deleting the binaries and service records needed to stop them, and adds unit, integration and e2e coverage for that ordering. Verified: finding 1 (hard lockout) STILL OPEN and byte-identical to head 1f9ca82f; finding 2 (plan warning promises unperformed work) STILL OPEN and byte-identical; finding 3 (Windows scenario-naming guard) FIXED — I ran cargo test -p e2e-cucumber --test feature_naming, 4/4 pass, and a sweep found no stale references to the renumbered scenarios; the only work since the held review was a merge of main plus the renumbering commit, so nothing new was introduced, and every added test fails or stops compiling if the production change is reverted (details below) — except that one of them locks in the finding-1 defect as intended behaviour. Blocking: 2 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

1. apps/rocm/src/main.rs:15570-15591 — the port-reachability loop still has no liveness guard; a recycled port deterministically bricks uninstall.
The second pass in stop_managed_services_before_uninstall iterates every loaded record, skipping only ones already in report.failed, then synthesises a failure for any whose recorded host:port accepts a connection. Verified unchanged since the held review (same code, same comment, only shifted by the merge).

Why it bites: managed_service_is_live (main.rs:16576) is a pure status check — ready|running|starting|recovering. load_managed_services (main.rs:16748) returns every *.json under the services dir with no age pruning, and refresh_managed_service_runtime_liveness (main.rs:16715) demotes a record to a non-live status when its recorded pids are gone while persisting it. services stop likewise writes the record back rather than deleting it (main.rs:15446-15456), and rocm services list --all exists precisely to show "failed, stopped, and old service records". So stale records carrying their old ports are the normal steady state, and there is no services remove/prune subcommand to clear them (ServicesCommand is List|Logs|Stop|Restart, main.rs:798). Any unrelated process that later binds a recycled port makes the gate bail!, and UninstallOptions (main.rs:17878) has no override (force_dev_binaries is unrelated). The abort tells the operator to run rocm services stop <id> --yes — which for an already-stopped record neither changes the record nor frees the foreign port, so every retry fails identically.

Fix: apply managed_service_is_live(record) in the second loop as well (or require a live recorded pid to corroborate a bare port hit), and add a documented override flag so an operator is never locked out. Note that main.rs:30729 (uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections) deliberately writes a status = "stopped" record with a live listener and asserts the gate fails — that test currently encodes the lockout as the contract and must be reworked alongside the fix.

2. apps/rocm/src/main.rs:17970-17971 vs apps/rocm/src/uninstall.rs:44-50 — the plan warning promises a stop that will not happen.
build_uninstall_plan pushes "…have a running server; those servers will be stopped before removal" whenever any record is live, with no reference to what the plan actually removes. uninstall() prints the plan at line 25 and prompts at line 35, both before line 44 computes plan_removes_recovery_tooling and skips the stop pass entirely on a false. Verified unchanged since the held review. So rocm uninstall --keep-binaries --keep-data shows the operator a promise to stop their live GPU-holding server, they confirm, and nothing is stopped. Fix: gate the warning on plan_removes_recovery_tooling(&plan, &paths) (the predicate already exists at main.rs:17993) and word the ungated case as informational.

This gap is exactly what the tests miss: only_an_uninstall_that_removes_the_recovery_tooling_stops_servers (main.rs:30777) asserts the predicate in isolation, and nothing asserts the warning text agrees with it.

Non-blocking

• Revert check, per test: the four uninstall.rs tests inject the stop closure into stop_managed_services_then_remove, so removing the stop call is a compile error — genuinely tied. The main.rs gate/report tests call stop_managed_services_before_uninstall / uninstall_removal_gate / plan_removes_recovery_tooling directly, so a revert deletes the symbols — tied. The e2e scenario plants a real sleep 600 plus a service record and polls try_wait() on its own child, so a revert leaves it running and the assertion fails — genuinely tied, not a pass-either-way test.
• apps/rocm/src/uninstall.rs test a_live_managed_server_is_stopped_before_the_planned_paths_are_removed hardcodes port 9, and the e2e step hardcodes 59999; anything listening there on the runner flips these from pass to fail (via the very second loop in blocking item 1). Prefer a bound-then-dropped ephemeral port.
• probe_host (main.rs:15607) maps only 0.0.0.0, ::, [::], ""; other wildcard spellings a record could carry fall through and are probed literally.
• The e2e step comment says liveness "falls through to the recorded process" because nothing listens on the chosen port — accurate today, but it depends on the same unguarded port probe that blocking item 1 asks you to change; revisit it with that fix.

[siloteemu]

🔴 Automated review · pr-review-watcher · 451a8ee

Summary

Adds a stop-before-remove gate to rocm uninstall (stop every live managed service, abort with nothing removed if any stop cannot be confirmed), plus a port reality-check, plan-warning rework, an atomic service-record write, and unit + e2e coverage. Verdict: Needs work — the core gate is well-designed and genuinely tested, but the remediation commit introduces a Windows-risky third copy of an atomic-write helper, and two fail-direction defects remain. Verified: compiled a standalone probe confirming ("[::1]", port).to_socket_addrs() and whitespace-padded hosts fail resolution (so the port probe silently reads "nothing serving"); cargo test -p rocm and the feature-naming suite pass, and reverting the for record in attempted line makes a_stale_record_whose_old_port_was_reused_does_not_block_uninstall fail, so that test is real regression coverage; confirmed by reading source that stop_internal_managed_service does record.write()?, that apps/rocm/src/therock.rs:3042 and apps/rocmd/src/lib.rs:1139 already implement a hardened publish_temp_file, and that only *.json manifests live directly under services_dir. CI for this head: 18 success, 1 skipped, no failures. Blocking: 3 · Non-blocking: 4.

Prior review status (judged from the current code, not from the earlier report):

• Stale records judged by their recorded port → permanent, unrecoverable uninstall block: FIXED (probe restricted to services this pass attempted; pinned by a test verified to fail on revert).
• Plan warning promising a stop the run would not perform: FIXED (warning built after actions are final and gated on the same predicate, pinned by the_plan_warning_only_promises_a_stop_the_uninstall_will_perform).
• Wiring between plan / gate / removal untested: FIXED (uninstall_with_paths now driven end to end).
• probe_host not covering all wildcard spellings: PARTIALLY FIXED — see Blocking 2; the non-wildcard branch still discards the normalization.
• "Fail closed with no override and no recovery" as a class: STILL PRESENT for the unparseable-manifest path — see Blocking 3. The same reasoning that justified fixing the recycled-port case applies here and was not carried over.

🚫 Blocking (must fix before merge)

1. crates/rocm-core/src/lib.rs:7317-7357 — the new atomic write is a third, weaker copy of a helper this repo already hardened, and it now sits on the uninstall critical path.
ManagedServiceRecord::write was changed from fs::write to temp-file + bare fs::rename. This project already has that pattern twice — apps/rocm/src/therock.rs:2925-3080 and apps/rocmd/src/lib.rs:1053-1163 — and both wrap the publish step in publish_temp_file, which on Windows deliberately routes through ReplaceFileW (replace_file_windows) whenever the destination exists, precisely because a plain rename-over-existing is not reliable enough here. Both copies also carry a collision-avoidance loop and tests that a failed publish leaves no scratch file. The new implementation has none of that, and manifests are overwritten on every status transition. It matters more than usual because the failure propagates: apps/rocm/src/main.rs:15456 is record.write()?, so a publish failure inside stop_internal_managed_service becomes a failed entry in the stop report and hard-aborts the uninstall this PR is trying to make safe. I could not run Windows here, so I am stating the platform semantics from the repo's own prior art rather than from a live repro — but the prior art is unambiguous that this codebase does not consider bare rename sufficient. Fix: hoist write_file_atomically/stage_file_for_atomic_publish/publish_temp_file into rocm-core and call it from ManagedServiceRecord::write (which also removes the existing duplication between therock.rs and rocmd); note crates/rocm-core/Cargo.toml:31 currently lacks the Win32_Storage_FileSystem feature that apps/rocm/Cargo.toml:54 has.

2. apps/rocm/src/main.rs:15614-15624 — probe_host computes a normalized host and then throws it away, so the port reality-check fails open.
The function builds normalized (trimmed, bracket-stripped, lowercased) to classify the host, then the default arm returns host.to_owned() — the raw string. loopback_tcp_port_is_reachable (main.rs:13359) does (host, port).to_socket_addrs(), which I confirmed by compiling a standalone probe rejects both "[::1]" and " 127.0.0.1 ". A resolution failure returns false, and the caller at main.rs:15585 treats false as "nothing is serving" and continues. So for a record whose host is a bracketed IPv6 literal or carries stray whitespace, the only check that can catch a surviving engine grandchild — the documented reason this probe exists — silently disappears, and uninstall removes the tooling while the endpoint is live. This is representable: main.rs:10232 (loopback_host_key) already normalizes "[::1]", so the codebase itself expects bracketed spellings in records, and --host is a free-form string. Fix: _ => normalized in the default arm, and add a bracketed non-wildcard IPv6 case to every_wildcard_bind_spelling_is_probed_on_loopback — that test currently only exercises unbracketed literals, which is why this slipped through.

3. apps/rocm/src/main.rs:15674-15716 (with 15638-15667) — an unparseable manifest blocks uninstall permanently and the abort message prescribes a fix that cannot work.
unreadable_service_manifests pushes the filename as the service_id (main.rs:15660-15665), and the gate then tells the operator: "Stop them with rocm services stop <id> --yes, then re-run uninstall." For this failure mode that command cannot succeed — rocm services stop loads the same record and fails on the same JSON — so every retry aborts identically, with no flag to override and no mention anywhere (message, doc comments, docs) that the real remedy is to inspect or remove the file on disk. This is the same "no override, no recovery" property that was correctly judged unacceptable for the recycled-port case in this very commit; it should be judged the same way here. Minimum fix: include the full manifest path and state the actual remedy for the unparseable case (repair or delete the record), distinct from the still-serving case; better, add an explicit override for "I have verified nothing is serving".

Non-blocking

• tests/e2e-cucumber/features/install_lifecycle.feature:129 — the new scenario is @requires-os:linux only, so the gate has no Windows e2e coverage even though the Windows grandchild case is exactly why the port probe exists.
• crates/rocm-core/src/lib.rs:7344-7352 — a crash between write and rename leaves a hidden .<name>.<pid>.<ms>.tmp file that nothing ever sweeps; harmless to readers (all scanners filter on the json extension) but it accumulates.
• apps/rocm/src/uninstall.rs (a_live_managed_server_is_stopped_before_the_planned_paths_are_removed, the_uninstall_command_itself_stops_a_managed_server_before_removing_anything) and tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs:485 — the bind-then-drop ephemeral-port trick can race on a busy runner; uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections shows the safer shape (hold the listener open).
• apps/rocm/src/uninstall.rs tests — temp roots and spawned children are cleaned only on the success path, so a failing assertion leaks a directory; matches existing convention in this file, so only worth tightening opportunistically.

Superseded: this change request was left at an earlier head. The findings it raised are re-judged against 451a8ee in the current round — the stale-record port block, the plan-warning overclaim and the untested wiring are all fixed there. A fresh change request stands at the current head for the remaining items.

Withdrawing this change request as superseded: all three blocking points from 451a8ee are genuinely fixed at 4db3a7f — the atomic-write helper is single-sourced, the port probe keeps its normalization, and each abort class now names a remedy that can actually clear it. A fresh review at the current head is being filed separately, raising a new concern introduced by the latest commits; only that newer one is operative.

[siloteemu]

🔴 Automated review · pr-review-watcher · 4db3a7f

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

Summary

Adds a stop-before-remove gate to rocm uninstall (stop every live managed service, abort with nothing removed if any stop cannot be confirmed), and in the latest commits consolidates the atomic-write helper, makes the port probe resolvable, gives each abort class a remedy that actually works, re-probes every record so a retry stays honest, and stops the rocmd background helper first. Outcome: Needs work — all three of our prior blocking points are genuinely fixed, but the newest commit adds a destructive path we have not reviewed before and that this PR does not test. Verified: cargo test -p rocm-core --lib passes (333/333) on this checkout, and targeted cargo test -p rocm --bin rocm uninstall_removal_gate passes (3/3); confirmed by scratch-revert that the uninstall-ordering and uninstall_removal_gate tests fail when their production lines are reverted; confirmed by reading source that probe_host's default arm now returns normalized, that publish_temp_file/replace_file_windows moved into rocm-core byte-for-byte with Win32_Storage_FileSystem following them and no Windows FFI left in the app crates, that unreadable_service_manifests now pushes the full path with a RepairTheRecord remedy, that ProcessIdentity::new(pid, None) makes identity_state return Matches unconditionally, and that terminate_verified(..., Tree, .., force=true) escalates to SIGKILL across the whole tree. Blocking: 2 · Non-blocking: 5.

Prior objection status

• 1 — atomic write was a third, weaker copy of a hardened helper: RESOLVED. publish_temp_file and replace_file_windows (both cfg arms, the try_exists → ReplaceFileW → rename-race fallback, the SAFETY comment) are now single-sourced in crates/rocm-core/src/lib.rs:7634-7692; therock.rs:3038 and rocmd/src/lib.rs:1135 delegate to it, ManagedServiceRecord::write goes through the new write_file_atomically (collision loop, sync_all, cleanup-on-failure), and the windows crate feature moved to crates/rocm-core/Cargo.toml:31 with no direct FFI left behind in the app crates.
• 2 — probe_host discarded its normalization and failed open: RESOLVED. apps/rocm/src/main.rs:15763 is now _ => normalized, and every_wildcard_bind_spelling_is_probed_on_loopback gained [::1], " 127.0.0.1 " and [FE80::1] cases that assert both the string and to_socket_addrs().is_ok() — all three fail on revert, so it is real regression coverage.
• 3 — unparseable manifest was a permanent block with impossible advice: RESOLVED. main.rs:15733 now records the full path, the remedy is RepairTheRecord, and the gate's text (main.rs:15877-15891) tells the operator to check/stop the server, then repair or delete the named file — with a test asserting the message names the file and does not say rocm services stop. No override flag was added, and none is needed now that each failure class names a remedy that can clear it.
• Prior non-blocking Windows e2e gap: also resolved — lifecycle-24 adds the @requires-os:windows sibling of the Linux scenario.

🚫 Blocking (must fix before merge)

1. apps/rocm/src/main.rs:15557-15591 — rocm uninstall can now SIGKILL an entire process tree at a PID it cannot prove is rocmd.
stop_background_helper_before_uninstall builds ProcessIdentity::new(state.daemon_pid, None) (line 15573) and passes it to terminate_verified(&identity, KillScope::Tree, MANAGED_STOP_GRACE, true). With start_ticks: None, identity_state (crates/rocm-core/src/proc_lifecycle.rs:87-104) takes the (None, _) => Matches arm — the "legacy state file, best-effort proceed" path — so Recycled and Indeterminate can never be returned for this PID. force = true then means SIGTERM followed by SIGKILL across process_tree_pids(pid). This is the only kill in the codebase with no recorded identity: the pre-existing managed-service call site (main.rs:15394) passes real ticks, and ProcessIdentity's own doc comment says it exists to be "robust to PID recycling". Nothing invalidates daemon_pid: crates/rocm-core/src/lib.rs:5878-5892 only reads/writes automations/runtime-state.json, never deletes it, and running = false is written only on a clean shutdown (apps/rocmd/src/lib.rs:3053) — after a crash, OOM-kill or reboot the file survives with a stale PID. The guard at main.rs:15564-15567 checks only pid != 0, pid != self, and process_is_running, and (unlike the repo's own background_helper_already_running, main.rs:5963-5966) does not consult state.running. So the reachable sequence is: rocmd dies uncleanly → machine reboots → the OS reissues that low PID to an unrelated process → the user runs rocm uninstall → that process and every descendant are killed. That is exactly the "uninstall destroys something the user did not install" case, and it is new in this PR. Fix: persist the daemon's start ticks alongside daemon_pid in AutomationRuntimeState (capture with ProcessIdentity::capture at spawn, as services already do) and pass them here, so Recycled/Indeterminate are detected; when identity cannot be verified, do not signal — record a failed entry with the existing StopTheDaemon remedy (which already tells the operator to kill the pid themselves) rather than killing on a guess.

2. apps/rocm/src/main.rs:15557 — the new daemon-stop behaviour has no test at all, in a PR where every other new behaviour is revert-pinned.
Grepping the unit tests in main.rs and uninstall.rs, the feature file and lifecycle_steps.rs turns up no test that constructs an AutomationRuntimeState with a live PID, and no assertion on the "rocmd (background helper)" / "rocmd (pid …)" strings this function produces. Every test added or changed by this PR would still pass if the call at main.rs:15614 were deleted outright — the tests that do fail on revert (a_service_that_cannot_be_stopped_leaves_every_planned_path_in_place, uninstall_removal_gate_aborts_when_a_service_cannot_be_stopped, a_stopped_record_still_serving_its_own_model_blocks_uninstall, every_wildcard_bind_spelling_is_probed_on_loopback, both Linux-only live-server tests) all exercise the service path, not the daemon path. Given the blast radius in item 1, the untested part is the one that kills. Fix: add a Linux unit test in the shape of the existing live-child tests — write a runtime-state file pointing at a real spawned child, run the stop pass, assert the child is gone and appears in stopped; and a second asserting that a daemon which cannot be confirmed stopped lands in failed and makes the gate abort with nothing removed.

Non-blocking

• apps/rocm/src/main.rs:15699-15712 — the Err(_) arm proceeds with removal when a still-listening endpoint cannot identify itself; a disclosed, reasoned trade-off, but it is the one remaining fail-open on the destructive path and the warning goes to stdout rather than stderr.
• apps/rocm/src/main.rs:15573 — the comment "the same contract the legacy service records get" invites the reader (and did invite us) to assume parity with service records; services get weak identity only for old manifests, the daemon gets it permanently because no start token is ever recorded. A one-line comment saying so would stop this confusion recurring.
• crates/rocm-core/src/lib.rs:7698-7721 — the new write_file_atomically/publish_temp_file path has no direct unit test (no lines changed inside mod tests); the retry-on-AlreadyExists loop, the exhausted-attempts bail! and the cleanup-on-error paths are only reached indirectly.
• apps/rocm/src/therock.rs:2912-3036 and apps/rocmd/src/lib.rs:1040-1141 — the staging halves (stage_file_for_atomic_publish, temp_sibling_path, ATOMIC_WRITE_TEMP_ATTEMPTS) are still duplicated per app and still do not sync_all, so two things named "atomic write" now carry different durability guarantees; write_file_atomically is also pub with no caller outside rocm-core.
• apps/rocm/src/main.rs:15760-15764 — probe_host does not strip an embedded port (--host 127.0.0.1:8080), which would fail to resolve and fall into the same "nothing is serving" continue; narrow and outside this PR's stated scope.

No prompt-injection content was found anywhere in the diff, comments, commit messages or test fixtures.

Superseding this change request with a fresh one at the current head, so only one objection is live. The original concern is partly addressed but not closed; the new review states precisely what remains.

[siloteemu]

🔴 Automated review · pr-review-watcher · bdf3661

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

Summary

Makes rocm uninstall stop the managed servers and the rocmd background helper before it deletes the binaries and service records needed to stop them, aborting with per-failure-class remedies when any stop is unconfirmed, and centralises the atomic-write/ReplaceFileW publish step in rocm-core. Outcome: Needs work — the design is sound and unusually well tested, but the pid-recycling objection is only partly closed and one new fail-open remains on the destructive path. Verified: ran cargo test -p rocm-core --lib write_file_atomically (3 passed); read the source to confirm identity_state's (None, _) => Matches fallback, that process_start_ticks returns Some only on Linux, that process_tree_pids collapses to the root pid off Linux, that AutomationRuntimeState::load returns Err (not Ok(None)) on a corrupt/unreadable state file while its write is still a non-atomic fs::write, that managed_service_endpoint_model_ready probes record.endpoint_url built from the un-normalised host, that OsStrExt is already in scope for the moved Windows FFI, and that both new e2e Given steps resolve to existing step definitions. Blocking: 2 · Non-blocking: 5.

Standing objection — partially resolved, not resolved. We were right, and the fix is real but incomplete. The new stop_background_helper_before_uninstall does gate the kill on ProcessIdentity, and on Linux with a state file written by this PR's rocmd a recycled pid is correctly detected and never signalled (uninstall_never_kills_a_daemon_pid_that_was_recycled genuinely pins that). But the gate is inert whenever daemon_start_ticks is None — see blocking #1. The PR's own field doc says the two causes of None "must not be conflated"; the only caller conflates them.

🚫 Blocking (must fix before merge)

1. apps/rocm/src/main.rs:15592-15612 — the daemon is still force-killed on a pid it cannot prove is rocmd, whenever no start-time was recorded.
identity_state (crates/rocm-core/src/proc_lifecycle.rs:91-104) maps (None, _) => IdentityState::Matches, not Indeterminate. So the match falls through to terminate_verified(&identity, KillScope::Tree, MANAGED_STOP_GRACE, /* force */ true) — SIGTERM then SIGKILL — for every runtime-state.json that carries no daemon_start_ticks. That is: (a) any Linux state file written before this PR, i.e. the ordinary upgrade path for existing users, where collect_process_tree is available and the kill is the whole tree of a possibly-unrelated process; and (b) every macOS and Windows install, permanently, since process_start_ticks is #[cfg(not(target_os = "linux"))] -> None (proc_lifecycle.rs:312-316) — blast radius there is the single root pid, since process_tree_pids returns vec![root] off /proc, but it is still an unrelated process killed by an uninstall. This PR introduces the kill site; before it, uninstall signalled nothing. The stale-file scenario the code comment itself describes ("survives a crash, OOM-kill or reboot with running still true") is exactly the case a pre-upgrade state file represents.
Also note the new code never checks state.running, while the pre-existing double-spawn guard at apps/rocm/src/main.rs:5965 does — so even a cleanly shut-down daemon's leftover pid is a kill candidate.
Fix: distinguish "no identity recorded" from "identity verified" at this call site rather than relying on the best-effort fallback. Concretely, before signalling: if rocm_core::process_start_ticks(state.daemon_pid).is_some() && state.daemon_start_ticks.is_none(), this platform can verify and the record simply predates the field — treat it as Indeterminate (push StopFailureRemedy::StopTheDaemon, abort, leave the tooling in place) instead of killing. That closes the Linux upgrade hole outright and leaves the documented macOS/Windows best-effort degradation as the only remaining gap, which should then be stated in the daemon_start_ticks doc at crates/rocm-core/src/lib.rs:5869-5880 rather than only in a call-site comment.

2. apps/rocm/src/main.rs:15574 — an unreadable or corrupt runtime-state.json silently skips the daemon stop entirely.
let Ok(Some(state)) = AutomationRuntimeState::load(paths) else { return; }; collapses three outcomes into one. load (crates/rocm-core/src/lib.rs:5890-5901) returns Ok(None) only when the file is absent; a permission error or a truncated/unparseable file returns Err, and that Err is discarded. Nothing is pushed to report.failed, nothing is printed, and uninstall proceeds to delete the binaries and every service record while a live rocmd — which respawns a managed service whose endpoint stops answering — may still be running. This is the precise defect the PR exists to close, and it is the opposite of how the PR treats the equivalent service-side case (unreadable_service_manifests, apps/rocm/src/main.rs:15772-15781, deliberately turns an unparseable manifest into a hard failure with the RepairTheRecord remedy). It is reachable in practice because AutomationRuntimeState::write (crates/rocm-core/src/lib.rs:5903-5912) is still a plain non-atomic fs::write executed on every daemon tick, so a crash mid-write leaves exactly this file.
Fix: match explicitly — Ok(None) => return, Err(error) => report.failed.push(FailedManagedServiceStop { service_id: "rocmd (runtime state unreadable)", reason: format!("{error:#}"), remedy: StopFailureRemedy::StopTheDaemon }), Ok(Some(state)) => ….

Non-blocking

• apps/rocm/src/main.rs:15729 — the model-identity probe is handed the raw record, whose endpoint_url is built from the un-normalised host (format_http_base_url, crates/rocm-core/src/lib.rs:124), so a 0.0.0.0/:: bind is connected to literally — the exact non-portability probe_host was written three lines earlier to avoid; the fallout is a warned-but-fail-open pass on a wildcard-bound orphaned engine.
• crates/rocm-core/src/lib.rs:5903 — AutomationRuntimeState::write was left on non-atomic fs::write while ManagedServiceRecord::write moved to write_file_atomically; that is the file blocking #2 depends on being intact.
• apps/rocm/src/therock.rs:2925 and apps/rocmd/src/lib.rs:1053 — both keep their own full write_file_atomically staging pipelines and only call into the shared publish primitive, so neither picked up the new sync_all()-before-publish that the PR added specifically to prevent a zero-length file after a crash; three same-named functions now exist in the workspace.
• apps/rocm/src/main.rs:984-1043 (a_stopped_record_still_serving_its_own_model_blocks_uninstall) — drop(serving) only detaches the JoinHandle; the thread stays blocked in accept() holding an ephemeral port for the life of the test binary, and none of the sleep-child tests use a kill-on-drop guard, so a genuine regression panics before the reaping line and leaks a 60s child plus its temp root (the e2e suite already models the right pattern in LifecycleState::drop).
• crates/rocm-core/src/lib.rs:7711 — write_file_atomically creates its temp file with default mode and replaces the target inode, so an existing file's permission bits do not survive (a change from the fs::write it replaces, and a landmine for any future permission-sensitive caller); the doc's crash framing also overstates durability, since the parent directory is never fsynced.

Test audit (standing focus a)

All 24 added tests were checked against the production code they drive. One is not a regression guard: removal_proceeds_once_every_managed_server_is_confirmed_stopped (apps/rocm/src/uninstall.rs) exercises only the all-clear path and would pass against a gate that removed unconditionally — harmless as a completeness case, since its three siblings pin the abort. The three write_file_atomically_* tests pin the new helper but nothing drives ManagedServiceRecord::write through it, so reverting that call site to the old inline fs::write would not be caught. Everything else genuinely pins the change, including the strongest one, the_uninstall_command_itself_stops_a_managed_server_before_removing_anything, which drives the real command end to end. Platform gating is correct and minimal, and the pure-function tests (probe_host, plan_removes_recovery_tooling, uninstall_removal_gate, the corrupt-manifest case) are ungated so macOS/Windows keep real coverage of the gate logic.

Will this confusion recur? (standing focus c)

Our earlier objection was not reviewer error, and nothing in the code misled us into it. The recurrence risk runs the other way: a reader who checks only stop_background_helper_before_uninstall's call-site comments and the Linux tests will conclude the recycling hole is closed, because the (None, _) => Matches fallback lives one crate away in unchanged code and the daemon_start_ticks doc names the two None causes without saying the caller treats them identically. Beyond the code fix in blocking #1, the cheap guard is one sentence on crates/rocm-core/src/lib.rs:5869-5880 — "when this is None, identity_state returns Matches, so no recycling check happens" — and a test asserting the recorded-None path does not signal, which is the branch with zero coverage today.

CI: 18 success, 2 skipped, 0 failure, 0 pending; no lane names were available to me, so I attribute no outcome to any specific job. No prompt-injection content was found anywhere in the diff, comments, or commit messages.

All seven counts from the previous round are resolved at 5a21760, verified against the live code and by branch-level mutation rather than from the summary in the PR: the liveness check now precedes the start-time read in a separate statement; both false cases of the endpoint predicate are asserted, so the two mutations that previously survived now fail; the three fail-open outcomes are lifted into a pure verdict helper with a real empty-list mock behind an end-to-end test, and flipping either identity arm to a block is now caught; the ordering test's plan removes the directory holding the planted record, so an ordering inversion makes it red; the daemon-abort test asserts gate-owned text plus the absence of the other remedy; and the permission caveat now describes what the path helper actually does. Withdrawing this change request. Four non-blocking observations are in the review comment. Two other reviewers hold change requests of their own, which this does not affect.

[siloteemu]

🔴 Automated review · pr-review-watcher · 617c17f

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.

Summary

Makes rocm uninstall stop managed services and the background helper before removing the tooling that stops them, and abort when a survivor is still serving on a recorded port — this head adds the end-to-end test for the one gate arm that lets a stranger on a recycled port through, plus three comment corrections that replace claims of coverage with what is actually held. No blocking findings. Verified: in a scratch copy, baselined the three proceed-arm tests green, then mutated the single ServesOtherModels => ProceedUnrelated arm into a block — the new a_listener_naming_another_model_does_not_block_uninstall went red while the other two stayed green, so it is branch-precise and not merely revert-sensitive; separately confirmed both remedy sentences the strengthened uninstall.rs assertions pin exist verbatim in production (apps/rocm/src/main.rs:16161 and :16188, so the negative assertions can genuinely fail), confirmed no test in the module reads stderr (so the corrected comment's admitted gap is accurate rather than a new false claim), and confirmed the daemon-identity matcher model_refs_match is pre-existing and errs toward blocking rather than toward silent removal. The full workspace suite, clippy and the e2e suite were not run here; CI covers those. CI at this exact head: 24 success, 2 skipped, 1 pending, 0 failures — no per-lane detail was available, so no outcome can be attributed to a named job. One candidate blocker was raised and refuted: uninstall_stops_a_live_background_helper states in its own comment that it does not pin identity verification and names the two tests that do, both of which exist (apps/rocm/src/main.rs:31290 and :31382) — the honest disclaimer reads like a gap admission, but the code already carries the cross-reference that prevents the confusion. The checkout was left unmodified (git status clean at 617c17f); all mutation work was done in a scratch copy, since deleted. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• apps/rocm/src/main.rs:16016 — StoppedRecordVerdict::blocks() uses matches! with an implicit wildcard, so a future verdict variant silently defaults to non-blocking on a destructive path; an exhaustive match would make the compiler catch it instead of relying on a test that enumerates today's variants.
• apps/rocm/src/main.rs:31986 — the corrected comment is right that the two fail-open stderr warnings are pinned by nothing (confirmed: no test in the module reads stderr); routing them through a writer the tests can capture would close the last unpinned behaviour on this path.
• apps/rocm/src/main.rs:15849 — an unreachable probe host is a silent continue, and the reachability probe cannot tell "nothing is listening" (the normal case) from "the recorded host no longer resolves"; only the latter is a fail-open, and unlike its siblings it says nothing on stderr.
• crates/rocm-core/src/proc_lifecycle.rs:104 — the liveness-before-read ordering is genuinely load-bearing and the "held by construction" comment is sound, but a #[cfg(test)] inner function taking the two readings as parameters would pin the ordering deterministically without adding any runtime indirection to the primitive.
• Commit message — the head commit's Signed-off-by sits above a blank line, so git interpret-trailers --parse sees only the Co-Authored-By line as a trailer; AGENTS.md §11 also asks to "avoid AI-generated boilerplate footers", and the base branch's last 30 commits carry none.

[siloteemu]

🔴 Automated review · pr-review-watcher · 0907d82

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.

Summary

Makes rocm uninstall stop managed services and the rocmd daemon before deleting the tooling that stops them, aborting instead of removing when a stop cannot be proven; the newest commit turns the stop pass's operator warnings into assertable values, discloses a third previously-silent fail-open when a recorded host no longer resolves, makes StoppedRecordVerdict::blocks exhaustive, and moves the liveness/start-time ordering behind FnOnce probes so a test can pin it. No blocking findings. Verified: branch-level mutation confirmed the three warning tests genuinely fail when their own branch is deleted or inverted (deleting the host_port_resolves warning push, emptying the ProceedListingNothing arm, and making the silent ProceedUnrelated arm speak all go red), and hoisting the start-time read back into argument position turns a_dead_process_is_never_read_for_a_start_time red while cargo test -p rocm-core --lib proc_lifecycle is 17/17 green unmutated; also confirmed no-such-host.invalid truly fails to resolve here (so that test cannot pass vacuously — it would fail loudly instead), cargo check --locked agrees with the manifest/lockfile changes, cargo build -p rocm succeeds, every ManagedServiceStopReport literal was updated for the new field, the !(A && !B) == !A || B rewrite in identity_state preserves both semantics and short-circuit order. The full test suite was not run here, nor were the e2e lifecycle scenarios. Check runs at this head: 18 success, 2 skipped, 0 pending, 0 failures; no per-lane detail was available, so nothing is attributed to any named job. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• apps/rocm/src/main.rs:16037 — the StoppedRecordVerdict doc comment still says the proceed arms are reachable only "by standing up a server … and then reading stderr, which is why two of them went untested"; this very commit made them report values with assertions, so the comment now invites the next reader to conclude the arms are uncovered when they are.
• tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs:470 — the Windows scenario's justification comment claims it exercises the port reality-check against a surviving engine grandchild, but the planted record uses port: 0, so that branch is satisfied trivially on every run; what the scenario actually pins is PID-based termination.
• apps/rocm/src/main.rs:16214 — the remedy text in uninstall_removal_gate is assembled by an if-chain over StopFailureRemedy, not the exhaustive match deliberately used for blocks(); a sixth variant would still block correctly but silently lose its recovery advice.
• crates/rocm-core/src/proc_lifecycle.rs:150 — identity_state_with_observed re-runs the liveness check that identity_state_from_probes just performed, doubling /proc reads inside the 25 ms poll loop; pre-existing rather than introduced here, but the refactor is the natural moment to split out a compare-only helper.
• crates/rocm-core/src/proc_lifecycle.rs:165 — (None, _) => Matches plus process_start_ticks returning None on Windows means the "never signal a PID it cannot prove is rocmd" property is Linux-only in practice, permanently rather than only for legacy records; unchanged by this PR, but worth stating in the PR text given how central that claim is to it.

Check-run conclusions were re-read immediately before posting and are unchanged from the counts above.

[siloteemu]

🔴 Automated review · pr-review-watcher · e9bd2af

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.

Summary

Makes rocm uninstall stop managed services before removing tooling and refuse to remove anything while a stop is unconfirmed, with remedy advice per failure class; outcome: needs work — one test assertion does not constrain the code it names. Verified: ran the rocm-core process-lifecycle unit tests (17 pass) and the rocm binary unit tests, then branch-level mutation of three individual branches added by this PR — deleting the ProceedListingNothing warning and flipping that arm from proceed to block are each caught by a named test, but deleting the remedy-ordering sort leaves the entire suite green; the full test suite and the e2e suite were not run here. Check-run conclusions at this head: 18 success, 2 skipped, 0 failure, 0 pending. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

• apps/rocm/src/main.rs:31689-31700 — the ordering assertion inside every_failure_class_carries_its_own_recovery_advice cannot fail for the defect it claims to catch ("remedies must stay ordered most-actionable-first: a dead-end-avoidance ordering, not cosmetics"). The five FailedManagedServiceStop fixtures at main.rs:31631-31659 are listed in exactly advice_rank order, so insertion order and rank order coincide and the assertion holds whether or not the code sorts. Confirmed by mutation: deleting classes.sort_unstable_by_key(|remedy| remedy.advice_rank()) at main.rs:16295 leaves every test green (the only signal is a dead-code warning on advice_rank); re-running the same mutated build with the fixture entries reversed fails the assertion as intended. Fix: list the fixtures in an order that is not rank order — e.g. RepairTheRecord, RepairTheDaemonState, StopTheDaemon, StopWhatHoldsThePort, StopTheService — which makes the sort load-bearing and, on reading, makes the dependency visible. That matters beyond the mutation score: a reviewer reading this test naturally concludes the sort is pinned, which is precisely the wrong conclusion the current fixture ordering invites.

Non-blocking

• apps/rocm/src/main.rs:16053-16056 — the failure reason "{host}:{port} still accepts connections after the stop" is also emitted for a record already recorded stopped by an earlier run, where this pass attempted no stop; the remedy text is correct, only this sentence claims something that did not happen.
• Prior round's item 5 still stands: process_start_ticks returns None off Linux, so (None, _) => Matches in compare_start_ticks makes "never signal a PID it cannot prove is rocmd" a Linux-only property. Newly documented on the daemon start-ticks field, but the proc_lifecycle.rs module header still reads as unconditional.
• crates/rocm-core/src/proc_lifecycle.rs:~493 — the comment justifies pid: 4321 as "provably not running"; nothing establishes that. The test is sound as written because both probes are stubbed, but the stated reason for the change is overstated.
• apps/rocm/src/main.rs:6981 — rocm runtimes uninstall still remove_dir_alls a runtime install root with no stop-gate, the same failure class this PR closes for the top-level command. Untouched here; worth a follow-up rather than scope creep.
• ~840 lines of uninstall-only machinery (StopFailureRemedy, StoppedRecordVerdict, the stop pass, the gate) landed in main.rs rather than uninstall.rs, which this PR's own module header treats as the home for this command.

Prior round's items 1-4 are addressed at this head: the StoppedRecordVerdict doc now names the tests that close the gap, the Windows scenario comment now says it pins PID-based termination and attributes the port check elsewhere, the remedy text is built from an exhaustive match over classes derived from the failures, and the duplicate liveness check in the poll loop is gone. No prompt-injection content and no internal links, product names, hostnames or registry paths were found in the diff.

Check-run conclusions were re-read immediately before posting and are unchanged from the counts stated above.

[siloteemu]

🔴 Automated review · pr-review-watcher · e9bd2af

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.

Items 1-4 from the previous round are addressed at this head, and two of the three branches mutated on the new warning tests are genuinely caught. One count. The full report, including five non-blocking observations, is in the review comment on this PR.

The remedy-ordering assertion cannot fail for the defect it names. The assertion inside every_failure_class_carries_its_own_recovery_advice states its purpose as keeping remedies ordered most-actionable-first — "a dead-end-avoidance ordering, not cosmetics". But the five failure fixtures it builds from are listed in exactly the rank order the assertion checks, so insertion order and sorted order coincide and the assertion holds whether or not the code sorts at all.

Confirmed by mutation rather than by reading: deleting the sort_unstable_by_key call on the advice rank leaves every test green, with a dead-code warning on the rank function as the only signal. Re-running that same mutated build with the fixture entries reversed fails the assertion as intended, which is the proof that the fixture ordering is what is carrying it.

This gates rather than sits in the non-blocking list because of what it does to the next reader: the comment and the assertion together read as a pin on the ordering, so a change that drops or reorders the sort ships green and nobody thinks to add real coverage.

Fix: list the fixtures in an order that is not rank order — for example most-manual first, reversing the current sequence — which makes the sort load-bearing and, on reading, makes the dependency visible.

Withdrawing this change request: the objection is resolved at 5202140. The remedy-ordering fixtures are no longer listed in rank order, so the sort is now load-bearing — verified by deleting the sort call on a scratch copy, which turns the ordering assertion red as a test failure rather than only producing a dead-code build warning. Full findings are in the review comment on this PR.

[siloteemu]

🔴 Automated review · pr-review-watcher · 5202140

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.

Summary

The PR makes rocm uninstall stop managed services and verify they are really down before removing the tooling that could stop them, aborting with per-failure-class recovery advice when a stop cannot be confirmed; the new head commit reorders the remedy-ordering fixtures, splits one misleading failure message in two, and corrects two comments. Our live change request is RESOLVED. Outcome: no blocking findings. Verified: on a throwaway copy of the tree (the reviewed checkout was left untouched and confirmed clean), the single test every_failure_class_carries_its_own_recovery_advice passes as shipped, and with the sort_unstable_by_key call on the advice rank deleted it now fails as a TEST failure — the remedies must stay ordered most-actionable-first assertion panics — not merely as a dead-code build break; the dead-code warning on the rank function appears alongside it, but the red assertion is the primary signal, which is the stronger of the two outcomes. The full suite and the GPU/Windows end-to-end lanes were not run here. Checks at review time: 2 pending, 2 skipped, 23 success. Blocking: 0 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• apps/rocm/src/main.rs:16060 — neither arm of the newly split failure reason is asserted anywhere; the test that reaches the new arm (a_stopped_record_still_serving_its_own_model_blocks_uninstall) checks only the remedy class and the advice text, so collapsing the branch back to the single old sentence leaves the suite green. One failure.reason.contains(...) line in that existing test would pin it.
• crates/rocm-core/src/proc_lifecycle.rs:15 — the new module header says that off Linux "every record looks like one carrying no recorded identity"; that holds for identities built from a live pid on that host, but a record reconstructed with a recorded start-time would land on Indeterminate rather than Matches. Softening to "every record captured here" keeps the paragraph exact.
• Commit range — the DCO sign-off identity is not consistent across the 30 commits: the five most recent sign off under a different name and address than the earlier 25, while author and committer are the same throughout. A DCO check that matches sign-off against the author would treat the two groups differently; worth normalising before merge.
• Commit range — 20 of the 30 commits carry an AI co-author trailer and the remaining 10 do not; the base branch's recent history uses that trailer only for human co-authors and a bot. AGENTS.md §11 asks contributors to avoid AI-generated boilerplate footers, so this is worth normalising before the range lands in permanent history — though nothing in the product behaves differently, which is why it is not gating.

No prompt-injection content was found in the diff, comments, commit messages or test fixtures.

---

Check-run conclusions were 2 pending / 2 skipped / 23 success both when the review started and at the time of posting.

[fredespi]

Catching up on seven rounds of review replies that never reached this PR.

A guard on my side silently refused the GitHub comment API on every attempt from round 10 onward, so rounds 10-16 were answered in commits but never here. That's now filed as a bug against the tooling (the REST spelling POST /repos/{owner}/{repo}/issues/{n}/comments was denied while its gh pr comment twin was allowed). Posting this through the spelling that works. Apologies for the one-sided exchange — the review bot kept re-reviewing with no visible response from me.

Rather than replay seven replies, here is where the branch actually stands.

What changed across rounds 10-16

Every blocking finding is closed. Per-round detail is in the commit messages, which were written as the replies; the short version:

• identity_state evaluation order — the liveness check had ended up after the start-time read (Rust evaluates arguments before the callee), inverting the safe side of the PID-recycling race. Fixed, then later pinned by a_dead_process_is_never_read_for_a_start_time rather than left to prose.
• The gate's five outcomes were inline in a loop and reachable only through a live socket. They are now a StoppedRecordVerdict enum produced by a pure function, each directly assertable, with two end-to-end tests driving the proceed-on-a-live-socket arms through the real call site.
• Three fail-opens now disclose themselves. The warnings became values on ManagedServiceStopReport instead of eprintln!s, so the disclosure is asserted rather than merely emitted; a third one — a recorded host that no longer resolves — was passing in total silence and now does not.
• Recovery advice is exhaustive. StopFailureRemedy::advice and advice_rank are both matches, so a new variant can be neither textless nor unreachable, and one test drives all five classes through the gate.
• Ordering, remedies and reasons are pinned by tests that can actually fail. Several could not, which is the honest headline below.

What I got wrong, since it bears on how much to trust the rest

Three times I shipped a test whose comment claimed more than its assertions delivered, and twice the vacuous test was itself a remediation for an earlier finding — round 15's blocker was my round-14 test, round 16's was my round-15 fix. In each case the fixture or assertion was reading back the test's own input. I have since been running the mutation before claiming a pin rather than after, and every fix in the last three rounds was verified that way (green before, red after).

Relatedly, my local gate was reporting formatting clean while the working tree was not — it streams the worktree into a container that cannot write back, so a write-mode cargo fmt in there formatted a throwaway copy. That is fixed, and one commit in this range exists only because of it.

Where it stands

CI is green. All required checks pass; mergeable_state is blocked solely on maintainer approval.

The last four review rounds scored 0 blocking findings between them (the one exception being my own regression above). By the reviewer skill's own clean-PR rule that is convergence, so I am stopping here rather than continuing to turn non-blocking nits into pushes — each push re-triggered the bot, which re-triggered me, and 16 rounds is more than this change warranted.

Needs a human, not another round

1. Maintainer approval — the only thing blocking merge.
2. One history rewrite, if wanted: five commits in this range carry a Signed-off-by identity that matches neither the author nor the other 25 (my error, introduced while fixing trailer placement), and 20 of 30 carry an AI co-author trailer that AGENTS.md §11 asks contributors to avoid. Both need a force-push, which my authorisation on this branch excludes. Happy to do it on a word.
3. Offered, not done: relocating ~840 lines of uninstall machinery from main.rs into uninstall.rs (where this PR's own module header says they belong) as a mechanical follow-up PR, and splitting out the atomic-write consolidation. Both would invalidate the line anchors of sixteen rounds of review if done here.
4. Follow-up worth filing: rocm runtimes uninstall still remove_dir_alls a runtime install root with no stop-gate — the same failure class this PR closes for the top-level command.

[siloteemu]

🔴 Automated review · pr-review-watcher · f323e7f

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.

Summary

The PR makes rocm uninstall stop the services it manages — and prove they stopped — before deleting the tooling that stops them, adding a verdict gate, recovery advice, endpoint probing and an atomic service-record write. The code is in good shape; the only rule issue found is in commit metadata, and it is raised below as a non-gating note rather than a merge gate. Verified: on a scratch copy (never the branch) I moved the start-time read back ahead of the liveness check in crates/rocm-core/src/proc_lifecycle.rs and ran cargo test -p rocm-core proc_lifecycle — a_dead_process_is_never_read_for_a_start_time failed (16 passed, 1 failed), so that test genuinely catches the PID-recycling defect it names; the full test suite, the e2e lifecycle scenarios and the Windows lanes were not run here. I also independently confirmed the IdentityState::Indeterminate claim in the module header against compare_start_ticks, that the three leaves_every_planned_path_in_place tests create a real file before asserting it survives, that the e2e Then step polls its own child's exit rather than a PID probe (so it can fail on a revert), that the lifecycle scenarios really do run in CI rather than being opt-in-only there, that scenario renumbering matches the convention enforced by tests/feature_naming.rs, and that the diff carries no internal names, hosts or links. Roughly 50 added tests were audited for whether their assertions can fail for the defect their names claim; none were found vacuous. Blocking: 0 · Non-blocking: 6.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• Commit metadata across the range — 21 of the 31 commits carry a Co-Authored-By: Claude … trailer, while none of the 60 commits behind the base branch tip do. AGENTS.md §11 states verbatim: "avoid AI-generated boilerplate footers", so this is a real rule violation that lands permanently in public history on merge. It is raised as a note rather than a merge gate for two reasons: earlier review rounds on this PR passed these same commits without objecting, so gating on them now would move the line mid-review; and nothing about it changes what the built tool does for a user. A maintainer who wants the history clean should say so — the author has offered to rewrite it and notes that doing so needs a force-push, which is a maintainer's call. If the rewrite happens, §11 otherwise prefers individual commits over a squash, so a rebase over the range preserves the per-commit history deliberately built here.
• Commit range — five commits sign off as a different name and address than the commit author and than the other 26; the DCO trailer should identify one consistent signer. Worth settling in the same rewrite if one happens.
• apps/rocm/src/therock.rs:2922 — the atomic-write/ReplaceFileW consolidation into rocm-core is a second logical change riding along with the uninstall gate; AGENTS.md §11 asks for one logical change per PR, and splitting it would also shrink the review surface.
• apps/rocm/src/main.rs:15491 — roughly 840 lines of uninstall-only machinery (stop report, verdict, remedies, probing) were added to main.rs, already ~32,900 lines, rather than to apps/rocm/src/uninstall.rs where the command lives; a mechanical follow-up move would help.
• apps/rocm/src/main.rs:6994 — rocm runtimes uninstall still remove_dir_alls the runtime install root with no stop gate. I confirmed this directly rather than taking the author's word for it; severity is lower than the bug this PR fixes because the recovery tooling survives that path, but it is the same failure class and deserves a follow-up.
• README.md:477 and docs/ — rocm uninstall can now abort and remove nothing when a managed service cannot be confirmed stopped, which is a new user-visible outcome that no document describes; one line next to the synopsis would save an operator guessing at the abort message.

---

Check state: this review worked from 18 success / 0 failure / 7 pending / 2 skipped on this commit, and immediately before posting the same commit reports 22 success / 0 failure / 3 pending / 2 skipped. No failing check was observed at any point, but some were still running, so this is not a statement that every check has finished.

[fredespi]

Zero blocking findings, so I'm treating this as converged and not pushing another round.

Four of the last five rounds have scored Blocking: 0, and the one exception was a regression in my own previous round's fix rather than the review finding new ground. Continuing to turn non-blocking notes into commits is what took this to sixteen rounds: each push re-triggers this review, which re-triggers me. Stopping there.

None of the six notes are dropped. Four are already with a maintainer and unchanged: the co-author trailer and the inconsistent DCO signer (one rebase settles both — I've offered, it needs a force-push authorisation I don't have); splitting out the atomic-write consolidation; and moving the ~840 lines into uninstall.rs. Thanks for confirming §11 prefers a rebase over a squash if the rewrite happens — that matches how the per-commit history was built.

Two I'd add to that list rather than act on now:

• The docs gap is a fair hit and it's mine. rocm uninstall can now abort having removed nothing, and no document says so. It's the most substantive of the six and I'd normally just fix it — but a one-line README change is still a push, a re-review and another round, which is the trade I'm declining. It should land in whichever touch a maintainer decides on above.
• rocm runtimes uninstall — agreed on both the diagnosis and the lower severity, and thanks for verifying it directly rather than taking my word. Genuinely separate work.

If a maintainer wants any of these, I'd rather take them as one batch than one per round. CI is green and the only thing blocking merge is approval.

[fredespi]

@michaelroy-amd @rominf — status for the two CHANGES_REQUESTED reviews. Both were filed at e2015fb1; head is now f323e7fe, 29 commits later. Everything you blocked on is fixed, so this is a request to re-check rather than new work.

I'm posting because GitHub still shows CHANGES_REQUESTED and that's accurate bookkeeping — a dismissed automated review doesn't clear a human one. The inline threads were resolved as the fixes landed, but neither of you was told directly, which is my omission.

1. load_managed_services silently skipping a corrupt manifest (both of you; rominf noting it contradicted the fail-closed doc comment).

Fixed, though not where you'd first look. load_managed_services still uses if let Ok(..) at main.rs:17402 — deliberately, because skipping an unparseable record is correct for listing. The uninstall path no longer relies on it. unreadable_service_manifests (main.rs:16242) scans the services directory independently, and stop_managed_services_before_uninstall runs it as the first step (:15850), pushing each unparseable file into report.failed with RepairTheRecord, then returning early at :15861 before anything destructive runs.

The ordering is deliberate and worth a look: the manifest scan is the only non-destructive check of the two, so it runs ahead of the rocmd stop. Otherwise a doomed run would force-kill a live, perfectly verifiable daemon for an uninstall that then removes nothing.

Regression test is uninstall_refuses_when_a_service_record_cannot_be_parsed (main.rs:31525): plants { not json, asserts the gate fails naming that path, and asserts the abort message says "repair or delete the file" and does not say rocm services stop — because rocm services stop loads the same file and fails identically, so prescribing it would make every retry abort the same way with no way out.

2. No Cucumber scenario / dependency on #241 (michaelroy-amd; rominf confirming the id was nowhere in the tree and #241 was CONFLICTING).

Fixed by adding the scenario here rather than depending on #241, which is the option you offered and the right one given #241's state. @id:lifecycle-linux-uninstall-stops-managed-server — install_lifecycle.feature:129, Scenario: lifecycle-10 - Linux - uninstall stops the local server it manages. It starts a real process, plants a service record, and asserts the process exited. There's a Windows counterpart at lifecycle-24. Both run on the opt-in @lifecycle lane; I've run the Linux one locally, all 8 steps passing.

3. rominf's additional points — all taken. The abort no longer claims a clean no-op: it states what the aborted run already did, because the stop pass isn't atomic and a service stopped before the failing one is down for good, with its endpoint key dropped. The stop pass is now scoped to uninstalls that actually remove the tooling, the plan warning counts only live records, and the .ok() that swallowed the diagnosis is gone — the load error propagates and aborts.

On the refresh_managed_service_runtime_liveness Windows grandchild gap you flagged but didn't file: that's covered by a post-stop socket probe, which fails the gate on anything still accepting connections regardless of what the PID tree says.

State: all 29 checks pass, 0 failing, 0 pending, MERGEABLE. All 12 review threads resolved. The automated reviewer has returned Blocking: 0 on the last several rounds and I've stopped pushing on its non-blocking notes, which are listed in my comment above and are maintainer calls (the Co-Authored-By trailers and DCO signer inconsistency need a rebase and a force-push authorisation I don't have; the atomic-write split and the main.rs → uninstall.rs move are follow-ups; the README line for the new abort outcome is a fair hit I'd fold into whichever touch you choose).

No action needed from me that I can see — but if either of you wants any of that batched into one rewrite, say which and I'll do it in a single push rather than one per round.

Refs: EAI-8014

[rominf]

🔴 Automated review · pr-review-watcher · f323e7f

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.

Summary

rocm uninstall now stops the background helper (rocmd) and every live managed server before it removes anything. If any stop can't be confirmed it exits non-zero, removes nothing and prints advice for each kind of failure. The PR also moves the workspace onto one shared atomic-write/ReplaceFileW helper in rocm-core. Outcome: Needs work. The stop logic itself checks out; the gaps are project-rule gaps (docs and a scenario) plus two test/dead-code items. Reviewed: the whole change, prw-base...HEAD (31 commits, 12 files), including callers and callees in main.rs, rocmd, proc_lifecycle.rs and the e2e harness. The PR description was not available to this review, so I did not compare it against the diff. Verified: the PR merges cleanly onto the current base. On the merged tree, cargo test --workspace --all-targets --no-run builds and cargo clippy --workspace --all-targets -D warnings is clean. 119 filtered uninstall/gate tests pass on the merged tree; on HEAD the 54 uninstall/gate tests pass, and rocm-core --lib passes 344/344. Confirmed against the source: rocmd writes no state file without --automations-enabled and writes running:false only on clean shutdown. stop_internal_managed_service reports stopped only once every recorded process is gone. terminate_verified re-checks identity before it sends any signal. OsStrExt is imported for the Windows ReplaceFileW path (checked by reading only — no Windows target is installed). The lifecycle-NN renumbering satisfies feature_naming.rs, and nothing else in the merged tree uses the old indices. Not run: the e2e lifecycle lanes and any Windows build. Blocking: 4 · Non-blocking: 9.

🚫 Blocking (must fix before merge)

1. tests/e2e-cucumber/features/install_lifecycle.feature:126 — The new abort behaviour has no Gherkin scenario and no stated reason for the gap. That behaviour is: non-zero exit, nothing removed, and recovery advice naming what to fix. AGENTS.md §3 requires a scenario for changes to exit codes and output. The only coverage is uninstall.rs unit tests that hand in a fake failing stop pass, which proves the function, not what the user sees. This is easy to stage black-box. Plant an unparseable *.json under the isolated services/ dir, then run rocm uninstall --yes from the installed binary. Assert three things together: a non-zero exit, the install and state dirs still present, and the output naming that manifest path with the "repair or delete the file" advice. That also checks the advice against the behaviour, as §3 requires.
2. apps/rocm/src/main.rs:508, README.md:477, docs/testing.md:808, docs/manual-testing.md — None of these were updated, although uninstall's observable behaviour changed. It now kills a live rocmd and stops running servers. It can refuse and exit non-zero, leaving everything in place. And --keep-binaries --keep-data deliberately leaves servers running. AGENTS.md §5 requires the --help doc comment ("Remove ROCm CLI-managed files from this computer."), README, docs/testing.md and docs/manual-testing.md to change in the same PR. Specifically:
   • docs/testing.md: its list of what @lifecycle covers ends at "full-purge uninstall" and should mention the two new scenarios.
   • docs/manual-testing.md: needs a manual check for the refuse-and-advise flow.
3. apps/rocm/src/main.rs:18630 — No test covers the plan's "none is recorded as running" warning. the_plan_warning_only_promises_a_stop_the_uninstall_will_perform only stages a live record, so it reaches the "will be stopped" and "left running" variants. This line is new user-facing output, and its own comment says the exact wording matters: it is "the last thing read before confirming". Yet changing it to promise a stop leaves the suite green. Add a case with a record that exists but is stopped, and assert this exact line.
4. apps/rocm/src/main.rs:16034-16036 — This report.stopped.retain(...) in the BlockAuthRefused branch can never do anything. That branch is only reached when !stopped_by_this_pass, and report.stopped only ever holds ids from attempted plus the rocmd entry, so the id being removed is never in the list. No test can catch it being deleted. It also misleads the reader into thinking a record this pass stopped can arrive here. Delete it, as the project's no-dead-code rule requires; the retain at :16051 is the one that matters.

Non-blocking

• apps/rocm/src/main.rs:15916 and :31795 — Both comments say "nothing prunes them, and there is no services remove". The current base has since added rocm services remove and rocm services prune (feat(services): add a supported way to remove non-live local server records (EAI-8075) #411), so the stated reasoning is false once merged. The user-facing advice for an unparseable record stays accurate, because feat(services): add a supported way to remove non-live local server records (EAI-8075) #411 leaves unparseable manifests alone.
• apps/rocm/src/main.rs:15778 — No test reaches the daemon IdentityState::Indeterminate branch. Deleting it is still safe: terminate_verified returns Unverified and the run still aborts. Only the reason text and the "stopping the background helper" line would change. A one-line comment saying so would help.
• apps/rocm/src/main.rs:18606-18642 vs :15792 — Before the user confirms, the plan announces the server stops but not that a live rocmd will be force-killed (whole process tree on Linux). The kill is first mentioned after confirmation, during the stop pass.
• apps/rocm/src/uninstall.rs:80 / main.rs:16242 — If the services dir can't be read, the error propagates through ? without the "uninstall aborted … No files were removed." wording that every other abort uses. An operator can't tell from the output that nothing was touched.
• apps/rocm/src/uninstall.rs:271, :404, :475, apps/rocm/src/main.rs:32531, crates/rocm-core/src/proc_lifecycle.rs:465 — These comments narrate review history ("an earlier round found", "an earlier version of this comment claimed both", "a review caught here") rather than the code. Readers without the PR thread can't use them.
• Comments throughout the new gate code (e.g. the StoppedRecordVerdict docs and the "tests live at the bottom of this file" pointers) name specific tests and say which will go red. They are very long, and every rename or refactor will make them stale. Consider cutting them to the invariant each one protects.
• crates/rocm-core/src/proc_lifecycle.rs:178-192 — The doc warns about a stale reading between taking it and calling the function. It doesn't note that at the only caller (main.rs:15768) the window can't produce a wrong signal, because terminate_verified re-reads identity first. A cross-reference would save the next reader from re-deriving that.
• apps/rocm/src/main.rs:31255, :32424, :32471, :32755 (and the uninstall.rs Linux tests) — These spawn sleep 60 with no drop guard, so a failing assertion leaks the child. Other code in the same file already uses a drop guard for exactly this case.
• apps/rocm/src/main.rs:32263 — The test relies on no-such-host.invalid failing to resolve. Under a resolver that answers every name, it would stop testing the "does not resolve" warning and nothing would flag it. There is no self-skip like the one the IPv6 test has.

[rominf]

🔴 Automated review · pr-review-watcher · f323e7f

Requesting changes at f323e7fe for the four blocking findings in the automated review comment on this PR. In short:

1. The new abort path (non-zero exit, nothing removed, recovery advice) has no Gherkin scenario, which AGENTS.md §3 requires for exit-code and output changes. One way to stage it: plant an unparseable manifest under services/, run rocm uninstall --yes, and assert the exit code, that the dirs are still there, and the advice text, all in the same scenario.
2. Uninstall's observable behaviour changed, but the Uninstall --help doc comment, README.md, docs/testing.md and docs/manual-testing.md were not updated in the same change (AGENTS.md §5).
3. No test covers the plan warning "none is recorded as running" (main.rs:18630), so it could be reworded to promise a stop and the suite would stay green.
4. The report.stopped.retain(...) in the BlockAuthRefused branch (main.rs:16034-16036) can never do anything. Please remove it.

Addressing these will resolve this request. The automation will withdraw it on a later round once the code no longer shows them.

[fredespi]

#299 (comment)

Fixed in b7423d5 (also merged main, conflict in rocmd resolved). 1: new scenario lifecycle-25 (unparseable services/ record -> non-zero, install+state kept, advice names the file); red on origin/main, green here. 2: Uninstall --help, README, docs/testing.md, docs/manual-testing.md updated. 3: added test for the "none is recorded as running" plan warning. 4: removed the dead retain. Nine non-blocking notes not addressed in this push.

[fredespi]

#299 (review)

Fixed in b7423d5 (also merged main, conflict in rocmd resolved). 1: new scenario lifecycle-25 (unparseable services/ record -> non-zero, install+state kept, advice names the file); red on origin/main, green here. 2: Uninstall --help, README, docs/testing.md, docs/manual-testing.md updated. 3: added test for the "none is recorded as running" plan warning. 4: removed the dead retain. Nine non-blocking notes not addressed in this push.