[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 — the command handler: 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/uninstall_gate.rs (new; owns its types) — stop_managed_services_before_uninstall() returns a ManagedServiceStopReport { stopped, failed, warnings, helper_stopped }, 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/service.rs), and the gate classifies the PID with rocm-core's existing identity_state, which checks liveness before it reads the start time, so a recycled PID cannot read as a match. 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.

Windows start-times

• crates/rocm-core process_start_ticks now reads the process creation time through GetProcessTimes on Windows, so PID-reuse checks run there too: every Windows managed-service stop can now return Recycled or Indeterminate where it used to always match.

What uninstall leaves running

• A run that keeps the binaries and data (--keep-binaries --keep-data) skips the stop pass and leaves running servers alone; plan_removes_recovery_tooling decides this and the plan warning says so. Documented in --help and the README.

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.
• The uninstall plan and the stop pass share one non-destructive pre-flight (unreadable manifests, live records with no start time, already-stopped records whose port still answers, helper identity), so the plan predicts a refusal instead of promising a stop. A record is stopped when its recorded pids are still in use, not only when its status reads live, and a service counts as stopped only on the confirmed-stop result.
• A removal that fails partway reports what was removed, what failed (and may be partly removed), and what was not attempted, and says recovery tooling may be gone. It is not transactional.
• The relaunch advice for a lost endpoint key comes from one shared builder (rocm-engine-protocol). Only the uninstall gate fills in the model, engine and port from the service record. The rocm serve and rocmd lost-key refusals now print the same shared command (with the <model>/<port> placeholders still unfilled) instead of their old hardcoded text. The relaunch command carries the model, engine, host, port and key flags only, not --runtime-id, --env-id or the device policy a service was launched with.
• Process-tree stop is Linux-only in effect: on Windows only the recorded process is terminated, and the help, README and plan say so.
• 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 @lifecycle scenarios: lifecycle-10 (Linux) and lifecycle-24 (Windows) stop the local server uninstall manages; lifecycle-25 refuses and keeps everything when a service record cannot be parsed; lifecycle-26 stops the background helper; lifecycle-27 runs --keep-binaries --keep-data and leaves a running server and its record alone; lifecycle-28 refuses a live server recorded without a start time; lifecycle-29 refuses on an unreadable helper state file. Existing scenarios were renumbered. The Windows scenario cannot run in the Linux gate I used; CI is its first run.
• 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.
• The vLLM and Lemonade engine state files, and the JSON merge that patches them (merge_json_state_file), are now published atomically through write_file_atomically, so rocmd and uninstall never read a torn document.

