[jussielo-amd]

Why

A closing audit of the modularization work found three pieces of dead or duplicated code in the rocm and rocmd binaries that the move-only changes could not carry. main.rs and bridge_snapshot.rs each held an identical const fn table of the built-in engine ids and summaries. rocmd kept a helper that was compiled only under cfg(test) and called only by its own test, plus a call whose result was discarded. And missing_packaged_engine_reason returned None for every engine, yet three call sites still branched on its answer. This is part of the follow-up work from that audit. Completes ROCMAI-848.

What changes

Three commits, one per item, so each can be reviewed or dropped on its own. After them come a merge of origin/main and a wording-only commit that answers the review nits.

The first keeps one engine table. builtin_codex_bridge_engine_inventory in bridge_snapshot.rs now reads main.rs's engine_inventory(), the table that rocm examine and rocm update already use, and builtin_engine_inventory is deleted. The two tables were identical at this PR's base commit (1b06c9e): the body of builtin_engine_inventory in bridge_snapshot.rs and the body of engine_inventory() in main.rs hash to the same value when extracted with git show and compared (three (id, summary) tuples for lemonade, vllm and aim, byte for byte), so the consolidation cannot change what the bridge snapshot reports. The main.rs test that checks the serve --engine possible values against the inventory now compares against the surviving table, so it covers every reader instead of only the bridge snapshot's copy. The bridge_snapshot.rs module doc and the docs/architecture.md entry no longer say the two tables are kept separate. The separate rocmd_engine_inventory in rocmd is untouched: rocmd cannot depend on rocm, so that table is a different case and not part of this item.

The second deletes common::find_engine_plugin_binary from rocmd, which existed only under cfg(test) and was used only by its own test, together with that test and the three test-only imports it needed. It also removes the let _ = daemon_binary_path(); line in supervise_service. That function only reads the current executable path and checks sibling files, with no writes or other side effects, so the line did nothing, and its import goes with it. The rocm crate's own private find_engine_plugin_binary in engine_ipc.rs is a different function and is untouched.

The third deletes missing_packaged_engine_reason. It has returned None for every engine since serving was limited to Lemonade and vLLM, so the three branches that acted on its answer never ran: resolve_engine_binary_path_with_paths never bailed on it, and the two sites in the examine report never printed an unavailable reason= row or a note: line. They fall through to the arms that already ran today, so the engine resolution and the rendered output are unchanged. The function, its test and the dead branches are removed, and the module docs of engine_ipc.rs and examine_report_text.rs, the docs/architecture.md entries and the focused test list in docs/testing.md no longer name it. Giving it a real rule instead was considered and rejected: no engine is deferred, so a rule would only add user-visible text with no use, and one can be introduced when an engine is actually deferred.

The branch has since been brought up to date with origin/main by signed merge commits (no rebase, so the history of the three commits and the earlier review stay intact), and one further commit, docs: fix the wording nits from review, fixes two review nits: "help read too" is now "help reads too" in the bridge_snapshot.rs module doc, and the bridge_snapshot.rs entry in docs/architecture.md now says the module's engine list "is derived from main.rs's engine_inventory" instead of a clause with no subject. The latest merge of origin/main (which brought in the change that moved the engine runtime and env-root helpers into engine_install_env.rs) had one conflict, in docs/architecture.md: main added the engine_install_env.rs entry directly above the engine_ipc.rs entry this PR rewords. It was resolved by keeping both, main's new entry and this PR's engine_ipc.rs entry without the missing_packaged_engine_reason clause; no code conflicted, and none of the code main's change touched calls the symbols this PR removes.

Verification

Run locally on the merged tip (the latest merge of origin/main; the test count is unchanged at 1639 after that merge, and the coverage figures below were measured before it):

