Repository navigation
Backport fixes and docs for v0.2.1 - #753
Merged
Merged
Conversation
* fix(uv): serialize concurrent managed-uv bootstraps Two rocm processes bootstrapping uv on a fresh machine could delete each other's freshly published copy, or share and delete a staging directory named only by the millisecond. Hold a per-version FileLock beside the install dir across download, extract and publish, and re-check for a usable install once it is held, so a waiting process reuses the first one's result. Staging is now named by version, pid and time without with_extension, which dropped the last component of a dotted version. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * docs(services): count the uv bootstrap lock in prune's no-nested-acquire note The note claimed the daemon-autostart lock was the only other FileLock::acquire in the tree; the managed-uv bootstrap now takes one too, on its own lock file and unreachable from prune. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(uv): satisfy clippy in the concurrent-bootstrap test The collect is what makes both bootstraps run at once; say so and allow needless_collect there. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(uv): keep the unix-only bootstrap fixture warning-free on Windows Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> --------- Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> (cherry picked from commit 0a02178) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
#562) * test(deletion): property harness for uninstall and storage remove paths Property tests that build a real sandbox tree (planted symlinks, sibling folders whose names are prefixes of a root, sentinels outside it), snapshot it by (dev, ino), run the real uninstall/remove-downloads plan and removal code, and compare. The known defects are pinned as ignored tests until the fixes that follow land. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(uninstall): unlink a symlinked root instead of emptying its target `remove_path` chose between `remove_file` and `remove_dir_all` with `symlink_metadata`, but a trailing separator (`link/`) or trailing `.` makes the kernel resolve the final link. With ROCM_CLI_CACHE_DIR set to a symlinked folder written with a trailing slash, `rocm uninstall` emptied the folder the link pointed to and then failed on the link with "Not a directory", exiting 1 partway through. Separately, its early return on `!path.exists()` followed links, so a dangling link was skipped yet still reported as removed, both by `rocm uninstall` (a planned link whose target an earlier step deleted) and by `rocm storage remove-downloads`. `remove_path` now respells its argument from its components, which drops trailing separators and `.` segments, and treats only NotFound from `symlink_metadata` as already gone. That covers every caller. The uninstall plan uses the same spelling and existence test, so the review names the entry that is actually removed and lists a dangling-link root instead of calling it "not present". The deletion property tests now include trailing-`/.` spellings and dangling links, and the tests pinned as findings for this are enabled. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(uninstall): a regular file written as a folder is not a root Respelling a root from its components turned `notes.txt/` into `notes.txt`, so ROCM_CLI_CACHE_DIR=/x/notes.txt/ planned and removed a file the kernel would not even resolve: a trailing separator or `/.` only ever names a directory. When the original spelling requires a directory and the entry is neither a directory nor a symlink, it is now not present, both in the uninstall plan and in remove_path. The uninstall property also generates regular-file and dangling-link roots. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * docs(uninstall): claim the symlinked-root behaviour for Linux only Windows directory symlinks and junctions need remove_dir, which uninstall does not do yet, and no test covers them there. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(uninstall): fail the plan when a root cannot be checked The uninstall plan treated any error from checking a config, data or cache folder as "not present": a folder behind a looping link, or one that cannot be read, dropped out of the review while it may still be on disk. Fail the plan instead, naming the folder, so the uninstall stops before removing anything. remove_path already reports these errors. "Not a directory" stays an absence: a path that runs through a regular file (notes.txt/../x) names nothing, the same as "not found". The property harness found that case when only "not found" was treated as absent. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(uninstall): a folder that cannot be checked stops the uninstall Cover the plan-time failure through the built binary: with the cache folder behind a link that loops back on itself, uninstalling the config and the cache exits non-zero, names the cache folder, and leaves the config folder's files in place. Document the behaviour in the README next to the symlinked-root note. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(deletion): point the ignored HOME-guard finding at #531 The ignored property names the gap it pins, and its comment no longer claims a trailing-slash link to HOME deletes HOME: since the symlinked root fix such a link is only unlinked. It shrinks to whichever root names HOME, not specifically the cache. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> --------- Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> Backport note for v0.2.1: the `mod driver_install;` line from the conflicting context is dropped; v0.2.0 has no such module. (cherry picked from commit 0573d7f) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
…all (#600) * fix: report the ComfyUI source folder consistently in status and install `rocm comfyui status` and `rocm comfyui install` each printed a `folder:` line, but they disagreed on what it meant: `install` reported the app root (`<env>/apps/comfyui`), while `status` reported the runtime root (`<env>`). Neither was the actual ComfyUI source checkout that contains `main.py` (`<env>/apps/comfyui/source`). Since `models path:` is already printed as its own line, make `folder:` mean the ComfyUI source checkout (`source_path`) in both commands, so the two labels agree and both point at something a user can actually open and inspect. Tighten the existing `install_dry_run_uses_selected_runtime_folder` and `status_labels_managed_rocm_as_runtime_not_install` tests with exact `folder:` line assertions; their prior substring checks still passed on the old behavior. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * test(comfyui): assert folder: path agreement, fix status fixture Extend e2e scenario comfyui-02 to assert the `install` `folder:` line names the planted source checkout and that `rocm comfyui status` reports the same line afterward -- the Gherkin coverage the pr-review-watcher review on #600 flagged as missing for a user-observable output change. Rebuild the `status_labels_managed_rocm_as_runtime_not_install` fixture's `source_path` from `runtime_app_root`/`source_path_from_app_root`, the same helpers `install()` uses, instead of the test-only, data-dir-rooted `source_path(&paths)` -- so the test can no longer pass on a manifest layout no real install produces. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * test(comfyui): split status-check step into When + Then status_reports_same_folder_line ran `rocm comfyui status` inside a #[then] step, which README.md's "Given = setup, When = action, Then = assertion" convention reserves for the When step -- flagged by the pr-review-watcher review on #600. Split it: a new "When the user checks ComfyUI status" step runs the command and stores its output on E2eWorld, same as the existing "When the user installs ComfyUI" step; the "Then status names the same ComfyUI source folder as the install" step only asserts. Reworded the expected_comfyui_folder_line doc comment, which the same review read backwards. Signed-off-by: Jussi Elo <jussi.elo@amd.com> --------- Signed-off-by: Jussi Elo <jussi.elo@amd.com> (cherry picked from commit 34fbdfe) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
* fix(providers): end a local stream cut off before its final chunk `read_chunked_sse_body` read each chunk-size line with `read_line` and skipped a line that was empty after trimming. At end of stream `read_line` returns `Ok(0)` and leaves the line empty, so a connection closed before the terminating `0` chunk looked like a blank separator and the loop re-read end-of-stream forever. Each read returns immediately, so this was a busy loop: one core at 100%, and the call never returned. The streaming path's only production caller is the smoke test `rocm serve` runs after launching an engine, which only runs when stdout is a terminal. `rocm chat` uses the non-streaming request and is unaffected. vLLM streams with chunked encoding, so an engine that crashed, was OOM-killed or was stopped while answering that first request left `rocm serve` stuck on "Running smoke test…". `Ok(0)` is now end of stream, distinct from a blank line that does contain bytes, and ends the read with "the local provider closed the connection before the final chunk". An end of stream inside a size line, chunk data or a chunk's trailing CRLF reports the same message, with the underlying error kept as context, so every cut point says the same thing. The close-delimited reader also stopped on a single `ErrorKind::Interrupted` instead of retrying; it now retries. The chunked and length-delimited readers already retried, through std's `read_line` and `read_exact`. Covered by properties that re-chunk the real fixture bodies at every boundary (parsing was already chunking-invariant, and still is), cut them at every point before the final chunk, and inject interrupted reads. A reader that panics after 10,000 consecutive empty reads turns a regression into a fast failure instead of a hung test. Two loopback TCP tests drive the real smoke test and the stream call against an engine that closes after one chunk, each bounded at 10 s, waiting on the client before joining the server thread so a future change cannot hang them. Fixes #514 Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(providers): assert the cut-off summary without echoing it The new smoke-test regression test printed the whole rendered deployment summary in its assert messages. The summary struct carries an api_key field (None here), so CodeQL's rust/cleartext-logging rule flagged the echo. Assert on the two rows directly, as the sibling serve_summary tests do. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(providers): name a short Content-Length body as cut off too A `Content-Length` body that ended early still failed with "failed to read local provider SSE body: unexpected end of file", while a chunked one said the engine closed the connection. Both framings declare where the body ends, so an end of stream before that point is the same event; the message now names it the same way in both and no longer mentions chunks. The framing dispatch moves into `read_sse_body` so the tests drive the same call sites as the live client. The close-delimited framing keeps returning success at end of stream, now with a comment saying why: there the closing connection is the terminator, and only a required `[DONE]` could tell a finished answer from a cut-off one, which would be a policy for every engine rather than a transport fix. New properties pin both sides of the relabelling: a `Content-Length` body cut anywhere reports the truncation, and a stream that stalls until the read times out, in either framing, keeps the timeout and is never reported as a closed connection. The two loopback tests no longer join the server thread unbounded. A client that returns without connecting left the server parked in `accept`, so the join hung; it now fails within the same 10 s bound with an error naming it, after the temp directory is removed rather than before. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> --------- Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> (cherry picked from commit 4292164) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
…550) * fix(release): make --require-signatures verify, not just find a .sig release_readiness.py is the only thing in CI that ever verifies a release artifact's signature, but --require-signatures asserted only that a non-empty .sig sidecar existed. Cryptographic verification was gated by a separate condition, true only when --public-key was passed (no caller does) or ROCM_CLI_SIGNING_PUBLIC_KEY_PEM was non-empty. GitHub expands an unset secret to the empty string and env_text maps empty to None, so removing or rotating that secret would silently downgrade the gate from "every artifact verifies against the release key" to "a .sig file is present" -- no failure, no warning, the only trace being which message appears in the log. A sidecar produced by the wrong key, truncated, or corrupted would pass. Requiring signatures now means requiring they verify. The presence check and the verification branch collapse into one `verify` flag, the key is resolved once (--public-key, else the inline PEM), and a key that cannot be resolved is a hard error rather than a fallback to the weaker check. Each run prints `signature verification key:` naming the trust root used, so the mode is recorded rather than inferred. Verification is deliberately not defaulted to the in-repo pinned key: AGENTS.md section 9 reserves production trust roots as owner-controlled inputs, and `cargo xtask verify-pinned-keys` already asserts a configured CI key equals the canonical one. That cross-check had the same fail-open shape -- it returned silently when no CI key was configured, so an unset secret read as a clean run. It now reports the skip. It stays non-fatal because ci.yml never exposes the signing secret, so demanding one would fail every PR. No workflow changes: the secret is configured today, so release and nightly already take the verifying path and this is a no-op for them. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * fix(release): resolve the signing key from the path variable too Self-review found that making --require-signatures fail closed turned a silent pass into a misleading failure for anyone configuring the key as a file. resolve_signing_key() consulted only the inline ROCM_CLI_SIGNING_PUBLIC_KEY_PEM, so an operator who had set ROCM_CLI_SIGNING_PUBLIC_KEY_PATH -- a documented input this script already accepts in PRODUCTION_TRUST_ENV_NAMES and validate_production_trust() -- was told to "set ROCM_CLI_SIGNING_PUBLIC_KEY_PEM" while the key they had supplied went unused. Resolution now consults --public-key, then the key file named by ROCM_CLI_SIGNING_PUBLIC_KEY_PATH, then the inline PEM. That path-before-PEM order matches how install.sh resolve_public_keys() and xtask/src/package.rs signing_private_key_pem() already resolve their keys. A path that does not exist stays an error rather than falling through to the inline PEM: verifying against a different key than the operator named is the class of failure this gate exists to prevent. Self-test covers the new source and its precedence; the no-key and empty-string cases now clear both variables so they still prove a hard failure rather than passing because one source happened to be unset. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * fix(release): pin the verify gate with a self-test that can fail `--require-signatures` forcing cryptographic verification is the whole point of this change, and nothing tested it. `main` returns on the `--self-test` branch before the gate, and the self-test's `validate_release` cases pass `verify` explicitly, so the flag-to-verify decision was never reached. Proven rather than assumed: deleting `require_signatures or` from the gate -- which restores the pre-PR weakness, where `--require-signatures` accepts a `.sig` signed by the wrong key whenever no key is configured -- left `--self-test` exiting 0 with "self-test: ok". Extract the decision into `verification_is_required` so the self-test can reach it, and assert each arm: `--require-signatures`, production trust and an explicit `--public-key` each force verification, and nothing else does (an ordinary readiness run must not demand a signing key it has no reason to need). Both mutations now fail the self-test; the unmutated file passes. Not added: a keygen/sign/verify round trip with a wrong-key rejection. The self-test writes a placeholder `.sig` and does no crypto anywhere, so that would introduce an openssl dependency into an otherwise hermetic self-test -- worth doing, but as its own change. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * fix(release): pin the verify wiring, not just the decision The previous round extracted `verification_is_required` and tested it in isolation. That was not enough, and the comment claiming the self-test now pinned the gate overclaimed: it pinned the decision function while `main`'s use of it stayed untested. Reproduced both holes the review named. Neutralising the call in `main` (`verify = False and verification_is_required(...)`) left `--self-test` exiting 0 -- the original fail-open, where `--require-signatures` accepts a `.sig` signed by the wrong key. Dropping the `resolve_signing_key` call stayed green too. So make the sequence the unit. `resolve_verification` now decides AND resolves the key, returning `(verify, public_key, key_source)`, and `main` holds no copy of that logic -- it cannot drift from what the tests cover because there is nothing left in it to drift. The new self-test case asserts a required verification actually yields a key, and that nothing is resolved when verification is off. Both mutations now fail and name what regressed: neutralise the gate call -> "--require-signatures must force verification through the wiring main uses" skip key resolution -> "verification was required but no key was resolved ... the fail-open this gate closes" The unmutated script passes. Still not covered, and not claimed to be: that `validate_archive` calls `verify_signature`, and the `public_key=None` inline-PEM path CI uses. Both need the keygen/sign round trip noted as a follow-up, rather than another isolated assertion. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * test(release): pin main's use of the verify gate, not just the gate Extracting the decision and then the wiring still left `main`'s call to `resolve_verification` unexercised, because `--self-test` returns long before `main` reaches it. Replacing that call with `verify, public_key, key_source = False, None, None` kept every check green while restoring the original fail-open, where `--require-signatures` accepts a `.sig` signed by the wrong key. Drive `main` itself against a dist that is valid on every other axis, so the gate is the only thing that can reject it: with signatures required and no key resolvable, `main` must exit 1. Mutation results, via `release_readiness.py --self-test` (what ci.yml:412 and :648 run): drop `require_signatures` from verification_is_required -> fails drop `require_production_trust` -> fails make resolve_verification skip resolve_signing_key -> fails neutralise main's resolve_verification call -> fails (was passing) Each fails with its own message. The new case is not vacuous: under the last mutation the same dist is accepted with `release readiness: ok`, and unmutated it is rejected for the key specifically. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * test(release): pin that main verifies, not just that it refuses The previous round's case drove only the no-key path. That path fails inside `resolve_verification`, before `validate_release` is reached, so `main` could still pass `verify=False` at that call and verify nothing while printing the key line as though it had. It also accepted any `SystemExit(1)`, so a `main` that skipped the gate and failed for an unrelated reason counted as a pass. Both were reproduced here before changing anything. Record the call instead: with a key resolvable, a `--require-signatures` run must succeed AND have reached `verify_signature`. The refusal case now asserts the message, not just the exit code. Mutation results via `release_readiness.py --self-test` (what ci.yml:412 and :648 run) -- the first three were green before this commit: main passes verify=False to validate_release -> fails gate skipped, main exits 1 for an unrelated reason -> fails verify_signature call removed from validate_archive -> fails drop require_signatures from the decision -> fails drop require_production_trust -> fails resolve_verification skips resolve_signing_key -> fails neutralise main's resolve_verification call -> fails Also prints the signature-verification key immediately instead of collecting it into `messages`, which is only flushed on success: a run that fails verification is exactly when you need to know which key it used. Flushed so it cannot land after the unbuffered stderr of `fail()`. The inner `main()` calls now capture stdout and stderr, so a passing self-test no longer prints `release readiness failed: ...` into CI logs. Comments reworded to state the invariant each case guards rather than the history of how it got there. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * docs(release): record the verification trigger that was removed `--require-signatures` implying verification replaced an older trigger: a key present in ROCM_CLI_SIGNING_PUBLIC_KEY_PEM used to make a run verify opportunistically, with no flag asked for. That narrowing was never written down, so a reader would expect a configured key to still be checked. Also notes that the key line is printed as soon as the key resolves, so it survives a failed run, and stops describing install.sh (a public-key resolver) and cargo xtask package (a private-key one) as if they resolved the same key. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * test(release): pin which key verification uses, and that the log names it The previous case recorded only that `verify_signature` was reached. It threw away `public_key` and discarded stdout, so three separate edits stayed green: `main` passing `public_key=None`, `verify_signature` dropping its `key_args`, and the key log line going away. Each is silent in production too -- `cargo xtask verify` falls back to the inline PEM when `--public-key` is omitted, so the run would verify against one key while reporting another. Intercept at the subprocess boundary instead of stubbing `verify_signature`, so the real argv is built and the `--public-key` handoff is covered. The case now asserts three things together: that verification happened, that every invocation names the resolved key, and that stdout reports that same key. This needs a key file that exists, since the real `verify_signature` rejects a named key that is not a file. Added one rather than reusing `key_path`, which stays absent on purpose to pin that a named-but- missing key still wins resolution instead of falling through to the PEM. Mutation results via `release_readiness.py --self-test` -- the first three were green before this commit: main passes public_key=None to validate_release -> fails verify_signature drops its key_args -> fails key log line removed, and replaced with a wrong one-> fails main passes verify=False to validate_release -> fails verify_signature call removed from validate_archive-> fails drop require_signatures from the decision -> fails resolve_verification skips resolve_signing_key -> fails neutralise main's resolve_verification call -> fails Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * fix(xtask): cross-check a path-supplied CI signing key too `resolve_signing_key` in scripts/release_readiness.py prefers ROCM_CLI_SIGNING_PUBLIC_KEY_PATH over the inline PEM and verifies release artifacts against whichever it picks. This check read only the PEM, so a path-supplied key was never compared with docs/keys/ -- and the run still logged "ci public key: not configured — cross-check skipped" while that key was in use. The log line this PR added was false in exactly the configuration the PR made reachable. Resolve both sources here, in the same order the gate uses, so the key cross-checked is always the key verified with. A named path that cannot be read is an error rather than a fall-through to the PEM: silently comparing a different key than the operator named is the failure this check exists to catch. Blank counts as absent on both, matching the gate and the shape of an unset GitHub secret. `check_ci_public_key` is unchanged and still takes PEM text, so its existing cases keep their meaning; resolution is a separate pure function taking both values as arguments, so the new test needs no process-environment mutation. NOT YET VERIFIED ON LINUX: the devbox is unreachable (stale Keycloak client secret) and macOS cannot build this workspace. Checked locally: rustfmt parses and reports no change, `Context` is in scope for with_context, and tempfile is an existing xtask dev-dependency used the same way in architecture_doc.rs and e2e_report.rs. `cargo clippy --workspace --all-targets -- -D warnings` and `cargo test -p xtask verify_pinned_keys` still owe a run before this is trusted. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * test(release): drive main on the paths the gate's own promises live on The self-test exercised `main` only on the `_PATH` success path, so four single-line edits in `main` stayed green: key line collected into `messages` (flushed only on success) key line gated on `public_key is not None` main passing False for require_production_trust main passing None for args.public_key The first two break promises `docs/release-trust.md` makes: that the line names the key a run used, including a run that then fails, and that it appears for the inline PEM -- the source release and nightly actually use, where no key file exists to name. The last two leave `--require-production-trust` and `--public-key` pinned only on the `verification_is_required` helper, not on `main`'s call to it. Adds three cases, built on a shared `_run_main_verifying` harness that can make the stubbed verify fail: the key line must survive a failed verification; an inline-PEM run must report the PEM label and pass no `--public-key`; and each remaining trigger must make `main` verify. The failing-verify case also covers the previously untested branch where `cargo xtask verify` returns non-zero -- the PR's headline behaviour, which no automated case reached. Also names both environment triggers in `docs/release-trust.md` and in the missing-key error, since packaging steps export ROCM_CLI_REQUIRE_SIGNATURE and a developer who did got no hint why the gate fired. Mutation results via `release_readiness.py --self-test`; each mutant is asserted to differ from the baseline before running, because a mutant that fails to apply is indistinguishable from one that survives: M14 messages-instead-of-print -> fails (was passing) M26 print gated on public_key -> fails (was passing) M23 False for production trust -> fails (was passing) M24 None for args.public_key -> fails (was passing) M21 verify failure ignored -> fails (was passing) K1/K2/R1/A/C earlier mutations -> fail Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * fix(xtask): treat a named-but-empty CI key as an error, and pin the env names Three defects in the cross-check this branch added, all of them mine. A key file named by ROCM_CLI_SIGNING_PUBLIC_KEY_PATH that is empty or only whitespace resolved to `Some("")`. `check_ci_public_key` then trimmed it away and logged "ci public key: not configured — cross-check skipped" before exiting 0 -- a key *was* configured, and the log said otherwise. That is exactly what that function's own doc forbids: "'no key configured' and 'key matches' must not look alike in the log". Only an unset variable counts as absent now; a named path that is unreadable or blank is an error. `run()` read the variable names inline, so nothing covered them. Misspelling one left all six tests green while the check queried a variable nothing sets. The names are now constants, `resolve_ci_public_key` takes an env lookup so the names themselves are exercised, and a new case asserts both appear as key sources in release_readiness.py -- the lookup test alone cannot catch a wrong value, because it matches on the same constants. A non-UTF-8 path fell through to the PEM, because `std::env::var(..).ok()` turns NotUnicode into None. Resolution now goes through `var_os`, so a non-UTF-8 path still names the file the operator meant. Also corrects two statements about which workflow step sets what. The packaging steps export ROCM_CLI_REQUIRE_SIGNATURE=1 (release.yml:133,228, nightly.yml:137,232); ROCM_CLI_REQUIRE_PRODUCTION_TRUST is set once for the whole workflow from a repository variable (release.yml:24, nightly.yml:20). The error hint said "the last of which packaging steps export", which pointed at the wrong one. And the environment triggers now have a test: dropping either `truthy(os.environ.get(...))` term from `main` left --self-test green while a packaging run that exports the variable stopped verifying. Verified on Linux. `cargo test -p xtask verify_pinned_keys` 7/7, `cargo clippy --workspace --all-targets -- -D warnings` exit 0, `--self-test` exit 0. Mutations, each asserted to apply first: empty named key file passed through as Some("") -> fails (was passing) env name misspelled in the constant -> fails (was passing) ROCM_CLI_REQUIRE_SIGNATURE term dropped -> fails (was passing) ROCM_CLI_REQUIRE_PRODUCTION_TRUST term dropped -> fails (was passing) Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * fix(xtask): resolve a blank key path like the gate, and test that run() resolves Three gaps, all in the same seam between `run()` and the key comparison. A blank `ROCM_CLI_SIGNING_PUBLIC_KEY_PATH` was not treated as absent. A whitespace-only value made the run fail with "failed to read the CI signing public key named by ...: ", while `release_readiness.py`'s `env_path` strips the value, treats it as unset and falls through to the inline PEM. The doc claimed the two resolved alike. They now do: the value is trimmed before it is taken as a path, lossy replacement characters still count as named so a non-UTF-8 path is unaffected, and the doc states the rule it actually implements -- an empty variable is absent, a variable naming a missing or empty file is an error. Nothing reached `run()`. Its lookup closure could query the wrong variable, answer `None` for everything, or hand `check_pinned_keys` a `None`, and all nine tests stayed green while the path resolution became a silent no-op reporting "not configured -- cross-check skipped" on a run that did have a key. `run_with(root, lookup)` now holds that handoff and is driven by a fake lookup against the real repo root: no key reports the skip, a path key that disagrees with `docs/keys/` fails the run, and one that agrees is reported as a match. What is left in `run()` is `env_lookup`, a pass-through shim with nothing to get wrong, pinned by comparing it with the environment under a name that is always set. The env-name test only asked whether each literal appeared somewhere in `release_readiness.py`. Both appear there anyway, in `PRODUCTION_TRUST_ENV_NAMES` and `validate_production_trust`, so swapping the two constants or pointing either at `ROCM_CLI_METADATA_*` passed -- the first would make release CI read the PEM text as a file path, the second would make the cross-check skip in silence. It now matches the gate's assignment lines, which ties each name to the role the gate gives it. Also pins the message on an unreadable named key. Asserting only `is_err()` let a failed read fall through as an empty string and still pass, telling the operator the key "is empty" and sending them to a file that is fine. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * test(release): pin that a configured PEM alone no longer verifies This change removed a trigger: before it, a set `ROCM_CLI_SIGNING_PUBLIC_KEY_PEM` was on its own enough to make a readiness run verify signatures. `docs/release-trust.md` now promises the opposite -- "a readiness run that asks for neither now checks neither" -- and AGENTS.md section 3 requires a promise that something will *not* happen to be tested. Nothing tested it. The only "off" case ran with the PEM unset, so it could not tell the two behaviours apart: putting the old `or env_text(SIGNING_PUBLIC_KEY_ENV) is not None` term back into `verification_is_required` left `--self-test` at exit 0. Re-adding the same trigger one level up, in `main`'s flag assembly, was equally invisible. `_assert_the_inline_pem_alone_triggers_nothing` runs with the PEM set and nothing asking for signatures, and pins both levels, because each is silent on its own: `resolve_verification(False, False, None)` must stay `(False, None, None)`, and `main` -- which assembles what is fed to it -- must run no verification at all. Each of the two mutations above now turns the self-test red, and on the assertion that matches it. Also relabels the block that drives `validate_release` directly with `require_signatures=True, verify=False`. `main` can no longer produce that combination, so "valid signed dist accepted" overclaimed: those cases use placeholder `.sig` bytes the real gate would reject. They are presence-half unit checks and now say so, with a note pointing at the cases that cover verification end to end. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * docs(release): cite the workflow steps by name, not by line number juhovainio flagged these as bare `file:line` spans. lychee reads the `:NN` suffix as part of the filename, so neither the docs-links CI job nor the prek hook can validate them -- they were accurate at the time of writing and would rot silently. Cite the steps by name ("Package release bundle", "Package Windows bundle", "Package nightly bundle") with plain relative links to the workflow files, and describe production trust as workflow-level `env:` rather than pointing at a line. All three step names verified present in the files they are attributed to. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * test(xtask): pin the path-before-PEM key order across both gates The env-name cross-check pinned which variable each side reads for each role, but nothing pinned the order they are consulted in. Reversing either implementation alone left both suites green: the Rust lookup tests observe only the Rust resolver, and release_readiness.py's --self-test observes only the Python one. A release would then be verified against the inline PEM while the pinned-key cross-check compared the key named by path (or the reverse) -- two different trust roots, with the run still reporting success and naming only one of them. Establish the order twice and require the two to agree: observe it by running resolve_ci_public_key with both sources set to different keys, and read it out of resolve_signing_key's own body in the gate. The body is sliced to that one function because both constants appear elsewhere in the file, where a file-wide search would order unrelated lines. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * refactor(release): reuse the signing-key env constants in the trust check validate_production_trust() spelled ROCM_CLI_SIGNING_PUBLIC_KEY_PATH and _PEM as raw strings while the rest of the gate goes through SIGNING_PUBLIC_KEY_PATH_ENV / SIGNING_PUBLIC_KEY_ENV, so a rename would have missed this spot and silently stopped checking the key it names. Also record why the signing-key checks here are now defence in depth: --require-production-trust makes verification_is_required() true, so main resolves the key and fails closed before this function runs. They are kept because the function is still reachable directly, and because a check that fails closed costs nothing when it never fires -- deleting them would leave the production-trust report resting entirely on a guarantee made elsewhere. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> --------- Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> (cherry picked from commit 8e2e890) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
* fix: unify tail_lines in rocm-core, fix lemonade's boundary bug lemonade's tail_lines (state.rs) always dropped the first line read after seeking into a log file larger than MAX_TAIL_READ, even when the seek landed exactly on a line boundary and that first line was complete. vLLM's sibling implementation had the same bug and was fixed by probing the byte preceding the seek position to classify whether the first line is genuinely partial; lemonade never received that fix, so the two copies drifted (#597). Rather than port the fix into lemonade's copy a second time, extract a single tail_lines into rocm-core so both engines share one tested implementation. The shared version combines vLLM's boundary-probe correctness with lemonade's bounded VecDeque streaming (avoids materializing the whole post-seek window when it holds far more lines than the caller's limit). Each engine keeps its own MAX_TAIL_READ constant, passed in as a parameter. Ported vLLM's two boundary regression tests into rocm-core, added a small-file suffix test and a limit == 0 fast-path test. Not adding a Gherkin scenario: this is a byte-precision edge case in an internal log-tailing routine exercised by rocm serve logs and startup-failure diagnostics, and constructing a multi-megabyte log with an exact newline placement isn't something a CLI-level scenario can assert meaningfully -- the same judgment vLLM's own original fix made when it shipped with unit tests only. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * rocm-core: simplify tail_lines and its tests Reuse test_support::unique_suffix() for the three temp-file tests instead of hand-rolling pid+timestamp uniqueness: the two don't separate two tests starting in the same millisecond on parallel threads, which unique_suffix()'s monotonic counter already exists to fix. Drop the redundant skip_next variable in tail_lines itself; it was just a copy of first_line_is_partial, consumed once. Reuse first_line_is_partial directly. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * rocm-core: fix tail_lines aborting on a mid-char UTF-8 seek When the seek window lands inside a multi-byte UTF-8 character, BufRead::lines() surfaces the invalid leading fragment as an Err and aborts the whole read before first_line_is_partial can drop exactly that fragment. vLLM's startup_log_context then loses the entire log excerpt, and Lemonade reports a bare read failure. Read raw bytes with read_until and decode lossily so the existing partial-line-drop logic stays in charge of the fragment. Also fixes summarize_startup_log_tail's doc comment, stale since it now delegates to rocm_core::tail_lines instead of reading backwards itself. Found during pr-review-watcher's review of this PR; same seek-boundary class as #597, which this PR already unifies. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * rocm-core: move tail_lines into its own log_tail module Mechanical relocation per AGENTS.md's architecture guardrail: a single pub(crate)-free fn with no owned types, moved out verbatim with its existing tests. No behavior change. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * rocm-core: cover CRLF stripping and lossy decoding in tail_lines The read_until loop that replaced BufRead::lines() owns two behaviors std used to provide for free. Neither had a test: reverting the CRLF strip, or swapping the lossy decode for strict UTF-8, left all existing tests green. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * rocm-core: fix rustfmt trailing blank line in lib.rs tests mod Signed-off-by: Jussi Elo <jussi.elo@amd.com> --------- Signed-off-by: Jussi Elo <jussi.elo@amd.com> Backport note for v0.2.1: adds the small `unique_suffix` test helper (from #603) to rocm-core's `test_support.rs`, which the new tests use. (cherry picked from commit f0d4c17) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
…s files (#546) * fix(comfyui): reinstall replaces only the code, never the user's files `rocm comfyui install --reinstall` deleted ComfyUI's `source/` folder before it downloaded anything. `rocm comfyui start` runs ComfyUI from that folder without `--user-directory`, `--output-directory` or `--input-directory`, so it is where the user's models (the `models path:` the CLI prints), saved workflows, generated images and uploads live. A reinstall wiped all of them without a prompt, and a failed download left neither an install nor the data. The dry run mentioned none of this and dropped `--reinstall` from the command it printed. A reinstall now swaps the code around the user's content instead of moving that content anywhere: - The new release is downloaded and unpacked into a staging folder first, so a failed download or a bad archive changes nothing. An archive that cannot be unpacked is dropped from the download cache so the next attempt fetches it again instead of failing the same way. - In `source/`, every top-level entry that is not preserved is the previous release's code and is removed; the release's own entries are moved in. - Preserved entries are never moved, copied, overwritten or deleted. Inside them the release only adds what is missing (a model folder new in this release appears), the user's copy wins on a name collision, and a folder the user replaced with a symlink is never written into. - The release is moved in by rename; only when staging and `source/` are on different filesystems is it copied instead, without following symlinks. User data never takes that path. Preserved: `models/`, `user/`, `output/` (ComfyUI also saves models there), `input/`, `custom_nodes/` and `extra_model_paths.yaml` (user-written; the release ships only the `.example`). Not preserved: `temp/`, which holds disposable previews ComfyUI clears on startup by default. `custom_nodes/` is code, but it is the user's code: nodes they installed, often with their own settings or downloaded weights inside. Deleting them is the same loss this fixes, and nothing the CLI knows can recreate them. A node that does not work with the new release costs little to keep, since ComfyUI logs a node that fails to import and starts without it, and the user can remove it. The packages those nodes installed live in the runtime environment, which a reinstall does not touch. The code swap is not atomic, and does not need to be: code can be fetched again. A marker file in `source/` records a swap in progress and which preserved entries the user had before it began. A re-run, with or without `--reinstall`, removes whatever release code is there and finishes the swap, so an interrupted reinstall converges instead of leaving a half-replaced tree that a plain `install` would reuse, and `kept:` still reports only what was the user's. A reinstall is refused while the ComfyUI this CLI started runs from that folder: it keeps the old code loaded and keeps writing outputs into it. The refusal names `rocm comfyui stop`, which removes the saved state the check reads. The dry run now says what a reinstall replaces and which of the user's entries it keeps, and its printed command carries `--reinstall`. A real reinstall prints a `kept:` line built from the entries the folder held. The swap is unit-tested directly, without a fake Python. Stopping it before each filesystem change in turn, as a kill would, and checking a re-run converges with the user's files untouched also runs on Windows. The tests for symlinked model folders, symlinks inside them, loops and dangling links, and that the user's files keep their inode, mtime and ctime, are Unix-only. Install-level tests and e2e scenarios comfyui-05 to comfyui-08 assert each printed claim together with the files on disk, including the failed download and the refusal while running. Fixes #530 Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(comfyui): delete only release-shipped entries on reinstall The reinstall treated every top-level entry of ComfyUI's `source/` folder that is not preserved as the previous release's code and deleted it. That premise is false: ComfyUI registers `datasets/` (training data) under the same folder, and users keep a `.git` checkout, a `venv/`, `styles.csv`, logs and similar there. A reinstall removed all of them. Only what a ComfyUI release shipped is now removed. Each install records the release's top-level names in `source/.rocm-cli-release-entries`; a reinstall removes the installed release's recorded names, the names the new release ships (which also covers an install from before the record existed, and whatever an interrupted run already moved in) and `__pycache__/`, which Python generates from that code. Anything else is left in place, and the reinstall lists it on a `left in place:` line. `datasets` joins the preserved entries. A reinstall cut short leaves a half-replaced folder, and `start` only checked for `main.py`. `start` now refuses while the swap's marker is present, and `status` reports it, both naming `rocm comfyui install --runtime-id <key>`: the install for that runtime finishes the swap. The swap also removes `main.py` first and moves it in last, so a half-replaced folder never has it. Smaller hardening in the same code: - The marker is written through a temporary file and a rename. An unreadable marker only feeds the report, so it now gives a warning and an empty `kept:` instead of failing the reinstall. - When the copy fallback (staging and `source/` on different filesystems) is used, it copies to a temporary name and renames that into place, so a crash never leaves a truncated release file under its real name; a leftover is removed by the next swap. - On Windows, a rename of a freshly unpacked file that is denied is retried briefly, since scanners open new files for a moment. A removal that fails says to close the program using the file and run `rocm comfyui install` to finish the reinstall. - The dry run's `keeps:` reads the interrupted swap's marker, as the real run's `kept:` does, so the two agree. New tests: unknown entries and `datasets/` survive a swap and are reported; an install from before the record removes only what the new release ships; `main.py` is removed first and moved in last; an unreadable marker; a leftover copy is cleaned up; `start` and `status` refuse an interrupted reinstall until the command they name is run (unit test, and e2e comfyui-09, which runs the command taken from the CLI's own message); and the running guard lets a reinstall through over a stale saved state (dead process, closed port) and while ComfyUI runs from a different folder. The symlink and inode/ctime tests are Unix-only; the rest of the swap tests also run on Windows. README, `--help`, docs/testing.md and docs/manual-testing.md now say exactly what is deleted, kept and left in place. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(comfyui): keep a unix-only test helper out of the Windows build plant_cached_source_archive is called only by cfg(unix) tests; Windows builds the test binary with -D warnings, so the unused helper failed it. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(comfyui): set aside a user's entry the new release ships A reinstall removed every top-level name the new release ships, on top of the installed release's recorded names. A folder of the user's that a newer ComfyUI release starts shipping under the same name (an `app/`, a `venv/`) was deleted without a word, while README, `--help` and the dry run said the new release's names were used only for installs from before the record. With a record, such an entry is now renamed to `<name>.rocm-cli-kept-<time>` and reported on a `set aside:` line with its new name. The decision is made before anything of the new release moves in, and the swap marker records it together with the names of the release being installed, so a re-run after an interruption neither deletes the user's entry nor sets aside release code the interrupted run already moved in. Without a usable record (an install from before it, or an unreadable one) nothing tells the two apart, so those entries are still replaced, and are now listed on a `replaced:` line. A record line that is not the name of a single entry (`..`, an absolute path, `a/b`, `.`) is ignored with a warning. A release entry that cannot be removed stops the swap with a typed error, and the install names `rocm comfyui install --runtime-id <key>` to finish it, instead of a bare `rocm comfyui install` that finishes whichever runtime is the default. `status` no longer advises `rocm comfyui start` while the reinstall is unfinished, since `start` refuses then, and the chat's status answer is built from the install's state instead of scraping `status`'s text: it passes the interrupted-reinstall note on and drops the `start` advice too. On Windows a rename is also retried on a sharing or lock violation (OS errors 32 and 33), which std reports with no specific kind. README, `--help`, docs/testing.md, docs/manual-testing.md and the dry run now say what is deleted, set aside, replaced and left in place; the dry run says which of the two applies to this install rather than claiming to list entries it cannot know before the download. Tests: set-aside, with the swap stopped at every step and re-run (with and without an entry to set aside); the `replaced:` list without a record; an unreadable record (falls back with a warning, does not abort); malformed record lines; an entry that cannot be removed (typed error, then the named command finishes the install); the chat status answer and `status` before and after that command; the dry run with and without a record; and, on Windows, a rename that waits for a file held with no sharing. e2e comfyui-05 asserts the `replaced:` line with the files on disk, and the new comfyui-10 the `set aside:` line with the renamed folder's contents. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(comfyui): cover the swap's marker checks, copy path and set-aside names Make a release entry's rename and removal injectable so the tests reach the cross-filesystem copy arm and the removal failure on any host, including as root. Test that marker lines naming anything but a single entry are dropped and never move anything outside source/, that the copy path cleans up a leftover or partial temporary copy, and that a set-aside name skips names already taken. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(comfyui): finish-advice on any mid-swap failure, dry run names the running guard - A reinstall that fails after its swap started names the `rocm comfyui install --runtime-id <key>` command that finishes it whatever the cause, not only when a removal failed. - A failure after the swap (dependency install, GPU check) names the entries the swap replaced or set aside, which the success report would have listed. - The reinstall dry run says when ComfyUI is running from the folder and the real run would be refused. - An unreadable saved run state no longer blocks a reinstall: `stop` cannot read it either, so nothing could clear the refusal. It is reported and treated as not running. - The plain-words status answer no longer advises `start` while ComfyUI is starting. Tests cover the chat status answer's running and starting branches, a plain install while ComfyUI runs, the staging folder's removal and the no-record dry-run line. comfyui-08 and comfyui-09 now also ask for the status in plain words, and comfyui-08 previews the refused reinstall. The reinstall notes in docs/testing.md get their own section. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(e2e): ask comfyui-08's plain-words status once ComfyUI is installed Before the reinstall the planted install has no app manifest, so the answer advises installing and could not show whether it advises start while ComfyUI is starting. Ask after the reinstall instead, with a stand-in that has not answered on its port, and require the answer to say ComfyUI is installed. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(comfyui): name the finishing command after any post-swap failure A reinstall or first install that fails after its new code is in place (the dependency install or the GPU check) now always names the `rocm comfyui install --runtime-id <key>` command that finishes it, and lists what was replaced or set aside when there is anything. An install that put no new code in place names nothing to finish. The note for an unreadable saved run state now says what happens -- the reinstall is not refused for it -- instead of asking the user to stop a ComfyUI before a reinstall that is already under way. The --reinstall help, README and manual tests state that exception. comfyui-05 now plants an unreadable saved state and asserts the note with the reinstall's success. The new comfyui-11 fails the GPU check after the swap, asserts the error and the files, then runs the command the error names once the check passes. comfyui-06 asserts a failed download names nothing to finish. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(comfyui): swap refuses a release without main.py, keeps its replaced list Three gaps in the code swap a reinstall runs: - A release with no main.py at its top passed unpacking, removed the installed code and moved nothing startable in. It is now refused before the marker is written, so the install is left exactly as it was. - In an install with no release record, `replaced:` was rebuilt on a re-run from whatever was still in the folder, so entries the interrupted run had already deleted (main.py goes first) were never reported, nor anything once the new record was written. The list is now worked out before anything is removed, kept in the swap marker, and a re-run reports it. - On Windows a user's `readme.md` and a release's `README.md` are one file, but the ownership checks compared names case-sensitively: the file was reported as left in place and then renamed over. Names are compared without case there. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(comfyui): re-check the running guard before the swap; test what it guards The running guard was checked only before the download, so a ComfyUI started during a long download was not caught. It is checked again once the new release is unpacked, just before the code is replaced. Tests that could not fail now can: - The guard was only tested against a ComfyUI still starting (live process, nothing on its port). It is now also tested against one answering on its port, which reads as running. - A plain `install --dry-run` must not print a `--reinstall` command, a plain reinstall preview must not claim an interrupted reinstall, and a reinstall preview with nothing installed must not describe replacing code in a folder that does not exist. - The success-path check that "the previous code was removed" looked for siblings nothing creates; it now checks the staging folder is gone. The summary the assistant gives after a status tool call dropped the interrupted-reinstall note `status` prints; it now passes it on. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(e2e): comfyui-08 against a running ComfyUI, comfyui-12 for the pre-swap check - comfyui-08's stand-in now holds its port open, so rocm-cli reads it as running rather than starting; the Given says so. A starting ComfyUI is still planted at the end for the status question. - comfyui-12 starts ComfyUI from the loopback server once the reinstall's download has begun, so only the check just before the swap can refuse it, then runs the `rocm comfyui stop` the refusal names and reinstalls. - Steps that only set up state mid-scenario are `Given`, in comfyui-08 and comfyui-11 as already in comfyui-09. - The success-path check that the previous code was removed looked for siblings nothing creates; it now checks the staging folder is emptied. - comfyui-07's preview must not claim an interrupted reinstall. Docs: README and the manual checks cover the second running check and the refused wrong-layout release; docs/testing.md lists the new tests and cites comfyui.feature as a relative link. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(comfyui): drop a cached archive that is not a ComfyUI release A downloaded archive that unpacks but has no main.py in its top folder was refused by the swap, but stayed cached, so every later reinstall reused it and failed the same way. It is now rejected while unpacking, which drops the cached copy as an unreadable one already was. Also: - The tool-call status summary only takes notes from `status`'s ComfyUI block, not from other tool output ahead of it. - `--reinstall`'s help says a ComfyUI still starting also blocks it, that the check runs again just before the swap, and that a download that is not a ComfyUI release changes nothing. - The comment on the second running check says what it does not cover: `start` takes no lock, so one begun during the swap itself is not caught. - A Windows test for the recorded-install path: a user's `readme.md` is set aside when the release adds `README.md`. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(e2e): give comfyui-04's archive a main.py; comfyui-08 proves its premises - An archive without main.py is now refused, and comfyui-04's fixture had none, so its install would fail before the spinner checks finished. - comfyui-08 checks through `rocm comfyui status` that its planted ComfyUI reads as running, and that the first refusal comes before any download, which is what keeps comfyui-12's later check honest. - docs/testing.md: reflowed, and names the new ways a reinstall leaves the install untouched. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(e2e): comfyui-13, a download that is not a ComfyUI release Rejecting an archive without main.py is something a user sees: the reinstall fails saying so, the install is left as it was, and the download is not kept. comfyui-13 serves such an archive and checks all three. The --reinstall help says the download is discarded, not just that nothing changes; source_swap's own main.py check is described as the safety net it now is behind install's; the manual checks and docs/testing.md name the new scenario and comfyui-08's refused-before-download check. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(comfyui): merge the swap's identical removal branches; name comfyui-13 Moving the `replaced:` list into the marker left two `if` arms that both queue the entry for removal, which clippy rejects. The feature header and README now also mention a download that is not a ComfyUI release, and that such a download is not kept. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(comfyui): require main.py only when replacing an install The main.py check exists so a wrong-layout download cannot empty an existing install. Applied to a first install as well, it also refused the archives main's unpack property tests generate, which stopped those tests before the first install write they watch. A first install has nothing to lose, so the check now applies only when an install is there to be replaced, in both the unpack and the swap. Also passes main's new archive property harness the `before_swap` argument download_and_extract_source now takes. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * docs(comfyui): say the main.py check guards an existing install only The swap's comment and docs/testing.md still described the main.py refusal as applying to every install; since it was narrowed to installs being replaced, they say so, and why a first install is not checked. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> --------- Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> Backport note for v0.2.1: the `archive_properties` test module is left out; its `archive_props` harness (#559) is not in this release. (cherry picked from commit ee3131b) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
…604) * fix: print copy-pasteable --yes on the remaining service stop hints Two more printed `rocm services stop <id>` hints fail without --yes, the same bug fixed in #601 for the serve summary. One is the require-api-key reuse refusal in spawn_managed_engine_child; the other is the remote serve already-serving refusal, which tells the user to ssh into the remote and run the remote CLI's own stop command - same --yes gate applies there too. Tightens the two existing unit tests covering these refusal paths to assert the full runnable command rather than a loose substring. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * test: cross-reference the e2e proof for the --yes stop hints Mirrors the comment PR #601 added for the sibling serve_summary.rs case: name service_actions_require_yes_and_render_sandbox_result as proof the approval gate recommends this exact form, and stop_managed_services (tests/e2e-cucumber/tests/e2e.rs) as the test that actually runs `rocm services stop <id> --yes` as a subprocess and relies on it succeeding. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * docs: show --yes on the README's rocm services stop example The "match the original flags" paragraph told users to run `rocm services stop` to clear a mismatched-options service, but that command fails without --yes (the same bug just fixed for the two refusal-path hints in this PR). Same fix shape, applied to the doc example. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * fix: print a copy-pasteable --yes on the serve summary stop hint The serve deployment summary's stop row told users to run `rocm services stop <id>`, but run_approved_service_action refuses to act without --yes, so copy-pasting that exact line always failed. Print the canonical, actually runnable form instead, and tighten the regression test so it pins the full command rather than a loose prefix that would have passed on the buggy string too. Signed-off-by: Jussi Elo <jussi.elo@amd.com> (cherry picked from commit 4b04dd4) Signed-off-by: Jussi Elo <jussi.elo@amd.com> * test(serve_summary): stop overclaiming e2e proof of the stop hint The regression test's comment said stop_managed_services "relies on it succeeding", implying an e2e assertion that the printed stop command actually works. It doesn't: both its callers (scenario teardown and the serve retry's failure evidence) discard or merely print the exit status rather than asserting on it. Correct the comment to say what is and isn't verified, per review on #601. Signed-off-by: Jussi Elo <jussi.elo@amd.com> (cherry picked from commit 1631f41) Signed-off-by: Jussi Elo <jussi.elo@amd.com> * test(e2e): prove `rocm services stop <id> --yes` actually stops a service service-stop-01 plants a running managed-service record on the mock lane, runs the stop command the deployment summary prints, and asserts the record is left stopped — closing the gap the last review round flagged: existing tests only proved the summary prints the canonical `--yes` form, not that running it works. Signed-off-by: Jussi Elo <jussi.elo@amd.com> (cherry picked from commit ee687f7) Signed-off-by: Jussi Elo <jussi.elo@amd.com> * refactor(services): share the `<action> <id> --yes` hint behind one helper Every surface that told a user how to stop or restart a specific managed service built the string by hand: `main.rs` (the require-api-key reuse refusal, two already-running/detach hints, the approval gate's own `Try:` line, the remove-while-running refusal, the prune skip message, and both services-list hints), `remote/mod.rs` (the teardown rollback exec, the already-serving refusal text, and the session-stop exec), and `serve_summary.rs`. That duplication is what let the same bug (a hint missing `--yes`, which the command refuses without) recur three times (#417, #601, #604). Add `apps/rocm/src/service_hints.rs` with `service_stop_hint`, `service_action_hint` (for the gate shared by stop and restart), and `remote_service_stop_hint` (for the remote-prefixed form), and convert every one of those call sites to use it. Output is byte-identical, so no test assertion needed to change for this alone. While here, two of the tightened assertions' cross-reference comments (`main.rs`, `remote/mod.rs`) claimed `stop_managed_services` — teardown-only, in `tests/e2e-cucumber/tests/e2e.rs`, which explicitly discards its result — "relies on [the stop] succeeding". It does not; nothing there would turn red if the printed command stopped working. Point them at `@id:service-stop-yes-stops-a-running-service` instead, which actually asserts a planted running record is left `stopped`. Also correct the two surfaces that still showed `--yes` as optional on `stop`/`restart`: the README synopsis (`[--yes]` → `--yes`, matching `remove`'s already-correct form) and the `--help` text on `Stop`, `Restart`, `Remove`, and `Prune` ("Do not ask for confirmation", which wrongly implies an interactive prompt exists where the command only ever refuses outright). Signed-off-by: Jussi Elo <jussi.elo@amd.com> * test(e2e): prove the --require-api-key reuse refusal's stop hint actually works Neither existing test for `spawn_managed_engine_child`'s require-api-key reuse refusal proves the hint it prints is directly runnable: one only pins the string, the other (cited by its comment) is best-effort teardown that discards its result. AGENTS.md's rule for a printed remediation command is that it "must exist, accept those flags, and actually clear the condition that printed it" — nothing here checked that. Add serve-24 (`model_serving.feature`): plant a live, unauthenticated managed-service record for the built-in Lemonade smoke-test recipe (its catalog entry has an empty `engine_recipes`, so a plain serve resolves no engine recipe override — matching the planted record's unset `engine_recipe_json` so the idempotency guard's recipe-mismatch check never fires before the require-api-key check this scenario exercises), then serve that same model again with `--require-api-key`. Assert the refusal names the exact, directly-runnable stop command, then actually run it and check the record is left `stopped` — the same proof `@id:service-stop-yes-stops-a-running-service` gives the serve summary's own hint. Runs on the Lemonade GPU lane (`@requires-gpu @requires-engine:lemonade`): the idempotency match is on `(engine, canonical_model_id)` only, not a real listening port or process, so no actual model load is needed, but `--managed` serve still refuses before reaching this guard without a real GPU, like every other `@requires-gpu` scenario on this lane. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * test(e2e): prove the remote already-serving refusal's stop hint actually works `discover_started_service`'s "already serving" refusal (`remote/mod.rs`) had the same gap as the local require-api-key refusal: a unit test pinning the string, and a comment citing teardown-only `stop_managed_services` as proof it works, which it is not. Add remote-15 (`remote.feature`, `@requires-docker`): the fake `rocm serve` stand-in on the containerised remote always appends a new service record keyed on the target port, so a second `remote serve` at the same (default) port reuses the first call's `service_id` — reproducing the reuse condition with no change to the stand-in. Reuses the existing "a model serving on a reachable GPU machine" / "the user serves a model on that machine" steps for the Given/When. Asserts the refusal names a directly-runnable `rocm services stop <id> --yes`, then runs that exact command inside the container and checks the service is actually gone — the remote-side counterpart to `@id:service-stop-yes-stops-a-running- service` and to remote-13's proof for the session-stop path. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * docs: document the new stop-hint scenarios and the service_hints module docs/testing.md's endpoint-auth coverage note now names serve-24 and remote-15 alongside service-stop-01. docs/architecture.md's apps/rocm shared-CLI-output-components list now includes service_hints.rs, with a one-line note on why it exists (#417, #601, #604 were all the same duplicated-string bug). Signed-off-by: Jussi Elo <jussi.elo@amd.com> * fix: borrow service_id at the detach-path stop hint call site Missed in 958b98e: this call site holds service_id as an owned String, and service_stop_hint takes &str. Caught by a full cargo build after the fact, not by the clippy/build run that preceded the commit — that run was piped through `tail`, which masked cargo's real exit code behind tail's. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * style: cargo fmt the --yes stop-hint changes cargo-fmt flagged three lines this PR touched: the detach-path stop hint in main.rs, and two lines in serving_steps.rs that now exceed the line width after adding REUSE_REFUSAL_ENGINE/_SERVICE_ID. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * fix(e2e): un-swap serve-23/serve-24's closing steps and gate serve-24 to Linux Inserting serve-24 (the --require-api-key reuse-refusal scenario, added in ca9e8d5) left serve-23's original closing step attached to the new scenario instead: serve-23 (vLLM, low-VRAM OOM guidance) stopped short of checking the memory-knob explanation, and serve-24 (Lemonade) ended with that vLLM-only assertion instead of its own last step. The vLLM step can never pass against Lemonade's serve output, so serve-24 always failed on every GPU lane. Also add @requires-os:linux to serve-24: `--require-api-key` forces auth even on a loopback bind, and `ensure_public_bind_engine_supported` in serve_cmd.rs refuses that combination for Lemonade on Windows (its server can't receive the key through the Windows detached-spawn primitive), so the scenario cannot reach the refusal it exercises there. Matches the Strix Halo Windows failure observed on PR #604's E2E run. Verified locally: both scenarios parse and resolve their steps correctly (`cargo xtask e2e -- -n 'serve-23|serve-24'`); this sandbox has no GPU, so the `@requires-gpu` gate itself fails here, same as it would for any scenario on this lane without hardware. CI's GPU runners are the authoritative check for the scenario bodies themselves. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * fix(e2e): plant remote-15's pre-existing service on the container Both the Given and When steps called `rocm remote serve`, so the second call tripped serve_with_transport's local session::exists guard before any SSH round trip ever happened — remote-15 never reached discover_started_service's "reused instead of started" refusal it was meant to exercise. A new Given step plants the service directly on the stand-in container instead, leaving no local session, so the one `remote serve` call actually discovers it and refuses. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * refactor(hints): route the last two hand-built hints through service_hints remove_managed_service_record still formatted its "remove"/"restart" hints by hand, so service_hints.rs's "the one place every hint is built" claim didn't hold. Both now go through service_action_hint, and its doc comment no longer limits `action` to "stop"/"restart" only. Also corrects the module doc and architecture.md: they counted this PR and an unmerged, closed-as-superseded sibling PR as two more drift incidents alongside #417, when only #417 actually drifted in the codebase. Two test comments cited @id:service-stop-yes-stops-a-running-service as proof that a printed stop hint actually clears the refusal it names, but that scenario only proves a generic stop works — not that it clears *this* refusal. Repointed each to the scenario that actually reproduces its own refusal and runs the hint against it (serve-require-api-key-refuses-unauthenticated-reuse, remote-serve-refuses-reusing-already-serving-session). Finally, a_failed_publish_stops_the_model_it_started only asserted the executed remote command contained "services stop svc-1", so a regression dropping --yes from the shared hint builder would not have failed it. Now asserts the full "--yes" form. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * docs(e2e): fix a --yes-less example and overclaiming test comments tests/e2e-cucumber/README.md's own illustration of the "assert the state, not just the message" principle still showed the stop command it was warning about, minus --yes. service_stop_steps.rs's stop_with_yes claimed to run "the exact line serve_summary.rs's stop row prints", but the step builds that line from a known constant, not from anything parsed out of printed output — nothing here actually ties the two together; that equivalence is what the hint-specific scenarios prove instead. service_stop.feature's own gap-closing claim is narrowed: the planted record's supervisor_pid: 0 means the scenario proves the on-disk record is persisted stopped, not that a real process was signalled. model_serving.feature's serve-24 comment misattributed ensure_public_bind_engine_supported to serve_cmd.rs; it is defined in main.rs and only called from serve_cmd.rs. docs/testing.md's serve-24 mention now notes the Linux-only gate. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * docs(architecture): correct service_hints.rs drift history serve_summary.rs's stop row never had --yes and later lost it; it shipped without --yes from the commit that introduced it (4108ea7) until 5b33fd4 added it. "Cannot drift out of sync... as it once did" implied a prior synced state that never existed. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * test(e2e): assert token count before indexing the stop hint then_stop_hint_clears_the_service indexed a hand-split hint string with args.get(3) and no check on its length first. Both of remote_cli's current values are whitespace-free, so this can't fail today, but a future value containing whitespace would silently grab the wrong token instead of failing loudly. assert_eq! on the token count up front makes that failure mode loud. Signed-off-by: Jussi Elo <jussi.elo@amd.com> --------- Signed-off-by: Jussi Elo <jussi.elo@amd.com> Backport note for v0.2.1: `docs/architecture.md` keeps v0.2.0's module-map layout, with `service_hints.rs` added to it. The feature-file comment naming `serve_cmd.rs` (main-only) is pointed at `main.rs`, where the guard is called on this release. (cherry picked from commit e4b4155) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
* fix: give the automations proposal line real KiB/MiB/GiB units format_bytes_for_user divided by 1024 at every step but labeled the result KB/MB/GB, while a sibling format_bytes (same file) does the identical math with correct KiB/MiB/GiB labels. Drop the duplicate: repoint its one call site (the automations proposal-history "approved up to" line) at format_bytes, and fold its stronger boundary/proptest coverage onto the survivor. Adds an e2e scenario covering the corrected line against the real binary, deduplicating its AppPaths-from-isolated-root construction into a shared E2eWorld::app_paths() (dash_steps.rs had an identical copy), and fixing a doc comment that overstated the proposal list as status-filtered when it is not. Fixes #190 Signed-off-by: Jussi Elo <jussi.elo@amd.com> * fix(e2e): keep app_paths()'s cache dir in sync with isolate_env() app_paths() built cache_dir as a bare <root>/cache, while isolate_env() (which sets the spawned rocm process's ROCM_CLI_CACHE_DIR) routes it through a shared CI cache dir when one is configured. A fixture written through app_paths().cache_dir would land somewhere the CLI process never looks, in that case. Extracts the shared derivation into one cache_dir_for() both call, so they can't drift apart again. No current app_paths() caller reads cache_dir (dash_test_clock_file and append_automation_proposal both resolve under data_dir), so this closes a latent landmine rather than a live failure. Addresses a Copilot review comment on this PR. Signed-off-by: Jussi Elo <jussi.elo@amd.com> --------- Signed-off-by: Jussi Elo <jussi.elo@amd.com> (cherry picked from commit f308121) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
* fix(lemonade): fall back to platform home dir when HOME is unset Windows never sets HOME, but Lemonade still caches downloaded GGUFs under %USERPROFILE%\.cache\huggingface\hub. hf_cache_roots_from only checked the HOME env var, so on Windows the lookup skipped that cache root entirely and ensure_hf_checkpoint_downloaded reported the file as missing even though the download had succeeded. Fall back to runtime_home_dir() (which resolves USERPROFILE on Windows) when HOME is unset, and name the directories that were searched in the "did not download" errors so a future mismatch is easier to diagnose. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * test(lemonade): assert the HF cache-root error text names every root The "did not download" errors now claim a specific set of searched directories, but nothing tied that claim to the lookup it describes. Split the rendering out of describe_hf_cache_roots so the text can be asserted against the roots hf_cache_roots_from actually returns, and cover both the populated case (including the Windows USERPROFILE fallback this change adds) and the no-roots-resolved case. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> --------- Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> (cherry picked from commit e4a20d9) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
…iled read (ROCMAI-449) (#691) * fix(rocm-core): stop fix-6-path from wiping Windows User PATH on a failed read (ROCMAI-449) ps_env_scope collapsed any PATH read failure -- PowerShell missing, blocked by execution policy, or QUERY_TIMEOUT expiring -- to "", indistinguishable from a genuinely empty PATH. run_path_export_windows then treated that as "no existing PATH" and ran `setx PATH "<bin_dir>"`, replacing the user's entire real PATH with one entry. Under --yes the confirm() gate is skipped, so this could happen with no chance to notice and no way to undo from within the tool. ps_env_scope now returns Option<String>: None on a failed read, Some(..) (including Some(String::new()) for a genuinely empty PATH) only when the read actually succeeded. The PATH read/plan/write logic moves into a new path_export_windows, which takes the read as an injected closure the same way report_and_clear_override_windows takes consent as a closure, so the failure path is testable without a real Windows host. A None now aborts loudly with a non-zero/non-success exit code before any "Plan: ..." preview is printed and before the write is ever attempted. Separately, the computed new value contradicted its own printed "Plan: prepend..." message: it appended (`format!("{user_path};{bin_dir}")`) rather than prepending, unlike the Linux sibling's intent (`run_path_export_linux`'s `export PATH="{bin_dir}:$PATH"` puts bin_dir first) of letting the newly-fixed SDK's bin dir take priority over whatever else is already on PATH. Extracted into windows_path_prepend, called from both the printed plan and the real write, so the two can no longer disagree. Finally, `setx` silently truncates values past 1024 characters while still exiting 0, so a normal-length PATH could be cut on write even when the read succeeded. The write now uses PowerShell's `[Environment]::SetEnvironmentVariable('PATH', $value, 'User')`, invoked the same way ps_env_scope already invokes PowerShell for reads, which has no such limit. The other two ps_env_scope call sites (fix-2's HSA_OVERRIDE_GFX_VERSION report/clear, fix-9's HIP_VISIBLE_DEVICES) now call .unwrap_or_default() to keep their existing behavior unchanged -- neither overwrites a whole env-var value the way fix-6-path's PATH write does, so collapsing a failed read to "unset" there remains safe and is out of scope for this ticket. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * fix(rocm-core): escape single quotes in the Windows PATH write script, add real e2e coverage (ROCMAI-449 round 2) Issue 1 (quoting): path_export_windows's write script interpolated new_path -- built from the real existing User PATH read back from the host, not a literal this code controls -- directly into a PowerShell single-quoted string: `format!("...SetEnvironmentVariable('PATH', '{new_path}', 'User')")`. PowerShell single-quoted strings are literal, and its own convention for a literal `'` inside one is to double it (`''`); an unescaped `'` instead ends the string early, so the rest of the line is parsed as separate PowerShell tokens. An ordinary Windows path containing an apostrophe (`C:\Users\O'Brien\bin` on the real PATH, not a contrived edge case) would make the write fail every time, or worse, for a crafted PATH entry, become a string-injection point into an executed `-Command`. The code this replaced (`run("setx", &["PATH", &new_path], ...)`) passed new_path as a separate argv element with no shell parsing at all, so this exposure is newly introduced, not pre-existing. Added ps_single_quote_escape, PowerShell's own doubling convention, applied once to the combined new_path (which already carries bin_dir, so both inputs are covered by the one call) before it is embedded — both in the printed "Plan: ..." line and in the real write script, so the two stay unable to disagree the same way windows_path_prepend already keeps them from disagreeing on ordering. write_script_escapes_apostrophe_in_real_path_value builds the exact script path_export_windows's write path constructs against a user_path containing an apostrophe and asserts the embedded value literal appears intact with the quote doubled, not split mid-statement. Issue 2 (e2e coverage): cb9f748 changed user-observable `rocm fix fix-6-path` behavior on Windows -- a failed PATH read now exits 4 instead of silently proceeding, the printed plan line changed from `setx` to `SetEnvironmentVariable`, and the success message's wording changed -- with zero e2e coverage of any of it; the suite's only fix-6-path touchpoint was its "no ROCm install found" early exit. Every other fix-recipe e2e mock in this suite fakes a Unix shell script standing in for an external helper (usermod/sudo); faking powershell.exe/setx.exe itself would fake the thing under test rather than a dependency, so it was rejected as the easy option. Instead, diagnose-38 and diagnose-39 (@requires-os:windows) run the real write against the real User-scope PATH, following lifecycle-13's precedent in lifecycle_steps.rs for doing this safely: stage_user_path_for_test (added to lifecycle_steps.rs and reused from diagnose_steps.rs) stages a known value, captured for LifecycleState's Drop to restore even on panic -- the exact mechanism install.ps1's own PATH-mutating scenarios already rely on for the same machine-global state. diagnose-38 stages a real pre-existing User PATH containing an apostrophe, runs the fix, and asserts both the new wording and that the real PowerShell write left bin_dir prepended with the apostrophe entry intact -- the one way to validate the Issue 1 escape against a real powershell.exe, which no unit test can do. diagnose-39 proves the read-failure-aborts-with-no-write behavior for real (not simulated) by emptying the child's own PATH so its Command::new("powershell") cannot resolve the binary (powershell.exe lives under System32\WindowsPowerShell\v1.0, which Windows does not search unconditionally the way it searches System32 itself) -- a genuine instance of the "PowerShell missing" cause ps_env_scope's doc comment already names, never touching the real User PATH at all. Verified on the remote Linux box (no Windows host available, so diagnose-38/-39 could not execute for real; their existing @requires-os:windows precedent plus resolve()'s skip logic is what polices that mechanically): cargo test -p rocm-core (57 passed), cargo test --workspace --all-targets (full workspace, 0 failed), cargo clippy --workspace --all-targets -- -D warnings (clean), python3 scripts/smoke_local.py (ok), and cargo xtask e2e against diagnose.feature in isolation with no -n/--tags override (so the real Expectation-based skip logic ran, not bypassed by a CLI name filter) -- 35/39 scenarios passed, the other 4 (including the two new ones) correctly skipped on this non-Windows, non-WSL host. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> * fix(e2e): allow missing_const_for_fn on the non-Windows User-PATH stubs The e2e-only clippy lane (`cargo clippy --locked -p e2e-cucumber --test e2e`, which `--all-targets` cannot reach because the harness is `harness = false`) failed on the two `#[cfg(not(windows))]` arms of the User-PATH helpers the fix-6-path scenarios need. Taking clippy's suggestion would make only the inert arm `const`: the Windows twins shell out to PowerShell and can never be, so the two platforms would disagree on the signature and push the lint onto callers. Follow the stub convention already used in `capability.rs`, `proc_lifecycle.rs` and friends instead. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> --------- Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> Backport note for v0.2.1: the e2e PATH-staging helper captures and restores the User PATH with v0.2.0's existing helpers, since #636's registry-kind-preserving capture is not in this release. (cherry picked from commit fee201c) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
`main`'s "E2E tests (Strix Halo, Windows, current)" lane is permanently red on diagnose-38, and the CLI is behaving correctly -- the assertion is wrong. The step banned the substring `setx ` across the *whole* of `rocm fix fix-6-path` output, and `rocm fix <id>` prints the catalog recipe before it runs anything, so the catalog's own documented Windows command tripped the test the real (PowerShell) write was meant to prove. The documented command is also wrong on its own terms, which is the defect underneath the broken test. `setx PATH "%PATH%;<bin>"` truncates the value at 1024 characters while still exiting 0, and `%PATH%` expands to the combined user+system PATH, so pasting it writes every system entry into the user's own PATH. cb9f748 moved the real write off `setx` to `[Environment]::SetEnvironmentVariable` for exactly these reasons and left the advertised commands behind. - `fix.rs`: the `fix-6-path` recipe and `diagnose.rs`'s copy of the same plan now both document the PowerShell prepend the CLI performs -- reading and rewriting the User scope only -- with notes saying why it is not `setx`. `diagnose.rs` reuses the real write's `ps_single_quote_escape`, so an apostrophe in the bin dir cannot break the printed copy either. - diagnose-38 now asserts over the write region (the `Plan: prepend ...` line through the post-write confirmation) instead of the whole output, via `user_path_write_region`. Prose about `setx` anywhere else -- a note, a rationale, a future warning -- no longer fails the scenario, while a write that regressed to `setx` still does. Five unit tests in the e2e lib cover both directions on any platform. Signed-off-by: Roman Sirokov <roman.sirokov@amd.com> (cherry picked from commit 0acf698) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
* fix(ci): keep the GPU reclaim decoys alive on multicall coreutils ubuntu-latest now resolves to Ubuntu 26.04, which ships uutils coreutils. Its /bin/sleep is one multicall binary that picks the tool to run from argv[0], so the decoys made by copying it to .../llama-server print "coreutils: unknown program 'llama-server'" and exit at once. The reclaim self-test then fails three checks and the xtask contract test that runs the real --assert-clear passes a scan it should fail, which turns build-and-test, Coverage and Test (affected crates) red for every run on the new image. Copy bash instead: it ignores the name it runs under, so argv[0] stays the real-shaped path the rule is meant to match. The body is a loop because bash execs a lone `sleep 300` in place, which would put `sleep` back in argv[0]. Its `sleep 1` children name no E2E root, so they are never selected, and SIGTERM still ends the decoy. Verified with the self-test (plain and with TMPDIR redirected) and the xtask test in ubuntu:26.04 and ubuntu:24.04 containers; both failed on 26.04 before this change. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(ci): correct the delimiter decoy's reason and detach the xtask decoy The delimiter decoy's comment said bash would swallow an extra argument as $0; it does not, and the argument reaches /proc/<pid>/cmdline. It is a separate helper only because spawn_decoy passes no arguments through. Detach the xtask test's decoy from the test's stdio, as the script's decoy already is: its sleep 1 child outlives the decoy's SIGKILL by up to a second and would hold the inherited output open meanwhile. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> --------- Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> Backport note for v0.2.1: only the `reclaim-gpu.sh` decoy change is taken; the workflow-contract tests in the conflict come from #672 and are not in this release. (cherry picked from commit 5dfa9a8) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
* fix(therock): pin torchaudio to the requested ROCm 10 build `rocm install sdk --version 10.0.0` failed for every arch with "selected torchaudio 2.11.0.3+rocm10.1.0, which does not share ROCm build 10.0.0". On the ROCm 10 layout, `uv pip compile` picks torch and torchvision from the `rocm==<version>` pin, because their metadata depends on the exact ROCm build. torchaudio's metadata names no ROCm build, so left unversioned it resolved to the newest build on the index. That works for the newest ROCm 10 release only; every older pin got a mismatched torchaudio and was refused by the consistency check. Pin torchaudio before resolving: list torchaudio from the index (one extra fetch, with its own single-fetch timeout) and take the newest build published for the requested ROCm version and this Python and platform, in PEP 440 order with final releases ahead of pre-releases, as uv would choose. The existing major.minor.patch parser cannot be reused, because torchaudio publishes rebuilds as a fourth release segment (2.11.0.2). The newest ROCm build resolves the same versions as before. When the requested build publishes no installable torchaudio, or none with a readable version, the install stops and says which. The e2e scenarios publish two ROCm 10 builds whose dependency metadata matches the live index's, with a four-segment torchaudio rebuild and a rebuild wheel for another platform, and cover both the pin and the refusal. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(therock): pin the hardware-verified torchaudio before the newest Picking the newest torchaudio carrying the requested `+rocm<version>` label is not safe on its own: `whl-next` publishes `2.11.0`, `2.11.0.2` and `2.11.0.3` all as `+rocm10.1.0`, and `2.11.0` is a CPU-only rebuild that nothing in its version or label distinguishes. The vLLM ROCm 10.x discover-build table already pins `2.11.0.2` exactly for that reason. `install sdk` now reads that same pin rather than keeping a second copy: the vLLM engine exposes `verified_torchaudio_release`, the row's exact torchaudio for a ROCm line (`rocm` already depends on the engine crate, so no new crate edge). When the verified release is published for this Python and platform, it is pinned. Otherwise, or when the line has no verified release (ROCm 10.0 today), the newest build is pinned as before. The plan's `torchaudio_selection:` line, and an install-log note for the fallback, are both rendered from the selection itself and say whether the build is hardware-verified. ROCm 10.1 on the live index now pins `2.11.0.2+rocm10.1.0` instead of `2.11.0.3+rocm10.1.0`, matching what the vLLM install pins. therock-next-11 asserts the not-hardware-verified line beside the plan it describes. The new therock-next-13 pins ROCm 10.1 against a fixture that also publishes the original and a newer rebuild under the same label, and asserts the plan takes the verified build and says so. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(therock): test the unverified-torchaudio install note and name the ROCm build in its reason The install-log note for a torchaudio pin that is not hardware-verified had no test: its condition lived inline at the call site, after the dry-run return every scenario takes. Move the text and the condition into unverified_torchaudio_note and test it for both unverified bases and for a verified pin. The VerifiedNotPublished reason blamed "this Python and platform", but the verified release is looked up only among builds labelled for the requested ROCm build, while it is matched by ROCm line. When it is published here only under another label of the same line, the reason now says so and names that build; it says "not published for this Python and platform" only when no ROCm build of it is published here at all. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(e2e): reach the unverified-torchaudio install note in a scenario therock-next-14 runs a real (not dry-run) ROCm 10.0.0 wheel install with an active default runtime and no consent flag, so it is refused right after the install-log note and before uv runs or anything changes on disk. It asserts the note together with the refusal and an unchanged runtime registry. Also name the situations therock-next-12 and -13 set up in their Givens: a pinned build whose only torchaudio is a wheel this host cannot install, and several torchaudio rebuilds sharing one ROCm label. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * docs: state the torchaudio pin rule rather than the table's current rows Describe the verified torchaudio as the vLLM ROCm 10.x row's exact pin instead of repeating today's values, add "for this Python and platform" to the refusal, mention the install-log note, and point the discover-table rows at rocm install sdk, which reads their torchaudio pin. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> --------- Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> Backport note for v0.2.1: `verified_torchaudio_release` is added next to v0.2.0's `rocm_sdk_series_matches` in `install.rs`, where that helper still lives. (cherry picked from commit 3067a10) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
…r offers a downgrade (#566) * test(therock): property-test version ordering and update selection Adds a proptest suite over the comparator that decides which ROCm runtime an install or update resolves to, plus deterministic reproducers for what it found. compare_version_strings compares numerically when both sides match parse_version's X.Y.Z[rcN|aN] grammar and falls back to a plain string compare otherwise. Mixing those two relations in one comparator is not a strict weak ordering, so sort_by has no defined result and the "newest" candidate depends on the order the index listed its versions in. The same fallback inverts the verdict in runtime_freshness, which is what decides whether an update is offered at all. The suite is red on purpose. Seven properties fail; each one is a defect, and the repro_* tests pin the exact minimal inputs by asserting the current, wrong behaviour so the following commits show the change as a diff. Retention grouping, select_runtime_update_source and comparator antisymmetry all hold. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(therock): make version ordering a total order compare_version_strings compared numerically when both sides matched parse_version's X.Y.Z[rcN|aN] grammar and fell back to a byte-wise string compare otherwise. A beta (7.0.0b1), a post-release (7.0.0.post1), a dev release or a two-component version (7.9) all miss that grammar, so the comparator silently switched relations mid-sort. Mixing two relations is not transitive — 7.0.0 < 7.0.0b1 < 7.0.0rc1 < 7.0.0 is a cycle — and sort_by has no defined result on a comparator like that. What users hit: * runtime_freshness pointed the wrong way in both directions. Installed 7.10.0 against an index offering 7.9 reported UpdateAvailable, so `rocm update --apply` would install 7.9 — a silent downgrade. Installed 7.9 against an index offering 7.10.0 reported AheadOfIndex, so a real upgrade was never offered. update-07 reproduces both in one report. * select_rocm_version sorts then pops, so with a cyclic comparator the chosen build depended on the order the index listed its candidates in, and an alpha could be selected over the final release. * The module contradicted itself: compare_version_strings called 7.9 newer than 7.10.0 while parse_host_version, in the same file, read 7.9 as 7.9.0 and called it older. Severity is channel-dependent. channel_rocm_candidates filters the release channel to is_stable_runtime_version, which drops everything parse_version rejects and so masked this there. The nightly channel does not filter, and a runtime added by `runtimes adopt`/`import` carries whatever version its manifest states, bypassing any index filter. Ordering is now PEP 440's, which is the right reference because these versions come from a Python simple index: dev < alpha < beta < rc < final < post within a release, and a missing patch component reads as zero so 7.9 is 7.9.0. A string the parser cannot read keeps a defined position — below everything readable, with unreadable strings ordered against each other by the string — instead of switching the comparator to a different relation. Sorting low is the conservative end for the "newest candidate" pickers, which take the maximum: an unreadable string can only win when every candidate is unreadable, where before it could beat a real release by being lexicographically longer. It cuts the other way in runtime_freshness, where the unreadable string is the installed version, so a runtime whose version cannot be identified now reports UpdateAvailable rather than an arbitrary verdict; that direction is documented at the comparator. Numerically equal spellings (7.9 and 7.9.0, 7.9.0 and 7.9.0+local) are separated by the string, so no two distinct strings compare Equal and every sort is deterministic. The lenient parsing lives in one place now: parse_host_version strips the build number a system ROCm reports (7.2.4-98) and then defers to the same parser the comparator uses, so a host version and an index version cannot disagree about which is newer. That build-number strip stays on the host side on purpose — `-` is also a PEP 440 pre-release separator, so doing it for index versions would read 7.0.0-rc1 as the final 7.0.0. For the same reason host_version_newer_than now reads its resolved_version, which comes from the index and not from the host, with the index parser. parse_version itself is deliberately untouched. It is the strict grammar gate for routing decisions, not an ordering parser: select_source_layout documents that "an unparseable pin ... stays canonical, so nothing that resolves today starts resolving somewhere else", is_stable_runtime_version is the release-channel filter, and parse_package_base_version gates the PyTorch stack match. Widening it would change which wheels get installed, which is a different question from which of two versions is newer. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(therock): break installed-runtime ties on the runtime key load_runtime_manifests_reporting_unparsed sorted the registry by Reverse(installed_at_unix_ms) with no tiebreak, so runtimes installed in the same millisecond kept whatever relative order read_dir returned them in. select_startup_update_manifest falls back to the first entry when no active runtime key is configured, which handed the filesystem the choice of which runtime the startup update check reports on. A `--devel` install and its plain sibling are the ordinary way to land on the same timestamp, and the field is only millisecond-resolution. select_runtimes_to_remove already breaks its retention-group ties on the runtime key; this applies the same rule to the registry load. The sort is now a named helper that the loader and its property test both call, so those two cannot drift. The order-independence property alone cannot pin the rule down — it sorts both views with this same function, so it is satisfied by any deterministic comparator, including a reversed tiebreak or an oldest-install-first sort. A deterministic test asserts the direction as well, since which runtime the startup check reports on is user-visible. Separate from the version-ordering fix in the previous commit: that one is about what "newer" means, this one is about the registry read order being an input at all. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(therock): order four-component releases, and stop offering downgrades Two defects a review found in the previous commit, both of which it introduced. The first is a silent downgrade, deterministically reachable. The ordering parser accepted at most three release components, so `7.2.4.70204` — the shape ROCm's own packages are named with — was unreadable and sorted below every readable version, including `6.4.3`. An installed runtime at `7.2.4.70204` with an index offering `6.4.3` therefore reported UpdateAvailable, and `rocm update --apply` would have installed ROCm 6 over ROCm 7. The old comparator got that pair right by accident, so for this input the previous commit was a regression, and of exactly the class this work exists to remove. It is fixed on both levels it is wrong on: * The release segment is now a vector, as PEP 440 specifies, with trailing zeros stripped so plain `Vec` ordering matches PEP 440's zero-padding. `7.2.4.70204` sits above `7.2.4` and below `7.2.5`, and `7.2.4.0` is `7.2.4`. This moves the ordering key into its own type: `ParsedVersion` keeps the fixed major/minor/patch that the PyTorch stack matchers read directly, and the new `VersionOrderKey` exists only to be compared. `parse_version` stays byte-identical. * "Sorts below everything readable" is the safe end for the pickers that take a maximum, and the wrong end for `runtime_freshness`, where the unreadable string is the *installed* version and "lowest" reads as "older than the index". That caller no longer uses the sort order to decide: `actionable_version_relation` requires both sides to be identifiable as versions, and otherwise reports AheadOfIndex, which offers nothing. Identical strings still compare equal whatever they spell, so a runtime whose version cannot be parsed but plainly matches the index stays up to date instead of being declared ahead of it. A runtime adopted from an existing environment carries whatever its probe reported, so this is reachable without any index being odd. The second is that the parser rejected spellings PEP 440 calls equal while the module documented its order as PEP 440's. `7.0.0-rc1`, `7.0.0_rc1` and `7.0.0RC1` were all unreadable, so a release candidate sorted below `0.0.0`. The stated reason — that `-` is a PEP 440 pre-release separator — argued against *stripping* `-`, not against parsing it. Now accepted: case, any of `.`, `-`, `_` or no separator, the alias labels (`alpha`, `beta`, `c`, `pre`, `preview`, `rev`, `r`), an omitted stage numeral, and a leading `v`. What is still refused is refused with a reason, in the parser's doc: an epoch, a combined stage suffix, and PEP 440's implicit post-release, whose spelling collides with the build number a system ROCm reports and which `parse_host_version` must keep reading as a build number. `parse_host_version` now drops only a *numeric* `-` suffix, so `7.2.4-98` is still `7.2.4` while `7.0.0-rc1` keeps its stage and is read the same way on both sides. Also removes a re-sort in `newest_installed_runtime_manifest` that the registry-ordering commit made a no-op: `load_runtime_manifests` already returns the list in byte-for-byte that order. Tests: an exhaustive total-order check over the whole alphabet replaces nothing but covers every triple deterministically, where the proptest properties sample. The alphabet gains a four-component release, a `-`-separated pre-release and a second unreadable string, so a triple can mix two distinct unreadable strings with a readable one — the arm the previous commit introduced was otherwise only ever reached reflexively. The claim that a date-stamped nightly rc "orders by its number, not as text" is corrected: both of those pairs were already correct before this work, so the assertion passed under a full revert and proved nothing. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(therock): decide updates on versions, not on their spelling Widening the parser to accept PEP 440's alternative spellings left the update decision reading them as different builds. `compare_version_strings` breaks a final tie on the raw string, which is deliberate and right: `sort_by` needs every distinct string to have a distinct place. But `7.0.0-rc1` and `7.0.0rc1` are one version — that is why the parser now accepts both — so inheriting that tiebreak for "is there a newer build?" turned a spelling difference into an update offer. Applying it re-downloads a multi-gigabyte runtime the machine already has. The same held for `7.0.0RC1`, `v7.0.0` against `7.0.0`, `7.0.0rc` against `7.0.0rc0`, and `7.9` against `7.9.0`, and it pointed either way depending on which side the string tiebreak favoured, so the verdict could also be AheadOfIndex. This became reachable *because* of the widening rather than in spite of it: PyPI normalises versions, so an index can legitimately serve a normalised spelling while an older manifest recorded the raw one it was installed from. The split is now explicit. `compare_version_strings` answers "where does this sort?" and keeps the string tiebreak. `version_relation` answers "is this a different build?" by comparing the parsed keys, which are equal exactly when two strings denote one version, and returns None when either side cannot be identified at all. Byte-identical strings still short-circuit, so a runtime whose version cannot be parsed but plainly matches the index stays up to date. Three callers ask the second question and now use it: * `runtime_freshness` — the update verdict, as above. * `repo_version_without_wheels` — otherwise warned that the newest version has no wheels while naming the same version on both sides of "installing X instead", and warned on pairs it could not identify. * `active_default_relation_text` — otherwise called a reinstall an "upgrade". When neither side can be identified there is no better word available, so the total order still decides rather than inventing one. The property that checks the verdict against the version order asserted nothing on the Equal case, which is the hole this fell through; it now requires UpToDate. A deterministic test pins the spellings and, in the same place, pins that the comparator still orders them apart, so the two behaviours cannot be collapsed into each other by a later change. Also records what makes the two parsers agree about `7.2.4-98`: the numeric-only `-` strip in `parse_host_version` is the load-bearing detail, not the refusal in the ordering parser alone. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(rocm-core): break registry ties on the runtime key too `rocm` now orders its runtime registry newest install first with ties broken on the runtime key, so the runtime its startup update check falls back to no longer depends on `read_dir` order. `rocm-core` reads the same registry for two fallbacks of its own and still sorted on the install time alone: * `select_active_managed_therock_record` -- with no active runtime key configured, "the most recently installed" runtime supplies the managed ROCm environment, channel and version. * `resolve_amd_smi_binary_in_registry` -- which runtime's `amd-smi` is used. Records that share an `installed_at_unix_ms` (a `--devel` install and its plain sibling, or two records written before the field existed, which both read as 0) were therefore picked by the filesystem, and could disagree with the runtime `rocm` reports on. Both now use one helper that applies `rocm`'s rule, then the record path for records with no key. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(therock): require the generator to reach two spellings of a version The verdict property only checks the update verdict's `Equal` arm when it draws two spellings of one version, and that arm is the hole the spelling-difference update offer fell through. `generator_reach_report` printed how often that happens but did not assert it, so an alphabet change could drop the case with the suite still green. It now fails below 0.5% (it measures about 3%). Also corrects the comment on `active_default_relation_text`'s fallback, which said the string order decides when *neither* version can be identified; `version_relation` returns `None` when *either* cannot, and the fallback then reads the unreadable side as the older one. Documents the version-ordering tests in docs/testing.md. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(e2e): pin that a catalog with nothing newer offers no update update-07 pins the two directions in which a newer and an older catalog version used to be inverted. The fix also changed two cases where the catalog has nothing newer, and only unit tests covered them: * the catalog spells the installed version differently (`7.0.0rc1` for an installed `7.0.0-rc1`, one version under PEP 440). Current main reports `status=update_available` and prints an `--apply` offer that re-downloads the installed runtime. * a four-component installed release (`7.2.4.70204`) against a catalog `6.4.3`. Main gets this right, but an intermediate version of the fix regressed it into a downgrade offer, so it is pinned here as well. update-08 checks both against the same loopback nightly tarball catalog as update-07, asserting the status and the absence of the `--apply` offer together. The catalog fixture is now a helper shared by both scenarios. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm-core): cover both registry fallbacks, not just their sort The previous commit's test calls the sort helper directly, so either call site could be put back on the install-time-only sort with the suite still green. Each fallback now has a test that writes two runtimes installed in the same millisecond under both pairings of file name and runtime key: a pick that follows `read_dir` order is wrong in one of the two pairings, whatever order the filesystem returns, so the test fails deterministically rather than by luck of the directory hash. Also applies rustfmt to the previous commit's test. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(therock): pin the version-level callers, not only the comparator Mutation testing the fix showed three behaviours it introduced that no test could fail on: * `active_default_relation_text` and `repo_version_without_wheels` were switched from the string order to `version_relation`, so a respelling of the installed version reads as a "reinstall" and does not produce a no-wheels warning naming one version on both sides. Reverting either to `compare_version_strings` left the suite green. A test now pins both, plus "an unidentifiable resolved version produces no warning". * Truncating the release segment to three components also survived: the comparator's string tiebreak still orders `7.2.4.70204` above `7.2.4`, but the update verdict would call `7.2.4` up to date against an index offering `7.2.4.70204`. The four-component test now asserts the version relation as well. Also rewrites a comment in `host_version_newer_than` that justified reading the resolved version with the ordering parser by saying the host parser "would strip `-rc1`". It no longer does: `parse_host_version` now drops only a numeric `-` suffix. The remaining difference is that build number, which an index version does not carry. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(therock): let the epoch case fail if epochs are read approximately `compare_version_strings_places_unreadable_versions_below_readable_ones` claimed to pin that an epoch is unreadable, but used `1!2.0`. A parser that dropped the epoch reads that as `2.0`, which still sorts below `7.0.0.dev1`, so mutating the epoch rejection left the suite green. The case is now `1!8.0`, which an approximate read places above it. Also points a doc comment at `version_relation`, the name the helper has had since it was renamed. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(therock): assert the generator reaches every shape it is meant to `generator_reach_report` printed several rates without asserting them, and the shapes the ordering fix is about were not measured at all: four-component releases and the individual dev/a/b/rc/post stages. The unreadable-version check only asserted "not every pair is readable", which one sample satisfies. The report now asserts a 2% floor on pairs containing a four-component release, each stage, an unreadable version, and an installed version newer than the catalog's (the downgrade direction the verdict property must see). It also runs on proptest's deterministic RNG, so the floors cannot flake with the seed. Measured rates are 8.5-60%. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(vllm): break registry install-time ties on the runtime key The vLLM engine reads the same runtime registry as the CLI and rocm-core, and picks a runtime's interpreter and recorded SDK torch build from the newest entry matching a runtime id. It sorted on `installed_at_unix_ms` alone, so a `--devel` install and its plain sibling, which share a runtime id and land in the same millisecond, were ordered by `read_dir`. The engine could then use a different runtime's environment than the CLI reports on. Ties are now broken on the runtime key, then on the manifest path, matching the CLI's registry order. The reading moves into `load_runtime_manifests_in` so a test can point it at a temporary registry; the test writes both pairings of file name and key, so a read-order pick fails in one of them whatever the filesystem returns. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(rocm-core): order SDK probe candidates by the shared registry rule `managed_therock_sdk_probe_candidates` is the third reader of the runtime registry in this crate, and it still sorted on the install time alone, so on a tie `read_dir` decided whose `rocm_agent_enumerator` was probed first. The records carry a runtime key; it was dropped before sorting. It now orders the qualifying records with `sort_records_newest_install_first`, like the active-runtime and `amd-smi` fallbacks, and the install time no longer needs a field of its own on the candidate. The test writes both pairings of file name and runtime key. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(e2e): make update-08's four-component case fail on the old order The four-component half of update-08 installed `7.2.4.70204` against a catalog `6.4.3`. The old comparator fell back to text for that pair and `"7.2.4.70204" > "6.4.3"` as text too, so the step passed before the fix as well as after it and pinned nothing the fix changed. It now installs `7.10.0.71000` against a catalog `7.9.0`: still a four-component release ahead of the catalog, but one that sorts below it as text. With the old comparator restored, the step fails with `status=update_available` and an `--apply` offer to install 7.9.0; on this branch it reports `ahead_of_index` with no offer. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm-core): let the sort-helper test fail without the key tiebreak The helper breaks install-time ties on the runtime key and then on the record path. The unit test named each record's file after its key, so path order equalled key order and dropping the key tiebreak left the test green: the path tiebreak produced the same order. The three registry-reader tests caught that mutant, but the helper's own test did not. The file name now mirrors the key's letters, so path order is the reverse of key order and only the key tiebreak gives the asserted order. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(registry): pin the path and missing-time tiebreaks too The registry sorts in rocm-core and the vLLM engine break install-time ties on the runtime key, then on the record's path, and read a record without `installed_at_unix_ms` as installed at 0. Every tie test used distinct keys, so the path step never decided anything, and no test had a record without a time: dropping either rule left both crates green. Add a case with two keyless records in one millisecond, in both input orders, and one with an untimed record beside an explicit 0 and a real time. The vLLM sort moves into its own function so it can be tested directly rather than through `read_dir`, whose order the test cannot control. rocm-core's doc also claimed its fallback picks the runtime the CLI's startup update check reports on. Only the tiebreak is shared: rocm-core considers only records whose SDK probe imported, while the CLI takes the first of every manifest, so with no active key a tarball installed after a wheel is what the CLI reports on while rocm-core falls back to the wheel. Say that instead. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(therock): pin the registry load, stage aliases and approval fallback - The registry load is what the startup update check and `newest_installed_runtime_manifest` rely on, and only the sort helper was tested. Load two same-millisecond manifests from disk, under both pairings of file name and key, and check both callers' pick. - `parse_version_stage`'s `preview` and `r` aliases, its longest-first label order, and a separator inside the numeral (`rc.1`, `-rc-1`) had no test. Add them to the PEP 440 spelling test and place them on the hand-written stage ladder. - The install approval's fallback for a version it cannot identify was untested; check that an unreadable side reads as the older one. - `generator_reach_report` now enforces the reach the file claims for `latest` (two distinct unreadable strings) and `7.9rc1` (a stage on a two-component release), and asserts that `manifest_set` never yields a duplicate key instead of printing a counter that is always 0. Also: drop `ParsedVersion`'s ordering derives and its never-read `patch` and `stage_number` (still parsed, so the strict gate is unchanged), and correct the docs that described them; say the comparator is the order used for sorting candidates, not the single definition of newer; keep the redundant epoch guard and say it is documentation; and move the unreadable-version rationale from the four-component case, which never reaches it, to the `custom-build` case that does. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * docs: say what the version order decides, and what it does not docs/testing.md said both what `rocm install sdk` resolves to and whether `rocm update` offers a build follow one PEP 440 order, and that two spellings of one version are never an update. Tarball builds are picked by upload time first, the release channel only considers strict `X.Y.Z` versions, and a wheel runtime for an exact device target can report `repair_available` across a respelling because its runtime key carries the index's spelling. Narrow the paragraph to what holds. The update steps' module doc still said every scenario runs with no managed runtimes; update-07 and update-08 plant some and serve the catalog from a loopback server. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> --------- Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> Backport note for v0.2.1: `newest_installed_runtime_manifest` lives in `main.rs` on v0.2.0 (not `runtimes_cmd.rs`); main-only tests carried in the conflict context are left out. (cherry picked from commit f53815d) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
…#499) * test(rocmd): pin identity-verified stop of a managed service `stop_managed_service` signals the PIDs recorded in a service manifest without consulting the `supervisor_start_ticks` / `engine_start_ticks` identity tokens stored next to them, so a PID the kernel has since handed to an unrelated process is signalled as if it were still the service. It then expands each recorded PID against a live process listing, dragging that stranger's whole subtree into the kill set, and leaves the PIDs in the record afterwards so a repeat stop signals them again. These four tests fail today and describe the intended contract: - a PID whose recorded start-time no longer matches is never signalled; - neither is anything below it; - "refuted" and "absent" are reported distinguishably, because they mean different things to a caller; - a confirmed stop clears the PIDs it terminated. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(rocmd): verify process identity before stopping a managed service `rocmd` stopped a managed service by signalling the PIDs persisted in its manifest, without ever checking that those PIDs still belonged to the service. PIDs are recycled, so a stale manifest — a crashed engine, a restarted host, a service stopped twice — could send SIGTERM and, 750 ms later, SIGKILL to an unrelated process. The descendants made it worse: they were expanded from a *live* process listing, so a stale root also took that stranger's entire subtree with it. This is reachable from `rocmd sandbox-tool stop-server` and from the daemon's `stop_server` MCP tool, so an agent can trigger it. The record already carried the answer. `supervisor_start_ticks` and `engine_start_ticks` hold the kernel start-time for their PIDs, which is what distinguishes a recorded process from a recycled PID, and `rocm services stop` has always paired them through `rocm_core::terminate_verified`. The stop now goes through that same primitive with each PID matched to its own token, so a refuted identity is signalled by neither command, and the descendants are reached from a root that was actually checked. `rocmd`'s `ps`-based expansion and its unverified kill helpers are gone rather than left around to be reused. Two gaps would have left that hollow: - `rocmd` recorded PIDs but never the matching tokens, so its own services were exactly the records that could not be verified. Identity is now captured at each site that records a PID, while the process is known alive. - A completed stop left the PIDs in the record, so the next stop signalled them again. They are cleared once every recorded process is confirmed gone, and deliberately kept when one is not — an unconfirmed survivor is still the service's, and a later stop has to be able to reach it. Windows keeps its `taskkill /T` tree kill, taken from the still-live root before the verified stop. `rocm_core` cannot walk a process tree there, and `rocmd` launches the engine one level below the recorded PID through an `__engine-serve-http` process, so a root-only kill would strand the engine holding the device. It costs no safety: without `/proc` there is no start-time to verify against on that platform either way. The result distinguishes the two reasons a PID goes unsignalled, which `skipped_pids` alone cannot express: a new `pid_outcomes` list reports `already_gone` (nothing there) separately from `identity_mismatch` (live, but somebody else's), alongside `stopped` for whether every recorded process is confirmed down. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocmd): guard the sites that record a process identity token The stop's identity check is only as good as the token the launcher wrote down: `rocm_core::identity_state` treats a missing one as a best-effort match, so a record saved without it is signalled blind — the original defect, in full. Setting all four production captures to `None` left `cargo test -p rocmd --lib` at exit 0, byte-identical to an unmutated run, because every existing test hands itself a record with the token already set and so exercises only the read side. These two drive the real write paths and read back the manifest they persisted. `supervise_service` is stopped at its first write by making the service log path a directory, so the `File::create` immediately after it fails; going further reaches the engine spawn, whose failure path calls `std::process::exit` and would take the test binary with it. The recovery restart re-execs `rocmd supervise`, which under test is the test binary rejecting those arguments, so it fails right after the write. Two of the four captures stay uncovered, deliberately. The engine-child token in `supervise_service` is past that `std::process::exit`. The recovery restart's own-PID token is overwritten microseconds later by the child's and is observable only if the spawn in between fails, which a test could force only by mutating `PATH` in a suite that shares one process. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(rocmd): only claim a stop, and drop its key, once one is confirmed `rocm services stop` gates three things on every recorded process being confirmed gone — the status, the pending-stop marker, and deleting the 0600 endpoint key file — and says why: an unconfirmed stop may have left the engine alive and still enforcing that key, and it is the only copy, so discarding it locks the CLI's own probes, chat and service discovery out of a service that is otherwise fine, with no way to re-mint it. `rocmd` did none of that on the same records. It asserted `stopped` unconditionally, deleted the key unconditionally under a comment claiming that was safe, and never wrote the marker at all. Until now it had no basis to do otherwise: it could not tell a signal that worked from one that did not. The verified stop gives it `all_stopped`, so it keeps the same contract, and the two commands stop leaving one manifest in two different states. The marker is what makes the deferred case terminate rather than strand the key: `rocm`'s liveness refresh keys its cleanup on it and drops the key once the processes are actually observed gone. Gating the deletion without writing the marker would have kept the file forever. The unconfirmed branch is reachable on a `timed_out` or `unverified` outcome — the latter being precisely a live process whose identity could not be read, which may still be the engine and may still hold the device. Neither is constructible in a unit test without injecting the grace period or the outcome, so only the confirmed branch is covered here. Also names `unverified` in the `skipped_pids` comment, which listed three cases for a list that now has four, and drops a clause about identity being verifiable from a Windows-only block, where it never is. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(rocmd): report an unconfirmed stop as one in both stop_server tools The stop itself already knew when it could not confirm that a recorded process had exited, and kept the manifest PIDs and endpoint key for that case. Its two callers threw the verdict away: the MCP tool answered "Stopped managed service" with isError: false, and the sandbox tool reported status: stopped, whatever the stop had found. Both now read the stop's own `stopped` verdict. The MCP tool returns an error naming what was kept, and the sandbox tool reports `stop_unconfirmed`. The new tests force an unconfirmed termination and assert the reply together with the manifest and key file it describes. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(rocmd): label a PID recorded as supervisor and engine as both When a record holds the same PID for the supervisor and the engine, the stop handles it once. Its pid_outcomes entry kept only the first role, so the engine looked absent from the report. Label the shared entry `supervisor_and_engine`. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * refactor(rocmd): write a recorded PID and its start-time token together Three sites recorded a supervisor or engine PID and then, on a separate line, the start-time token that a later stop verifies it against. Route them through one helper per role so the token cannot be left behind when one of those sites changes. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(rocmd): keep recovery and concurrent writers from undoing a stop An unconfirmed stop deliberately leaves the status at ready/running, which is exactly what `find_recoverable_service` scans for, and nothing in rocmd read the stop request it records. With automations on, the server-recover watcher (contained, so acting, by default) could restart the service the operator had just asked to stop. Recovery now skips any record with a standing stop request, both in the scan and when a queued event is handled. The request is also written before the stop starts rather than after it ends: the verified stop can wait out the grace period twice per PID, and for that whole window the endpoint is going down while the status still reads ready. The final manifest write was built from the record loaded before that wait, and `write` replaces the whole file, so anything persisted meanwhile was discarded — including fresh PIDs from a restart, which the stop then cleared, leaving a live engine no later stop could reach. The stop now re-reads the record and applies only its own changes. If the record names different processes by then, those were never stopped: the stop reports itself unconfirmed and leaves their PIDs and key alone. A recovery restart moved the supervisor PID but carried the replaced run's engine PID forward. On a record written by `rocm serve --background` that PID has no token, so once the two diverged the next stop held an entry it could only signal blind. The restart drops it (the new supervisor records its own) and, like `rocm services restart`, clears any stop request. Also: - the termination is now injected as a parameter instead of production code reaching into the test module; the reply builders for both stop_server tools are split out so they can be fed an injected stop - the Windows comment no longer reads as "Windows is covered": identity verification does not apply there and the recycled-PID hazard remains - the deferred key cleanup comment names the binary that runs it, and how a rocmd-only caller completes it (by repeating the stop) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(rocm): record the engine PID's token at the two launch sites `rocm serve --background` (`spawn_managed_engine_child`) and `rocm services restart` (`restart_internal_managed_service`) record the child as both supervisor and engine, but captured a start-time token for the supervisor role only. While the two PIDs are equal the stop's de-duplication hides it. Once they diverge — a `rocmd` recovery restart moves the supervisor — the engine entry has a PID and no token, which `identity_state` resolves as a legacy match: the stale PID gets an unverified forced tree kill, the hazard this branch closes everywhere else. The pairing now lives in `ManagedServiceRecord` itself (`record_supervisor_identity`, `record_engine_identity`), so `rocm` and `rocmd` share one implementation instead of each keeping its own, and every production site that records a PID goes through it. The only other writer, `refresh_from_engine_state`, already adopts the engine PID with its own token. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(e2e): cover a confirmed rocmd stop_server end to end The confirmed-stop output of `rocmd sandbox-tool stop_server` changed — `status`, and the new `stopped` and `pid_outcomes` keys — and needs no identity injection to reach: a live process recorded with its own kernel start-time token is confirmed by the identity check and terminated for real. The scenario checks the report together with the state it claims: the process has exited, the record reads stopped with no PIDs and no pending stop, and the endpoint key file is gone. Linux-only, because the token comes from `/proc`. The unconfirmed half cannot be staged from outside the binary and stays with rocmd's unit tests. `rocmd_binary` moves from the artifact-prefetch steps to the shared harness so both rocmd-backed features use the same resolution. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * style: satisfy clippy on the review fixes Make `stop_requested` a const fn and split the `rocmd_binary` doc comment's first paragraph, both flagged under -D warnings. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(rocm): persist the stop marker before signalling the service `rocm services stop` wrote `stop_requested_unix_ms` only after the engine Stop and the termination of the recorded processes. For that whole window the record still read ready/running while its endpoint stopped answering, which is what the daemon's server-recover watcher restarts. Write and persist the marker first, as rocmd's stop already does; the confirmed-stop gating of status, marker and key is unchanged. The termination step is now injectable so a test can observe the on-disk record at the moment the processes would be signalled. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(rocmd): keep the supervisor's writes from erasing a stop marker supervise_service built its record once and wrote that snapshot back after the engine spawned, on each startup phase, on ready, and after the engine exited. `write` replaces the whole file, so each of those erased whatever a stop had persisted meanwhile, including stop_requested_unix_ms. A stop that could not verify the supervisor but did kill the engine then ended with the supervisor writing `failed` with no marker: exactly what the daemon's recovery restarts. Every write after the first now re-reads the record and applies only the supervisor's own fields, never the marker. It writes nothing when the record is gone (a removed service must not be resurrected) or names a different supervisor (a confirmed stop cleared the PIDs, or a restart recorded its own). The first write, which rebuilds the record from the supervisor's arguments, carries a standing marker over: the restart that launches a supervisor clears the marker first, so one found there was set by a stop issued since. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(vllm): record the server PID's start-time token under its own key vLLM's running state names one process as both `pid` and `server_pid` but recorded its token only as `start_ticks`. rocm-core adopts `server_pid` paired with `server_start_ticks`, so every vLLM engine PID reached the service record with no token, and a stop could only signal it unverified once the PID was recycled. Write the same token under `server_start_ticks` as well. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(core): take a same-process start_ticks for an adopted server PID refresh_from_engine_state prefers `server_pid` and read its token only from `server_start_ticks`. State files that record one process as both `pid` and `server_pid` with only `start_ticks` (vLLM's, before it wrote the paired key) therefore left the adopted engine PID with no token. Fall back to `start_ticks` when the chosen PID is the one recorded as `pid`; a distinct server PID never borrows the launcher's token. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(rocmd): never record the daemon as a restarted service's supervisor restart_managed_service recorded the daemon's own PID and token as the supervisor before spawning the real one. When the spawn failed, the manifest kept naming the daemon, and a stop run from any other process would verify that identity, find it live and terminate the daemon's process tree. Record no supervisor until the spawn succeeds. The restart's `failed` write, made when the spawned supervisor exits at once, wrote back the restart's own copy of the record and so erased a stop marker persisted during the 200 ms it waits: the same defect as the supervisor's exit write, with the same consequence. It now goes through update_supervised_record too. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * docs(testing): list the stop-marker ordering and preservation tests Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocmd): build the identity probe record on every platform Two supervisor exit-write tests that run on every platform used a helper gated to Linux, so rocmd's test target failed to compile on Windows. The helper only builds a ManagedServiceRecord and is portable; drop its gate. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(rocm): re-read the record before a stop's final write `rocm services stop` wrote back the record it loaded before the engine stop and the termination wait, which can take some forty seconds. A restart, supervisor or second stop that wrote meanwhile was reverted, and the stop confirmed "stopped", cleared the marker and dropped the endpoint key even when the record had since come to name processes the stop never touched. Re-read the manifest as-is before the final write, apply only this stop's own fields, and confirm only when the re-read record names the same processes the stop handled; otherwise re-stamp the marker and keep the key, as rocmd's stop does. The comparison moves into rocm-core as ManagedServiceRecord::names_same_processes so both stops share it. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * docs(rocmd): say why an unverifiable PID cannot be forced terminate_verified returns Unverified before consulting force when a PID answers kill(pid, 0) but its identity cannot be read (another uid, hidepid, an LSM). Such a stop never confirms, and the CLI cannot force it. Note at the verdict that this is deliberate. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(rocmd): record a restarted supervisor without erasing a stop A restart clears the recorded PIDs, spawns the supervisor, then wrote its whole copy of the record back with the new PID. A stop that persisted its marker in between had it erased while the new supervisor came up, and the stop's own re-read then had no marker to keep. Apply only the spawned supervisor's identity, to the record as it is on disk, through update_supervised_record against the copy from before the assignment, so the ownership guard checks for the cleared PID. Also drop an in-memory status assignment on the startup-failure path that no caller reads and that read as if it were the write. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * docs(testing): list the restart-identity and stop re-read tests Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * fix(rocm): restart only over a confirmed stop, and re-read before writing `rocm services restart` and the `restart_server` sandbox tool discarded the result of their stop and started the new engine regardless, next to a survivor still holding the port and the GPU. They then wrote the record loaded before the stop back over the manifest, twice: on the startup-failure path and after the readiness wait. A stop marker, or any other write that landed during the stop, the spawn or the up-to-45s wait, was silently reverted. Start a new engine only once the stop is confirmed; otherwise report the unconfirmed stop and start nothing, leaving the stop's re-stamped marker and the endpoint key in place. Apply the restart's later writes to the record as it is on disk, and only while it still names the processes and status the restart left, through one guarded helper. The child is recorded before the readiness wait, so a stop issued during it finds the new process. A stop asked for during the restart keeps its marker; one that finished meanwhile keeps its "stopped". The confirmation comes from the stop itself rather than its report's status, which a record already reading "stopped" keeps through a stop that confirmed nothing. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> --------- Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> Backport note for v0.2.1: `service_launch.rs` does not exist on v0.2.0, so its launch-site identity capture is applied to `main.rs`. The restart path is ported without the runtime-pin parts (`load_service_for_restart`, `apply_restart_runtime_pin`, `restart_child_argv`), which come from #424 and are not in this release; its restart-pin test is dropped with them. Main-only tests and helpers carried in conflict context (#603's root- uniqueness helper, #424's runtime-move manual steps) are not included. `docs/testing.md` is updated so it no longer says the endpoint key being dropped lacks e2e coverage; `service-stop-02` covers it. (cherry picked from commit b8c4b3e) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
…ne (#662) * fix(e2e): time the chat request off the scenario-polling thread cucumber polls every in-flight scenario from one task, and most steps run `rocm` synchronously, so that thread is blocked whenever another scenario's command runs. The chat round trip was polled from that same thread: its client timeout kept counting while nothing read the in-process mock's reply, so a server that answers in milliseconds was reported as a client timeout once those stalls outgrew the 10s budget. The ComfyUI reinstall scenarios added in #546 pushed them past it on the hosted mock lane: each new scenario's first `rocm` call pays the CLI's startup update check (a real uv download and Python preparation) for its fresh data dir, 10-15s on that thread. chat-06 then failed with "budget 10s ... failed after 17.1s". Run the exchange as its own task on the runtime's worker threads, so the budget times the server again, and cover it with a test that blocks the awaiting task past the budget while the server answers. Fixes #661 Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(e2e): keep the ComfyUI reinstall scenarios off the update check With a runtime planted, each reinstall scenario's first `rocm` call ran the CLI's startup update check for it: a real uv download, a managed Python setup and a package-index query, 10-15s per scenario on a hosted runner. None of these scenarios is about updates, and every second of it blocks the thread cucumber polls all other scenarios from. The nine scenarios added in #546 stretched those stalls from about 20s to over 100s on a 4-CPU run, enough to expire time-bounded steps elsewhere, such as the 30s autostart claim automations-04 plants. Disable the check for the commands these scenarios run. Refs #661 Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> --------- Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> (cherry picked from commit d965e66) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
juhovainio
previously approved these changes
Oct 27, 2026
juhovainio
left a comment
juhovainio
left a comment
Collaborator
There was a problem hiding this comment.
Approving. I found no blocking issues. Commits are signed off, no conflict markers or internal-name leaks, and each claimed fix has a regression test, with Gherkin scenarios and docs updated alongside. ROCMAI-449 scope matches the fix-6-path changes.
Non-blocking notes:
- The #499 stop/restart port is feature-sized for a patch release (stop_requested marker, recovery gating,
--yesnow required onrocm services stop|restart). Restart is covered by unit tests only, as the description says; the e2e scenarios cover stop. scripts/release_readiness.pynow fails closed without a signing key. Please confirm the signing public-key secret is set for release/v0.2.1 runs.- See the inline note on the Windows PATH write (REG_EXPAND_SZ).
Caveats: the diff exceeds GitHub's 20k-line limit, so I reviewed it via the files API and read the hand-adapted areas (rocmd stop/restart, fix.rs, release_readiness.py, docs) closely. Other areas were skimmed, and I did not build or run tests. At review time the MI300X, Strix Halo Ubuntu (nightly/release), Strix Halo Windows and windows-build-and-test jobs were still pending; the Windows ones cover the #691/#710 paths, so please let them go green before merging.
| // truncates values past 1024 characters while still exiting 0, which a | ||
| // normal-length PATH can hit even when the read above succeeded. | ||
| let script = | ||
| format!("[Environment]::SetEnvironmentVariable('PATH', '{escaped_new_path}', 'User')"); |
Collaborator
There was a problem hiding this comment.
Writing PATH via SetEnvironmentVariable('User') stores REG_SZ, and the read side returns %VAR%-expanded text, so a User PATH with REG_EXPAND_SZ entries (e.g. %USERPROFILE%\bin) gets flattened to literal paths. #636 (kind-preserving) is not in this release. Is that acceptable, and could the PR text say so?
Collaborator
Author
There was a problem hiding this comment.
Accepted for this release, and now stated in the PR text under "Known limit".
You're right that [Environment]::SetEnvironmentVariable(..., 'User') stores REG_SZ and the read returns the expanded value, so %VAR% entries in a REG_EXPAND_SZ user Path come out expanded. main does the same today: #636's kind-preserving registry code only covers rocm uninstall, not fix-6-path.
For v0.2.1 it's still a strict improvement. v0.2.0 ran setx PATH "%PATH%;…", which also writes REG_SZ, and on top of that merges the machine Path into the user value and truncates it at 1024 characters. That truncation is the wipe #691 removes.
I'll open a follow-up on main to read and write the raw registry value with its original kind (the same approach user_path.rs uses), and carry it into a later patch release.
nowycondro
reviewed
Oct 27, 2026
nowycondro
left a comment
nowycondro
left a comment
Collaborator
There was a problem hiding this comment.
🔴 Automated review · pr-review-watcher · 2de7107
Review — no blocking findings
DEGRADED REVIEW — no subagent/fan-out tool was available in this context; this ran as a single-context pass only (see What this covered).
Full review of the whole diff against release/v0.2.1's merge-base, narrowed per git cherry -v origin/main pr-753-head: 7 of the 17 backported commits are byte-equivalent (patch-id match) to commits already merged and reviewed on main and were only spot-checked for safe combination; the 10 commits git cherry marked as hand-adapted (#562, #527, #634, #546, #604, #691, #712, #630, #566, #499) were reviewed in full against their named main origin commit to isolate exactly what the backport resolution changed, plus surrounding context for the largest ones (#499 stop/restart identity, #691 Windows PATH write, #546 ComfyUI reinstall).
This implements a v0.2.1 patch-release backport of 17 named upstream fixes (including ROCMAI-449 for #691); it does not claim to close any ticket outright. No ticket was fetched from an internal tracker (see What this covered); the description's own commit-to-issue mapping was checked against the diffs instead and matches.
Blocking
None
Non-blocking
rocm fix fix-6-path's Windows PATH write can silently flip the UserPathregistry value fromREG_EXPAND_SZtoREG_SZ, freezing any%VARIABLE%references in it —crates/rocm-core/src/fix.rs(path_export_windows,ps_env_scope), introduced into this branch by commit98894df3(#691).
The read (ps_env_scopevia[Environment]::GetEnvironmentVariable('PATH','User')) and the write ([Environment]::SetEnvironmentVariable('PATH', ..., 'User')) are both the well-documented .NET/PowerShell API that always materializes aREG_SZvalue regardless of the PATH's original type, andGetEnvironmentVariableadditionally expands any%VAR%references on read — so a User PATH entry like%USERPROFILE%\binbecomes a hard-coded, no-longer-expanding literal after this command runs, for every future shell. This is exactly the registry-kind problem the same codebase's owncrates/rocm-core/src/user_path.rs(used byrocm uninstall) and the e2e staging helpers intests/e2e-cucumber/tests/e2e/lifecycle_steps.rs(comment: "the Path back through[Environment], which storesREG_SZ") go out of their way to avoid elsewhere, via raw registry calls that preserveREG_SZ/REG_EXPAND_SZ. No test in this PR (or inmain, where the same code is unchanged) exercises the registry kind of the written value.
This is pre-existing — identical inmaintoday, and not something this backport's hand-resolution altered (the only delta versus themainorigin commit is in the e2e PATH-staging helper, per the commit's own backport note) — so it is not attributed to this PR's adaptation work. It is flagged as non-blocking and pattern-relevant because it sits in the exact function this PR (ROCMAI-449) is hardening against silently damaging a user's real Windows PATH, and a reader fixing one PATH-corruption risk here would reasonably want to know a related one remains in the same function.
Confidence 78 · architectural · Fix: read/write via the raw registry APIs (asuser_path.rsalready does) preserving the originalRegistryValueKind, or at minimum detect and warn when the existing value's kind cannot be preserved.
Decisions for the author
None
Positive signals
#499's hand-adaptation correctly matchesrelease/v0.2.1's existing (parameter-less)restart_internal_managed_servicecall sites rather than importing themain-only runtime-pin parameter dropped per the backport note — verified by diffing both branches' call sites directly; no stray dependency on the excluded #424 runtime-pin feature was left behind.- Each hand-resolved commit carries a specific, checkable "Backport note for v0.2.1" explaining exactly what was adapted or left out, and every one checked here matched what the diff against its named
mainorigin commit actually shows (e.g. #712's note that only thereclaim-gpu.shdecoy change is taken, not thextask/src/workflow_contract.rstest from #672 — confirmed bygit show --name-onlyon both commits). #499's backport note's doc claim (docs/testing.mdno longer says the endpoint-key drop lacks e2e coverage becauseservice-stop-02covers it) was checked againsttests/e2e-cucumber/features/service_stop.feature, and the scenario does assert "the server's endpoint key file is gone."
Deployment notes
- At the time of this review, three CI lanes were still
QUEUEDrather than complete:E2E tests (MI300X),E2E tests (Strix Halo, Ubuntu) (release), andE2E tests (Strix Halo, Ubuntu) (nightly). Everything else reported, includingwindows-build-and-test, both Windows/Strix-Halo E2E lanes,build-and-test,clippy, and the E2E consolidated report, isSUCCESS. Confirm the three GPU-hardware lanes land green before merging — the PR's own description states the GPU-lane scenarios were not independently verified by the author. - This is a release-branch backport (
release/v0.2.1), not amainmerge; the base branch has nofix-6-pathWindows-PATH-write safety net at all prior to this PR. If the non-blocking item above is ever addressed onmain, carry it forward into the next v0.2.1.x patch.
What this covered
Read in full: the 10 hand-adapted commits' diffs against their named main origin commits (git show <oid> dumped per commit and diffed pairwise), plus the surrounding full-file context in apps/rocm/src/main.rs, apps/rocmd/src/service.rs, crates/rocm-core/src/lib.rs, crates/rocm-core/src/fix.rs, and apps/rocm/src/comfyui.rs for the three largest/riskiest (#499, #691, #546). The 7 patch-equivalent commits were not re-read in full — established via git cherry -v origin/main pr-753-head, a legitimate narrowing given they are already merged and reviewed on main — but one cross-file interaction between two of them and a hand-adapted one (#691 → #710, both touching the same fix.rs PATH-write region) was checked directly and combines cleanly. gh pr view --json statusCheckRollup was read at face value for CI state; no workflow internals were inspected beyond that.
Did not run: history (git log/git blame on the modified regions) and prior-changes passes did not run as separate steps; their signal is folded into the per-commit diffing above, which already surfaces exactly what changed and why. No internal ticket tracker was queried, to avoid pulling non-public content into a public review venue. This review could not spawn parallel sub-reviewers — no multi-agent fan-out tool was available in this context — so it ran as a single, degraded pass rather than the independent-worker fan-out this methodology calls for on a change this size (74 files, ~17k added lines); measured recall for a single-context pass on a diff this large is a fraction of what a fanned-out review achieves, particularly on the untouched majority of files (test step files, docs, and the non-hand-adapted logic in the 7 equivalent commits). Re-running with fan-out available is recommended. Not reconciled against the existing human review already posted on this PR, or any other discussion thread.
…els that don't (#535) * fix(dash): show the rocm commands the ROCm tab actually runs The ROCm tab's "Runs:" line named commands that do not exist: `rocm install --channel … --format …` (the install manager runs `rocm install sdk …`; clap rejects --channel on `install`), `rocm update --check` (no such flag; the manager runs `rocm update`), and `rocm doctor` (not a subcommand; from a shell it reaches the natural-language planner and prints "No ROCm action matched"; the manager runs `rocm examine`). A user copying them hits an error. The README storage synopsis `rocm storage [report] [--json]` implied `rocm storage --json`, which clap rejects: --json belongs to `report`. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm): every advised rocm/rocmd command must parse Remediation and help text name commands for users to run, but tests only pin the wording, so a renamed flag or removed subcommand strands users silently. Enumerate every advised invocation (rendered --help of every visible command, production Rust string literals, README/docs/skills markdown, VHS tapes), substitute placeholders, and route each through the same entry points run() uses: the natural-language router, then the real clap tree (rocmd via its own parser). A single-word request that falls through to the planner is flagged too: it means the advised subcommand does not exist. Templated retry advice for services stop/restart is checked through the real message function for every engine and every character class a model ref can contribute. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * docs(testing): describe the advised-commands contract test Also apply rustfmt to the new module. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(dash): ROCm tab Runs: labels must match what the managers spawn A label that parses is not a label that is true. Pin each label to the argv its manager builds: Update to UpdateAction::Check, doctor to the examine screen's argv (now one EXAMINE_ARGS constant), and Install to build_args()'s subcommand and value flags. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm): require full commands where advice is meant to be run A missing required argument was always accepted as a prose reference, so if a command gained a required argument every bare copy of it in RECIPES, next-step lines, fenced examples and help EXAMPLES would stay green. Record which kind of text each invocation came from and allow the omission only in inline prose spans or text that marks it with an ellipsis. A single total floor also let one extractor break silently while the others kept the count up; require each source to still yield a known command and a minimum count. Skip clap's generated help subcommand so a help-text problem is reported once. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm): compare advice source extensions via Path Satisfies clippy::case_sensitive_file_extension_comparisons. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test: exempt the model-readiness report heading from the advised-command check #407 landed `render_model_readiness_text`, whose first line is `rocm diagnose --model <ref>: <verdict>`: a heading naming the command that produced the report, not advice to run anything. The advised-command check reads it as an invocation, substitutes both placeholders, and fails because `<verdict>` becomes a second positional argument. That is what ejected this change from the merge queue once main gained #407. Exempt it in NOT_INVOCATIONS with that reason, beside the other status headings. It is the only new hit from rebasing onto current main. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm): close three false passes in the advised-command check - Check every command of a shell list: split at `&&`, `||`, `;` and a standalone `|` (not inside quotes, `<…>`, `[…]` or `{…}`), and check each part that runs `rocm`/`rocmd`. Before, only the first was seen. - Stop cutting a command at a double space. Only an indented EXAMPLES row (rendered help, or the after_help literal it is written in) has a description column after one; README synopsis rows are now checked past their alignment padding. - Treat a freeform verdict as a deliberate request only when the advice itself is multi-word (a quoted request or `<natural language request>`), not when a placeholder filled in several words: `rocm frobnicate <TEXT>` no longer passes. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm): pin that only an indented row loses its description column Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * docs(testing): say exactly what the advised-command check covers The section said it checks "every command" and that each source must still yield a known command, but only help and Rust were held to a named command; Markdown and tapes were checked by count alone. Hold those two to a named command as well (README fenced and inline lines, the CLI tape), and describe the shell-list split, the EXAMPLES-row column cut, the natural-language rule, and the scanner's known gaps. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(dash): derive the Install label from build_args() The label test checked the subcommand prefix and two hard-coded flag names, so a label that grew an invented flag still passed. Build the expected label from the default form's argv instead: the subcommand, then every value flag with its value elided as `…`. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm): check advice around an options placeholder, exclude two prose notes Main's out-of-memory recipes advise `rocm serve <model> <case-appropriate options above>`, and the checker filled that placeholder with an invented value that clap rejects. A placeholder naming options stands for flags the reader picks from the text above it, so it now contributes no words and `rocm serve <model>` is still checked. The code object manager probe's notes start with `rocm serve/rocm chat` as prose about what those commands load; they are listed as not advice. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm): key four advised-command exclusions to the files they moved to Main's rocmd and rocm-core extractions moved four excluded log and error strings out of `apps/rocmd/src/lib.rs` and `crates/rocm-core/src/lib.rs` into `sandbox.rs`, `service.rs` and `host_gpu.rs`. Exclusions are keyed by file, so the moved strings were reported as advice that does not parse and the old entries as matching nothing. Point the entries at the strings' new files. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm): skip test-only module files when scanning for advice main gained deletion_properties.rs (#562) and archive_props.rs (#559), declared as #[cfg(all(test, unix))] modules, the second through #[path]. Their comments and ignore reasons read as advised commands (`rocm dirs`, `rocm uninstall applies no guard ...`), so every_advised_command_parses failed on main merged with this branch. The scanner skipped only whole *tests.rs files and items gated on exactly #[cfg(test)]. Skip every file declared only as a test module, following #[path], and treat #[cfg(all(test, ...))] items like #[cfg(test)] ones. Removing either change, or ignoring #[path], turns a test red. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm): check the advice main added since the last rebase Main moved the service hints into service_hints.rs (#604), added the runtime-activation retry hint (#424) and moved the labelled `stop:` line out of a literal (#625). Re-key the services exclusion and add one for the runtimes hint; both are format templates, so each gets a test that checks every real value against the parser: the literal verbs callers pass to service_action_hint (found in the source, so a new caller is covered), and the refusal for every `rocm runtimes` subcommand taking --restart-services, re-parsed to show following it clears the refusal. That check is hoisted into ensure_runtime_service_restart_approved so the test exercises the dispatch's own hint, not a copy of it. README advertised `[--restart-services] [--yes]`, but --yes requires --restart-services, so `rocm runtimes rollback --yes` fails. Nest the flag, and teach the scanner nested groups: each prefix of a nested group is now its own form, so `[--distro [NAME]]` also checks bare --distro. The architecture doc names the hint's real verbs, so it is checked rather than excluded. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm): close three gaps the review found in the advice checker Natural language: a quoted argument after one word passed as a deliberate request, so `rocm frobnicate "two words"` was never reported. Judge only the first request word, which is what routing judges. rocmd: the check appended `--help`, and clap stops at help before it validates conflicting or required arguments. Parse with rocmd's own parser through a new run-nothing `rocmd::parse_args` instead, so rocmd advice gets the same validation as rocm advice. Markdown: the `$ `/`PS> ` prompt stripping and the wrapped-span join had no test. Add one carrying a removed subcommand in each shape. A `\` continuation cut off by the closing fence or the end of the file is now checked as it stands instead of swallowing the next fence's first line. Swap negatives that a future alias or flag could make legitimate for `frobnicate`. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm): handle wrapped help spans and branching synopsis groups A span an author breaks across `--help` lines is joined and checked, as wrapped Markdown spans already are. In a synopsis group a value list now branches the alternative and later words extend every branch, so `[stop|restart <id>]` yields both forms with the id rather than `stop restart <id>`; nested groups are tried with and without their inner part. Say exactly what is skipped: whole-line comments only, not a trailing comment on a line of code. Narrow the dashboard label test's doc to what it checks, drop a skip the test-module rule already covers, and bring the testing guide up to date with these and the earlier fixes. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(e2e): check the dashboard's diagnose action names rocm examine The ROCm view's `Runs:` line is a command a user can copy into a shell, and it named `rocm doctor`, which does not exist. The unit test checks the labels against what each screen runs; this checks the rendered line through the real TUI. It extends dash-01 rather than opening another dashboard session on the no-GPU lane, whose dash timing scenarios are sensitive to added load. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(xtask): exempt the test-only advised-command checker from the ASCII scan `advised_commands.rs` is declared only as `#[cfg(test)] mod advised_commands;`, so none of its literals reach a binary. The ASCII-output contract's test-only detection is path-based and cannot see that, so the checker's prose markers (em dash, en dash, arrow), its ellipsis needle and a Unicode model-reference fixture were reported as output. Exempt the file with the reason; `\u{..}` escapes would not help, since the scan flags those too. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm): key the `rocm command` exclusion to chat.rs, where it moved Main moved `chat_tool_call_display_label` and its `rocm command` fallback label out of main.rs into chat.rs. The NOT_INVOCATIONS entry still named main.rs, so the label was reported as unparseable advice and the entry as stale. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm): pin that a dash or arrow ends an advised command `command_parts` cuts a command at an em dash, en dash or arrow so the description after it is not read as arguments, but no test exercised those markers: removing any of them passed the suite, before and after main's ASCII conversion of CLI output. Pin each one. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> * test(rocm): key six chat exclusions to chat.rs, where they moved Main's chat/MCP-bridge extraction moved the `rocm tools: {}` status row, the planner's natural-language phrases (`rocm install folder`, `rocm installed at`, `rocm installed`, `rocm please`) and the `rocm` tool's empty-argv validation error from main.rs into chat.rs, verbatim. None of them is advice to run a command; only the file the exclusions name changes. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> --------- Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com> Backport note for v0.2.1: only the dashboard label fix is taken — the ROCm tab's `Runs:` lines named `rocm doctor`, `rocm update --check` and `rocm install --channel`, none of which parse on this release — with its unit test and the dash-01 step. The advised-command scanner, the rocmd `parse_args` hook, the `runtimes` refusal text (from #424), and the README, docs and xtask hunks are not included. (cherry picked from commit dd8ecea) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
* docs: publish the WSL2 guide on the docs site
Add an Install page, ROCm CLI on WSL2, that single-sources the
published sections of docs/wsl.md (prerequisites, ROCDXG install,
diagnosing a WSL host, and what `rocm examine` reports). Maintainer
notes in docs/wsl.md stay unpublished.
Copyedit the published sections for the style guide, replace the
hard-coded ROCDXG version and digest in the by-hand install steps with
placeholders, and say which `rocm examine` fields appear only in the
JSON output. Link the page from the installation page.
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
* docs: move the WSL2 guide link out of the platform table
The table is single-sourced into the installation page, where the
GitHub URL in the WSL2 row sat next to the internal link to the new
WSL2 page. Drop it from the row and link docs/wsl.md from a sentence
after the table in the README instead.
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
* docs: add the WSL2 page to the docs index
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
* ci(docs): build the docs site when docs/wsl.md changes
The new WSL2 page single-sources docs/wsl.md through MyST `{include}`
directives, so an edit to that file alone must not skip the `-W` docs
build. Adds it to the `docs:` path filter alongside docs/vllm.md.
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
* docs: tighten WSL2 guide wording per review
Use "ROCm CLI" instead of rocm-cli in prose, attribute the non-login
shell to the explicit /bin/sh -c rather than wsl.exe --exec, and rewrap
an overlong line.
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
* docs: clarify the host-side WSL diagnose form and define the catalog
Explain what `rocm diagnose --distro` does, what "the same catalog"
means, and define "WSL catalog" at its first use in the published page.
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
* docs: name the --distro form as the subject of the WSL limitation bullets
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
* docs: move --distro detail into a WSL subsection and tighten claims
Move the host-side diagnose detail out of Prerequisites into a "Diagnose
a distro from Windows" subsection, state the limits as facts, and add
WSL1 and non-Ubuntu abstention notes. Keep the devs' 7.13+ wording for
HSA_ENABLE_DXG_DETECTION and add a Next steps link on the install page.
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
* docs: link WSL page references to site pages
Turn the text references in the single-sourced WSL sections into links:
same-page anchors for the prerequisites and diagnose sections, and
cross-page links to the command reference and installation guide. The
includes use :relative-docs: so the links resolve as site pages.
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
* docs: retarget WSL installation-guide link to the docs site
Copy the "Diagnose a distro from Windows" lead-in sentence into the
wrapper so its installation-guide link resolves within the site, and
split the include around it.
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
* docs: address review feedback on the WSL2 page
- Drop the site cross-links from docs/wsl.md that lychee cannot resolve
(the headings live only in README.md); link the installation guide to
README.md, and add a Command reference link to the site wrapper.
- Mark the four rocm examine items that appear only in --json output.
- Add the rocminfo/PATH limit to the --distro limits and say that the
shared checks (fix-1, fix-2, fix-6, fix-8, fix-16) also run on WSL.
- List fix-18 and fix-19 as skipped under --distro in the README.
- Return to "legacy ROCm release" without an unsourced version bound.
- Reword the manual-install trust claim: the CLI checks a digest pinned
in the source, which a digest read from the release page does not match.
Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
* docs: list the shared WSL checks exactly and drop the inert relative-docs option
The shared-checks paragraph in docs/wsl.md said fix-16-vllm-oom reads the
environment or the ROCm install and called fix-18 not an environment check.
fix-16 is driven by the --symptom text alone, and fix-18 reads the loader
path and the ROCm install. List the seven shared entries (fix-1, fix-2,
fix-6, fix-8, fix-16, fix-18, fix-19) exactly, without "such as".
Remove :relative-docs: from the four includes in install/wsl.md. MyST
rewrites a link only when it starts with the option's value, and no
included range of docs/wsl.md has a link starting with rocm-docs/, so the
option had no effect.
Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
---------
Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com>
Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com>
Backport note for v0.2.1: the guide's "Doing it by hand" section points
at `ROCDXG_PINNED_DIGESTS` in `apps/rocm/src/driver_install.rs`; that file
does not exist on v0.2.1, where the table lives in `apps/rocm/src/main.rs`,
so the path is adapted. The "Co-Authored-By" trailers of the squashed
commits are omitted. Everything else applied unchanged and was checked
against the v0.2.1 diagnose/fix catalog and the `--distro` probe.
(cherry picked from commit af9bb31)
Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
…fy guidance (#616) * docs: call out supported ROCm versions (7.14, 10.0, 10.1) State the default ROCm version and the supported versions in the ROCm installation intro and Getting started, and move the "ROCm 10 and newer" section ahead of driver installation. Explain that ROCm 10 or newer needs a `--version` pin and the exact GPU arch as a raw gfx code via `--family`. Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: copyedit README, CONTRIBUTING, and vLLM guide Editorial pass for style and consistency: remove em dashes and spaced double hyphens, use "instead of" and "through", replace and/or and word slashes, spell out TUI on first use, use American spellings, split long paragraphs, and use absolute GitHub URLs for doc links in CONTRIBUTING.md and README.md so they resolve on the docs site. No changes to technical content or to the anchor strings the docs-site include directives depend on. Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: tighten README flag lists and add lead-ins Keep the rocm diagnose flag list tight by moving the long --report and --send detail below it, add colon lead-ins to the flag lists that had none, and clarify the --family sentence. Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: rejoin prune lock sentence and complete acceptance-script list Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: add install verification and rocm uninstall guidance Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: clarify verify and uninstall guidance Add expected output and a PATH fallback to the verify section. Name the four uninstall categories, list managed ROCm SDK installs under --keep-data, and split the uninstall warnings into a list that also names the shared caches left in place. Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: link install pages to uninstall guidance Point the ROCm installation section and the site install page at Logs and cleanup, so readers who finish installing can find how to remove ROCm CLI. Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: give the install page uninstall pointer its own heading Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: fix command synopses and document model, chat provider, and automation modes Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: clarify that rocm chat --provider applies only to one-prompt use Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: correct contained-mode, chat flag, and model verbose descriptions Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: clarify uninstall cache and data scope, add APU case to diagnose Distinguish the managed uv cache from the shared one, list downloaded models under --keep-data, and include the APU case in the undetermined verdict conditions. Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: address review findings on uninstall, ROCm versions, and synopses Note that --keep-* flags don't protect their files by themselves on the default layout, where config and data share ~/.rocm. State the supported ROCm versions and describe the default as the latest stable release. Correct the storage and diagnose synopses, point the undetermined list at rocm model --verbose, narrow the "never removed" claim to rocm storage, and resync the copied getting-started sentence. Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: avoid calling 7.14 the latest stable ROCm release ROCm 10.1 is the latest stable release, so describe the unpinned `rocm install sdk` result as currently ROCm 7.14 and point to the `--version` and `--family` steps for 10.0 and 10.1. Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: address review comments on getting started, uninstall, and vLLM - Start the "Configure ROCm" section with `rocm examine` and keyed on `active_runtime_status: ready`; `rocm install sdk` is for a different version or configuration. - Name the command in the "install over an active default" sentence, and update the hand-copied sentence and include anchor in getting-started.md to match. - Add a `rocm uninstall` example and a `--dry-run` note to the Uninstall section of installation.md. - vllm.md: say "torch release" and split the long discovery sentence in three. Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: correct stale install, serve, and cleanup guidance - Say `--family` is needed for ROCm 10 only when ROCm CLI can't resolve an exact GPU arch or you target a different one. - Add the Windows and `ROCM_CLI_INSTALL_DIR` forms to the PATH advice. - Add the missing flags to the `rocm fix`, `rocm serve`, and `rocm engines install` synopses, with short descriptions. - Drop the unsupported "including 7.14" claim from the vLLM page. - Update the `rocm chat` and `rocm diagnose --send` help text, and the `rocm storage` heading and models note, to match current behavior. Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: align vLLM, uninstall, and help wording with the code Revert the rocm storage output wording and its loosened test so this PR stays docs-only; the wording moves to a separate change. Fix the vLLM pin-table scope sentence, the stale --keep-data claim in MANIFEST.md, and the --send help text. Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> * docs: tie the serve step to the install it follows "Then serve a model:" followed a paragraph about the activation gate, so "Then" had no clear referent. Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com> --------- Signed-off-by: pmoutsias-amd <peter.moutsias@amd.com> Signed-off-by: pmoutsia_amdeng <peter.moutsias@amd.com> Backport note for v0.2.1: docs and `--help` text only; each changed sentence was checked against the v0.2.1 code. Adapted or omitted: - README `rocm uninstall`: the PATH-undo and "removes the running `rocm.exe`" text describes #636, which v0.2.1 lacks. It now says that uninstall leaves the installer's PATH change and, on Windows, the running `rocm.exe` in place, and keeps the v0.2.1 `--force-dev-binaries` flag, its dev-build paragraph, and the service, remote-session, and shared-cache warnings. - README chat: on v0.2.1 the no-key ChatGPT fallback drops the sampling flags silently (the warning is #704, not in v0.2.1), so it says that. - README quit: the quit-while-serving confirmation (#595) is not in v0.2.1, so that paragraph is omitted. - README fix: `info-20-gpu-no-rocm-support` does not exist on v0.2.1, so DIAGNOSE-ONLY is still described as reserved, with no entry carrying it. - CONTRIBUTING: the hawkeye no-file-filter and `SKIP=license-headers` text, the lychee prek hook, the `docs-links` coverage of `docs/architecture.md`, and the AGENTS.md link-citation rule describe #475, #582 and #602, none of which is in v0.2.1. The v0.2.1 hawkeye, `check-architecture-doc`, and `docs-links` paragraphs are kept, copyedited instead. - `--help`: the `rocm diagnose --send` and `rocm chat` doc comments are taken, because v0.2.1 already opens a prefilled mail and already routes a no-prompt terminal to the dashboard chat; nothing pins that text. (cherry picked from commit ea1d871) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
* fix(lemonade): enable backend version alignment on Windows Windows was hard-skipping backend alignment entirely: a `!runtime_is_linux()` early-return and a non-Linux `rocm_backend_resolves` stub that always returned `false` guaranteed no aligned backend could ever be verified and accepted there. Every Windows install fell through to Lemonade's own pinned-version install of the `llamacpp:rocm` backend, paying its full redundant multi-GB ROCm download even when rocm-cli already manages a compatible ROCm install (#392). Add a Windows verification path for `rocm_backend_resolves`: confirm `ggml-hip.dll` exists beside the llama-server binary, then confirm each required ROCm DLL (`amdhip64`, `hipblas`, `rocblas`, `hsa-runtime64`) has a matching file, by case-insensitive filename-prefix match, either in the backend directory or in the real resolved PATH the process would run with. This is weaker than Linux's `ldd`-based check (a prefix match doesn't confirm the OS loader can actually load and link the file), but is strictly better than the previous status quo of alignment never even being attempted. Signed-off-by: Juho Vainio <juho.vainio@amd.com> * fix(lemonade): make Windows backend-resolution logic testable on Linux CI jussielo-amd review finding on #576: rocm_backend_resolves (Windows) had no regression test beyond its private library_present_in helper, so the "alignment can never be verified" bug's actual fix surface went untested end-to-end outside the separate self-hosted Windows CI lane. Split the DLL-presence verification out of rocm_backend_resolves into windows_backend_resolves(backend_dir, search_dirs), gated cfg(any(target_os = "windows", test)) like this repo's established Linux-equivalent pattern (host_gpu.rs's kfd_gpu_nodes_in, examine.rs, etc.) so it compiles and runs under plain `cargo test` on every target, not only real Windows builds. Added a cross-platform regression test exercising it directly across multiple search dirs (backend dir + a separate PATH-like dir), asserting true when every required DLL is present and false when one is missing. rocm_backend_resolves itself (the real dispatcher that resolves PATH via lemonade_process_environment_vars/ParentRuntimeEnvironment) stays Windows-only, along with its existing rocm_backend_resolves_requires_every_dll_present test. Signed-off-by: Juho Vainio <juho.vainio@amd.com> * fix(lemonade): address review findings on Windows backend alignment - rocm_backend_resolves (Windows) now falls back to the real process's inherited PATH when the vars map has no "PATH" entry, instead of treating absence as "not found". The vars map is only ever additive on top of the spawned child's real environment, never env_clear()'d, so a missing entry there does not mean the child sees no PATH. - rocm_backend_resolves_requires_every_dll_present now drives the new rocm_backend_resolves_with_vars helper with an explicit, controlled PATH instead of resolving the live CI runner's real environment, making the regression test hermetic. - prepare_llamacpp_backend_for_active_rocm_impl now takes the OS gate (neither Linux nor Windows) as an injectable unsupported_platform bool, matching the existing disabled-bool pattern, with a new test covering that every lookup below the gate is skipped. - Tag @id:runtime-lemonade-backend-alignment-reported with @requires-os:windows in runtime_setup.feature, matching install_lifecycle.feature's precedent and the README's tag table. - Correct docs/engine-plugins.md: alignment is attempted on Linux/WSL and Windows, and is only skipped outright on any other OS. Addresses blocking findings #2-#6 from review 5513617425. Finding #1 (commit e17b137's non-identity author/committer and unverified signature) is not addressed by this commit: no local git config override exists in this worktree to unset, and rewriting the already pushed commit's identity requires an explicit human decision on rebasing and force-pushing, which this commit does not make. Signed-off-by: Juho Vainio <juho.vainio@amd.com> --------- Signed-off-by: Juho Vainio <juho.vainio@amd.com> (cherry picked from commit 80ca67f) Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
Collaborator
Author
|
Changes since the review of
The |
rominf
dismissed
juhovainio’s stale review
October 29, 2026 11:59
Stale review, please have a look at new changes.
juhovainio
approved these changes
Oct 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
v0.2.0 ships several bugs that
mainhas since fixed, and some of them destroy user data or act on the wrong thing:rocm uninstallcould empty the folder a symlinked root points to, a ComfyUI reinstall could delete the user's own files,rocm fix fix-6-pathcould wipe the Windows user PATH after a failed read,rocmdcould signal an unrelated process that had reused a stale PID,rocm updatecould offer a downgrade, androcm install sdk --version 10.0.0failed because torchaudio was left unpinned. The docs site builds frommain, so it already describes the fixed behaviour while the released binary still has the bugs.mainhas also gained features and large refactors since v0.2.0, so taggingmainwould not be a patch release.What changes
This branch is the v0.2.0 tag plus fixes cherry-picked from
mainwith-x, so each commit names themaincommit it came from. It contains bug fixes and documentation corrections only, each fix with the regression tests it was merged with.rocmdverifies a process's identity before stopping it), fix(therock): order runtime versions by PEP 440 sorocm updatenever offers a downgrade #566 (rocm updatenever offers a downgrade).llamacpp:rocminstall fails outright whenever the host serving the matching Windows TheRock tarball refuses it, as it currently does fortherock-dist-windows-gfx1151-7.14.1.tar.gz(HTTP 403).Runs:lines namedrocm doctor,rocm update --checkandrocm install --channel …, none of which parse; they now namerocm examine,rocm updateandrocm install sdk --channel … --format …, checked against what each screen runs.docs/wsl.mdchanges) and docs: copyedit README, CONTRIBUTING, and vLLM; add uninstall and verify guidance #616 (README, CONTRIBUTING and vLLM copyedits, plus uninstall and verify guidance), so the published docs match this release. Every changed sentence was checked against this branch: text about behaviour that is not in v0.2.1 is adapted or left out (uninstall does not undo the installer's PATH change, the no-key ChatGPT fallback drops sampling flags without a warning, the dashboard does not ask before quitting while serving). With fix(therock): pin torchaudio to the requested ROCm 10 build #630 included, ROCm 10.0 and 10.1 are documented as installable.--require-signaturesverifies the signature instead of only finding a.sig), fix(ci): keep the GPU reclaim decoys alive on multicall coreutils #712 (GPU-reclaim self-test on multicall coreutils), fix(e2e): stop executor stalls from timing out chat-06 on the mock lane #662 (the chat-06 mock-lane stall that fix(comfyui): reinstall replaces only ComfyUI's code, never the user's files #546's new scenarios otherwise trigger).Most fixes had to be re-targeted, because
mainmoved code out ofmain.rsandlib.rsafter v0.2.0. Every commit where that needed a hand resolution ends with a "Backport note for v0.2.1" naming what was adapted or left out. The largest is #499: its restart rework is ported without the runtime-pin parts, which come from #424 and are not in this release.Scope
Left out on purpose: features and behaviour changes (#424, #595, #647, #679, #704, #646), refactors, lower-severity fixes entangled with those refactors (#526, #544, #675, #636, #676, #648, #698, #699, #674, #683), the rest of #535 (its advised-command scanner), and documentation for main-only behaviour.
Verification
The docs site builds with
sphinx-build -Wandlycheereports no broken links.cargo test --workspace --all-targets,cargo clippy --workspace --all-targets -- -D warnings,cargo fmt --checkand a Windows-targetcargo check -D warnings(x86_64-pc-windows-gnu) pass on this branch.The full mock-lane
cargo xtask e2esuite reports no unexpected failures. The two failing scenarios are the expected-fail rowsserve-01andserve-02.dash-01checks the corrected diagnose label on screen, and the label unit test fails ifrocm doctoris restored.fix(rocmd): verify process identity before stopping a managed service #499: with the identity token forced to
None,stop_managed_service_refuses_a_pid_whose_recorded_identity_no_longer_matchesandstop_managed_service_leaves_the_subtree_of_a_refuted_pid_runningfail, and both pass with the fix.service-stop-01andservice-stop-02pass.Known limit:
rocm fix fix-6-pathon Windows (fix(rocm-core): stop fix-6-path from wiping Windows User PATH on a failed read (ROCMAI-449) #691) reads the userPaththrough[Environment]::GetEnvironmentVariableand writes it back asREG_SZ, so%VAR%entries in aREG_EXPAND_SZuserPathare stored expanded.mainbehaves the same. v0.2.0'ssetx PATH "%PATH%;…"also wroteREG_SZ, and additionally merged in the systemPathand truncated it at 1024 characters, so this release is strictly better there; a kind-preserving write is a follow-up onmain.Not verified: the GPU-lane scenarios, and the Windows-only code paths of fix(rocm-core): stop fix-6-path from wiping Windows User PATH on a failed read (ROCMAI-449) #691 and fix(fix-6-path): stop advertising setx for the Windows PATH write #710, which were type-checked but not run on a Windows host.
rocm services restarton the ported fix(rocmd): verify process identity before stopping a managed service #499 code is covered by unit tests only.If this PR fixes a bug, searched
tests/e2e-cucumber/expectations.tomlfor the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No row refers to these fixes; the e2e run reports no XPASS.)If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per
docs/architecture.md. (No new subcommand.)Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state — not only the wording — per AGENTS.md §3. (Messages and their tests are carried unchanged from the merged PRs.)