refactor(e2e-report): group the expectation grid by feature and fix scenario indexing

[rominf]

Summary

The consolidated E2E report's expectation grid rendered all 66 scenarios as one
undivided table, sorted alphabetically by scenario id. This splits it into one
sub-table per feature and gives every scenario an index that actually
identifies it.

• Split by feature — the grid (markdown + HTML) now renders one sub-table
  per Feature:, with rows in feature-file order. The Scenario reference
  section follows the same grouping, so the links from the grid land in a
  matching layout.
• Fixed scenario indexing — every feature file numbered its scenarios from
  1, so 1 named eight different scenarios. Some files had drifted further:
  examine ran 1, 2, 5, 3, 4; model_serving had a stray 6b and a 10
  sitting in sixth place; install_lifecycle had no indexes at all. Scenarios
  are now <feature-key>-<NN>, sequential in declaration order.
• Feature-qualified ids — four @ids didn't carry their feature's key
  (help-* in examine.feature, fix-* in diagnose.feature), so the id
  alone didn't say where the scenario lived. Renamed, along with the matching
  expectations.toml key.
• Drift guard — tests/feature_naming.rs enforces the convention (indexes
  sequential per feature and unique suite-wide, ids feature-qualified) in the
  ordinary cargo test run.

Why: the grid is the main artifact for "where should each test pass", and at 66
undivided rows it was hard to scan a single area of the CLI or find a scenario
you cared about.

Risk: low. The report is a CI artifact — no product code is touched. The
scenario renames are internal to the suite; nothing outside it keys on scenario
names, and the four renamed ids were grepped repo-wide.

Non-obvious decisions

• Feature name comes from platform.json, not the id prefix. A scenario
  skipped on every platform never reaches report.json, so the harness records
  each scenario's feature and name alongside its resolved expectation. Deriving
  the feature from the id prefix would re-encode structure in a string and break
  the moment a key is renamed, so it is kept only as the last fallback.
• Fallback chain for older artifacts: platform.json's feature →
  report.json's feature name → the id's leading segment. Both new fields are
  #[serde(default)] on the consuming side (ManifestExpectation), so
  pre-existing artifacts still render rather than dropping rows. (An earlier
  revision of this description said "on both sides" — that was wrong: the
  producer derives Serialize only, where a deserialization attribute is dead.
  The attributes have been removed there.) An artifact that names the feature is treated as
  authoritative and overrides a fallback guess — without that, the first input
  to mention an id fixed its feature permanently and one feature could split
  into two groups.
• Every grid row now gets an anchor in the Scenario reference. A scenario
  that was n/a on every platform previously had a dangling link.

Test plan

• cargo test -p e2e-report -p e2e-cucumber — new unit tests cover grouping,
  numeric-not-lexical index ordering (09 before 10), the no-feature-field
  fallback, the authoritative-override case, and the anchor for a scenario that
  ran nowhere.
• The drift guard was verified to fail on each of the drift shapes that
  existed before this change (restarted numbering, out-of-order index,
  unqualified id) before passing on the fixed files.
• cargo fmt --check and both CI clippy invocations clean.
• End-to-end on real artifacts: ran the mock lane, then
  cargo xtask e2e-report over its output — 66 rows across 8 feature groups in
  the right order, and 66 resolving anchors. Re-ran with the new fields stripped
  from platform.json: all 66 rows still render via the fallback.

Two diagnose-* scenarios fail on my local WSL2 box (rocm diagnose reports
itself out of scope on WSL2, which uses /dev/dxg rather than /dev/kfd);
they are unrelated to this change and pass on the Linux runners.

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows.

[volen-silo]

Reviewed the full diff, the rename blast radius, and the new drift guard. No blocking issues — the risky part is provably complete. Findings below are robustness nits in the guard plus one inaccurate claim in the description. Leaving the formal approve to a maintainer.

Verified

• The four @id renames are complete. Grepped each old id repo-wide (excluding .git/target, filtering out the new qualified forms): zero live occurrences of help-lists-subcommands-alphabetically, fix-lists-known-recipes, fix-dry-run-changes-nothing, fix-unknown-id-rejected. Nothing in expectations.toml, feature files, step defs, crates/e2e-report/**, xtask/**, scripts/**, .github/workflows/**, or any doc/fixture.
• No dead xfail rows. Parsed the tree directly: 11 unique expectations.toml keys, 66 unique @ids, and every key resolves to a live scenario id. (chat-tool-definitions-accepted and serve-readiness-contract appearing twice are array-of-tables OR-conditions, not duplicates.)
• Merge-order hazard is moot — #182 merged 2026-08-05, and it only appended flaky = true to rows on lines disjoint from the renamed key. git merge-tree --write-tree against current main is clean, and enumerating keys vs ids on the resulting merged tree still yields zero orphans. #174 shares no files.
• Ordering is numeric, not lexical. scenario_index parses to u32 and the sort is (index.is_none(), index, id), so None sorts last and chat-09 precedes chat-10; ≥100 follows from u32. Malformed names return None via ? rather than panicking.
• No anchor collisions — grid rows come from a BTreeMap keyed by id, so one id → one row → one heading. Uniqueness is triple-enforced (the map, the new duplicate scenario @id assert at tests/e2e-cucumber/tests/e2e.rs:844-853, and the guard's suite-wide check). That new assert is a good defensive addition: a copy-pasted scenario with a forgotten id now fails loudly instead of silently overwriting a resolution the grid keys on.
• scripts/xfail_expectations_hint.py is rename-agnostic — it derives ids from whatever keys are in expectations.toml at runtime and hardcodes nothing.
• Locally: cargo fmt --all --check, cargo test -p e2e-report -p e2e-cucumber (98 passed), cargo test -p e2e-cucumber --test feature_naming (4/4), both of CI's clippy invocations, and cargo xtask manifest --check — all pass. 21/21 CI checks green.

---

1. The drift guard passes vacuously if a feature file yields zero parseable scenarios

tests/e2e-cucumber/tests/feature_naming.rs:101-153

All three enforcement tests are for … in scenarios_of(file) loops, so an empty Vec means zero loop bodies and a green test. feature_files_and_declared_keys_agree (:80-98) is the only file-level test, and it asserts only that each FEATURE_KEYS entry has a file and vice versa — never that a file contains any scenarios. There's no minimum-count assertion anywhere in the file.

The parser is strict enough to make this reachable: :70 requires the exact literal "Scenario: ", so Scenario:Foo, Scenario: Foo, or a keyword typo silently yields nothing for that file. Concrete scenario: a bad bulk find-replace — exactly the class of change this PR is — mangles Scenario: in one feature file. That file's scenarios vanish from the guard's view, the guard stays green, and the grid then renders that feature's rows unsorted (all index == None, sorted last) with nobody warned. Reproduced in a throwaway copy: renaming all seven Scenario: keywords in chat.feature to Situation: still gives 4 passed; 0 failed.

One line in feature_files_and_declared_keys_agree closes it:

assert!(!scenarios_of(file).is_empty(), "{file}: no scenarios parsed — the naming checks would pass vacuously");

2. The guard's tag parser diverges from the harness's — @id: must be first on its line

tests/e2e-cucumber/tests/feature_naming.rs:63-68

if let Some(rest) = line.strip_prefix('@') {
    for tag in rest.split_whitespace() {
        if let Some(id) = tag.strip_prefix("id:") {

Only the line's leading @ is stripped, so on a multi-tag line every token after the first keeps its own @ and "@id:examine-version".strip_prefix("id:") returns None. The production parser does it per-tag and correctly — tests/e2e-cucumber/src/expectation.rs:96-101:

let tag = tag.as_ref().strip_prefix('@').unwrap_or_else(|| tag.as_ref());
if let Some(rest) = tag.strip_prefix(ID_PREFIX) {

So a contributor writing @requires-os:linux @id:examine-version — valid Gherkin, resolved correctly by ScenarioDecl::from_tags — gets a CI failure from feature_naming reading scenario "…" has no @id: tag. Latent today because every @id: in the corpus happens to sit first on its line. Fix: tag.strip_prefix('@').unwrap_or(tag).strip_prefix("id:").

3. #[serde(default)] on the producer side is a no-op; the backward-compat claim is half wrong

tests/e2e-cucumber/src/expectation.rs:267, 273, 278

ResolvedScenario derives #[derive(Debug, Clone, serde::Serialize)] — Serialize only. #[serde(default)] is a deserialization attribute, so on :273 and :278 it does nothing, and nothing in the repo deserializes ResolvedScenario anyway.

The description says "Both new fields are #[serde(default)] on both sides, so pre-existing artifacts still render rather than dropping rows." The backward compatibility is real, but it comes entirely from the consumer: ManifestExpectation at crates/e2e-report/src/lib.rs:609-619 derives Deserialize with #[serde(default)] on both fields and no deny_unknown_fields. The producer attribute contributes nothing. Worth noting this PR is copying an existing no-op — flaky at expectation.rs:287 has the same dead attribute. Harmless, but both the attribute and the claim are misleading; drop one or reword the other.

4. Stale comment introduced by this PR

crates/e2e-report/src/lib.rs:1450-1451 — the inline comment still says GitHub anchors #### <id>, but this PR moved those headings to ##### {id} (now :1543) so they nest under the new #### {feature} group headings. The function doc at :1505-1508 was updated correctly; this comment wasn't. Comment rot in the exact code the PR touches.

5. Feature/name merge is write-order-dependent between two named artifacts, and the display name has no override at all

crates/e2e-report/src/lib.rs:832-848

The "authoritative overrides a guess" rule is inferred from exp.feature.is_empty() on the incoming expectation rather than tracked with a flag. Consequences:

• Guess vs. name is correctly order-independent — the named value always wins, and the new test exercises both orders. That's the case the PR set out to fix, and it's fixed.
• Name vs. name is last-write-wins, silently. Two artifacts naming different non-empty feature values for one id resolve by read order. Only reachable when mixing artifact vintages across a Feature: rename, and no test covers it.
• A feature can still split across vintages — the fallback at :790-792 yields the lowercase id prefix ("serve") while the authoritative value is the full title ("Model serving"), so in a mixed-vintage run some ids group under each. This looks like an accepted best-effort limit and grid_falls_back_to_report_feature_then_id_prefix documents the shape.
• entry.1, the display name (:845-847), has no override — first non-empty wins permanently, which is precisely the asymmetry feature was just fixed out of. That name is what scenario_index (:769) parses the sort index from, so a stale name from an older artifact would pin a stale sort position. Same mixed-vintage precondition. Worth at least a comment on why feature gets an override and scenario doesn't.

6. The README duplicates the FEATURE_KEYS list, unguarded

tests/e2e-cucumber/README.md:98-99 hardcodes all eight keys inline. The guard forces FEATURE_KEYS to stay in sync with the filesystem, but nothing keeps the README copy in sync with FEATURE_KEYS — adding a ninth feature file fails the guard until FEATURE_KEYS is updated, and the README then silently goes wrong. :111 already points at the real list; consider dropping the enumeration and keeping just the lifecycle-vs-install_lifecycle example, which is the one non-obvious case since the key isn't the filename.

7. Pre-existing, not introduced here

crates/e2e-report/src/lib.rs:1518 — scenario_reference_markdown bails on scenarios.is_empty() || grid.is_empty() while expectation_grid_markdown gates only on grid.is_empty(), so if platform.json sidecars exist but no report.json recorded a scenario, every grid link dangles. Confirmed against ad12ac7 that this guard is unchanged — and this PR fixes the far more common dangling case, since the reference now iterates grid.groups rather than scenarios. Noted only so the "every grid row gets an anchor" claim is understood to hold except in that degenerate corner.

---

Not verified: a real cargo xtask e2e-report over genuine CI artifacts (none available here — the unit tests cover the same logic on synthetic fixtures and pass), the full e2e suite (needs a built binary and Linux runner conditions; CI's four E2E jobs are green and authoritative), and rendered-output inspection in GitHub's markdown renderer — anchor resolution is inferred from documented heading-slug behaviour plus the unit tests' string assertions, not observed in a browser.

Heads-up: the branch is currently BEHIND main and will want an update-branch before merge.

[rominf]

Thanks — this was a genuinely useful review. I reproduced findings 1 and 2 before fixing them, and both behaved exactly as you described. All six actionable items are addressed in 7c03df5, and the branch is now rebased onto current main.

1 — vacuous guard. Confirmed: renaming the seven Scenario: keywords in chat.feature to Situation: still gave 4 passed; 0 failed. Added the non-empty assertion to feature_files_and_declared_keys_agree; the same mutation now fails with chat.feature: no scenarios parsed — the naming checks would pass vacuously.

2 — tag parser divergence. Confirmed: @requires-os:linux @id:examine-version failed with has no @id: tag. Now strips @ per tag rather than off the head of the line, matching ScenarioDecl::from_tags; the same input passes. Good catch — this would have bitten the first contributor to order their tags differently.

3 — dead #[serde(default)]. Correct, ResolvedScenario derives Serialize only. Removed both attributes I added and replaced them with a note saying where the compatibility actually lives. Also fixed the description, which now says "on the consuming side" and flags the earlier wording as wrong. I left the pre-existing one on flaky alone to keep this PR's scope honest — happy to fold it in if you'd rather it went with the others.

4 — stale comment. Fixed, #### <id> → ##### <id>. My own rot, in code this PR moved.

5 — asymmetric override. Applied the same last-non-empty-wins rule to the display name, but one part of the rationale doesn't hold and I want to flag it rather than quietly bank the finding: the previous entry.1.is_empty() check already meant first non-empty wins, so an empty value never stuck and a nameless older artifact could not pin a stale sort position. My first attempt at a regression test passed against the pre-fix logic, which is what surfaced this. The only case the two rules actually differ on is two artifacts naming the same id differently — a Scenario: renamed between vintages — so that is what the new test pins, and it does fail on the old logic. The change is still worth having (the rule is now stated rather than falling out of iteration order), just for a narrower reason than stated.

6 — README duplication. Dropped the enumeration; it now points at FEATURE_KEYS and keeps only the non-obvious install_lifecycle → lifecycle / model_serving → serve examples.

7 — pre-existing corner. Agreed, and thanks for checking it against ad12ac7 rather than attributing it here. The "every grid row gets an anchor" claim should be read as holding whenever any scenario was recorded at all; if sidecars exist but no report.json recorded anything, the reference bails and every link dangles. Out of scope for this PR, but worth its own issue — say the word and I'll open one.

Rebase. Onto ec2bcb3, clean, no conflicts. expectations.toml was the only file both sides touched and the changes are disjoint, as you predicted; I re-verified afterwards that all 11 keys still resolve to live ids (zero orphans) and that no feature files changed upstream. Local gate green: fmt, both clippy invocations, and all six test suites.

[rominf]

CI status after the rebase

20/21 green. Two jobs went red on the first post-rebase run; I chased both rather than waving them through.

E2E tests (Strix Halo, Windows) — flake, now green on re-run. It failed with two "regressions", chat-tool-definitions-accepted and chat-end-to-end-local-model, both on:

no non-empty choices array in response:
{"error":{"message":"No model loaded: Qwen3-0.6B-GGUF","type":"model_not_loaded"}}

That is a lemonade runtime failure — nothing a scenario-renaming and report-grouping change can reach. Notably these are the same two ids already declared flaky = true under EAI-7423 in expectations.toml, whose recorded reason is "Lemonade managed serve reaches ready then shuts down immediately, so the chat request never reaches a live model" — exactly the observed symptom. The existing rows are scoped os = "linux", so on Windows the matrix expects a pass and reported them as regressions. Re-ran the job and it passed, which confirms nondeterminism rather than my asserting it.

Worth a maintainer's judgement as separate work, not something I want to fold into this PR: we now have evidence EAI-7423 reaches the Windows lane too, so the os = "linux" scoping on those two rows may be too narrow. I have deliberately not widened it here — one observation is thin evidence, and loosening a gate is not a change to make in passing inside an unrelated refactor.

E2E tests (GPU) — broken runner, needs a maintainer. Failed twice in 0–1s with zero steps executed, on github-runner-0:

System.ArgumentNullException: Value cannot be null. (Parameter 'configuredSettings')
   at GitHub.Runner.Common.ConfigurationStore.GetSettings()
   at GitHub.Runner.Worker.JobRunner.RunAsync(...)

This is the runner agent failing to read its own configuration before any workflow step runs — not a test failure and not reachable from this diff. The same job passed on this branch pre-rebase in 7m54s on github-runner-1, so the MI300X lane itself is fine; github-runner-0 appears to have a corrupt config. gh run rerun re-queued it onto the same runner and it failed identically. I do not have access to fix the runner — could someone with runner admin either repair github-runner-0 or re-run once it is off that host? Happy to re-trigger from my side once it is healthy.