test(e2e): address PR review — clippy gate, real assertions, CI hardening

Resolves the review findings on PR #69:

Blocking:
- Wire clippy for the e2e harness + step files: the `test = false` e2e target is
  skipped by `clippy --all-targets`, leaving ~1.6k lines unlinted. Add a CI step
  `cargo clippy -p e2e-cucumber --test e2e -- -D warnings`, and fix the 9 lints it
  surfaced (map/unwrap_or_else→let-else, collapsible-if, Duration::from_mins,
  operator-precedence, doc-paragraph, items-after-statements).
- Two vacuous scenarios no longer pass while testing nothing:
  chat-privacy-notice-accurate now fails (TUI-only, unverifiable black-box) and is
  tracked xfail (EAI-7222); serve-vllm-default-on-instinct asserts rc == 0.

Should-fix:
- Unify the report.json pass predicate: the CI gate (scenario_results_by_id) and
  the grid (id_pass_map/tally) disagreed on "skipped" status; both now route
  through scenario_passed (all-steps-passed), so the same artifact can't fail the
  job yet render green.
- Enforce serial GPU serves: cap cucumber concurrency to 1 when a GPU is present
  (shared port 11435 / one card); mock keeps default parallelism.
- Mock e2e job runs at job level (not skipped on non-heavy PRs, which stalled the
  merge queue for a required check); heavy/build-and-test gating moved to a step.
- Remove the unanchored `pkill -f 'vulkan/llama-server'` (killed manual serves on
  the shared runner); the anchored /tmp/rocm-e2e line already covers it.
- scenario_status treats a failed before/after hook as failed (was scored passed).
- Default-engine serve uses a model-aware readiness wait + asserts the reply model
  id, so it can't pass against a leaked prior serve on the shared port.
- Update the stale README (abandoned tag model / 3-tier CI → @requires-* +
  expectations.toml + one-job-per-platform) and fix the expectations.toml path ref.

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

Signed-off-by: fredespi <fredrik.espinoza@gmail.com>

Author: fredespi

.github/workflows/ci.yml

@@ -358,6 +358,14 @@ jobs:
         if: needs.changes.outputs.rust == 'true'
         run: cargo clippy --locked --workspace --all-targets -- -D warnings
 
+      # The e2e-cucumber `e2e` target sets `test = false` (so nextest/`cargo test
+      # --all-targets` skip the custom harness), which also excludes it from
+      # `clippy --all-targets` above — leaving the ~1.6k lines of harness + step
+      # code unlinted. Clippy honours an explicit `--test`, so lint it directly.
+      - name: Clippy (e2e harness + steps)
+        if: needs.changes.outputs.rust == 'true'
+        run: cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings
+
       - name: MANIFEST.md dependency table is current
         if: needs.changes.outputs.rust == 'true'
         run: cargo xtask manifest --check
@@ -585,38 +593,56 @@ jobs:
     timeout-minutes: 15
     runs-on: ubuntu-latest
     needs: [changes, build-and-test]
-    # On push/PR/merge_group: run when build-and-test succeeded and the change is
-    # heavy. On workflow_dispatch: build-and-test is skipped, so tolerate that and
-    # gate on the platform input instead.
+    # This is a REQUIRED check, so — like every sibling required job — the job
+    # itself must always run (except on a manual dispatch that didn't select the
+    # mock platform), and the actual work is gated at the STEP level. Gating
+    # `heavy` at the job level would SKIP the job on a non-heavy PR, and a required
+    # check that is never produced stalls the merge queue. `merge_group` forces
+    # heavy=true, so the mock gate always executes there as a backstop.
     if: >-
       always()
       && needs.changes.result == 'success'
-      && (
-        (github.event_name != 'workflow_dispatch'
-          && needs.build-and-test.result == 'success'
-          && needs.changes.outputs.heavy == 'true')
-        || (github.event_name == 'workflow_dispatch'
-          && (inputs.platform == 'all' || inputs.platform == 'mock'))
-      )
+      && (github.event_name != 'workflow_dispatch'
+          || inputs.platform == 'all' || inputs.platform == 'mock')
     steps:
       - uses: actions/checkout@v6
 
