[fredespi]

Summary

Pins the public-bind endpoint authentication contract (EAI-7409) end to end: with --allow-public-bind, the CLI issues a key once with Bearer guidance, and the live endpoint answers 401 without a key, 401 with a wrong key, and 2xx with the issued key. This was the highest-value item left from EAI-8072; the pre-flight refusal was already pinned in the earlier PR.

The scenario is @requires-gpu @requires-engine:vllm, so it skips on Windows/lemonade hosts (where the CLI deliberately refuses a public bind) and runs only on MI300X. Readiness probing retries while the engine binds, and a timeout reports serve output, service log tail and the last endpoint response.

Test plan

• Linux container gate: 224 scenarios, 222 passed, 2 xfail as expected, 0 unexpected failures; exact CI clippy commands clean.
• MI300X dispatch earlier on this scenario: green (1 scenario, 6/6 steps).
• MI300X re-run on the current head is queued behind the serial runner; later dispatches on 2026-08-25 failed with a runner-side vLLM "Failed to infer device type", not a test failure.

[rominf]

🔴 Automated review · pr-review-watcher · f75e4ee

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.
Pins the "missing/invalid credentials are rejected" criterion of EAI-7409, which now resolves to ROCMAI-318. It does not cover the ticket's other criteria (secrets not logged or persisted insecurely, status/log protection), and it does not claim to.

Blocking

• (new) The new scenario's title breaks the required feature_naming test, so CI is red on Linux and Windows — tests/e2e-cucumber/features/networking.feature:64
  • The scenario is titled Scenario: 4 - A public endpoint ….
  • tests/e2e-cucumber/tests/feature_naming.rs:148 requires per-feature sequential prefixes, and the three scenarios above it use networking-01 to networking-03.
  • I ran cargo test -p e2e-cucumber --test feature_naming in the checkout. It exited 101: scenario_names_are_indexed_sequentially_per_feature panicked with networking.feature: scenario 4 is named "4 - A public endpoint issues a key once and then enforces it" but must start with "networking-04 - ".
  • The same panic is the only failure in both "Test (affected crates)" (job 120489475310) and "windows-build-and-test" (job 120489475455). The last three main runs of that workflow are green.
  • Confidence 100 · mechanical · Fix: rename it to Scenario: networking-04 - A public endpoint issues a key once and then enforces it.

Non-blocking

• (new) The endpoint URL hardcodes the shared serve port instead of using the existing constant — tests/e2e-cucumber/tests/e2e/endpoint_auth_steps.rs:25
  • ENDPOINT_URL = "http://127.0.0.1:11435/v1" copies the value owned by SERVE_PORT (serving_steps.rs:172) and SERVE_BASE_URL (serving_steps.rs:78).
  • If the port moves, ensure_serve_port_free frees the new port while these steps keep probing 11435.
  • Separately, the step sets world.endpoint and world.model_name (lines 119 and 121), but the Then-steps read the constant, not world.endpoint.
  • Confidence 75 · mechanical · Fix: make SERVE_BASE_URL pub(crate) or build the URL from SERVE_PORT, and have the Then-steps read world.endpoint.
• (new) The new serve step lacks the relaunch and failure evidence its sibling has — endpoint_auth_steps.rs:99-122, compared with serving_steps.rs serve_and_wait / serve_gpu_model
  • It launches through run_rocm_ok with a single attempt.
  • The sibling path reports a non-zero exit together with the engine log, and it has a budgeted relaunch. Per its own comment, that relaunch exists because this model on MI300X has both timed out and served fine within the same run.
  • Here, a non-zero exit panics without the service-log tail. Only the timeout path attaches it.
  • Reusing serve_and_wait as-is would not work, because its readiness probe is unauthenticated and would get 401. So this is a matter of extending the shared helper, not swapping it in.
  • Confidence 70 · logic.
• (new) The feature's header comment is now inaccurate — tests/e2e-cucumber/features/networking.feature:3-9
  • It says these scenarios "run on the mock (no GPU) on every platform the suite runs on".
  • The new scenario is @requires-gpu @requires-engine:vllm and runs only on vLLM-capable dcgpu hosts.
  • The scenario-level comment is correct; the header is what is now stale.
  • Confidence 75 · mechanical · Fix: qualify the header, e.g. "all but networking-04".

Decisions for the author

• A cold vLLM serve on the per-PR MI300X lane, versus pinning enforcement at all — tradeoff
  • Enforcement lives in the engine (ROCM_SERVE_API_KEY_FILE → VLLM_API_KEY at engines/vllm/src/process.rs:256), and nothing else tests that path: there is no unit test touching VLLM_API_KEY. So a GPU scenario is the only place this can be proven.
  • The cost is roughly 65 s of serial MI300X time per run (observed: 16:39:36 to 16:40:41). The scenario comment argues the choice, and this note is only to confirm it was deliberate.

