Repository navigation
Conversation
Operator-directed: flaky tests were adjudicated one red light at a time, costing a week on PR #222 (iOS app-build, 3 different failures, #273) and tripping the #270 remat gate (agentCliGuard.test.ts, #274). A tolerated red teaches everyone red doesn't mean red. config/flaky-quarantine.yaml is the single source of truth both the remat gate and CI consult (scripts/tooling/flaky-quarantine-gate.sh). Three rules: known id + matching failure signature excuses; a DIFFERENT signature under the same id blocks (closes most of the "new bug in the same test" gap); an expired entry excuses nothing. A ceiling on active entries (default 8) fails the gate independent of test outcomes once exceeded. Wired for cli (vitest JSON reporter -> adapter -> check) in both hapi-driver-rebuild.sh --verify and .github/workflows/test.yml (fork-only; upstream keeps plain `bun run test`). The iOS xcodebuild-log adapter is built and proven against the real PR #222 failure logs but NOT wired into ios.yml/test-transcript.sh yet — no macOS runner available here to verify the shell integration; wiring it blind risked turning a flake into a hard break on a live gating workflow. scripts/tooling/flaky-quarantine.test.sh fires three negative controls (matching signature excuses, mismatched signature blocks, expired entry blocks) plus schema/ceiling checks, picked up automatically by the existing tooling-tests CI job. via HAPI (https://hapi.run) Co-Authored-By: HAPI <noreply@hapi.run>
Cold review caught two Critical and two Important issues before this
reached a live gate:
- Cross-package coupling: `check` validated the WHOLE registry regardless
of which package actually ran, so an unrelated ios: entry going invalid
(e.g. its issue closing) would fail a cli-only gate for a reason that
has nothing to do with cli. Added `--package PREFIX` to `check`, scoping
both validation and excuse/block matching to that namespace; the ceiling
stays deliberately whole-registry (validate), run as its own explicit
step in flaky-quarantine-gate.sh.
- The one live cli: entry's signature ("expected true to be false") was
generic enough to also match a second, unrelated assertion two lines
below in the same test — a real bug there would have been silently
excused. Anchored the signature to the failing stack frame
(agentCliGuard.test.ts:309:) instead; verified the real flake still
excuses and a synthetic line-310 failure now correctly blocks.
- Expiry had a sub-day off-by-one: an entry expired at the first instant
of its `expires` date rather than at the end of it. Fixed to stay valid
through the whole calendar day.
- The new CI step's `gh issue view` (issue-must-be-open check) had no
GH_TOKEN wired, unlike every other gh-using workflow in this repo —
would have silently failed the gate on every run. Added the same
GH_TOKEN/GITHUB_TOKEN pattern used elsewhere.
Added a regression test for the cross-package fix (an ios: schema error
no longer fails a --package cli check; unscoped validate still catches
it) and a direct verification of the expiry boundary.
via HAPI (https://hapi.run)
Co-Authored-By: HAPI <noreply@hapi.run>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d6b4c7d50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…2 x6)
- Preserve non-assertion vitest failures: a runner-level crash (import
error, zero tests discovered) exiting non-zero with no per-test
assertions previously normalized to an empty results list and the gate
reported a clean pass. check now takes --runner-exit-code and
unconditionally blocks when it's non-zero with nothing attributable.
- Require the known assertion text AND its call site to both match, not
either alone: signature now supports " && "-joined required substrings.
A location-only anchor (this session's own prior fix) could still
mis-excuse a genuinely different failure that happens to throw from the
same line; the live cli: entry now requires both.
- Reject day-overflow dates (2026-02-30 normalizes to 2026-03-02 under
plain `new Date()` instead of failing) via a round-trip check.
- Make quarantine test fixtures independent of wall-clock expiry — they
used the same near-term dates as the real registry, so every "currently
active" scenario (including the ceiling fixture) would silently flip to
expired/inactive on 2026-10-30 and permanently redden the tooling-tests
CI job. Dates are now computed relative to the actual test run.
- Honor an inherited $BUN instead of bare `bun`/`bunx` throughout the gate
script; hapi-driver-rebuild.sh now passes it through explicitly (it was
never exported, so the previous refactor into a separate script silently
dropped a custom bun path despite the rebuild script itself resolving it
via "${BUN:-$HOME/.bun/bin/bun}").
- Reject partially-numeric issue/renewal_count values (parseInt("27O") ===
27) — now requires the entire scalar to match integer syntax.
- Parse every quarantine list item regardless of field order: only a
literal `- id:` line started an entry before, so `id` reordered or
misspelled silently dropped the whole entry instead of raising a schema
error.
- Default issue-open check now derives the repo from the `origin` git
remote (overridable with --repo) instead of hardcoding heavygee/hapi,
matching the module's own "generic mechanism" header.
Six new regression tests cover all of the above; full suite now 16/16.
Re-verified end-to-end against the real registry, the real PR #222/#270
captured failures, and a full gate run on the driver tree.
via HAPI (https://hapi.run)
Co-Authored-By: HAPI <noreply@hapi.run>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26d4180476
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… x2, P2 x4)
- Represent suite-level vitest failures (collection/import errors, zero
assertions) as their own normalized result instead of silently vanishing
when results also contains an unrelated quarantined assertion failure —
the id never matches a registry entry, so it blocks as "not quarantined"
by the same path a real failure would.
- Require EVERY attached failure message to match the signature, not just
one joined into a blob: normalized results now carry `messages: string[]`
end to end. A quarantined test with a known-signature failure plus a
second, different message (multiple expect.soft() failures is a real
shape already used in cli/src/agent/permissionJson.test.ts) now blocks
on the second message instead of being excused by the first.
- Start a YAML list item on ANY `- <key>:` marker, not only known field
names — `- iid: "..."` (typo'd id) now still creates an entry (which
then correctly raises "id is required") instead of silently discarding
the whole item and everything indented under it.
- Distinguish "renewal_count absent" (defaults to 0, fine) from "present
but malformed" (now a schema error) — coerceField returns an {invalid}
marker instead of collapsing both cases to the same default.
- Reject a future `added` date — a year typo (2099 vs 2026) previously
still satisfied the 14-day added/expires window and the entry read as
currently active.
- Reject an empty " && "-joined signature clause (trailing delimiter, or
an all-whitespace clause) — message.includes('') is true for every
message, so an empty clause silently excused anything under that id.
matchesSignature also defends against this directly, independent of
validation having run first.
Six new regression tests cover all of the above (22/22 total), including
one against the REAL vitest collection-error JSON captured from a live
`import of a nonexistent module` run, not just synthetic fixtures.
Re-verified end-to-end against the real registry and the real PR #222/#270
captured failures with the new messages[] shape.
via HAPI (https://hapi.run)
Co-Authored-By: HAPI <noreply@hapi.run>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Operator-directed: flaky tests were being adjudicated one red light at a time, each time an agent escalating to the operator for a judgement call on a test it didn't write. This tracked a week-long block on PR #222 (iOS
app-build, 3 different failures, #273) and a tripped remat gate (agentCliGuard.test.ts, #274) back to their actual CI evidence, filed both as issues, and builds the quarantine mechanism to stop future instances from needing a human in the loop.A quarantined test still runs and still reports. This is a tracked quarantine, not an ignore list:
Full design rationale:
docs/tooling/flaky-quarantine.md.What's wired vs. drafted
hapi-driver-rebuild.sh --verifyand.github/workflows/test.yml(fork-only — upstream keeps plainbun run test), via the sharedscripts/tooling/flaky-quarantine-gate.sh.from-xcodebuild-logadapter is built and proven against the three real PR feat(cli): land upstream #1564 display_links on the fork #222 attempt logs, but not wired intoios.yml/test-transcript.sh— no macOS runner available to verify the shell integration (bun-on-runner, PIPESTATUS behavior). Wiring it blind risked turning a flake into a hard break on a live gating workflow.Review history
A cold review (full diff trace, empirically tested against the real vitest 4.0.16 reporter source and the hand-rolled YAML parser) caught two Critical and two Important issues before this reached a live gate, all fixed in the second commit:
checkvalidated the whole registry regardless of which package ran, so an unrelatedios:entry going invalid (e.g. its issue closing) would fail a cli-only gate for an unrelated reason. Fixed with--package PREFIXscoping; the ceiling stays deliberately whole-registry via a separatevalidatestep.cli:entry — would have silently excused a real, different bug in a sibling assertion two lines below. Anchored to the failing stack frame instead; verified against both the real flake (excuses) and a synthetic sibling failure (now blocks).gh issue viewcall had noGH_TOKENwired — would have silently failed the gate on every run. Fixed to match the pattern already used elsewhere in this repo.Verification
scripts/tooling/flaky-quarantine.test.sh— 11 scenarios, all green: the three required negative controls (matching signature excuses / mismatched signature blocks / expired entry blocks), schema validation, ceiling enforcement, and a regression test for the cross-package fix. Picked up automatically by the existingtooling-testsCI job.flaky-quarantine-gate.sh) end-to-end against the driver tree (hub/web/shared/relay + cli's real 3578-test vitest suite) — passes.bun scripts/tooling/flaky-quarantine.mjs validate— confirms flaky: iOS app-build UI tests fail non-deterministically (3 different failures on PR #222) #273/flaky: agentCliGuard.test.ts crash-stale-marker test races under full-suite timing, tripped the #270 remat gate #274 are open).Test plan
bash scripts/tooling/flaky-quarantine.test.shbun scripts/tooling/flaky-quarantine.mjs validateflaky-quarantine-gate.shrun against the driver tree