ci: address serve-filter review — canary docs, cardinality test, gate order (EAI-7746)

Follow-up to the serve paths-filter + PR canary change, addressing review
findings on the same PR:

- Document @canary and @serves-on-gpu in the e2e-cucumber README tag table,
  and correct the now-stale "each job runs the whole suite" text and job
  table to reflect PR canary mode (MI300X runs only @canary) and the Strix
  lanes skipping on pull_request.
- Add a unit test that parses the real .feature files and asserts exactly
  one @canary scenario, so a later refactor dropping/renaming the tag fails
  cheaply instead of silently emptying the pre-merge serve smoke.
- Move the canary-mode skip after the host-applicability checks in resolve()
  so a scenario inapplicable for a hard reason (no GPU / wrong OS) reports
  that reason rather than the canary skip.
- Make the merge_group gating comment honest: the Strix lanes are
  continue-on-error on merge_group too, so they are not a gating backstop —
  a regression surfaces at nightly, a deliberate trade for serial hardware.

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

Author: fredespi

.github/workflows/ci.yml

@@ -967,6 +967,15 @@ jobs:
     # can still target it. The job is required-but-continue-on-error, so skipping
     # it on a PR reports the required check as skipped (branch protection
     # satisfied) without running the hardware.
+    #
+    # NOT a gating backstop: this lane is `continue-on-error` on merge_group too,
+    # so a Strix (lemonade / Windows) serve regression skips on the PR, fails
+    # non-blocking in the queue, and still lands on main — first hard signal is
+    # the nightly run, a day later, decoupled from the causing PR. Deliberate
+    # trade for scarce serial Strix hardware (a per-PR real Strix serve is the
+    # cost this change removes); the Strix lanes are proving-out and non-blocking
+    # by design. Drop `continue-on-error` on the merge_group path if/when they
+    # graduate to a true pre-merge gate.
     if: >-
       always()
       && needs.changes.result == 'success'

tests/e2e-cucumber/README.md

@@ -88,10 +88,14 @@ E2E_INCLUDE_LIFECYCLE=1 E2E_ONLY_LIFECYCLE=1 cargo xtask e2e
 
 ## 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.
+There is no tag-filter tiering: a job runs `cargo xtask e2e` with no `-t` filter,
+and 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. Two env-gated modes narrow *which* scenarios actually
+run (still via resolution, not a CLI filter): `E2E_PR_CANARY=1` on the per-PR
+MI300X lane runs only the `@canary` serve (see the job table below), and
+`E2E_INCLUDE_NIGHTLY` / `E2E_INCLUDE_LIFECYCLE` opt expensive scenarios in. Off
+those modes (merge queue, push, nightly, dispatch) a job runs the whole suite.
 
 Scenarios carry stable-id and capability tags:
 
@@ -104,6 +108,8 @@ Scenarios carry stable-id and capability tags:
 | `@serve-timeout:<secs>` | Lengthen the serve-readiness wait for a genuinely slow serve (e.g. a large model). |
 | `@nightly` | Expensive scenario skipped by default; included when `E2E_INCLUDE_NIGHTLY=1`. |
 | `@lifecycle` | Expensive, OS-mutating release-lifecycle scenario (packaging + real installer + install/uninstall). Skipped by default; included when `E2E_INCLUDE_LIFECYCLE=1`. `E2E_ONLY_LIFECYCLE=1` selects only this set without bypassing expectation resolution. |
