[jussielo-amd]

Why

The phase-7 split of crates/rocm-core/src/lib.rs (ROCMAI-27) left a set of helpers that no phase claimed. docs/architecture.md listed them as unassigned, but never said where each should end up. Phase 8 (ROCMAI-179) audits the result of the split, and without a recorded decision a later cleanup is likely to group these helpers by where they sit in the file rather than by what they do. This PR, for ROCMAI-662, records a home for each one. It also does the moves that are pure relocations today.

What changes

Moved now. Every item is re-exported from the crate root, so every existing rocm_core::X path in the apps, engines and rocm-engine-protocol still compiles unchanged:

• service_identity.rs: ServiceId and generate_service_id, plus their two unit tests.
• endpoint_key.rs: the endpoint-API-key generator and header-safety check, plus their two unit tests. Split out of service_identity.rs after review: an id is readable and timestamp-based, the key is CSPRNG-drawn, and the two meet only at the filesystem layer — the key's sidecar is named after the service id, never derived from it. That one coupling point wasn't a reason to share a module.
• codex_bridge.rs: the CodexBridgeSnapshot/CodexBridgeGpuSnapshot/CodexBridgeEngine DTOs. rocm and rocmd each build their own snapshot of this shape.

The moved code is unchanged except for one test assertion message, which no longer prints the generated endpoint key. Code scanning flags a credential in a failure message. The same finding was dismissed as a false positive while the test was in lib.rs, but the message does not need the key to report the failure.

Kept at the crate root, recorded as permanent: unix_time_millis, require_nonempty, command_failure_detail, sanitize_component, TORCH_ALIGNMENT_DISABLED_ENV/torch_alignment_disabled, and VLLM_GPU_MEMORY_UTILIZATION_HINT. Each is used by more than one module or crate, and none fits a domain an existing module owns.

Decided, with the move deferred to a follow-up:

• resolve_amd_smi_binary* will get its own amd_smi.rs (ROCMAI-723). Open fix(serve): run the KFD pre-flight at every amd-smi launch site (EAI-8457) #447 adds roughly 300 lines of amd-smi invocation code to the same stretch of lib.rs. Extracting first would make that PR move its additions mid-review, so the extraction happens after fix(serve): run the KFD pre-flight at every amd-smi launch site (EAI-8457) #447 and takes its additions with it.
• write_atomic is not permanent (ROCMAI-724). Open refactor(core): share one atomic writer between rocm and rocmd (ROCMAI-681) #610 adds a stronger shared atomic writer to this crate, and open fix(config): never replace an unreadable config.json with defaults (ROCMAI-532) #541 moves RocmCliConfig::save onto it. The remaining callers in automation.rs and service_record.rs then follow, and write_atomic is deleted. That changes behavior (on Windows the replace goes through ReplaceFileW, not rename), so it gets its own review rather than riding along with a move.

The ticket's starting proposal was to move sanitize_component into model_catalog.rs as its only consumer. In fact generate_service_id uses it too, to slug the engine name and model reference. So it is a slugger shared by two unrelated modules, not part of the catalog, and it stays at the root.

docs/architecture.md lists the three new modules in the rocm-core module map. The old "unassigned" paragraph is replaced by the decision record above.

Verification

• cargo test -p rocm-core --lib: 754 passed, 3 ignored, both before and after the change. The moved tests run under their new module paths. cargo test -p rocm-core generate_endpoint_api_key, cited in docs/testing.md, still selects its test.
• cargo test --workspace --all-targets --no-fail-fast: 4323 passed, 0 failed, 17 ignored, across 35 test binaries.
• cargo clippy --workspace --all-targets -- -D warnings and cargo fmt --all --check both pass.
• cargo xtask check-crate-edges and cargo xtask coverage both pass; the endpoint_key.rs split keeps every moved test inside rocm-core, so the crate's coverage floor is unaffected.
• A script compared the lines removed from lib.rs against the two originally-moved files, and separately against the service_identity.rs/endpoint_key.rs split. Every removed non-blank line appears verbatim, apart from the one assertion-message exception above.

Not run: Windows and the e2e-cucumber feature suite. This is a pure move inside one crate; neither touches CLI-observable behavior.