[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.

[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.

Superseded by a fresh review at the current head. Re-checked both earlier objections: the unreadable-state-file objection is resolved — the load outcomes are now distinguished and an unreadable file aborts the uninstall instead of silently skipping the helper stop. The force-kill-without-a-recorded-start-time objection is closed on Linux by the new guard; the residual Windows/macOS case is documented and pre-existing, and is carried forward as a non-blocking note rather than a block. A separate, newer review at this commit records the remaining concerns; only that newer one is operative.

[siloteemu]

🔴 Automated review · pr-review-watcher · 074b37b

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 rocmd background helper and every live managed model service — verifying identity by process start-time and by probing the recorded endpoint — before deleting the tooling that could stop them, aborting with everything intact if any stop cannot be confirmed. Outcome: Needs work — the safety design is sound and substantially better than the previous round, but one of the two remediation commits does not deliver the revert-protection it claims, and the fix for our second objection leaves the operator an abort whose stated remedy cannot be followed. Prior objection 1 (force-kill of an unrelated tree with no recorded start-time): partly resolved — closed on Linux via the new unverifiable_pre_upgrade_record guard, still live on Windows/macOS where no start-time is readable, identity_state returns Matches and the tree is killed with force: true; documented, pre-existing, and not a regression, but Windows is a first-class supported platform per AGENTS.md §6. Prior objection 2 (unreadable state file silently skipping the stop): resolved — load's three outcomes are now distinguished and Err aborts the uninstall; load returns Ok(None) only when the file is absent (verified at crates/rocm-core/src/lib.rs:5899-5910), so the silent-skip hole is not reopened — but its remediation introduced a new defect (blocking #2). Verified: ran cargo fmt --all -- --check (clean, exit 0); read the atomic-write helper and all three staging implementations, load's real contract, the daemon stop path end to end, the renumbered feature file against feature_naming.rs, and the Cargo manifest consolidation (windows-sys Win32_Storage_FileSystem correctly moved into rocm-core and dropped from both consumers). No prompt-injection content found. No internal leak: the bare ticket identifiers appearing in commit messages and comments are explicitly permitted by AGENTS.md §2 ("do not flag them") and the base branch already carries the same convention, so this is a project-sanctioned choice, not a leak. CI at this head: 1 failure, 2 skipped, 17 success — no per-job breakdown is available, so this failure cannot be attributed to a named job. Blocking: 3 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

crates/rocm-core/src/lib.rs:7828 — a_service_record_is_published_atomically does not pin the change its commit exists to pin. Commit 074b37b states the three helper tests left a gap because "nothing drove ManagedServiceRecord::write through it, so reverting that call site to a plain fs::write would not have been caught." This test does not catch it either. It performs two sequential, uncontended writes, then asserts (a) no .tmp- sibling remains in the services directory and (b) the manifest re-parses with status == "stopped". A plain fs::write creates no scratch file at all, so (a) is vacuously true, and it writes the final bytes correctly absent concurrency or a crash, so (b) holds. Revert ManagedServiceRecord::write to fs::write and this test stays green — the call-site wiring is still unpinned, and the commit's claim is false. Fix: assert something only the atomic path can satisfy. The clean discriminator is a failed publish: with write_file_atomically, a publish that fails leaves the original manifest intact and readable; with fs::write, the original is already truncated and destroyed. Drive ManagedServiceRecord::write at a destination whose publish cannot succeed (the same non-empty-directory trick write_file_atomically_cleans_up_when_publishing_fails already uses) and assert the pre-existing manifest still parses. Alternatively expose the existing before_publish seam to the record-level write and assert a scratch sibling exists mid-write.

apps/rocm/src/main.rs:15596-15606 (remedy text at 15966-15971) — the abort introduced by the objection-2 fix tells the operator to do something impossible. The new Err branch records service_id: "rocmd (runtime state unreadable)" with remedy: StopFailureRemedy::StopTheDaemon, whose printed advice is "kill that pid, then re-run uninstall." No pid was ever extracted on this path — the state file did not parse — so no pid is known or displayed, unlike the sibling daemon_identity_unverified failure which does carry pid {daemon_pid}. The message never names runtime-state.json, so the one action that actually clears the failure (repair or delete that file) is never communicated, and rocm uninstall has no --force or other escape hatch. A user with a corrupt or permission-denied runtime-state.json gets a hard abort plus misdirection. This is exactly the remediation-introduced defect the original code did not have: previously the path was silently skipped (wrong, but not a dead end). Fix: give this failure a remedy that names the file — either reuse StopFailureRemedy::RepairTheRecord, whose text already says "repair or delete the file" (15979-15984), or add a variant printing the concrete runtime-state.json path. The existing test uninstall_refuses_when_the_daemon_runtime_state_cannot_be_read only asserts remedy == StopTheDaemon, so it locks in the wrong text; update it to assert the message names the state file.

apps/rocm/src/main.rs:15544-15571 — a ~25-line design rationale is attached to the wrong function. The rustdoc block beginning "Stop the background helper before uninstall stops the services it supervises" — covering the rocmd respawn race, the pid-recycling hazard, why a whole tree is signalled with force, and the /proc-less degradation — sits immediately above fn daemon_identity_unverified (15572), a three-line constructor for a failure struct. fn stop_background_helper_before_uninstall (15582), the function that rationale actually describes and the one carrying the destructive behaviour, has no doc comment at all. Rustdoc will publish the whole safety argument on the wrong symbol. In a PR whose value rests on making a destructive gate reviewable, this is a documentation-correctness defect, not a nit, and it is objectively wrong rather than a matter of taste. Fix: move the block down to sit directly above fn stop_background_helper_before_uninstall, and leave daemon_identity_unverified with just the one-line sentence already written for it ("The failure recorded when the background helper is live but cannot be proven to be rocmd...").

Non-blocking

• apps/rocm/src/main.rs:15628-15662 — prior objection 1 is still live on Windows and macOS: with no readable start-time, identity_state returns Matches from its legacy arm and the pid is tree-killed with force: true; documented and consistent with existing managed-service kills, but Windows is a supported platform, so consider recording an alternative Windows identity (creation time via GetProcessTimes) as follow-up.
• apps/rocm/src/therock.rs:2971-3022, apps/rocmd/src/lib.rs:1053-1128 — only the publish step was consolidated; three near-identical staging implementations remain, and the two outside rocm-core lack the sync_all the new helper documents as its guarantee. Commit 911a399's subject "one atomic-write helper" overstates its own (accurate) body and cost a reviewer on this pass a false blocking call — a one-line comment on each local write_file_atomically saying only the publish step is shared, and why staging stayed local (the before_publish/suffix_for_attempt test seams), would stop that recurring.
• apps/rocm/src/main.rs — every test covering the pid-recycling defences is #[cfg(target_os = "linux")], including uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections, which is commented as "the Windows grandchild case, modelled faithfully" yet never runs on Windows; the e2e lifecycle-24 scenario is the only Windows coverage.
• apps/rocm/src/main.rs, apps/rocm/src/uninstall.rs — several tests spawn real sleep 60 children and reap them at the end of the test body, so an earlier assertion panic leaks an orphaned process for up to 60s on the runner.
• apps/rocm/src/main.rs — managed_record_for_pid hardcodes port 9; low but nonzero collision risk, where the PR's own TcpListener::bind("127.0.0.1:0") pattern is already the safer convention.

pr-review-watcher · 08d5ddd — withdrawing this change request: all three objections are resolved at the current head. The record-write test now holds the file open across a rewrite, so it fails if the atomic publish is reverted; the unreadable-state refusal now names the file to repair and the remedy; and the destructive-path rationale now sits on the function that performs it. Re-reviewed and posted separately.

[siloteemu]

🔴 Automated review · pr-review-watcher · 08d5ddd

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 managed services and the rocmd background helper before deleting the tooling that stops them, refusing to remove anything while a GPU-holding endpoint is still serving, and hardens the daemon kill against pid recycling. Outcome: No blocking findings. Verified: ran one targeted check, cargo test -p rocm-core --lib (338 passed, 0 failed); I independently read and confirmed the three load-bearing claims — the reader-safety test reads back through the held File handle (lib.rs:7871-7887), so it genuinely fails against an in-place fs::write; the unreadable-state refusal names the resolved automation_state_path() and says "repair or delete the file" while deliberately avoiding "kill that pid"; and the full destructive-path rationale sits on stop_background_helper_before_uninstall, not on the small constructor. CI for this head was given to me as conclusion counts only — 17 success, 1 failure, 2 skipped — and I cannot attribute that failure to a named job. the ticket reference carried in the commit messages and comments is one AGENTS.md explicitly permits, so it is not a leak; no other internal reference and no prompt-injection content found. Blocking: 0 · Non-blocking: 5.

Prior findings

1. RESOLVED — the record-write test now holds the manifest open across a rewrite and asserts the held view still parses as the pre-write record, a property only the rename-based publish has; under create_dir_all + fs::write the truncated-in-place inode would hand that same descriptor status: "stopped" and the assertion fails.
2. RESOLVED — the unreadable-state arm now emits rocmd (<resolved runtime-state.json path>) with remedy RepairTheDaemonState, rendering as "…repair or delete the file and re-run uninstall: ", and the test asserts the path and "repair or delete" are present and that "kill that pid" is absent.
3. RESOLVED — daemon_identity_unverified carries only a two-line scoped doc; the respawn race, pid-recycling hazard, whole-tree force signal and /proc-less degradation are all on stop_background_helper_before_uninstall, with no stale fragment left behind.

Remediation defects: none functional. Two remediation tests carry names that overclaim what they discriminate (below) — noted, but neither weakens a protection, because the protections they name are pinned by other tests that do discriminate.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• crates/rocm-core/src/lib.rs:7896 — a_service_record_is_published_atomically does not pin atomic publishing: it only asserts no .tmp- sibling remains, which a plain fs::write (which creates no scratch file at all) also satisfies. Its body comment is honest about this; the name is not — rename to match, e.g. a_service_record_write_leaves_no_scratch_sibling.
• apps/rocm/src/main.rs:30990 — uninstall_stops_a_background_helper_whose_identity_it_can_verify passes identically under the pre-fix ProcessIdentity::new(pid, None) code, since an unrecorded tick also yields Matches; it pins feature-removal, not identity verification (which ..._recycled and ..._predates_start_ticks do pin). Drop the "whose identity it can verify" framing.
• Recurrence risk (focus c): this PR already ate one false blocking finding from a shared helper name implying more than the code did, and fixed it with per-definition docs — but the same trap now lives in these two test names. A reviewer checking revert-pinning reads the name first; a one-line "this pins X, not Y" comment on each is the cheap fix.
• apps/rocm/src/main.rs:15586 — process_start_ticks(state.daemon_pid) is read twice in quick succession (once for unverifiable_pre_upgrade_record, once inside identity_state); harmless since both reads gate toward the conservative outcome, but a single read passed down would be clearer.
• apps/rocm/src/main.rs:15602 — a manually deleted runtime-state.json with a live rocmd still yields Ok(None) and silently skips the daemon stop; defensible, but unlike the /proc-less case this residual gap is not stated in the doc comment that carefully enumerates the other two outcomes.

[fredespi]

Round 5 addressed in df64d9a. All five non-blocking items taken, none declined.

Test names that overclaim (items 1, 2 and the recurrence note). Both calls are right, and in both cases the body comment being honest was not a defence — the name is what a reviewer checking revert-pinning reads first.

• a_service_record_is_published_atomically → a_service_record_write_leaves_no_scratch_sibling. Its comment now says what it pins (scratch hygiene), what it does not (atomic publishing — a plain fs::write creates no scratch file and passes too), and where the real property lives: a_service_record_write_never_shows_a_reader_a_truncated_manifest, which does fail against an in-place write. Kept rather than deleted, for the reason you gave: a leaked .tmp- sibling is an unparseable record to unreadable_service_manifests, so it would abort an uninstall.
• uninstall_stops_a_background_helper_whose_identity_it_can_verify → uninstall_stops_a_live_background_helper. Its comment states it pins feature-removal only, that an unrecorded tick also yields Matches so it passes against the pre-fix ProcessIdentity::new(pid, None), and names the two tests that do discriminate (uninstall_never_kills_a_daemon_pid_that_was_recycled, uninstall_never_kills_a_daemon_pid_from_a_state_file_that_predates_start_ticks).

Double process_start_ticks read (item 4). Taken — and on a closer look it is marginally worse than harmless, which is why it got a commit of its own rather than a local variable. The two readings can disagree: a PID whose /proc entry is momentarily unreadable answers None to one read and Some to the other, and that pair is exactly what selects between "record predates the field, leave it alone and abort" and the legacy best-effort Matches arm that force-signals a whole process tree. Both orderings land conservatively today only because of which read feeds which branch — nothing states it.

So the reading is now taken once and passed down. rocm_core::identity_state_with_observed(id, observed) is the same classification against a caller-supplied reading; identity_state keeps its signature and behaviour, delegating with a reading of its own, so no other call site changes.

That new seam is itself revert-pinned by identity_state_with_observed_classifies_the_reading_it_was_given: the live child's real start-time matches its record, so an implementation that quietly re-reads /proc answers Matches where the contract requires Recycled. Verified by reverting the body to a re-read — the test fails; restored, it passes.

Missing runtime-state.json (item 5). Agreed that the doc enumerating the other outcomes made the omission read as an oversight. The gap is now stated beside them: a missing state file is taken at face value as "no daemon", so hand-deleting it while rocmd is live skips the daemon stop silently. Deliberate — there is no pid to verify or signal without it — but the contrast is the point, and the doc now draws it: an unreadable state file aborts, an absent one proceeds.

One defect found while verifying, worth naming. The first version of the new test asserted before killing and reaping its child. That child holds the test harness's stdout pipe open, so the failing case did not fail — it hung the whole suite on the pipe until timeout. A test that hangs on failure is worse than the bug it guards. It now collects all three verdicts, cleans up unconditionally, and only then asserts.

Verification (Linux container, pinned 1.96.0 toolchain, isolated cargo/target volumes):

• cargo fmt --all --check clean; cargo clippy --workspace --all-targets -- -D warnings clean.
• cargo test --workspace --all-targets: rocm 626, rocm-core 339, rocmd 122, e2e-cucumber lib 113, feature_naming 4 — all pass.
• python scripts/smoke_local.py → smoke: ok.
• @lifecycle lane (E2E_INCLUDE_LIFECYCLE=1 E2E_ONLY_LIFECYCLE=1 cargo xtask e2e): lifecycle-10 - Linux - uninstall stops the local server it manages passes, all 8 steps — the EAI-8014 contract scenario over the daemon path this change touches.
• Two lifecycle scenarios (lifecycle-linux-install-signed-keypath, lifecycle-linux-reinstall-purges-stale) and ten rocm-engine-lemonade child_runtime_dir tests fail in that container. Both sets fail identically on the parent commit 08d5ddd4, are green in CI, and touch no code in this diff — container filesystem/ownership gaps, not regressions.

Refs: EAI-8014

[fredespi]

I found three correctness issues that should be fixed before merge, detailed inline below. Two let uninstall remove the recovery tooling while a managed endpoint can still be serving; the other can terminate an unrelated process tree from stale daemon state. This is submitted as COMMENT rather than REQUEST_CHANGES only because GitHub treats the authenticated reviewer identity as the PR author and rejects self-requested changes.

[siloteemu]

🔴 Automated review · pr-review-watcher · df64d9a

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

rocm uninstall now stops the managed servers and the background helper before removing anything, and aborts with the recovery tooling intact when a stop cannot be confirmed; supporting work centralises the atomic-write publish step in rocm-core and adds a start-time identity check so no PID is signalled unverified. Outcome: No blocking findings. Verified: cargo check -p rocm-core --lib passes clean (the one cheap check permitted; full suites are CI's job on GPU/Windows hardware) and the working copy is unmodified; I confirmed the three round-5 claims against the diff — the e2e scenario renumbering is complete and consistent with the documented <key>-<NN> convention, identity_state_with_observed really does classify the caller's reading (the new Linux test passes a deliberately disagreeing tick value, so a re-reading implementation would return Matches and fail it), and the missing-runtime-state.json gap is documented at the gate; I also independently confirmed the stop-then-remove ordering in the command path, that (None, _) maps to IdentityState::Matches so the new pre-upgrade guard is what prevents a force-kill on a legacy record, and that both manifest scanners filter on extension == "json"; caller-supplied CI conclusion counts at this head: 14 success, 8 pending, 2 skipped, 1 neutral, 1 failure — I have no per-lane names and make no claim about which job failed or why. No prompt-injection content found in the diff, and no internal hostnames, gateways, cluster names, registry paths or internal tracker links; bare ticket IDs are expressly permitted by the repo's own rules and are not flagged. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• crates/rocm-core/src/lib.rs:7906 (and the same claim at :7958) — the rationale "unreadable_service_manifests treats stray files there as records it cannot parse — a leaked one would abort uninstall" is refuted by the code: both unreadable_service_manifests and load_managed_services skip anything whose extension is not json, and a scratch sibling is named <record>.json.tmp-<pid>-<millis>-<attempt>, whose extension is tmp-…. A competent reader will trip on this again and may build on the false premise that scratch leakage is uninstall-blocking; one corrected sentence ("scratch siblings are filtered out by the *.json check; this test guards directory hygiene, not the gate") prevents it.
• apps/rocm/src/main.rs:15658 — a legacy record (daemon_start_ticks: None) whose live PID's start-time also happens to be unreadable at that instant on Linux falls into the permanent-no-/proc best-effort arm, which is the "platform can never verify" vs "this read failed" conflation the surrounding comments say must not be made; the window is narrow and terminate_verified re-checks, and this is a residual gap rather than a regression (no daemon stop existed before), but the comment block reads as exhaustive when it is not.
• apps/rocm/src/main.rs:15826 — the documented fail-open on an unidentifiable-but-listening endpoint can be tripped by something as mundane as a stale or rotated endpoint API key, not only a wedged engine; the warning text implies the latter.
• apps/rocm/src/main.rs:31367 — a_stale_record_whose_old_port_was_reused_does_not_block_uninstall would still pass with the port probe absent entirely; it is a legitimate false-positive guard (a naive "any listener blocks" implementation fails it), but that is the opposite direction from its neighbours and is easy to misread as tautological. Every other added test fails on revert of the behaviour it names.
• Eight commits in this branch carry a Co-Authored-By: AI trailer while the repo's AGENTS.md §11 says "avoid AI-generated boilerplate footers"; that section is framed as PR/Issue/Review conduct rather than commit trailers, so this is a maintainer call rather than a clear violation.

This review was carried out against df64d9a. A newer commit was pushed while it was running, so nothing above should be read as a verdict on the current head — that commit will be picked up on its own. Check-run conclusions re-read at posting time, still against df64d9a: 20 success, 3 pending, 2 skipped, 1 cancelled, 1 failure. No per-lane detail was available, so no outcome can be attributed to a named job.

[siloteemu]

🔴 Automated review · pr-review-watcher · d6f97c5

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 the managed servers and the background helper before it removes the tooling that stops them, aborting with nothing removed when any stop is unconfirmed. Outcome: Needs work — the ordering guarantee itself is sound and well covered, but the two process-based tests that claim to prove a server actually dies are vacuous, and two paths on the destructive side do more, or less, than intended. Verified: the added write_file_atomically/ReplaceFileW consolidation is byte-for-byte behaviour-preserving and daemon_start_ticks is serde(default) so old and new state files both degrade safely (cargo check -p rocm-core --tests clean in a scratch copy); a scratch-copy mutation that inverts the removes_recovery_tooling condition leaves all six uninstall::tests green (runtime 0.09s → 60.05s), confirming the finding below; the remedy strings, the StopFailureRemedy variants and the doc-comment claims about AutomationRuntimeState::load, load_managed_services and the absent services remove were each checked against source and hold; leak scan over the diff is clean. The full test suite, the e2e lanes and every Windows-only path were not run here. Blocking: 3 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

1. apps/rocm/src/uninstall.rs:310 and :387 — the two tests that claim to prove uninstall kills the server cannot fail for that defect.
Both tests spawn a real sleep 60 stand-in, run the stop-and-remove path, then do let _ = child.wait(); / let _ = server.wait(); before asserting !process_is_running(pid). wait() is unbounded, so if uninstall never kills the child the call simply blocks until sleep 60 exits on its own — after which the process is genuinely gone and the assertion passes. The tests therefore measure "did it eventually die", which sleep 60 guarantees, not "did uninstall kill it". Confirmed empirically: inverting the one condition at uninstall.rs:53 (so a run that does remove recovery tooling silently skips the stop pass — precisely the defect this PR fixes) leaves all six tests reporting ok; the only symptom is the suite taking 60s instead of 0.09s. This is worse than an absent test, because the_uninstall_command_itself_stops_a_managed_server_before_removing_anything carries a comment asserting that inlining the old remove-first loop "fails here even though every narrower test still passes" — true of the ordering half, false of the stop half, so the comment certifies coverage that does not exist and a future reader will trust it again. Fix: replace both wait() calls with a bounded try_wait deadline that asserts the exit, and keep the kill-and-reap fallback — the correct pattern already exists in this PR at tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs:1279-1298. Correct the comment at uninstall.rs:328 to say what the test actually pins.

2. apps/rocm/src/main.rs:15753 and :15924 — a run that is already guaranteed to abort still stops every managed service first.
stop_background_helper_before_uninstall returns () and only records into report, so an unreadable runtime-state file (:15628) or an unverifiable daemon identity (:15699-15709) falls straight through into the full per-service stop loop. Every live service is then killed, and stop_internal_managed_service drops each one's endpoint key on a confirmed stop — irreversible, as the abort message itself admits ("a publicly bound one has to be served again with --allow-public-bind to return"). Only then does the gate abort with nothing removed. So the failure the function documents at length as the reason to leave the daemon alone — it can respawn what was just stopped — is the same failure that authorises stopping everything else for no gain. unreadable_service_manifests (:15924) has the identical shape and is worse: a manifest that cannot be parsed may describe a live GPU-holding server, per its own doc comment, yet it is collected dead last, after every other service is already down. This is the partial-failure case that leaves the machine worse off than either completing or aborting. Fix: after stop_background_helper_before_uninstall returns, if !report.failed.is_empty() { return Ok(report); }; and hoist the unreadable_service_manifests check above the stop loop so an unparseable record aborts before anything is touched.

3. apps/rocm/src/main.rs:15851-15853 — a live server that lists no models is silently read as somebody else's, and removal proceeds.
For a record already recorded stopped, the gate asks managed_service_endpoint_model_ready whether the listener on the recorded port is this record's own engine. openai_models_endpoint_has_model (crates/rocm-core/src/lib.rs:889-891, pre-existing) returns Ok(false) whenever the model list is empty, before any model-name comparison — so an engine that is up but has not yet populated /v1/models (loading, or mid-unload) is indistinguishable from an unrelated service on a recycled port. The caller treats Ok(false) as the latter and continues: no failure, no warning, tooling removed while that endpoint keeps the port and the GPU. That is the original defect reached through the gate meant to prevent it. The asymmetry is what makes this a defect rather than a tradeoff: the adjacent "listening but unidentifiable" case at :15883-15890 is a deliberate, documented fail-open that at least prints a warning to stderr, while this one is silent. Minimal fix: route an empty model list to that same warned fail-open path instead of the silent not-ours branch, so the operator is told what was seen; a three-way result from openai_models_endpoint_has_model (matched / not matched / answered-but-empty) would be the cleaner version.

Non-blocking

• apps/rocm/src/uninstall.rs:53-60 — no test drives uninstall_with_paths with a cache-only plan and asserts a live server is left running; only the pure plan_removes_recovery_tooling predicate is covered, which is a materially weaker claim for a branch whose job is to not stop more than asked.
• apps/rocm/src/uninstall.rs:150-179, 278-281, 348-351 — temp roots and spawned sleep 60 children are cleaned up only on the happy path, so a panicking assertion leaks both; this PR already added a Drop guard for the e2e side at lifecycle_steps.rs:180-188, worth the same here.
• apps/rocm/src/main.rs:15854-15906 — the 401/403 check costs a second full round-trip via endpoint_refused_authorization; matching on the original response's status would save it and make the fail-open path easier to audit.
• apps/rocm/src/main.rs:13359 — loopback_tcp_port_is_reachable is now also called with a record's own, possibly non-loopback, host, since probe_hosts passes unrecognised hosts through unchanged; the implementation is generic and correct, only the name overpromises.
• 21 commits for one logical change, including a rustfmt fixup for "the two lines the last push missed"; squashing the fixups would leave a history that shows what changed.

[siloteemu]

🔴 Automated review · pr-review-watcher · d6f97c5

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. This review is comments only.

Three blocking findings. The ordering guarantee this PR is built around is sound and well covered; these are about the two tests that claim to prove a server dies, and two paths on the destructive side.

1. The two process-based tests cannot fail for the defect they name.
Both spawn a real long-lived stand-in process, run the stop-and-remove path, then call an unbounded wait() before asserting the process is gone. If uninstall never kills the child, wait() simply blocks until the stand-in exits on its own — after which the assertion passes. They measure "did it eventually die", which the stand-in guarantees, not "did uninstall kill it". Confirmed by mutation in a scratch copy: inverting the single condition that decides whether the stop pass runs — exactly the defect this PR fixes — leaves all six tests green, the only symptom being the suite taking 60s instead of 0.09s. This is worse than having no test, because the surrounding comment asserts that inlining the old remove-first loop "fails here even though every narrower test still passes" — true of the ordering half, false of the stop half — so the comment certifies coverage that does not exist. Fix: replace both wait() calls with a bounded polling deadline that asserts the exit, keeping a kill-and-reap fallback. The correct pattern already exists in this PR's own end-to-end step code. Please also correct the comment to say what the test actually pins.

2. A run already guaranteed to abort still stops every managed service first.
The helper that stops the background process records its failures into the report but returns nothing, so an unreadable runtime-state file or an unverifiable daemon identity falls through into the full per-service stop loop. Every live service is then stopped, and each confirmed stop drops that service's endpoint key — irreversible, as the abort message itself says a publicly bound one has to be served again with the explicit flag to come back. Only afterwards does the gate abort with nothing removed. So the failure documented at length as the reason to leave the background helper alone is the same failure that authorises stopping everything else for no gain. The unreadable-manifest check has the same shape and is worse, since a manifest that cannot be parsed may describe a live GPU-holding server yet is collected last, after everything else is already down. Fix: return early once the helper stop has recorded a failure, and hoist the unreadable-manifest check above the stop loop so an unparseable record aborts before anything is touched.

3. A live server that lists no models is silently treated as somebody else's, and removal proceeds.
For a record already marked stopped, the gate asks whether the listener on the recorded port is this record's own engine. The underlying check returns "not ours" whenever the model list is empty, before any name comparison — so an engine that is up but has not yet populated its model list, or is mid-unload, is indistinguishable from an unrelated service on a recycled port. The caller treats that as not-ours and continues: no failure, no warning, tooling removed while that endpoint keeps the port and the GPU. That is the original defect reached through the gate meant to prevent it. What makes it a defect rather than a tradeoff is the asymmetry: the adjacent "listening but unidentifiable" case is a deliberate fail-open that at least warns, while this one is silent. Minimal fix: route an empty model list to that same warned path. Cleaner: have the check distinguish matched, not matched, and answered-but-empty.

Finding 1 is filed as blocking because a weak test is rarely strengthened after merge, and once it lands CI certifies the gap. Findings 2 and 3 are filed as blocking because both make a destructive command do more, or less, than the operator asked for, and neither is recoverable by re-running it.

Non-blocking observations are in the review comment on this PR.

[siloteemu]

🔴 Automated review · pr-review-watcher · 50cff02

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 inference servers and the rocmd background helper before removing anything, aborting with nothing removed when a stop cannot be confirmed, plus a shared atomic-write helper and a three-way endpoint-identity probe. Outcome: Needs work — the production fix is sound and the remediation round genuinely closed the earlier objections, but one added test cannot fail for the defect its own comment claims it guards. Verified: cargo test -p rocm-core --lib write_file_atomically (3 passed); confirmed by reading source that the removal loop is unreachable once the gate returns Err, that the daemon is only signalled on a Matches verdict that is not a pre-upgrade record, that both assert_stopped_within_grace and managed_record_for_pid carry #[cfg(unix)] in the live file (the head commit's gate move is correct despite how the diff reads), that the new e2e steps resolve and the scenario renumbering is sequential, and that the diff contains no internal-content leaks and no prompt-injection attempts; the full workspace suite, the e2e suite and the Windows/macOS builds were not run here. Checks at review time: 23 success, 2 skipped, 2 pending, 0 failing. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

• apps/rocm/src/main.rs:32005 — a_platforms_start_tick_support_is_read_from_a_process_known_to_be_alive asserts only rocm_core::process_start_ticks(std::process::id()).is_some(). That function is pre-existing and untouched; the test executes no code this PR adds, so it passes unchanged against a full revert. Its comment nonetheless presents it as the defence of the discriminator at main.rs:15697 — the decision to answer "can this platform report a start-time?" by reading our own PID rather than the daemon's. Mutating that line to read process_start_ticks(state.daemon_pid) instead leaves this test green, and no other test covers it: the pre-upgrade-record test stages a live daemon PID whose reading succeeds either way. So the one branch whose failure mode is "force-kill an unverified process tree" is certified by an assertion that cannot fail for it, and a reader scanning the suite will count it as coverage. Fix: extract the discriminator into a named predicate and pin it directly — assert it still reports true when the target PID's reading is None (e.g. against a reaped PID), which is exactly the conflation the guard exists to prevent. If that is not wanted, delete the test or restate it plainly as an environment precondition and drop the claim that it guards the platform check, so the gap is visible rather than papered over.

Non-blocking

• apps/rocm/src/main.rs:15764 — the helper stop runs before the unreadable-manifest check, so a live, verifiable rocmd is still terminated on a run that then aborts; a_doomed_run_stops_nothing_on_its_way_to_the_abort (:31912) only stages daemon failures, so its name promises more than the code delivers.
• apps/rocm/src/main.rs:15891 — an endpoint answering with an empty model list, or unparseably, still fails open after a stderr warning; the comment calls this "the original defect" and then proceeds, which is defensible only because uninstall has no override flag — an explicit --force would let this branch fail closed.
• apps/rocm/src/main.rs:31792 — an_ipv6_only_wildcard_engine_is_not_waved_through returns silently when the host has no IPv6, passing with nothing asserted; make the skip visible, or rely on the deterministic probe_hosts unit test that already pins both families.
• apps/rocm/src/main.rs:9489 — write_active_runtime_marker still uses its own delete-then-rename scheme rather than the new shared helper, so the "one atomic-write helper" consolidation is incomplete and this call site keeps the weaker Windows behaviour.
• crates/rocm-core/src/lib.rs:7825 — write_file_atomically replaces a symlink at the destination with a regular file instead of writing through it, a change from the fs::write it replaces; the doc comment records the permission and durability caveats but not this one.

[nowycondro]

🔴 Automated review · pr-review-watcher · c512701

Review — needs work

Full review of the whole change (base 79f6e08c…head c512701d), split across coupled file groups plus the four source-reading passes (agent-instruction adherence, history, prior-changes, code comments), run blind with no access to this PR's description, ticket, or review threads until after findings were settled.
Title/commits name EAI-8014 ("uninstall reports success while leaving a managed server running and deletes the tooling that would stop it"); the change implements a stop-before-remove gate for that defect. No separate ticket text was retrievable from here (no tracker tool available to this reviewer beyond the PR's own GitHub text).

Blocking

• PR description overclaims the relaunch-advice consolidation — crates/rocm-engine-protocol/src/lib.rs (relaunch_command/relaunch_command_for), apps/rocm/src/service_launch.rs:479, apps/rocmd/src/common.rs:334,343, apps/rocm/src/uninstall_gate.rs:799
  The description states the shared builder is "used by the gate, rocm serve and rocmd, and now names a runnable command with the model and port." Only the uninstall gate's call (relaunch_command_for, fed record.model_ref/record.engine/record.port) actually fills those in. rocm serve's and rocmd's own refusal paths (ensure_public_service_has_endpoint_key, duplicated in both crates) still call the unfilled relaunch_command(host, bool), which prints literal <model>/<port> placeholder tokens — even though the caller at service_launch.rs:479 already holds the full ManagedServiceRecord and simply doesn't thread record.model_ref/record.port through. The description's claim is true for one of the three named consumers, not all three.
  Confidence 85 · architectural · Fix: either pass the record's model/port into ensure_public_service_has_endpoint_key in both crates (the data is already in scope at every call site), or narrow the description to say only the gate's advice is filled in today.

• 41 of this branch's 84 commits carry AI co-author footers, against this repo's own enforced policy — commit history (git log 79f6e08c..c512701d --format='%b' | grep -c "Co-Authored-By" → 41)
  The repo's AGENTS.md (§11, "PR/Issue/Review Conduct") states "avoid AI-generated boilerplate footers" as part of its blocking upstream-contribution workflow. This branch's commits carry Co-Authored-By: Claude <noreply@anthropic.com> and Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> footers in roughly half of its commits. Independently re-verified by direct count, not taken from a sub-pass's word.
  Confidence 90 · mechanical · Fix: squash/rebase to drop the footers before merge, consistent with the project's own documented contributor norm.

• Sphinx docs build (-W) shows CANCELLED, not SUCCESS, for the reviewed head commit — CI (statusCheckRollup, re-checked live immediately before filing this review; same result both times)
  All other CI jobs on this commit are green, but this one job completed with conclusion CANCELLED (20-minute run, not an instant supersede-by-push cancellation) while its sibling jobs in the same workflow run succeeded. This PR edits README.md, which AGENTS.md itself documents as {include}d into the Sphinx-built docs site — so this is a check relevant to this PR's own changes, not an unrelated job. Per the review method's rule to take CI status at face value rather than assume: this has not been demonstrated green on the current head.
  Confidence 75 · mechanical · Fix: re-run the job (or have a maintainer confirm it is non-required/known-flaky) and get it to a clean conclusion before merge.

Non-blocking

• helper_decision (apps/rocm/src/uninstall_gate.rs:283-327) is the one production call site using the pre-read-then-compare identity_state_with_observed instead of the race-free identity_state used by every other production call site in this PR (apps/rocmd/src/service.rs:316,1550). Two independent passes flagged this. identity_state's own doc explains the race-free design precisely so callers don't have to get this ordering right themselves; helper_decision's inline comment argues a single reading was needed, but nothing else in the function consumes observed_start_ticks a second time, so the narrower recycle-window this reopens (a handful of instructions, bounded similarly to identity_state's own internal window) appears to buy nothing over just calling identity_state.
  apps/rocm/src/uninstall_gate.rs:283-327. Confidence 72 · architectural · Fix: call rocm_core::identity_state(&identity) directly, consistent with the rest of the PR, unless there's a reason to pre-read documented elsewhere.

• rocm-core's new write_file_atomically doc comment makes a factually inaccurate blanket claim. It states the lack of permission-carryover is acceptable "only because every current caller writes non-sensitive state," but RocmCliConfig::save (crates/rocm-core/src/config.rs:559) persists this exact helper's output for DashboardDaemonConfig.token, a bearer secret required for TCP dashboard auth, with no set_permissions/mode call anywhere on that path. This is pre-existing behavior (the call site itself is an unchanged rename from the prior write_atomic, not newly exposed by this PR), but the new comment's claim is new and wrong.
  crates/rocm-core/src/lib.rs:451, crates/rocm-core/src/config.rs:302-304,559. Confidence 78 · mechanical (doc accuracy) · Fix: correct the comment, or harden the config file's permissions while consolidating its write path.

• Two e2e scenarios silently assert nothing on hosts without IPv6 loopback, a condition their own comments say happens in some CI containers: an_ipv6_only_wildcard_engine_is_not_waved_through and a_stranger_on_one_loopback_family_does_not_hide_the_record_serving_on_the_other (apps/rocm/src/main.rs, diff lines ~18354/18421) return early via eprintln! + return when bind_loopback_pair can't get an IPv6 socket, which the Rust test harness reports as a pass. These cover the dual-stack wildcard-probe fail-closed path — one of this PR's safety-critical branches. Partially mitigated by a companion pure unit test (every_wildcard_bind_spelling_is_probed_on_both_loopback_families) that does pin the host-expansion logic unconditionally.
  Confidence 70 · mechanical/architectural · Fix: skip explicitly with #[ignore]/a documented reason so a green run is distinguishable from a skip, rather than a silent no-assertion pass.

• lifecycle-26 (Linux, helper-only stop) never asserts the rocm/rocmd binaries or service records were actually removed, unlike its sibling scenarios lifecycle-10/lifecycle-24. Its own comment explains it's deliberately narrow ("pinned alone... so it is the helper stop being proven and not the service stop"), but the result is that no scenario covers "live helper only, no managed service, full teardown, binaries actually gone" — a regression specific to that combination would not be caught by any e2e scenario.
  tests/e2e-cucumber/features/install_lifecycle.feature:343-355. Confidence 70 · logic (test coverage) · Fix: add the binaries/manifest-gone assertions already used by lifecycle-10/-24.

• A new unit test exercises code this PR does not touch. a_vllm_shaped_record_with_a_distinct_engine_pid_carries_engine_ticks (apps/rocm/src/main.rs) only calls ManagedServiceRecord::refresh_from_engine_state() and asserts on engine_pid/engine_start_ticks; crates/rocm-core/src/service_record.rs's refresh_from_engine_state is unchanged by this diff. It would pass identically with every production change in this PR reverted. Plausibly intended as foundational coverage for a value the new gate logic reads (recorded_pid_identities consumes engine_start_ticks), but as written it doesn't pin anything this PR introduced.
  Confidence 70 · logic (test relevance) · Fix: none required, but if kept, a comment tying it to the gate's dependence on this field would avoid it reading as a straggler.

Decisions for the author

• PID + OS-reported start-time as process identity is inherently probabilistic, not absolute — tradeoff. The gate (and the pre-existing EAI-7410 machinery it reuses) compares kernel/clock-reported process-creation timestamps to tell a recycled PID from the recorded one. On Linux this is /proc clock-tick resolution; on Windows, GetProcessTimes' practical clock granularity is coarser than the 100ns units it's stored in, so two processes launched in rapid succession (e.g. a crash-loop respawn) could theoretically collide. This is a reasonable, already-well-reasoned design choice given the constraints (no cooperating daemon-side token at kill time), but it is a probabilistic guarantee, not a certain one, and that boundary isn't called out anywhere in the new code's docs. Confirm this residual risk is accepted as-is.

• This PR's size and commit shape — tradeoff/suggestion. 29 files, ~8,800 insertions, 84 commits (many Merge origin/main into ..., plus an extended "fix one fail-open class, find another, fix that" iteration trail) for what is conceptually one feature. AGENTS.md §11 asks PRs to "keep each PR scoped to one logical change" and to avoid "a trail of incremental noise." The underlying feature is coherent and the iteration produced unusually careful fail-closed coverage, but the history as it stands would read more easily post-squash/rebase; worth confirming this is the intended shape to merge versus cleaning up before landing.

Positive signals

• apps/rocm/src/uninstall_gate.rs's stop-before-remove gate is exceptionally carefully reasoned: every fail-open and fail-closed branch is pulled out into an enum with an exhaustive match (StopFailureRemedy, StoppedRecordVerdict, DaemonIdentityOutcome) specifically so a future added variant cannot silently default to the wrong (permissive) side, and the removal gate (uninstall_removal_gate) is called and its error propagated with ? strictly before any path is removed (apps/rocm/src/uninstall.rs:154-167) — traced this call sequence directly and confirmed no path lets removal proceed on an unconfirmed stop.
• The wildcard-bind probing correctly treats 0.0.0.0/::/[::]/*/empty as "probe both 127.0.0.1 and ::1, block if either answers" rather than connecting to the wildcard address literally, closing a real gap (a ::-bound server with IPV6_V6ONLY=1 refuses 127.0.0.1 and would otherwise be missed by a single-family probe).
• host_port_resolves is correctly split out from loopback_tcp_port_is_reachable so "nothing is listening" (safe to proceed) and "this name doesn't resolve at all" (unknown — must disclose, not silently proceed) are told apart, and the disclosure is returned as a Vec<String> on the report rather than only eprintln!'d, so it's independently assertable by tests.
• rocm-core's new write_file_atomically uses OpenOptions::create_new to reserve its scratch filename (not a check-then-create), correctly avoiding a TOCTOU the prior write_atomic didn't need to worry about with its deterministic name.
• Scenario IDs in tests/e2e-cucumber/features/install_lifecycle.feature are fully consistent after the renumbering (lifecycle-10..22 → 10-11..23 plus new 10/24-29), with no collisions, and every new assertion string was cross-checked against the literal text the gate actually produces.

Deployment notes

• Stopping a managed service as part of uninstall now drops that service's endpoint API key permanently (it cannot be re-minted), so any publicly-bound or --require-api-key server stopped by this path must be relaunched with a new key — this is by design and documented in the abort/complete messages, but is a behavior change worth calling out to anyone relying on the old "uninstall always fully removes, regardless of running state" semantics.
• rocm uninstall can now exit non-zero and remove nothing where it previously always proceeded — any automation/scripts that assumed uninstall always succeeds when invoked with --yes need to handle this new refusal exit path.

What this covered

Every changed file read in full against the PR head (c512701d) diffed from the merge-base (79f6e08c): apps/rocm/src/{uninstall.rs, uninstall_gate.rs, uninstall_plan.rs} and the non-test production hunks of main.rs were read directly by this reviewing context (not an independent sub-pass, due to persistent contention for this host's shared subagent capacity — see below); apps/rocmd/src/{common,persistence,sandbox,service,watchers}.rs likewise read directly by this context. Independent fan-out workers covered: crates/rocm-core/src/{automation,config,lib,net,proc_lifecycle,service_record}.rs; apps/rocm/src/{service_launch,therock,chat}.rs + crates/rocm-engine-protocol/src/lib.rs + engines/vllm/src/state.rs; the ~4,000-line new/changed unit-test block in apps/rocm/src/main.rs's mod tests; tests/e2e-cucumber/features/install_lifecycle.feature + lifecycle_steps.rs + docs/*.md + README.md + Cargo.lock; and the four source-reading passes (agent-instruction adherence against this repo's own AGENTS.md, history via git log/blame, prior-changes via commit-message pattern analysis only — no access to this PR's actual GitHub review threads, so that pass is necessarily limited — and code-comment accuracy) across the whole diff.

Degraded in part, not fully: this host's shared subagent pool was saturated by other concurrent sessions throughout this review. Two of seven planned independent passes (the core uninstall.rs/uninstall_gate.rs/uninstall_plan.rs logic, and the rocmd daemon-side files) could not be spawned as independent workers after repeated attempts and were instead reviewed directly by this orchestrating context — flagged per the method's fallback rule rather than silently absorbed. The other five passes did run as independent fan-out workers. Design-review (Step 7) questions were reasoned through by this context directly rather than via further sub-workers, given the depth of material already gathered; no fundamental approach problem was found.

Not reconciled against any prior review or discussion on this PR — this review was conducted blind, with no access to prior rounds.

[nowycondro]

🔴 Automated review · pr-review-watcher · f58f872

Review — needs work

Full review of the whole change (29 files, +8822/−474 between origin/main at d59f596f and PR head f58f872 — of which ~4120 of the insertions in apps/rocm/src/main.rs are test-module content; the production-code sliver in that file is ~90 lines).
Implements the fix described by Jira ROCMAI-290 (title: "rocm uninstall reports success while leaving a publicly-bound GPU-holding server running and removing the tooling to stop it"; also named here by its external alias EAI-8014): stop every managed service and the background helper (rocmd) before rocm uninstall removes files, and refuse non-destructively (remove nothing, exit non-zero) when a stop cannot be confirmed. The implementation matches the ticket's "Expected" section point-for-point (stop before delete, verify endpoints/process trees are gone, abort and retain recovery tooling on failure, never report success with a live endpoint). The findings below are about test-verification gaps and one dead-code item, not about the core approach being wrong.

Blocking

• A test named/commented as proving "a real stop" never checks that the process was actually stopped — apps/rocm/src/main.rs:19148 (a_real_stop_that_a_survivor_outlives_says_the_key_is_gone_and_it_is)
  The test's own doc comment states: "A keyed service is stopped for real (the production stop, which drops its endpoint key)... The abort must say the key was dropped, and the key must actually be gone: the message and the state asserted together." It spawns a real sleep 60 standing in for the service's pid, runs the real stop pass, then asserts only the abort message text and that the endpoint key was removed — it never calls rocm_core::process_is_running(pid) or the file's own assert_stopped_within_grace helper, and ends with an unbounded let _ = server.wait();. Its sibling test two hundred lines earlier, uninstall_refuses_while_a_recorded_endpoint_still_accepts_connections (apps/rocm/src/main.rs:17968), does call assert_stopped_within_grace in the equivalent spot. Because the stand-in process exits on its own after 60s, a regression that marked this service "confirmed stopped" (dropping its key) without actually killing it would still pass this test — it would just make the test slower. This also sits in tension with the PR description's own claim that "each of these was verified against a faithful revert of the production change it names... Two are labelled in-place as the exceptions" — this test is not one of the two labelled exceptions.
  Confidence 85 · logic · Fix: add assert_stopped_within_grace(&mut server, ...) (or at minimum assert!(!rocm_core::process_is_running(pid))) before the trailing server.wait().

• No confirmed test execution of the new feature's Windows-specific process-stop path for this commit — apps/rocm/src/uninstall.rs (all real-process gate tests, e.g. uninstall_stops_live_managed_service_and_reports_it, the_helper_is_stopped_with_the_whole_tree_scope), tests/e2e-cucumber/features/install_lifecycle.feature:310-311 (lifecycle-24)
  Every test in this PR that actually spawns and kills a real process to exercise the new stop-before-remove gate is gated #[cfg(unix)] or #[cfg(target_os = "linux")]; none is #[cfg(windows)]. The only coverage aimed at the platform-specific behavior this PR itself documents as different ("Process-tree stop is Linux-only in effect: on Windows only the recorded process is terminated") is the e2e scenario lifecycle-24, tagged @requires-os:windows, which runs only via the separate e2e-selfhosted.yml workflow (ci.yml explicitly says so at line 1516-1517: "SEPARATE workflow, e2e-selfhosted.yml... They live there — not here"). gh pr checks 299 for this exact commit lists every check that actually ran (including windows-build-and-test, which only runs the cfg(unix)-excluded unit suite on Windows) and contains no entry at all from e2e-selfhosted.yml — no strix-windows/Windows-e2e job appears, passing or otherwise. The PR description itself is candid that "The Windows scenario cannot run in the Linux gate I used; CI is its first run," but CI shows no evidence that run happened for this commit. As things stand, the platform-divergent behavior this PR adds has no demonstrated execution on the one platform it behaves differently on.
  Confidence 85 · logic · Fix: confirm (and show evidence) that the self-hosted Windows e2e lane actually ran lifecycle-24 for this commit before merging, or add a cfg(windows)-gated unit/integration test using a Windows-safe stand-in process so coverage doesn't depend solely on a non-required, easily-skipped lane.

• identity_state_with_observed is dead code — crates/rocm-core/src/proc_lifecycle.rs:200, re-exported at crates/rocm-core/src/lib.rs:151
  It's pub, re-exported from the crate root, and carries a substantial doc comment explaining exactly when a caller should prefer it over identity_state ("a caller that has already read [the start-time] — because it also needs the raw observation — should pass that one reading to identity_state_with_observed rather than calling this and reading again"). A repo-wide grep (git grep -n "identity_state_with_observed" pr/299 -- '*.rs') finds no call site anywhere outside its own definition, its own doc comments, and its own unit tests. Neither apps/rocm/src/uninstall_gate.rs nor apps/rocmd/src/service.rs — the two places in this PR that read a raw start-time and then need a verdict — actually call it; both call identity_state directly instead.
  Confidence 85 · architectural · Fix: either wire in the caller this function was written for, or drop the function (and its re-export) until one exists.

Non-blocking

• engines/lemonade/src/state.rs::write_state was not migrated to the atomic-write helper this PR introduces, unlike its sibling engines/vllm/src/state.rs::write_state — engines/lemonade/src/state.rs:205-211 vs. engines/vllm/src/state.rs:196-205
  vLLM's version now calls rocm_core::write_file_atomically, with a comment explaining why: "rocmd and rocm uninstall read this file while a launch rewrites it; the atomic publish means they never see a torn document." Lemonade's write_state, untouched by this PR, is still a plain fs::create_dir_all + fs::write (truncate-in-place) — the exact failure mode the new helper's own doc comment calls unsafe for a concurrent reader. Traced every consumer of the file this writes (ManagedServiceRecord::refresh_from_engine_state and its callers in main.rs, managed_service_status.rs, providers.rs): all wrap the read in .unwrap_or(false) or equivalent, so the practical blast radius today is a silently-missed status refresh, not an aborted uninstall or a panic — this is why it's non-blocking rather than blocking.
  Confidence 70 · logic · Fix: switch engines/lemonade/src/state.rs::write_state to rocm_core::write_file_atomically, matching vLLM, so the torn-read property this PR establishes for one managed-engine adapter holds for both.

• Leftover vestigial setup in a test leaks an unused temp directory — apps/rocm/src/main.rs:20499 (a_helper_that_cannot_be_confirmed_stopped_stops_no_service_after_it)
  The test's first three lines (let mut helper = plant_verified_helper(&test_paths("unused").1); let _ = helper.kill(); let _ = helper.wait();) spawn a process and write runtime state under a test_paths("unused") root, then immediately kill it and shadow the helper variable with a second, real one. The first root is never referenced again and its directory is never passed to fs::remove_dir_all, unlike every other test's cleanup — a harmless but confusing leftover that leaks a directory under .rocm-work/tests/main/ on every run.
  Confidence 85 · mechanical · Fix: delete the three leftover lines.

Decisions for the author

• PR size/scope — tradeoff
  This single commit changes 29 files and +8822/−474 lines. The bulk is legitimately coupled to the one fix (the atomic-write centralization supports torn-read safety for files uninstall now reads mid-stop; the windows-sys dependency removal is a direct consequence of centralizing the Windows publish step; the near-total main.rs test-module rewrite is overwhelmingly new tests for the new gate) rather than several unrelated changes bolted together, so this isn't a scope-mismatch finding. But it is large enough that a human reviewer will have a hard time giving it the attention a safety-critical process-killing change deserves in one pass — worth confirming the reviewing team is comfortable with that, or whether the supporting atomic-write refactor could have landed as a smaller, separately-reviewed precursor.
• Older on-disk records with no recorded start-time now cause rocm uninstall to refuse rather than silently succeed — tradeoff
  A live service or helper recorded by a pre-this-fix rocm-cli version (no supervisor_start_ticks/daemon_start_ticks) is now refused rather than killed, requiring the operator to stop it by hand and re-run. This is the deliberately stricter, safer behavior the ticket asks for, and it's documented in the README and docs/manual-testing.md, so it's flagged here only so the author can confirm this upgrade-path UX (a new hard refusal where there used to be silent, if unsafe, success) is the intended support stance rather than a surprise for existing installations.

Positive signals

• stop_managed_services_with_helper_stop stops rocmd before the per-service loop, matching its own doc comment's rationale that the helper would otherwise respawn a service mid-stop — a real race closed exactly the way it's documented.
• StopFailureRemedy::advice/advice_rank and StoppedRecordVerdict::blocks are matched arm-by-arm with no wildcard _ =>, so a newly-added variant forces a compile error until someone decides which side of the abort/proceed line it belongs on — a concrete defense against silently fail-open regressions in exactly the kind of gate where that matters.
• Partial-failure bookkeeping is handled with real care: when a service's stop was reported confirmed but a post-stop port probe later finds a survivor, the code explicitly retracts it from report.stopped and strips it from report.relaunch before re-filing it under report.failed (uninstall_gate.rs), and deferred_partners is filtered so one partner's failure isn't double-reported through a record merely sharing its port.
• EndpointIdentity (ServesExpectedModel / ServesOtherModels / ListsNoModels) is a genuinely useful three-way distinction for the gate's probe — a model-less /v1/models response (engine warming up) is kept apart from "a stranger is listening," which a boolean couldn't express.
• Windows process_start_ticks via GetProcessTimes closes a previously-silent gap: before this PR, Windows managed-service stops had no PID-recycling protection at all.
• The new atomic-write regression tests in crates/rocm-core/src/lib.rs are unusually well-reasoned, each with a comment stating what a naive fs::write-based implementation would also satisfy (so wouldn't be a useful regression test) versus the property only the rename-based implementation provides.
• The e2e scenario comments (e.g. lifecycle-24's Windows caveat) are candid about their own coverage boundaries rather than overclaiming what they test.

Deployment notes

• Behavior change: an installation with a live managed server or rocmd whose on-disk record predates supervisor_start_ticks/daemon_start_ticks (written by an older rocm-cli) will now have rocm uninstall refuse and remove nothing, where it previously removed the tooling while leaving the process running. Documented in README.md and docs/manual-testing.md.
• windows-sys is no longer a direct dependency of apps/rocm or apps/rocmd (the one feature they used, Win32_Storage_FileSystem/ReplaceFileW, is now centralized in crates/rocm-core, which still depends on it) — purely an internal dependency-graph change, no functional impact.
• Process-tree-kill semantics are platform-asymmetric by design: force-stopping rocmd kills its whole recorded process tree on Linux but only the recorded process on Windows (descendants aren't enumerable there). Already documented in --help, the README and the plan output.

What this covered

Reviewed the full diff between origin/main (d59f596f6e08297c207f008021eecb37defc3558) and PR head pr/299 (f58f8725b944817fd441cf25d430aa393c826a7b). Confirmed apps/rocm/src/main.rs's ~4213-line insertion is ~90 lines of production code plus ~4120 lines of test-module content, and reviewed both parts. Six independent fan-out workers each read their assigned files completely at PR head plus necessary surrounding/caller code: (1) uninstall_gate.rs/uninstall.rs/uninstall_plan.rs, (2) rocm-core's proc_lifecycle.rs/net.rs/service_record.rs/automation.rs/config.rs, (3) rocm-core::lib.rs/rocm-engine-protocol/engines/vllm/therock.rs/chat.rs/service_launch.rs, (4) the rocmd daemon files plus the windows-sys dependency removal, (5) main.rs's new/changed unit tests, (6) the e2e-cucumber feature file and step definitions. All three blocking findings and both non-blocking findings were independently re-verified by me directly against the source (git show/git grep on pr/299) and, for the Windows-coverage finding, against live CI check-run data (gh pr checks 299), rather than taken on a worker's word. Read this repo's AGENTS.md and CONTRIBUTING.md for its conventions (bare Jira/EAI ticket IDs are explicitly permitted and not flagged; crate-layering invariants; the atomic-write/torn-read convention this PR itself introduces; Windows+Linux-only platform support) and applied them throughout.

Read CI's statusCheckRollup for this exact head commit: every required check (clippy, fmt/prek, both build-and-test jobs, Test (affected crates), E2E tests, CodeQL, license/notices/docs-links) reports SUCCESS; Regenerate manifests and Skill checks (skillscope) are SKIPPED and not relevant to the files in this diff. Separately confirmed via gh pr checks 299 that e2e-selfhosted.yml (the workflow that would run the Windows-tagged lifecycle-24 scenario) produced no check-run at all for this commit — this fed directly into the Windows-coverage blocking finding above.

Did not run a full cargo test/cargo build myself (large multi-crate Rust workspace; not practical within this review's time budget) — relied on direct code reading plus the CI results above; one worker likewise declined to build the full workspace for the same reason. Did not run dedicated "history" or "prior-changes" passes as separate workers; spot-checked file history on uninstall.rs/main.rs via git log, which showed this as the latest in a series of mechanical-relocation refactors (#747, #742, #746, #636) with nothing in that history contradicting the current change. Not reconciled against any discussion on the PR — per the method, review threads and comments were never read.

[fredespi]

#299 (review)

Fixed in 594a2f5. (1) The real-stop test now asserts the stand-in process actually stopped (assert_stopped_within_grace) before it finishes. (3) identity_state_with_observed is removed with its re-export and tests; helper_decision calls identity_state directly. Also taken: lemonade write_state uses write_file_atomically like vLLM, and the leftover setup lines in the helper test are gone. (2) Not something I can show from here: the E2E self-hosted run for this head had not finished when I last looked, and lifecycle-24 runs only in that lane, so its first Windows result will come from it. I did not add a cfg(windows) unit test I cannot run. Please confirm that lane before merging.

[nowycondro]

🔴 Automated review · pr-review-watcher · 594a2f5

Review — needs work

Full review of the whole change (base d59f596f6e08297c207f008021eecb37defc3558 → tip 594a2f50a22086c79d599e3f5ca22ebb7e268489, the PR's single commit).
Implements the rocm uninstall "stop managed services before removing the tooling that stops them" fix tracked by EAI-8014; ticket named but not retrievable from here (internal Jira, no tracker tool available to this review). The description's "Related" section names PR #241 as carrying the same behaviour as a pinned-expected-failure row, still open/unmerged — confirmed live via gh pr view 241 (row present, PR still open).

Blocking

• Regression test for the "lost its API key" relaunch advice only checks a command prefix, never the --require-api-key flag that is the entire point of that branch — apps/rocmd/src/common.rs:464-468 (test recovery_refuses_a_service_that_lost_a_key_it_was_launched_with), production call site apps/rocmd/src/common.rs:334.
  This PR replaces a hardcoded message with one built from rocm_engine_protocol::relaunch_command(host, true). The updated test asserts only rendered.contains("rocm serve <model> --host {host} --port <port>") — it stops before the flag. Its sibling test two functions down (recovery_respawn_fails_closed_for_a_public_service_without_a_key, lines 481-493) was written correctly: it asserts the full string including --allow-public-bind and explicitly checks --require-api-key is absent where it should be. The asymmetry is the tell — one branch of the same function got a complete test, the other did not. A future edit that silently passes require_api_key: false (or drops the flag inside relaunch_command) for this branch would strip auth from the recovery advice for a service that was explicitly launched with a key, and this suite would still go green. This is exactly the repo's own convention in AGENTS.md ("remediation advice naming a command — the named command must exist, accept those flags, and actually clear the condition that printed it" must be asserted together with the behaviour), violated by the new test this PR itself wrote.
  Confidence 85 · test-coverage/logic · Fix: add assert!(rendered.contains("--require-api-key")) to the first loop, mirroring the sibling test's flag check.

Non-blocking

• PR description names a function that does not exist — the description states "rocm-core grows identity_state_with_observed," but git grep identity_state_with_observed across the PR tip returns nothing. The actual mechanism (one reading answering both "could this platform verify?" and "is this still the same process?") is identity_state/identity_state_from_probes/compare_start_ticks in crates/rocm-core/src/proc_lifecycle.rs:107-181, which I traced and found correctly implements exactly the behaviour described (liveness read before start-tick read, closures enforcing the order, IdentityState::Indeterminate for unconfirmable). The functionality is real and correct; only the cited symbol name is wrong, most likely stale from an earlier draft.
  Confidence 80 · doc-accuracy · Fix: correct the symbol name in the PR description (no code change needed).

Decisions for the author

• A missing runtime-state.json is taken at face value as "no daemon," silently — tradeoff, apps/rocm/src/uninstall_gate.rs:489-496. The function's own doc comment states this plainly: "a missing file is the ordinary never-started case, and there is no pid to verify or signal without it — but it does mean the gate is only as good as the state file. An unreadable one is the case that aborts; an absent one is the case that proceeds." This is the one fail-open path in the whole gate that is not disclosed via a warning (every other fail-open branch I traced — unresolvable host, empty model listing, unidentified listener, corrupt service record — pushes a warning onto ManagedServiceStopReport instead). Deleting runtime-state.json by hand while rocmd is live would let uninstall proceed without stopping it. Confirm this residual gap is accepted as-is rather than, say, also disclosing it as a warning when the gate otherwise found live managed services to stop.
• StopFailureRemedy::RemoveTheStoppedRecord's "if it is [your server], stop it first" clause names no command or discovery method — non-blocking-improvement, apps/rocm/src/uninstall_gate.rs:114-119. Its sibling StopWhatHoldsThePort remedy, for a structurally similar "something answers that isn't tracked by a live record" situation, tells the operator to run ss -ltnp / Get-NetTCPConnection -LocalPort <port> to find it. This remedy's "stop it first" branch gives no equivalent pointer. Pinned only by a wording test (main.rs:21482), not an actionability test. Worth adding the same discovery hint, or leaving it as is on the reasoning that an operator who already recognizes "their own server" doesn't need it.
• ~4,000 lines of new tests for the brand-new uninstall_gate.rs module (1,387 lines, zero tests of its own) live in main.rs's shared mod tests instead — non-blocking-improvement. The module's own comment concedes the cost: "The tests live in main.rs's mod tests, far from here; change an arm and expect them, not this loop, to be what goes red." Coverage itself is substantial and was verified present (directly or via integration tests through build_uninstall_plan/stop_managed_services_before_uninstall) for effectively every branch sampled; this is a discoverability/organization call rather than a gap, and apps/rocm/src/uninstall.rs and apps/rocm/src/service_launch.rs already keep their own tests colocated, so the inconsistency is worth a deliberate "yes, we're keeping it this way" rather than defaulting into it.

Positive signals

• stop_managed_services_before_uninstall → uninstall_removal_gate → execute_plan_with genuinely stops-before-removes and removes nothing on any unconfirmed stop; traced end to end and cross-checked against two Linux-gated integration tests that spawn a real child process and assert it is dead before any file disappears.
• PID-recycling identity logic (identity_state_from_probes) enforces liveness-before-start-tick-read via FnOnce closures specifically to close the one dangerous race (a recycled PID reading as a match); uninstall_gate::daemon_identity_outcome and unprovable_live_pids are even more conservative than the primitive requires, correctly erring toward "can't confirm, so abort" for the single highest-stakes caller in the PR.
• Every fail-open branch in the record/port-probe logic (stopped_record_verdict and friends) is enumerated by name in a doc comment and either discloses itself via a ManagedServiceStopReport.warnings value (assertable by a test, not an eprintln!) or is justified as the one case that is actual evidence of an unrelated server (ProceedUnrelated) — a notably rigorous treatment of fail-open paths for a gate in front of a destructive action.
• write_file_atomically's Windows ReplaceFileW path and its Linux rename path both clean up the scratch file on every failure branch, sync_all before publish, and are backed by a concurrent-reader-sees-no-truncation test; the two local variants in therock.rs/sandbox.rs that deliberately keep their own staging logic (for test-injection seams) say so in their own doc comments, and the claim checks out against the code.
• The shared relaunch_command/relaunch_command_for builder in rocm-engine-protocol removes a real drift risk between rocm serve, rocmd, and the uninstall gate's advice text; its one real call site per crate was traced and is correct (host/flag derivation matches the branch's own condition, so the message cannot disagree with the decision — the same property AGENTS.md calls out for preapproved_install_line).
• The 7 new/renumbered @lifecycle Gherkin scenarios were diffed carefully against the old numbering: every renumbered scenario is byte-identical apart from its number, nothing was silently dropped or weakened, and the new step definitions spawn real child processes / plant real on-disk records rather than mocking the behaviour under test.
• docs/architecture.md, docs/testing.md, docs/manual-testing.md, and README.md were all updated consistently with the new module and behaviour, using correctly-formed relative markdown links.

Deployment notes

• At review time, three checks for this exact commit had not finished: windows-build-and-test (IN_PROGRESS), Build Linux PR binaries (IN_PROGRESS), and E2E tests (QUEUED) — the last of these is what actually runs the new @lifecycle scenarios this PR's safety claims rest on. Nothing had failed, but none of the three should be read as green yet.
• This changes rocm uninstall's default behaviour: it now actively stops managed servers and the rocmd helper first, and refuses to remove anything if it cannot confirm they stopped. Operators relying on the previous "uninstall always finishes, cleans up after itself" behaviour will see a new non-zero-exit failure mode (with recovery commands named in the error) they did not see before.
• Force-stop reaches the whole process tree on Linux but only the single recorded PID on Windows — and, per crates/rocm-core/src/process.rs's own doc comment, identically on macOS, though this repo's conventions state macOS is not a supported platform, so that's informational only.

What this covered

Every file in the diff, read completely at the PR tip via git show pr-299:<path> (never checked out), plus the surrounding functions/callers needed to judge each change in place: apps/rocm/src/uninstall.rs, apps/rocm/src/uninstall_gate.rs (new), apps/rocm/src/uninstall_plan.rs, apps/rocm/src/main.rs (full diff plus every touched/new function and test body), apps/rocm/src/therock.rs, apps/rocm/src/service_launch.rs, apps/rocm/src/chat.rs, apps/rocm/Cargo.toml, Cargo.lock, apps/rocmd/src/common.rs, apps/rocmd/src/persistence.rs, apps/rocmd/src/sandbox.rs, apps/rocmd/src/service.rs, apps/rocmd/src/watchers.rs, apps/rocmd/Cargo.toml, crates/rocm-core/src/automation.rs, config.rs, lib.rs, net.rs, proc_lifecycle.rs, service_record.rs, crates/rocm-engine-protocol/src/lib.rs, engines/lemonade/src/state.rs, engines/vllm/src/state.rs, tests/e2e-cucumber/features/install_lifecycle.feature, tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs, docs/architecture.md, docs/manual-testing.md, docs/testing.md, README.md. Six independent blind workers covered these grouped by coupling (uninstall core; main.rs; rocm-core; rocmd+engine-protocol; therock/service_launch/chat; e2e-tests+docs); all six returned, none failed or had to be re-spawned.
Did not run as separate passes: history (git log/git blame on modified regions) — not performed; prior changes (do earlier review comments on these files apply here) — out of scope per this task's instructions (must not read other reviewers' threads to avoid biasing the blind pass). Agent-instruction adherence was run throughout against this repo's AGENTS.md (it ruled out one fan-out worker's candidate finding about macOS support, since AGENTS.md states only Windows/Linux are supported platforms). Code-comments compliance was checked inline by each file-group worker rather than as a separate pass. Step 7 (design review) was performed directly rather than via further spawned workers, given the depth the six-worker fan-out already provided; it found the fail-open handling coherent and did not surface a new blocking question beyond what is listed above. Not reconciled against any other reviewer's discussion or prior review rounds on this PR — this is an independent blind pass.

[rominf]

🔴 Automated review · pr-review-watcher · c512701

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.

Informational, not a binding round. These findings describe commit c512701d. While this round ran, the head moved to f58f8725, a single squashed commit on a newer main whose tree differs from c512701d. So this is a plain comment rather than a review, it files no new change request, and its findings are unanchored. The change requests already standing from earlier rounds are untouched and still gate. The next round reviews the current head and settles these findings there. Between now and then, the blockers first found below at c512701d do not gate on their own.

Round note (full review of the whole change at c512701d). Every blocking item below is marked new or standing.

• The standing gating objection from d37698a7 still holds at c512701d. The relaunch advice is built from record.model_ref. For qwen on vLLM, that is the override Qwen/Qwen3-4B-Instruct-2507, which matches_ref cannot resolve. The command also drops runtime_id, env_id and device_policy.
  • The blind review below did not re-find this.
  • After its report was frozen, the objection was put to the reviewer: uninstall_gate.rs:799-805, serve_cmd.rs:132-140,584-585,743, model_catalog.rs:237-244, model_catalog.json:110-117,138, rocm-engine-protocol/src/lib.rs:86-101.
  • That verdict is an anchored check, not part of the blind review.
  • Still open: whether the printed command fails outright or launches without the recipe's tool-call parser. The record already carries canonical_model_id, so the proposed fix needs no new plumbing.
• Other standing items, from d37698a7:
  • PATH-undo ordering. Not re-checked as worded. The blind review found a sibling defect in the same function (first Blocking item below).
  • vLLM atomic write comment. Re-found blind (second Blocking item).
  • Co-Authored-By trailers and the stale rocmd Windows comment. Not checked: the trailers live on the old commit list, which the squash replaced.
• CI at c512701d: every hosted check passed. Sphinx docs build (-W) was cancelled, and Regenerate manifests and Skill checks (skillscope) were skipped. No GPU-lane check appears.

Review — needs work

Full review of the whole change at c512701. The PR head has since moved to f58f872, which this review did not read.
Addresses EAI-8014 (now tracked as ROCMAI-290): stop managed servers before uninstall removes the tooling that stops them, and abort non-zero, keeping that tooling, when they cannot be confirmed stopped. The change covers all three expectations in the ticket.

Blocking

• A removal that fails partway does not say which services this run already stopped, or how to bring them back (new) — apps/rocm/src/uninstall.rs (execute_plan_with, the removal-loop error arm calling partial_removal_note), apps/rocm/src/uninstall_gate.rs:1262-1271 (stopped_during_run_note doc)
  The doc on stopped_during_run_note lists "a later step that fails" as a case it reports. But execute_plan_with only uses stopped_note in the PATH-undo failure arm. When removing a config/data/binary entry fails, the error comes from partial_removal_note(&removed, name, &untouched), which knows nothing about the stop report. The success line printed before removal is only a count (stopped N managed service(s) before removal). So after a mid-removal failure, the operator never sees which services were stopped, that their endpoint keys are gone, or the rocm serve commands that bring them back. The doc describes the situation correctly, so the code should change to match it.
  Confidence 85 · logic · Fix: thread stopped_note into the removal-failure error as well, and assert it in a_removal_that_fails_partway_says_what_is_gone_and_what_is_left.

• The new vLLM comment promises a torn-read guarantee that the same file does not have (standing since f88d72cb, re-found blind this round) — engines/vllm/src/state.rs:196-199, crates/rocm-core/src/net.rs:648-671 (merge_json_state_file), engines/lemonade/src/state.rs:205-209
  The new comment says "rocmd and rocm uninstall read this file while a launch rewrites it; the atomic publish means they never see a torn document". The doc comment directly above it says the inference-verified latch bypasses write_state and patches this file through rocm_core::merge_json_state_file. That function still truncates and rewrites in place with fs::write. Lemonade's identical write_state, read by the same ManagedServiceRecord::refresh_from_engine_state, was not converted either. The fix landed on one of two twin writers and one of two write paths.
  Confidence 85 · logic · Fix: publish through write_file_atomically in merge_json_state_file and in lemonade's write_state, or narrow the comment to what is actually guaranteed.

• The new relaunch command in the public-bind refusal in rocm is not covered by any test (new) — apps/rocm/src/service_launch.rs (second bail! in ensure_public_service_has_endpoint_key), tests at service_launch.rs:1565-1575 and respawn_fails_closed_for_a_public_service_whose_key_is_gone
  For ("0.0.0.0", false, false) the tests only assert that --allow-public-bind is present and --require-api-key is absent. The old literal (rocm serve --host {host} --allow-public-bind) satisfies both, so reverting this branch keeps every test green. The rocmd twin in apps/rocmd/src/common.rs does pin the full rocm serve <model> --host 0.0.0.0 --port <port> --allow-public-bind form.
  Confidence 85 · mechanical · Fix: add the same full-command assertion to the rocm test.

• The description misstates the change's scope (new; description only, so on its own it would not gate a squash merge) — PR description, "Supporting work"

  1. It says the rocm serve and rocmd lost-key refusals "are unchanged by this PR". This diff rewrites both: apps/rocm/src/service_launch.rs and apps/rocmd/src/common.rs replace rocm serve --require-api-key / rocm serve --host {host} --allow-public-bind with relaunch_command(...) output (commit 97ea46e).
  2. It does not mention the vLLM engine-state write becoming atomic (engines/vllm/src/state.rs).
     Confidence 85 · mechanical · Fix: correct the description to say both refusals now print the shared <model>/<port> command, and list the vLLM change.

Non-blocking

• A wildcard-bound live server can falsely block uninstall through an older stopped record on a specific address — apps/rocm/src/uninstall_gate.rs:564-594 (partition_records), :1077 (hosts_name_one_listener), :1100 (probe_hosts)
  Take a live record on 0.0.0.0:P and a stopped record for the same model on, for example, a LAN IP or 127.0.1.1 at port P. The live record probes only 127.0.0.1/::1, so the two are not treated as partners. The pre-flight then probes the stopped record first and reaches the live wildcard listener serving our model. The result is BlockServingOurModel, so the run refuses with "the engine outlived its supervisor" advice, which is false here. It fails closed, so it is only a usability problem.
  Confidence 70 · logic · Fix: treat a wildcard live host as a partner of any host on the same port.

• Two doc comments contradict the code — apps/rocm/src/uninstall_gate.rs:10-11, :1232
  The module doc says UninstallOptions/UninstallPlan "stay in the crate root", but they are defined in uninstall_plan.rs:54,71. unreadable_service_manifests says it "Returns the file names", but it returns full paths, which the abort relies on.
  Confidence 85 · mechanical · Fix: correct both comments.

• A test's comment and fixture describe one failure class and exercise another — apps/rocm/src/uninstall.rs:1958-1980 (a_background_helper_that_cannot_be_stopped_leaves_every_planned_path_in_place)
  The comment and reason say the helper's "identity could not be verified". Production maps that case to StopFailureRemedy::ConfirmTheDaemonPid ("check what that pid is"). The test builds StopTheDaemon and pins its "kill that pid" advice instead. The assertions hold for the branch actually built, but the comment misdescribes what the test covers.
  Confidence 85 · mechanical · Fix: reword the reason/comment to "signalled but not confirmed gone".

• host_port_resolves was added to main.rs but only the gate uses it — apps/rocm/src/main.rs:4887-4898
  AGENTS.md §6 says new subsystem code goes in its own file, not into main.rs. Separately, the gate's roughly 3.6k lines of tests also landed in main.rs mod tests; uninstall_gate.rs:966 admits they are "far from here".
  Confidence 70 · architectural · Fix: move the helper into uninstall_gate.rs (or next to loopback_tcp_port_is_reachable), and consider giving the gate its own test module.

• The tick-less service advice does not say which file to delete — apps/rocm/src/uninstall_gate.rs:101-108 (ConfirmTheServicePid)
  It says "delete the service's record file under the data directory's services folder by hand" without giving a path. Its sibling RepairTheRecord names the full path, and rocm services remove refuses a record whose pid is live.
  Confidence 70 · mechanical · Fix: include the manifest path in the failure id or the advice.

Decisions for the author

• One PR bundles several separable changes — non-blocking-improvement
  At c512701 this is 8.8k lines over 65 commits. Besides the uninstall gate, it carries:

  • the workspace-wide atomic-write consolidation (rocm-core, rocmd, therock, vLLM);
  • real Windows process start-times, which changes every Windows rocm services stop, not only uninstall;
  • a protocol-level relaunch-command builder that rewrites serve and rocmd messages.

  Each is reviewable on its own, and together they are hard to review completely. Splitting would cost re-coordination of the stacked tests; keeping them together keeps the motivating fix and its supporting pieces in one place.

Positive signals

• One pre-flight drives both the plan and the stop pass, so the plan cannot promise a stop that the run then refuses.
• StopFailureRemedy::advice/advice_rank and StoppedRecordVerdict::blocks are exhaustive matches, so a new class cannot silently go unadvised or default to proceeding.
• The e2e steps assert each printed claim next to the outcome it describes. For example, then_plan_names_helper is paired with then_background_helper_stopped. This is the pattern AGENTS.md §3 asks for.
• daemon_start_ticks and the tick fields use #[serde(default)], so state files and records written before them still parse rather than being refused as unreadable.
• The Windows GetProcessTimes FFI closes its handle on every path, and a FILETIME is only compared for equality, never mixed with Linux ticks.

Deployment notes

• After upgrading, rocm uninstall refuses until the user stops by hand:

  • a rocmd that was already running and so wrote no daemon_start_ticks;
  • a live server recorded without a start time.

  This is intended, and the README and advice cover it.

• On Windows, process identity is now checked, so rocm services stop and restart can return Recycled/Indeterminate where they always matched before.

• Config, service-record and automation-state writes now publish through ReplaceFileW on Windows instead of a rename. The runtime state also fsyncs on each daemon tick.

• rocm uninstall --dry-run and the chat dry-run tool now make HTTP identity probes (2 s timeout) for each reachable already-stopped record.

What this covered

• Every file in prw-base...HEAD (ea86403…c512701d; merge-base 79f6e08, and it merges cleanly onto the current base).
  • Read completely: the apps/rocm uninstall modules and the non-test main.rs hunks; the rocm-core/rocmd/protocol/vLLM changes; the docs; the e2e feature and steps.
  • The roughly 80 new tests in main.rs were cross-checked only by name against the branches they claim. They were not given individual revert/mutation verdicts.
• Fan-out: two read-only workers (uninstall modules; core/daemon/protocol). The docs, e2e, history, agent-instruction and Step 9 passes ran in this driving context.
• What ran:
  • cargo test -p rocm-core --lib with mutation rounds on the atomic-write, start-time identity and endpoint-identity helpers; every mutated test failed except those noted.
  • Sign-off check: exit 0, 65 commits, none missing.
• Did not run:
  • apps/rocm and rocmd test builds, the lifecycle e2e scenarios, and any Windows compile. Disk was near the 40G floor. The four blocking findings are from reading, not from runs.
  • The separate design-question workers (Step 7). The fan-out cap and time budget were spent, so that pass ran inline and is not independent.
  • The prior-changes pass: no earlier review comments on these files were read.
• CI: statusCheckRollup reports the PR's current head f58f872, not c512701. Nothing here establishes CI results for the reviewed commit. At f58f872 every required lane listed passed; "Sphinx docs build (-W)" was cancelled, and README.md, which that build includes, changed in this PR.
• No prompt-injection content was found.
• Not reconciled against any PR discussion.

The driver added the opening note and the new/standing tags. The verdict on the standing objection in that note is an anchored check, not part of what this review read blind.

[fredespi]

#299 (review)

Both points are fixed, in a2939d9 (the --require-api-key assertion landed in 1bec494). The rocmd recovery test now asserts the flag, and the PR description no longer names the removed identity_state_with_observed. I also corrected the description of the vLLM/Lemonade state writes. On the two decisions: a missing runtime-state.json is still read as "no daemon" and proceeds without a warning (accepted, the gate is only as good as that file); I left the RemoveTheStoppedRecord advice and the test layout as they are.

[fredespi]

#299 (comment)

Taken in a2939d9: (1) the stop pass note now also goes on a removal that fails partway (new test a_removal_failure_after_a_stop_says_what_the_stop_already_did); (2) merge_json_state_file and Lemonade write_state now publish atomically, matching vLLM; (3) the rocm public-bind refusal test asserts the full relaunch command; (4) the description now says the rocm serve and rocmd refusals changed to the shared command, and lists the vLLM change. Also fixed the two stale doc comments and the helper test reason. Not taken: the standing relaunch objection. record.model_ref is the raw CLI argument (serve_cmd.rs passes &model into the launch); the override id only goes to the engine over the wire, so the printed command resolves. The relaunch does not carry --runtime-id, --env-id or the device policy; the description now says so. Also left alone: moving host_port_resolves, the ConfirmTheServicePid path hint, the wildcard-partner case (it fails closed), and splitting the PR.

[nowycondro]

🤖 Automated review (Claude Code) · 364f5cc

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

Review — no blocking findings

Full review of the whole change. Implements the fix for ROCMAI-290 (EAI-8014 is the same ticket under a different project key alias): rocm uninstall reported success while a managed, GPU-holding server kept serving and deleted the tooling needed to stop it. The PR description's account of the defect and the fix matches the ticket exactly.

Blocking

None

Non-blocking

• relaunch_command_for's placeholder substitution can misfire if a model string contains the literal text <port> — crates/rocm-engine-protocol/src/lib.rs, relaunch_command_for (~line 73-82)
  The function does relaunch_command(host, require_api_key).replacen("<model>", &shell_word(model), 1).replacen("<port>", &port.to_string(), 1). shell_word(model) is substituted into the template first; if that substituted text happens to contain the literal substring <port> (e.g. a local model path or file name containing that text), the subsequent .replacen("<port>", ...) replaces the first occurrence of <port> — which is now inside the substituted model text — rather than the real placeholder later in the string, leaving the genuine <port> placeholder unfilled in the printed relaunch advice. Practical impact is low (requires a model ref/path containing exactly <port>) and the failure mode is cosmetic (malformed advice text, not a crash or data issue).
  Confidence 75 · logic · Fix: substitute <port> before <model>, or use a non-collidable placeholder token, or operate on the un-filled template positions directly instead of string replacement.

Decisions for the author

None

Positive signals

• Exhaustive match on StopFailureRemedy/StoppedRecordVerdict (no _ wildcard arm) means a newly added variant fails to compile until it is given advice text and an advice_rank — this is explicitly designed to prevent a new failure class from silently reaching the operator with no actionable guidance (apps/rocm/src/uninstall_gate.rs).
• identity_state_from_probes takes its liveness and start-time reads as lazily-evaluated closures specifically so the liveness check happens strictly before the start-time read, closing a PID-recycling TOCTOU race that the previous implementation's eager-argument evaluation was exposed to (crates/rocm-core/src/proc_lifecycle.rs); the ordering is pinned by a dedicated unit test rather than left as a comment.
• The relaunch-command text is centralized in one builder (rocm_engine_protocol::relaunch_command) used by both rocm serve's and rocmd's refusals and the uninstall abort, preventing the three previously-hardcoded copies of this advice from drifting apart.
• write_file_atomically's symlink-replacement behavior change (replacing a symlink at the target path rather than writing through it, closing a plant-a-link redirection risk) is called out explicitly in both the function doc and a dedicated test rather than left as an implicit side effect of the refactor.

Deployment notes

• Behavior change for operators: rocm uninstall now stops managed servers and the rocmd helper before removing anything, and refuses (exit non-zero, nothing removed) if a stop cannot be confirmed. Documented in --help, README.md, and docs/manual-testing.md.
• rocm serve/rocmd refusal text for a service that lost its endpoint key now prints a different (shared-builder-generated) command string than before; any tooling or docs scraping that exact previous wording elsewhere would need updating (none found in this diff).

What this covered

This ran as a single-context pass by deliberate choice (per explicit task instruction not to spawn sub-workers for this review), not a fan-out failure — so the per-pass independence and cross-worker consolidation (Steps 4-6) that the method's fan-out normally provides did not happen; one reviewer context read everything sequentially instead.

Read completely: all 35 changed files in df1548a2...364f5ccc, including the full new file apps/rocm/src/uninstall_gate.rs (1387 lines), the full diffs of apps/rocm/src/uninstall.rs, uninstall_plan.rs, service_launch.rs, services_text.rs, therock.rs, chat.rs, automations.rs, test_support.rs; crates/rocm-core/src/lib.rs, net.rs, proc_lifecycle.rs, automation.rs, config.rs, service_record.rs; crates/rocm-engine-protocol/src/lib.rs; all of apps/rocmd/src/{common,persistence,sandbox,service,watchers}.rs; engines/lemonade/src/{backend_alignment,install,state}.rs and engines/vllm/src/state.rs; tests/e2e-cucumber/features/install_lifecycle.feature and tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs in full; README.md, docs/architecture.md, docs/testing.md, docs/manual-testing.md, Cargo.lock, and both app Cargo.toml diffs.

Read partially: the ~3,600 lines of test additions inside apps/rocm/src/main.rs's mod tests (the non-test code in that file — the new host_port_resolves helper, --help text, and the mod uninstall_gate; wiring — was read in full). I enumerated and scanned every one of the ~98 new test functions by name and read several hundred lines of representative tests in detail (the daemon-identity/port-probe mapping tests, the plan/gate-wiring tests, the partial-removal-failure tests), cross-checking their assertions against the production logic in uninstall_gate.rs and uninstall.rs; I did not read every one of the ~98 test bodies line-by-line. Nothing sampled suggested weak assertions, disabled tests, or a gap between a test's name/claim and what it actually checks.

Did not run: history (git log/git blame on the modified regions) and prior-changes passes were not performed as separate dedicated passes, though the diff's own extensive commentary (e.g. in uninstall_gate.rs and the test names) made the rationale behind most non-obvious choices explicit without needing blame. CI was checked live via gh pr view --json statusCheckRollup against this exact head commit (364f5ccc): all 23 applicable checks (clippy, CodeQL, build-and-test, windows-build-and-test, E2E tests, Coverage, Commit signatures + sign-off, Sphinx docs build, etc.) report SUCCESS; 2 (Regenerate manifests, Skill checks (skillscope)) are SKIPPED. No failing check.

Not reconciled against any prior review discussion on this PR — this was a blind review per the method, and the many existing human/bot reviews on this PR at earlier commits were deliberately not read or reconciled against.

[nowycondro]

🤖 Automated review (Claude Code) · 364f5cc

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

Review — no blocking findings

Full review of the whole change. Implements the fix for ROCMAI-290 (EAI-8014 is the same ticket under a different project key alias): rocm uninstall reported success while a managed, GPU-holding server kept serving and deleted the tooling needed to stop it. The PR description's account of the defect and the fix matches the ticket exactly.

Blocking

None

Non-blocking

• relaunch_command_for's placeholder substitution can misfire if a model string contains the literal text <port> — crates/rocm-engine-protocol/src/lib.rs, relaunch_command_for (~line 73-82)
  The function does relaunch_command(host, require_api_key).replacen("<model>", &shell_word(model), 1).replacen("<port>", &port.to_string(), 1). shell_word(model) is substituted into the template first; if that substituted text happens to contain the literal substring <port> (e.g. a local model path or file name containing that text), the subsequent .replacen("<port>", ...) replaces the first occurrence of <port> — which is now inside the substituted model text — rather than the real placeholder later in the string, leaving the genuine <port> placeholder unfilled in the printed relaunch advice. Practical impact is low (requires a model ref/path containing exactly <port>) and the failure mode is cosmetic (malformed advice text, not a crash or data issue).
  Confidence 75 · logic · Fix: substitute <port> before <model>, or use a non-collidable placeholder token, or operate on the un-filled template positions directly instead of string replacement.

Decisions for the author

None

Positive signals

• Exhaustive match on StopFailureRemedy/StoppedRecordVerdict (no _ wildcard arm) means a newly added variant fails to compile until it is given advice text and an advice_rank — this is explicitly designed to prevent a new failure class from silently reaching the operator with no actionable guidance (apps/rocm/src/uninstall_gate.rs).
• identity_state_from_probes takes its liveness and start-time reads as lazily-evaluated closures specifically so the liveness check happens strictly before the start-time read, closing a PID-recycling TOCTOU race that the previous implementation's eager-argument evaluation was exposed to (crates/rocm-core/src/proc_lifecycle.rs); the ordering is pinned by a dedicated unit test rather than left as a comment.
• The relaunch-command text is centralized in one builder (rocm_engine_protocol::relaunch_command) used by both rocm serve's and rocmd's refusals and the uninstall abort, preventing the three previously-hardcoded copies of this advice from drifting apart.
• write_file_atomically's symlink-replacement behavior change (replacing a symlink at the target path rather than writing through it, closing a plant-a-link redirection risk) is called out explicitly in both the function doc and a dedicated test rather than left as an implicit side effect of the refactor.

Deployment notes

• Behavior change for operators: rocm uninstall now stops managed servers and the rocmd helper before removing anything, and refuses (exit non-zero, nothing removed) if a stop cannot be confirmed. Documented in --help, README.md, and docs/manual-testing.md.
• rocm serve/rocmd refusal text for a service that lost its endpoint key now prints a different (shared-builder-generated) command string than before; any tooling or docs scraping that exact previous wording elsewhere would need updating (none found in this diff).

What this covered

This ran as a single-context pass by deliberate choice (per explicit task instruction not to spawn sub-workers for this review), not a fan-out failure — so the per-pass independence and cross-worker consolidation (Steps 4-6) that the method's fan-out normally provides did not happen; one reviewer context read everything sequentially instead.

Read completely: all 35 changed files in df1548a2...364f5ccc, including the full new file apps/rocm/src/uninstall_gate.rs (1387 lines), the full diffs of apps/rocm/src/uninstall.rs, uninstall_plan.rs, service_launch.rs, services_text.rs, therock.rs, chat.rs, automations.rs, test_support.rs; crates/rocm-core/src/lib.rs, net.rs, proc_lifecycle.rs, automation.rs, config.rs, service_record.rs; crates/rocm-engine-protocol/src/lib.rs; all of apps/rocmd/src/{common,persistence,sandbox,service,watchers}.rs; engines/lemonade/src/{backend_alignment,install,state}.rs and engines/vllm/src/state.rs; tests/e2e-cucumber/features/install_lifecycle.feature and tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs in full; README.md, docs/architecture.md, docs/testing.md, docs/manual-testing.md, Cargo.lock, and both app Cargo.toml diffs.

Read partially: the ~3,600 lines of test additions inside apps/rocm/src/main.rs's mod tests (the non-test code in that file — the new host_port_resolves helper, --help text, and the mod uninstall_gate; wiring — was read in full). I enumerated and scanned every one of the ~98 new test functions by name and read several hundred lines of representative tests in detail (the daemon-identity/port-probe mapping tests, the plan/gate-wiring tests, the partial-removal-failure tests), cross-checking their assertions against the production logic in uninstall_gate.rs and uninstall.rs; I did not read every one of the ~98 test bodies line-by-line. Nothing sampled suggested weak assertions, disabled tests, or a gap between a test's name/claim and what it actually checks.

Did not run: history (git log/git blame on the modified regions) and prior-changes passes were not performed as separate dedicated passes, though the diff's own extensive commentary (e.g. in uninstall_gate.rs and the test names) made the rationale behind most non-obvious choices explicit without needing blame. CI was checked live via gh pr view --json statusCheckRollup against this exact head commit (364f5ccc): all 23 applicable checks (clippy, CodeQL, build-and-test, windows-build-and-test, E2E tests, Coverage, Commit signatures + sign-off, Sphinx docs build, etc.) report SUCCESS; 2 (Regenerate manifests, Skill checks (skillscope)) are SKIPPED. No failing check.

Not reconciled against any prior review discussion on this PR — this was a blind review per the method, and the many existing human/bot reviews on this PR at earlier commits were deliberately not read or reconciled against.