+| `@canary` | The single minimal real-serve scenario used as the per-PR pre-merge smoke on the MI300X lane. In canary mode (`E2E_PR_CANARY=1`) it is the **only** GPU-serving scenario that runs; every other one resolves to skip. Exactly one scenario must carry this tag (enforced by a unit test). |
+| `@serves-on-gpu` | Marks a scenario that does a **real** `rocm serve` on a GPU host even though it is not `@requires-gpu` (its assertion is engine-agnostic, so it also runs mock-backed on the no-GPU lane). Needed so canary mode skips it too; without the tag it would launch an extra real serve on every PR canary run. |
 
 Known bugs are **not** tagged in the `.feature` files — they live in
 `expectations.toml`, keyed by `@id`, each with a `when = { ... }` condition (e.g.
@@ -122,18 +128,28 @@ serve does not compete with the first for device memory, and the failure quotes
 the service log tail plus the device's free-VRAM state, which is where the
 engine's own reason for the stall is recorded.
 
-CI runs one job per platform, each executing the full suite:
-
-| Job | Platform | Blocking |
-|---|---|---|
-| `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.
+CI runs one job per platform. Because real GPU serves are the wall-clock long
+pole on scarce serial hardware, the heavy GPU lanes are narrowed on a
+`pull_request` (see the `changes` job's `serve` paths-filter and the per-job
+`if:`/`E2E_PR_CANARY` gating in `ci.yml`):
+
+| Job | Platform | On `pull_request` | On merge_group / push / dispatch | Blocking |
+|---|---|---|---|---|
+| `e2e` | Mock (no GPU, GitHub-hosted) | full suite | full suite | yes |
+| `e2e-gpu` | MI300X (self-hosted) | `@canary` scenario only (canary mode) | full suite | no |
+| `e2e-gpu-strix-ubuntu` | Strix Halo / Ubuntu (self-hosted) | skipped | full suite | no |
+| `e2e-gpu-strix-windows` | Strix Halo / Windows (self-hosted) | skipped | full suite | no |
+
+Only serve-affecting PRs run the GPU lanes at all: they are gated on the `serve`
+paths-filter, so a PR that can't touch serving skips the whole matrix. When a GPU
+lane does run on a PR, only the MI300X `@canary` serve executes as a pre-merge
+smoke; the two Strix lanes skip on PRs (their required checks report as skipped,
+satisfying branch protection) and run the full suite in the merge queue and on
+push. 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
+(`continue-on-error`) on every trigger — including merge_group — so they surface
+regressions without gating the queue. The `e2e-report` job consolidates all
+platforms' results into one cross-platform report.
 
 The nightly workflow runs three non-blocking jobs — the existing MI300X job and
 new Strix Halo jobs on Ubuntu and Windows — with `E2E_INCLUDE_NIGHTLY=1`. The

tests/e2e-cucumber/src/expectation.rs

@@ -369,16 +369,6 @@ pub fn resolve(
             reason: "lifecycle-only scenario; set E2E_INCLUDE_LIFECYCLE=1 to run".to_owned(),
         };
     }
-    // In PR canary mode, only the @canary scenario serves; skip every other
-    // scenario that would real-serve on this GPU host — both `@requires-gpu`
-    // scenarios and `@serves-on-gpu` ones (untagged-for-GPU scenarios that still
-    // launch a real serve when a GPU is present, e.g. the chat behavioural
-    // scenarios). Purely non-GPU scenarios are unaffected — they still run.
-    if canary_mode && (decl.requires_gpu || decl.serves_on_gpu) && !decl.canary {
-        return Expectation::Skip {
-            reason: "non-canary GPU-serving scenario; skipped in PR canary mode".to_owned(),
-        };
-    }
     if decl.requires_gpu && !cap.has_amd_gpu {
         return Expectation::Skip {
             reason: "requires an AMD GPU; none detected on this host".to_owned(),
@@ -396,6 +386,18 @@ pub fn resolve(
             reason: format!("requires os '{os}'; this host is '{}'", cap.os_family),
         };
     }
+    // In PR canary mode, only the @canary scenario serves; skip every other
+    // scenario that would real-serve on this GPU host — both `@requires-gpu`
+    // scenarios and `@serves-on-gpu` ones (untagged-for-GPU scenarios that still
+    // launch a real serve when a GPU is present, e.g. the chat behavioural
+    // scenarios). Purely non-GPU scenarios are unaffected — they still run. This
+    // sits AFTER the host-applicability checks (no-GPU / OS) so a scenario that
+    // is inapplicable for a hard reason reports THAT reason, not the canary skip.
+    if canary_mode && (decl.requires_gpu || decl.serves_on_gpu) && !decl.canary {
+        return Expectation::Skip {
+            reason: "non-canary GPU-serving scenario; skipped in PR canary mode".to_owned(),
+        };
+    }
     let engine = decl.effective_engine(cap);
     // A scenario that pins or defaults to a real engine and would actually serve
     // needs that engine to be startable here. We treat any GPU scenario with a
@@ -664,6 +666,51 @@ serve_timeout_secs = 90
         );
     }
 
+    // Guards the canary mechanism against silent decay. `E2E_PR_CANARY` skips
+    // every GPU-serving scenario except the one `@canary`; if a later refactor
+    // drops or renames that tag, the per-PR MI300X lane would skip everything,
+    // still write a valid platform.json, reconcile clean, and go green in ~2min —
+    // silently testing nothing. Parse the real .feature files and assert exactly
+    // one scenario carries `@canary` so that regression fails a cheap unit test
+    // instead of quietly gutting the pre-merge serve smoke.
+    #[test]
+    fn exactly_one_canary_scenario_across_feature_files() {
+        let features_dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("features");
+        let mut canary_ids: Vec<String> = Vec::new();
+        // A scenario's tags are the whitespace-separated `@tag` tokens on the
+        // line(s) immediately preceding its `Scenario:`. We only inspect lines
+        // whose first non-space char is `@` (real tag lines) — `#` comment lines
+        // that merely mention `@canary` in prose are ignored, and the tag line's
+        // own `@id:<slug>` gives us the scenario id for a useful failure message.
+        for entry in std::fs::read_dir(&features_dir)
+            .expect("read features dir")
+            .flatten()
+        {
+            let path = entry.path();
+            if path.extension().and_then(|e| e.to_str()) != Some("feature") {
+                continue;
+            }
+            let text = std::fs::read_to_string(&path).expect("read feature file");
+            for line in text.lines() {
+                let trimmed = line.trim_start();
+                if !trimmed.starts_with('@') {
+                    continue;
+                }
+                let tags: Vec<&str> = trimmed.split_whitespace().collect();
+                let decl = ScenarioDecl::from_tags(&tags);
+                if decl.canary {
+                    canary_ids.push(decl.id.unwrap_or_else(|| format!("<no @id> in {path:?}")));
+                }
+            }
+        }
+        assert_eq!(
+            canary_ids.len(),
+            1,
+            "expected exactly one @canary scenario, found {}: {canary_ids:?}",
+            canary_ids.len(),
+        );
+    }
+
     #[test]
     fn effective_engine_prefers_explicit_pin() {
         let d = decl(&["id:x", "requires-gpu", "requires-engine:vllm"]);