Positive signals

• The readiness wait polls with the issued key and treats a refused connection as "not yet". That means an open endpoint (200 for any bearer) cannot satisfy the 401 assertions, and a stray loopback engine on 11435 produces a red result, never a false green.
• The timeout panic carries serve stdout, the service-log tail and the last endpoint status. That separates "never started" from "still loading" from "rejecting its own key".

Deployment notes

None

What this covered

• What was read: every changed file in prw-base...f75e4ee6, read completely. Also: the CLI code that prints and hands over the key (service_launch.rs, serve_cmd.rs), the vLLM adapter's key handling, E2eWorld's Drop teardown, the expectation and tag resolution for @requires-engine, and runtime_steps.rs for the Given step.
• Verdicts on the added tests:
  1. Would it pass with the behaviour reverted? No. The PR changes no production code; it pins existing behaviour. By reading, if the key stopped reaching vLLM, the readiness wait would pass and the unauthenticated step would get 200 instead of 401, so the scenario fails.
  2. What stays green if one branch breaks? Nothing found. A wrong-key-accepted engine fails the wrong-key step. A key printed but not passed to the engine times out in readiness. A second dedicated api key: line fails the show-once step.
  • I did not run either mutation: the scenario needs a GPU and none was available here. vLLM's guarding of /v1/* is from knowledge of vLLM, not from source in the checkout.
• Lane gating, cleanup and CI:
  • Gating checked: the scenario skips on Strix Windows, Strix Ubuntu and W7900, and runs on MI300X.
  • Cleanup checked: E2eWorld Drop runs rocm services stop even when a step panics, so the 0.0.0.0 bind lasts no longer than any other serve. No expectations.toml rows are affected.
  • On "E2E tests (MI300X, current)" the new scenario ran and passed (6/6 steps). The job reports 246 scenarios (240 passed, 6 failed). The failures are at chat.feature:70, model_serving.feature:11/16/68, runtime_setup.feature:32 and therock_next_generation.feature:166. Main's MI300X run on the PR's base 1c33248 (run 39975715090) fails at exactly the same six locations with the same count. Five of the six are declared xfail; the one unexpected failure, therock-next-09 (a vLLM/amd-aiter dependency conflict), is the same on main. None of it is attributable to this PR.
• Local runs: cargo check and cargo clippy -D warnings on the e2e target are clean (exit 0).
• Passes that did not run:
  • Prior-changes pass: earlier reviews are out of bounds for this run.
  • History: only a light check (recent git log on the feature file).
• Fan-out: two independent workers, one for the code pass and one for CI attribution. I verified their findings myself.
• Discussion: not reconciled against the PR's comments or reviews.

Requested changes addressed: commits were pushed after this review and all review threads are resolved. Re-review requested.

[rominf]

🔴 Automated review · pr-review-watcher · 02726d1

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.

Round 2. No change request filed. The earlier change request's blocker was the networking-04 title failing feature_naming. It was standing, not re-found blind, and was settled by putting it to the reviewer after this review: the title now reads networking-04 - …, feature_naming passes 4/4 locally, and "Test (affected crates)" and "windows-build-and-test" are green at this head. The one new blocking-class finding below (a stale sentence in docs/testing.md) understates coverage rather than overstating it, so the automation reports it without gating on it. The only red check, "E2E tests (MI300X, current)", has the same 5 xfail + 1 unexpected failure (therock-next-09) as main's run at the base 1c33248; networking-04 passed all 6 steps there.

Review — needs work

Full review of the whole change.
This PR adds end-to-end evidence for one acceptance criterion of EAI-7409 (now ROCMAI-318): "Missing/invalid credentials are rejected". It does not claim to close the ticket, which is still In Progress.

Blocking

• (new) docs/testing.md still says this assertion is not exercised anywhere — docs/testing.md:955-958
  The doc says: "Verifying that the running server actually rejects an unauthenticated request requires a live engine; that end-to-end assertion is a deferred follow-up and is not yet exercised by the GPU acceptance scripts." networking-04 is exactly that assertion, so the doc became false when this PR landed the scenario. AGENTS.md §5 requires docs/testing.md to be updated in the same change and the claim to be checked wherever it repeats. The code is the side that governs here: the scenario exists and passed on MI300X at this head.
  Confidence 85 · mechanical · Fix: rewrite the sentence to name networking-04 (features/networking.feature) and say it runs on the MI300X vLLM lane only, which is non-required. Native Windows and lemonade-default hosts skip it.

Non-blocking

