fix(quality): report the real failure line and stop double-counting ci.yml gates (#11321)

Validated on a 17-PR combined board: validate-release-green within the board's 287/287, typecheck:core clean. Two accuracy bugs in the release-green verdict tool: an unanchored regex blamed a passing test line (matching a filename containing 'fail'), and 6 gates were double-recorded as both hard-failure and drift due to an id-format mismatch (ci.yml script name vs curated id). Found while reading the #9985 verdict — good catch.
This commit is contained in:
Paco Cartones
2026-08-24 06:50:26 +02:00
committed by GitHub
parent 79f8ae9d1e
commit 6984676d95
2 changed files with 168 additions and 2 deletions

View File

@@ -90,13 +90,30 @@ export function baselineValue(metric, root = ROOT) {
}
}
// A line that is unambiguously a PASS. Test reporters print the file name on BOTH the
// pass and the fail line, so a green line for a file whose NAME contains "fail"
// (fail-fast-*.test.ts, failover-*.test.ts) must never be offered as a failure cause.
const GREEN_LINE_RE = /^[✓✔√]/;
// Markers that are only meaningful at the START of a line: "FAIL" also occurs inside test
// FILE NAMES and inside summary prose ("Test Files 1 failed"), so matching it anywhere —
// and case-insensitively — reports a PASSING file as the cause of the red.
const LINE_START_FAILURE_RE = /^(?:[✖✗×]|FAIL\b|not ok\b|REGRESS)/;
// Markers that are unambiguous ANYWHERE in the line: tsc and Node emit them mid-line
// ("src/x.ts(10,5): error TS2322: ..."), so these stay unanchored. They are matched
// case-SENSITIVELY because that is how the emitting tools actually spell them.
const INLINE_FAILURE_RE = /\berror TS\d+\b|\bAssertionError\b|\bError:|\bREGRESS/;
/** Best-effort "first meaningful failure line" from captured command output. */
export function firstFailureLine(out) {
const lines = String(out || "")
.split("\n")
.map((l) => l.trim())
.filter(Boolean);
const hit = lines.find((l) => /||not ok|AssertionError|error TS|FAIL|Error:|REGRESS/i.test(l));
const hit = lines.find(
(l) => !GREEN_LINE_RE.test(l) && (LINE_START_FAILURE_RE.test(l) || INLINE_FAILURE_RE.test(l))
);
return (hit || lines[lines.length - 1] || "failed").slice(0, 200);
}
@@ -232,6 +249,36 @@ export function fullCiTimeoutFor(gateId) {
return FULL_CI_TIMEOUT_OVERRIDES_MS[gateId] ?? FULL_CI_DEFAULT_TIMEOUT_MS;
}
// ci.yml gate scripts whose result the CURATED pass already records under a DIFFERENT id.
// Without this map the --full-ci pass re-records them unconditionally as kind:"hard" while
// the curated pass recorded them as kind:"drift", and the SAME gate is printed in BOTH
// verdict buckets of one report (file-size / compression-budget appeared as a hard failure
// and as drift simultaneously in the #9985 verdict).
export const FULL_CI_CURATED_ALIASES = {
lint: "lint-errors",
"check:workflows": "workflow-lint",
"check:complexity-ratchets": "complexity",
};
/** Curated-pass id equivalent to a ci.yml gate script id ("check:file-size" -> "file-size"). */
export function curatedEquivalentId(scriptId) {
const id = String(scriptId || "");
if (Object.hasOwn(FULL_CI_CURATED_ALIASES, id)) return FULL_CI_CURATED_ALIASES[id];
return id.startsWith("check:") ? id.slice("check:".length) : id;
}
/**
* Bucket a --full-ci gate must be reported under: the classification the curated pass already
* gave the equivalent gate, else "hard" (the --full-ci default for gates the curated list does
* not cover). This only changes WHICH BUCKET a result is printed in — it never changes whether
* a gate runs, nor whether it passed.
*/
export function fullCiKindFor(scriptId, results) {
const equivalent = curatedEquivalentId(scriptId);
const curated = (results || []).find((r) => r.id === scriptId || r.id === equivalent);
return curated?.kind ?? "hard";
}
/**
* Parse a ci.yml text and return the ordered, de-duplicated list of gate commands to run.
* Each entry: { id, job, args:["run", <script>, ...("--" + args)], env }.
@@ -716,7 +763,10 @@ async function main() {
record({
id: g.id,
label: `ci.yml:${g.job} → npm ${g.args.join(" ")}`,
kind: "hard",
// Respect the curated classification when the curated pass already ran an equivalent
// gate under a different id — otherwise the same ratchet is reported as a HARD failure
// here AND as drift above, in one self-contradicting verdict.
kind: fullCiKindFor(g.id, results),
ok: code === 0,
detail: code === 0 ? "pass" : firstFailureLine(out),
});

View File

@@ -15,6 +15,8 @@ const {
extractCiGates,
FULL_CI_SKIP,
fullCiTimeoutFor,
curatedEquivalentId,
fullCiKindFor,
} = mod;
const extract = extractCiGates as (
@@ -361,3 +363,117 @@ test("extractCiGates: the REAL ci.yml yields the base-reds that leaked in v3.8.4
}
assert.ok(ids.size >= 20, "the real gate set is substantial (>= 20 static gates)");
});
// ─── Verdict accuracy (review of the #9985 release-green verdict) ────────────
test("firstFailureLine never blames a PASSING line whose test FILE NAME contains 'fail' (#9985)", () => {
// Observed in the 2026-08-23 verdict: the reported "cause" of the unit red was
// ✓ …fail-fast-concurrency-gate.test.ts (4 tests) 203ms
// i.e. a GREEN line, matched only because the unanchored /FAIL/i marker hit the
// substring "fail" inside the file name. The real ✖ line was three lines below.
const out = [
"> omniroute@3.8.50 test:unit",
" ✓ tests/unit/runtime/fail-fast-concurrency-gate.test.ts (4 tests) 203ms",
" ✓ tests/unit/router/failover-budget.test.ts (9 tests) 41ms",
" ✖ tests/unit/router/pricing.test.ts > picks the cheapest candidate",
"AssertionError [ERR_ASSERTION]: Expected values to be strictly equal: 2 !== 3",
].join("\n");
const hit = firstFailureLine(out);
assert.doesNotMatch(hit, /fail-fast-concurrency-gate/, "a green line is never the failure cause");
assert.doesNotMatch(hit, /failover-budget/, "a green line is never the failure cause");
assert.match(hit, /pricing\.test\.ts/, "the real failing line must be reported instead");
});
test("firstFailureLine still recognises every legitimate failure marker", () => {
const cases: [string, RegExp][] = [
["ok 1 - warms up\nnot ok 2 - routes to the cheapest key\n", /not ok 2/],
["Test Files 1 failed\nFAIL tests/unit/router/pricing.test.ts\n", /^FAIL /],
["src/x.ts(10,5): error TS2322: Type 'string' is not assignable.", /error TS2322/],
["✗ db-rules: raw sqlite handle left open", /db-rules/],
["Error: ENOENT: no such file or directory, open 'dist/server.js'", /ENOENT/],
["[cognitive-complexity] REGRESSÃO — 801 violações > baseline 797", /REGRESS/],
["[file-size] REGRESSED: open-sse/router.ts 1204 > cap 1100", /REGRESSED/],
];
for (const [out, expected] of cases) {
assert.match(firstFailureLine(out), expected, `marker lost for: ${out.slice(0, 40)}`);
}
});
test("firstFailureLine falls back to the last line when nothing matches", () => {
assert.equal(firstFailureLine("warming up\nall quiet\n"), "all quiet");
assert.equal(firstFailureLine(""), "failed");
});
test("curatedEquivalentId maps a ci.yml gate script onto the curated pass id (#9985)", () => {
assert.equal(curatedEquivalentId("check:file-size"), "file-size");
assert.equal(curatedEquivalentId("check:compression-budget"), "compression-budget");
// Curated ids that are NOT just the script name minus "check:".
assert.equal(curatedEquivalentId("check:workflows"), "workflow-lint");
assert.equal(curatedEquivalentId("check:complexity-ratchets"), "complexity");
assert.equal(curatedEquivalentId("lint"), "lint-errors");
// An uncurated gate keeps a stable, non-colliding identity.
assert.equal(curatedEquivalentId("check:route-validation:t06"), "route-validation:t06");
});
test("fullCiKindFor honours the curated classification of an already-known gate (#9985)", () => {
const curated = [
{ id: "file-size", kind: "drift", ok: false },
{ id: "compression-budget", kind: "drift", ok: false },
{ id: "workflow-lint", kind: "drift", ok: false },
{ id: "docs-all", kind: "hard", ok: true },
{ id: "lint-errors", kind: "hard", ok: true },
];
// Ratchets curated as DRIFT must stay drift when --full-ci re-runs them from ci.yml...
assert.equal(fullCiKindFor("check:file-size", curated), "drift");
assert.equal(fullCiKindFor("check:compression-budget", curated), "drift");
assert.equal(fullCiKindFor("check:workflows", curated), "drift");
// ...real-defect gates stay hard...
assert.equal(fullCiKindFor("check:docs-all", curated), "hard");
assert.equal(fullCiKindFor("lint", curated), "hard");
// ...and a gate the curated pass never ran defaults to hard (the --full-ci contract).
assert.equal(fullCiKindFor("check:bundle-size", curated), "hard");
assert.equal(fullCiKindFor("check:route-validation:t06", curated), "hard");
});
test("one gate can never land in BOTH verdict buckets of the same report (#9985)", () => {
// The 2026-08-23 verdict listed file-size and compression-budget as hard failures
// AND as drift, in the same table, because the --full-ci pass re-recorded every
// ci.yml gate as kind:"hard" and the dedupe only compared raw ids.
const curated = [
{ id: "file-size", kind: "drift", ok: false },
{ id: "compression-budget", kind: "drift", ok: false },
];
const fromCiYaml = ["check:file-size", "check:compression-budget"].map((id) => ({
id,
kind: fullCiKindFor(id, curated),
ok: false,
}));
const v = computeVerdict([...curated, ...fromCiYaml]);
const hardGates = new Set(v.hardFailures.map((r) => curatedEquivalentId(r.id)));
const contradictions = v.drift
.map((r) => curatedEquivalentId(r.id))
.filter((id) => hardGates.has(id));
assert.deepEqual(
contradictions,
[],
"a gate reported as hard must not also be reported as drift"
);
assert.equal(
v.releaseGreen,
true,
"a curated-drift ratchet must not block the release via the --full-ci path"
);
});
test("the --full-ci loop classifies from the curated results, not a hardcoded kind (#9985)", async () => {
const fs = await import("node:fs");
const src = fs.readFileSync(
new URL("../../scripts/quality/validate-release-green.mjs", import.meta.url),
"utf8"
);
assert.match(
src,
/kind:\s*fullCiKindFor\(g\.id,\s*results\)/,
"--full-ci must classify each ci.yml gate through fullCiKindFor()"
);
});