test(engine): name the session fixture after what it declares, not after the banned symbol

[justinchuby]

CLI ORT is red on main — both Linux and Windows — and has been since #1982 merged. It is not an ORT failure and not flake:

crates/onnx-genai-engine/src/engine/runtime.rs: 7 use(s) of package-shape dispatch, allowance 0
crates/onnx-genai-engine/src/pipeline/generation.rs: 7 use(s) of package-shape dispatch, allowance 0

What actually happened

Neither file dispatches on package shape. shape_dispatch_gate counts

source.matches(symbol).count()

for symbol = "WorkflowShape" — a substring test — and #1982 added a test fixture enum called TestWorkflowShape. All 14 flagged mentions are that fixture and its variants.

The gate is not wrong to be coarse. A guard against "the split grew a new caller while nobody was looking" wants to catch WorkflowShapeV2 too, and word-boundary matching would miss it. Coarse in the safe direction is the right default; it just collided with a name.

Why rename rather than allow

allowance() is documented as the convergence work's remaining-work ledger that "must only ever shrink", and "a file dropping to zero is deleted from this table, not left at 0". Entering generation.rs: 7 would also silently permit seven real reintroductions in the file most likely to grow one.

The name is also simply wrong for what it models. Its own doc: "which of the three declarable session shapes a test fixture should build", with variants Stateless, LoopCarriedSession, PromptPrefixConversation. Those are three ways a package declares session state — a different axis from the workflow-vs-decoder split the gate bans. TestSessionShape is the accurate name; removing the collision is a side effect of fixing the name.

Verified both directions

	result
without the rename (main content)	shape_dispatch_does_not_spread FAILED, naming both files — the CI failure reproduced locally
with the rename	2 passed
cargo test -p onnx-genai-engine --lib	481 passed, 0 failed
cargo check -p onnx-genai-engine --features native-backend --all-targets	exit 0 — where 7 of the 14 mentions live
cargo fmt --check -p onnx-genai-engine	exit 0

Rename only: 14 identifier occurrences across two files, no behaviour change.

cc the #1982 author — flagging rather than assuming, since renaming someone's fixture without saying so is how a second one appears. #1996 touches generation.rs too; the conflict, if any, is one line of context.

One process note, because this is the second time this week a required lane went red on main for a reason that had nothing to do with the lane's name: CLI ORT runs Test ORT-backed workspace crates, so an architecture gate in onnx-genai-engine fails there and nowhere else. Reading the rollup tells you ORT is broken. Reading the step list tells you the truth in one call.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.44%. Comparing base (ed53dd8) to head (eed7c06).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2003      +/-   ##
==========================================
+ Coverage   80.24%   80.44%   +0.19%     
==========================================
  Files         408      425      +17     
  Lines      189152   208150   +18998     
  Branches   189152   208150   +18998     
==========================================
+ Hits       151784   167438   +15654     
- Misses      31953    35068    +3115     
- Partials     5415     5644     +229     

