[jussielo-amd]

Summary

Four files in crates/rocm-core each held several unrelated jobs, and two pairs of modules imported each other. automation.rs (3,653 lines) was about 60% a generic JSONL store engine wrapped around three small record types. net.rs (2,385 lines) mixed the loopback HTTP/TCP client with streaming downloads and the managed-service readiness policy. openmpi.rs (2,010 lines) carried distro package-manager plumbing that knows nothing about OpenMPI. fix.rs (5,344 lines) held the fix catalog, the host-mutating runners, the rendering and the JSON contract. Anyone looking for the torn-tail repair, the download retry loop or the package planner had to read a file that was mostly something else, unrelated edits collided in the same files, and config <-> automation and net <-> service_record were import cycles. This series splits the four files by domain and removes both cycles. It is a pure move: no behaviour change, no renamed symbol, no changed output.

It is twelve commits in one branch, in the order they should land (eight moves, a commit that turns the bare file citations the series edited into relative links, a merge of origin/main, a commit that hardens the exit-code lint, and a second merge of origin/main), and each move compiles and passes clippy for rocm-core (Linux and the x86_64-pc-windows-msvc target) and its tests on its own. Every rocm_core::, rocm_core::openmpi::, crate::fix:: and crate::openmpi:: path that was in use still resolves through a re-export.

Completes ROCMAI-762.

What moved

Commit 1 moves the watcher catalog from automation.rs to config.rs, which removes the config <-> automation cycle (only automation now depends on config, for WatcherMode). Commit 2 splits net.rs: the readiness policy goes to a new endpoint_readiness.rs and the inference state keys go to service_record.rs, which removes the net <-> service_record cycle. Commits 3 to 5 move the JSONL engine, the downloads and the package planning into new modules. Commits 6 to 8 split fix.rs into children under fix/, with fix.rs kept as the module root (as examine.rs is for examine/) so every link to it stays live.

