[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 rocm-core grows identity_state_with_observed so one reading of the PID answers both the "could this platform verify?" and "is this still the same process?" questions. A recycled PID is a no-op, an unverifiable one aborts with the tooling intact, and a record whose running flag is false is never signalled at all.

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), used by the gate, rocm serve and rocmd, and now names a runnable command with the model and port.
• 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.

[r0x0r]

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

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

[michaelroy-amd]

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

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

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

Please address both and re-request review.

[rominf]

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

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

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

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

[siloteemu]

🔴 Automated review · pr-review-watcher · e2015fb

Summary

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

🚫 Blocking (must fix before merge)

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

Non-blocking

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

[siloteemu]

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

Summary

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

🚫 Blocking (must fix before merge)

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

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

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

Non-blocking

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

[siloteemu]

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

Summary

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

🚫 Blocking (must fix before merge)

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

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

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

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

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

Non-blocking

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

[siloteemu]

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

Summary

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

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

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

🚫 Blocking (must fix before merge)

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

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

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

Non-blocking

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

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

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

[siloteemu]

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

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

Summary

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

Prior objection status

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

🚫 Blocking (must fix before merge)

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

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

Non-blocking

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

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

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

[siloteemu]

🔴 Automated review · pr-review-watcher · bdf3661

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

Summary

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

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

🚫 Blocking (must fix before merge)

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

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

Non-blocking

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

Test audit (standing focus a)

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

Will this confusion recur? (standing focus c)

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

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

[fredespi]

#299 (comment)

Addressed in 7a1893d (main merged). Flaky wildcard-host test now asserts only on svc-a (25 clean runs of the uninstall subset). Each listed mutation now turns a test red: advice ids per class, full rank order incl. ConfirmTheServicePid, a status-less stop result, the pid in the tick-less reason, a deferred-partner survivor; the two unbounded wait()s use assert_stopped_within_grace. Added tests for the 403 arm, stored key reaching the probe, non-.json filter, starts_with arm, warnings loop, recycled-daemon (!helper_stopped), advice follow-through. Plan wording now derives from the gate's decision and names a live rocmd with no records. README/--help/manual-testing/testing.md state the helper force-stop and the tick-less refusal; lifecycle 10/24/26/27 assert removal state. Comments fixed; shared recorded_pid_identities helper; localhost names both loopback families. Not done: write_file_atomically write/sync-failure and retry-exhaustion branches need a test seam that does not exist. The recovery-writer and engine_start_ticks gaps are covered by main's #499 tests.

[rominf]

🔴 Automated review · pr-review-watcher · 4541c89

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.

Round note (full review of the whole change at 4541c899). This round replaces the change request on 3124f2ef. Every blocking item is marked new or standing.

• Your reply on the df51a02e report (issue comment 6167706877) was read and recorded with a reaction; it asked nothing, so this round is the answer. Your "no test seam exists" point on the atomic writer is weighed under that item below.
• Dropped as resolved (each put to the reviewer after its report was frozen; anchored check, consistent with the blind pass, most confirmed by a mutation run):
  • All five "further branches that stay green when broken" (remedy filter, unwrap_or("unknown"), deferred partner probe, starts_with arm, stored keys): each mutation now turns a test red.
  • uninstall_never_kills_a_daemon_pid_that_was_recycled: the fall-through mutation now fails it.
  • The background-helper stop is now in README, --help, docs/manual-testing.md and docs/testing.md, and a live rocmd with no records gets a plan line.
  • The plan no longer promises a stop for a live tick-less record; it predicts the refusal.
  • The intermittent wildcard-host test: it now asserts only on svc-a; about 23 full-suite runs and 10 subset runs passed.
  • From the standing test items: the .json filter and the 403 arm are now pinned; the rank check covers all eight classes; the tick-less test asserts the id; the unbounded wait() is now assert_stopped_within_grace.
• Would not gate on their own (marked below): the description finding (the squash body is built from commit messages), and the new untested-branch and missing-scenario items. Each is reported in full; the floor for a new test-strength gap is a test that claims coverage and cannot fail.
• Carried to the next round's scope as observations (not findings): whether a_deferred_stopped_record_is_judged_once_its_live_partner_has_stopped shares the ephemeral-port class that made the wildcard test flaky; whether anything proves the stop-pass warnings print before the abort line.
• CI at 4541c899: every reported hosted check passes. No self-hosted GPU or Windows lane appears in the rollup; that is missing evidence, not a defect.

Review — needs work

Full review of the whole change.
Implements EAI-8014 (now ROCMAI-290): uninstall stops what it manages before removing the tooling, or aborts and keeps everything. The ticket also expects "verify their … process trees are gone", which Windows does not do (see Blocking). The description names #241 as an open, unmerged dependency, which is correct.

Blocking