+      # Whether the real work runs this time. On push/PR/merge_group: only when
+      # build-and-test succeeded and the change is heavy. On workflow_dispatch:
+      # build-and-test is skipped, so tolerate that and rely on the job-level
+      # platform gate above.
+      - name: Decide whether to run
+        id: gate
+        run: |
+          if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then
+            echo "run=true" >> "$GITHUB_OUTPUT"
+          elif [ "${{ needs.build-and-test.result }}" = "success" ] \
+            && [ "${{ needs.changes.outputs.heavy }}" = "true" ]; then
+            echo "run=true" >> "$GITHUB_OUTPUT"
+          else
+            echo "run=false" >> "$GITHUB_OUTPUT"
+          fi
+
       - name: Install native build deps
+        if: steps.gate.outputs.run == 'true'
         run: sudo apt-get update && sudo apt-get install -y pkg-config libcap-dev
 
       - uses: actions-rust-lang/setup-rust-toolchain@v1
+        if: steps.gate.outputs.run == 'true'
 
       # The workspace test job selects only affected crates and runs the cucumber
       # harness, not the library's own unit tests — run the capability/expectation
       # /report unit tests (resolver logic, grid reconciliation) here.
       - name: Unit tests (e2e-cucumber lib)
+        if: steps.gate.outputs.run == 'true'
         run: cargo test -p e2e-cucumber --lib
 
       - name: Run E2E tests
+        if: steps.gate.outputs.run == 'true'
         run: cargo xtask e2e
 
       - name: Upload E2E report
-        if: always()
+        if: always() && steps.gate.outputs.run == 'true'
         uses: actions/upload-artifact@v4
         with:
           name: e2e-report
@@ -647,10 +673,11 @@ jobs:
       )
     continue-on-error: true
     env:
-      # Bound serve readiness below the 15-min job cap so a serve that never comes
+      # Bound serve readiness below the 35-min job cap so a serve that never comes
       # ready fails the scenario with a real error instead of hanging until the
       # job is cancelled. 300s is ample for a real MI300X vLLM cold-start;
-      # per-scenario known-bug overrides in expectations.toml shorten it further.
+      # per-scenario overrides in expectations.toml / a `@serve-timeout` tag adjust
+      # it (shorter for known bugs, longer for large models).
       E2E_SERVE_TIMEOUT_SECS: "300"
     steps:
       - uses: actions/checkout@v6
@@ -665,7 +692,11 @@ jobs:
           pkill -f '/tmp/rocm-e2e.*llama-server' 2>/dev/null || true
           pkill -f '/tmp/rocm-e2e.*vllm serve' 2>/dev/null || true
           pkill -f 'e2e-shared.*llama-server' 2>/dev/null || true
-          pkill -f 'vulkan/llama-server' 2>/dev/null || true
+          # NOTE: no unanchored `pkill -f 'vulkan/llama-server'` — the lemonade
+          # Vulkan assistant an e2e scenario spawns lives under /tmp/rocm-e2e-*
+          # and is already caught by the first line; an unanchored pattern would
+          # also kill a legitimate /workload manual-testing serve on this shared
+          # self-hosted runner.
           pkill -f '__engine-serve-http.*rocm-e2e' 2>/dev/null || true
           pkill -f 'e2e-target/release/rocm daemon' 2>/dev/null || true
           rm -rf /tmp/rocm-e2e-* 2>/dev/null || true

crates/e2e-report/src/lib.rs

@@ -4,7 +4,7 @@
 
 //! HTML/markdown reporting for the cucumber E2E suite.
 //!
-//! Lives in its own lean crate (only `maud` + `serde`) so both the
+//! Lives in its own lean crate (only `maud` + `serde`/`serde_json`) so both the
 //! `e2e-cucumber` test harness and `xtask` can depend on it without pulling the
 //! harness's heavy tree (cucumber/axum/reqwest/tokio) into `xtask`.
 
@@ -30,6 +30,21 @@ struct Element {
     tags: Vec<Tag>,
     #[serde(default)]
     steps: Vec<Step>,
+    /// Before-scenario hooks (cucumber JSON `before`). A failing Before hook
+    /// leaves `steps` empty, so it must be inspected too or the scenario scores
+    /// as passed despite never running.
+    #[serde(default)]
+    before: Vec<Hook>,
+    /// After-scenario hooks (cucumber JSON `after`).
+    #[serde(default)]
+    after: Vec<Hook>,
+}
+
+/// A cucumber before/after hook entry — we only need its result status.
+#[derive(Deserialize)]
+struct Hook {
+    #[serde(default)]
+    result: StepResult,
 }
 
 #[derive(Deserialize)]
@@ -102,7 +117,10 @@ impl Stats {
         }
         let pw = self.passed * 100 / self.total;
         let fw = self.failed * 100 / self.total;
-        let sw = 100 - pw - fw;
+        // Derive the skip width from the actual skipped count, not `100 - pw - fw`
+        // — the latter dumped the integer-division remainder into the skip
+        // segment, rendering a grey sliver even when there are zero skips.
+        let sw = self.skipped * 100 / self.total;
         Some((pw, fw, sw))
     }
 
@@ -130,6 +148,14 @@ fn stats_bar(stats: &Stats) -> Markup {
 }
 
 fn scenario_status(el: &Element) -> &'static str {
+    // A failing before/after hook fails the scenario even when `steps` is empty
+    // (a Before-hook failure prevents steps from running), so it must be checked
+    // — otherwise a hook-failed scenario falls through to "passed".
+    for h in el.before.iter().chain(el.after.iter()) {
+        if !matches!(h.result.status.as_str(), "" | "passed" | "skipped") {
+            return "failed";
+        }
+    }
     // Any non-pass, non-skip step status (failed, undefined, ambiguous, pending)
     // fails the scenario — an undefined step must not report as passed.
     for s in &el.steps {
@@ -145,6 +171,18 @@ fn scenario_status(el: &Element) -> &'static str {
     "passed"
 }
 
+/// The single source of truth for "did this scenario pass" across BOTH the CI
+/// gate (`scenario_results_by_id`) and the report grid (`id_pass_map`/tally).
+///
+/// A scenario counts as passed ONLY when every step passed — a `skipped` status
+/// (steps skipped after an early bail, or an undefined step) is NOT a pass. The
+/// gate and the grid previously disagreed on this (gate: `== "passed"`, grid:
+/// `!= "failed"`), so the same `report.json` could fail the job yet render green
+/// in the consolidated grid. Route both through here so they can never diverge.
+fn scenario_passed(el: &Element) -> bool {
+    scenario_status(el) == "passed"
+}
+
 fn scenario_duration(el: &Element) -> u64 {
     el.steps.iter().map(|s| s.result.duration).sum()
 }
@@ -247,7 +285,7 @@ pub fn scenario_results_by_id(json_path: &Path) -> std::io::Result<Vec<(String,
     for f in &features {
         for el in &f.elements {
             if let Some(id) = scenario_id(el) {
-                out.push((id, scenario_status(el) == "passed"));
+                out.push((id, scenario_passed(el)));
             }
         }
     }
@@ -699,7 +737,7 @@ fn id_pass_map(json_path: &Path) -> std::collections::HashMap<String, bool> {
     features
         .iter()
         .flat_map(|f| &f.elements)
-        .filter_map(|el| scenario_id(el).map(|id| (id, scenario_status(el) != "failed")))
+        .filter_map(|el| scenario_id(el).map(|id| (id, scenario_passed(el))))
         .collect()
 }
 
@@ -1801,6 +1839,35 @@ mod tests {
         );
     }
 