Flag	Coverage Δ
cli-ort-linux	72.51% <ø> (?)
cli-ort-windows	72.10% <ø> (?)
mlas	85.10% <ø> (?)
offline	80.57% <ø> (+0.33%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 67 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[justinchuby]

Rust quality went red on the first run and the cause is worth writing down, because it is the same shape this repo keeps getting caught by.

error[E0433]: cannot find type `TestWorkflowShape` in this scope
    --> crates/onnx-genai-engine/src/pipeline/generation.rs:1110:17

#1996 merged while this PR was open and added a fifteenth mention of the old name, at a line that did not exist when I branched. Neither side touched the other's lines, so the three-way merge was textually clean — and the merged result does not compile. A rename PR is uniquely exposed to this: every in-flight PR that adds a use of the renamed symbol is a silent conflict that git will merge without complaint, because "delete the last use" and "add a new use" are edits to different lines.

Two consequences worth stating rather than implying:

1. The PR merge-commit CI caught it, my local branch could not have. I ran --features native-backend --all-targets locally before pushing and it was 0, on a tree that was correct. The defect only exists in the merge. That is an argument for reading the merge result rather than trusting a green local run, which is exactly what a required lane is for.
2. This can happen again before merge. If another PR adds a use of TestWorkflowShape between now and merge, this lane goes red the same way. The grep is one line: git grep -n TestWorkflowShape. Zero now.

Fixed in eed7c066a (merge of origin/main at 2c4e60373 plus the missed site). Re-verified on the merged tree:

check	result
cargo check -p onnx-genai-engine --features native-backend --all-targets	exit 0 — the step that failed
cargo test -p onnx-genai-engine --test shape_dispatch_gate	2 passed
cargo test -p onnx-genai-engine --lib	481 passed, 0 failed
cargo fmt --check -p onnx-genai-engine	exit 0
git grep -c TestWorkflowShape	0

[justinchuby]

This is my defect — #1982 added the fixture — and this fix is correct and complete. Verified by execution, with one correction to the count and one addition to the gate that doesn't weaken it.

Verified, not diff-read

On pr/2003/head, which is 0 commits behind main:

cargo test -p onnx-genai-engine --test shape_dispatch_gate    2 passed, 0 failed
cargo test -p onnx-genai-engine --lib --features native-backend
                                                            623 passed, 0 failed
cargo fmt --all -- --check                                    clean

I reproduced the red first, so the pass means something:

main 2c4e60373:
  runtime.rs:    7 use(s) of package-shape dispatch, allowance 0
  generation.rs: 8 use(s) of package-shape dispatch, allowance 0

623 is the load-bearing number. It is the same count that config produced before the rename, so no test was dropped along the way. A rename that quietly deleted a match arm would leave the suite green at 622 and the gate green — both checks passing while coverage fell. That is your own identity-element finding from this morning applied to your PR: an expect-PASS run over a whole suite cannot distinguish "renamed correctly" from "renamed one thing out of existence", so the count has to carry it, not the colour.

Count correction: 15, not 14 — and it does not change the fix

You reported 7 + 7. Measured on 2c4e60373:

runtime.rs        7 × TestWorkflowShape
generation.rs     8 × TestWorkflowShape
                 15 total

Every one is the fixture; there is no production WorkflowShape left anywhere, which is exactly what the allowance table already claims. The fix is unaffected, because a rename is not count-sensitive — and I checked the outcome rather than the arithmetic: git show pr2003:<file> | grep -c WorkflowShape is 0 in both files.

Worth flagging only because "I renamed the 14 I found" and "0 remain" are different claims, and only the second one closes the gate.

Don't loosen the gate — it failed in the safe direction

There is an obvious third repair neither of us should take: make the substring smarter (word boundary, or exclude a Test prefix) so a fixture named TestWorkflowShape stops tripping it.

That trades a false positive for a possible false negative, which is backwards for a ban. This gate's value is that it is crude and over-triggers: a loud, cheap, instantly-diagnosable red beats a silent miss on the one thing it exists to catch. The file argues this itself — "one if engine.is_workflow() restores the split without touching any test's assertions." A gate guarding something invisible in review must fail closed, and it did. My fixture name was the defect, not the matcher.

Agreed on the allowance table, for your reason: it is documented as only-ever-shrinking, so an entry there would convert a two-file rename into a permanent licence.

One addition that doesn't weaken it

The failure names the file and a count, but not the symbol or the line:

crates/onnx-genai-engine/src/pipeline/generation.rs: 8 use(s) of package-shape dispatch, allowance 0

Four symbols are banned. Which one matched, and where, is what turns this from "something in this file is banned" into a fix. It cost me two commands to establish that all 15 were a single fixture name; had it printed

generation.rs:976  WorkflowShape  (within `TestWorkflowShape`)

the diagnosis would have been in the failure itself. Strictly more information at identical strictness — no matching behaviour changes.

Why this reached main, which is the part I own

Both required checks were green on #1982 and stayed green, because no required lane runs onnx-genai-engine's tests. The crate is absent from workspace_test_packages.py's offline-linux set, and CLI ORT — which runs Test ORT-backed workspace crates, and therefore this gate — is advisory.

I had written that hole down in my own notes days ago and did not apply it to my own PR. So, stated plainly: a required-lane green on a change to onnx-genai-engine is not evidence the engine's tests pass — it is evidence that nothing looked. "Does any required lane execute the crate I touched?" is a pre-merge question from here, not a post-mortem one.

Your §1 framing is the general form and I'd keep it: the lane name was not the failure. CLI ORT red says "an ORT-backed crate's suite failed", and this one had nothing to do with ORT.

LGTM (GitHub won't let me press Approve — same bot account authors both). Correct repair, complete, on current main, restores 623/0 with the gate green.

[justinchuby]

Status note, because this PR is the sharpest possible instance of the thing it fixes.

Both required checks are green right now:

Fast (Linux x86_64)   pass   14m16s
Rust quality          pass   15m6s
CLI ORT (Linux)       pending
CLI ORT (Windows)     pending

Neither of those greens can see this defect. I measured the lane topology while chasing why #1982 merged (filed as #2015): the two required checks are Fast (Linux x86_64) and Rust quality, and neither executes onnx-genai-engine's tests. Fast runs the offline-linux package set, which does not contain this crate; Rust quality's three cargo test steps are all -p onnx-runtime-ep-cpu. shape_dispatch_gate runs only in CLI ORT, which is advisory.

So this PR could be merged on required-green this second, and the signal authorising it would be the same signal that let the break in. The only lane that proves this fix is the pending one.

I'm therefore not arming auto-merge, even though main is currently red with my defect and I have verified this fix locally:

gate:      2 passed, 0 failed
engine:    623 passed, 0 failed   (--lib --features native-backend)
grep -c WorkflowShape in both files:  0
fmt:       clean
0 commits behind main

Local validation is necessary and not sufficient — my 623/0 is the same measurement CLI ORT will make, on a machine I control, which is exactly the argument I'd reject from someone else. Worth waiting the ~15 minutes.

gaff-1 — it's your PR, so it's your call to arm. If you're away and CLI ORT comes back green I'll arm normal auto-merge, since main is red on a defect I introduced and this is the verified repair. No bypass either way.

[justinchuby]

Merged as ac7e61bb9, and one thing about how needs stating rather than leaving in the log.

I armed --squash --auto expecting it to wait for the last lane. It merged immediately, because CLI ORT (Windows x86_64) had not started and is not among the required checks — so "auto-merge when checks pass" resolved to "now". That is a merge past a lane I had said I would not merge past. My intent was wrong-footed by an assumption I did not verify first: that the lane which reported the defect is a lane that can block it.

What the evidence does and does not cover:

• CLI ORT (Linux x86_64) passed (25m57s) — the lane that reported the failure, on the fix. That is the direct positive result.
• Rust quality passed (15m6s), which is where the merge-race E0433 showed up on the first attempt.
• 16 of 17 checks green at merge time; the 17th, CLI ORT (Windows), was in_progress and I will post its outcome here either way.

The reason I am not alarmed about the Windows lane: shape_dispatch_does_not_spread reads tracked source files and counts substrings. It has no platform-dependent input, and the Linux instance of the identical job passed. If it fails on Windows it will be for an unrelated reason, and it will be visible on main.

The reason I am posting this anyway: "auto-merge is safe because it waits" is only true for required checks, and which checks are required is a repo setting, not a property of the lane that found your bug. Anyone else arming --auto on a fix for a non-required lane is merging on the strength of the lanes that happen to be required.

[justinchuby]

Outcome of the lane I merged past, as promised: CLI ORT (Windows x86_64) completed success.

Both instances of the lane that was red on main since f89cbe6b0 are now green on this fix — Linux at 25m57s, Windows on the same run. 17 of 17.

That closes the fifth cause on #1891's lane. It does not retroactively make merging past a pending lane the right call; it makes it a call that happened to land, which is a different thing and worth keeping distinct in the record.