Skip to content

Pre-push gate: run selected classes by full name, RED on zero tests, STOP on a whole-suite selection (#5868) - #5870

Merged
erikdarlingdata merged 2 commits into
devfrom
fix/5868-gate-zero-total-red
Oct 15, 2026
Merged

erikdarlingdata merged 2 commits into
devfrom
fix/5868-gate-zero-total-red

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 15, 2026 •

Copy link
Copy Markdown
Owner

Claude posting for Erik Darling.

Fixes #5868.

Why: the cause, with evidence

The issue's guess (a filter past the command-line limit) is not the cause. The test map and the gate's symbol scan both name a class by its simple name (AgAlertEvaluatorTests), and the gate passed that name straight to -class. The xunit v3 runner's -class filter matches only the full name (Lite.Tests.AgAlertEvaluatorTests). For anything else it runs nothing, prints Total: 0 and exits 0, and the gate counted exit 0 as a pass. CI never had this problem: run-darling-pg-shard.ps1 resolves the same simple names against the runner's own -list classes/json.

Evidence (no suite run):

  • Direct, on a Debug build of Lite.Tests: -class AgAlertEvaluatorTests gives exit 0; Total: 0. -class Lite.Tests.AgAlertEvaluatorTests gives exit 0; Total: 21, Errors: 0, Failed: 0.
  • Reproduction: I cherry-picked the reported commit a5b6360 (the one [Collection] change) onto a scratch branch and ran the old gate's --dry-run. It gave map: darling SELECTED 2324, lite SELECTED 616 and classes: darling 2324, lite 616, the same as the report. Every listed name was a simple name (one of them was 53).
  • Length was never the problem. On that selection, the old batches of 150 names were at most 6,226 characters (Darling) and 5,381 (Lite), far below the 32,767 limit.
  • So the map-selected classes never ran at any selection size. On the smaller selection the issue mentions, the real totals came from the symbol-set classes: the awk scan gives those full names. In a batch that mixed the two kinds of name, only the full names ran.

The same dry run shows a second problem. Today the nightly map selects close to the whole suite for any change: 2,324 to 2,325 Darling classes and 614 Lite classes. That held for each of this PR's three files given to map-select alone, including the SKILL.md docs file. The reasons were 1,562 "covers a changed file", 374 "new class" and 249 "text pattern". So for now the gate gives GATE STOP on nearly every diff. See "For the coordinator".

What changes (by brief item)

Commit aa38d9c, plus a merge of origin/dev (aa4b889). Three files: .github/scripts/pre-push.sh, .github/scripts/tests/pre-push-test.sh (new) and .claude/skills/lane-orders/SKILL.md.

  1. Reproduction and dry run. --dry-run now prints each batch's class count and its character length: [classes-darling] batch 1 of 4: 564 classes, 29956 characters of -class arguments. It resolves names against an existing Debug build. Without a build it says the names are not resolved. A new PRE_PUSH_SELECTION=<selection.json> replays a given selection instead of fetching the nightly map. Both the reproduction and the tests use it.

  2. Fix.

    • Name resolution (the root cause). Each selected name, from the map or from the symbol scan, is matched against the build's own -list classes/json by its simple name, split on [.+/] the way test-map.py, ci-select.py and run-darling-pg-shard.ps1 split it. The runner gets full names, nested ones included (Outer+Inner). A name with no test class in the listing is counted on an unlisted: line and not run. These are helper classes, collection definitions and the stray 53. A failed class listing makes the gate RED.
    • Zero total is RED. Any run whose last Total: is 0 or missing makes the gate RED, with one line that names the batch and its class count: [classes-darling-1] batch 1 of 1 (2 classes) ran 0 tests: the filter matched nothing. The Guard stage is covered too.
    • Whole suite is a STOP. When one project's selection is over PRE_PUSH_MAX_CLASSES (default 400, the brief's suggested cap), the gate runs no selected class. It ends with GATE STOP: selection is the whole suite (darling N, lite M): run the brief's classes; CI decides and exit 3. It is never GREEN. The Guard stage still runs first, and a red there wins (GATE RED, exit 1).
    • Batches cut by length. Batches are cut by the length of their -class arguments, not by a count of 150. The budget is PRE_PUSH_ARG_BUDGET, default 30,000, the same as CI's shard script. On the reproduction that gives 4 Darling batches of at most 29,980 characters and 1 Lite batch of 28,033.
  3. Tests. The repo had no script-test harness, so I added .github/scripts/tests/pre-push-test.sh. It copies the gate into a throwaway git repository and puts a fake dotnet first on PATH. The fake lists the classes it is given, prints the Total it is told to, and records every call. Five cases:

    • zero: every batch reports Total 0 → GATE RED, exit 1, and the batch line;
    • green: the control, which also checks that the full names (nested included) reach the runner and that a helper class is counted as unlisted;
    • guard0: a Guard total of 0 → RED;
    • stop: 401 classes → the exact GATE STOP line, exit 3, no -class run, and the Guard stage still ran;
    • chunks: under a 2,000-character budget, 401 classes run in 7 batches, the longest 1,984 characters, each class once.

    CI does not run the file. These are behavior tests, not source pins.

  4. Docs. In the lane-orders skill's gate paragraph, I added one sentence for the ran 0 tests line (→ GATE RED) and one for the GATE STOP line (exit 3, over 400 classes in one project). Brief-common's copy of the gate rules lives outside the repo and is unchanged.