+    #[test]
+    fn scenario_passed_is_strict_and_shared() {
+        // The unified predicate: only an all-steps-passed scenario counts as
+        // passed. A skipped scenario is NOT a pass — both the CI gate and the
+        // grid go through scenario_passed, so they can't diverge on this.
+        assert!(scenario_passed(&scenario_from(&["passed", "passed"])));
+        assert!(!scenario_passed(&scenario_from(&["passed", "skipped"])));
+        assert!(!scenario_passed(&scenario_from(&["failed"])));
+    }
+
+    #[test]
+    fn before_hook_failure_scores_scenario_failed() {
+        // A failing Before hook leaves steps empty; without checking hooks the
+        // scenario would fall through to "passed". It must score failed.
+        let el: Element = serde_json::from_str(
+            r#"{"name":"s","steps":[],"before":[{"result":{"status":"failed"}}]}"#,
+        )
+        .expect("valid element json");
+        assert_eq!(scenario_status(&el), "failed");
+        assert!(!scenario_passed(&el));
+
+        // A passed Before hook + passed steps is still a pass.
+        let ok: Element = serde_json::from_str(
+            r#"{"name":"s","steps":[{"keyword":"Given ","name":"x","result":{"status":"passed"}}],"before":[{"result":{"status":"passed"}}]}"#,
+        )
+        .expect("valid element json");
+        assert!(scenario_passed(&ok));
+    }
+
     fn write_report(features_json: &str) -> tempfile::NamedTempFile {
         use std::io::Write as _;
         let mut f = tempfile::NamedTempFile::new().expect("temp file");

tests/e2e-cucumber/README.md

@@ -27,19 +27,26 @@ Tests exercise the `rocm` binary as a black box — no imports from the rocm-cli
 tests/e2e-cucumber/
 ├── Cargo.toml                    # crate definition, dependencies
 ├── README.md
+├── expectations.toml             # per-scenario known-bug (xfail) matrix, keyed by @id
 │
 ├── features/                     # .feature files — one per feature area
 │   ├── chat.feature
-│   └── model_serving.feature
+│   ├── examine.feature
+│   ├── model_serving.feature
+│   └── runtime_setup.feature
 │
 ├── tests/                        # test binary + step modules
-│   ├── e2e.rs                    # World struct, runner, Drop cleanup
+│   ├── e2e.rs                    # World struct, runner, expectation reconciliation, Drop cleanup
 │   └── e2e/                      # step functions — one file per feature area
 │       ├── chat_steps.rs
+│       ├── examine_steps.rs
+│       ├── runtime_steps.rs
 │       └── serving_steps.rs
 │
 └── src/                          # shared test infrastructure
     ├── lib.rs
+    ├── capability.rs             # host capability probe (OS / GPU / effective engine)
+    ├── expectation.rs            # tag parsing + pass/xfail/skip resolution
     └── mock_server.rs            # axum mock OpenAI server
 ```
 
@@ -55,9 +62,6 @@ cargo xtask e2e
 # Filter by scenario name:
 cargo xtask e2e -- -n "model name"
 
-# Skip known-bug scenarios:
-cargo xtask e2e -- -t "not @expected-failure-EAI-*"
-
 # Stop on first failure:
 cargo xtask e2e -- --fail-fast
 
@@ -74,24 +78,42 @@ ROCM_CLI_BINARY=./target/release/rocm cargo xtask e2e
 | `ROCM_CLI_DATA_DIR` | (temp dir) | Isolated data directory per scenario |
 | `ROCM_CLI_CACHE_DIR` | (temp dir) | Isolated cache directory per scenario |
 
-## Tags
+## Tags and per-scenario expectations
+
+There is no tag-filter tiering. Each CI job runs the **whole** suite
+(`cargo xtask e2e`, no `-t` filter); the harness resolves every scenario to
+**pass / xfail / skip** at runtime from its capability tags plus the known-bug
+matrix, then reconciles the actual result against that expectation.
 
-Cucumber tag filters match exact tag names (no globbing), so scenarios carry a
-bare category tag for filtering plus an optional specific tag for traceability.
+Scenarios carry stable-id and capability tags:
 
 | Tag | Meaning |
 |---|---|
-| `@gpu` | Requires real AMD GPU / ROCm hardware. Runs only in the non-blocking `e2e-gpu` CI job; excluded from the mock-tier job via `-t "not @gpu"`. |
-| `@expected-failure` | Known bug — excluded from the blocking mock job (`-t "not @expected-failure"`) and run in the non-blocking known-bugs job. |
-| `@expected-failure-EAI-NNNN` | Traceability to the specific tracked bug. Always paired with the bare `@expected-failure`. Remove both when the bug is fixed. |
-
-CI runs three selections:
-
-| Job | Filter | Blocking |
+| `@id:<slug>` | Stable scenario id. Keys the expectation matrix and the report grid; every scenario has one. |
+| `@requires-gpu` | Needs a real AMD GPU. Resolves to **skip** (n/a) on a host with none (e.g. the mock job). |
+| `@requires-engine:<vllm\|lemonade>` | Pins the serve engine. Resolves to skip where that engine can't start (e.g. vLLM on a lemonade-only Strix host). |
+| `@requires-os:<linux\|windows>` | Premise is OS-specific; skip on other OSes. |
+| `@serve-timeout:<secs>` | Lengthen the serve-readiness wait for a genuinely slow serve (e.g. a large model). |
+
+Known bugs are **not** tagged in the `.feature` files — they live in
+`expectations.toml`, keyed by `@id`, each with a `when = { ... }` condition (e.g.
+`effective_engine = "vllm"`), a `bug` reference, and a `reason`. A scenario that
+matches a condition is expected to fail (xfail); if it then passes, that is an
+**XPASS** (stale entry — remove it). See `src/expectation.rs` for the resolver
+and `expectations.toml`'s header for the condition grammar.
+
+CI runs one job per platform, each executing the full suite:
+
+| Job | Platform | Blocking |
 |---|---|---|
-| `e2e` | `not @gpu and not @expected-failure` | yes |
-| `e2e-known-bugs` | `@expected-failure and not @gpu` | no |
-| `e2e-gpu` | `@gpu` | no |
+| `e2e` | Mock (no GPU, GitHub-hosted) | yes |
+| `e2e-gpu` | MI300X (self-hosted) | no |
+| `e2e-gpu-strix-ubuntu` | Strix Halo / Ubuntu (self-hosted) | no |
+| `e2e-gpu-strix-windows` | Strix Halo / Windows (self-hosted) | no |
+
+The blocking mock job passes when every applicable scenario is pass-or-xfail with
+no XPASS or unexpected failure; the GPU jobs are non-blocking. The `e2e-report`
+job consolidates all platforms' results into one cross-platform report.
 
 ## From scenarios to tests
 

tests/e2e-cucumber/expectations.toml

@@ -6,7 +6,7 @@
 # does `serve` default to? does this platform prefer vLLM? — is DERIVED from the
 # host capability probe (see src/capability.rs) and is NOT encoded here.
 #
-# Resolution at runtime (see tests/e2e-cucumber/tests/e2e/expectation.rs):
+# Resolution at runtime (see tests/e2e-cucumber/src/expectation.rs):
 #   1. A scenario whose @requires-gpu / @requires-engine can't be satisfied on
 #      this host is SKIPPED (not-applicable) — never listed here.
 #   2. Otherwise, the FIRST matching condition below marks it expected-fail
@@ -23,6 +23,14 @@
 #
 # An empty `when = {}` means "always xfail" (platform-independent bug).
 
+# --- EAI-7222: privacy notice is only shown in the interactive dash/TUI, so a
+# black-box CLI test can't verify it. Tracked as xfail (any platform) rather than
+# a silent green no-op — the step panics, which is the expected outcome here. ---
+[["chat-privacy-notice-accurate"]]
+when = {}
+bug = "EAI-7222"
+reason = "Privacy notice is TUI-only; cannot be verified by a black-box CLI test."
+
 # --- EAI-7219: short-name expansion not surfaced in serve output (any engine) ---
 [["serve-short-name-expansion"]]
 when = {}