diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 7231df20cf..3e94eb4e4a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -24,7 +24,27 @@ jobs: # inspect alone does not pull; silent skip was fake B1 coverage (#1911). - run: docker pull mcr.microsoft.com/powershell:latest - run: bun run test:e2e -- terminal-wrap-fidelity.spec.ts composer-copy.spec.ts session-list-scroll.spec.ts cold-initial-tail.spec.ts scrollbar-auto-hide.spec.ts - - run: bun run test + # Fork: cli tests run through the flaky-quarantine gate (same + # script the local remat gate uses — config/flaky-quarantine.yaml + # is fork-only data, see docs/tooling/flaky-quarantine.md). + # Upstream/other forks: unchanged plain `bun run test`. + - name: Run tests + env: + # flaky-quarantine.mjs validate/check shell out to `gh issue + # view` to confirm a quarantine entry's issue is still open + # (requirement: a closed-issue entry must fail loudly, not + # sit there forever). Without this, every gh call returns + # UNKNOWN on a fresh runner and the gate fails for the wrong + # reason — matches the GH_TOKEN/GITHUB_TOKEN pattern already + # used by codex-mention-response.yml / issue-auto-response.yml. + GH_TOKEN: ${{ github.token }} + GITHUB_TOKEN: ${{ github.token }} + run: | + if [[ "${{ github.repository_owner }}" == "heavygee" ]]; then + bash scripts/tooling/flaky-quarantine-gate.sh "$PWD" + else + bun run test + fi # Fork has no windows-latest budget; cover the same file on Linux. - name: Codex MCP proxy unit (Linux stand-in for windows-codex-mcp) if: github.repository_owner == 'heavygee' diff --git a/config/flaky-quarantine.yaml b/config/flaky-quarantine.yaml new file mode 100644 index 0000000000..ab7c4dd956 --- /dev/null +++ b/config/flaky-quarantine.yaml @@ -0,0 +1,128 @@ +# Flaky-test quarantine registry — tracked, not ignored. +# +# FORK-ONLY DATA: these entries are fork-specific CI observations and issue +# links (heavygee/hapi). The mechanism that reads this file +# (scripts/tooling/flaky-quarantine.mjs + lib/flaky-quarantine-core.mjs) is +# generic — any repo with CI would want it — and is a candidate for an +# eventual standalone/upstream extraction. Keep the split: this file carries +# the fork's observations, not the other way around. +# +# WHY THIS EXISTS (2026-10-15): operator-directed after two unrelated pieces +# of work were held hostage by red lights nobody could fix locally — PR #222 +# (iOS app-build failed 3 different ways on a diff that never touches the +# failing screens, heavygee/hapi#273) and the #270 idle-gate remat (tripped +# by agentCliGuard.test.ts under full-suite timing, heavygee/hapi#274). Each +# time, an agent escalated to the operator for a judgement call on a test it +# didn't write. "Red doesn't really mean red" is the disease this fixes. +# +# Evidence bar — do not add an entry without ONE of: +# - "isolation passes, full-suite fails" (both runs actually observed, not +# asserted — log what you saw in the linked issue) +# - ">=2 failures with DIFFERENT assertions/signatures on the same +# unchanged commit" (same bug retried is not flake; this is a regression) +# "Failed once" is never sufficient evidence. +# +# Checked by: scripts/tooling/flaky-quarantine.mjs (validate|check), called +# from both CI (.github/workflows/test.yml, ios.yml) and the local remat gate +# (hapi-driver-rebuild.sh) via scripts/tooling/flaky-quarantine-gate.sh — ONE +# script, so the two gates cannot drift onto different lists. +# +# Fields (all required unless noted): +# id - stable test identifier. +# vitest: ": " +# e.g. produced verbatim by vitest's own +# --reporter=json `fullName` field — see +# `flaky-quarantine.mjs from-vitest-json`. +# XCTest: "ios:<TestClass>/<testMethod>" +# owner - person/agent handle responsible for eventually fixing or +# removing this entry. No anonymous entries. +# issue - heavygee/hapi issue number tracking the root cause. MUST +# be OPEN (validated against the live GitHub issue state, +# not just present) — a quarantined test whose issue is +# closed is either fixed or abandoned, and both deserve to +# fail loudly again, not sit here forever. +# signature - one or more " && "-joined required substrings of the +# known failure text — ALL must be present in the message +# for a match. A failure under this id that doesn't match +# is treated as a NEW failure and BLOCKS — this is what +# stops quarantine from hiding a real regression that +# happens to land in the same test. Prefer combining the +# assertion text with its stack-trace call site (e.g. +# "expected true to be false && myTest.ts:42:") — either +# alone can over-match (generic assertion text can hit a +# different assertion in the same test; a bare call site +# can match a different kind of failure thrown from the +# same line). See the cli: entry below for the real case +# that forced this (cold review, 2026-10-15). +# added - ISO date quarantined. +# expires - ISO date. Default policy is +14 days from `added`. Past +# this date the entry excuses nothing — it is not a grace +# period, the entry simply drops out of the active set and +# the test goes back to blocking until fixed, deleted, or +# deliberately renewed. +# renewal_count - (optional, default 0) each +1 permits 14 more days +# beyond the default (enforced by the validator: expires +# may not be more than 14 + 14*renewal_count days after +# added). You cannot push the date out without bumping +# this — the renewal itself is a diff. +# renewal_reason - required whenever renewal_count > 0. A concrete reason +# ("renewed: still intermittent under CI load, see #274"), +# not a bare date bump. Older renewal reasons live in git +# log on this file, not here — this field is the latest. +# +# Ceiling: scripts/tooling/flaky-quarantine.mjs defaults to 8 active entries. +# Past that, the gate fails regardless of test outcomes until the registry is +# cleaned up — the count is a forcing function, not a decoration. + +quarantine: + - id: "cli:agentCliGuard prunes crash-stale registering/<deadPid> so list-models is not pinned" + owner: "heavygee" + issue: 274 + # Two cold-review rounds landed here (2026-10-15): + # Round 1: "expected true to be false" alone is too generic — this test + # has a SECOND assertion two lines below (existsSync(dir)).toBe(false)) + # that fails with byte-identical text for a real, different reason. + # Round 2: the stack-frame line ALONE is also not enough — if + # isAgentAcpTransportActive() itself started throwing instead of + # returning the wrong boolean, the thrown error's stack would still + # name this same line, and a location-only signature would wrongly + # excuse a completely different failure mode. + # `signature` supports " && "-joined required substrings — ALL must be + # present. Combining the assertion text with its call site closes both + # gaps: neither alone can satisfy this quarantine's excuse condition. + # Line-number signatures are brittle to lines being inserted above 309 in + # this file — if that happens, this stops matching and BLOCKS (fails + # closed), which forces someone to look rather than silently re-matching. + signature: "expected true to be false && agentCliGuard.test.ts:309:" + added: "2026-10-15" + expires: "2026-10-29" + + # NOT WIRED YET (see docs/tooling/flaky-quarantine.md — the ios adapter is + # built and proven against real logs, but has no macOS runner to verify the + # live ios.yml integration). Known limitation carried over from the cold + # review that caught the cli entry's generic-signature problem above: these + # three ios signatures are the most specific substring captured from the + # real failing run WITHOUT including per-run-variable text (memory + # addresses, measured pixel values). They have NOT been checked against the + # iOS test source for a sibling assertion in the same method that could + # produce the same generic text — do that check before wiring these live. + - id: "ios:ToolTranscriptPresentationTests/testPlanPublicationUpdatesRecyclingAndInspectorPreserveTheDocumentAndAnchor" + owner: "heavygee" + issue: 273 + signature: "XCTAssertNil failed: \"<UICollectionViewCell" + added: "2026-10-15" + expires: "2026-10-29" + + - id: "ios:HomeFilterPresentationTests/testFilteringResetsScrollButLiveUpdatesAndChatReturnPreserveIt" + owner: "heavygee" + issue: 273 + signature: "XCTUnwrap failed: expected non-nil value of type \"UICollectionView\"" + added: "2026-10-15" + expires: "2026-10-29" + + - id: "ios:TranscriptTypographyTests/testShrinkingTextClampsAnOffsetBeyondTheNewRowHeight" + owner: "heavygee" + issue: 273 + signature: "XCTAssertEqualWithAccuracy failed" + added: "2026-10-15" + expires: "2026-10-29" diff --git a/docs/tooling/flaky-quarantine.md b/docs/tooling/flaky-quarantine.md new file mode 100644 index 0000000000..38ca30b178 --- /dev/null +++ b/docs/tooling/flaky-quarantine.md @@ -0,0 +1,101 @@ +# Flaky-test quarantine + +## Why this exists + +Two unrelated pieces of work were held hostage by red lights nobody could fix +locally (2026-10-15): [PR #222](https://github.com/heavygee/hapi/pull/222) +blocked on iOS `app-build` failing 3 different ways on a diff that never +touches the failing screens ([#273](https://github.com/heavygee/hapi/issues/273)), +and the #270 idle-gate remat tripped by `agentCliGuard.test.ts` under +full-suite timing ([#274](https://github.com/heavygee/hapi/issues/274)). Each +time, an agent escalated to the operator for a judgement call on a test it +didn't write. A tolerated red light teaches everyone that red doesn't really +mean red — this is the fix. + +**A quarantined test still runs and still reports.** This is a tracked +quarantine, not an ignore list: every entry has an owner, an open issue, a +captured failure signature, and an expiry. Nothing here hides a test from CI +output; it only decides whether a *known* failure may be excused for +gate-pass purposes. + +## The three rules + +1. **Known id + matching failure signature → excused (non-blocking).** +2. **Known id + a DIFFERENT signature → BLOCKS.** Id-matching alone can't + tell "the same old flake" from "a new bug that landed in the same test" — + comparing the failure text closes most of that gap. A warning nobody must + act on is exactly the mechanism that trained everyone to ignore red in the + first place, so a mismatch blocks rather than warns. +3. **An expired entry excuses nothing.** It is not a grace period — the + entry drops out of the active set, and the next failure under that id is + an ordinary blocking failure until someone renews (with a reason) or + removes it. + +Plus a **ceiling** (default 8) on the number of active entries: past it, the +gate fails regardless of whether today's tests pass, until the registry is +cleaned up. The count is a forcing function, not a decoration. + +## Where the data lives + +`config/flaky-quarantine.yaml` — one file, read by both consumers (the local +remat gate and CI), so they cannot drift onto different lists. Field +reference and evidence bar are documented in the file's own header comment. + +**Fork-only data; generic mechanism.** The YAML's content (which tests, which +issues, which owners) is specific to `heavygee/hapi`'s own CI observations. +The code that reads it (`scripts/tooling/flaky-quarantine.mjs` + +`scripts/tooling/lib/flaky-quarantine-core.mjs`) is generic — any repo with +CI would want it — and is a candidate for an eventual standalone/upstream +extraction. Don't let fork-specific assumptions leak into the script. + +## How it's wired + +``` + config/flaky-quarantine.yaml + / \ + scripts/tooling/flaky-quarantine-gate.sh ios/scripts/test-transcript.sh + (hapi-driver-rebuild.sh --verify, (ios.yml — DRAFTED, not yet + .github/workflows/test.yml) applied; see below) +``` + +- **cli (vitest):** `flaky-quarantine-gate.sh <repo-root>` runs hub/web/ + shared/relay exactly as before (unchanged, fully blocking), then runs + `cli`'s tests via `vitest run --reporter=default --reporter=json + --outputFile.json=...`, converts the JSON report + (`flaky-quarantine.mjs from-vitest-json`) and feeds it to + `flaky-quarantine.mjs check`. Same script, called from both + `hapi-driver-rebuild.sh`'s `--verify` test step and + `.github/workflows/test.yml`'s `test` job (fork-only — upstream and other + forks keep plain `bun run test`, see the `github.repository_owner` guard in + that file). +- **iOS (XCTest):** `flaky-quarantine.mjs from-xcodebuild-log` parses + xcodebuild's `error: -[Class method] : <message>` failure lines into the + same normalized `{id, failed, message}` shape, proven against the three + real PR #222 attempt logs (`ios:<TestClass>/<testMethod>` ids). **Not yet + wired into `ios/scripts/test-transcript.sh` / `ios.yml`** — this host has + no macOS runner, so the actual shell integration (capturing xcodebuild's + stdout, `bun` on the GitHub-hosted `macos-15` runner, which `ios.yml` + doesn't currently set up at all) could not be executed or verified here. + Wiring it blind into a live gating workflow risked turning a flake into a + hard break. Apply + smoke-test on an actual macOS run before trusting it. + +## Validating the registry + +```bash +bun scripts/tooling/flaky-quarantine.mjs validate +``` + +Checks schema (owner/issue/signature/dates present and sane, renewal math +consistent with `renewal_count`), confirms every `issue` is still OPEN on +GitHub, and reports the active count against the ceiling. Exit 2 on any +failure — safe to run standalone in a lint step. + +## Negative controls + +`scripts/tooling/flaky-quarantine.test.sh` (picked up automatically by the +existing `tooling-tests` CI job, no workflow change needed) proves, with +synthetic fixtures, that the gate can actually go red three ways before +anyone trusts it to go green: a quarantined test failing with its known +signature does not block; the same id failing with a different signature +blocks; an expired entry blocks even with a matching signature. Plus schema +and ceiling checks. diff --git a/scripts/tooling/flaky-quarantine-gate.sh b/scripts/tooling/flaky-quarantine-gate.sh new file mode 100644 index 0000000000..691d744d23 --- /dev/null +++ b/scripts/tooling/flaky-quarantine-gate.sh @@ -0,0 +1,71 @@ +#!/usr/bin/env bash +# flaky-quarantine-gate.sh <repo-root> +# +# Shared `bun run test` replacement for BOTH the local remat gate +# (hapi-driver-rebuild.sh) and CI (.github/workflows/test.yml) — the SAME +# script, so the two gates cannot drift onto different interpretations of +# config/flaky-quarantine.yaml. See docs/tooling/flaky-quarantine.md. +# +# Only `cli` is quarantine-aware today — that is where the two seed entries +# (heavygee/hapi#273's iOS siblings are a separate `ios:` id space checked by +# ios/scripts/test-transcript.sh; #274 is the cli-side entry here) live. +# hub/web/shared/relay run exactly as before, unchanged, fully blocking. +# Extending quarantine awareness to another package is the same +# vitest-json-reporter + adapter + check sequence repeated for that package. +set -euo pipefail + +REPO_ROOT="${1:?usage: flaky-quarantine-gate.sh <repo-root>}" +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# Honor an inherited $BUN (hapi-driver-rebuild.sh resolves "${BUN:-$HOME/.bun/bin/bun}" +# but does not export it — callers must pass it through explicitly, e.g. +# `BUN="$BUN" bash flaky-quarantine-gate.sh ...`). Falling back to bare `bun` +# only works when it happens to be on PATH, which the caller's own fallback +# does not guarantee (cold review, 2026-10-15). +BUN="${BUN:-bun}" + +run_plain() { + echo "Running $1 tests..." + (cd "$REPO_ROOT/$1" && "$BUN" run test) +} + +run_plain hub +run_plain web +run_plain shared +run_plain relay + +# Whole-registry health (ceiling + schema + issue-open-ness) is its OWN gate, +# run once, clearly attributed — NOT folded into the package-scoped `check` +# below. Without this split, an unrelated package's entry going bad (e.g. an +# `ios:` issue getting closed) would fail THIS cli-only run for a reason that +# has nothing to do with cli, which is exactly the cross-package coupling the +# --package flag on `check` exists to avoid. The ceiling itself is +# deliberately whole-registry (not per-package) — see flaky-quarantine.mjs. +echo "Validating quarantine registry (whole-file: ceiling + schema + issue-open)..." +if ! "$BUN" "$HERE/flaky-quarantine.mjs" validate --registry "$REPO_ROOT/config/flaky-quarantine.yaml"; then + echo "ERROR: config/flaky-quarantine.yaml itself is invalid or over ceiling — fix the registry before any gate can trust it." >&2 + exit 1 +fi + +echo "Running cli tests (quarantine-aware)..." +CLI_JSON="$(mktemp -t flaky-quarantine-cli-XXXXXX.json)" +cleanup() { rm -f "$CLI_JSON" "${NORMALIZED:-}"; } +trap cleanup EXIT + +set +e +(cd "$REPO_ROOT/cli" && "$BUN" run tools:unpack && "$BUN" x vitest run --reporter=default --reporter=json --outputFile.json="$CLI_JSON") +VITEST_RC=$? +set -e + +if [[ ! -s "$CLI_JSON" ]]; then + echo "ERROR: cli test run produced no JSON report (a setup step likely failed before vitest ran, exit $VITEST_RC) — hard failure, not quarantine-eligible" >&2 + exit 1 +fi + +NORMALIZED="$(mktemp -t flaky-quarantine-results-XXXXXX.json)" +"$BUN" "$HERE/flaky-quarantine.mjs" from-vitest-json "$CLI_JSON" --package cli >"$NORMALIZED" +# --runner-exit-code: a JSON report CAN exist (vitest wrote it) while vitest +# itself still exited non-zero with zero attributable per-test failures — a +# collection/import error or zero tests discovered. Without passing this +# through, that failure mode would silently normalize to an empty results +# list and the gate would report a clean pass (cold review, 2026-10-15). +"$BUN" "$HERE/flaky-quarantine.mjs" check --results "$NORMALIZED" --package cli --registry "$REPO_ROOT/config/flaky-quarantine.yaml" --runner-exit-code "$VITEST_RC" diff --git a/scripts/tooling/flaky-quarantine.mjs b/scripts/tooling/flaky-quarantine.mjs new file mode 100755 index 0000000000..d5bd5a54f2 --- /dev/null +++ b/scripts/tooling/flaky-quarantine.mjs @@ -0,0 +1,261 @@ +#!/usr/bin/env bun +/** + * flaky-quarantine — tracked quarantine for intermittently-failing tests. + * + * GENERIC MECHANISM, FORK-ONLY DATA: see the header of + * lib/flaky-quarantine-core.mjs — this CLI and that module contain no + * heavygee/hapi-specific logic and are a candidate for an eventual + * standalone/upstream extraction. config/flaky-quarantine.yaml (the data) is + * fork-specific and stays behind. + * + * A quarantined test still runs and still reports. This tool never hides a + * failure — it only decides whether a failure may be excused for gate-pass + * purposes, based on three rules (see core module for the why): + * 1. Known id + matching failure signature -> excused (non-blocking) + * 2. Known id + DIFFERENT failure signature -> BLOCKS (new/real failure) + * 3. Expired entry (regardless of signature) -> BLOCKS + * Plus: more than --ceiling active entries -> the registry itself BLOCKS, + * independent of whether today's tests pass. + * + * Usage: + * flaky-quarantine.mjs validate [--registry PATH] [--ceiling N] [--repo OWNER/NAME] [--skip-issue-check] + * flaky-quarantine.mjs check --results PATH.json --package PREFIX [--runner-exit-code N] [--registry PATH] [--repo OWNER/NAME] [--skip-issue-check] + * flaky-quarantine.mjs from-vitest-json PATH.json # adapter: vitest --reporter=json -> normalized results + * flaky-quarantine.mjs from-xcodebuild-log PATH.log # adapter: xcodebuild test stdout -> normalized results + * + * `--repo` (default: parsed from the `origin` git remote, falling back to + * heavygee/hapi) is where issue-open-ness is checked — this tool has no + * heavygee/hapi-specific logic, so a reused copy elsewhere must not silently + * keep checking THIS repo's issue numbers. + * `--runner-exit-code N` (check only) tells the gate decision that the test + * runner itself exited N. If N is non-zero and --results contains no failed + * entries at all, that is a runner-level crash (import error, zero tests + * discovered) with nothing to attribute to a specific test — it BLOCKS + * unconditionally rather than silently passing because nothing matched. + * + * `check` reads normalized results from --results, or from stdin if omitted. + * `--package PREFIX` (e.g. "cli") restricts BOTH which registry entries are + * validated (schema/issue-open) AND which are eligible to excuse a failure to + * ids starting with "PREFIX:". Without it, a schema problem or a closed issue + * on an entry for a package this invocation never touched (e.g. an `ios:` + * entry during a cli-only run) would fail the gate for a reason that has + * nothing to do with what actually ran — exactly the cross-package coupling + * this flag exists to prevent. The CEILING is deliberately NOT scoped by + * package (see `validate`, always whole-registry) — it is meant to be felt + * everywhere once the registry as a whole gets too big, by design. + * Exit codes: + * check: 0 = gate passes. 1 = blocking failure(s), OR a --package-scoped + * registry entry is invalid — either way, the gate must stop. + * validate: 0 = registry OK. 2 = schema error or over ceiling (distinct + * code so a dedicated lint step can tell "registry itself is + * broken" apart from "a test failed"). Always whole-registry. + */ +import { readFileSync, existsSync } from 'node:fs'; +import { join, dirname } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { spawnSync } from 'node:child_process'; +import { parseQuarantineYaml, validateEntries, evaluate, DEFAULT_CEILING } from './lib/flaky-quarantine-core.mjs'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const REPO_ROOT = join(HERE, '..', '..'); + +const argv = process.argv.slice(2); +const cmd = argv[0]; +const flag = (name, fallback) => { + const i = argv.indexOf(`--${name}`); + return i >= 0 && argv[i + 1] ? argv[i + 1] : fallback; +}; +const has = (name) => argv.includes(`--${name}`); + +const registryPath = flag('registry', join(REPO_ROOT, 'config/flaky-quarantine.yaml')); +const ceiling = Number.parseInt(flag('ceiling', String(DEFAULT_CEILING)), 10); +const skipIssueCheck = has('skip-issue-check'); + +/** + * Default repo for the issue-open check. Hardcoding heavygee/hapi would make + * this generic tool silently check the WRONG repo's issue numbers if reused + * elsewhere (as the module header says it's designed for) or if this repo is + * ever transferred — derive from the origin remote, falling back to the + * fork's own slug only when that fails (e.g. no git context at all). + */ +function detectRepo() { + // `gh repo view`'s default resolution is NOT reliable here: this repo + // carries multiple remotes (origin=heavygee/hapi, upstream=tiann/hapi, + // plus contributor forks), and `gh repo view` has been observed to + // resolve to upstream rather than origin in this exact mirror. Parse + // `origin` explicitly instead of trusting gh's own default. + const r = spawnSync('git', ['-C', REPO_ROOT, 'remote', 'get-url', 'origin'], { encoding: 'utf8' }); + if (r.status === 0) { + const m = r.stdout.trim().match(/[/:]([^/:]+\/[^/.]+?)(\.git)?$/); + if (m) return m[1]; + } + return 'heavygee/hapi'; +} +const repo = flag('repo', null) ?? detectRepo(); + +function loadEntries() { + if (!existsSync(registryPath)) return []; + return parseQuarantineYaml(readFileSync(registryPath, 'utf8')); +} + +function fetchIssueStates(entries) { + if (skipIssueCheck) return {}; + const nums = [...new Set(entries.map((e) => e.issue).filter((n) => Number.isInteger(n)))]; + const states = {}; + for (const n of nums) { + const r = spawnSync('gh', ['issue', 'view', String(n), '--repo', repo, '--json', 'state', '--jq', '.state'], { encoding: 'utf8' }); + states[n] = r.status === 0 ? r.stdout.trim() : 'UNKNOWN'; + } + return states; +} + +function printValidation({ errors, expired, valid }) { + console.log(`flaky-quarantine: ${valid.length} active, ${expired.length} expired, ${errors.length} schema error(s) (registry: ${registryPath})`); + for (const e of expired) { + console.log(` EXPIRED (no longer excuses anything): ${e.id} — issue #${e.issue}, owner ${e.owner}, expired ${e.expires}`); + } + for (const err of errors) { + console.error(` INVALID [${err.field}] ${err.id}: ${err.message}`); + } + for (const e of valid) { + console.log(` active: ${e.id} — issue #${e.issue}, owner ${e.owner}, expires ${e.expires}`); + } +} + +function runValidate() { + const entries = loadEntries(); + const issueStates = fetchIssueStates(entries); + const result = validateEntries(entries, { issueStates }); + printValidation(result); + const activeCount = result.valid.length; + const ceilingExceeded = activeCount > ceiling; + if (ceilingExceeded) { + console.error(`flaky-quarantine: CEILING EXCEEDED — ${activeCount} active entries > ceiling ${ceiling}. Clean up before adding more.`); + } + const ok = result.errors.length === 0 && !ceilingExceeded; + console.log(ok ? 'flaky-quarantine: validate OK' : 'flaky-quarantine: validate FAILED'); + process.exit(ok ? 0 : 2); +} + +function runCheck() { + const resultsPath = flag('results', null); + const raw = resultsPath ? readFileSync(resultsPath, 'utf8') : readFileSync(0, 'utf8'); + const results = JSON.parse(raw); + + const pkg = flag('package', null); + let entries = loadEntries(); + if (pkg) { + entries = entries.filter((e) => e.id === pkg || e.id.startsWith(`${pkg}:`)); + } else { + console.error('flaky-quarantine: WARNING — check running with no --package filter; every registry entry (all packages) participates in this gate\'s pass/fail, including unrelated ones. Pass --package to scope this.'); + } + const issueStates = fetchIssueStates(entries); + const { valid, errors, expired } = validateEntries(entries, { issueStates }); + printValidation({ valid, errors, expired }); + + const runnerExitCode = Number.parseInt(flag('runner-exit-code', '0'), 10); + const verdict = evaluate({ validEntries: valid, errors, expired, results, ceiling, runnerExitCode }); + + console.log(`flaky-quarantine: ${verdict.excused.length} failure(s) excused, ${verdict.blocking.length} blocking, ${verdict.activeCount}/${verdict.ceiling} active`); + for (const x of verdict.excused) { + console.log(` EXCUSED: ${x.id} (issue #${x.issue}, owner ${x.owner}) — known signature`); + } + for (const b of verdict.blocking) { + console.error(` BLOCKING: ${b.id} — ${b.reason}`); + if (b.expectedSignature) { + console.error(` expected signature: ${JSON.stringify(b.expectedSignature)}`); + for (const m of b.messages ?? []) { + console.error(` actual message: ${JSON.stringify(m.slice(0, 200))}`); + } + } + } + if (verdict.ceilingExceeded) { + console.error(`flaky-quarantine: CEILING EXCEEDED — ${verdict.activeCount} active entries > ceiling ${ceiling}. Gate fails until the registry is cleaned up, independent of test results.`); + } + + process.exit(verdict.pass ? 0 : 1); +} + +/** + * Adapter: vitest `--reporter=json` -> normalized [{id, failed, messages}]. + * + * `messages` is an ARRAY, not a joined string: a quarantined test can fail + * with more than one message in one run (multiple `expect.soft()` failures, + * or a known assertion plus an unrelated hook failure). Joining them would + * let the signature match anywhere in the combined blob, excusing a second, + * different regression riding along next to the known flake — evaluate() + * requires every element of `messages` to independently match. + * + * A test FILE can also fail with NO assertions at all — a collection/import + * error, or an unhandled exception during setup. Vitest represents this as a + * `testResults[]` entry with `status: "failed"`, an empty `assertionResults`, + * and a file-level `message`. Emitting that as its own failed result (with + * an id no registry entry will ever match) is what makes it BLOCK instead of + * silently vanishing because nothing in `assertionResults` represented it. + */ +function fromVitestJson(path, pkgPrefix) { + const data = JSON.parse(readFileSync(path, 'utf8')); + const out = []; + for (const file of data.testResults ?? []) { + const assertions = file.assertionResults ?? []; + if (assertions.length === 0 && file.status === 'failed') { + out.push({ id: `${pkgPrefix}:(suite) ${file.name}`, failed: true, messages: [file.message ?? 'suite failed with no assertions'] }); + continue; + } + for (const a of assertions) { + out.push({ + id: `${pkgPrefix}:${a.fullName}`, + failed: a.status === 'failed', + messages: a.failureMessages ?? [], + }); + } + } + return out; +} + +/** + * Adapter: `xcodebuild test` plain stdout -> normalized results. Parses the + * `error: -[Module.Class method] : <message>` lines xcodebuild emits for + * each XCTest failure. Only emits failed entries (xcodebuild's plain stdout + * does not cleanly enumerate every PASSED test by id the way vitest JSON + * does) — sufficient for this gate, which only ever needs to classify + * failures, never passes. One result per matched line (not grouped/deduped + * by id) so a test with multiple XCTAssert failures in one run is treated + * the same way the vitest adapter treats multiple failureMessages — every + * message must independently match the signature. + */ +function fromXcodebuildLog(path) { + const text = readFileSync(path, 'utf8'); + const grouped = new Map(); + const re = /error: -\[\S+\.(\w+) (\w+)\] : (.+)$/gm; + let m; + while ((m = re.exec(text))) { + const [, cls, method, message] = m; + const id = `ios:${cls}/${method}`; + if (!grouped.has(id)) grouped.set(id, []); + grouped.get(id).push(message.trim()); + } + return [...grouped.entries()].map(([id, messages]) => ({ id, failed: true, messages })); +} + +switch (cmd) { + case 'validate': + runValidate(); + break; + case 'check': + runCheck(); + break; + case 'from-vitest-json': { + const results = fromVitestJson(argv[1], flag('package', 'cli')); + console.log(JSON.stringify(results, null, 2)); + break; + } + case 'from-xcodebuild-log': { + const results = fromXcodebuildLog(argv[1]); + console.log(JSON.stringify(results, null, 2)); + break; + } + default: + console.error('Usage: flaky-quarantine.mjs <validate|check|from-vitest-json|from-xcodebuild-log> [...]'); + process.exit(2); +} diff --git a/scripts/tooling/flaky-quarantine.test.sh b/scripts/tooling/flaky-quarantine.test.sh new file mode 100755 index 0000000000..490dc8b14a --- /dev/null +++ b/scripts/tooling/flaky-quarantine.test.sh @@ -0,0 +1,366 @@ +#!/usr/bin/env bash +# Negative controls for flaky-quarantine.mjs — prove it can go red, three +# ways, before anyone trusts it to go green: +# A. quarantined test fails with its KNOWN signature -> does NOT block. +# B. quarantined test fails with a DIFFERENT signature -> BLOCKS. +# C. an EXPIRED entry (even with a matching signature) -> BLOCKS. +# Plus schema (missing owner, renewal math, unmatched expiry) and ceiling. +set -euo pipefail + +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +CLI="$HERE/flaky-quarantine.mjs" +TMP="$(mktemp -d)" +trap 'rm -rf "$TMP"' EXIT + +# Computed relative to the actual run, not hardcoded — a fixed "currently +# active" date pair goes stale the moment the wall clock passes it, silently +# flipping these fixtures' meaning (control A would start blocking instead of +# excusing; the ceiling fixture's entries would stop counting as active) and +# permanently reddening the tooling-tests CI job that runs this file on every +# relevant PR (cold review, 2026-10-15). Control C's PAST dates are +# deliberately literal — "already expired" needs to stay true forever, not +# just today. +TODAY="$(date -u +%Y-%m-%d)" +FUTURE="$(date -u -d '+10 days' +%Y-%m-%d)" +RENEWAL_TOO_FAR="$(date -u -d '+47 days' +%Y-%m-%d)" + +fail=0 +check_exit() { + local desc="$1" expected="$2" actual="$3" + if [[ "$actual" -eq "$expected" ]]; then + echo "OK: $desc (exit $actual)" + else + echo "FAIL: $desc — expected exit $expected, got $actual" + fail=1 + fi +} + +registry() { + # __TODAY__/__FUTURE__/__RENEWAL_TOO_FAR__ placeholders let call sites + # stay single-quoted (no accidental bash interpolation of the literal + # YAML) while still resolving to the actual run's dates. + local content="$1" + content="${content//__TODAY__/$TODAY}" + content="${content//__FUTURE__/$FUTURE}" + content="${content//__RENEWAL_TOO_FAR__/$RENEWAL_TOO_FAR}" + cat >"$TMP/registry.yaml" <<EOF +quarantine: +$content +EOF +} + +results() { + printf '%s' "$1" >"$TMP/results.json" +} + +run_check() { + set +e + bun "$CLI" check --results "$TMP/results.json" --registry "$TMP/registry.yaml" --skip-issue-check --ceiling "${1:-8}" >"$TMP/out.log" 2>&1 + echo $? + set -e +} + +run_check_pkg() { + local pkg="$1" + set +e + bun "$CLI" check --results "$TMP/results.json" --registry "$TMP/registry.yaml" --skip-issue-check --package "$pkg" >"$TMP/out.log" 2>&1 + echo $? + set -e +} + +# --- Control A: matching signature does not block --------------------------- +registry ' - id: "cli:suite does the thing" + owner: "alice" + issue: 1 + signature: "expected true to be false" + added: "__TODAY__" + expires: "__FUTURE__"' +results '[{"id":"cli:suite does the thing","failed":true,"messages":["AssertionError: expected true to be false"]}]' +rc=$(run_check) +check_exit "A: matching signature excuses the failure" 0 "$rc" +grep -q "EXCUSED: cli:suite does the thing" "$TMP/out.log" || { echo "FAIL: A missing EXCUSED line"; fail=1; } + +# --- Control B: id matches, signature does not -> blocks --------------------- +results '[{"id":"cli:suite does the thing","failed":true,"messages":["AssertionError: expected 1 to be 2"]}]' +rc=$(run_check) +check_exit "B: signature mismatch blocks (new/real failure)" 1 "$rc" +grep -q "BLOCKING: cli:suite does the thing" "$TMP/out.log" || { echo "FAIL: B missing BLOCKING line"; fail=1; } +grep -q "quarantined signature mismatch" "$TMP/out.log" || { echo "FAIL: B missing mismatch reason"; fail=1; } + +# --- Control C: expired entry blocks even with a matching signature --------- +registry ' - id: "cli:suite does the thing" + owner: "alice" + issue: 1 + signature: "expected true to be false" + added: "2020-01-01" + expires: "2020-01-15"' +results '[{"id":"cli:suite does the thing","failed":true,"messages":["AssertionError: expected true to be false"]}]' +rc=$(run_check) +check_exit "C: expired entry blocks despite matching signature" 1 "$rc" +grep -q "EXPIRED (no longer excuses anything): cli:suite does the thing" "$TMP/out.log" || { echo "FAIL: C missing EXPIRED line"; fail=1; } +grep -q "BLOCKING: cli:suite does the thing" "$TMP/out.log" || { echo "FAIL: C expired entry did not fall through to blocking"; fail=1; } + +# --- Sanity: a non-quarantined failing id blocks normally -------------------- +registry ' - id: "cli:unrelated test" + owner: "alice" + issue: 1 + signature: "x" + added: "__TODAY__" + expires: "__FUTURE__"' +results '[{"id":"cli:something else entirely","failed":true,"messages":["boom"]}]' +rc=$(run_check) +check_exit "sanity: unquarantined failure blocks" 1 "$rc" + +# --- Sanity: passing tests never block regardless of registry --------------- +results '[{"id":"cli:something else entirely","failed":false,"messages":[""]}]' +rc=$(run_check) +check_exit "sanity: passing test is always fine" 0 "$rc" + +# --- Schema: missing owner is a registry error, not silently ignored ------- +registry ' - id: "cli:no owner" + issue: 1 + signature: "x" + added: "__TODAY__" + expires: "__FUTURE__"' +results '[{"id":"cli:no owner","failed":true,"messages":["x"]}]' +rc=$(run_check) +check_exit "schema: missing owner is a registry error (fails closed)" 1 "$rc" +grep -q "INVALID \[owner\]" "$TMP/out.log" || { echo "FAIL: missing-owner error not reported"; fail=1; } +grep -q "BLOCKING: cli:no owner" "$TMP/out.log" || { echo "FAIL: invalid entry should fail closed (not excuse)"; fail=1; } + +# --- Schema: expires pushed past renewal_count math is rejected ------------- +registry ' - id: "cli:pushed too far" + owner: "alice" + issue: 1 + signature: "x" + added: "__TODAY__" + expires: "__RENEWAL_TOO_FAR__" + renewal_count: 1 + renewal_reason: "still flaky"' +results '[]' +rc=$(run_check) +check_exit "schema: expires beyond renewal_count math is rejected" 1 "$rc" +grep -q "only allows 28d" "$TMP/out.log" || { echo "FAIL: renewal-math error message not found"; fail=1; } + +# --- Schema: renewal_count > 0 without a reason is rejected ----------------- +registry ' - id: "cli:no reason" + owner: "alice" + issue: 1 + signature: "x" + added: "__TODAY__" + expires: "__FUTURE__" + renewal_count: 1' +results '[]' +rc=$(run_check) +check_exit "schema: renewal without a reason is rejected" 1 "$rc" +grep -q "renewal_count > 0 requires a renewal_reason" "$TMP/out.log" || { echo "FAIL: missing-renewal-reason error not found"; fail=1; } + +# --- Regression (cold review, 2026-10-15): a partially-numeric issue must be +# rejected, not silently truncated to its numeric prefix. Before the fix, +# Number.parseInt("27O", 10) === 27 — a typo'd issue number would silently +# point at a different, unrelated open issue. +registry ' - id: "cli:bad issue number" + owner: "alice" + issue: 27O + signature: "x" + added: "__TODAY__" + expires: "__FUTURE__"' +results '[]' +rc=$(run_check) +check_exit "schema: a partially-numeric issue (27O) is rejected, not truncated to 27" 1 "$rc" +grep -q "issue must be a positive GitHub issue number" "$TMP/out.log" || { echo "FAIL: bad-issue error not found"; fail=1; } + +# --- Regression (cold review, 2026-10-15): a day-overflow date (2026-02-30, +# which JS Date silently normalizes to 2026-03-02) must be rejected, not +# silently shifted — a mistyped expiry should not quietly extend or shorten +# the quarantine window. +registry ' - id: "cli:bad date" + owner: "alice" + issue: 1 + signature: "x" + added: "__TODAY__" + expires: "2026-02-30"' +results '[]' +rc=$(run_check) +check_exit "schema: a day-overflow date (2026-02-30) is rejected" 1 "$rc" +grep -q "not a valid calendar date" "$TMP/out.log" || { echo "FAIL: day-overflow error not found"; fail=1; } + +# --- Regression (cold review, 2026-10-15): an entry must be recognized even +# when `id` is not the FIRST field on the list item. Before the fix, only a +# literal `- id:` line started a new entry, so an item like `- owner: ...` +# followed by ` id: ...` silently never created an entry at all — the +# fields were dropped on the floor rather than raising a "missing id" error. +registry ' - owner: "alice" + id: "cli:id not first" + issue: 1 + signature: "x" + added: "__TODAY__" + expires: "__FUTURE__"' +results '[{"id":"cli:id not first","failed":true,"messages":["x"]}]' +rc=$(run_check) +check_exit "schema: an entry is parsed even when id is not the first field" 0 "$rc" +grep -q "EXCUSED: cli:id not first" "$TMP/out.log" || { echo "FAIL: reordered-field entry was not recognized"; fail=1; } + +# --- Regression (round 2 cold review, 2026-10-15): an item whose FIRST field +# is unknown/misspelled (not just reordered) must still start an entry, so +# `id` missing from a genuinely broken item surfaces as a schema error rather +# than silently discarding the whole item (and everything after it). +registry ' - iid: "cli:typo id field" + owner: "alice" + issue: 1 + signature: "x" + added: "__TODAY__" + expires: "__FUTURE__"' +results '[]' +rc=$(run_check) +check_exit "schema: an item starting with an unknown field still raises missing-id" 1 "$rc" +grep -q "INVALID \[id\]" "$TMP/out.log" || { echo "FAIL: unknown-first-field item was silently dropped instead of erroring"; fail=1; } + +# --- Regression (round 2): renewal_count present but malformed must error, +# not silently fall back to the same default (0) as "field absent". +registry ' - id: "cli:bad renewal count" + owner: "alice" + issue: 1 + signature: "x" + added: "__TODAY__" + expires: "__FUTURE__" + renewal_count: 1oops' +results '[]' +rc=$(run_check) +check_exit "schema: a malformed renewal_count (1oops) is rejected, not defaulted to 0" 1 "$rc" +grep -q "renewal_count must be a whole number" "$TMP/out.log" || { echo "FAIL: malformed-renewal_count error not found"; fail=1; } + +# --- Regression (round 2): a future `added` date must be rejected — a year +# typo (2099 instead of 2026) would otherwise excuse failures for decades +# while still satisfying the 14-day expiry-window check. +registry ' - id: "cli:future added" + owner: "alice" + issue: 1 + signature: "x" + added: "2099-01-01" + expires: "2099-01-15"' +results '[]' +rc=$(run_check) +check_exit "schema: a future added date is rejected" 1 "$rc" +grep -q "is in the future" "$TMP/out.log" || { echo "FAIL: future-added error not found"; fail=1; } + +# --- Regression (round 2): an empty " && "-joined signature clause (e.g. a +# trailing delimiter) must be rejected — message.includes('') is true for +# EVERY message, so an empty clause would excuse anything under that id. +registry ' - id: "cli:empty clause" + owner: "alice" + issue: 1 + signature: "known text && " + added: "__TODAY__" + expires: "__FUTURE__"' +results '[]' +rc=$(run_check) +check_exit "schema: a signature with a trailing empty && clause is rejected" 1 "$rc" +grep -q "empty \" && \"-joined clause" "$TMP/out.log" || { echo "FAIL: empty-signature-clause error not found"; fail=1; } + +# --- Regression (round 2): EVERY attached failure message must match the +# signature, not just one of several joined into a blob. A quarantined test +# with a known-signature failure PLUS a second, different failure message +# (e.g. two expect.soft() failures in one test) must block on the second. +registry ' - id: "cli:soft assertions" + owner: "alice" + issue: 1 + signature: "expected true to be false" + added: "__TODAY__" + expires: "__FUTURE__"' +results '[{"id":"cli:soft assertions","failed":true,"messages":["AssertionError: expected true to be false","AssertionError: a completely different, new regression"]}]' +rc=$(run_check) +check_exit "multi-message: a second, different failure message blocks even when the first matches" 1 "$rc" +grep -q "BLOCKING: cli:soft assertions" "$TMP/out.log" || { echo "FAIL: multi-message mismatch did not block"; fail=1; } + +# Sanity: when EVERY attached message matches, it's still excused. +results '[{"id":"cli:soft assertions","failed":true,"messages":["AssertionError: expected true to be false","AssertionError: expected true to be false (second assertion)"]}]' +rc=$(run_check) +check_exit "multi-message: all attached messages matching the signature still excuses" 0 "$rc" +grep -q "EXCUSED: cli:soft assertions" "$TMP/out.log" || { echo "FAIL: all-matching multi-message did not excuse"; fail=1; } + +# --- Ceiling: too many active entries blocks regardless of test outcome ---- +registry ' - id: "cli:a" + owner: "alice" + issue: 1 + signature: "x" + added: "__TODAY__" + expires: "__FUTURE__" + - id: "cli:b" + owner: "alice" + issue: 1 + signature: "x" + added: "__TODAY__" + expires: "__FUTURE__" + - id: "cli:c" + owner: "alice" + issue: 1 + signature: "x" + added: "__TODAY__" + expires: "__FUTURE__"' +results '[]' +rc=$(run_check 2) +check_exit "ceiling: 3 active entries over ceiling=2 blocks with no test failures" 1 "$rc" +grep -q "CEILING EXCEEDED" "$TMP/out.log" || { echo "FAIL: ceiling message not found"; fail=1; } + +# --- Regression (cold review, 2026-10-15): an unrelated package's broken +# entry must NOT fail a --package-scoped check. Before the fix, `check` with +# no package filter validated the WHOLE registry, so a schema error on an +# `ios:` entry (here: missing owner) would fail a cli-only gate run even +# though every cli test passed. --package cli must ignore it entirely; an +# unscoped `validate` must still catch it (whole-registry health is its job). +registry ' - id: "cli:unaffected cli test" + owner: "alice" + issue: 1 + signature: "x" + added: "__TODAY__" + expires: "__FUTURE__" + - id: "ios:some broken entry" + issue: 1 + signature: "x" + added: "__TODAY__" + expires: "__FUTURE__"' +results '[{"id":"cli:unaffected cli test","failed":false,"messages":[""]}]' +rc=$(run_check_pkg cli) +check_exit "cross-package: --package cli ignores a broken ios: entry" 0 "$rc" +grep -q "INVALID" "$TMP/out.log" && { echo "FAIL: --package cli should not even report the ios entry's error"; fail=1; } + +set +e +bun "$CLI" validate --registry "$TMP/registry.yaml" --skip-issue-check >"$TMP/out.log" 2>&1 +vrc=$? +set -e +check_exit "cross-package: unscoped validate still catches the broken ios: entry" 2 "$vrc" +grep -q "INVALID \[owner\] ios:some broken entry" "$TMP/out.log" || { echo "FAIL: validate did not report the broken ios entry"; fail=1; } + +# --- Regression (cold review, 2026-10-15): a runner-level crash (import +# error, zero tests discovered) exits non-zero while producing NO per-test +# assertion results at all. Before --runner-exit-code existed, that normalized +# to an empty results list and the gate reported a clean pass despite the +# runner itself having failed. +registry ' - id: "cli:irrelevant" + owner: "alice" + issue: 1 + signature: "x" + added: "__TODAY__" + expires: "__FUTURE__"' +results '[]' +set +e +bun "$CLI" check --results "$TMP/results.json" --registry "$TMP/registry.yaml" --skip-issue-check --package cli --runner-exit-code 1 >"$TMP/out.log" 2>&1 +rc=$? +set -e +check_exit "runner-exit-code: nonzero exit with zero attributable failures still blocks" 1 "$rc" +grep -q "produced no attributable per-test failure" "$TMP/out.log" || { echo "FAIL: runner-exit-code mismatch message not found"; fail=1; } + +# Sanity: runner-exit-code 0 (or omitted) with empty results is a clean pass. +set +e +bun "$CLI" check --results "$TMP/results.json" --registry "$TMP/registry.yaml" --skip-issue-check --package cli --runner-exit-code 0 >"$TMP/out.log" 2>&1 +rc=$? +set -e +check_exit "runner-exit-code: zero exit with empty results passes normally" 0 "$rc" + +if [[ "$fail" -eq 0 ]]; then + echo "ALL OK: flaky-quarantine negative controls" +else + echo "FAILURES ABOVE" +fi +exit "$fail" diff --git a/scripts/tooling/hapi-driver-rebuild.sh b/scripts/tooling/hapi-driver-rebuild.sh index 78ce0bd769..691603ad0a 100755 --- a/scripts/tooling/hapi-driver-rebuild.sh +++ b/scripts/tooling/hapi-driver-rebuild.sh @@ -554,7 +554,10 @@ if [[ "$VERIFY" -eq 1 ]]; then fi fi echo "Running tests..." - if ! (cd "$DRIVER" && "$BUN" run test); then + # Quarantine-aware for cli (config/flaky-quarantine.yaml); hub/web/shared/ + # relay unchanged. Same script CI uses (.github/workflows/test.yml) — one + # list, both gates. See docs/tooling/flaky-quarantine.md. + if ! BUN="$BUN" bash "$PRIMARY/scripts/tooling/flaky-quarantine-gate.sh" "$DRIVER"; then remat_rollback_live_tip "tests failed" exit 1 fi diff --git a/scripts/tooling/lib/flaky-quarantine-core.mjs b/scripts/tooling/lib/flaky-quarantine-core.mjs new file mode 100644 index 0000000000..02ca11e43b --- /dev/null +++ b/scripts/tooling/lib/flaky-quarantine-core.mjs @@ -0,0 +1,337 @@ +/** + * Flaky-test quarantine — pure decision core (no I/O, no network, no fs). + * + * GENERIC MECHANISM, FORK-ONLY DATA: this module and its CLI wrapper + * (../flaky-quarantine.mjs) contain no heavygee/hapi-specific logic — any repo + * with CI could use them unchanged. The data that gives them meaning + * (config/flaky-quarantine.yaml: which tests, which issues, which owners) is + * fork-specific. Keep that split when touching this file: if a change only + * makes sense because of something about THIS repo, it belongs in the YAML or + * the caller, not here. This is the shape an eventual standalone/upstream + * extraction would take with the YAML left behind. + * + * Design (see heavygee/hapi issue thread for #273 / #274, the two seed + * entries, and the review that shaped these rules): + * + * - A quarantined test still runs and still reports. This module never + * hides a failure; it only decides whether a failure may be ignored for + * gate-pass purposes. + * - Signature match -> excused (non-blocking). Signature MISMATCH -> BLOCKS. + * Id-matching alone cannot tell "the same old flake" from "a new bug that + * landed in the same test" — comparing the failure text closes most of + * that gap. A warning nobody must act on trains people to ignore red, so + * a mismatch blocks rather than merely warns. + * - An expired entry excuses nothing. It is not a grace period; it is the + * entry falling out of the active set, so the next failure under that id + * is treated as an ordinary (blocking) failure until someone renews (with + * a reason) or removes it. + * - A ceiling on the number of ACTIVE (valid, non-expired) entries is a + * forcing function, not a decoration: once active count exceeds it, the + * gate fails regardless of whether today's tests pass, until someone + * cleans the registry up. + */ + +/** Default max days an entry excuses anything before it must be renewed or removed. */ +export const DEFAULT_EXPIRY_DAYS = 14; + +/** Default ceiling on ACTIVE entries before the registry itself is the failure. */ +export const DEFAULT_CEILING = 8; + +const DATE_RE = /^\d{4}-\d{2}-\d{2}$/; + +function parseDate(s, field, errors, id) { + if (typeof s !== 'string' || !DATE_RE.test(s)) { + errors.push({ id, field, message: `${field} must be an ISO date (YYYY-MM-DD), got ${JSON.stringify(s)}` }); + return null; + } + const d = new Date(`${s}T00:00:00Z`); + if (Number.isNaN(d.getTime())) { + errors.push({ id, field, message: `${field} is not a valid calendar date: ${s}` }); + return null; + } + // JS Date silently normalizes day-overflow (2026-02-30 -> 2026-03-02) + // instead of failing — round-trip through toISOString so a mistyped date + // is rejected rather than quietly shifting the quarantine window. + if (d.toISOString().slice(0, 10) !== s) { + errors.push({ id, field, message: `${field} is not a valid calendar date (normalizes to ${d.toISOString().slice(0, 10)}): ${s}` }); + return null; + } + return d; +} + +function daysBetween(a, b) { + return Math.round((b.getTime() - a.getTime()) / 86_400_000); +} + +/** + * Minimal block-scalar-unaware YAML read for the flat `quarantine:` list — + * deliberately hand-rolled rather than pulling a parser dependency, matching + * the house style in hapi-soup-excised-check.mjs / config/soup-excised.yaml. + * Every field is a plain scalar on its own line; no nested lists except the + * `quarantine:` top-level one. + */ +const FIELD_NAMES = ['id', 'owner', 'issue', 'signature', 'added', 'expires', 'renewal_count', 'renewal_reason']; +const STRICT_INT_RE = /^-?\d+$/; + +/** + * Returns NaN (not a partial parse) for anything that isn't ENTIRELY digits — + * `Number.parseInt` happily reads "27O" as 27 and "1oops" as 1, which would + * let a typo'd issue number silently point at a different, unrelated open + * issue, or a typo'd renewal_count silently still grant a renewal. + */ +function strictInt(s) { + return STRICT_INT_RE.test(s) ? Number.parseInt(s, 10) : NaN; +} + +/** + * For issue/renewal_count: returns the parsed integer, or an { invalid } + * marker (NOT a silent default) when the scalar is present but not entirely + * digits — "renewal_count: 1oops" must be a schema error, not silently + * treated as "renewal_count absent" (which would default to 0 and pass). + */ +function coerceField(key, rawVal) { + const val = stripQuotes(rawVal.trim()); + if (key === 'issue' || key === 'renewal_count') { + const n = strictInt(val); + return Number.isNaN(n) ? { invalid: val } : n; + } + return val; +} + +export function parseQuarantineYaml(text) { + const entries = []; + let current = null; + // A `-`-prefixed line ALWAYS starts a new list item, regardless of which + // field (or even an unknown/misspelled one) comes first — matching only + // known field names here would let `- iid: "cli:test"` (typo'd `id`) + // start no entry at all, silently discarding every field that follows it + // instead of surfacing a "missing id" schema error. + const startRe = /^\s*-\s+([\w-]+):\s*(.*)$/; + const continuationRe = new RegExp(`^\\s+(${FIELD_NAMES.join('|')}):\\s*(.*)$`); + for (const raw of text.split('\n')) { + const line = raw.replace(/\s+$/, ''); + if (/^\s*#/.test(line) || !line.trim()) continue; + const start = line.match(startRe); + if (start) { + if (current) entries.push(current); + current = { id: null, owner: null, issue: null, signature: null, added: null, expires: null, renewal_count: 0, renewal_reason: null }; + if (FIELD_NAMES.includes(start[1])) { + current[start[1]] = coerceField(start[1], start[2]); + } + continue; + } + if (!current) continue; + const kv = line.match(continuationRe); + if (!kv) continue; + current[kv[1]] = coerceField(kv[1], kv[2]); + } + if (current) entries.push(current); + return entries; +} + +function stripQuotes(s) { + const t = s.trim(); + if (t.startsWith('"') && t.endsWith('"') && t.length >= 2) { + // YAML double-quoted scalars support backslash escapes — a signature + // that legitimately contains an embedded quoted type name (e.g. + // `expected non-nil value of type "UICollectionView"`) is written as + // \" in the file. Un-escaping \\ before \" matters so "\\\"" round-trips. + return t.slice(1, -1).replace(/\\\\/g, '\\').replace(/\\"/g, '"'); + } + if (t.startsWith("'") && t.endsWith("'") && t.length >= 2) { + return t.slice(1, -1).replace(/''/g, "'"); + } + return t; +} + +/** + * Schema + lifecycle validation. Pure and synchronous — issue-open-ness is + * NOT checked here (that needs a network call); pass `issueStates` (a map of + * issue number -> 'OPEN' | 'CLOSED') when the caller has fetched it, or omit + * to skip that one check (used by unit tests that must stay network-free). + * + * Returns { valid, errors, expired } where `valid` is entries that are + * schema-correct, issue-open (if checked), and NOT expired as of `today`. + * Schema-invalid or closed-issue entries are dropped from `valid` and fail + * closed (treated as if no quarantine exists for that id) rather than being + * silently kept — see module doc. + */ +export function validateEntries(entries, { today, issueStates } = {}) { + const now = today ?? new Date(); + const errors = []; + const valid = []; + const expired = []; + + for (const e of entries) { + const localErrors = []; + if (!e.id) localErrors.push({ id: e.id ?? '(no id)', field: 'id', message: 'id is required' }); + if (!e.owner) localErrors.push({ id: e.id, field: 'owner', message: 'owner is required — no anonymous quarantine entries' }); + if (!Number.isInteger(e.issue) || e.issue <= 0) { + localErrors.push({ id: e.id, field: 'issue', message: 'issue must be a positive GitHub issue number' }); + } + if (!e.signature) { + localErrors.push({ id: e.id, field: 'signature', message: 'signature is required — "failed once" with no captured failure text is not sufficient evidence' }); + } else if (typeof e.signature === 'string' && e.signature.split(' && ').some((part) => part.trim() === '')) { + // "known text && " or " && " alone produces an empty clause after + // split+trim, and `message.includes('')` is true for EVERY + // message — an empty clause silently excuses anything. + localErrors.push({ id: e.id, field: 'signature', message: `signature has an empty " && "-joined clause: ${JSON.stringify(e.signature)}` }); + } + const added = parseDate(e.added, 'added', localErrors, e.id); + const expires = parseDate(e.expires, 'expires', localErrors, e.id); + if (added && added.getTime() > now.getTime()) { + localErrors.push({ id: e.id, field: 'added', message: `added (${e.added}) is in the future — a typo'd year would otherwise excuse failures for decades while still satisfying the expiry window` }); + } + // A present-but-malformed renewal_count ("1oops") must not silently + // fall back to the same default (0) as a genuinely absent field — + // that would let a typo quietly skip renewal enforcement instead of + // failing the schema check. + if (e.renewal_count !== null && typeof e.renewal_count === 'object') { + localErrors.push({ id: e.id, field: 'renewal_count', message: `renewal_count must be a whole number, got ${JSON.stringify(e.renewal_count.invalid)}` }); + } + const renewalCount = Number.isInteger(e.renewal_count) ? e.renewal_count : 0; + if (renewalCount > 0 && !e.renewal_reason) { + localErrors.push({ id: e.id, field: 'renewal_reason', message: 'renewal_count > 0 requires a renewal_reason — renewal is a deliberate act with a reason attached, not a bumped date' }); + } + if (added && expires) { + const maxAllowedDays = DEFAULT_EXPIRY_DAYS + DEFAULT_EXPIRY_DAYS * renewalCount; + const actualDays = daysBetween(added, expires); + if (actualDays > maxAllowedDays) { + localErrors.push({ + id: e.id, + field: 'expires', + message: `expires is ${actualDays}d after added, but renewal_count=${renewalCount} only allows ${maxAllowedDays}d — bump renewal_count and add a renewal_reason to extend further`, + }); + } + if (actualDays <= 0) { + localErrors.push({ id: e.id, field: 'expires', message: 'expires must be after added' }); + } + } + if (issueStates && Number.isInteger(e.issue) && issueStates[e.issue] && issueStates[e.issue] !== 'OPEN') { + localErrors.push({ + id: e.id, + field: 'issue', + message: `issue #${e.issue} is ${issueStates[e.issue]}, not OPEN — a quarantined test whose issue is closed is either fixed or abandoned; remove the entry`, + }); + } + + if (localErrors.length > 0) { + errors.push(...localErrors); + continue; + } + + // `expires` parses as UTC midnight of that calendar date (parseDate + // above) — an entry must stay valid through the WHOLE of that date, + // not just its first instant, or "expires 2026-10-29" silently means + // "expires 2026-10-28T23:59:59.999Z" to anyone reading the date. + if (expires && now.getTime() >= expires.getTime() + 86_400_000) { + expired.push(e); + continue; + } + + valid.push(e); + } + + return { valid, errors, expired }; +} + +/** + * A `signature` may name more than one required substring, joined by ` && `, + * ALL of which must appear for a match. This exists because a single + * substring is a choice between two different gaps: generic assertion text + * alone ("expected true to be false") can match a DIFFERENT assertion in the + * same test; a stack-trace call site alone ("agentCliGuard.test.ts:309:") + * can match a DIFFERENT kind of failure that happens to throw from that same + * line (e.g. the function under test starts throwing instead of returning + * the wrong boolean). Requiring both closes both gaps at once (cold review, + * 2026-10-15 — see config/flaky-quarantine.yaml's cli: entry for a real one). + */ +function matchesSignature(message, signature) { + if (typeof message !== 'string') return false; + const clauses = signature.split(' && ').map((p) => p.trim()); + // Defense in depth: validateEntries already rejects an empty clause at + // schema time (an empty clause would otherwise match any message), but + // this function has no other guarantee its caller went through that + // check first — never let a blank clause silently pass. + return clauses.every((part) => part.length > 0 && message.includes(part)); +} + +/** + * A quarantined test can legitimately fail with MORE THAN ONE message in one + * run — multiple `expect.soft()` failures, or a known assertion plus an + * unrelated hook failure. Requiring EVERY attached message to match is what + * stops a second, different regression from riding along: joining all + * messages into one string and matching the signature anywhere in that + * blob would let one real match excuse an unrelated failure sitting right + * next to it (cold review, 2026-10-15 — cli/src/agent/permissionJson.test.ts + * already uses soft assertions, so this is a real, not hypothetical, shape). + */ +function matchesAllMessages(messages, signature) { + const list = messages.length > 0 ? messages : ['']; + return list.every((m) => matchesSignature(m, signature)); +} + +/** + * The actual gate decision. `results` is runner-agnostic: + * [{ id: string, failed: boolean, messages: string[] }] + * `validEntries` must already be schema-valid and non-expired (the output of + * validateEntries().valid) — this function does not re-check lifecycle. + * + * `runnerExitCode`, when provided and non-zero, guards against a failure mode + * none of the per-test bookkeeping above can see: a runner-level crash + * (import error, uncaught rejection, zero tests discovered) that exits + * non-zero while producing NO per-test assertion results at all. Without + * this check, `results` would be empty, the loop above would find nothing to + * block on, and the gate would report a clean pass despite the runner itself + * having failed (cold review, 2026-10-15). Adapters should ALSO represent a + * suite-level/collection failure as its own failed result with no matching + * registry id (so it blocks as "not quarantined") rather than relying on + * this as the only backstop — see from-vitest-json's suite-failure handling. + */ +export function evaluate({ validEntries, errors = [], expired = [], results, ceiling = DEFAULT_CEILING, runnerExitCode = 0 }) { + const byId = new Map(validEntries.map((e) => [e.id, e])); + const blocking = []; + const excused = []; + + for (const r of results) { + if (!r.failed) continue; + const messages = r.messages ?? []; + const entry = byId.get(r.id); + if (!entry) { + blocking.push({ id: r.id, reason: 'not quarantined', messages }); + continue; + } + if (matchesAllMessages(messages, entry.signature)) { + excused.push({ id: r.id, issue: entry.issue, owner: entry.owner }); + } else { + blocking.push({ + id: r.id, + reason: 'quarantined signature mismatch — treated as a new failure', + messages, + expectedSignature: entry.signature, + }); + } + } + + if (runnerExitCode !== 0 && results.filter((r) => r.failed).length === 0) { + blocking.push({ + id: '(runner)', + reason: `test runner exited ${runnerExitCode} but produced no attributable per-test failure — cannot quarantine an unattributed failure (collection error, crash, or zero tests discovered)`, + message: '', + }); + } + + const activeCount = validEntries.length; + const ceilingExceeded = activeCount > ceiling; + + return { + blocking, + excused, + activeCount, + ceiling, + ceilingExceeded, + schemaErrors: errors, + expiredEntries: expired, + pass: blocking.length === 0 && errors.length === 0 && !ceilingExceeded, + }; +}