• (standing, expanded) serve_public_bind brings back the serve-then-poll pattern that serve_and_wait was written to replace — tests/e2e-cucumber/tests/e2e/endpoint_auth_steps.rs:84-102, :108-129

  • What the in-file guidance says: the doc comment on serve_and_wait in serving_steps.rs says it "Replaces the run_rocm_ok + readiness-poll pair", and adds "A non-zero exit is reported the same way rather than through run_rocm_ok, since the engine log explains those launches too."
  • Lost failure evidence: the new step calls crate::run_rocm_ok and discards the device-state string that ensure_serve_port_free() returns. Its timeout panic carries no device state, stderr, archived log or stop status (serve_failure_evidence).
  • Lost readiness guard: it returns on any 2xx and drops host_serve_target().2 as _. model_is_ready's doc warns that a plain 200 on the shared port 11435 can come from a leaked serve. In that case the "without the key is refused" step would fail with a message that looks like a security defect, when the real cause is a harness leak. This is a misleading red, not a false green.
  • Commit message: 039d0e4 claims "the same diagnostics the other serve steps already collect", which is not accurate. The repo squash-merges, so the squash message should not repeat that claim.

  Confidence 85 · architectural · Fix: add an optional bearer token to model_is_ready/serve_and_wait (and have it return stdout), or make serve_failure_evidence pub(crate). Then route both the non-zero-exit and timeout paths through it, and require the ready-substring in the authenticated /models body.

• (standing) The feature header comment is now partly stale and not re-wrapped — tests/e2e-cucumber/features/networking.feature:6-9
  Line 6 still says the scenarios drive the binary "against the mock server", and the feature's stated purpose is the CLI's native HTTP stack. networking-04 runs on a real engine, and its HTTP checks go through the harness's reqwest, not the CLI. The edited line 8 is 105 characters while its neighbours wrap at about 80.
  Confidence 90 · mechanical · Fix: re-wrap the header, and say networking-04 is a GPU serve scenario that checks enforcement from the harness.

Decisions for the author

• (standing) The scenario only runs on a non-required lane — tradeoff
  @requires-engine:vllm limits it to MI300X. .github/workflows/e2e-selfhosted.yml says that check "isn't in the required list" and skips merge_group, so a regression in key enforcement would not block a merge. The lemonade-on-Linux enforcement path (--api-key-file) has no scenario. The tag is well argued (Windows lemonade refuses a public bind by design) and the PR text names the lane. Confirm you accept that this contract is guarded only in an advisory way.
• (new) What "issues a key once" proves — non-blocking-improvement
  shows_key_once counts lines that start with api key:. It would stay green if the key leaked onto any other line. It also never checks the documented promise that the key does not appear in rocm services or rocm logs. Possible fixes:
  • assert output.matches(key).count() == 2 (the api key: line plus the curl example), or
  • add a step that lists services and checks the key is absent.

Positive signals

• try_models_status and get_models_status keep two cases apart: "no listener yet" is retried during readiness, and "answered with a status" is asserted. The timeout panic names the three cases that look identical from outside, including the one where the engine rejects the key it was issued, which would be a real contract defect.
• The @requires-engine:vllm comment explains why this is a tag rather than an xfail row, and the cited facts check out: ensure_public_bind_engine_supported and resolve_endpoint_auth_loopback_stays_credential_free exist as described.

Deployment notes

None

What this covered

• Read: all four changed files in full, against prw-base 1c33248…02726d1a. Also the surrounding harness (serving_steps.rs serve/readiness helpers, run_rocm_ok, capability tags, the World cleanup), the CLI's public-bind and key-printing code, and how the vLLM and lemonade engines receive the key. The work was split across two independent read-only workers (code pass; agent-instruction/history/comments/commits pass), plus my own verification.
• Run locally:
  • cargo test -p e2e-cucumber --test feature_naming: 4/4 passed.
  • cargo clippy -p e2e-cucumber --all-targets -- -D warnings: clean.
  • Sign-off check: all 6 commits signed off.
• CI at the head:
  • Every check passes except "E2E tests (MI300X, current)".
  • In that job, networking-04 passed all 6 steps.
  • The job's 6 failures are the same 6 scenarios that fail on main's run at this base SHA. They are pre-existing, not caused by this PR.
• Step verdicts:
  • The "without the key" and "wrong key" steps both go red if the engine stops enforcing the key.
  • The "issued key is accepted" step adds nothing beyond the readiness wait.
• Could not run: GPU scenarios (no GPU here), so the GPU evidence above is CI's, not mine. Prior-changes pass: no earlier review comments were read, by design.
• Not reconciled: this report was not checked against the PR's comment discussion.

[fredespi]

#722 (review)

Fixed in 9008999. docs/testing.md now names networking-04 and says it runs on the MI300X vLLM lane only (non-required; Windows/lemonade-default hosts skip it). The key-once step now fails if the key appears on any line other than the api key line and the curl example. The networking.feature header is re-wrapped and says networking-04 is a GPU serve scenario checked from the harness. Not done, left for a follow-up: routing serve_public_bind through serve_and_wait/serve_failure_evidence needs a shared-helper change (bearer token in model_is_ready), so it stays out of this PR. The non-required-lane tradeoff is accepted as documented.