Commit	Symbols	Destination
1	BuiltinWatcherSpec, BUILTIN_WATCHERS, builtin_watchers, builtin_watcher and their four builtin_watchers_include_* tests	config.rs
2	INFERENCE_VERIFIED_STATE_KEY, INFERENCE_PROBE_ATTEMPTED_STATE_KEY, INFERENCE_PROBE_RETRY_INTERVAL	service_record.rs
2	openai_chat_completion_probe, openai_chat_completion_status, openai_models_endpoint_has_model, managed_service_endpoint_model_ready, EndpointReadiness, EndpointReadinessOutcome, managed_service_endpoint_readiness, openai_loaded_model_ids, model_refs_match and its three helpers, INFERENCE_PROBE_TIMEOUT, merge_json_state_file, engine_state_inference_verified, with their tests	new endpoint_readiness.rs
3	jsonl_lock_path, JSONL_SCAN_CHUNK, JsonlRotation, jsonl_generation_path, rotate_jsonl_generations, open_jsonl_for_append, append_jsonl_record, repair_torn_tail_and_append, truncate_jsonl_in_place, rotation_failure_warning, append_or_tolerate_a_missing_newline, landed_despite_the_error, TailState, inspect_tail, read_range, parse_jsonl_records, load_recent_jsonl_records (and _from), open_jsonl_generations, scan_recent_jsonl_records, is_blank, is_torn_record, ReverseLines, the cfg(test) test_hooks, and the engine tests	new jsonl_store.rs
3	audit_record, audit_line, audit_messages, seed_audit_store (fixtures both sides' tests use)	test_support.rs
4	DOWNLOAD_MAX_ATTEMPTS, DOWNLOAD_CHUNK_BYTES, Backoff, DownloadRequest, DownloadOutcome, download_file_streaming(_with_progress), download_file_to_path(_with_progress), partial_download_path, the attempt/retry helpers, and the download tests with their server fixture	new download.rs
5	InstallCommand, SystemPackageInstallPlan, running_as_root, can_autoinstall, sudo_noninteractive_available (both cfg variants), PackageManager, InstallPlanBuilder, plan_for_os_release_at/_read, InstallHint, install_hint_for_at/_read, apt_simulate_argv, parse_apt_simulate_removals, is_protected_rocm_package, resolve_package_manager, and their tests	new package_plan.rs (re-exported from openmpi.rs)
6	the exit module, FixOptions, FixClass	fix/options.rs
6	fail!, relay_output, RUN_TIMEOUT, QUERY_TIMEOUT, current_os, consent_without_prompt, confirm, and their tests	fix/consent.rs
7	home_dir, is_root, shell_rc_file, append_line, run_render_group, run_unset_override*, report_persistent_override, report_and_clear_override_windows, run_path_export*, windows_path_prepend, path_export_windows, run_hip_visible_devices*, pin_device_in_rc_file, ps_env_scope, newest_rocm_install_dir, first_install_path, newest_rocm_install_dir_in, and the runner tests	fix/runners.rs
8	FixRecipe, Platform, PRINT_ON_*/AUTO_ON_*, RECIPES, find_recipe, is_catalog_id, class_on, needs_on, COMGR_CONFLICT_NEITHER_OPTION_RECOMMENDED, the cfg(test) oracles, and the catalog tests	fix/catalog.rs
8	list_recipes, format_flags, print_recipe, verify_lines, verify_steps, indent_continuation, diagnosis_position_refusal, looks_like_a_diagnosis_position, and their tests	fix/render.rs
8	CATALOG_CONTRACT_VERSION, ManifestPlatform, ManifestEntry, CatalogManifest, catalog_manifest, catalog_manifest_json, and their tests	fix/manifest.rs

apply, Plan, plan_of, act_on and the exit-code lint infrastructure stay in fix.rs; net.rs keeps the default local host and port, endpoint formatting and parsing, and the loopback client; automation.rs keeps the record types, the runtime state, the store accessors and the per-store policy (EVENT_LOG_ROTATION, proposal compaction). The new modules are private, and lib.rs re-exports their public names.

Where the work went beyond the move itself, it was only as far as the move required. Visibility is widened only as far as callers need: pub(crate) on JsonlRotation, append_jsonl_record, parse_jsonl_records, load_recent_jsonl_records, jsonl_generation_path, InstallCommand::sudo and install_hint_for_at/_read; pub(super) inside fix/ on the consent helpers, the four runner entry points, and FixRecipe/Platform with their fields, methods and a few helpers that the catalog's siblings read. The two re-exec tests that select themselves by test path (a_failed_rotation_prints_its_warning_once_to_stderr and rotation_under_concurrent_processes_loses_no_records) name the new jsonl_store::tests path, and the one include_str! in the moved catalog tests (RULE_CASES) gets one more ../. The exit-code lint test (fix_runners_return_named_exit_constants_not_integers) now scans every file under fix/ as well as fix.rs, so splitting a runner out cannot take it out of its net; with a bare -> i32 literal planted in fix/options.rs it fails, and without it it passes. The fail! macro is re-exported by path from fix/consent.rs (pub(super) use fail) instead of relying on declaration order; its text is unchanged and each user imports std::io::Write as before.

Path pins and docs that name moved code are updated in the commit that moves it: the .pre-commit-config.yaml cargo-catalog hook now matches fix/ (commit 6; the hook did run on commits 6 to 8), AGENTS.md (the confirm() citation in commit 6, the catalog's location in commit 8), docs/architecture.md (every commit; it lists the new modules and no longer describes the two cycles), docs/testing.md (the jsonl_store::tests and package_plan::tests filters and the links that named fix.rs tests), the same-file comment in crates/rocm-core/Cargo.toml, tests/e2e-cucumber/README.md, and the comments and messages in the e2e features and tests that named fix.rs or automation.rs. The pin in apps/rocm/src/advised_commands.rs that keys on the format_flags wording (rocm fix can run it) now names fix/render.rs; pointing it back at fix.rs fails exclusions_still_match_something and every_advised_command_parses.

Dropped module paths

The new modules are private and their names are re-exported at the crate root only, so these module paths no longer resolve: rocm_core::net:: for the 21 names that moved to download.rs, endpoint_readiness.rs and service_record.rs (the download functions and types, Backoff, DOWNLOAD_MAX_ATTEMPTS, the INFERENCE_* constants, the openai_* probes, managed_service_endpoint_*, EndpointReadiness*, merge_json_state_file, engine_state_inference_verified), rocm_core::automation::{BuiltinWatcherSpec, builtin_watcher, builtin_watchers, jsonl_lock_path}, and the cfg(all(test, windows)) crate::fix::run_typed_in_cmd and crate::fix::run_typed_in_powershell. They are deliberately not re-exported: a re-export from net.rs of endpoint_readiness items would bring back the net <-> service_record cycle this change removes. rocm-core is publish = false, a grep of apps/, engines/, crates/, xtask/, tests/ and docs/ found no user of any of these paths, every crate-root rocm_core:: path is unchanged, and cargo check --workspace --all-targets passes. The ticket's acceptance wording about every existing rocm_core:: path resolving was read as crate-root paths for this reason.

Deviations

The moved JSONL-engine tests (jsonl_store::tests) drive the engine through append_audit_event, load_recent_audit_events and AuditEventRecord, so there is an edge from jsonl_store's test module to automation, contrary to what the ticket text and the message of the JSONL commit say. It exists in test builds only and was kept rather than moved back: the production cycle is gone (automation -> jsonl_store is the only production edge), and docs/architecture.md says the edge is test-only.

Verification

Ran on each of the eight move commits (against the previous base, 44f8bcf; the final tip was re-run after the last rebase), checked out on its own in a clean worktree: cargo fmt --all --check; cargo clippy -p rocm-core --all-targets -- -D warnings on Linux and on x86_64-pc-windows-msvc; the test-name diff, cargo test -p rocm-core --lib -- --list | grep ': test$' | sed 's/: test$//; s/.*:://' | sort against origin/main, with and without --features e2e-test-hooks, which is empty for every commit (1,010 tests default, 1,014 with the hooks on the final base); and cargo test -p rocm-core --no-fail-fast (1,007 passed, 3 ignored on the final tip). cargo check --workspace --all-targets ran on the working tree of commits 2 to 8 as each was built, and again on the final tree. On the final tree also: cargo xtask catalog --check (doctor-catalog.json is byte-identical), cargo xtask manifest --check, cargo xtask check-crate-edges, cargo test -p xtask env_mutation (the whole-tree environment-mutation contract still finds the moved lock), cargo test -p rocm advised_commands (20 passed), cargo test -p e2e-cucumber --test skill_reference and the e2e-cucumber lib tests. A source-level check shows the same 1,008 #[test] functions with the same cfg attributes before and after, so Windows-only tests that the Linux list cannot show are not lost. skills/rocm-doctor is untouched, so the reference.md fixture and the two sentences in SKILL.md stay as they were.

Not run: cargo xtask e2e and the full workspace test gate (CI carries them); scripts/smoke_local.py; any test on Windows. The Windows cross-check is clippy only, with a stub archiver standing in for lib.exe so that ring's build script succeeds on a Linux host; it type-checks and lints every Windows-gated item, including the Windows-only oracles and tests that moved in commit 8, but does not execute them, so the Windows CI job is the first run of those.

The last commit makes the exit-code lint test read the fix/ directory and fail, naming the file, when a child module is not in its scan list (shown red with a scratch fix/zz_scratch.rs), so a new child cannot silently go unscanned.

User-visible behaviour

None. This is an internal reorganisation of rocm-core: no command output, exit code, file path or engine selection changes, so no Gherkin scenario is added. The existing scenarios that exercise the moved code (automations-07 through automations-10 for rotation and torn-tail repair, the diagnose-* and fix-catalog-is-complete scenarios for the catalog and manifest) are untouched and still apply; only comments in them that named a moved file changed.

Notes for reviewers

The branch was first merged with origin/main at 3944380 (that merge resolved test_support.rs by keeping both the four audit fixtures and main's proptest_cases) and again at 6e424e7 (that merge had one conflict, in the module doc of quoting.rs, where main repointed the ComfyUI citation to comfyui/process.rs and this branch repointed the path_export_windows citation to fix/runners.rs; both are kept), and it ends at the final commit listed by git log; the fix.rs test that landed there in the meantime (the_utilization_hint_states_the_bound_the_parser_accepts) now lives with the other catalog tests in fix/catalog.rs. Suggested reading order is the commit order, and because every commit is a move, git diff --color-moved=dimmed-zebra per commit makes the few lines that are not moves stand out. Commit 1 is the smallest and shows the cycle break. Commit 2 is the next cycle break; its one judgement call is that two HTTP read-bound tests (http_read_is_bounded_across_reads_not_just_per_read and http_read_survives_a_signal_arriving_mid_response) move to endpoint_readiness.rs with the readiness tests because they drive read_http_response_bounded through openai_chat_completion_probe, and leaving them in net.rs would make net's tests depend on a module that depends on net. Commit 3 is the largest judgement call: the engine tests that call engine internals, or fixtures that do, moved; the tests that go through the append_*/load_* wrappers stayed in automation.rs, and the four audit-store fixtures both groups use moved to test_support.rs. The moved tests drive the engine through the audit-store records, so jsonl_store's test module (test builds only) imports types from automation; the production edge runs one way, automation -> jsonl_store. Commit 5 keeps the os-release plan and hint tests in openmpi.rs, because they drive the generic functions through the three per-library builders. The rocm-core module is named package_plan rather than system_packages because apps/rocm/src/system_packages.rs now exists. Commits 6 to 8 are the fix.rs split in dependency order: the leaves first, then the runners, then the catalog with the rendering and manifest that read it.

Two things differ from what the plan assumed. The *_windows runners in fix.rs are ordinary functions with no cfg, so they compile on every platform; only the cfg(all(test, windows)) oracles and the cfg(windows) tests are Windows-only, and they all moved in commit 8. And the module paths rocm_core::net::... and rocm_core::automation::... no longer carry the moved names (rocm_core::net::download_file_to_path, rocm_core::net::openai_chat_completion_probe, rocm_core::automation::builtin_watcher and so on), because the new modules are private and the names are re-exported at the crate root only. Nothing in the repository uses those module paths (checked by grep over every crate and the e2e crate), and every crate-root path is unchanged, but it is the one place where the visible path of a public item changes.

service_record.rs still has two doc comments that link to crate::managed_service_endpoint_readiness and crate::INFERENCE_PROBE_TIMEOUT through the crate-root re-exports; those are links in prose, not imports. Risk is low: each commit is a move checked by the unchanged test-name list and the existing tests, and the two cycles are checked by grepping crate::<module> both ways (net and service_record no longer import endpoint_readiness, and config no longer imports automation). The remaining watch item is the Windows CI job, for the reason above.

Other open pull requests that touch the same files, so authors may need to rebase over a moved block: lib.rs's re-export block is touched by #883, #879, #854, #826, #817, #811, #775, #725, #724, #723, #678, #610, #581, #569, #541, #447, #437, #299 and #243, which conflicts with commits 1 to 5 mechanically. config.rs (#811, #724, #723, #631, #610, #541, #299) conflicts with commit 1; automation.rs (#811, #299) with commits 1 and 3; service_record.rs (#883, #811, #725, #724, #723, #447, #299, #243) with commit 2; net.rs (#826, #817, #725, #724, #299) with commits 2 and 4; test_support.rs (#811, #557) with commit 3; the comment edit in process.rs (#854, #800, #447, #437, #243) is one line. #811 (the atomic-writer retirement) and #610 are deliberately not touched here: the engine still calls crate::write_atomic, so whichever lands second rebases. No open pull request touches openmpi.rs or fix.rs; the comment-only edits in diagnose.rs (#394) and quoting.rs (#563, #884), the pin in advised_commands.rs (#678, #563), AGENTS.md (#878, #588, #372), .pre-commit-config.yaml (#370, #369, different hook), the e2e feature and test comments (#826, #818, #518, #517, #241, #292) and docs/architecture.md and docs/testing.md (many) can conflict textually in a few lines.

[Copilot]

🔵 Needs a closer look

Missing extraction hunks and base history prevent verification of move-only parity, requiring review of the complete diff and native Windows CI results.

No open issues 🎉

What changed in this PR

This PR reorganizes rocm-core by domain, separating storage, networking, package planning, and fix responsibilities while removing two production import cycles. It preserves crate-root APIs and intends no CLI behavior changes; unused module-qualified paths are deliberately dropped.

Changes:

• Extract domain-specific implementations and move their existing tests alongside them.
• Adjust module visibility and re-exports to preserve callers.
• Update documentation, source-path checks, and catalog-hook coverage for the new layout.

File	Description
tests/​e2e-cucumber/​tests/​skill_reference.rs	Update source paths in comments and assertion messages.
tests/​e2e-cucumber/​tests/​e2e/​skill_steps.rs	Point contract-version guidance at fix/manifest.rs.
tests/​e2e-cucumber/​tests/​e2e/​diagnose_steps.rs	Update catalog references in comments.
tests/​e2e-cucumber/​src/​powershell_text_rule.rs	Update the documented rule-source location.
tests/​e2e-cucumber/​README.md	Link to the relocated authoritative catalog.
tests/​e2e-cucumber/​fixtures/​powershell_text_rule_cases.txt	Update the catalog path in fixture comments.
tests/​e2e-cucumber/​features/​rocm_doctor_skill.feature	Update runner-test references in comments.
tests/​e2e-cucumber/​features/​diagnose.feature	Update Windows verification-test references in comments.
tests/​e2e-cucumber/​features/​automations.feature	Update JSONL-test references in comments.
docs/​testing.md	Update test filters and links for moved code.
docs/​architecture.md	Describe module responsibilities and dependency directions.
crates/​rocm-core/​src/​test_support.rs	Centralize shared audit-store fixtures.
crates/​rocm-core/​src/​service_record.rs	Own inference state keys and the probe retry interval.
crates/​rocm-core/​src/​quoting.rs	Update runner references in documentation.
crates/​rocm-core/​src/​process.rs	Update the lock-helper reference in documentation.
crates/​rocm-core/​src/​package_plan.rs	Extract generic package planning and its tests.
crates/​rocm-core/​src/​openmpi.rs	Retain library-specific logic and re-export generic planning APIs.
crates/​rocm-core/​src/​lib.rs	Declare extracted modules and preserve crate-root exports.
crates/​rocm-core/​src/​fix/​render.rs	Extract fix rendering, flag wording, and related tests.
crates/​rocm-core/​src/​fix/​options.rs	Extract fix options, classifications, and named exit codes.
crates/​rocm-core/​src/​fix/​manifest.rs	Extract catalog JSON serialization and its tests.
crates/​rocm-core/​src/​fix/​consent.rs	Extract consent, platform selection, and execution helpers.
crates/​rocm-core/​src/​diagnose.rs	Update catalog and runner references in comments.
crates/​rocm-core/​src/​confirm.rs	Update the documented consent-helper location.
crates/​rocm-core/​src/​config.rs	Move the watcher catalog beside its configuration types.
crates/​rocm-core/​Cargo.toml	Update the JSONL dependency comment only.
apps/​rocm/​src/​advised_commands.rs	Retarget the wording exclusion to fix/render.rs.
AGENTS.md	Update consent-helper and catalog documentation links.
.pre-commit-config.yaml	Extend the catalog hook to cover fix/ children.

_{Review effort: Balanced}

[Copilot]

🟢 Approval recommended

No active review issues require resolution before approval.

No open issues 🎉

_{Review effort: Balanced}

[juhovainio]

I went through this as a pure-move refactor and found no behavior changes.

• Compared every added and removed line as a multiset, ignoring whitespace. What differs is visibility widening, use/mod/re-exports, doc and comment paths, and the new lint test. No logic line changed.
• 133 #[test] removed and 133 added, so nothing was dropped.
• Crate-root re-exports in lib.rs are preserved, and the cfg(windows) gates moved intact.
• Path pins are updated: the pre-commit hook, advised_commands.rs, the RULE_CASES include_str! path and the re-exec test paths.
• CI is green and the change matches the ROCMAI-762 scope.

Non-blocking notes:

1. The jsonl_store tests import from automation. That is a test-only edge against the ticket wording, but the PR discloses it.
2. The exit-code lint is hardened, which is slightly beyond a pure move.
3. I only read the diff, so a repo-wide grep for stale "fix.rs / automation.rs / net.rs" claims in docs is worth running before merge.

This reflects a diff read plus CI status, not a runtime check.