• cargo fmt --all --check: clean.
• cargo clippy -p rocm -p rocmd --all-targets -- -D warnings: clean.
• cargo test -p rocm -p rocmd --no-fail-fast: 1639 passed, 0 failed, 9 ignored across the nine test binaries.
• cargo test -p rocm advised_commands: 20 passed, 1 ignored.
• The examine and engine tests that cover the removed branches: all 48 tests matching examine_report_text, engine_ipc, engine_inventory and serve_engine pass, including examine_engine_inventory_*, render_engine_inventory_text_*, engine_plugin_discovery_* and serve_engine_help_lists_match_engine_inventory. None of them was edited by this PR except the help-sync test, which now reads the surviving table.
• cargo xtask manifest --check and cargo xtask check-crate-edges: both pass.
• A list of every test leaf name in rocm and rocmd on origin/main against this tip differs by exactly the two tests this PR deletes (missing_packaged_engine_reason_has_no_deferred_first_party_engines, and engine_plugin_discovery_finds_runtime_binary in rocmd; engine_ipc::tests::engine_plugin_discovery_finds_runtime_binary in rocm is a different test and is still there). Nothing else was added or lost.
• Coverage floors, measured per crate with cargo llvm-cov --no-cfg-coverage --locked -p rocm -p rocmd after cargo llvm-cov clean --workspace, on origin/main and on this tip: rocm 83.67% (52018 of 62169 lines) on main and 83.68% (51994 of 62131) here; rocmd 83.20% (7715 of 9273) on main and 83.15% (7687 of 9245) here. Both movements are well inside the 0.2 point tolerance, and both crates sit far above their committed floors (77.13 for rocm, 75.36 for rocmd), so no floor is breached and nothing was re-blessed. These come from a two-package run rather than the workspace-wide cargo xtask coverage, so the absolute figures are not the ones that command prints; the comparison between main and this tip is like for like. The workspace-wide cargo xtask coverage is left to CI.
• The repository's pre-commit hooks passed on each commit (fmt, license headers, markdown links, commit signing and sign-off). A grep for the removed names across apps/, crates/, docs/, skills/ and the top-level markdown finds nothing left except the unrelated private find_engine_plugin_binary in engine_ipc.rs. A leak scan of the diff against origin/main and a conflict-marker grep are clean.

Each surviving examine path is the arm that already ran, so the argument that the examine output is unchanged rests on reading the three call sites together with the unchanged examine tests passing, not on a before and after run of the binary.

Not run

cargo xtask coverage over the whole workspace, scripts/smoke_local.py, the tests/e2e-cucumber suite and the Windows lane were not run locally; CI carries them and starts fresh with the push of the merge commit.

Overlaps and scope

Open PRs #826 and #857 touch main.rs and #854 touches engine_ipc.rs (#836, #838 and #872 have merged and are already part of the merge of origin/main here), so a conflict is likely for whichever lands second; the edits here are small and local, and the docs/architecture.md lines they touch are different from those. No user-visible behavior changes, so no new scenario is needed: the branches removed from the examine report were unreachable, and the existing examine tests, which render both the engine inventory and the per-engine adapter rows, are unchanged by this PR and are the ones that would show a difference if the output moved.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

2 open issues

• Grammar: “help read too” should be “help reads too”. · New
• The sentence is grammatically incorrect: “... (builtin_codex_bridge_engine_inventory) and derives… · New

What changed in this PR

Removes dead/duplicated engine-inventory and “missing packaged engine” logic from rocm/rocmd, and updates docs/tests to align with the single remaining inventory source.

Changes:

• Consolidated rocm built-in engine inventory to use main.rs::engine_inventory() and removed the duplicate table from bridge_snapshot.rs.
• Deleted dead rocmd test-only plugin-discovery helper/test and removed a no-op daemon_binary_path() call.
• Removed missing_packaged_engine_reason and its unreachable branches, updating related docs.

File	Description
docs/​testing.md	Removes the deleted test from the “focused test list”.
docs/​architecture.md	Updates module inventory documentation to reflect the removed helpers/tables.
apps/​rocmd/​src/​supervise.rs	Removes unused import and a no-op call.
apps/​rocmd/​src/​common.rs	Deletes test-only helper and its test that only exercised it.
apps/​rocm/​src/​main.rs	Removes missing_packaged_engine_reason and updates the --engine help sync test to use engine_inventory().
apps/​rocm/​src/​examine_report_text.rs	Removes unreachable output branches and drops the removed symbol import.
apps/​rocm/​src/​engine_ipc.rs	Removes unreachable bail-out branch and drops the removed symbol import.
apps/​rocm/​src/​bridge_snapshot.rs	Switches to engine_inventory() and removes the duplicate inventory table.

_{Review effort: Lite}

[Copilot]

🟢 Approval recommended

No active review issues remain for the engine-inventory consolidation and dead-code cleanup.

2 open issues

• The sentence is grammatically incorrect: “... (builtin_codex_bridge_engine_inventory) and derives…
• Grammar: “help read too” should be “help reads too”.

_{Review effort: Balanced}

[nowycondro]

🤖 Automated review (Claude Code) · 2bab02e
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 — no blocking findings

Full review of the whole change (3 commits, 8 files, +32/-133).
Implements ROCMAI-848 (Phase 8.1-H step 2 of 7, items 8/9/10) in full — the ticket's scope (consolidate the duplicate engine-inventory table, delete the dead rocmd test-only helper and no-op call, delete the always-None missing_packaged_engine_reason) matches the three commits exactly, and the PR's "Completes ROCMAI-848" claim holds.

Blocking

None

Non-blocking

None

Decisions for the author

None

Positive signals

