[rominf]

Fixes #515.

Problem

RocmCliConfig::load treats a missing config.json as "use defaults" and an unreadable one as an error. Three code paths ignored that difference:

• Managed-Python bootstrap wiped the config. The first time the CLI installs its own Python (system Python can't create a venv — common on Windows, and on Linux without python3-venv), record_managed_python_config loaded the config with unwrap_or_default(), added the interpreter and saved. Both callers ignored the result. An unreadable config was silently replaced with defaults, losing the active and default runtime, the TheRock folder, providers, permissions mode, engine preferences and dashboard chat settings. Every kind of damage did this: truncation, a null tick, a field with the wrong type, an out-of-range value.
• Data-dir discovery guessed. AppPaths::discover reads setup.therock_venv from the same file to find the data dir, and turned any read or parse error into "nothing recorded". A truncated config moved the data dir (registry, active-runtime marker, service records) back to the default without a message: a --prefix install's runtimes vanished, and new state landed in the wrong place.
• Saves could produce the damage. save used fs::write (truncate, then write), so a full disk mid-install left a truncated file. Non-finite dashboard ticks serialized as JSON null, which load rejects, and legacy rocm-dash TOML migration copied them unchecked. rocm-dash's own reader already rejects such a file, so the two readers disagreed.

What changes

1. No more wipe. An unreadable config fails the managed-Python step with "cannot record the managed Python in … it was left unchanged", plus how to recover. Skipping the record with a warning was rejected: activating the new runtime loads the config strictly, so the install could not finish anyway. Failing here stops before the runtime download.
2. Atomic, durable, faithful saves. save writes through a stage-then-rename writer. The staged data is flushed before the rename; on Unix the directory is flushed after it. Bytes are parsed back before anything is written, so a value load would reject fails the save and leaves the old file alone. The writer keeps the replaced file's Unix mode (a 0600 file stays 0600). If config.json is a symlink (a dotfiles checkout, say), the link's target is replaced and the link survives. A symlink loop is an error.
3. Discovery agrees with load. A missing file still means "nothing recorded". An unreadable one (unparseable, wrong shape, or an I/O error such as a permission problem) is an error. When load accepts the file, discovery reads it exactly as load does. A valid-JSON file that load rejects for some other field still yields the recorded data dir; load reports that field itself. load itself draws the same line: only a missing file means defaults, and a directory at config.json, a permission error or a symlink loop is an error.
4. Migration matches rocm-dash. A legacy tick that Duration::try_from_secs_f64 rejects (NaN, ±inf, negative, overflow) refuses the migration. The key is named, nothing is written and the TOML is untouched. Replacing the bad value with a default was rejected: rocm-cli would then honour a file the dashboard ignores. The refusal also warns that migration only runs while config.json doesn't exist.

The recovery advice is checked against what each step actually does

Example of the new discovery error, for a file cut off just after the folder:

cannot tell where rocm-cli keeps its data: …/config.json cannot be read (not valid JSON: …). Repair that file to restore your settings. It still names /data/rocm as the ROCm folder: setting ROCM_CLI_DATA_DIR=/data/rocm points commands back at it meanwhile. Until the file is repaired, any command that changes settings still stops on it, and other commands either stop too or run on default settings. Moving the file aside resets every setting to its default, and the runtimes under /data/rocm then also need ROCM_CLI_DATA_DIR=/data/rocm

The folder is named only when the damaged bytes still hold the complete string; a value cut off part-way is never guessed at. Unit tests follow each piece of advice and check the result:

• the suggested ROCM_CLI_DATA_DIR finds the same data dir the healthy file did;
• with that override, load still fails, and the lenient read-only loaders run on default settings without saving;
• moving the file aside puts the data dir back to the default.

Blast radius: what now refuses on a corrupt config.json

This change makes more commands stop, on purpose. There are two cases.

A — the file can't be parsed or read (for example, truncated), and ROCM_CLI_DATA_DIR is unset. AppPaths::discover now errors, so every command that finds its paths that way stops with the message above:

• rocm itself: version, bare rocm (launcher), freeform requests, setup, install, runtimes, config, engines, serve, services, comfyui, examine, storage, automations, uninstall, remote (serve/status/attach/stop), dash/chat/bench and the dashboard flows, and the engine shell.
• All of rocmd: run, supervise, status, bridge-snapshot, sandbox-run. They all go through run_cli's discovery. A daemon that is already running is unaffected until it restarts. The daemon starts the rocm binary with ROCM_CLI_DATA_DIR set, so those child runs skip this check.
• Engine plugins started by rocm get ROCM_CLI_DATA_DIR from their parent, so they skip this check; started by hand without it, they stop too.
• Still running, as before: rocm diagnose, startup logging, the legacy-uv-cache notice and the startup update check. These already treat a discovery error as "no paths".

Before this change these commands carried on against the default data dir. Read-only answers came out wrong ("ROCm SDK: not detected" with a runtime installed in the custom folder). Writes landed in the wrong place: an install downloaded into the default dir, then failed at activation anyway.

B — the file parses but load rejects a field (for example, chat_top_p: 1.5). Discovery still works. Commands that already loaded strictly fail as before. The one new failure is the managed-Python step of rocm install sdk, which used to wipe the file here (rocm update already loads the config strictly before reaching it).

Why this is safer: every command that fails now either already failed on this file further in, or was running against a guessed data dir whose answers were wrong and whose writes went elsewhere. Stopping costs one command and names the file. Carrying on cost the user's settings or runtimes, silently. Two escape hatches remain:

• ROCM_CLI_DATA_DIR keeps the read-only commands that load settings leniently working, on default settings. No lenient loader saves, so none overwrites the file.
• rocm diagnose still runs.

The writer move

The stage-then-rename writer lived only in the rocm binary (therock.rs), with a near-identical copy in rocmd. It now lives in rocm_core::atomic_write. Both binaries use it and rocmd's copy is gone (cargo xtask check-crate-edges passes; rocmd already depended on rocm-core). Main has since moved rocmd's copy into apps/rocmd/src/sandbox.rs (#483); it is removed from there.

Its tests moved with it, and duplicates were merged. Only write_file_atomically and stage_file_for_atomic_publish are public API. write_file_atomically_with_publish and publish_temp_file stay public but #[doc(hidden)], because the rocm binary's metadata-cache tests use them to make the final rename fail or race.

On Windows, a ReplaceFileW that fails part-way (ERROR_UNABLE_TO_MOVE_REPLACEMENT or _2) can leave the destination gone. The writer retries once as a plain rename; if the destination is still gone, it keeps the temp file and names it in the error rather than deleting the only copy of the new contents.

It is now the one general writer. #546 later added write_atomically in apps/rocm/src/comfyui/source_swap.rs, which stays separate because its temp-file prefix is what the swap's recovery removes and it carries the crash tests' step hook; docs/architecture.md says so.

Side effects for rocmd's users: a full disk is now reported through the shared out-of-space message, and every write is flushed (a little slower, deliberately).

Dependencies

No new dependency edges: proptest was already a dev-dependency of apps/rocm on main. Cargo.lock only loses windows-sys from rocmd and rocm, whose only use was their own ReplaceFileW copy (cargo xtask manifest --check passes). rocm-core turns on windows-sys's Win32_Storage_FileSystem feature for ReplaceFileW.

Commits

1. test: property-test persisted config, registry and marker state — proptest properties for persisted state. They were committed with #[ignore = "CONFIRMED BUG: …"] reproducers; the later commits un-ignore the ones they fix:
   • fixed and now running normally: config_save_never_writes_a_file_load_rejects, legacy_toml_migration_always_produces_a_loadable_config, data_dir_never_silently_relocates_when_config_is_unreadable, managed_python_recording_never_replaces_an_unreadable_config_with_defaults, every_corruption_class_is_wiped_by_managed_python_recording (renamed no_corruption_class_is_wiped_by_managed_python_recording), legacy_inf_tick_migration_then_python_bootstrap_loses_migrated_settings (rewritten as legacy_inf_tick_never_reaches_config_json);
   • still ignored, out of scope here: runtime_manifest_round_trips_through_the_registry (a non-UTF-8 install folder is written lossily; ignored on Unix, and runs and passes on Windows, where such names cannot occur) and unknown_keys_survive_a_load_save_cycle (an older binary drops keys a newer one wrote).
2. refactor: move the writer into rocm-core.
3. fix: atomic save that refuses contents load would reject.
4. fix: migration refuses ticks rocm-dash rejects.
5. fix: discovery stops guessing the data dir.
6. fix: the managed-Python step never replaces an unreadable config.
7. fix: the recovery advice says what each way out does; read errors other than "not found" are covered.
8. test: both managed-Python callers fail on an unreadable config.
9. refactor: one writer for rocm and rocmd, tested in rocm-core.
10. fix: durable writes that keep permissions and symlinks.
11. fix: the migration refusal warns the window closes once config.json exists.
12. docs(testing): how to run the properties and the ignored reproducers.
    13–18. test: the property tests run on Windows (install roots Windows can store, the host's path form, unix-only helpers); clippy fixes; rocmd's coverage floor lowered for the writer it no longer owns.
13. fix: load treats only a missing config.json as no settings.
14. docs: which atomic-write entry points keep a symlinked file's link.
15. test(e2e): config-09 runs the ROCM_CLI_DATA_DIR value the refusal printed, read from its stderr.
16. test: the marker check moves into runtimes_cmd after ROCMAI-89: extract runtimes command and SDK-install-finalization clusters #653 made the marker functions private there.
17. fix: keep the new contents when a Windows replace fails part-way.
18. fix: say what ROCM_CLI_DATA_DIR does to a broken settings file; config-09 checks each claim; config-10 and config-11; discovery's redundant typed parse dropped.
19. test: the migration round-trip property can fail; reach: lines need --nocapture.
20. chore: drop rocm's unused windows-sys.
21. docs(architecture): name the ComfyUI swap's own atomic writer.

Tests

• Every fix was shown red before and green after by reverting only that fix. A mutation pass over the final tree caught all 12 mutations, including the review's two survivors ("non-NotFound read error → Ok(None)", "callers back to let _ =").
• Read errors other than "not found" are tested with a config.json that is a directory. That is a non-NotFound error on every platform, with no chmod or root needed.
• ensure_managed_python is driven end to end against a stand-in uv planted where the managed copy lives (no download, no environment override). On the reuse path it must fail before uv python install runs; on the install path, before returning success. Both leave the file byte-for-byte. Unix only: the stand-ins are shell scripts, as in the existing resolver tests, so the Windows lane does not run them.
• Scenario config-09 (@id:config-unreadable-settings-not-guessed, mock lane, every OS). It plants a config.json cut short after it recorded a custom folder, runs rocm version without ROCM_CLI_DATA_DIR, and asserts:
  • the refusal names the file, the folder, and the exact ROCM_CLI_DATA_DIR= value;
  • the file is byte-for-byte unchanged;
  • opening the CLI with that exact value reports the recorded folder as its data dir, on default settings, and the file is still unchanged;
  • changing a setting with that value is refused, and the file is still unchanged;
  • after moving the file aside, the data dir is the default folder again, on default settings.
• Scenarios config-10 (@id:config-save-keeps-a-private-settings-file-private) and config-11 (@id:config-save-keeps-a-linked-settings-file-linked), mock lane, Linux only (@requires-os:linux): a 0600 settings file stays 0600 after a save, and a settings file that is a link stays a link while its target gets the new setting.
• Not covered by an e2e scenario: the managed-Python failure in rocm install sdk. Reaching it needs a host whose system Python can't create a venv, plus uv and a Python download. It's covered by the unit and property tests above instead.
• Not testable here: whether data survives a power cut. The fsync calls can't be observed without a crash harness.

What the Windows lane does and does not exercise

• Not run on Windows: the two ensure_managed_python_*_fails_on_an_unreadable_config tests are #[cfg(unix)], because the stand-in uv and Python are shell scripts. The Windows lane therefore never runs the managed-Python caller tests; on Windows those callers are covered only by the shared record-function tests and the property tests, which do run there.
• Type-checked, not run: the Windows arm of the atomic writer (ReplaceFileW publish, and keeping permissions by relying on ReplaceFileW rather than copying them) was type-checked against x86_64-pc-windows-msvc in a scratch crate. A deliberately injected type error showed the check really compiles that code, but nothing executed it. The required windows-build-and-test lane is its first real run. The same holds for the partial-replace recovery: it is type-checked, and the Windows lane is its first run.
• Unix-only by design: the permission-preservation, symlink and symlink-loop writer tests. The Windows lane skips them. saving_the_config_replaces_the_file_rather_than_rewriting_it_in_place has a Unix form (it watches the old file through a hard link and its inode) and a Windows form (GetFileInformationByHandle); the duplicate Unix writer test it overlapped with is folded into it.

Docs

• README (Configuration): how settings are saved, and the recovery choices with what each one does.
• docs/manual-testing.md: an optional unreadable-settings check.
• docs/testing.md: how to run the persisted-state properties and the --ignored reproducers.

Not in this PR

• Making rocm diagnose / rocm fix recognise an unreadable config.json (separate issue; it belongs to the doctor-skill catalog).
• The rocm-dash TUI's persist_chat_endpoint, which does the same load-defaults-then-save on the legacy config.toml (separate issue).
• Atomic writes for the runtime registry manifest and the managed-Python manifest. (The active-runtime marker already uses the shared writer.)

[jussielo-amd]

This is a genuinely careful piece of work — the commit-by-commit structure, the red-before/green-after discipline on each fix, and the mutation-testing pass (12/12 caught) are well above the median bar, and the atomic-writer test suite (permission preservation, symlink replacement, symlink-loop rejection, concurrent writers, a documented ENOSPC repro) is thorough. The e2e scenario for the unreadable-config case is also well built: it checks byte-for-byte file preservation, not just message wording.

Two things need to be resolved before this can merge, though:

• windows-build-and-test is currently red: runtime_manifest_round_trips_through_the_registry and runtime_manifest_round_trips_for_utf8_install_roots both fail on Windows. Root cause looks like a gap in the new proptest generator — python_launcher/python_executable get arbitrary strings including a bare "/", but production code (normalize_runtime_path_text_for_host/_for_storage) normalizes path-like text to the host's native separator on save/load, so the round-trip isn't byte-for-byte on Windows for that input. Separately, the PR description still describes runtime_manifest_round_trips_through_the_registry as "ignored, Linux only," but its #[cfg_attr(unix, ignore...)] doesn't cover Windows, so it runs (and fails) there too — worth fixing the description along with the test.
• RocmCliConfig::load() still gates on path.is_file(), which quietly returns false (no error) for a directory sitting at config.json, and folds any other metadata error (permission denied, a symlink loop) into that same false. In both cases load() returns Ok(default()) — the exact "unreadable config silently replaced with defaults" behavior this PR sets out to kill, just via a different trigger than the one it tests for. discover() correctly rejects a directory at that path (and it's tested), but load() itself doesn't, which also means the new doc comment claiming the two "agree" isn't quite true yet for this case. configured_managed_root_from_config already has the right pattern next door (fs::read + an explicit NotFound match), so this looks like a small, local fix rather than a redesign.

Nothing else here is blocking. A couple of small things worth a look when you're back in this code: stage_file_for_atomic_publish's public API doesn't resolve a symlink target before staging the way the other two writer entry points do (not hit by any current caller, but worth a doc note or guard), and a test comment in config_steps.rs slightly overclaims that it "runs the advice as printed" when it actually reuses a pre-validated fixture value.

[rominf]

Rebased onto main c1c2aeb3; the head is now 14f68b5f.

• rocmd's writer: main had moved rocmd's atomic writer into apps/rocmd/src/sandbox.rs (ROCMAI-83: common.rs follow-up, webhook.rs, cli.rs and sandbox.rs extraction from apps/rocmd/src/lib.rs #483), so I removed it there by hand. The removed lines match this PR's original deletion line for line.
• rocmd's windows-sys: that dependency is now unused, so it goes too. This is the same edit refactor(core): share one atomic writer between rocm and rocmd (ROCMAI-681) #610 makes; whichever of the two PRs merges second will need a small rebase.
• Config loads: I checked every command that loads the config. None that main added since the branch was cut saves after a lossy load. record_managed_python_config remains the only such site, and this PR fixes it.

On the review of 808aa9e2, point by point:

1. Windows proptests: fixed earlier in 6a50721b, 808aa9e2 and 4a8cdd08. Generated install roots are limited to names Windows can store, and the expected record is in the host's path form; windows-build-and-test passed. runtime_manifest_round_trips_through_the_registry is ignored only on Unix, where non-UTF-8 folder names exist. On Windows it runs and passes. The description said "Linux only" and is now corrected.
2. load() and is_file(): fixed in 11899ae7. load now reads directly and treats only NotFound as absent, so it agrees with discovery. A directory at config.json now fails both, and the managed-Python step reports it as "left unchanged". Restoring the is_file() gate makes both new assertions fail.
3. stage_file_for_atomic_publish: its doc now says it takes the path as given, and that publishing over a symlink replaces the link (bad2b3a6). Its only caller verifies the staged file and then deletes it, so it never publishes over a link.
4. config_steps.rs: the step now reads the ROCM_CLI_DATA_DIR value from the refusal's stderr, so it really follows the printed advice (14f68b5f).

Verified locally on the new head:

• fmt
• tests for rocm-core (617), rocmd (126), rocm (1035), xtask (247) and e2e-cucumber lib (142), plus feature_naming
• both clippy invocations and check-crate-edges
• a Windows-target cargo check --all-targets -D warnings
• cargo xtask coverage; rocmd measures about 75.1%, above the PR's floor
• cargo xtask e2e -- -n config-09

On the old head, MI300X failed only therock-next-09, where vLLM on the ROCm 10 preview cannot load libtorchaudio. The same scenario fails on other PRs, so it is unrelated to this change.

This reviewed 808aa9e. The head is now 14f68b5, rebased onto main. The Windows proptest findings were fixed by 808aa9e and 4a8cdd0. 11899ae makes load() treat only NotFound as absent, bad2b3a documents stage_file_for_atomic_publish's symlink behaviour, and 14f68b5 makes config-09 run the value the refusal printed. All required checks are green on 14f68b5.

[rominf]

Rebased onto main 0573d7fb to resolve a conflict with #562, which also edited the comment above proptest in apps/rocm/Cargo.toml. I took main's version of that file; this PR's other changes are line-for-line unchanged. The head is now 33e125e1.

Verified locally on the new head, all exiting 0:

• fmt
• tests for rocm --bin (1040), rocm-core (612), rocmd (126), xtask (247) and e2e-cucumber lib (142), plus feature_naming
• check --test e2e
• both clippy invocations
• a Windows-target cargo check -D warnings

[rominf]

Rebased again, onto main 34fbdfe9; the head is now 2a40e182. #584 added a command_failure_detail import to apps/rocmd/src/sandbox.rs next to this PR's atomic_write import, and I kept both. Every other line of the diff is unchanged.

Verified locally on the new head, all exiting 0:

• fmt
• tests for rocm --bin (1040), rocm-core (615, including rocm-core: share command_failure_detail across vllm, rocm, rocmd #584's), rocmd (126), xtask (247) and e2e-cucumber lib (142), plus feature_naming
• check --test e2e
• both clippy invocations
• a Windows-target cargo check -D warnings

[jussielo-amd]

Builds on the prior review round (jussielo-amd, against 808aa9e). Both blockers from that round are fixed and verified against current HEAD (2a40e18), not just trusted from the dismissed state:

• The Windows proptest round-trip failures are fixed: bd9835cd and 31d9f196 adjust the install-root generator to avoid names Windows normalizes away (trailing dot/space), and 86d9cc4a compares the loaded manifest against the saved value in host path form instead of the raw value. windows-build-and-test is green on the current head (confirmed via gh pr checks).
• RocmCliConfig::load() (crates/rocm-core/src/lib.rs) no longer gates on path.is_file(). 9e9fea64 rewrote it to fs::read + match on ErrorKind::NotFound, so a directory at config.json or a symlink loop now fails the same way AppPaths::discover already did, with a new regression test (lib.rs ~7539) and a managed-Python-recording test for the same case in therock_persisted_state_props.rs.
• The two minor items (stage_file_for_atomic_publish's missing symlink note, and the e2e step comment overclaiming it "ran the advice as printed") are also addressed: b7144c7b documents the symlink behavior on stage_file_for_atomic_publish/publish_temp_file (crates/rocm-core/src/atomic_write.rs), and 2a40e182 makes config_steps.rs's version_with_suggested_data_dir step actually read ROCM_CLI_DATA_DIR out of the refusal's own stderr instead of reusing a value the test already knew.

I then did a fresh pass over the diff (atomic_write.rs, lib.rs's load/save/migration/discovery rewrite, the therock.rs/sandbox.rs writer deduplication, and the new proptest suites) looking for anything new. Nothing rises to a blocker:

• resolve_symlink_target (atomic_write.rs:119) deliberately replaces a symlink's target rather than the link itself, so the link survives a save — this is the documented, intended behavior for a dotfiles-managed config.json and is covered by write_file_atomically_replaces_a_symlinks_target_and_keeps_the_link and write_file_atomically_refuses_a_symlink_loop (atomic_write.rs:601,643), not a new overwrite hazard.
• The managed-Python step's load error now propagating instead of being swallowed (therock.rs:5555, call sites 5709/5779) is exactly this PR's stated fix, and the PR description's "Blast radius" section already discloses that the rocm install sdk managed-Python failure path has no e2e scenario and explains the unit/property-test coverage used instead, which satisfies the "state the gap" bar in AGENTS.md §3.
• The legacy-TOML tick-refusal behavior (lib.rs ~2908) has a dedicated unit test, migrate_legacy_dashboard_toml_refuses_an_invalid_tick_and_writes_nothing.
• save_runtime_manifest's non-atomic, non-transactional two-file write (therock.rs:6300) and write_active_runtime_marker's delete-then-rename (main.rs, untouched by this diff) are real but pre-existing and out of scope — the PR's own "Not in this PR" section already names the runtime registry manifest and the active-runtime marker as excluded.

Worth a follow-up, non-blocking: the fresh_paths/NEXT_DIR scratch-dir helper is duplicated across the three new proptest files rather than shared; configured_managed_root_from_config now does a full RocmCliConfig parse before falling back to its targeted Value lookup, and config.json ends up read/parsed twice per invocation (once in discovery, once in load); RocmCliConfig::save re-deserializes the whole struct just to catch a non-finite float. None of these affect correctness.

CI is green on 2a40e18 (windows-build-and-test, clippy, coverage, E2E consolidated, etc.); the three hardware-gated lanes (MI300X, Strix Halo WSL2/Windows) are still pending, consistent with how those lanes run in this repo.

[rominf]

Rebased onto main 7a062ad7; the head is now b69f6a7e.

• Windows clippy: ci: run clippy on Windows so cfg(windows) code is lint-gated too #488 made Windows CI run clippy, and b69f6a7e keeps the two cfg(not(unix)) stubs in atomic_write.rs non-const, with the same allow and comment ci: run clippy on Windows so cfg(windows) code is lint-gated too #488 uses for its stubs.
• Architecture doc: docs(architecture): reflow module-map paragraphs into per-module lists #624 reformatted docs/architecture.md into per-module lists, so this PR's two edits are re-applied in that form: sandbox.rs no longer claims atomic-write helpers, and atomic_write.rs has its own entry.
• Everything else: the code diff is unchanged, and it now sits on top of test(core): let e2e builds re-root the hardware probes #510.

Verified locally on the new head, all exiting 0:

• fmt
• tests for rocm --bin (1041), rocm-core (619), rocmd (126), xtask (247) and e2e-cucumber lib (142), plus feature_naming
• check --test e2e
• both clippy invocations, on Linux and for x86_64-pc-windows-gnu

[rominf]

Rebased onto main 2844d797; the head is now 99d89921. It still merges cleanly with current main.

• EAI-7404: make runtime activation and rollback transactional #424: it had made RocmCliConfig::save atomic on its own, so save now keeps this PR's version: the bytes are checked to load back, then published through rocm_core::atomic_write. EAI-7404: make runtime activation and rollback transactional #424's two marker writes in main.rs call rocm_core::atomic_write directly.
• ROCMAI-154: extract net.rs/process.rs/paths.rs from rocm-core lib.rs #599: it moved data-dir discovery into paths.rs, so the unreadable-config refusal, its recovery advice and their tests now live there. The persisted_paths_props property is declared from paths.rs so it can still reach the private discover_from_paths. EAI-7404: make runtime activation and rollback transactional #424's config-save tests now point at the shared writer.
• Config loads: every config load→save path EAI-7404: make runtime activation and rollback transactional #424 added uses a strict load(..)?.
• Survival: the PR's diff is otherwise unchanged. Saving with a plain fs::write instead of the atomic writer fails five tests.

Verified locally:

• fmt
• tests for rocm --bin (1098), rocm-core --features e2e-test-hooks (635), rocmd (126), xtask (247) and e2e-cucumber lib (142), plus feature_naming
• check --test e2e and check-crate-edges
• both clippy invocations, on Linux and for x86_64-pc-windows-gnu

The one local rocm failure was sdk_tarball_unpack_extracts_a_benign_tarball_into_the_install_root ("failed to launch tar"), which passes when run alone. It is the PATH-mutation race tracked in #650.

Two things noticed but out of scope:

• ManagedServiceRecord::write, which EAI-7404: make runtime activation and rollback transactional #424's re-pin uses, is still a plain fs::write.
• No test fails if write_active_runtime_marker's forward write becomes a plain fs::write. That gap was already on main.

[siloteemu]

🔴 Automated review · pr-review-watcher · 99d8992

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

Review — needs work

Full review of the whole change.
Implements #515 (the managed-Python wipe, data-dir guessing, non-atomic save and the unchecked tick migration it lists). It does not close the "other writes are not atomic" item, which the description defers explicitly.

Blocking

• new · The recovery advice says the opposite of what most commands do with ROCM_CLI_DATA_DIR set — crates/rocm-core/src/paths.rs:397-398, README.md:868-869, docs/manual-testing.md (new block)

  • What the text says: the error tells the user "commands that read your settings keep failing until the file is repaired". The README and manual-test text repeat this.
  • What the code does: with ROCM_CLI_DATA_DIR set, discovery never reads config.json. About 20 production call sites then do RocmCliConfig::load(&paths).unwrap_or_default() and carry on with default settings without a word. Examples: version (apps/rocm/src/main.rs:1572), the bare launcher and freeform request (:1680, :1688), examine (:2810), comfyui (:3786), apps/rocmd/src/service.rs:448, apps/rocmd/src/common.rs:32, engines/lemonade/src/process.rs:177.
  • The PR's own scenario relies on the opposite: config-09 runs rocm version with the suggested folder and the file still broken, and expects success.
  • Which side governs: the code, because those callers are what actually runs.
  • Confidence 85 · logic · Fix: say "commands that change your settings fail, and others run on default settings until the file is repaired". Or make the lenient loaders warn when they fall back.

• new · legacy_toml_migration_round_trips_finite_values cannot fail on what its name and doc claim — crates/rocm-core/tests/persisted_config_props.rs:327, :351-360

  • The shared property body treats Err(_) | Ok(None) as a pass.
  • The "finite" generator includes -1.0 and f64::MAX, which this PR's migration now refuses. About 70% of cases therefore pass without migrating anything.
  • Mutation run: I made the tick check in migrate_legacy_dashboard_toml_from refuse every tick (try_from_secs_f64(-1.0 - secs.abs())). cargo test -p rocm-core --test persisted_config_props stayed green, and this property passed.
  • The doc comment ("finite ticks and arbitrary strings migrate and load back") is false of the test.
  • Confidence 100 · logic (test quality) · Fix: generate only ticks Duration::try_from_secs_f64 accepts, require Ok(Some(_)), and compare the migrated ticks, not only default_engine.

• new · docs/testing.md says every run prints a reach: line; a plain run prints none — docs/testing.md:142-145

  • The counters are eprintln! calls in passing #[test]s, and libtest captures that output.
  • Run: cargo test -p rocm-core --test persisted_config_props printed zero reach lines.
  • So the doc's safeguard ("a property whose reach is zero passed without testing anything") is invisible exactly where it matters, and nothing asserts any counter is non-zero.
  • Confidence 100 · mechanical · Fix: assert the meaningful counters are greater than 0 after runner.run, or add -- --nocapture to the documented commands and say the line only shows then.

• new · config-09's last Then cannot fail, and the scenario comment claims more than the steps do — tests/e2e-cucumber/tests/e2e/config_steps.rs:333 (moved_aside_settings_unchanged), tests/e2e-cucumber/features/config.feature:76

  • The last Then: the step renames the file to config.json.broken itself, and nothing in apps/ or crates/ refers to that name. The assertion checks only the test's own rename. The behaviour the refusal promises for this way out — settings reset, data folder back to the default — is not observed.
  • The comment: "Each way out it offers is then taken, as printed" is also untrue:
    • the repair route is never taken;
    • no runtime is planted in the custom folder, so "lets commands find those runtimes" is never checked.
    • Only the first Then's exact-string match guards the suggested value.
  • Confidence 85 · logic (test quality) · Fix: after the move-aside, assert something the user can see, such as the resolved data folder or "no active runtime". Narrow the comment to the ways out actually taken, or plant a runtime and assert version reports it.

• new · apps/rocm keeps a windows-sys dependency that nothing uses — apps/rocm/Cargo.toml:58

  • Its only use at base was ReplaceFileW in therock.rs, which this PR moves into rocm-core. A grep of apps/rocm for windows_sys now finds nothing.
  • The PR removes the same dead dependency from rocmd but not from rocm.
  • Confidence 85 · mechanical · Fix: drop the line and re-lock.

• new · On Windows, config.json now goes through ReplaceFileW, which has a documented failure that leaves no config.json — crates/rocm-core/src/atomic_write.rs:292-335, crates/rocm-core/src/lib.rs (save)

  • At base, save used fs::rename. Its doc said the destination is never removed first, because load reads a missing file as defaults.
  • publish_temp_file now calls ReplaceFileW whenever the destination exists, with no backup name, and returns the error without recovering.
  • Microsoft documents ERROR_UNABLE_TO_MOVE_REPLACEMENT_2 as "the file to be replaced still exists with a different name". The next load then returns defaults, and the next save writes them: the loss this PR exists to prevent.
  • The ReplaceFileW path already existed for the active-runtime marker; this PR makes config.json a new user of it. I argued this from the API documentation and the code path; I did not run it on Windows.
  • Confidence 70 · architectural · Fix: on that error, rename the displaced file back. Or use plain rename for config.json. Restore the "not guaranteed" Windows notes in save's doc either way.

• new · New user-observable file behaviour has no scenario and no stated justification — crates/rocm-core/src/atomic_write.rs (permission copy, symlink resolution), AGENTS.md §3

  • After rocm config …, a 0600 config.json now stays 0600, and a symlinked config.json keeps its link. At base the link was replaced by a regular file. Both are visible in files the CLI writes.
  • Only unit tests cover them. AGENTS.md says a unit test "does NOT discharge this" and asks for a scenario, or for the PR text to say why there is none. The description lists these tests as "Unix-only by design" but does not address the scenario requirement.
  • Confidence 85 · architectural · Fix: add a config.feature scenario (Linux lane is fine, tagged accordingly), or justify the gap in the PR text.

• new · The description claims dependency changes and a deferral that the diff contradicts — PR description, "Dependencies" and "Not in this PR"

  • It says proptest is "now used by apps/rocm too" and "Cargo.lock gains only the rocm → proptest edge". That edge is already on the base branch: apps/rocm/Cargo.toml is untouched and the only Cargo.lock change is removing rocmd's windows-sys.
  • It lists atomic writes for "the active-runtime marker (whose own temp-then-rename deletes the old file first)" as out of scope. write_active_runtime_marker (apps/rocm/src/main.rs:8431-8452) already uses the shared atomic writer and never removes the destination first.
  • Confidence 100 · mechanical · Fix: narrow the description to what the diff does.

Non-blocking

• new · Wrong reason in the new module doc — crates/rocm-core/src/atomic_write.rs:9-10
  • It says ReplaceFileW is used on Windows "where a plain rename cannot replace an existing file". std::fs::rename does replace on Windows, as the base save doc said.
  • Confidence 85 · mechanical · Fix: state the real reason, which is keeping the replaced file's attributes and ACL.
• new · Stale comment in the new property — crates/rocm-core/src/persisted_paths_props.rs:64
  • "An interrupted RocmCliConfig::save (a plain fs::write)" no longer describes save, which was already temp-and-rename on base and is atomic and durable here.
  • Confidence 85 · mechanical · Fix: describe the case as a truncated file (hand edit, or one damaged before this change).
• new · The new Unix test duplicates an existing one — crates/rocm-core/src/lib.rs:3890 vs :4380
  • config_save_replaces_the_file_instead_of_rewriting_it_in_place proves the same property as the existing #[cfg(unix)] saving_the_config_replaces_the_file_rather_than_rewriting_it_in_place. Its leftovers check repeats :4349.
  • Its doc names the Windows twin as its counterpart, which hides that a Unix twin already exists.
  • Confidence 85 · mechanical · Fix: fold the hard-link witness into the existing test.
• new · The commit trail should be squashed before merge
  • Six commits patch tests added earlier in this same PR: 06b78e0, 971eb4a, c260fc0, d72fe1f, 41c9e28, 0fb7345.
  • 655845f says save "truncated config.json and then wrote it", which is false against the current base.
  • Confidence 85 · mechanical.
• new · "E2E tests (MI300X)" has not reported for this head. A check that has not reported is not a pass, so that lane's class of coverage is not established yet. "Skill checks" and "Regenerate manifests" were skipped; neither applies to the files this PR touches.

Decisions for the author

• Strict discovery, lenient settings loaders — tradeoff
  • What happens now: for the same broken file, about 20 read-mostly commands error out when ROCM_CLI_DATA_DIR is unset, and run silently on default settings when it is set. rocm examine and rocm version now stop exactly when a user is trying to find out what is wrong.
  • Current choice: it is safe and actionable.
  • The alternative: read-only commands warn and fall back. That would make the env-var path consistent with the error text (see the first blocking finding).
• The test seam is public API — tradeoff
  • write_file_atomically_with_publish and publish_temp_file are pub #[doc(hidden)] only so rocm's tests can inject a failing publish.
  • Current choice: simple, and documented in the code.
  • The alternative: gate them behind a test-only feature, which keeps rocm-core's surface minimal.
• The writer now follows symlinks, and creates the parent of a dangling link's target — tradeoff
  • Current choice: the dotfiles case works.
  • Cost: for rocmd's artifact-cache marker, a dangling link planted in the data dir is now written through rather than replaced. Only the data dir's owner can plant one, so there is no privilege gain in the same-user model.
• configured_managed_root_from_config has a redundant strict fast path — non-blocking-improvement
  • The typed RocmCliConfig parse at crates/rocm-core/src/paths.rs:~338-347 returns what the lenient Value path below it returns for every input load accepts. It adds a full typed deserialize to every discover.
  • Drop it, or say in its comment what it adds.

Positive signals

• recorded_root_in_damaged_config names the folder only when the damaged bytes still hold a complete JSON string, and never guesses past a truncation.
• save parses its own bytes back before publishing, so a value load would reject can no longer replace a good file.
• config-09 reads the suggested ROCM_CLI_DATA_DIR from the refusal's own stderr, and its new helper points both HOME and USERPROFILE into the sandbox, so the default-folder fallback cannot reach the runner.
• The managed-Python tests plant a stand-in uv that logs its calls, which proves the refusal happens before any download.

What this covered

• Read: every file in prw-base...99d89921, completely. Also the callers of AppPaths::discover, RocmCliConfig::load/save and write_file_atomically across apps/, crates/ and engines/, and the issue named in the description (#515: title, body, state).
• How: a fan-out of four coupled-group passes, with Step 7's design questions run by a separate worker. I re-checked the surviving findings against the source.
• Runs:
  • built and ran the rocm-core tests in full;
  • ran rocm's legacy-migration filter;
  • two mutations. The first deleted instance_tick from the migration check and was caught by legacy_tick_readers_agree_on_which_files_are_readable, so a suspected gap was dropped. The second, refuse-every-tick, survived, as described above.
• Did not run:
  • the prior-changes pass, because review comments on earlier PRs to these files were not fetched;
  • anything on Windows (the ReplaceFileW finding is argued from documentation);
  • the e2e suite;
  • full-workspace clippy.
• Not reconciled: this report is not reconciled against any discussion on the PR.

[rominf]

Rebased onto main ee3131be; the head is now 7ae147d4. #653 moved the marker writes into runtimes_cmd.rs, so the two atomic_write calls are re-applied there. The PR's added and removed lines are identical before and after the rebase. The rebase also broke a test that git reported no conflict on: marker_key_drops_that_split_the_two_readers called marker functions #653 made private. It moved unchanged into runtimes_cmd's tests (56c836e2), rather than widening their visibility again. 7ae147d4 corrects docs/architecture.md's "the one writer" claim, because #546 added ComfyUI's own write_atomically. The description is updated to match everything below.

This push answers the automated review of 99d89921:

Blocking

1. Recovery advice under ROCM_CLI_DATA_DIR (00dfdefa). No lenient call site can wipe the settings, so I narrowed the message rather than changing call sites. It now says that any command that changes settings still stops, and the others either stop too or run on default settings. README, manual-testing, the doc comment and the unit tests match.
2. The migration property could not fail (ccc7c8b6). It now generates only ticks rocm-dash accepts, requires every case to migrate, and compares the migrated ticks. Your "refuse every tick" mutation now fails it. The comparison allows one ULP: serde_json without float_roundtrip reads some floats back one ULP off, which affects every float in config.json. That is worth its own issue.
3. reach: lines (ccc7c8b6). The documented commands pass -- --nocapture, and the doc says the line is captured without it. I did not add minimum-count asserts: proptest seeds randomly here, so small floors could flake.
4. config-09 checks each claim as printed (00dfdefa).
   • With the suggested folder, the CLI reports the recorded folder as its data dir, on default settings.
   • config set-telemetry is refused, and the file is left untouched.
   • After moving the file aside, the data dir is the default again.
   • Making config load leniently fails the refusal step.
5. windows-sys (42bed921). Removed from apps/rocm and re-locked; xtask manifest --check passes.
6. ReplaceFileW partial failure (4130b43a). It is worse than described. Under ERROR_UNABLE_TO_MOVE_REPLACEMENT with no backup name, the replaced file can be gone while the replacement is still under its temp name. Our cleanup then deleted the only copy. The writer now retries once with fs::rename. If the destination that existed before is still gone, it keeps the temp file and names it in the error. A unit test covers the keep path, and removing the keep branch fails it. Windows clippy type-checks it, and the Windows lane is its first real run.
7. Scenarios for the save behaviour (00dfdefa).
   • config-10: a 0600 file stays 0600.
   • config-11: a linked settings file stays a link, and its target gets the new setting.
   • Both are tagged @requires-os:linux. Removing the permission copy fails config-10; skipping symlink resolution fails config-11.
8. Description. Rewritten to match: the error text, the escape hatch, the Windows recovery, the dependency changes, the commit list, the tests and what the Windows lane runs.

Non-blocking: the module doc reason and the stale comment in persisted_paths_props.rs are fixed. The duplicate Unix test is folded in. On squashing, the repo squash-merges.

Decisions

• Strict discovery vs lenient loaders. Kept strict discovery; the message now says what actually happens with the override. Making the lenient read-only commands warn when they fall back to defaults touches about 20 call sites, so it belongs in a follow-up. None of them saves, so none can overwrite the file.
• Test seam as public API. Kept as pub #[doc(hidden)]. A test-only feature turned on from rocm's dev-dependencies would work, at the cost of one more feature flag; I can do it as a follow-up if you prefer the smaller surface.
• Symlink-following writer. Under the single-user model only the owner of the data dir can plant that link, so there is no privilege gain.
• Fast path in configured_managed_root_from_config. Removed (00dfdefa), with discovery_reads_the_recorded_folder_of_a_loadable_config_as_load_does. Removing the trim fails it.

Verified locally on the rebased head, all clean or passing:

• fmt
• tests: rocm --bin (1150; the two wait_with_output_bounded_* flakes pass alone), rocm-core (644), persisted_config_props (5), rocmd (126), xtask (252), e2e-cucumber lib (142) and feature_naming
• xtask manifest --check, cargo check --test e2e
• clippy, Linux and Windows, both invocations
• e2e config- (11 of 11) and comfyui- (13 of 13)

Addressed in 4130b43, 00dfdef, ccc7c8b and 42bed92 (head 7ae147d, rebased onto ee3131b). The PR comment of 2026-10-13 answers each finding point by point.

[rominf]

Rebased onto main 07166450 (#526); the head is now c9690e50.

• Conflict: one, in the imports of apps/rocmd/src/sandbox.rs. fix(rocmd): drain child pipes concurrently so output size stops faking timeouts #526 replaced sandbox's run_process_with_timeout with rocm_core::process::run_with_timeout and dropped the command_failure_detail import. This PR imports rocm_core::atomic_write::write_file_atomically instead of the local writer. The resolution keeps both changes.
• What fix(rocmd): drain child pipes concurrently so output size stops faking timeouts #526 means for this PR: it adds no new atomic-write call sites and does not use windows-sys in rocmd or rocm, so both removals stand. The PR's added and removed lines are line-for-line unchanged.
• Verified locally:
  • fmt;
  • tests: rocmd, rocm-core, rocm, persisted_config_props, the config- e2e scenarios (11/11), xtask, e2e-cucumber lib and feature_naming;
  • manifest --check and tpn --check;
  • cargo check --test e2e;
  • clippy, both invocations, on Linux and the Windows target.

[rominf]

Rebased onto fcc646e9; the head is now 92b1f7ed. It still merges cleanly with current main. On that merge, cargo test -p xtask passes (313, including the ASCII-output contract), and rocm, rocmd and rocm-core build.

• Moved code: ROCMAI-151: extract config/automation/service_record/binary_paths from rocm-core lib.rs #666 moved RocmCliConfig (load, save, migration and their tests) into config.rs. This PR's hunks are re-applied there with the same lines, plus a bail import.
• save: ROCMAI-151: extract config/automation/service_record/binary_paths from rocm-core lib.rs #666 had routed save through a new crate-private write_atomic. save now uses atomic_write, as this PR intended. write_atomic remains for the automation stores and service-record manifests, and the architecture note says so.
• fix(output): replace non-ASCII punctuation with ASCII in CLI output text #683's ASCII rule:
  • The Windows partial-replace error had an em dash; it is now -.
  • The persisted-state property file is renamed to therock_persisted_state_proptests.rs, so the guard treats it as test-only.

Verified locally, all green:

• fmt;
• tests: rocm-core (763), rocmd, rocm (1244), persisted_config_props, xtask;
• manifest --check and tpn --check;
• the config- e2e scenarios (11/11);
• e2e-cucumber lib, feature_naming and check --test e2e;
• clippy on Linux and the Windows target.

[nowycondro]

🤖 Automated review (Claude Code) · 92b1f7e

This automation never files a GitHub approval; the merge call stays with a human reviewer.

Scope and method

This PR already has two long, detailed review rounds (jussielo-amd at 808aa9e/2a40e182, siloteemu at 99d8992) that raised a substantial set of blocking findings. Per the batch instructions for this pass, I did not re-litigate that whole history. Instead I:

• Cloned the repo over HTTPS and checked out the PR head locally (92b1f7edc046daac22d8a0edde635de197198564), confirmed it matches the PR's reported head SHA.
• Diffed the last-reviewed commit (99d89921) against the current head to isolate what changed since siloteemu's round (several commits plus three rebases onto main, one of which relocated RocmCliConfig into crates/rocm-core/src/config.rs via an unrelated landed PR, #666).
• Read, line by line, each commit that responds to siloteemu's seven blocking findings: the Windows ReplaceFileW partial-failure fix (8342be1), the recovery-advice wording + redundant-typed-parse removal (32ba384), the migration round-trip property fix (3211ece), the unused windows-sys removal (abd1f3e), the e2e config-09/config-10/config-11 scenario rewrite (26a3d08 + related), and the symlink-handling doc note (a4d2e51).
• Re-read the core fix itself: record_managed_python_config (apps/rocm/src/therock.rs:5624) now uses RocmCliConfig::load(paths).with_context(...)? instead of unwrap_or_default(), and both call sites (therock.rs:5778, 5848) propagate with ? instead of discarding the result — this is the central behavior change the PR claims and it checks out.
• Cross-checked the author's own point-by-point rebuttal comments on the PR against the actual diffs (not just trusting the prose).

Findings

Blocking: none found. Every blocking item from the 99d89921 review round appears to have a corresponding fix in the commits since, and I verified each by reading the diff rather than trusting the commit message alone:

• replace_file_windows (crates/rocm-core/src/atomic_write.rs) now detects ERROR_UNABLE_TO_MOVE_REPLACEMENT{,_2} and retries once as a plain rename; if that also fails, the temp file is kept and named in the error instead of deleted. A new unit test (a_publish_that_removes_the_destination_keeps_the_new_contents) pins the keep-path.
• legacy_toml_migration_round_trips_accepted_ticks (crates/rocm-core/tests/persisted_config_props.rs) now filters to ticks Duration::try_from_secs_f64 accepts, requires every case to actually migrate (must_migrate: true), and compares migrated values (within 1 ULP, justified by serde_json lacking float_roundtrip) — this closes the "refuse every tick stays green" gap siloteemu demonstrated by mutation.
• apps/rocm/Cargo.toml's unused windows-sys is dropped and Cargo.lock re-locked.
• The e2e config_steps.rs suggested_data_dir helper now parses the actual suggested folder out of the refusal's own stderr (split_once("setting ROCM_CLI_DATA_DIR=") / split_once(" points commands")), rather than asserting against a value the test already planted — and I confirmed this parsing matches the current wording in unreadable_config_error (crates/rocm-core/src/paths.rs), i.e. it wasn't left stale after the wording changed in a later commit.
• Discovery's redundant typed-parse fast path in configured_managed_root_from_config (paths.rs) is removed, with a new regression test pinning that the lenient path alone agrees with load on every loadable file.

I did not find anything new introduced in this round of changes that would rise to blocking. The bulk of the lib.rs diff churn versus 99d89921 is the unrelated RocmCliConfig file-split (#666) rebasing in underneath this PR, not new logic.

Non-blocking, low confidence (not independently verified further):

• siloteemu's prior non-blocking item about ManagedServiceRecord::write and write_active_runtime_marker still using a plain write / delete-then-rename outside this PR's scope remains true at HEAD and is explicitly disclosed in the PR's "Not in this PR" section — flagging only so it isn't lost, not asking for action here.
• The PR has 30 commits including several "fixup"-style commits patching tests added earlier in the same PR (per siloteemu's prior note); whether to squash before merge is a repo-convention / maintainer call, not a correctness issue.

What I could not verify

• No Rust toolchain available in this sandbox — I did not build, run cargo test, clippy, or the e2e suite myself. All conclusions above are from static reading of the diff and tracing call sites, not execution. The author's own PR comments report local test/clippy/e2e runs at this head (and prior rebased heads) as green, including the specific config- e2e scenarios and the Windows-target cargo check, which I did not independently reproduce.
• I did not re-verify CI status/check results for the current head via gh.
• I relied on the author's rebase comments to confirm that the three intervening rebases (onto fcc646e9 etc.) carried the fixed logic over unchanged; I spot-checked this by diffing specific files/functions rather than diffing the entire tree commit-by-commit.

🤖 Generated with Claude Code

[nowycondro]

🔴 Automated review · pr-review-watcher · 92b1f7e

Review — needs work

Full review of the whole change (21 files, d59f596f6e08297c207f008021eecb37defc3558...92b1f7ed), via an 8-way blind fan-out plus direct verification of every cross-worker disagreement.
Fixes GitHub issue #515 ("a corrupt config.json is silently overwritten with defaults when the CLI records its managed Python") and names Jira ROCMAI-532 (status: In Progress, same bug). The PR description's detailed, itemized account of the change (the three original bugs, the atomic-write move, the Windows caveats, the explicit "Not in this PR" list) matches what every review pass independently found in the code — no scope mismatch.

Blocking

• CI check "E2E tests (Strix Halo, Windows, current)" is failing on the reviewed commit — reported by gh pr view 541 --json statusCheckRollup (job 117421948601, run 39041532923), re-confirmed immediately before this review was posted (head still 92b1f7ed, PR still OPEN/non-draft).
  This is not a generic CI nit: the PR description itself flags the Windows arm of the new atomic-write primitive (ReplaceFileW publish, and the partial-replace recovery path) as "type-checked, not run" prior to this PR, and states "the required windows-build-and-test lane is its first real run." windows-build-and-test (the build/unit-test lane) did pass, but the separate Strix Halo Windows E2E lane — which is where the real filesystem-level ReplaceFileW behavior actually executes — is red. That is exactly the gap the author called out as unverified. A reasonable reviewer needs this resolved (or shown to be an unrelated, already-tracked flake with evidence) before merging a PR whose entire point is durable, correct atomic writes.
  Confidence 100 · architectural · Fix: inspect the Strix Halo Windows E2E job's log and either fix the real failure in the Windows atomic-write/replace path, or if it's confirmed unrelated/flaky, say so on the PR with evidence and get it re-run green.

Non-blocking

• Newer, more durable atomic_write primitive isn't used everywhere in the same crate — crates/rocm-core/src/lib.rs:502 still defines the older, weaker write_atomic (no directory fsync, no symlink-target resolution, no Unix permission preservation), and crates/rocm-core/src/automation.rs:223,368 and crates/rocm-core/src/service_record.rs:345 still call it for automation audit logs/proposals and service manifests, instead of the new rocm_core::atomic_write module this PR introduces. Pre-existing, not introduced by this diff, and explicitly out of this PR's stated scope (the PR's "Not in this PR" list covers the runtime-registry/managed-Python manifests and the rocm-dash TUI config, but not these two). A reasonable follow-up for consistency, not a defect in what's here.
  Confidence 70 · architectural · Fix: file a follow-up to migrate automation.rs/service_record.rs onto rocm_core::atomic_write, or note why the weaker primitive is acceptable there.
• Cosmetic module-declaration placement inconsistency — apps/rocm/src/therock.rs: the new persisted_state_props proptest child-module declaration sits near the bottom of the file, while the pre-existing, same-pattern version_proptests declaration sits at the top. No effect on compilation or correctness.
  Confidence 70 · mechanical · Fix: move the new declaration next to version_proptests for consistency, if touching the file again.
• Slightly weaker assertion in one e2e step — tests/e2e-cucumber/tests/e2e/config_steps.rs, the "Unreadable settings file" / refuses_to_change_setting step only asserts rc != 0 and that stderr contains the bare substring "config.json", noticeably weaker than the multi-needle check used a few steps earlier in the same scenario (config-09). Plausibly deliberate — the richer message is already verified once in-scenario, and a comment notes repair is not exercised here — so this is flagged as a minor test-strength note rather than a defect.
  Confidence 70 · logic · Fix: consider asserting the same multi-needle message check here too, for symmetry with the earlier step in the same scenario.

Decisions for the author

None — the one real tradeoff in this change (many more commands now hard-fail on a corrupt config.json instead of silently falling back to defaults) is already deliberated and justified at length in the PR description's "Blast radius" section, with the two escape hatches (ROCM_CLI_DATA_DIR, rocm diagnose) called out; nothing here needs further confirmation from the author.

Positive signals

• Mutation-tested fix discipline: the PR description states a full mutation pass over the final tree caught all 12 injected mutations, and multiple independent review passes confirmed specific tests fail when a specific branch is mutated or a fix is individually reverted (e.g. managed_python_recording_never_replaces_an_unreadable_config_with_defaults, the per-call-site ensure_managed_python_reuse_path_fails_on_an_unreadable_config / ..._install_path_fails_on_... pair) — not vacuous happy-path-only tests.
• Clean, verified dedup: the stage-then-rename writer previously duplicated (with drift) between apps/rocm/src/therock.rs and apps/rocmd/src/sandbox.rs is now one implementation in crates/rocm-core/src/atomic_write.rs; grepping both call sites for leftover inline rename/tempfile/ReplaceFileW logic and for dangling references to the removed functions turned up nothing, and the corresponding windows-sys dependency removals from apps/rocm/Cargo.toml/apps/rocmd/Cargo.toml line up exactly with what was deleted.
• Honest, explicit disclosure of verification gaps — the PR description itself names which Windows-only paths were only type-checked before this PR (see the Blocking finding above), and a confirmed pre-existing, unrelated bug (runtime_manifest_round_trips_through_the_registry, non-UTF-8 install root) is left #[ignore]'d with a comment explaining why, rather than hidden or worked around.
• User-facing recovery messaging is itself test-verified: the new "cannot tell where rocm-cli keeps its data" error names the file, the recovered folder (only when determinable from the surviving bytes), and the exact ROCM_CLI_DATA_DIR= workaround, and e2e scenario config-09 exercises that the suggested override actually works, that the file stays byte-for-byte unchanged throughout, and that moving the file aside resets to defaults.

Deployment notes

• User-facing behavior change, by design: many rocm/rocmd commands that previously tolerated an unreadable config.json by silently falling back to default paths/settings now hard-fail with a descriptive error unless ROCM_CLI_DATA_DIR is set. Anyone upgrading with an already-corrupted config.json will see new failures post-upgrade. This is thoroughly documented in the PR body's "Blast radius" section and in the README changes, but is worth calling out explicitly in release notes since it changes observable CLI behavior for an existing (if buggy) state.
• apps/rocmd and apps/rocm drop their direct windows-sys dependency (moved into rocm-core, which already carries it under cfg(target_os = "windows")); verified nothing else in either crate referenced it.

What this covered

Every file in the diff, read completely at the PR head, across 8 independent fan-out workers grouped by coupling (core atomic_write.rs/config.rs/paths.rs/lib.rs + their property tests; therock.rs + its proptest file; runtimes_cmd.rs; sandbox.rs + the three dependency-manifest files; the e2e-cucumber config suite; docs + coverage-floors.toml; AGENTS.md/CONTRIBUTING.md adherence; git history on the core files), plus my own direct verification of: the coverage-floors.toml change against xtask/src/coverage.rs's TOLERANCE constant and the file's own "do not edit by hand / re-bless" header (the one cross-worker disagreement — resolved as not a violation: TOLERANCE bounds runtime coverage vs. the recorded floor, not how much the floor file itself may change on a re-bless, and the PR's own commit list explicitly documents lowering it), the sibling rocm-dash-core::Config::load_default() (same load-defaults-then-save bug pattern but a different file/crate, no write-back path found, and explicitly listed in the PR's own "Not in this PR" section — correctly out of scope), GitHub issue #515 and Jira ROCMAI-532 (title/summary/status only), and the live PR/CI state (re-checked immediately before posting: head 92b1f7ed, OPEN, non-draft).
d59f596f6e08297c207f008021eecb37defc3558...92b1f7edc046daac22d8a0edde635de197198564.

Did not run: "prior changes" (review comments on earlier PRs touching these files) — not retrievable without reading GitHub PR discussion, which this review is barred from; substituted a best-effort local git log subject-line scan (found nothing beyond the already-cited, directly-relevant recent commits on config.rs/paths.rs). Did not independently execute the test suite or the failing CI job's logs — the job logs were not fetchable from this environment (proxy-blocked); relied on the CI status as reported by gh, per the method's instruction to take CI at face value. Not reconciled against any prior review or discussion thread on this PR — this is a fresh, first review.

[jussielo-amd]

Review — needs work

Full review of the current head (92b1f7ed) against merge-base fcc646e9. The branch was rebased since my earlier review at 2a40e182, so I re-reviewed the whole change rather than only what moved. Closes #515 as described.

Blocking

1. The managed-Python error advises "move it aside to start from default settings", which silently relocates a custom install's data dir. — apps/rocm/src/therock.rs:5628
   This error is reached when AppPaths::discover succeeded and RocmCliConfig::load then failed on some other field (e.g. a retyped chat_top_p). paths.rs says discovery still returns the recorded root in that case. Following the advice makes config.json missing, so discovery silently falls back to the default data dir and a --prefix install's runtimes vanish, which is the harm this PR exists to prevent. paths.rs:395, README.md:950 and the PR's own "blast radius B" section all say that moving the file aside needs ROCM_CLI_DATA_DIR; this message does not. No test follows this advice to its result (AGENTS.md §3 asks for message and state to be asserted together). paths.rs and the README govern, since they describe the real effect.
   Fix: drop the "move it aside" clause or reuse the discovery error's wording, and add a test asserting the message together with the data dir after the advice is followed.

2. The description and a doc comment misstate what save did before this PR. — crates/rocm-core/src/config.rs:557-562, PR body ("save used fs::write (truncate, then write)", "Atomic, durable, faithful saves")
   At the merge-base save already called crate::write_atomic (temp file, then rename): base config.rs:559, lib.rs:502, and the base doc comment even says "destination is never removed first". A full disk mid-write therefore failed on the temp file and left the old config.json alone. What this PR actually adds is fsync, mode preservation, symlink handling, the parse-back check and the shared writer. The doc comment now states false history, and several added tests (saving_the_config_replaces_it_without_leaving_a_temp_file_behind, a_config_save_that_cannot_publish_cleans_up_its_temp_file, the inode tests) pass on the base, so "every fix was shown red before" does not hold for this part.
   Fix: narrow the description to what is new, and state only guarantees in the comment.

3. Two docs disagree with the code after this change.

   • docs/architecture.md:94 still says write_atomic is shared by config.rs, automation.rs and service_record.rs; line 77 of the same file and config.rs:587 say RocmCliConfig::save no longer uses it. The code governs.
   • docs/testing.md:157 says each property prints a reach: line; unknown_keys_survive_a_load_save_cycle, windows_path_normalization_is_idempotent and startup_update_check_record_round_trips print none. The code governs.
     AGENTS.md §5 requires claims repeated across docs to stay in sync. Fix: drop config.rs from line 94, and reword the sentence or add the counters.

4. The new Windows partial-replace recovery has no test. — crates/rocm-core/src/atomic_write.rs:335-396
   The classification of ERROR_UNABLE_TO_MOVE_REPLACEMENT/_2 and the retry rename (fs::rename(replacement, path), :390) run in no test on any platform. The "keeps the new contents" message is tested by injecting a publish closure, which never reaches this code, and the description itself says it is "type-checked, not run".
   Fix: pull the "is this a partial replace" check into a pure function and unit-test it; inject a rename failure to cover the retry.

5. The last Then of config-09 claims more than it checks. — tests/e2e-cucumber/tests/e2e/config_steps.rs:475
   "the CLI keeps its data in its default folder" is assert_ne!(data_dir, custom_rocm_folder), so any wrong data dir passes. "On default settings" (active runtime key: <unset>) is already true while the file is unreadable, so it cannot show that moving the file aside reset anything. The description says "the data dir is the default folder again", and the scenario title says it stops the CLI "hiding the user's runtimes", but no runtime is ever planted.
   Fix: compare against <home>/.rocm and plant a runtime in the custom folder.

6. E2E tests (Strix Halo, Windows, current) is red on this head (run 39041532923). Four scenarios fail: the PowerShell SetEnvironmentVariable/setx PATH step, two "resolved model" steps, and runtime-02 "managed runtime folder path is not recursively nested". The same four failed on main's own runs 38911370206 and 38909090099 on 2026-10-16, and recent main runs are green; the new config-09 passed on Windows. This looks inherited from an old base rather than caused by this change. The branch is 66 commits behind main, so please rebase and re-run.

Non-blocking

• Staged file is written at default permissions and chmod'ed afterwards — atomic_write.rs:180-212. A 0600 config.json holding the dashboard token is briefly world-readable in its temp file, and a crash in the gap leaves a config.json.tmp-* that nothing sweeps. Still better than the base, which published at the umask default. Fix: create the temp file with the existing mode.
• After a Windows partial replace there is no config.json, and load reads that as "no settings" — config.rs:567-576. The save doc admits it; the next save (e.g. the managed-Python record) would write defaults over settings that survive only in config.json.tmp-*, which is this PR's own hazard in a rare path. The README sentence "an interrupted save leaves the previous file" (README.md:938) omits it. Fix: have load treat a missing config.json with a leftover config.json.tmp-* beside it as unreadable, or add the caveat to the README.
• rocm dash panics on tick values that load and save accept — apps/rocm/src/dash.rs:54-56 (Duration::from_secs_f64 on a negative or overflowing *_tick_secs). Pre-existing, but it sits in the same validity question the PR settles for migration; the gpu_tick() accessors in config.rs already guard it.

Decisions for the author

• A damaged config.json now stops almost everything unless ROCM_CLI_DATA_DIR is set, including all of rocmd (tradeoff). One side: silent relocation of the data dir and writes to the wrong place. The other: a lockout of examine, uninstall and remote, with only diagnose exempt. The description documents this; please confirm a lenient discovery with a warning for read-only commands was considered.
• Lenient loaders still run on defaults without saying so (tradeoff), e.g. apps/rocm/src/main.rs:7799 and apps/rocmd/src/mcp.rs:597: gpu_snapshot with an unreadable file runs with telemetry at its default even if the user set it off. They never save, so the file is safe, but nothing prints a notice.
• A refused legacy-TOML migration is never retried (tradeoff): any later save creates config.json, which closes the migration window for good. The refusal warns about this, but the command that prints it usually performs that save.
• Property reach counters are printed, never asserted (non-blocking improvement). docs/testing.md concedes a property with reach zero "passed without testing anything"; an assert!(reach > 0) after each run would make CI see it.
• unknown_keys_survive_a_load_save_cycle is #[ignore]d as a "by-design gap", while docs/testing.md says ignored reproducers are removed when fixed (non-blocking improvement). Track it as an issue or make it a real assertion; nothing runs --ignored in CI.

Positive signals

• app_paths_refuse_to_guess_the_data_dir_from_an_unreadable_config (paths.rs) asserts the advice and the resulting state together and drives each way out as printed.
• The inode/hard-link witness test is a proper mutation check against an in-place rewrite.
• Using a directory at config.json to produce a non-NotFound read error avoids chmod and root, so it works on every platform.
• The "blast radius" section of the description lists what now refuses and why.
• recorded_root_in_damaged_config never guesses past a truncated string.

Deployment notes

• With config.json unreadable and ROCM_CLI_DATA_DIR unset, nearly every rocm command and all of rocmd now stop with an error; a running daemon is unaffected until it restarts.
• Every config.json and rocmd write now fsyncs.
• Cargo.lock loses windows-sys from rocm and rocmd; rocm-core enables Win32_Storage_FileSystem.
• rocmd's coverage floor is lowered by hand in coverage-floors.toml.
• The PR is 66 commits behind main; it is mergeable, but the Windows lane has not re-run against current main.

What this covered

Every changed file at 92b1f7ed (all 30 commits, 21 files), read in full, plus callers and consumers of the changed symbols, AGENTS.md, the README and docs diffs, issue #515, and the failing Windows E2E job log compared with main's runs on the same dates. I did not run a local build or tests; I relied on CI for compile and unit results (clippy, build-and-test, windows-build-and-test and the Linux E2E lanes pass). Not read in full: runtimes_cmd.rs test lines 1810-4296 and therock.rs tests outside the changed hunks. This review was not reconciled against the existing discussion on the PR.