Test plan

  • bash .github/scripts/tests/pre-push-test.sh: 15 PASS, pre-push-test: all passed, both before and after the merge of origin/dev.

  • The tests catch a reverted fix. I ran them against three mutant copies of the gate (PRE_PUSH_TEST_GATE):

    • zero check removed: 4 FAILED;
    • STOP removed: 3 FAILED;
    • the simple name passed instead of the full name: 2 FAILED.
  • The gate itself on this branch (bash .github/scripts/pre-push.sh), result lines:

    • build: both test projects built (Debug)
    • [guard-darling] Darling.Tests Total: 4181, Errors: 0, Failed: 0, Skipped: 0, Not Run: 0
    • [guard-lite] Lite.Tests Total: 1134, Errors: 0, Failed: 0, Skipped: 0, Not Run: 0
    • classes: darling 1955, lite 474 (selected names: darling 2325, lite 614; symbols: 0 candidates)
    • unlisted: darling 370, lite 140 ...
    • GATE STOP: selection is the whole suite (darling 1955, lite 474): run the brief's classes; CI decides (exit 3)

    The brief expected GATE GREEN for this diff. The map selects the whole suite for this diff, as it did for Lite tests that count log lines no longer lose lines to a parallel class (#5863) #5865, so STOP is the correct verdict now. The brief's classes are the script tests above, and CI decides the rest.

  • The real gate on a small replayed selection (PRE_PUSH_SELECTION: 2 Darling classes and 2 Lite classes, real build and runner):

    • [classes-darling-1] Darling.Tests Total: 22, Errors: 0, Failed: 0, Skipped: 1
    • [classes-lite-1] Lite.Tests Total: 31, Errors: 0, Failed: 0
    • GATE GREEN: guard stage of both suites and 2+2 selected classes in 2 batches passed
  • The reproduction selection replayed through the fixed --dry-run: classes: darling 1954, lite 476, 4+1 batches under 30,000 characters, then GATE STOP (exit 3).

  • FleetIdentifierScrubTests reads the tracked SKILL.md and the new script: Total: 3, Errors: 0, Failed: 0.

  • Full suite: not run (laptop rule). CI decides.

For the coordinator

  • The map selects close to the whole suite today. For any diff on this base (456b1a3, map artifact map-38734757244), even a lone docs file, it selects about 2,325 + 614 classes. So every lane's gate will now say GATE STOP instead of the old false GATE GREEN. CI's own selection uses the same map, so CI runs about the whole suite on these PRs too. That looks like map staleness against a fast-moving dev ("covers a changed file" ×1,562, "new class" ×374). It is in ci-select.py's map logic, outside this lane, and I did not file an issue. It is your call whether it needs one.
  • Please check the STOP policy wording in the SKILL.md sentence: "push on its green Guard lines plus your brief's own classes, and CI decides the rest". That follows the brief's STOP line, but it is a rule change for lanes.
  • I left a local scratch branch, scratch/5868-repro (the cherry-picked a5b6360, used for the reproduction), in the shared repo. It was never pushed. Delete it with git branch -D scratch/5868-repro. My rules did not allow me to run branch -D.

Not done

Nothing from the brief's four items. The one difference from the brief: the gate on this diff ends in GATE STOP, not GATE GREEN, for the reason above.

SECTION: Fixed
ENTRY: - The pre-push gate no longer reports GATE GREEN when its selected test classes ran nothing ([#5870]) - developer tooling: the gate passed simple class names to the test runner, which matches only full names, so every map-selected class ran zero tests and the gate still passed; it now resolves names against the build's class listing, fails any run that reports zero tests, cuts batches by command-line length, and stops with GATE STOP when the selection is the whole suite.

…0, STOP on a whole-suite selection (#5868)

The test map and the symbol scan name classes by their simple name, and the
runner's -class filter matches only a full name: every map-selected batch
matched nothing, printed Total: 0, exited 0, and the gate printed GATE GREEN.
The gate now resolves each name against the build's own -list classes/json
(keyed as CI's shard scripts key it), cuts batches by argument length, turns
any run that reports Total: 0 into GATE RED, and stops with GATE STOP (exit 3)
when one project's selection is over 400 classes. Adds a behavior test with a
fake dotnet (.github/scripts/tests/pre-push-test.sh) and documents the two new
lines in the lane-orders skill.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: LGTM

Claims

  • VERIFIED — "Each selected name... is matched against the build's own -list classes/json by its simple name, split on [.+/] the way test-map.py, ci-select.py... split it" — pre-push.sh:166-199 implements the resolve step; ci-select.py:363 (re.split(r"[.+/]", cls)[-1]) and test-map.py:95 (re.split(r"[.+/]", full)[-1]) use the identical regex.
  • VERIFIED — "A failed class listing makes the gate RED" — pre-push.sh:190-193 (red+=(...) on list_classes failure outside --dry-run).
  • VERIFIED — "Any run whose last Total: is 0 or missing makes the gate RED... The Guard stage is covered too" — pre-push.sh:51-57, exercised for both the Guard calls (pre-push.sh:73-74) and the class batches (pre-push.sh:236).
  • VERIFIED — "Whole suite is a STOP... never GREEN... the Guard stage still runs first, and a red there wins" — pre-push.sh:211-213 and the final exit sequence at pre-push.sh:249-256 (red checked before stop).
  • VERIFIED — "Batches cut by length... budget PRE_PUSH_ARG_BUDGET, default 30,000, the same as CI's shard script" — pre-push.sh:30,219-223 matches the cost formula and $argBudget = 30000 in run-darling-pg-shard.ps1:120,125.
  • VERIFIED — "CI does not run the file" — no .github/workflows/*.yml references pre-push.sh or pre-push-test.sh.
  • UNVERIFIED — "The xunit v3 runner's -class filter matches only the full name... prints Total: 0 and exits 0" — this is live runner behavior; this review ran on a read-only tree with no dotnet build available, so the direct-run evidence in the PR body could not be reproduced here.
  • UNVERIFIED — the test-plan's concrete run outputs (pre-push-test: all passed, the three mutant-copy failure counts, the real GATE STOP/GATE GREEN runs with specific class counts) — all require executing pre-push-test.sh and/or a real Debug build against live nightly-map data; this sandbox's safety hook blocks declare -A (used throughout the script and its tests), so the script could not be run here to confirm.

Riskiest lines

  1. pre-push.sh:195-199 — the awk step that resolves a selected simple class name to the runner's full name (the actual root-cause fix; a wrong match here silently runs the wrong class or nothing) — pinned by pre-push-test.sh case "green" (checks nested Outer+Inner names reach the runner and a helper class lands on unlisted: rather than running).
  2. pre-push.sh:211-213 — the GATE STOP threshold computed from post-resolution sel_n counts — pinned by pre-push-test.sh case "stop" (401 classes → exact GATE STOP line, exit 3, no -class run).
  3. pre-push.sh:51-56 — turning an exit-0/Total: 0 run into a red (the original defect being fixed: a matched-nothing run used to read as a pass) — pinned by pre-push-test.sh cases "zero" and "guard0".

Traps near this diff

  • "Under set -euo pipefail, a bare $(gh api …) assignment that fails kills the step" (Workflows and CI) — does not apply: pre-push.sh:29 uses set -uo pipefail (no -e), and the gh api call at pre-push.sh:91-92 is wrapped with 2>/dev/null, so a failed lookup degrades to an empty ids rather than aborting the script.
  • "Ruling (#5833): tests are load-bearing... answered with a behaviour test that fails on the defect and passes on the fix" (Tests and censuses) — applies, and the diff satisfies it rather than violating it: pre-push-test.sh drives the real gate through a fake dotnet and asserts on its exit code/output, not on source text; its header even states "CI does not run this file... These are behavior tests, not source pins," matching the ruling.
  • Lite/Darling parity (Repository mechanics) — does not apply to this file (dev tooling, not SKU app code), and the script itself treats both suites symmetrically throughout (for suite in darling lite, $DARLING/$LITE handled identically at every step changed here).

Divergence (blind read vs PR body)

None — the body's account matches the diff on every claim gradable from the tree. The one acknowledged behavior change not framed as a "fix" — class batches no longer run at all once the Guard stage, a class listing, or the STOP threshold has already decided the gate (pre-push.sh:249-250), versus the old code which ran them unconditionally — is explained inline by the new comment at pre-push.sh:249 and isn't claimed otherwise by the body; non-blocking.

Reviewer checklist

  • Run bash .github/scripts/tests/pre-push-test.sh for real (blocked here by a sandbox hook on declare -A) and confirm the "15 PASS" / mutant-failure counts the PR body reports.
  • Run bash .github/scripts/pre-push.sh once on a real Debug build to confirm the simple→full name resolution actually matches live -list classes/json output (including a nested Outer+Inner class) and that GATE STOP fires as described given the current nightly map.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant