[jussielo-amd]

Summary

Delivers ROCMAI-876. The guard that refuses to bring a managed service back up when it needs an endpoint API key and has none existed twice: in rocm (service_launch.rs) and in rocmd (engine_ipc.rs, which said it "mirrors the guard of the same name in rocm"). Both applied the same two rules (a service launched with --require-api-key that has no key; a service bound to a public host with no key) but worded them differently and named slightly different remediation commands, so a user saw one text when rocm services restart refused and another when the daemon's recovery refused the same service. This replaces both with one guard in rocm-core (which rocmd may depend on; rocmd must not depend on rocm), takes the action wording as a parameter, and deletes both copies. is_public_bind_host, which the guard has to apply itself, moves with it from rocm-engine-protocol to rocm-core (no engine used it; rocm-engine-protocol was the wrong home since it depends on rocm-core, not the other way round).

While proving that the remediation command the refusal names actually clears the condition, the new test found that the printed command was not runnable: rocm serve --require-api-key leaves out the required model, so clap rejects it as printed. The shared text now names rocm serve <model> --require-api-key (and rocm serve <model> --host {host} --allow-public-bind).

What moved

Symbol	From	To
ensure_public_service_has_endpoint_key	apps/rocm/src/service_launch.rs and apps/rocmd/src/engine_ipc.rs (two copies, deleted)	crates/rocm-core/src/endpoint_key.rs, taking a ServiceStart
ServiceStart (Launch, Restart, Respawn)	new	crates/rocm-core/src/endpoint_key.rs
is_public_bind_host	crates/rocm-engine-protocol/src/lib.rs	crates/rocm-core/src/endpoint_key.rs (its test moved with it, same leaf name)

Callers: service_launch.rs (spawn, Launch), service_control.rs (rocm services restart, Restart), supervise.rs (two sites) and watchers/server_recover.rs (Respawn). mcp_argv.rs and is_loopback_host in service_launch.rs now reach is_public_bind_host through rocm_core.

User-visible behaviour

Which wording wins: rocm's. It says why the key cannot be recovered ("the key is dropped when a service stops") and how to choose one (--api-key, ROCM_SERVE_API_KEY), and it states the rule that actually fires (--require-api-key) where rocmd's said "launched with an endpoint API key", which is also true of every public bind. The action wording is the only thing the caller supplies. Every difference, per binary:

• rocm services restart, both rules: the text is byte-identical except the named command now carries the model, rocm serve <model> --require-api-key and rocm serve <model> --host {host} --allow-public-bind (was rocm serve --require-api-key and rocm serve --host {host} --allow-public-bind).
• rocm serve spawn (reached only when a key file is present but unusable, since serve() stores the key first): "so restarting it would reopen it without authentication" becomes "so launching it would open it without authentication", because nothing is being restarted there; plus the same <model> change.
• rocmd respawn (the supervisor's refusal on stderr, and the server-recover watcher's restart_managed_service_refused event, which wraps it in "cannot recover managed service ... : ..."): rule 1 was "refusing to recover a service that was launched with an endpoint API key but no longer has one: it would come back up without authentication, and something outside this machine may still be publishing its port. Relaunch it with rocm serve --require-api-key to issue a new key." and is now rocm's rule 1 with "respawning it would reopen it without authentication", the dropped-key sentence, the <model> command and the "add --api-key <key>, or set ROCM_SERVE_API_KEY" hint. Rule 2 was "refusing to respawn a service bound to the public host {host} without an endpoint API key: it would come back up without authentication. Relaunch it with rocm serve --host {host} --allow-public-bind to issue a new key." and is now the shared rule 2 with the same changes. The "refusing to ..." prefix is gone.
• Remediation commands: before, both binaries named the same two commands (the rocm copy added an optional-flag hint). Now both name the same two commands with <model>.
• No change to which services are refused, to the rule order (a service that asked for a key is told about the key before the public bind is mentioned), or to the refusal happening before a restart stops anything.

Scenarios: nothing in tests/e2e-cucumber/features printed these messages before (searched for the message text and for --allow-public-bind/--require-api-key; serve-24 and serve-30 exercise the separate reuse refusal, which is unchanged). The rocm services restart refusal is the user-facing path, so this adds service-stop-06 (@id:service-stop-restart-refuses-a-public-server-without-its-key, in service_stop.feature, Linux, mock lane, runs every PR). It plants a publicly bound server with no key file and a live recorded process, runs rocm services restart --yes, and asserts the exit status, the refusal text, that the named rocm serve <model> ... command is accepted by the real parser (model filled in, --help last so the parse ends in help only if every earlier word was accepted, with a bogus flag as the negative control), and that the recorded process is still running with no stop recorded and no key created. service-stop-01 to -05 are unaffected and still pass. The daemon's respawn has no scenario because it needs the daemon's recovery tick over a failed service; each of its call sites is covered by a unit test that drives the real call site (below).

Verification

Each test below was shown to fail when its target is broken, by targeted edits that were then reverted. Dropping the public-bind rule from the shared guard fails the rocm-core guard tests, three rocm caller tests (respawn_fails_closed_for_a_public_service_whose_key_is_gone, a_public_bind_is_refused_with_the_command_that_restores_it, the_command_a_respawn_refusal_names_clears_the_refusal), the two service_control restart tests, and the three rocmd tests (recovery_respawn_fails_closed_for_a_public_service_without_a_key, the supervisor's public-service test and the server-recover test); dropping the --require-api-key rule fails the analogous set including a_managed_spawn_refuses_an_invalid_key_file_on_a_service_that_requires_one and the_key_file_rocm_serve_stores_clears_the_recovery_refusal. Re-introducing a private copy of rule 1 in the rocmd supervisor, and passing the wrong ServiceStart at the rocm spawn, the rocm restart and the server-recover sites, each fail exactly the test for that call site (they compare the caller's refusal to the guard's text for its own start). With the public-bind rule dropped, service-stop-06 fails at its refusal step.

Ran: cargo fmt --all --check; cargo clippy --workspace --all-targets -- -D warnings; cargo test -p rocm-core -p rocm-engine-protocol -p rocmd -p rocm --no-fail-fast (all pass: rocm 1500, rocm-core 1013, rocm-engine-protocol 22, rocmd 171) plus e2e-cucumber's feature_naming; cargo xtask manifest --check; cargo xtask check-crate-edges; lychee --offline over docs/architecture.md and docs/testing.md; the e2e scenarios service-stop-01 to -06 against a hooks build; a leaf-name test diff of every touched file against the base (the only removal is the is_public_bind_host test, which moved to rocm-core under the same name); a leak scan of the diff (only hits are the words "private copy" in comments).

Not run: cargo clippy for the x86_64-pc-windows-msvc target (the dependency tree's C build cannot cross-compile on this host); no cfg(windows) code is touched and the guard and its call sites are unconditional. cargo test --workspace --all-targets, scripts/smoke_local.py and the full cargo xtask e2e are left to CI. serve-24 is @requires-gpu @requires-engine:lemonade and does not run on this host.

Notes for reviewers

Read the two commits in order. The first only adds tests that pass on unmodified code: both copies' shipped text pinned, and the check that the command each names clears the refusal (for rocm, through the real clap tree, validate_bind_host, resolve_endpoint_auth and the stored key file; for rocmd, the key file rocm serve writes). It records the missing-model gap as it stood. The second commit moves the guard, flips that assertion, rewrites the call-site tests to go through the shared guard, adds service-stop-06 and updates docs/architecture.md and docs/testing.md.

rocm_engine_protocol::is_public_bind_host no longer exists; nothing in the workspace outside the two binaries used it. The rocm copy's <model> placeholder is the one advised_commands.rs already substitutes, so its scan covers the shared text too (it scans crates/).

Other open PRs touching the same files: the clap-tree move (#889) and #880 touch main.rs/supervise.rs; #859 touches service_launch.rs; #722, #859 and #581 edit docs/testing.md. Expect small textual conflicts in the call sites and docs, none semantic.

[Copilot]

🟢 Approval recommended

No active review issues remain for the shared endpoint-key authentication guard consolidation.

No open issues 🎉

What changed in this PR

Consolidates endpoint-key checks in rocm-core so rocm and rocmd share the same authentication rules and refusal messages without introducing a dependency between the binaries.

Changes:

• Replaces duplicate guards with a shared guard and action-specific wording for launch, restart, and respawn.
• Moves public-bind classification into rocm-core and adds the required <model> placeholder to remediation commands.
• Adds unit and end-to-end coverage for refusal messages, remediation parsing, and leaving running services untouched.

File	Description
tests/​e2e-cucumber/​tests/​e2e/​service_stop_steps.rs	Adds restart-refusal, command-parsing, and unchanged-service assertions.
tests/​e2e-cucumber/​features/​service_stop.feature	Adds the Linux public-server restart-refusal scenario.
docs/​testing.md	Documents shared-guard tests and new restart coverage.
docs/​architecture.md	Documents the guard’s shared ownership and callers.
crates/​rocm-engine-protocol/​src/​lib.rs	Removes public-bind classification and its relocated test.
crates/​rocm-core/​src/​lib.rs	Exports the shared guard, action enum, and host classifier.
crates/​rocm-core/​src/​endpoint_key.rs	Implements the shared guard, refusal wording, and policy tests.
apps/​rocmd/​src/​watchers/​server_recover.rs	Uses the shared respawn guard and tests refusal reporting.
apps/​rocmd/​src/​supervise.rs	Updates both guard calls and strengthens caller tests.
apps/​rocmd/​src/​mcp_argv.rs	Uses public-bind classification from rocm-core.
apps/​rocmd/​src/​engine_ipc.rs	Removes the duplicate guard and updates recovery tests.
apps/​rocm/​src/​services_text.rs	Uses restart-specific shared guarding before stopping services.
apps/​rocm/​src/​service_launch.rs	Removes the local guard and tests shared launch and remediation behavior.
apps/​rocm/​src/​main.rs	Updates an explanatory comment after guard relocation.

_{Review effort: Balanced}

[jussielo-amd]

Superseded by #929: this pull request's commits are included in that branch (head ef5155b), and #929's description covers this change. Closing so the review happens in one place.