fix: Auto-detected flaky tests (merge-group) — review for quarantine (#2740) - #2769
zaxbysauce wants to merge 2 commits into
Conversation
Quarantine the three auto-detected flaky test paths from merge-group CI run 34726206593 (filed by flake-detection workflow run 34728138233, issue #1782) with OWNER/EXPIRY metadata per issue #2477: - tests/unit/services/evidence-summary-adversarial.test.ts — general ledger (scripts/ci/quarantined-tests.txt). Flake: unit (ubuntu-latest, 2) shard, attempts 1+2 failed, passed on retry 2. The 'should not block on slow event handlers' case registers a real 100ms setTimeout handler and asserts expect(duration).toBeLessThan(500) at tests/unit/services/evidence-summary-adversarial.test.ts:882 — under merge-group runner CPU contention the publish exceeds the 500ms budget. No ubuntu-specific ledger exists, so the general ledger is used, matching the #2368 dispatch-lanes precedent. - tests/unit/utils/bun-compat-exit-first-2530.test.ts — macOS ledger (scripts/ci/quarantined-tests-macos.txt). Flake: unit (macos-latest, 4) shard, passed on retry 1. Shells out to bun build + node probes under hard 5s timeouts (PROBE_TIMEOUT_MS at line 9); a runner stall >5s on cold build or CPU contention trips the probe timeout. - tests/unit/commands/promote-registration.test.ts — macOS ledger. Flake: unit (macos-latest, 6) shard, passed on retry 1. beforeEach mutates process.env.HOME/LOCALAPPDATA/XDG_DATA_HOME to redirect the hive knowledge path into a fresh mkdtempSync temp dir (cleaned in afterEach) — the classic env-mutation + temp-dir teardown surface. All three files pass locally (62 pass / 0 fail in 4.51s), confirming environment-sensitivity rather than a logic bug; the ci.yml retry loop discards attempt-1 output when a retry passes, so no failing assertion text exists to drive a root-cause fix. The unit (windows-latest, 4) failure in the same run was close-active-state-unlink-retry.test.ts (unrelated, not flagged). Verification: - ci.yml grep|sort|comm consumer pipeline: all 3 paths present in discovery (3501 files), all 3 removed from gated-tests; no same-basename siblings affected. - scripts/check-invariants.ts Check 7: all entries carry OWNER/EXPIRY (EXPIRY 2026-10-14, 30 days out). - bun run test:unit:ci <files>: 1 quarantined on Linux (general ledger), 2 run (macOS-scoped ledger) — matches platform semantics. - tests/unit/scripts/ci/ci-yml-integration.test.ts: 31/31 pass. - tests/unit/scripts/ci/ci-yml-windows-quarantine.test.ts: 4/4 pass. - tests/unit/scripts/check-quarantine-metadata.test.ts + ci-coverage- sharding.test.ts: 37/37 pass. - bun run check:pending-fragment: OK (fragment present, all refs verified). Refs: #2740 (this issue), #1782 (flake-detection workflow), #2477 (OWNER/EXPIRY grammar), #1908 (general-ledger retirement precedent), #2368 (general-ledger ubuntu precedent), #2738 (macOS ledger precedent).
Add tests/unit/scripts/ci/ci-yml-quarantine-2740.test.ts with 9 regression tests (3 per quarantined path, mirroring the #2761 / #2368 / #2185 precedents) covering the three entries the previous commit added for the flaky-test candidates the merge-group flake-detection workflow (#1782) auto-filed in issue #2740: - tests/unit/services/evidence-summary-adversarial.test.ts -> general ledger - tests/unit/utils/bun-compat-exit-first-2530.test.ts -> macOS ledger - tests/unit/commands/promote-registration.test.ts -> macOS ledger Each path gets three guards: 1. active entry in its expected ledger (active-entry extraction mirrors the ci.yml grep -vE pipeline exactly; CRLF normalized) 2. scoped to that ledger only - no duplicate in the general/macos/windows/integration ledgers (cross-ledger duplicates would falsely imply per-OS evidence) 3. path exists on disk and is reachable by the ci.yml unit find chain (a typo would silently no-op the quarantine) RED/GREEN verification: - With entries in place: 9/9 pass (~250 ms). - With entries removed (sed deletion): 3/9 fail (the active-entry tests), confirming the tests drive the ledger fix rather than pass vacuously. - Full fast suite: ci-yml-integration + ci-yml-windows-quarantine + new file 44/44 pass; check-quarantine-metadata + ci-coverage-sharding 37/37 pass; the three quarantined files themselves run green locally (62 pass, clean TMPDIR). - bun x tsc --noEmit: clean. biome check --write applied. check:invariants (incl. Check 7 OWNER/EXPIRY): all pass. Also corrects the pending release fragment's stale claim that no test code changed. Refs: #2740, #1782 (flake-detection workflow), #2477 (OWNER/EXPIRY grammar), #2761/#2368/#2185 (consumer-side quarantine-pin test precedents).
zaxbysauce
added
the
auto-fix-attempt
Drift check reportFound 2 drift finding(s): 0 error, 0 warning, 2 notice. required-check-contract (2)
|
There was a problem hiding this comment.
🟡 Changes recommended
A new test comment cites a nonexistent recipe path (.hermes/skills/auto-fix-issue/references/flaky-test-quarantine.md) instead of the canonical docs/testing/test-stability.md, which should be corrected.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR quarantines the three merge-group flaky tests reported in issue #2740 by appending OWNER/EXPIRY-annotated entries to the CI quarantine ledgers, so CI's unit/coverage shards stop failing on environment-sensitive (passed-on-retry) flakes. It follows the repository's established quarantine convention (structured metadata enforced by check-invariants Check 7, per-OS ledger routing) and is a data-only change with no source or hook modifications.
Changes:
- Appends
evidence-summary-adversarial.test.tsto the general ledger (ubuntu flake) andbun-compat-exit-first-2530.test.ts+promote-registration.test.tsto the macOS ledger, each with a full# OWNER:/# EXPIRY: 2026-10-14block. - Adds a consumer-side pinning test (
ci-yml-quarantine-2740.test.ts) that mirrors ci.yml's active-entry extraction and asserts each path is present in its ledger, scoped there only, and exists on disk. - Adds the mandatory pending release fragment documenting the quarantine and its rationale.
File summaries
| File | Description |
|---|---|
scripts/ci/quarantined-tests.txt |
Adds the ubuntu flake (evidence-summary-adversarial.test.ts) to the general ledger with OWNER/EXPIRY metadata. |
scripts/ci/quarantined-tests-macos.txt |
Adds the two macOS flakes with OWNER/EXPIRY metadata. |
tests/unit/scripts/ci/ci-yml-quarantine-2740.test.ts |
New pinning test verifying ledger presence, cross-ledger scoping, and on-disk existence; a comment cites a nonexistent recipe path. |
docs/releases/pending/issue-2740-three-merge-group-flaky-tests-quarantine.md |
Mandatory release fragment describing the quarantine, per-OS routing, and EXPIRY caveats. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // run 34726206593; per the recipe in | ||
| // .hermes/skills/auto-fix-issue/references/flaky-test-quarantine.md, the |
Swarm PR Review — #2769Scope: Method3 parallel explorer lanes (correctness+tests / security+reliability+compatibility / intent+architecture) → 7 candidates → independent fresh-context reviewer (re-derived every claim against source, ran Obligation mapping
FindingsF-01 — LOW — General-ledger placement silences the file across all OSes + the coverage gate, not just ubuntu F-02 — LOW — F-03 — LOW — Grace-window phrasing misdescribes Check 7's actual state F-04 — ADVISORY — Test title overclaims F-05 — ADVISORY — Dangling doc citation Disproved candidateAn initial candidate claiming the coverage-gate side effect was "undocumented" and "unpinned" was disproved by critic re-verification: No discrepancy foundIssue #2740 open/closed-claim integrity, cross-OS ledger scoping (general ledger applies unconditionally, macOS/Windows ledgers correctly Verdict: APPROVE_WITH_NOTESZero unresolved CRITICAL/HIGH findings; all findings are LOW/ADVISORY and non-blocking. All blocking obligations MET. No changes required before merge; F-01–F-03 are worth a quick follow-up but don't block this PR. 🤖 Generated with Claude Code — swarm-pr-review (Profile B: 3 explorer lanes → independent reviewer → critic challenge) |
Closes #2740
Summary
|Scope: Quarantines the three merge-group flaky tests from issue #2740 by appending OWNER/EXPIRY-annotated entries to
scripts/ci/quarantined-tests.txt(evidence-summary-adversarial, ubuntu evidence) andscripts/ci/quarantined-tests-macos.txt(bun-compat-exit-first-2530, promote-registration), plus a consumer-side pinning test and the mandatory release fragment.|Verdict: APPROVE
|
|### Findings
|
|#### LOW — Stale general-ledger header contradicts the new entry
|- Location:
scripts/ci/quarantined-tests.txt:10|- Evidence: The pre-existing header comment reads "No active global test quarantines. Issue #1908 retired the tracked backlog." immediately above the first active general-ledger entry since #1908 (
tests/unit/services/evidence-summary-adversarial.test.ts). Similarly, the macOS ledger header "No active macOS-only quarantines besides the entry below." now precedes three entries.|- Why it matters: Cosmetic only — ci.yml's
grep -vE '^\s*#|^\s*$'and Check 7 ignore comment prose, and no test asserts these header sentences — but a future triager skimming the header could mis-read ledger state.|- Suggested fix: In a follow-up (or this PR), reword the general-ledger header to match the windows ledger's machine-checked
# STATUS: N active entriesconvention.|
|### What I Checked
|- Target file present in diff: yes (all three issue-listed paths are active entries; general ledger for the ubuntu flake, macOS ledger for the two macOS flakes — verified against the actual consuming pipelines, not just the issue text)
|- New/updated test exercises the changed code path: yes — RED-check performed: with the ledger entries reverted to origin/main,
tests/unit/scripts/ci/ci-yml-quarantine-2740.test.tsfails exactly its 3 "is an active entry" assertions (6 pass / 3 fail); green at HEAD (9 pass / 0 fail). No mocks — it reads the real ledger files.|- Scope creep beyond the issue: no — ledger appends, one pinning test, one release fragment; zero source changes. Second commit (
28f8cb97c) follows the repo's two-commit quarantine pattern (#2761 precedent).|- Ledger routing verified against consumers, not convention: replicated ci.yml's
grep | sort -u | comm -23for the ubuntu unit shape, macOS unit shape, and the coverage-gate shape (run-coverage-gate.sh:85reads only the general ledger — this is why the ubuntu flake correctly lands there despite the recipe's "cross-OS only" phrasing; matches the #2368 precedent in commit efb4a47). evidence-summary-adversarial is excluded from ubuntu unit AND coverage; the two macOS-ledger files remain gated on ubuntu (intended — macOS ledger applies on macOS runners only, per ci.yml'sRUNNER_OSbranch at .github/workflows/ci.yml:632).|- Ledger comments' factual citations spot-checked against the test files:
PROBE_TIMEOUT_MS = 5_000attests/unit/utils/bun-compat-exit-first-2530.test.ts:9; wall-clockexpect(duration).toBeLessThan(500)in the slow-event-handler case oftests/unit/services/evidence-summary-adversarial.test.ts; HOME/LOCALAPPDATA/XDG_DATA_HOME mutation +mkdtempSyncintests/unit/commands/promote-registration.test.tsbeforeEach/afterEach — all present as described.|- Stale-checks: none of the three files was modified after the issue's triggering run (2026-09-13) — last commits 2026-06-29, 2026-09-06, 2026-07-24 — so no root fix landed between filing and quarantine (the #2073 failure mode). Paths are not core-tree (Rule C covers scope/agents/hooks only) and were not already quarantined (Rule A) — no
ALREADY_RESOLVEDcondition.|- Check 7 metadata grammar: every new entry has a contiguous comment block with
# OWNER:and a well-formed# EXPIRY: 2026-10-14(in the future — no grace-window warning).bun run check:invariantsexits 0 with "All active quarantine entries carry OWNER + EXPIRY metadata." (the spawn-timeout WARNINGs in its output are pre-existingsrc/debt, untouched by this diff).|- Regression sweep over ledger consumers:
ci-yml-integration.test.ts,check-quarantine-metadata.test.ts,ci-yml-windows-quarantine.test.ts(including the windows STATUS-count assertion — windows ledger untouched),ci-coverage-sharding.test.ts— 72 pass / 0 fail.|- Formatting hazards: no trailing whitespace on any active entry line, so the test's
.trim()-normalized comparison cannot diverge from ci.yml's untrimmedgrep -qxF-style matching; no same-basename sibling files exist for any of the three paths.|
|### Anti-patterns scanned
|- silent-except: n/a (no executable code changed)
|- mock-without-call: n/a (pinning test is file-read-based, proven by the RED-check)
|- off-by-one: n/a
|- signature-break: n/a
|- scope-creep: no
|- stale-docs: one LOW finding (ledger header prose), release fragment content verified accurate
|
|### Confidence
|high — the fix is a data-only change whose every factual claim survived adversarial verification (consumer pipelines, cited line numbers, precedent commit, RED-check); only a comment-prose nit remains, and only a demonstrated suppression of a healthy gated test would change the verdict.
Invariant audit
Test plan
See the linked issue and the change summary.