• The plan says "those servers will be stopped before removal" for a run the gate will abort (new; the tick-less case of the earlier plan objection is resolved, this is a different case) — apps/rocm/src/main.rs:13748-13803, apps/rocm/src/uninstall_gate.rs:289-292, apps/rocm/src/uninstall_gate.rs:562-575

  • build_uninstall_plan reads records through load_managed_services, which skips a manifest that does not parse without saying so (main.rs:12477). The plan never calls unreadable_service_manifests, whose only other caller is a test.
  • Run: a temporary Linux test with one live ticked record plus services/bad.json = {. The plan's only service line was "1 of 1 … those servers will be stopped before removal", and the helper note said "it will be stopped". The real stop pass then returned failed=[bad.json RepairTheRecord], stopped=[], and the server was still alive.
  • An already-stopped record whose port still answers as our model is also an abort the plan does not predict. The plan prints "none is recorded as running" for it (main.rs:13770).
  • This is the last line read before confirming. AGENTS.md §3 forbids an outcome claim that is false.
  • Confidence 100 · logic · Fix: one non-destructive pre-flight function (manifests, unprovable pids, already-stopped probes, helper identity). The plan renders its result, and the stop pass calls it again just before acting.

• The gate picks what to stop with managed_service_is_live, so a record reading failed whose recorded engine pid is still alive is never stopped (new) — apps/rocm/src/uninstall_gate.rs:581-583, :610, :637, :641, :654; plan at main.rs:13748-13760

  • refresh_from_engine_state adopts failed from the engine's state file. vLLM writes failed after a best-effort let _ = terminate_process_tree(…) (engines/vllm/src/process.rs:162-164). rocmd writes failed when a wrapper exits non-zero (apps/rocmd/src/service.rs:822).
  • refresh_managed_service_runtime_liveness returns early for non-live statuses without looking at the pids (main.rs:12364).
  • Result: the record is only port-probed. An engine that is still loading, or that holds the GPU with no port, passes the gate and the tooling is removed. If it is listening, the advice says "Every process recorded … is gone" (uninstall_gate.rs:92-98), which is false; rocm services stop would still work.
  • The base already has the right predicate. managed_service_record_is_in_use (main.rs:4147-4152) checks the recorded pids too, and its doc comment describes exactly this case. services remove uses it; the gate uses the weaker twin.
  • Confidence 85 · logic · Fix: select stop and pre-check candidates with managed_service_record_is_in_use, and use it in the plan too.

• A service counts as stopped on its status string, not on the confirmed-stop boolean (standing, re-found blind) — apps/rocm/src/uninstall_gate.rs:662-676; doc claim :521-523

  • stop_internal_managed_service drops the all_stopped bool and re-reads status from disk. The doc of stop_internal_managed_service_confirmed_with (main.rs:11070-11073) says a record that already reads stopped "keeps that status through a stop that confirmed nothing".
  • A supervisor's clean-exit write (rocmd/src/service.rs:822) followed by a TimedOut grandchild therefore lands in report.stopped. Only a survivor that is listening is caught by the later probe.
  • The gate's doc ("only counted stopped when … confirms every recorded process is gone") states the opposite. The code governs, because it is what runs. The restart path already uses the bool (main.rs:11296).
  • Confidence 85 · logic · Fix: call stop_internal_managed_service_confirmed_with and gate on the bool.

• "Whole process tree is force-stopped" is printed and documented, but is false on Windows (new) — apps/rocm/src/uninstall_gate.rs:290-291 (plan note), apps/rocm/src/main.rs:654 (--help), README.md:952-953, docs/manual-testing.md:630

  • On Windows process_tree_pids returns vec![root] (crates/rocm-core/src/process.rs:271-273), and send_signal ignores _tree (proc_lifecycle.rs:393-396).
  • The PR's own comments admit this (uninstall_gate.rs:686-688, install_lifecycle.feature:292-294).
  • Confidence 85 · mechanical · Fix: word the claim per platform, or derive it from the platform, or implement descendant enumeration on Windows.

• Five new branches are broken by a single mutation with every test still green (1237 tests ran per mutation) (the first is standing, re-found blind; the other four are new and would not gate on their own)

  • uninstall_gate.rs:200: dropping the process_is_running filter in unprovable_live_pids means a dead tickless pid refuses the whole uninstall.
  • main.rs:13776: the partial-refusal suffix "; the others will be stopped before removal" is never asserted.
  • uninstall_gate.rs:255: the unreadable runtime-state.json plan warning can be removed (Err arm → None).
  • uninstall_gate.rs:288: NothingOfOurs can be made to print "will be stopped".
  • uninstall_gate.rs:271: the !state.running guard can be dropped, so the plan promises to stop a helper the stop pass leaves alone.
  • Confidence 100 · logic · Fix: one test per branch that asserts the plan line together with the stop-pass outcome.

• A test name claims the advice "works", but the test only pins strings (standing, re-found blind) — main.rs:30347 the_tickless_pid_advice_names_ids_and_a_manual_action_that_works

  • It asserts that the text contains kill <pid> / Stop-Process -Id <pid>. No test follows the ConfirmTheServicePid, ConfirmTheDaemonPid, RepairTheRecord or StopWhatHoldsThePort advice and shows that a re-run then proceeds.
  • AGENTS.md §3: named remediation must actually clear the condition. removing_the_stopped_record_as_advised_lets_the_retry_proceed (main.rs:29585) is the pattern to copy.
  • Confidence 85 · logic · Fix: rename the test to what it pins, and add follow-the-advice retry tests, at least for the tickless-pid and daemon-pid cases.

• No scenario covers the refusal of a live server or helper recorded without a start time (new; would not gate on its own) — tests/e2e-cucumber/features/install_lifecycle.feature

  • README, --help and docs/manual-testing.md describe this refusal: plan line, non-zero exit, nothing removed. A grep of features/ finds no scenario for it, and none for an unreadable runtime-state.json.
  • AGENTS.md §3: output and exit-code behaviour needs a Gherkin scenario, and a unit test does not discharge that.
  • Confidence 85 · logic · Fix: a lifecycle scenario that plants a tickless live record and asserts the plan line, the non-zero exit, the files kept and the server alive.

• The description misstates the change (standing in part; would not gate on its own, see round note)

  • It claims "write_active_runtime_marker now publishes through the atomic helper instead of remove-then-rename". The diff does not touch it; that code is already on the base (apps/rocm/src/runtimes_cmd.rs:1780).
  • It says the --keep-binaries --keep-data exemption and the helper stop are "covered by unit tests only, not by a scenario". lifecycle-26 and lifecycle-27 cover both.
  • It does not mention the changed relaunch advice in service_launch.rs and rocmd/src/common.rs, or the partial-removal report in uninstall.rs.
  • Confidence 85 · mechanical · Fix: correct the description. The code is fine on these points.

The items below are standing objections from the previous round that this round's blind review did not re-find. After its report was frozen, each was put to the reviewer separately, with your defence where you gave one. Those verdicts are anchored: the reviewer was told what to look for. Only the part that still holds is listed.

• The stop loop keeps stopping services after one fails, and the abort message calls those later stops "before the failure" (standing; anchored check: holds, by reading) — apps/rocm/src/uninstall_gate.rs:653-684, :1095-1104, :545-552
  • The loop still continues after a failure, and uninstall_removal_gate still renders every report.stopped entry as "stopped before the failure and stay stopped". The comment at :545-552 ("decided BEFORE anything is stopped", "pure cost") still contradicts the loop.
  • Fix: break on the first failed stop, or keep the loop and say "stopped during this run".
• The fail-open warnings tell the operator to act "first", but removal goes ahead straight after them (standing; anchored check: holds, by reading) — apps/rocm/src/uninstall_gate.rs:764-771, :818-833, apps/rocm/src/uninstall.rs:99
  • "stop it before re-running uninstall" and "stop whatever holds that port first" reach stderr only after confirmation, removal then follows with no prompt, and a full uninstall leaves no rocm to re-run. They never appear in the plan or a dry run.
  • Your defence (proceeding on an inconclusive probe is deliberate and stated in the warning text) covers the decision to proceed, which is listed as a tradeoff. It does not cover wording that names an action the operator has no window to take.
  • Fix: reword as after-the-fact disclosures, or move these lines into the plan before confirmation.
• Nothing proves uninstall prints the stop-pass warnings (standing in part; anchored check: holds for this part, confirmed by a run) — apps/rocm/src/uninstall.rs:99
  • Replacing write_stop_warnings(&mut std::io::stderr(), &stop_report) with a no-op left every test green. The helper itself is tested; the call site is not.
• The "id-carrying remedies must name their ids" loop cannot fail (standing in part; anchored check: holds for this part, by reading) — apps/rocm/src/main.rs:27749-27762 in every_failure_class_carries_its_own_recovery_advice
  • Every id it checks is a service_id in failed, and the gate's detail line prints every {service_id} ({reason}) (uninstall_gate.rs:1084-1089), so the loop passes whatever the advice says. Per-class naming is pinned separately by each_advice_names_only_the_ids_of_its_own_failure_class, so delete the loop or check the advice text alone.
• write_file_atomically's retry and exhausted-attempts branches are untested (standing; anchored check: holds; your defence accepted for the sync-failure branch only) — crates/rocm-core/src/lib.rs
  • Nothing covers the write/sync-failure cleanup, the AlreadyExists retry, or the exhausted-attempts bail!.
  • "No test seam exists" is fair for a sync_all failure. For the retry and exhaustion branches the workspace already has the seam: the twin write_file_atomically_with in apps/rocmd/src/sandbox.rs takes suffix_for_attempt. Splitting the temp name out the same way lets a test pre-create the names and drive both branches deterministically.
• Fail-closed branches no test exercises (standing in part; anchored check: holds in part)
  • The production KillScope::Tree argument is outside the injected seam (uninstall_gate.rs:398-405), and lifecycle-26's planted helper has no descendants, so Tree and Single are indistinguishable there.
  • Making host_port_resolves always return false left the whole suite green (confirmed by a run).
  • "Publish order" from the earlier wording could not be checked; it is dropped rather than carried.

Non-blocking

• The abort message under-reports what was already changed, and the gate's comments contradict its loop — uninstall_gate.rs:545-552, :653-684, :853-855, :1095-1117

  • The comment says everything that can doom the run is "decided BEFORE anything is stopped" and that any earlier stop is "pure cost". But the helper and service stops, the post-stop probe and the partner probe all abort after stops have happened, and the loop keeps stopping after the first failure.
  • A service dropped from stopped by the post-stop probe has already lost its key (main.rs:11131-11133), and the message does not say so.
  • On Linux the helper's tree kill takes down rocmd-supervised servers, but a TimedOut helper abort still says only "No files were removed."
  • Confidence 85 · logic.

• A stranger serving our model on an old port gets advice that steers the operator to kill it — uninstall_gate.rs:92-98, :853-880, :990-992

  • BlockServingOurModel on an already-stopped record gets StopWhatHoldsThePort ("outlived its supervisor").
  • rocm services remove <id> --yes would clear it, and the 401/403 arm already offers that (RemoveTheStoppedRecord).
  • Related: a stopped record on a specific host is not paired with a live 0.0.0.0 partner by hosts_name_one_listener.
  • Confidence 85 · logic.

• The gate's relaunch advice is a third copy and disagrees with the two twins — uninstall_gate.rs:1095-1106 vs service_launch.rs:198-222 and rocmd/src/common.rs:335-355

  • It omits --require-api-key for loopback key-protected services.
  • "(add --api-key <key>)" reads as required; the flag is optional, and its help prefers ROCM_SERVE_API_KEY to keep the key out of shell history.
  • The two twins agree with each other and match the Serve clap definitions, so they should govern.
  • Confidence 85 · mechanical · Fix: one shared advice builder.

• The helper note gives the wrong reason in the Indeterminate case — uninstall_gate.rs:293-298

  • It says "no start time was recorded" when a start time was recorded but cannot be read now (e.g. OpenProcess fails on Windows).
  • Confidence 85 · mechanical.

• The stop pass loads the service records twice — uninstall_gate.rs:580, :602

  • The unprovable-pid pre-check runs on the first load and the stop acts on the second. A record that turns live in between reaches terminate_verified with ticks=None and is force-killed on an unproven pid.
  • Each load also re-runs the readiness probes.
  • Confidence 70 · logic · Fix: load once.

• Stale or inaccurate comments — in each case the code governs:

  • Start-ticks still described as Linux-only after this PR added Windows support: uninstall.rs:613, :690; proc_lifecycle.rs:50 ("clock ticks since boot"); crates/rocm-core/src/lib.rs:1004-1005; uninstall_gate.rs:463; the expect message in apps/rocmd/src/service.rs:1026.
  • uninstall.rs:104-105 says "binaries before config, data and cache", but the actions are sorted by path (main.rs:13723).
  • main.rs:27871-27872 is garbled and says "there is no services remove", which is false.
  • docs/architecture.md:55-56 still calls uninstall.rs a mechanical relocation, and calls uninstall_gate.rs a "full domain extraction" although its plan half lives in main.rs.
  • Confidence 85 · mechanical.

• "Cache-only" is not what --keep-binaries --keep-data does, and the scenario runs different flags

  • Locations: main.rs:13811-13812, README.md:957-958, docs/manual-testing.md:648.
  • Those two flags also remove config. lifecycle-27 adds --keep-config (lifecycle_steps.rs:1083-1093), so the documented combination is never run.
  • Confidence 85 · mechanical.

• The e2e success steps never check the exit code — lifecycle_steps.rs:1353 then_removal_complete (used by lifecycle-10, -24, -26, -27)

  • last_rc is captured but read only by the refusal step.
  • In lifecycle-27, contains("left running") (lifecycle_steps.rs:1474-1480) also matches the refusal text.
  • Confidence 70 · mechanical.

• Liveness assertions that cannot fail — main.rs:29051, :29054

  • process_is_running on an unreaped child is kill(pid, 0), which succeeds on a zombie. Only the report fields catch a wrong kill (mutations M05 and M11).
  • Confidence 85 · mechanical · Fix: use try_wait()?.is_none(), as sibling tests do.

• Five gate tests are Linux-only with no stated reason — main.rs:28499, :28539, :28653, :28781, :28891

  • They use loopback HTTP and injected stops, like their ungated 401 sibling. As a result the 403 path, the stored-key path and the deferred-partner path run on no Windows lane.
  • AGENTS.md §6 asks for a comment stating why a test is skipped.
  • Confidence 70 · mechanical.

• Plan claims pinned without the resulting state or a cross-reference — main.rs:29634, :29483

  • The behaviour is proven elsewhere (e.g. uninstall.rs a_cache_only_uninstall_leaves_a_live_server_running). AGENTS.md §3 asks the pin to name that test.
  • Confidence 70 · mechanical.

• Three copies of stage-and-publish — crates/rocm-core/src/lib.rs:2022-2186, apps/rocm/src/therock.rs:4708-4844, apps/rocmd/src/sandbox.rs:652-753

  • Only the publish step was single-sourced. The two app copies skip sync_all, and their doc comments say so.
  • Confidence 85 · architectural.

Decisions for the author

• No --force on uninstall — tradeoff
  • For: deleting the record or state file is already a force, just with more friction and more room for mistakes.
  • Against: a flag would bring back exactly the EAI-8014 defect.
  • Fine either way, provided every refusal's advice names the file path or the services remove alternative.
• Keep stopping services after the first failure, or stop early — tradeoff
  • Continuing gives a full report and takes down servers the operator wanted down anyway.
  • Stopping early matches the "pure cost" rationale in the comment.
  • Pick one and make the comment match.
• Windows descendant handling — non-blocking-improvement
  • Enumerate descendants with Toolhelp32 (parent pid + creation time) in process_tree_pids, or use a Job Object at spawn.
  • The post-stop port probe misses a launcher's child that is still loading or holds the GPU with no port.
• Test placement — non-blocking-improvement
  • About 2,880 lines of tests for uninstall_gate.rs were added to main.rs's test module.
  • Child modules can see crate-root items, and test helpers can be exposed as service_launch::tests::runtime_state already is. The gate itself notes the cost at uninstall_gate.rs:808-809.
• Scope — non-blocking-improvement
  • 66 commits and about 6.8k changed lines. The atomic-write single-sourcing and the serve relaunch-advice change could each have been reviewed on their own.

Positive signals

• StopFailureRemedy::advice/advice_rank and StoppedRecordVerdict::blocks are exhaustive matches, so a new variant cannot silently default to proceeding or to no advice.
• stop_then_remove_with injects both the stop and the removal. The ordering guarantee and the "removed / failed and possibly partial / not attempted" report are asserted against files on disk, not return values.
• assert_stopped_within_grace is a bounded try_wait poll, so a missing kill actually fails instead of waiting out the stand-in.
• identity_state_from_probes takes the probes as arguments, which pins the order "liveness before reading the start time".
• probe_hosts tries both loopback families for wildcard binds, and the identity probe reuses whichever host answered.

Deployment notes

• After upgrading, a live server or rocmd started by an older version has no start time recorded, and rocm uninstall refuses until the user stops it by hand. This is documented, but expect support questions.
• runtime-state.json gains an optional daemon_start_ticks. On Windows, start ticks are FILETIME values (about 1.3e17, above 2^53), so a JSON consumer that parses numbers as doubles loses precision. No in-repo consumer is affected.
• ManagedServiceRecord::write and AutomationRuntimeState::write now replace the file instead of writing through it: a symlink at the path is replaced and the old file's mode is not kept.
• http_get_text_with_auth error text changed to "returned HTTP ". No caller matches on the old text.

What this covered

• Reviewed: prw-base 21be0034 (merge-base 666cb34a) … 4541c899.
• Read completely: uninstall_gate.rs, uninstall.rs, every non-code and core hunk with its surrounding functions, and both e2e files' diffs.
• main.rs: the PR's non-test hunks (Uninstall clap doc, host_port_resolves, terminate_recorded_service_pids, build_uninstall_plan, plan_removes_recovery_tooling) with their callers and callees (stop_internal_managed_service*, the liveness refresh, load_managed_services, managed_service_is_live, managed_service_record_is_in_use, the plan/render/remove helpers, the Serve/Services clap), and every added or changed test in its test module.
• Merge 82ded68: --remerge-diff is empty, so no conflict resolution was involved.
• Runs:
  • The rocm/rocm-core/rocmd test builds.
  • A baseline of 50 uninstall-filtered tests: all pass.
  • 14 single-branch mutations over the full rocm binary suite (1237 tests each). One unrelated test fails locally at HEAD: comfyui::source_swap::….
  • rocm-core new tests: 15 pass. One mutation of write_file_atomically's cleanup was caught.
  • A Windows-cfg scratch-crate type check of publish_temp_file/replace_file_windows/process_start_ticks. A full mingw cargo check could not run (ring needs gcc).
• CI: at face value, every hosted check is green. No self-hosted GPU or Windows hardware lanes appear in the rollup, which is missing evidence rather than a defect. Whether @lifecycle scenarios ran in CI was not established.

Did not run:

• Prior-changes pass: earlier PR review comments were outside the reads granted to this run.
• Design worker (Q1–Q5): ran, folded in above.
• History: ran by the driver. It found nothing beyond the twin predicate already reported.
• Agent-instruction and code-comment adherence: ran inside the code workers.
• Fan-out: four independent workers, two at a time. None failed.
• No prompt-injection content was found.

Not reconciled against any PR discussion here.

The driver added the round note, the new/standing tags and gating notes, and the standing items after the italic line in Blocking. The anchored checks are not part of what this review read blind.

[fredespi]

#299 (review)

Pushed 3932470 (main merged, incl. #636). Plan and stop pass now share one non-destructive pre-flight (unreadable manifests, tick-less live records, already-stopped records still answering, helper identity), so the plan predicts the refusal; stop candidates use managed_service_record_is_in_use; the gate acts on the confirmed-stop bool, not the status string; tree-kill wording is per platform (Windows terminates only the recorded process, now said in help/README/plan). Each listed mutation (process_is_running filter, partial-refusal suffix, unreadable-state warning, NothingOfOurs wording, !running guard, host_port_resolves, KillScope::Tree, the write_stop_warnings call) turns a test red; the tickless/daemon advice have retry tests; new lifecycle-28/29 cover a tick-less live server and an unreadable helper state; success steps check exit 0. write_file_atomically has a temp-name seam with retry and exhaustion tests; relaunch advice is one shared builder; comments/docs fixed; description rewritten. Kept as decisions: no --force, stop loop continues after a failure (wording now says stopped during this run), no Windows descendant enumeration, tests stay in main.rs.

[rominf]

🔴 Automated review · pr-review-watcher · aab76d1

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.

Round note (full review of the whole change at aab76d10). This round replaces the change request on 4541c899. Every blocking item is marked new; none of the earlier ones still stands.

• Your reply (issue comment 6189391308) was read and recorded with a reaction. It reported fixes and asked nothing, so this round is the answer.
• Every standing blocker from 4541c899 is dropped. For each one, the blind full review did not re-find it. After its report was frozen, the objection was put to the reviewer with your defence. These are anchored checks, and most were confirmed by mutation runs over the 1290-test rocm suite:
  • The plan now predicts the refusal for an unparseable record and for a stopped record still serving.
  • Candidate selection uses record_needs_stopping, which includes managed_service_record_is_in_use. Reverting it fails a test.
  • The gate acts on the confirmed-stop bool. Ignoring it fails a test.
  • The tree-kill wording is per platform in the plan, --help, README and manual-testing. Only internal comments still say "whole process tree" unqualified.
  • Mutations (a)–(f) and the host_port_resolves and KillScope mutations now turn a test red. The (b) suffix is gone.
  • The ids loop is gone, and id naming is pinned elsewhere.
  • The atomic writer has a temp-id seam with retry and exhaustion tests.
  • The abort says "stopped during this run".
  • The fail-open warnings now read as after-the-fact disclosures.
  • The tickless and daemon advice have follow-the-advice retry tests.
• Residue of those objections, not gating, carried as observations:
  • No follow-the-advice test exists for RepairTheRecord or StopWhatHoldsThePort.
  • No test shows a helper descendant actually dying (also below as non-blocking).
  • The fail-open warnings still reach stderr only after confirmation (also below).
  • A record whose status is literally stopped while its pid is alive is only port-probed, and its "Every process recorded … is gone" advice was not re-verified.
  • The Unix test seam stop_managed_services_with still infers "confirmed" from status == "stopped".
• What gates this round: four new items, marked below.
  • The wildcard dual-family fail-open.
  • The helper plan line that can promise a stop on an aborting run while the tests stay green.
  • A test message claiming a regression it cannot catch.
  • The shared-builder claim. Commit 14f17e6's message says "relaunch advice comes from one shared builder", and a squash carries that onto main.
• Would not gate on their own (each marked): the other new test-coverage items. For a new test-strength gap the floor is a test that claims coverage and cannot fail.
• CI at aab76d10: every reported hosted check passes. Self-hosted GPU and Windows lanes are not in the rollup; that is missing evidence, not a defect.
• Coverage line corrected at the reviewer's request: in What this covered, the reviewer asked for its garbled range to be corrected to 12342b3…aab76d10, and that is the only change made to its report text.

Review — needs work

Full review of the whole change.
Implements what EAI-8014 asks for (the tracker now files it as ROCMAI-290): stop managed services before removing the tooling, abort non-zero if they cannot be stopped, and keep the recovery state. One fail-open below breaks the ticket's "never report uninstall complete while a managed endpoint is still serving". The description overstates one shared-builder claim.

Blocking

• A wildcard record whose own model still serves on ::1 passes the gate silently when an unrelated listener holds 127.0.0.1 on the same port (new; gates) — apps/rocm/src/uninstall_gate.rs:847-850

  • probe_records_for_survivors uses candidates.iter().find(..) and identity-probes only the first loopback family that accepts a connection.
  • The probe_hosts doc says the caller blocks if either family answers, and the description says "A wildcard bind is probed on both loopback families". Neither holds for the identity probe.
  • A temporary test bound ::1:P serving the record's model and 127.0.0.1:P serving another model, with a stopped record on host ::. The report came back with nothing failed and no warnings, so the removal proceeds.
  • The same mechanism makes an_ipv6_only_wildcard_engine_is_not_waved_through (main.rs ~28150) depend on no concurrent test binding the same v4 port.
  • Confidence 100 · logic · Fix: identity-probe every reachable candidate, block if any of them blocks, and add the dual-family test.

• The plan's helper line can promise a stop the run will not perform, and three of its branches are untested (new; gates: the helper-refusal tests stay green while the plan says "will be stopped before removal" on a run that aborts) — apps/rocm/src/uninstall_gate.rs:361-362, :374-377, apps/rocm/src/main.rs:13744

  • Each mutation below left the full rocm bin suite green (1284 passed):
    • deleting the (HelperDecision::Stop { .. }, true) if run_refuses arm;
    • dropping || services_read_error.is_some() from run_refuses;
    • making the (UnreadableState | Unverifiable, true) arm return "will be stopped before removal".
  • In each case the plan, which is the last thing read before confirming, says the helper "will be stopped before removal" on a run that aborts first.
  • AGENTS.md §3 requires a printed claim about what the CLI will do to be asserted together with the behaviour.
  • Confidence 100 · logic · Fix:
    • Stage a verified live helper plus a refusing record (once with an unreadable services directory, once with a tickless pid).
    • Assert that the plan says "left running" and never "will be stopped", and that the helper is still alive after the stop pass.
    • In the two unverifiable/unreadable-helper tests, add !plan.warnings.iter().any(|w| w.contains("will be stopped")).

• The test that claims a doomed run stops nothing cannot fail on the helper-failure branch (new; would not gate on its own) — apps/rocm/src/uninstall_gate.rs ~743

  • This is the early return after a failed stop_background_helper_before_uninstall. Deleting it left every uninstall test green.
  • a_doomed_run_stops_nothing_on_its_way_to_the_abort reaches the abort only through the pre-flight (unreadable state).
  • The helper termination cannot be injected through stop_managed_services_confirmed_with, so no test can stage this path.
  • Confidence 100 · logic · Fix: make the helper termination injectable in the confirmed stop pass, return TimedOut, and assert the per-service stop is never called.

• The "Not attempted: …" part of the partial-removal error is never produced by any test (new; would not gate on its own) — apps/rocm/src/uninstall.rs:230-238, :271-275

  • Forcing untouched_part to always be empty left the suite green (1284 passed).
  • Every partial-removal test fails on the last or only step, so untouched is always empty and the self_delete "(the running program)" entry is never seen.
  • a_directory_removal_that_deletes_some_children_then_fails_is_reported_as_partial asserts !contains("Still present"), a string no code produces (uninstall.rs:1690).
  • Confidence 100 · logic · Fix: a three-entry plan that fails in the middle; assert that the later entries are named under "Not attempted" and still exist on disk.

• The claim that endpoint keys were dropped is pinned by wording only (new; would not gate on its own) — apps/rocm/src/uninstall_gate.rs:961-983, :1213-1225

  • Forcing key_dropped to false left the suite green.
  • The abort note ("Stopping them dropped their endpoint keys…") is asserted only against a hand-built report (uninstall_abort_says_which_services_it_already_stopped). No test checks that the key is actually gone after a real stop followed by an abort.
  • This is the AGENTS.md §3 outcome-claim rule.
  • Confidence 85 · logic · Fix: stop a keyed service for real, force the abort, and assert both the message and that endpoint_keys::endpoint_api_key returns None.

• The lifecycle scenarios that stop a server never assert the lines saying it will be or was stopped (new; would not gate on its own) — tests/e2e-cucumber/features/install_lifecycle.feature:143-153, :309-319; tests/e2e-cucumber/tests/e2e/lifecycle_steps.rs:1509

  • lifecycle-10 and lifecycle-24 do not assert the plan's "those servers will be stopped before removal" (main.rs:13781) or the result "stopped 1 managed service(s) before removal" (uninstall_gate.rs:1295).
  • lifecycle-26 does not assert "stopped the background helper (rocmd)".
  • These lines are pinned only by unit tests on hand-built reports, and those tests carry no comment pointing at a behaviour test, which §3 requires when wording is pinned alone.
  • Confidence 85 · mechanical · Fix: add Then steps asserting those lines next to the existing process-exit and file-state steps.

• A test message claims a regression it cannot catch (new; gates) — crates/rocm-core/src/proc_lifecycle.rs:110, :563-598

  • a_dead_process_is_never_read_for_a_start_time drives the private identity_state_from_probes with stub closures. Its message says it fails if the read "is back in argument position" in identity_state.
  • Rewriting identity_state to read the start time eagerly (let early = process_start_ticks(id.pid); move || early) left cargo test -p rocm-core --lib -- proc_lifecycle green (18 passed).
  • A check claiming a branch it cannot fail on is worse than no check. The production code itself is correct.
  • Confidence 100 · mechanical · Fix: reword the test and the doc comments to claim only the helper's internal ordering, or route the public identity_state through a seam the test can drive.

• The description and the protocol doc claim the uninstall gate uses the shared relaunch builder; it does not (new; gates: commit 14f17e6's message makes the same claim and lands on main) — apps/rocm/src/uninstall_gate.rs:1216-1223, crates/rocm-engine-protocol/src/lib.rs:55, apps/rocmd/src/common.rs:336-351

  • The description says the builder is "used by the gate, rocm serve and rocmd". relaunch_command's doc says "One builder for the three places that print it, so they cannot drift".
  • In fact the gate hand-writes rocm serve <model> --host <host> --port <port>… and shares only RELAUNCH_KEY_CHOICE.
  • There are four builder call sites (two in service_launch.rs, two in rocmd/common.rs). The rocmd copy omits RELAUNCH_KEY_CHOICE, so its message already differs from the rocm serve one.
  • The rocmd public-host branch test asserts only "without authentication", so swapping that branch's require_api_key argument would stay green (traced, not run).
  • This governs both ways: the code is what ships. Either route the gate through the builder or correct the description and the doc comment.
  • Confidence 85 · architectural · Fix: build one relaunch line per stopped service from its record with relaunch_command(host, requires_key), and assert the public-branch command in rocmd.

Non-blocking

• The plan discards the fail-open warnings the pre-flight computes. apps/rocm/src/main.rs:13735-13736, apps/rocm/src/uninstall_gate.rs:669-670

  • The pre-flight's doc says the plan gets "the fail-open disclosures it would make". build_uninstall_plan uses only preflight_refusal_line and drops report.warnings.
  • So warnings such as "does not resolve", "empty model list" and "unidentified listener" reach stderr only after the operator has confirmed, and they say "this uninstall does not wait for you to".
  • Confidence 85 · logic · Fix: add report.warnings to plan.warnings, or correct the doc.

• The plan omits an unverifiable or unreadable helper when an earlier pre-flight group refuses first. apps/rocm/src/uninstall_gate.rs:361-362, :631-662

  • The comment says these cases are "Named by the refusal line", but preflight_with_records returns before reaching helper_refusal when it finds a bad manifest, a tickless pid or a survivor.
  • The operator learns about the helper only on the retry.
  • Confidence 80 · logic · Fix: collect the helper refusal whatever earlier group refused, or emit the note when the refusal line lacks it.

• Tests the comments call platform-neutral are gated to Unix, so the required Windows lane runs no unit test where the real stop kills a live server. apps/rocm/src/uninstall_gate.rs:718, plus five tests carrying "nothing here is Linux-specific … but no Windows lane has run it yet"

  • Those five are a_forbidden_answer_on_a_stopped_records_port_blocks_like_an_unauthorized_one, the_stored_endpoint_key_reaches_the_identity_probe, serving_the_same_model_again_after_a_stop_does_not_doom_the_uninstall, a_stopped_record_spelled_with_a_wildcard_host_still_waits_for_its_live_partner and a_failed_live_partner_is_not_named_twice_through_its_stopped_twin.
  • stop_managed_services_with is #[cfg(all(test, unix))] with no stated reason.
  • lifecycle-24 is the only Windows exercise of the stop. No Windows twin exists for the helper scenario (26) or the tickless scenario (28).
  • Confidence 80 · mechanical · Fix: ungate the HTTP-only tests and the seam, use a Windows stand-in process, and add a Windows twin of lifecycle-26.

• The kill-breadth wording is not asserted. apps/rocm/src/uninstall_gate.rs:250-257

  • tree_scope_words() produces "its whole process tree" / "only the recorded process", the same claim --help makes. Swapping the Linux string for the Windows one left the suite green.
  • the_helper_is_stopped_with_the_whole_tree_scope pins only the KillScope value, so no test shows a descendant dying.
  • Confidence 85 · mechanical · Fix: assert the words appear in the plan note, and add a Linux test with a grandchild (sh -c 'sleep 60 & wait') that must die.

• Remediation commands are pinned as text, not parsed.

  • The rocm serve … --allow-public-bind --require-api-key advice and rocm services stop|remove <id> --yes exist (traced against clap), but no test parses them through Cli::try_parse_from.
  • removing_the_stopped_record_as_advised_lets_the_retry_proceed calls the function directly rather than going through the CLI.
  • Confidence 85 · mechanical.

• a_deferred_stopped_record_is_judged_once_its_live_partner_has_stopped is timing-sensitive (main.rs ~28420).

  • It asserts stopped == ["svc-b"], which depends on nothing binding ::1:P. Its sibling avoids that assertion for exactly this reason.
  • Confidence 70 · mechanical.

• The plan's pre-flight Err arm is effectively unreachable and untested (main.rs:13734-13740).

  • services_read_error is already set by the same read failures, so the arm runs only if the two reads race. Neutering it left the suite green.
  • Confidence 70 · mechanical · Fix: drop the arm or fold it into the read-error path.

• The atomic-write staging now exists three times and the copies differ. crates/rocm-core/src/lib.rs:2023, :2144; apps/rocm/src/therock.rs:4708, :4760; apps/rocmd/src/sandbox.rs:652, :700

  • Only the publish step was single-sourced. The rocm-core copy calls sync_all and retries 128 temp names; the other two do neither. The new doc comments state the difference.
  • Nothing reaches the write_all/sync_all error-cleanup arms (lib.rs:2174-2187).
  • Reverting vLLM write_state (engines/vllm/src/state.rs:195) to plain fs::write left rocm-engine-vllm green (98 passed).
  • The ReplaceFileW move itself was checked to be identical to base logic.
  • Confidence 75 · architectural · Fix: give the rocm-core helper the suffix_for_attempt/before_publish seams and delete the local copies.

• The expected-model selection is duplicated. crates/rocm-core/src/net.rs:881-897 vs :858-874

  • The fallback to model_ref and the empty-URL bail in the new copy are untested. The bail is unreachable from the only caller, which always rebuilds the URL.
  • Confidence 75 · mechanical.

• A Windows test leaves a ping -n 30 grandchild running after child.kill() kills cmd (crates/rocm-core/src/proc_lifecycle.rs:541-556).

  • Confidence 70 · mechanical · Fix: spawn ping directly.

• Docs disagree with the code and with each other.

  • docs/manual-testing.md:660 quotes "will be force-stopped", which the CLI never prints. The real line reads "will be stopped before removal, and the kill covers …".
  • README.md:973-985, --help (main.rs:659-668) and manual-testing do not mention the refusal on an unreadable helper state.
  • The same surfaces say "nothing is removed" on refusal without saying that an abort after the stops leaves those services and the helper stopped with their keys dropped.
  • docs/architecture.md:55-56 gives uninstall_gate.rs no extraction type, and says storage.rs reuses build_uninstall_plan, which it does not.
  • Confidence 85 · mechanical.

• Smaller test-hygiene points in lifecycle_steps.rs.

  • :1679 — the "left running" match also matches the helper notes; assert "so those servers are left running".
  • :1651 — cites a scenario by number in the same PR that renumbered them; cite the @id instead.
  • :618 — the stand-ins inherit the harness's stdio; use Stdio::null().
  • :687, :1700 — the start time is spliced into e2e-mock.json after it is written instead of being an option field.
  • Confidence 70 · mechanical.

• The pull request does several separable things. Beyond the stop gate, it:

  • single-sources the atomic-write publish and moves vLLM state writes onto it;
  • changes how http_get_with_auth reports a response status;
  • adds Windows GetProcessTimes start times, which changes every Windows rocm services stop;
  • changes the rocm serve/rocmd relaunch wording.

  All of it is described, but at roughly 7,900 added lines the uninstall gate cannot be reviewed apart from them. Splitting out the supporting work would make each piece reviewable.

Decisions for the author

• A stop pass that aborts partway is not transactional — tradeoff
  • Services stopped before the abort stay stopped, and their endpoint keys are gone.
  • The other order would remove tooling while servers run, which is the bug. The abort message says so, and the pre-flight shrinks the window.
  • Confirm that is the intended contract, and say it in README/--help (see the docs item above).
• The survivor probe sends the stored endpoint key to whatever now listens on a stopped record's old port, including during --dry-run — tradeoff
  • It makes an auth refusal provable rather than guessed. Its own comments say that port can belong to a stranger, and the key is the one a restart reuses.
  • Probing without credentials first, and sending the key only on a 401, would narrow that.
• Records are loaded once, in the pre-flight — non-blocking-improvement
  • A service started while the live helper is still up during the stop (up to its 10 s grace) is never stopped, and its record is then deleted with the data directory.
  • Re-loading after the stops, and failing on any record that needs stopping but was not attempted, would close that.

Positive signals

• The plan and the stop pass share one pre-flight function, so a run that refuses has stopped nothing before it aborts.
• StopFailureRemedy::advice/advice_rank and StoppedRecordVerdict::blocks are exhaustive matches, so a new variant cannot silently default to "proceed".
• assert_stopped_within_grace replaces an unbounded wait() that passed whether or not anything killed the child.
• No test signals a process it did not spawn.
• The lifecycle steps assert real state (live child via try_wait, files present or absent), and a Drop guard cleans up.
• The advice-following tests carry out the printed remedy and re-run uninstall.

Deployment notes

• Live managed services recorded by an older version have no start time. On Windows that is every one, since start times were not read there before. Uninstall refuses them until they are stopped by hand: rocm services stop still works for them through the legacy best-effort path.
• A rocmd started by an older version has no daemon_start_ticks, so uninstall refuses until that helper is confirmed and stopped. Old state files still deserialize (#[serde(default)]).
• On Windows, rocm services stop can now return Recycled/Indeterminate where it always matched before. A process whose creation time cannot be opened (for example an elevated one) is left alone rather than killed.

What this covered

Read:

• Every file in the change, 12342b3…aab76d10 (the merge-base with prw-base fee201c, through head).
• uninstall_gate.rs, uninstall.rs and the main.rs diff completely. service_launch.rs and therock.rs through their diffs and surrounding code. The rocm-core, rocmd, protocol and vLLM changes completely. The feature, steps and docs completely.
• Fan-out was four independent workers in two waves of two.

Ran:

• The rocm bin suite at head (1284 passed) and about 13 single-branch mutations across rocm, rocm-core and rocm-engine-vllm.
• The Linux lifecycle e2e (19/19 passed, plus a mutation run).
• feature_naming, and a scratch-crate type and clippy check of the cfg(windows) start-time and ReplaceFileW code.
• CI at head is green on every reported check, including windows-build-and-test and E2E tests. The self-hosted GPU/Windows lanes are not in the rollup, which means missing evidence, not a defect.

Did not run, and why:

• The full Windows-target cargo check/clippy: ring needs x86_64-w64-mingw32-gcc, which is not installed.
• The Windows-only unit tests and the Windows lifecycle scenarios (11-24, 31): Linux host.
• The design review (Step 7) was investigated by the driver from the workers' results, not by separate workers, because of the two-worker cap and the time budget.
• The history pass was light. The prior-changes pass covered only #636's review threads.
• Ticket EAI-8014 was read by its ID (now ROCMAI-290; title, body, status).

Not reconciled against this PR's review discussion.

The driver added the round note and the new/gating tags, and corrected the coverage range at the reviewer's request. The standing-objection verdicts in the round note are anchored checks, not part of what this review read blind.

[fredespi]

#299 (review)

Pushed c6e3107 (main merged). Gating: (1) the survivor identity probe now probes every reachable loopback host and blocks if any does; dual-family test added (probing only the first host turns it red, as does the IPv6-only wildcard test, which no longer needs a free v4 port). (2) The plan's helper line says 'left running' on aborting runs and 'will be stopped' only on proceeding runs; the helper refusal is recorded even when an earlier group refuses; helper termination is injectable, with tests for the three branches (each mutation red) and a timed-out helper stop asserting no per-service stop runs. (3) proc_lifecycle test reworded to claim only the helper ordering. (4) The abort's relaunch line now goes through relaunch_command per stopped service, and rocmd's copy carries RELAUNCH_KEY_CHOICE with the public-host command asserted. Also: 'Not attempted' covered by a three-entry middle-failure test; a real stop plus abort test asserts the key is gone; lifecycle-10/24/26 assert the plan and result lines; plan carries pre-flight warnings and names the helper; docs updated. Decisions: the abort is not transactional by design (services stopped before it stay stopped with keys dropped; now stated); the stored-key probe and load-once points are noted for a follow-up; atomic-write staging dedup, expected-model dedup and PR splitting are deferred.

[fredespi]

Status of the automated CHANGES_REQUESTED review at b062582d (2026-10-07). Head is now f88d72cb; each finding is addressed on the branch, so a maintainer can dismiss it. Comment only, no new review requested.

Blocking

1. Untested branches: daemon-stop failure, per-class advice dedup and the "No files were removed." tail in 9606c37f; ProceedUnidentified warning in 824a3eb5; 403 arm, stored key reaching the identity probe and the non-.json filter in 46241387; the stderr warning writer (write_stop_warnings, test in uninstall.rs).
2. Windows start-time capture: #[cfg(windows)] tests (own pid readable and stable, None for an impossible pid, differs per process) in 9606c37f; the recorded supervisor start time in a361d947.
3. Liveness assertions that pass whether or not the stop ran: now poll the child instead of kill(pid, 0) (32fc5197), stop waits bounded (8ec977c5).
4. Tautological assertions: advice, rank order and fail-closed arms pinned (8ec977c5); one advice per class (6555f0e1).
5. Description: rewritten with the real scenario list and omitted items (77208854).

Non-blocking taken: stale Windows start-time comments (1edde9cb, 32fc5197); cache-only plan warning conditioned on plan_removes_recovery_tooling (43fbb588); daemon reported apart from services (824a3eb5); conflict with base resolved by merging main (latest f88d72cb).

Not taken, deliberately: splitting out the write_file_atomically consolidation (one logical change per PR) and moving the gate tests out of main.rs; both are open for a maintainer to call.

CI: the only red check is the Windows E2E diagnose-fix-path-prepends-with-apostrophe-intact. It fails identically on main at 07166450, introduced by #691, and is unrelated to this change.

[rominf]

🔴 Automated review · pr-review-watcher · f88d72c

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.

Round note (full review of the whole change at f88d72cb). This change request replaces the one on aab76d10. Every blocking item below is new; none of the earlier ones still stands.

• Your two replies (issue comments 6193133024 and 6229851028) were read and recorded with a reaction. Both reported fixes and asked nothing, so this round is the answer. The CI claim in 6229851028 was checked and holds (see CI below).
• Every standing blocker from aab76d10 is dropped. The blind full review did not re-find any of them. After its report was frozen, each objection was put to the reviewer together with your defence. These are anchored checks, settled by mutation runs:
  • Dual-family probe. probe_records_for_survivors now identity-probes every reachable loopback address. Probing only the first one turns a_stranger_on_one_loopback_family_does_not_hide_the_record_serving_on_the_other and an_ipv6_only_wildcard_engine_is_not_waved_through red.
  • Helper plan line. Each of the three named mutations now fails at least one test.
  • proc_lifecycle test message. It now claims only the helper's ordering, and no test claims the eager-read regression.
  • Shared relaunch builder. The gate, rocm serve and rocmd all build through relaunch_command, so the claim in 14f17e6 and 9c40b3b is true of what lands. Swapping rocmd's public-host argument turns a test red.
• Residue, carried as observations, not gating:
  • Both dual-family tests skip and pass on a host without IPv6.
  • The helper-refusal append at uninstall_gate.rs:637-640 is fixed in code but untested. It is also listed below.
  • every_wildcard_bind_spelling_is_probed_on_both_loopback_families pins only probe_hosts.
• What gates this round: two new items, marked below.
  • Three em dashes in new user-facing literals. Main's ASCII-output contract rejects them.
  • The abort message says each relaunch command has "its own model and port filled in", but it prints <model>/<port> placeholders. The only test reads back the list it built itself.
• Would not gate on their own (each marked):
  • The merge conflict. GitHub already refuses to merge a dirty branch, but a rebase and a fresh CI run are needed.
  • The untested branches. No test claims to cover them. For a new test-strength gap, the floor for gating is a test that claims coverage and cannot fail.
  • The stale rocmd Windows comment.
  • The Co-Authored-By trailers. These are an attribution footer rather than a false claim, and whoever merges can drop them from the squash body.
• CI at f88d72cb:
  • The only red check is E2E tests (Strix Halo, Windows), with one unexpected failure, diagnose-38. Three more failures are expected xfails.
  • Main failed the same scenario the same way (run 39220155843, 2026-10-19) until main fixed it. This branch predates that fix, so it does not gate.
  • Regenerate manifests and Skill checks (skillscope) were skipped.
  • Every check ran against the old base. A rebase will re-run them.

Review — needs work

Full review of the whole change.
EAI-8014 now resolves to ROCMAI-290 ("rocm uninstall reports success while leaving a publicly-bound GPU-holding server running…"), status In Progress. This change implements that ticket's three expectations, with fail-open exceptions for records that were already stopped. The PR does not claim to close the ticket.

Blocking

• The branch does not merge with current main (new; would not gate on its own: GitHub already blocks merging a dirty branch) — apps/rocm/src/main.rs, crates/rocm-core/src/lib.rs

  • GitHub reports mergeable_state: dirty.
  • git merge-tree prw-base HEAD shows content conflicts in both files.
  • On main, fcc646e (#666) moved ManagedServiceRecord and AutomationRuntimeState out of lib.rs. The new daemon_start_ticks field and the atomic write() bodies must be re-applied in the new modules. A resolution that keeps main's side silently loses the atomic writes.
  • Every green check is against the old base 03cdbcc, 23 commits behind main.
  • Confidence 100 · architectural · Fix: rebase onto main. Then grep that the moved write() methods still call rocm_core::write_file_atomically, and re-run CI.

• Three new user-facing messages contain em dashes, which main's ASCII-output contract rejects (new; gates) — apps/rocm/src/uninstall_gate.rs:95, :983, :994

  • Main added xtask/src/ascii_output_contract.rs in f0d6ce3 (#683).
  • I ran that test against this tree. It names exactly these three literals: the StopWhatHoldsThePort advice and the two "already recorded stopped — proceeding" warnings.
  • Everything else it flagged comes from main-side conversions this branch predates. These three are new in this change, so they remain after a rebase and will turn the check red.
  • Confidence 100 · mechanical · Fix: replace — with ASCII (- or ;) in all three.

• The abort message says each relaunch command has "its own model and port filled in", but it prints placeholders (new; gates: a printed remediation claim that is false, pinned only by a test that reads back its own input) — apps/rocm/src/uninstall_gate.rs:1284-1291, :796-800, crates/rocm-engine-protocol/src/lib.rs:61-73

  • relaunch_command(host, require_api_key) returns rocm serve <model> --host {host} --port <port>…. The record's model and port are never passed in.
  • The PR description repeats the claim: "now names a runnable command with the model and port".
  • Nothing tests this per-record construction. When I mutated :799 to ignore the record's host and key, the full rocm binary stayed green.
  • the_stopped_service_advice_names_a_serve_command_that_restores_a_public_bind (main.rs:28722) builds the relaunch list itself and reads its own input back, so its name claims more than it checks.
  • This is AGENTS.md §3: remediation advice pinned as a string, never checked against behaviour.
  • Confidence 85 · mechanical · Fix: either pass record.model_ref and record.port (and --engine where needed) into the builder, or drop "filled in" from the message and the description. Then drive a real stop with a public, keyed record and assert the abort carries --allow-public-bind --require-api-key.

• Five branches the change introduces have no test (new; would not gate on its own: no test claims to cover them). Mutating each one alone left the full -p rocm --bin rocm binary green (1327 tests), and the e2e lifecycle steps would not catch them either.

  • uninstall_gate.rs:637-640: the pre-flight adds the helper refusal. Dropping it removes rocmd from the plan whenever a run already refuses for a service reason.
  • uninstall_gate.rs:183-186: recorded_pid_identities prefers a known start time when the supervisor and engine share a pid. Without it, both the gate and terminate_recorded_service_pids (main.rs:10572) refuse that service.
  • main.rs:13358: the self_delete arm of plan_removes_recovery_tooling (Windows).
  • uninstall.rs:234: the running executable in the "Not attempted" list (Windows).
  • main.rs:13276: the plan line for a pre-flight error.
  • More broadly, 52 of the 97 new uninstall tests are cfg(unix) or Linux-only. Lifecycle-25 to 29 are @requires-os:linux. On Windows the gate is covered only by lifecycle-24, the happy path, which uses a PowerShell sleep as a stand-in server.
  • These Windows-reachable behaviours are untested on Windows: the tickless refusal (now live there through GetProcessTimes), the root-only helper-stop wording (tree_scope_words), and the survivor-after-stop arm, whose own comment says it exists because of Windows.
  • Confidence 100 · logic · Fix: a unit test for each arm above. Put the self_delete and running-exe cases in platform-neutral tests, which means un-gating the stop_gate_tests run helper.

• A comment this change made false: rocmd's Windows stop says start times are unavailable there (new; would not gate on its own) — apps/rocmd/src/service.rs:309-317

  • It says process_start_ticks "has no /proc to read there and returns None… Closing it needs rocm_core to read a Windows process creation time".
  • This change adds exactly that (GetProcessTimes), so the identity_state gate below the comment can now refuse a recycled PID on Windows.
  • The same stale claim is in the test docs at service.rs:1611 and around crates/rocm-core/src/lib.rs:2808 ("None elsewhere").
  • AGENTS.md §5 requires that behaviour claims stay in sync with the code.
  • Confidence 85 · mechanical · Fix: update all three. Optionally widen the refuted-PID rocmd tests to Windows, which can now set up a refuted identity.

• 41 of the 75 commits carry Co-Authored-By: Claude <noreply@anthropic.com> (new; would not gate on its own: an attribution footer, not a false claim, and the merger edits the squash body)

  • The repository builds the squash body from the commit messages, so these trailers ship to main.
  • AGENTS.md §11: "avoid AI-generated boilerplate footers".
  • Confidence 85 · mechanical · Fix: strip the trailers when rewriting the history or the squash body. Consider writing a clean squash body anyway, since the 75-commit trail includes "address review of aab76d1".

Non-blocking

• The help text and README promise a command that brings the helper back, but none is printed — main.rs:666-668, README hunk, uninstall_gate.rs:1293-1295

  • "those stay stopped… the error lists the commands that bring them back" covers "services (or the helper)".
  • helper_note only says the helper stays stopped "until it is started again".
  • Confidence 70 · Fix: name the command, or narrow the wording.

• partial_removal_note is fixed text — uninstall.rs:265-283

  • A failure on config or cache, which run before the binaries, still says the binaries and service records may be gone and that rocm services stop may no longer work.
  • Confidence 70 · Fix: derive that clause from removed.

• The PATH-undo failure still says "Nothing else was removed…" — uninstall.rs:200-204

  • It now fires after services and the helper were stopped and their endpoint keys dropped, and it prints no relaunch commands.
  • The PATH edit could be checked in the pre-flight, before anything is stopped.
  • Confidence 70.

• False refusal when a stopped record and a live wildcard-bound record share a port under different host spellings — uninstall_gate.rs:585-597, :1071-1074

  • Example: a stopped record on a LAN IP and a live record on 0.0.0.0.
  • The wildcard expands only to loopback addresses, so the two records are not paired. The stopped record is probed in the pre-flight and finds the live server answering with the same model.
  • The run refuses with StopWhatHoldsThePort instead of stopping the live server itself.
  • Confidence 70.

• Two refusals give advice that does not clear the condition

  • RemoveTheStoppedRecord (uninstall_gate.rs:115-121) says "if it is [yours], stop it first", but names no way to do that. rocm services stop has nothing left to kill there.
  • When services_dir cannot be enumerated (uninstall.rs:153), the run exits with a bare anyhow error, without the "uninstall aborted… nothing removed" context the help text promises.
  • Confidence 70.

• The gate reads the start time before checking the pid is alive — uninstall_gate.rs:312-316

  • It calls process_start_ticks and then identity_state_with_observed, the exact order that function's new doc warns can yield Matches after the pid exits and is reused.
  • It cannot cause a wrong kill, because terminate_verified re-checks. It does mislabel the outcome, and it is the only caller of a new public API that invites misuse.
  • Confidence 85 · Fix: use identity_state and make identity_state_with_observed private.

• On Windows, a failed ReplaceFileW can delete both the record and its replacement — crates/rocm-core/src/lib.rs:2074, :2191

  • On ERROR_UNABLE_TO_MOVE_REPLACEMENT the target is already gone, and the cleanup then deletes the temp file holding the new bytes.
  • Service and runtime-state records now go through this path, and a lost record leaves uninstall unaware of a live server.
  • Confidence 70 (from the Win32 contract; not run).

• On Windows the uninstall stop kills only the recorded root process — main.rs:10570-10603, crates/rocm-core/src/process.rs:290

  • The rocmd twin of this stop uses taskkill /T /F, because "the engine runs one level below the recorded PID" (apps/rocmd/src/service.rs:303-308).
  • If a child survives and has already bound its port, the run aborts after the keys are dropped. If it has not bound yet, removal proceeds under it.
  • Confidence 70 (argued from the twin; not run on Windows).

• The vLLM state comment overclaims — engines/vllm/src/state.rs:196

  • It says readers "never see a torn document". The same files are still rewritten in place by rocm_core::merge_json_state_file (net.rs:~666), and lemonade's write_state (engines/lemonade/src/state.rs:205) also writes in place.
  • Confidence 70.

• Gate code and tests live in main.rs — main.rs:8501-8512, main.rs test hunk at 26874

  • host_port_resolves is used only by uninstall_gate.rs but was added to main.rs.
  • About 3,660 lines of gate tests went into main.rs's mod tests; the gate file itself notes they are "far from here" (uninstall_gate.rs:959-962).
  • AGENTS.md §6 asks for new subsystems in their own file rather than growth in main.rs.
  • Confidence 70.

• Test quality

  • Several message-only pins lack the §3 comment naming the test that proves the behaviour: the_tickless_pid_advice_text…, a_helper_only_stop_prints_a_success_line, a_stopped_helper_is_reported_apart…. The proving tests do exist.
  • a_directory_removal_that_deletes_some_children_then_fails_is_reported_as_partial (uninstall.rs:1715) asserts on what its own stub deleted.
  • Confidence 70.

Decisions for the author

• Auto-stop plus a heuristic gate, versus refuse-and-advise — tradeoff

  • For auto-stop: it gives the one-command uninstall the ticket asks for, and it reuses the rocm services stop primitive.
  • Against: roughly half the 1,376-line gate exists only because it kills: helper identity, the partial-stop/relaunch reporting, deferred partners, and the plan's refusal matrix.
  • Against: the paths that proceed without proving the port is free can fail closed cheaply only before anything is stopped. These are an ListsNoModels listener (a loading engine), ProceedUnidentified, an unresolvable host, and a silently refused port. Under refuse-and-advise they could all fail closed. With auto-stop they fire after keys are dropped, which pushes the design toward failing open.
  • Confirm the choice was deliberate.

• Splitting the change — non-blocking-improvement

  • About 8,500 lines bundle several independent changes:
    • the atomic-write consolidation and the windows-sys removal
    • vLLM atomic state writes
    • Windows GetProcessTimes, which changes identity_state for every Windows caller
    • the rocm serve and rocmd refusal-text fixes
    • http_get_with_auth status parsing
    • partial-removal reporting
    • renumbering about 20 existing lifecycle scenarios instead of appending the new ones
  • AGENTS.md §11 asks for one logical change per PR, and several of these could land first on their own.

• The deferred-partner probe (uninstall_gate.rs:865) — non-blocking-improvement

  • It mostly repeats the partner's own survivor probe, and only adds the other loopback family.
  • Narrowing it to the addresses the partner's probe does not cover would remove a post-stop abort path.

Positive signals

• The plan and the stop pass share one non-destructive pre-flight (preflight_with_records), so the plan predicts a refusal instead of promising a stop the run then refuses.
• The stop pass is injected into stop_then_remove_with, so the ordering can be tested: reversing stop and remove turns 7 tests red.
• daemon_identity_outcome treats a missing start time as unverifiable rather than a match, so the gate never force-kills a process tree on an unproven pid.
• The StopFailureRemedy::advice and StoppedRecordVerdict::blocks matches are exhaustive, so a new variant cannot default to "proceed" or go unprinted.
• The new record fields use #[serde(default, skip_serializing_if)], so old and new binaries read each other's files.

Deployment notes

• Older records refuse. A live server or helper started by a version that predates supervisor_start_ticks/daemon_start_ticks makes uninstall refuse rather than stop it. Users upgrading with servers running must stop them by hand before uninstalling.
• Windows stop behaviour changes. Managed-service stops on Windows (in rocm services stop, rocmd and lemonade) now verify the process start time, and can return Recycled or Indeterminate where they always matched before.

What this covered

Every file in the change was read completely, split across four independent workers. Their scopes were:

• uninstall production code
• core, rocmd and engines
• the tests, with mutation checks
• design questions

I also read the callees of the changed symbols and the rocmd twin of the stop path. The diff read was 03cdbcc…f88d72cb; the merge preview and the ASCII check were against main at c7a0d65.

What ran:

• Unit tests: the full rocm binary passed at HEAD (1327 passed, 7 ignored), as did rocm-core (735), rocmd (154), rocm-engine-vllm (98) and the protocol crate (23).
• Mutations: 16 single-branch mutations, plus several more, against the full rocm binary.
• Main's ASCII contract: run against this tree.

CI: the only failing check, "E2E tests (Strix Halo, Windows)", fails on diagnose-fix-path-prepends-with-apostrophe-intact. Main failed the same way (run 39220155843, 2026-10-19) until #710, which this branch predates, so this change did not cause it. The lifecycle-10 and 24–29 scenarios ran and passed in build-and-test (19/19) and windows-build-and-test (15/15).

Did not run:

• The e2e lifecycle scenarios locally; they were read only.
• Any Windows-gated test, and a full Windows cross-check: ring needs mingw gcc, which is not installed. The two Windows functions were clippy-checked in a scratch crate instead.
• Mutations of the probe and verdict logic (stopped_record_verdict, probe_records_for_survivors).
• The prior-changes pass was light: only #636's review comments were consulted.

Not reconciled against any discussion on this PR here.

The driver added the round note and the new/gating tags. The review paused overnight with the session and then resumed. The standing-objection verdicts in the round note are anchored checks, not part of what this review read blind.

[juhovainio]

This closes the primary failure mode in the linked ticket: uninstall_gate.rs stops rocmd and every managed service first, independently re-probes all managed records for survivors (TCP reachability + HTTP identity check, with explicit fail-open disclosure for unresolvable hosts and wildcard binds), and aborts via bail! before any deletion runs if a stop can't be confirmed. That ordering and the "abort leaves everything in place" guarantee are backed by dedicated tests (e.g. a_service_that_cannot_be_stopped_leaves_every_planned_path_in_place), and the coverage is convincing.

The directory-recreation race from the follow-up nightly report ("Directory not empty" after the live service kept rewriting files mid-delete) looks substantially addressed by that same ordering change: deletion no longer starts until liveness is independently confirmed gone, so the window that caused the race should mostly be closed. partial_removal_note reads as a diagnostic fallback for a partial removal that still happens for an unrelated reason, not the mechanism that itself prevents the race — worth confirming that's the intended split.

Requesting changes mainly because:

• The PR currently shows as CONFLICTING / DIRTY against main — needs a rebase before this can land.

Non-blocking, worth a second look before merging:

• Process-identity verification (daemon_start_ticks / IdentityState) fails closed on Indeterminate/tickless pids, which is the right default, but I didn't trace the exact window between reading start-ticks and acting on them end-to-end — worth a quick confirm there's no realistic staleness window there.
• CI: one failing check, Strix Halo Windows E2E (diagnose-fix-path-prepends-with-apostrophe-intact) — this is the known PATH-apostrophe flake that fails identically on main, not a regression from this diff. Everything else, including clippy with --workspace --all-targets -D warnings, is clean.

Nice test coverage on the gate logic overall — the fail-open disclosures in probe_records_for_survivors show real thought about the actual failure modes (wildcard binds, unresolvable hostnames, stale endpoint keys, already-dropped endpoint keys).

[fredespi]

Review - needs work

The stop-before-remove gate addresses the original uninstall failure: it stops the helper before managed services and refuses removal when a stop cannot be confirmed. One remaining issue needs fixing: the error promises recovery commands with the model and port filled in, but prints placeholders. See the inline comment.

Delivered as a COMMENT because the available posting identity is the PR author and GitHub prohibits requesting changes on your own PR; this does not make the issue advisory. Recommendation: fix the recovery advice before merging.

Reviewed f53815d...4dbdb2f and surrounding stop, process-identity, atomic-publication, and lifecycle-test code. This was a code-only review; Linux/Windows tests and runtime behavior were not executed.

[fredespi]

The abort promises a command for each stopped service with its own model and port filled in, but the production construction at lines 796-800 calls relaunch_command(&record.host, record.requires_api_key). That builder always returns rocm serve <model> --host ... --port <port>...: neither the model nor the port is passed. After a partial stop the operator is given placeholders, not the runnable recovery command this message and the PR description promise. The test the_stopped_service_advice_names_a_serve_command_that_restores_a_public_bind constructs report.relaunch itself, so it cannot catch this production defect. Populate the command from the stopped record (including any required engine selection), or explicitly describe it as a template instead of claiming the values are filled in. Test the production stop-report construction with a specific model and non-default port and assert the resulting abort advice.

[fredespi]

Fixed in 01f6510 and d37698a. The abort advice is now built from the stopped record by relaunch_command_for (model, engine, port filled in; the template stays the single flag source), so it no longer prints /. Arguments are quoted for the platform shell (PowerShell doubles apostrophes, POSIX sh closes-escapes-reopens). the_abort_advice_for_a_stopped_service_carries_its_own_model_engine_and_port drives the production stop pass with a specific model and non-default port and asserts the abort text; an_apostrophe_in_a_model_path_is_escaped_for_the_shell_that_will_read_it covers quoting.

[fredespi]

Review - no blocking findings

Re-reviewed the fix at d37698a against 4dbdb2f. The earlier recovery-advice issue is fixed: the production stop pass now supplies the stopped record’s model, engine, host, port, and authentication requirement. The new regression test runs that stop-report construction and asserts concrete recovery advice rather than supplying its own relaunch list.

The POSIX and PowerShell apostrophe escaping is appropriate for each shell; live shell probes preserved a model path containing spaces and apostrophes as one argument. No blocking issues found in this fix.

This looks safe to approve pending final Linux/Windows validation and maintainer review. This is a code-review comment, not a formal approval; I did not rerun the project test suites.

[rominf]

🔴 Automated review · pr-review-watcher · d37698a

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.

Round note (full review of the whole change at d37698a7). This change request replaces the one on f88d72cb.

• Your reply (issue comment 6365328979) was read and recorded with a reaction. It reported fixes and asked nothing, so this round is the answer.
• Standing blockers from f88d72cb, settled. The blind review below did not re-find either one as worded. After its report was frozen, each was put to the reviewer together with your reply. These verdicts are anchored checks, not part of the blind review:
  • Em dashes: dropped. The three literals are ASCII now. every_literal_that_can_reach_a_redirectable_sink_is_ascii passes, and putting an em dash back at uninstall_gate.rs:95 turns it red, naming that line.
  • Relaunch placeholders: dropped as worded. The abort no longer prints <model>/<port>. the_abort_advice_for_a_stopped_service_carries_its_own_model_engine_and_port runs the production stop pass, and mutating the host, key, port, model or engine at the call site each turns it red. Two residues remain:
    • The fixture sets model_ref == canonical_model_id, so the test cannot see the defect in the next bullet.
    • The test is cfg(target_os = "linux"), so the Windows lane does not run it.
  • What now gates is a new defect in the same advice: the second Blocking item, marked below. The relaunch command for a catalog model with a vLLM model_id_override (the ticket's own qwen repro) starts a server that is not the one stopped. I re-checked it against the code: matches_ref matches only canonical_model_id and aliases, and Qwen/Qwen3-4B-Instruct-2507 is neither.
• Would not gate on their own:
  • The PATH-undo ordering. An earlier round rated it non-blocking, and nothing new makes it more than that.
  • The untested vLLM atomic write. No test claims to cover it, and the comment overclaim was rated non-blocking before.
  • The Co-Authored-By trailers. They are still on 41 commits; as before, they do not gate.
  • The stale rocmd Windows comment (standing, not re-found blind). apps/rocmd/src/service.rs:309-317 still says process_start_ticks returns None on Windows. crates/rocm-core/src/service_record.rs:458 makes the same claim.
• Gone since f88d72cb: the merge conflict. GitHub no longer reports the branch as dirty.
• CI at d37698a7: every hosted check is green. Regenerate manifests and Skill checks (skillscope) were skipped. No GPU-lane check appears in the rollup.

Review — needs work

Full review of the whole change.
Implements what ticket EAI-8014 expects: stop managed servers before removing anything, abort non-zero and keep the tooling when a stop cannot be confirmed, and never report success while a managed endpoint still serves. Issue #241, which the description names, is an open PR, so it was not treated as a ticket.

Blocking

• A PATH-undo failure now comes after the servers and the helper were stopped, and the message says "Nothing else was removed" (standing as non-blocking since f88d72cb; does not gate on its own) — apps/rocm/src/uninstall.rs:153-166 (stop_then_remove_writing_to), apps/rocm/src/uninstall.rs:197-204 (execute_plan_with)

  • Order. stop_managed_services() runs first, then execute_plan_with undoes the PATH change.
  • The PATH abort is predictable. It is the lifecycle-33 case, yet nothing checks for it before the stop pass.
  • What the user is told. The run bails with could not undo the installer's PATH change … Nothing else was removed, so you can fix this and run rocm uninstall again.
  • What actually happened. Every live server and rocmd have already been killed, and their endpoint keys dropped (uninstall_gate.rs:1028-1052, 1282-1291). Those keys cannot be re-minted.
  • What is missing. Unlike the gate's own abort, this message has no relaunch commands and does not say the services stay stopped.
  • Rules it breaks.
    • The change's own "stop nothing on a doomed run" rule (541db48, and the pre-flight shared with the plan).
    • AGENTS.md §3: an outcome claim must be asserted together with the resulting state.
  • Tests. None combines a live service with a failing PATH undo.
  • Confidence 85 · logic · Fix: check that the PATH edits can be applied in the non-destructive pre-flight, before anything is stopped. Otherwise, carry the stop report into this abort the same way the gate abort does.

• The relaunch command rebuilds the service from the engine-specific model id, so it does not bring the same service back (new; gates: remediation advice that does not restore what it stopped, AGENTS.md §3) — apps/rocm/src/uninstall_gate.rs:797-805, crates/rocm-engine-protocol/src/lib.rs (relaunch_command_for)

  • Which field it uses. It passes record.model_ref. serve_cmd.rs:584,743 sets that from serve_model_ref_for_engine, which returns the recipe's model_id_override when one exists.
  • Why that id does not resolve. ModelRecipeRecord::matches_ref (model_catalog.rs:237-244) matches only canonical_model_id and aliases, never an override.
  • The ticket's own repro hits this. It runs rocm serve qwen --host 0.0.0.0 --allow-public-bind on vLLM.
    • The record then holds Qwen/Qwen3-4B-Instruct-2507 (model_catalog.json:139).
    • The printed rocm serve Qwen/Qwen3-4B-Instruct-2507 … --engine vllm resolves no recipe.
    • So it either fails or relaunches without the catalog recipe (tool-call parser, recipe hints).
  • Other dropped settings. runtime_id, env_id and device_policy, which the record also holds, are dropped as well.
  • What is claimed. The abort says "the command for each, with its own model and port filled in", and the README says it "brings it back". AGENTS.md §3 requires that advised commands actually clear the condition.
  • Confidence 85 · logic · Fix: use record.canonical_model_id, carry over the runtime and device selections, and add a test that the printed command resolves the same recipe.

• The vLLM engine-state write was made atomic with no test, the description does not mention it, its comment overclaims, and the lemonade twin was left alone (new as a test gap, the comment part standing as non-blocking since f88d72cb; does not gate on its own: no test claims to cover it) — engines/vllm/src/state.rs:194-205; twins at engines/lemonade/src/state.rs:205-211 and crates/rocm-core/src/net.rs:648-671 (merge_json_state_file)

  • No test. A worker reverted write_state to create_dir_all + fs::write, and cargo test -p rocm-engine-vllm --lib still passed (111 passed, 0 failed).
  • Not in the description. It names only ManagedServiceRecord::write and AutomationRuntimeState::write as moved to the atomic publish.
  • The comment overclaims. It says rocmd and rocm uninstall "never see a torn document". But the same file is still patched by merge_json_state_file, which uses plain fs::write (the doc comment right above says so).
  • The twin was not changed. Lemonade's identical write_state and merge_json_state are untouched.
  • Impact if a read tears. refresh_from_engine_state errors; this does not cause a wrong kill.
  • Confidence 100 (the mutation run) · mechanical · Fix: add a reader-holds-old-inode test like the record ones, route the patch writers and lemonade through write_file_atomically (or narrow the comment), and mention the change in the description.

Non-blocking

• The relaunch quoting is incomplete (new) — crates/rocm-engine-protocol/src/lib.rs (relaunch_command, shell_word_for)

  • The host is interpolated unquoted. Records can hold * or [::] (uninstall_gate.rs:1107): bash -c 'echo --host *' globbed, and zsh failed on [::].
  • @ is treated as plain. In PowerShell a bare word starting with @ is splatting; with pwsh locally, & printf "[%s]\n" @g dropped the argument.
  • Typographic apostrophes are not escaped. PowerShell treats U+2018–U+201B as quotes and failed with a missing terminator.
  • A model containing <port>. replacen("<port>") would rewrite text inside the quoted model.
  • Confidence 85 · mechanical · Fix: pass the host through shell_word, drop @ from the plain set (or quote words that start with it), and escape U+2018–U+201B for PowerShell.

• No test parses the filled-in relaunch command with the real argument parser (new) — crates/rocm-engine-protocol/src/lib.rs tests

  • advised_commands.rs scans only the template literal.
  • --engine, the substituted values and the quoting are added with push_str/replacen, so the scanner never sees them.
  • --engine has value_parser = SUPPORTED_ENGINES (main.rs:488).
  • Confidence 85 · mechanical · Fix: shell-split the output of relaunch_command_for and run it through Cli::try_parse_from.

• The new public identity_state_with_observed has one caller, and that caller uses the read order the function's own docs warn against (standing) — apps/rocm/src/uninstall_gate.rs:312-316, crates/rocm-core/src/proc_lifecycle.rs:185-198

  • The caller reads start ticks before checking liveness. The docs say that order can report Matches for a recycled pid.
  • Nothing else uses the observed reading.
  • It cannot cause a wrong kill, because terminate_verified re-checks in the safe order.
  • Confidence 85 · architectural · Fix: call identity_state and drop the extra API, or say why the order is safe here.

• The write_file_atomically doc says every current caller writes non-sensitive state (new) — crates/rocm-core/src/lib.rs:589-594

  • RocmCliConfig::save (config.rs:559) now goes through it, and that file holds DashboardDaemonConfig.token.
  • The "permissions are not carried over" note is also Unix-only: on Windows, ReplaceFileW keeps the replaced file's ACLs.
  • Confidence 85 · mechanical · Fix: correct the doc.

• The docs promise a relaunch command for the helper, but none is printed (standing) — help text at apps/rocm/src/main.rs:~672, README uninstall paragraph, uninstall_gate.rs:1298-1301

  • Both say the services or helper that the stop pass stopped "stay stopped… the error lists the commands that bring them back".
  • The helper note names no command.
  • Confidence 70 · mechanical · Fix: name how to restart rocmd, or limit the claim to services.

• A scenario reference went stale in the renumbering (new) — tests/e2e-cucumber/features/diagnose.feature:575

  • It cites "lifecycle-13's install.ps1 PATH scenario".
  • That scenario is now lifecycle-14, because inserting lifecycle-10 shifted everything after it.
  • Confidence 85 · mechanical · Fix: cite the @id: rather than the index.

• rocmd now fsyncs inside its async tick every 5 s (new) — crates/rocm-core/src/automation.rs:161-168, apps/rocmd/src/service.rs:437-447

  • The tick goes through write_file_atomically, which runs sync_all, so it blocks on the disk inside a tokio task.
  • Confidence 70 · mechanical · Fix: skip sync_all for this runtime-state file, or move the write onto spawn_blocking.

Decisions for the author

• The PR is three changes in one — non-blocking-improvement

  • About 8,700 lines over 60 commits mix three things:
    • the uninstall gate;
    • a workspace-wide atomic-write consolidation (therock.rs, rocmd/sandbox.rs, config.rs, vLLM state);
    • new relaunch wording for the rocm serve and rocmd lost-key refusals.
  • The last two are separable and would be easier to review on their own.

• The quoting assumes PowerShell on Windows — tradeoff

  • shell_word uses cfg!(windows) to mean PowerShell. In cmd.exe, single quotes do not group words.
  • That is defensible as the documented default shell. Confirm cmd users are out of scope.

• Live processes recorded by older versions are refused, not stopped — tradeoff

  • This applies to servers or helpers recorded without start ticks.
  • Safe against pid reuse, but every user who upgrades with a server still running must stop it by hand before uninstall works.
  • It is documented. Confirm that this friction is intended.

Positive signals

• stop_managed_services_then_remove takes the stop pass as a parameter, so the stop-before-remove ordering is checked on planned files still existing after an aborted run. A forced-on mutation of the keep-binaries/keep-data gate made a_cache_only_uninstall_leaves_a_live_server_running fail, which shows the gate is really tested.
• The plan and the stop pass share one non-destructive pre-flight, so the dry run predicts the same refusal the real run makes.
• One relaunch_command builder now serves the gate, rocm serve and rocmd, which removes three drifting copies of the advice.

Deployment notes

• Live servers or rocmd instances started by a version without start-time recording are refused by rocm uninstall until they are stopped by hand.
• windows-sys is dropped from apps/rocm and apps/rocmd; the Windows publish step now lives in rocm-core.
• AutomationRuntimeState gains daemon_start_ticks, which has a serde default, so old state files still parse.

What this covered

What was read: every file in prw-base…HEAD (f53815d5…d37698a7; the merge base equals the base tip).

• Two coupled-group workers covered the uninstall flow and the core/daemon helpers.
• The main.rs test hunks (about 4,100 lines) were read selectively by test name and where they bear on findings; all of them were run.
• The source-reading passes were run by the driver, not by an independent worker: instruction adherence, history, code comments, plus a leak scan and the ASCII contract (cargo test -p xtask ascii: 27 passed).

What was run:

• All rocm bin tests: 1404 passed.
• Targeted rocm-core tests (40), the rocmd start-time test, the protocol relaunch tests, and the rocm-engine-vllm lib (111).
• CI on d37698a: every hosted check is green, including windows-build-and-test and E2E. No GPU-lane checks appear in the rollup.

What did not run, and why:

• The lifecycle scenarios (10, 24–29) were not run locally; CI shows no check named for them.
• No Windows-target check or clippy of the cfg(windows) arms: the mingw toolchain is missing, so the Windows code was read only.
• The prior-changes pass did not run: earlier reviews on these files were not consulted.
• Ticket EAI-8014 was retrieved by its identifier.

Not reconciled against any discussion on the PR. No prompt-injection content was found.

The driver added the round note and the new/standing/gating tags. The standing-objection verdicts in the round note are anchored checks, not part of what this review read blind.