• The two previously-duplicated engine-inventory tables were verified byte-for-byte identical (bridge_snapshot.rs's deleted builtin_engine_inventory() vs. main.rs's surviving engine_inventory() — same three (id, summary) tuples), so the consolidation is a genuine no-op, not just an assumed one.
• The serve --engine help-sync test was repointed at the surviving engine_inventory() rather than dropped, so it now covers every reader of the table instead of only the deleted copy — a widening of coverage through the refactor, not a narrowing.
• Deleting the rocmd-side find_engine_plugin_binary test helper does not drop real coverage: that helper and its test were the only reference to discover_engine_plugins/platform_engine_plugin_binary_name anywhere in the rocmd crate, so no production path in rocmd loses test coverage (plugin-binary resolution for the rocm binary is a separate, untouched function in engine_ipc.rs).

Deployment notes

• No CI check-runs exist yet for this head commit (get_check_runs returns 0; the only status is a passing Read-the-Docs preview build) — the required Rust gates (build-and-test, clippy, coverage, e2e-cucumber) have not reported one way or the other as of this review. Don't read this report as a substitute for green CI.
• coverage-floors.toml was not touched even though two tests were deleted alongside the dead code they covered (missing_packaged_engine_reason_has_no_deferred_first_party_engines, engine_plugin_discovery_finds_runtime_binary). The ticket's own rule only requires a re-bless commit message if one turns out to be needed, and the deleted branches were unreachable (hence uncovered) lines, so the net effect on the floor is plausibly a wash or slight improvement — but this wasn't run here, so let cargo xtask coverage be the actual gate before merging rather than assuming it clears.

What this covered

Every file in the diff read in full (apps/rocm/src/{bridge_snapshot,engine_ipc,examine_report_text,main}.rs, apps/rocmd/src/{common,supervise}.rs, docs/architecture.md, docs/testing.md), via three independent fan-out passes: (1) the rocm-crate engine-inventory/dead-branch cluster, (2) the rocmd/docs cluster, (3) repo-wide impact analysis (grep for the three removed symbol names across the whole tree, tests/e2e-cucumber/ for any dependency on the removed output strings, git history for the removed symbols, and CI/check status) plus an agent-instruction (AGENTS.md) adherence check. All three passes, plus my own direct verification of the inventory-table equality and the rocmd coverage question, came back clean. Reviewed against base a537f1a9281264a7f4cdd85e9fe757c529e76ec5 → head 2bab02e7b6519fe47cc99ad3398285c3acddfa6b.

Did not run: cargo test/cargo clippy/cargo xtask coverage locally (no Rust toolchain in this environment) — correctness of the removed-branch claims was established by reading the call sites and control flow directly rather than by executing them. History and prior-review passes found nothing: the removed functions have a clean, well-documented lineage (missing_packaged_engine_reason traced to "Limit serving engines to Lemonade and vLLM (#79)", matching the PR's stated rationale) with no pending-reuse markers in any of the ~30 other open PRs checked. Not reconciled against the two existing Copilot review comments on this PR (both are grammar nitpicks on a doc comment, already surfaced there).

🤖 Generated with Claude Code

[juhovainio]

The change reads correctly to me: the removed branches in engine_ipc.rs and examine_report_text.rs were unreachable because the stub always returned None, so examine output should be unchanged, and the diff stays within ROCMAI-848's items 8-10 and its doc list.

I'm requesting changes for one reason: there are no CI results on this head commit. Only the Read the Docs check has reported. There is no clippy, test, coverage, commit-signatures or e2e result, and the PR description says the tests, cargo xtask checks and coverage floors were not run. The ticket asks for the existing tests to show examine output is unchanged, so that evidence is still missing. Please get the workflows running (or run the gate locally and paste the results) and I'll take another look.

Two smaller things:

• Two tests were deleted along with the code they covered. Please confirm cargo xtask coverage still passes against coverage-floors.toml; if a floor does drop, say so in the commit rather than re-blessing silently.
• Small wording nit in docs/architecture.md (inline).

I did not check that the deleted table is identical to main.rs's engine_inventory(), since that table is not in the diff; I'm taking the ticket's word on that.

[juhovainio]

Nit: the edited sentence now runs "the host snapshot behind the internal bridge surface (...) and derives the engine list from main.rs's engine_inventory", and "derives" has no clear subject (the snapshot doesn't derive anything; the module does). Something like "...surface (...), whose engine list is derived from main.rs's engine_inventory, the single table of built-in engine ids that ..." would read cleanly.

[jussielo-amd]

Reworded in 091d923: the sentence now says the host snapshot behind the internal bridge surface is one "whose engine list is derived from main.rs's engine_inventory", so the subject of "derived" is clear.

[jussielo-amd]

Already addressed in 091d923, pushed after this comment: the bridge_snapshot entry now reads "..., whose engine list is derived from main.rs's engine_inventory, the single table of built-in engine ids that rocm examine, rocm update and the --engine help test read as well." I checked the claim against the code: bridge_snapshot.rs, examine_report_text.rs, update_cmd.rs and the serve_engine_help_lists_match_engine_inventory test all read engine_inventory().

[jussielo-amd]

Pushed 091d923 (main merged in, plus the two wording fixes from review), which starts the full CI lanes on this head; before this push only the docs build had run because no workflow had triggered for the branch. I also ran the checks locally and put the results in the description: fmt, clippy, cargo test -p rocm -p rocmd (1639 passed), advised_commands, the examine and engine tests covering the removed branches, and the manifest and crate-edge checks. For the coverage floors: per-crate llvm-cov on main versus this branch shows rocm 83.67% to 83.68% and rocmd 83.20% to 83.15%, both far above their committed floors, so nothing needed re-blessing. On the table identity: at the PR's base commit the removed builtin_engine_inventory body and main.rs's engine_inventory() body hash identically (the same three tuples: lemonade, vllm, aim).

[Copilot]

🟢 Approval recommended

No active review issues are reported for the inventory consolidation and dead-code cleanup, though tests were not rerun during this review.

No open issues 🎉

2 resolved since last review

• The sentence is grammatically incorrect: “... (builtin_codex_bridge_engine_inventory) and derives…
• Grammar: “help read too” should be “help reads too”.

_{Review effort: Balanced}

[jussielo-amd]

Pushed 56424ef, which merges main into this branch after the engine-helper move (#872) landed; there was one conflict, in docs/architecture.md, where main added the engine_install_env.rs entry next to the engine_ipc.rs line this PR rewrites, and I kept both. Nothing else changed. On the merged tree no caller of builtin_engine_inventory or missing_packaged_engine_reason exists, fmt and clippy are clean, cargo test -p rocm -p rocmd gives the same 1639 passed as before the merge, and advised_commands and the manifest, crate-edge and link checks pass. The earlier full CI lanes on this branch had already run on the previous head; the new push starts them again. The only red lanes before were the two Strix Halo Ubuntu end-to-end jobs, which fail the same way on unrelated pull requests. The description is updated to say the merge had a conflict and that #872 has landed.

fixed

[juhovainio]

Looks good. The three removals are each behavior-neutral:

• The two engine tables were identical at the base commit. bridge_snapshot and the --engine help sync test now read engine_inventory, so one sync test covers every reader.
• missing_packaged_engine_reason returned None unconditionally, so the three branches that acted on it were unreachable.
• In rocmd, find_engine_plugin_binary was cfg(test)-only and the daemon_binary_path() call discarded its result. The rocm_engine_protocol dependency and crate edges are unaffected.

No references to the removed names remain in docs, README, skills, tests or e2e features, and CI is green. One cosmetic nit inline.

Non-blocking, outside the diff: the comment in tests/e2e-cucumber/features/examine.feature still says render_engine_inventory_text_with_paths lives in main.rs.

[juhovainio]

Nit: this rewrap leaves a short orphan line (main.rs and are reached). Reflowing the paragraph would tidy it.

[jussielo-amd]

Reflowed the paragraph so the short orphan line is gone (0db18a4).

[jussielo-amd]

Thanks for the approval. Pushed 0db18a4, which changes comments only: it reflows the engine_ipc.rs module comment and fixes the pointer you noted outside the diff, since the comment in examine.feature now says that render_engine_inventory_text_with_paths and append_examine_engine_inventory live in examine_report_text.rs. I grepped the rest of docs, tests, AGENTS.md, CONTRIBUTING.md, xtask and skills for the same stale claim and found none. cargo test -p rocm --bin rocm and the e2e-cucumber lib tests pass, and the branch still merges cleanly with main.

[jussielo-amd]

Merged main into the branch three times (cd1d89b, 9d71097, 6cb8244: plain merge commits, no rebase) because it moved under this PR. The only conflict was docs/architecture.md: I kept this PR's rewritten bridge_snapshot.rs and examine_report_text.rs entries (the engine list derived from engine_inventory, and "two items") and main's newer chat.rs and freeform_answer.rs entries; main never edited the two entries this PR changed, so nothing was lost. Nothing on main uses the helpers this PR deletes: a grep for builtin_engine_inventory and missing_packaged_engine_reason outside the architecture doc is empty, and cargo test -p rocm (1498) and cargo test -p rocmd (167) pass with clippy and fmt clean on the first merge, with fmt, clippy and the rocmd tests re-run after the second. The only test functions this PR removes are missing_packaged_engine_reason_has_no_deferred_first_party_engines and engine_plugin_discovery_finds_runtime_binary, both for the deleted code.