No Gherkin scenario: nothing a CLI user can observe changes. Output, exit codes, files written and the public rocm_core:: API are all unchanged.

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (Not a bug fix.)
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. (No new subcommand; both new modules are full-domain extractions.)
• Every new or changed user-facing message was read against the code path that runs after it, and its test asserts the resulting state. (No user-facing message changed.)

[Copilot]

🟢 Approval recommended

Static review supports these mechanical relocations preserving behavior, public imports, and tests, with no blocking issues identified; tests were not rerun during this review.

No open issues 🎉

What changed in this PR

This PR advances rocm-core modularization by extracting service identity helpers and bridge snapshot types, while recording permanent and deferred homes for the remaining crate-root helpers.

Changes:

• Moves service identity and endpoint-key helpers, including their four tests, into service_identity.rs.
• Moves bridge snapshot types into codex_bridge.rs, preserving serialization and existing crate-root imports.
• Documents the remaining helpers’ ownership and deferred work, and removes the generated key from one assertion message.

File	Description
docs/​architecture.md	Records new modules, permanent root helpers, and deferred extractions.
crates/​rocm-core/​src/​service_identity.rs	Houses service identifiers, endpoint-key helpers, and their existing tests.
crates/​rocm-core/​src/​lib.rs	Re-exports relocated items to preserve existing import paths.
crates/​rocm-core/​src/​codex_bridge.rs	Houses shared bridge snapshot types without changing their serialized shape.

_{Review effort: Balanced}

[nowycondro]

🔴 Automated review · pr-review-watcher · a746bb2

Review

Full review of the whole change (merge-base 63ecab2d…head a746bb27): 4 files, +288/-224. This implements the recorded-decision and pure-relocation part of ROCMAI-662; the ticket itself could not be retrieved from this review context (no tracker access here), so its full scope is not verified — only that the PR's own description is internally consistent with the diff.

Blocking

• CI check "E2E tests (MI300X, current)" is failing on this commit — gh pr checks 735 reports fail for this job (run 40357085305, job 121957919099) at the current head sha. The job's logs are not accessible from here (self-hosted runner logs returned 403), so the cause could not be narrowed further. Given the change is a pure code relocation inside rocm-core with no touched CLI/runtime paths, this looks environmental/hardware-lane related rather than caused by the diff, but per CI-status policy a failing check blocks regardless of how plausible that is. Also note "E2E tests (W7900, current)" was still queued/pending at review time — its result isn't in yet.
  Confidence 100 · architectural · Fix: re-run or investigate the MI300X lane; confirm it is unrelated to this change before merge.

Non-blocking

None

Decisions for the author

• service_identity.rs bundles two mechanistically-unrelated concerns (path-safe id validation and CSPRNG secret generation) in one module — tradeoff
  The module doc comment explains the two meet because the per-service key sidecar is named after the service id, while stating they are "deliberately unrelated in how they are made." That's a defensible boundary (operational coupling at the filesystem layer) but another reasonable split would keep ServiceId/generate_service_id separate from the endpoint-key generator/validator, since nothing but the sidecar filename actually couples them. Worth confirming this is the intended long-term shape rather than a case of "these were adjacent in lib.rs so they stayed adjacent."

Positive signals

• The claimed "pure move, one assertion-message exception" was spot-checked by diffing the removed lib.rs region against the new service_identity.rs/codex_bridge.rs content directly — it matches verbatim apart from the one documented test-message redaction.
• Removing the generated API key from the test failure message (generate_endpoint_api_key_is_random_and_alphanumeric) is a real fix to a credential-in-logs smell, done carefully rather than just suppressing the scanner finding.
• All re-exports needed for the moved items were verified present at the crate root, and a repo-wide grep found zero remaining references to the old lib.rs-local symbols — the relocation doesn't break any rocm_core::X call site.
• docs/architecture.md's new/edited cross-links (to codex_bridge.rs, service_identity.rs, and the three referenced open PRs #447/#610/#541) all resolve to real files and real open PRs.

Deployment notes

None — internal module reorganization only; no public API, CLI behavior, config, or dependency changes.

What this covered

Read completely: crates/rocm-core/src/lib.rs (diff + full current file), the two new files crates/rocm-core/src/service_identity.rs and crates/rocm-core/src/codex_bridge.rs in full, and the docs/architecture.md diff in full, plus AGENTS.md for agent-instruction adherence and a repo-wide grep for stray references to every moved symbol. Verified via git log that this follows the same established extraction pattern as prior phase-7 PRs on this file. Checked that all non-rocm-core consumers of the moved Codex* DTOs (apps/rocm/src/main.rs, apps/rocmd/src/common.rs, apps/rocmd/src/watchers.rs) still resolve through the crate-root re-export. Did not independently run cargo build/cargo test/clippy — CI's "build-and-test", "Test (affected crates)", and "clippy" jobs already ran and passed on this commit, so that was taken as sufficient rather than re-derived. Single-context pass (change is small/well-coupled across 4 files; fan-out was not needed for coverage). Not reconciled against any prior review discussion on this PR. Ticket ROCMAI-662 could not be retrieved (no tracker access in this context); the PR description's account of itself matches the diff.

[jussielo-amd]

Re: the service_identity.rs bundling question (review ID 5663860636) — split it. Pushed 8739e5d1: generate_endpoint_api_key/endpoint_api_key_has_forbidden_chars now live in a new endpoint_key.rs, leaving service_identity.rs with just ServiceId/generate_service_id. You were right that the only thing coupling them was adjacency in the old lib.rs, not the mechanism — an id is readable/timestamp-based, the key is CSPRNG-drawn, and they meet only at the filesystem layer (the key's sidecar is named after the id). All four external consumers go through the crate-root re-export (never rocm_core::service_identity::* directly), so the split needed no caller changes — just the lib.rs module/re-export update and a docs/architecture.md entry for the new module. Full gate re-run clean: build, clippy, affected tests (including the moved ones), crate-edges, and coverage.

[juhovainio]

Approving. This is a mechanical relocation (ServiceId/generate_service_id, endpoint-key helpers, Codex bridge DTOs) into three new modules, with root re-exports keeping every rocm_core::X path intact; the only deliberate change is the key test's assert message no longer printing the generated key.

Non-blocking:

1. sanitize_component stays at the crate root since generate_service_id also uses it, so the ticket's 'sole consumer' claim is stale and worth a note on ROCMAI-662.
2. generate_service_id has no doc comment, unlike its neighbours.
3. The amd_smi move and write_atomic removal are deferred to ROCMAI-723 and ROCMAI-724 behind open #447, #610 and #541, which the doc explains.

[jussielo-amd]

Thanks for the review. Re: the three non-blocking notes (review ID 5664499778):

1. sanitize_component's stale "sole consumer" claim — already recorded, not new: the ROCMAI-662 comment logged when this PR went up states it explicitly ("Its sole consumer is not model_catalog.rs: generate_service_id also uses it"). The PR description carries the same correction. No further action needed there.
2. generate_service_id had no doc comment — fixed in f4520c80: documents the id format, why it always passes ServiceId::new, and the id-uniqueness property callers rely on (don't key duplicate-launch detection on service_id, as main.rs already notes elsewhere).
3. amd_smi/write_atomic deferral — confirmed correct as described, no change needed.

Build/clippy/fmt/affected tests all clean on the new head.

[nowycondro]

🔴 Automated review · pr-review-watcher · 8739e5d

Review — no blocking findings

Full review of the whole change (merge-base 63ecab2d…head 8739e5d1): 5 files, +306/-224. This implements the decided-homes part of ROCMAI-662 for rocm-core's residual lib.rs helpers, and performs the moves recorded as pure relocations; the amd-smi and write_atomic moves are explicitly deferred to follow-up tickets, consistent with the ticket's own framing of its suggested resolution as non-binding.

Blocking

None

Non-blocking

None

Decisions for the author

• apps/rocm/src/endpoint_keys.rs (plural, pre-existing, different crate) and the new crates/rocm-core/src/endpoint_key.rs (singular) now both exist, dealing with related but distinct concepts — non-blocking-improvement
  This predates the PR and isn't something it introduced, but the near-identical names for two unrelated modules in different crates is a minor readability trap for future readers grepping for "endpoint key(s)". Not worth blocking on; worth a one-word rename if either file is touched again.

Positive signals

• The "pure move, one assertion-message exception" claim was independently re-verified by byte-diffing the removed lib.rs regions against service_identity.rs/endpoint_key.rs/codex_bridge.rs directly — it matches verbatim apart from the documented test-message redaction.
• The mid-PR self-correction that split endpoint_key.rs back out of service_identity.rs (an id is readable/timestamp-based; the key is CSPRNG-drawn; they meet only at the filesystem layer via the sidecar filename) is a real module-boundary improvement, not just a file-count change, and the doc comments on both sides describe the split accurately and consistently.
• The PR text itself catches and corrects a stale claim in the ticket's own starting proposal (that sanitize_component has a single consumer) rather than relocating it on the ticket's say-so — generate_service_id also depends on it, so it correctly stays at the crate root.
• Removing the generated API key from a test failure message is a real credential-hygiene fix, done by changing the assertion's message rather than by suppressing the scanner finding.
• All crate-root re-exports needed for every relocated item are present, and a repo-wide grep across apps/, engines/, and crates/rocm-engine-protocol found zero call sites that would break — every consumer reaches the moved symbols through the unchanged rocm_core::X path.

Deployment notes

None — internal module reorganization only; no public API, CLI behavior, configuration, or dependency changes.

What this covered

Read completely: crates/rocm-core/src/lib.rs (diff and full file), the three new files service_identity.rs, endpoint_key.rs, codex_bridge.rs in full, and the docs/architecture.md diff in full, plus AGENTS.md for agent-instruction adherence. Work was fanned out to two independent background workers: (1) a code-correctness pass that re-derived the pure-move claim by byte-diffing old vs. new locations, checked re-export completeness, import correctness, the rand/Alphanumeric/control-character claims, and the sidecar-naming mechanism referenced by the new doc comments; (2) a combined pass covering agent-instruction adherence, this file's prior "phase 7" extraction history, patterns from earlier extractions' review feedback, and code-comment consistency. Both reported no findings at or above the confidence bar. CI: all checks are green on this exact head commit (gh pr checks 735, 24/24 passing, no pending lanes). Ticket ROCMAI-662 was retrieved and checked against the PR description: the description's account of itself matches the diff, and every place it deviates from the ticket's own (explicitly non-binding) starting proposal is explained in the PR text. Did not independently re-run cargo build/test/clippy/coverage — CI already ran and passed build-and-test, Test (affected crates), clippy, and the ratcheted coverage check on this commit. Not reconciled against any existing review discussion on this PR.

[nowycondro]

🔴 Automated review · pr-review-watcher · f4520c8

Review — no blocking findings

Full review of the whole change (merge-base 63ecab2d…head f4520c80): 5 files, 4 commits, +~316/-224. Implements the decision-recording and pure-relocation part of ROCMAI-662 (ticket retrieved directly; status: Done). The ticket's own "suggested resolution" is explicitly framed as "a starting proposal, not a mandate," and this PR deviates from it in three places — keeping write_atomic non-permanent instead of permanent, splitting service_identity.rs/endpoint_key.rs into two modules instead of the ticket's single suggested module, and deferring the amd-smi extraction — each deviation is explained in the PR body and verified consistent with the current code and the open PRs it cites (#447, #610, #541).

Blocking

None

Non-blocking

None

Decisions for the author

• endpoint_key.rs (new, singular, crates/rocm-core/src/) and endpoint_keys.rs (pre-existing, plural, apps/rocm/src/) now sit one letter apart while covering related-but-distinct concerns — non-blocking-improvement
  Confirmed both files exist: the new endpoint_key.rs generates/validates the key value (CSPRNG draw + header-injection check), while the pre-existing endpoint_keys.rs persists that value to a 0600 sidecar file keyed by service id. The pairing is logical (one produces what the other stores) but the near-identical names across two different crates is a minor trap for anyone grepping for "endpoint key" later. Not worth blocking on; worth a one-word disambiguation if either file is touched again.
• Deferring the amd-smi and write_atomic extractions behind other open, unlanded PRs (#447, #610, #541) — tradeoff
  This sequencing avoids making those PRs relocate their in-flight additions mid-review, which is a real cost worth avoiding. The tradeoff is that "decided homes" for those two groups stays incomplete until PRs this PR doesn't control land — if any of #447/#610/#541 stalls or changes shape, the deferred moves could drift from what's documented here. Worth revisiting if those PRs are still open by the time Phase 8 (ROCMAI-179) starts, since that's the deadline ROCMAI-662 itself was raised against.
• Splitting ServiceId/generate_service_id and the endpoint-key generator into two modules instead of the ticket's single suggested "service-identity module" — tradeoff
  The module doc comments on both sides give a specific, consistent rationale (an id is readable/timestamp-based; the key is CSPRNG-drawn; they meet only at the filesystem layer via the sidecar name) and that boundary holds — independently confirmed there are no use cross-imports between the two files, only rustdoc intra-links. It is nonetheless a deviation from the ticket's own "starting proposal," for two very small files. Worth confirming this is the intended long-term shape rather than a case of "neater in theory" for files this size.

Positive signals

• The "pure move, one assertion-message exception" claim holds: independently byte-diffed the removed lib.rs regions against service_identity.rs/endpoint_key.rs/codex_bridge.rs — logically identical apart from the documented test-message redaction (the generated key no longer appears in the failing-assertion message, a real credential-in-logs fix).
• All re-exports needed for every relocated item are present and correct, and a repo-wide grep across apps/, engines/, and crates/ found zero call sites using a path that would break — every consumer reaches the moved symbols through the unchanged rocm_core::X crate-root path.
• The PR's own text catches and corrects a stale claim in the ticket's starting proposal (that sanitize_component has a single consumer in model_catalog.rs) — generate_service_id also depends on it, independently confirmed, so it correctly stays at the crate root rather than being moved on the ticket's say-so.
• endpoint_api_key_has_forbidden_chars's header-injection defense and generate_endpoint_api_key's entropy claim are not just internally consistent but provably so: since the generator draws only from [A-Za-z0-9] and no alphanumeric ASCII character is a control character, a freshly generated key can never fail its own forbidden-chars check — the test asserting this is a logical guarantee, not an empirical coincidence.
• docs/architecture.md's new prose was checked claim-by-claim against real call sites (every named consumer of sanitize_component, command_failure_detail, TORCH_ALIGNMENT_DISABLED_ENV, VLLM_GPU_MEMORY_UTILIZATION_HINT, require_nonempty, and the "rocmd's automation watchers use only the GPU part" claim) and matches the actual code in every instance.
• The split follows the repo's established phase-7 extraction precedent exactly: copyright/SPDX header, //! module-doc-first convention, alphabetical placement of both the new pub mod and pub use entries in lib.rs, and tests moved intact into #[cfg(test)] mod tests at the bottom of their new file.

Deployment notes

None — internal module reorganization only; no public API, CLI-observable behavior, configuration, or dependency changes. Every existing rocm_core::X import path is preserved via crate-root re-exports.

What this covered

Read completely: the lib.rs diff and surrounding retained code, all three new files (service_identity.rs, endpoint_key.rs, codex_bridge.rs) in full, and the docs/architecture.md diff in full. Fanned out to two independent background workers: (1) a code-correctness pass that byte-diffed old vs. new locations, verified re-export completeness and import correctness via repo-wide grep, and re-derived the security/entropy reasoning; (2) four source-reading passes — agent-instruction adherence against the repo's root AGENTS.md (no directory-scoped override exists for crates/rocm-core/ or docs/), history against the four prior phase-7 extraction commits this PR's pattern follows, prior-change/established-pattern conformance against four reference modules, and code-comment accuracy (every non-trivial doc-comment claim individually verified against real call sites). Both workers confirmed, on direct follow-up, that their passes were fully complete with no outstanding work. One worker had no Rust toolchain available in its sandbox and verified compilation correctness by manual use/import inspection rather than cargo check; this is a disclosed limitation, not a finding, and is independently covered by CI (see below). CI: gh pr view --json statusCheckRollup on the current head shows 24 checks — 22 success, 2 skipped (Skill checks, Sphinx docs build), zero failures, including build-and-test, Test (affected crates), clippy, windows-build-and-test, and the ratcheted coverage check. Ticket ROCMAI-662 was retrieved directly (status: Done) and its "starting proposal, not a mandate" framing checked against every place this PR's approach diverges from it — each divergence is explained in the PR body and holds up. Not reconciled against any existing review discussion on this PR.