test: add BDD E2E tests with cucumber-rs

[fredespi]

Summary

Adds a behavioral end-to-end test layer using cucumber-rs — standard Gherkin .feature files backed by Rust step functions. Tests exercise the full user journey (install → examine → configure → serve → detect → chat) to catch cross-component integration failures that unit tests and smoke scripts miss.

Why cucumber-rs over Robot Framework / plain Rust tests

This PR started with Robot Framework, then switched to cucumber-rs after team discussion. The reasoning:

• BDD earns its keep regardless of audience. The value isn't non-engineers reading scenarios — it's tests that describe user-facing behavior independently of implementation. "Short model names are expanded to their full name" tells you what's broken without reading code. assert_cmd/insta couples to output format and breaks on cosmetic changes.
• Already proven. The 6 bugs at component seams were found by thinking in user journeys, not by reading code. That's what BDD forces you to do.
• One toolchain. Rust step functions compile with the project. No Python dependency, no separate CI setup. Addresses the concern from EAI-7164 about consolidating on Rust.
• Not two stacks. The xtask acceptance path handles install/upgrade lifecycle (imperative, infrastructure-level). Cucumber handles user journey scenarios (declarative, behavioral). Different concerns, one toolchain.
• Real Gherkin. cucumber-rs uses standard Gherkin with Scenario Outline, Background, Rule, and @tags — not Robot Framework's cosmetic prefix stripping. The .feature file is the executable test, not a rewrite of a separate spec.
• Reporting included. HTML report (comparable to Robot Framework's), JUnit XML for GitHub Actions, and Cucumber JSON — all generated automatically.

What's included

• 20 BDD scenarios across 4 features: examine, runtime setup, model serving, chat/detection
• Gherkin .feature files — the spec is the test
• Rust step functions — organized by feature area (chat_steps.rs, examine_steps.rs, serving_steps.rs, runtime_steps.rs)
• Mock OpenAI server — axum-based, OS-assigned ports (no port conflicts)
• HTML report — summary, statistics by tag/feature, expandable scenario details with step-by-step results
• Environment isolation — each scenario gets isolated config/data/cache directories
• CI job on hosted runners (every PR)

Expected-failure pattern

Scenarios that exercise known bugs are tagged @expected-failure-EAI-XXXX. On CI without GPU hardware, these scenarios fail and produce clear failure messages. On GPU hardware, they exercise the actual bug. When a bug is fixed, the test passes — signaling the tag can be removed.

Tag	Scenarios	Bug
@expected-failure-EAI-7218	model_serving 5	PyTorch engine dependency resolution
@expected-failure-EAI-7219	model_serving 1, 2, 6	Short model names not expanded
@expected-failure-EAI-7220	model_serving 3, 4; chat 2, 4	Service registry / default port not in detection
@expected-failure-EAI-7221	chat 6	TUI hardcodes model name instead of querying endpoint
@expected-failure-EAI-7223	chat 5	vLLM rejects tool-calling requests

Each Jira ticket has a comment linking back to its test scenario(s).

Running tests

# All scenarios:
bash tests/e2e-cucumber/run.sh

# Via cargo directly:
ROCM_CLI_BINARY=./target/release/rocm cargo test -p e2e-cucumber --test e2e

# Filter by name:
cargo test -p e2e-cucumber --test e2e -- -n "short model"

# Skip known bugs:
cargo test -p e2e-cucumber --test e2e -- -t "not @expected-failure-EAI-7219"

Test plan

• All 20 scenarios compile and run
• Mock-tier scenarios pass locally
• Expected-failure scenarios fail with clear messages
• HTML report generated with summary, statistics, and step details
• JUnit XML generated for GitHub Actions
• CI job passes on this PR

[rominf]

Nice to see a real behavioral layer going in — the feature files read very cleanly. A few things before this is ready, one of which is functional and important.

🔴 Blocking — the suite can't currently fail CI

tests/e2e.rs uses the builder form E2eWorld::cucumber()....run(...).await; and discards the result. In cucumber 0.23 that path records failures into the writers but does not set a non-zero exit code — only run_and_exit() (or World::run()) does. So cargo test -p e2e-cucumber --test e2e exits 0 even when scenarios fail, and the e2e job goes green regardless of results. That undercuts the PR's premise ("as bugs are fixed, more tests pass") — right now the job wouldn't report either way.

Fix: capture the returned writer and std::process::exit(1) on failed_steps() + parsing_errors() + failed_hooks() > 0 (after the HTML report is generated so the artifact still uploads), or switch to run_and_exit().

Related, same theme:

• @expected-failure-* tags are inert in CI. They only take effect when a human passes -t "not @expected-failure-*"; neither the e2e nor e2e-gpu job passes any filter, so tagged scenarios run like any other. Once the exit-code issue above is fixed, every known-bug scenario would turn the job red — there's no xfail/skip mechanism wired up. Needs a real design (dedicated non-blocking job for the tagged set, or genuine skip-on-tag handling).
• A couple of scenarios don't exercise rocm. e.g. "Chat requests use the model name reported by the endpoint" — the assertion checks what the test's own helper sent to the mock (it reads the model from /models and echoes it back); the rocm binary is never invoked, so it can't fail for a product reason. The mock-based services list scenarios assert the registry contains a mock that's never registered with the CLI, so they fail today for the wrong reason and won't flip to passing when the underlying bug is fixed. Worth routing these through the actual binary or dropping them.

🟠 Fit the project

• run.sh → an xtask subcommand. We already standardize dev/CI tasks in xtask (signing, manifest, tpn, powershell-lint…). A bash wrapper is off-convention and bash-only in a repo that also builds/tests on Windows, and the build step is now duplicated between run.sh and the CI job. A cargo xtask e2e [args] used by both would be cross-platform and single-source.
• src/report.rs (≈400 lines of hand-built HTML) — do we need it? The crate already emits JUnit XML (rendered natively in the GitHub Actions checks UI) and Cucumber JSON (consumable by standard HTML reporters). The custom generator duplicates that, and it currently miscounts undefined/ambiguous steps as passed and prints a placeholder -xx-xx date. I'd lean toward dropping it in favor of the two standard outputs; if we want an HTML artifact in-tree, a compile-checked template (maud/askama) beats format! soup.

🟡 Should-fix

• Per-scenario isolation isn't isolated (e2e.rs): the temp root is keyed only on PID, so all concurrent scenarios share one config/data/cache tree, and the first scenario's Drop remove_dir_alls it out from under the others (risking a fall-back to the real ~/.rocm). Add a per-World unique suffix.
• reqwest::blocking inside async steps (serving_steps.rs) panics under the Tokio runtime ("cannot start a runtime from within a runtime"). GPU-tier only, but it hard-panics instead of waiting — use an async poll loop.
• The new crate gets no clippy (excluded from both the clippy job and the pre-commit hook). Add cargo clippy -p e2e-cucumber --all-targets -- -D warnings to the e2e job.
• .feature files aren't in the heavy path filter, so a feature-only change skips the e2e job. Add **/*.feature.

🔵 Nit

• @expected-failure-EAI-XXXX puts internal Jira IDs in files contributors read/type. Non-blocking and I know it's under discussion — flagging only so it's tracked; GitHub issue numbers would keep the same test↔bug link.

[fredespi]

Thanks for the thorough review — all points addressed. Rebased on main and pushed as one commit (6741054). Summary of what changed:

🔴 Blocking

• Suite can now fail CI. The runner captures the summarized writer and process::exit(1) on failed_steps + parsing_errors + hook_errors (after the HTML report is generated so the artifact still uploads). Verified: the blocking selection exits 0 when green and 1 when a scenario fails.
• @expected-failure tags are now real. Cucumber tag filters are exact-match (no globbing), so each known-bug scenario carries a bare @expected-failure (for filtering) plus its @expected-failure-EAI-NNNN (for traceability). CI now runs three selections:
  • e2e (blocking): not @gpu and not @expected-failure
  • e2e-known-bugs (non-blocking): @expected-failure and not @gpu
  • e2e-gpu (non-blocking): @gpu
• Scenarios now exercise rocm. Mock-based scenarios plant a managed-service record (plain JSON matching the on-disk schema, no crate import) into the isolated services dir, so rocm services list and the local chat provider actually discover the mock. The scenarios that were mislabeled EAI-7220 are retagged/untagged — EAI-7220 is a TUI-only wrong-port bug unrelated to services list, and its real surface can't be driven by the non-interactive CLI (noted below). The mock-only "chat model name" scenario that only tested the helper was dropped.

🟠 Fit the project

• run.sh → cargo xtask e2e. New xtask subcommand builds the release binary + runs the suite (forwarding cucumber args), used by both CI and local dev. Cross-platform, single-source; run.sh removed.
• report.rs kept, bugs fixed. Rather than drop it, fixed the two concrete issues: undefined/ambiguous steps now count as failures (not passes), and the -xx-xx placeholder is replaced with a real UTC timestamp. Happy to revisit dropping it in favor of the JUnit/JSON outputs as a follow-up if you'd prefer.

🟡 Should-fix

• Per-scenario isolation: temp root now has a per-World unique suffix (was PID-only, so scenarios shared one tree and Drop raced).
• reqwest::blocking → async poll loop in the serving steps (dropped the now-unused blocking feature).
• Clippy on the crate: added cargo clippy -p e2e-cucumber --all-targets -- -D warnings to the e2e job; fixed the warnings it surfaced.
• .feature in the heavy path filter so feature-only changes trigger the job.

🔵 Nit

• Kept the EAI-XXXX IDs for now since it's under discussion — happy to switch to GitHub issue numbers if we land on that.

Also, while rebasing

• Engine set: main now supports only lemonade/vllm (Limit serving engines to Lemonade and vLLM #79), so I updated the engines-list assertion, dropped the PyTorch scenario (EAI-7218 is Won't Fix and the engine is gone), and compare expansion across the supported engines.
• Restored the mock/gpu split that the cucumber rewrite had lost: hardware-dependent scenarios (install sdk, real serve, GPU detect) are tagged @gpu so the mock-tier job stays green on ubuntu-latest.

The full workspace test suite and the mock-tier e2e selection pass on Linux.

[rominf]

Reviewed the full diff (33 files, ~50 commits) with a coupling-grouped fan-out and verified findings in source. Overall this is solid, well-tested test infrastructure with no app-runtime changes — but there are a few real defects worth fixing before merge, plus some stale docs.

Verified positives: black-box discipline is fully held (no crate imports in step files); the CodeQL path-injection concern is resolved via tempfile::TempDir; test = false + harness = false correctly isolate the suite from the default workspace build/test; no dependency blast radius; managed-serve teardown covers the real case.

🔴 Blocking

1. ~1,600 lines of new step code + the harness main get zero clippy coverage. tests/e2e-cucumber/Cargo.toml:32 sets test = false on the e2e target, so clippy --workspace --all-targets (ci.yml:359) and the prek hook never lint e2e.rs or the mod-included *_steps.rs — only the lib (capability/expectation/mock_server) is checked. The PR advertises a clippy gate, but it misses the bulk of the added code. Suggest adding a CI step cargo clippy -p e2e-cucumber --test e2e (clippy honors explicit --test the same way the xtask e2e run does); please confirm it actually compiles the step files.

2. Vacuous / silent-pass scenarios that validate nothing but resolve to expect-pass.

• chat_steps.rs:142 assert_privacy_notice_accurate has no assertion — only an eprintln! warning. Its scenario chat-privacy-notice-accurate isn't in expectations.toml, so it resolves to ExpectPass and is guaranteed-green while testing nothing.
• serving_steps.rs:327 (scenario 9, serve-vllm-default-on-instinct) spawns a real --managed serve but never checks rc or readiness; a launch failure after a correct plan-print goes undetected.

Suggest implementing a real check or marking chat-privacy-notice-accurate @skip/xfail with a reason instead of a green no-op, and asserting rc == 0 in scenario 9.

🟠 Should-fix

3. Two divergent reconciliations of the same report.json in crates/e2e-report/src/lib.rs: the CI-gate path (scenario_results_by_id, lib.rs:250, consumed at e2e.rs:580) treats a cucumber "skipped-status" scenario as not passed → exit 1, while the grid/consolidated path (id_pass_map lib.rs:702, tally lib.rs:786) treats it as passed. The same artifact can fail the job while rendering green in the consolidated grid. Suggest one shared predicate.

4. GPU serve jobs assume serial execution but nothing enforces it. There's no @serial tag and no .max_concurrent_scenarios(1) (e2e.rs:508); cucumber-rs defaults to 64 concurrent scenarios, yet serving_steps.rs hardcodes shared ports (SERVE_PORT = 11435, ASSISTANT_PORT = 8001) with kill-by-port logic whose comments assume "one serial GPU box." This does not affect the PR gate — every GPU serve scenario is @requires-gpu and resolves to Skip on the blocking mock job — but it makes the non-blocking GPU jobs flaky. Suggest tagging @requires-gpu scenarios @serial, or capping concurrency to 1 when a GPU is detected.

5. Blocking mock e2e job skips on non-heavy PRs — inconsistent with every sibling required job. e2e (ci.yml:591) gates needs.changes.outputs.heavy == 'true' at the job level, so the job is skipped on a non-heavy PR. Every sibling required job (build-and-test, test, clippy, prek, windows-build-and-test) instead runs if: github.event_name != 'workflow_dispatch' at the job level and gates heavy at the step level — deliberately, per this file's own warnings that a never-produced required check stalls the merge queue. The heavy glob doesn't match expectations.toml, so a PR editing only the xfail matrix (exactly what this suite exists to validate) skips the mock gate on that PR. merge_group forces heavy=true as a backstop, which is why this is Should-fix rather than hard-Blocking. Suggest matching the sibling pattern (job-level if != workflow_dispatch, heavy/build-and-test.result on the steps).

6. pkill -f 'vulkan/llama-server' (ci.yml:668) is unanchored. Unlike its five sibling pkill lines (all scoped to /tmp/rocm-e2e.* / e2e-shared.* / rocm-e2e), this pattern kills any process containing vulkan/llama-server on the shared self-hosted runner — including a legitimate manual-testing serve — contradicting the step's own "never any /workload manual-testing processes" comment. Suggest anchoring it to an e2e marker.

7. Hook/before-failure scenarios can be mis-scored as passing in scenario_status (lib.rs:132): it inspects only el.steps, never el.before/el.after, so a Before-hook failure (steps: []) falls through to "passed". Not a live gate escape (the harness independently checks summary.hook_errors() > 0 at e2e.rs:569 and exits first), but the HTML/consolidated report built from the same report.json would score a hook-failed scenario green. Suggest treating a Failed before/after entry as failed.

8. user_serves_default_engine (serving_steps.rs:323) waits with the model-agnostic wait_for_endpoint while every sibling serve step uses wait_for_model(..., Some(substr), ...) specifically to defend against a leaked prior serve still answering on shared port 11435. Its Then (assert_endpoint_responds) also never checks resp["model"], so scenarios 6/6b can pass against a stale/wrong server. Suggest using the model-substring guard and asserting the model id.

9. README is significantly stale vs. the current design. It documents the abandoned global @expected-failure/@gpu tag model and 3-tier CI scheme that no longer exist (no .feature uses those tags; CI runs one untiered cargo xtask e2e per platform). README.md:59 shows a -t "not @expected-failure-EAI-*" filter that now matches nothing, and the directory-layout diagram omits expectations.toml, src/capability.rs, src/expectation.rs, and two feature files. Separately, expectations.toml:9 references tests/e2e-cucumber/tests/e2e/expectation.rs but the file is at tests/e2e-cucumber/src/expectation.rs.

🟡 Nits

• resolutions map is keyed only by @id with no collision detection (e2e.rs:550): a copy-pasted scenario with a forgotten id-tag would silently overwrite an earlier resolution (no live collision today). Consider a startup dedupe assertion.
• Stats::bar_widths() (lib.rs:99) dumps the integer-division remainder into the skip bar, so a report with 0 skips can render a nonzero grey sliver (counts are correct; bar only is cosmetically off).
• e2e-report crate doc says "only maud + serde" but it also depends directly on serde_json.
• journey_theme_picker (dash journeys) asserts only !out.is_empty() rather than the theme_name/modal state transition the sibling unit tests already assert cheaply.
• normalize_model_id bidirectional substring match (serving_steps.rs:542) is looser than exact match; safe for current fixtures, latent if shorter/overlapping model names are added.
• commands.jsonl append (e2e.rs:316) relies on incidental POSIX write atomicity rather than a lock; low risk today, compounds with #4.
• Stale comment at ci.yml:650 says "15-min job cap" but the job's timeout-minutes is 35.
• Head branch is still named test/add-e2e-robot-framework while the PR is cucumber-rs (cosmetic).

🔵 Consider

• Internal Jira IDs (EAI-XXXX) pervade public files (expectations.toml, README, feature files, serving_steps.rs, rocm-core test comments). CONTRIBUTING says work is tracked via GitHub Issues, but there's no documented rule against ticket IDs specifically, so flagging as a Consider rather than a violation: these refs are meaningless to external contributors and leak internal tracker structure into a public repo. Consider mapping them to GitHub issues or a neutral KNOWN-BUGS.md.
• effective_serve_engine in capability.rs deliberately duplicates rocm-core's engine rule — verified no divergence for any covered family and the drift is documented with guard tests; fine as-is.

---

Nice work overall — the per-scenario expectation resolution and reconciled report grid are a real improvement over the global-tag model. The blocking items above (clippy coverage hole + the two vacuous scenarios) are the main things I'd want fixed; #3/#5 are the highest-value should-fixes for CI trustworthiness.

[fredespi]

Thanks for the thorough second pass — all findings addressed. Summary (latest commits 9f786da..0e6c80e):

🔴 Blocking

1. Clippy on the harness + step files — added a CI step cargo clippy -p e2e-cucumber --test e2e -- -D warnings (clippy honours the explicit --test, so it now compiles/lints e2e.rs + all *_steps.rs). It surfaced 9 real lints, all fixed.
2. Vacuous scenarios — chat-privacy-notice-accurate now fails (it's TUI-only, unverifiable black-box) and is tracked xfail in expectations.toml (EAI-7222) instead of a silent green no-op; serve-vllm-default-on-instinct now asserts rc == 0.

🟠 Should-fix
3. Unified the report.json pass predicate — the CI gate (scenario_results_by_id) and the grid (id_pass_map) both route through one scenario_passed (all-steps-passed), so a skipped scenario can no longer fail the job while rendering green.
4. GPU serves are now serial — .max_concurrent_scenarios(1) when a GPU is present (mock keeps default parallelism).
5. Mock e2e job now runs at the job level (not skipped on a non-heavy PR — no more stalled required check); heavy/build-and-test gating moved to the steps, matching the sibling jobs.
6. Removed the unanchored pkill -f 'vulkan/llama-server' — the anchored /tmp/rocm-e2e line already covers the scenario-spawned assistant.
7. scenario_status now treats a failed before/after hook as failed.
8. Default-engine serve uses a model-aware readiness wait + asserts the reply's model id (can't pass against a leaked serve on the shared port).
9. README rewritten to the current @requires-* + expectations.toml + one-job-per-platform model; fixed the expectations.toml path reference.

🟡 Nits — bar_widths skip remainder, e2e-report crate doc (serde_json), duplicate-@id startup assertion, stale 15-min comment.

🔵 Consider (EAI IDs in public files) — noted; leaving as a follow-up decision since it spans many files and isn't a code defect.

Also, separately: added large-model (Qwen3.6-27B) vLLM coverage behind a new @nightly tag (skipped per-PR, run in a nightly e2e-gpu-nightly job) so the per-PR GPU run stays fast; and a VRAM-drain wait so a large serve's residual memory can't starve the next scenario.

Re-review welcome.

[volen-silo]

🔴 Automated review · pr-review-watcher · 0e6c80e

Summary

Adds a BDD (cucumber-rs) end-to-end test layer — Gherkin features + Rust steps, a mock OpenAI server, per-scenario pass/xfail/skip resolution, an HTML/JUnit/JSON report crate, and CI wiring across mock + self-hosted GPU runners. Verdict: Needs work — the design is sound and the code is high quality, but it ships internal tracker IDs upstream, which the repo's own contribution rules forbid. Verified: e2e-report (31) and e2e-cucumber --lib (23) unit tests pass, both new crates compile clean, no conflict markers; confirmed the blocking e2e mock job is a real gate (xtask e2e bail!s on cucumber non-zero exit) and the consolidated report is informational-only (does not gate CI). Blocking: 1 · Non-blocking: 6.

🚫 Blocking (must fix before merge)

Internal ticket identifiers (EAI-NNNN) shipped on upstream surfaces. AGENTS.md §2 explicitly forbids "internal ticket identifiers" in "PR titles/bodies, ... commit messages, ... code comments, fixtures, and logs," and the review treats a project-instruction violation introduced by the PR as blocking. This PR introduces them broadly:

• Code / fixtures (52 refs, 8 files): tests/e2e-cucumber/expectations.toml (26), tests/e2e-cucumber/tests/e2e/serving_steps.rs (e.g. serving_steps.rs:432,446), tests/e2e-cucumber/src/expectation.rs, tests/e2e-cucumber/tests/e2e.rs, tests/e2e-cucumber/tests/e2e/chat_steps.rs, tests/e2e-cucumber/features/model_serving.feature, crates/e2e-report/src/lib.rs (test fixtures ~1910/2132/2160/2297), crates/rocm-core/src/lib.rs (test comments).

• Commit messages: ~20 (e.g. EAI-7052, EAI-7333, EAI-7383/7384 in subjects).

• PR title/body: EAI-7164/7218/7219/7220/7221/7223.

  Fix: keep the (good) fixed-bug-flips-to-XPASS design, but replace internal IDs with the public issue tracker (ROCm/rocm-cli#NNN) or a neutral bug slug across code, fixtures, commit messages (rebase/reword), and the PR title/body. Run the AGENTS.md §2 leak scan until the diff and non-diff surfaces are clean.

Non-blocking

• crates/e2e-report/src/lib.rs:825 — scenario_pass_map uses scenario_status(el) != "failed", counting a skipped scenario as passed, contradicting the file's own single-source-of-truth scenario_passed (line 182). Affects only the command-coverage display table (not the CI gate); route it through scenario_passed.
• crates/e2e-report/src/lib.rs:193,606 — a truncated/malformed report.json parses as zero scenarios (.ok() swallows the error), so with a valid platform.json every expect-pass reconciles to Missing and the platform renders "PASS"; a lost-results run is indistinguishable from all-pass. Report is non-gating, but distinguish unparseable from empty and surface it as ERROR/UNKNOWN.
• tests/e2e-cucumber/src/capability.rs:173 — the capability probe ignores rocm examine/engines list exit status and treats spawn failure as empty output, so a broken rocm on a GPU host silently degrades to a GPU-less profile and every @requires-gpu scenario resolves to skip. Check exit status and fail loudly.
• tests/e2e-cucumber/tests/e2e/chat_steps.rs:32-35,44 — dead step code: send_chat_request (#[when]) is referenced by no feature; user_offered_endpoint is a no-op. Remove.
• tests/e2e-cucumber/tests/e2e/serving_steps.rs:507 — assert_service_in_list only checks "127.0.0.1" appears anywhere in services list output, not the row for the model under test; tighten to the specific port (as the sibling assert_endpoint_port does).
• PR body references bash tests/e2e-cucumber/run.sh (3×) but no run.sh exists — stale from the Robot Framework iteration; the README correctly documents cargo xtask e2e. Update the PR body. (Also: every new-job uses: in ci.yml/nightly.yml is a moving tag vs AGENTS.md §6's SHA-pin rule — but this is a pre-existing repo-wide pattern this PR extends, not a regression; worth a follow-up pinning pass.)

[rominf]

The remaining review findings are non-blocking test-infrastructure hardening and can be handled in a focused follow-up.