Repository navigation
fix(quarantine): quarantine the two unit-lane flakes - one owned, one an explicit rule-1 exception - #288
Merged
Conversation
Both tests went red on this PR's own CI while it changed no Java at all, and both have a written fix sitting in an open PR. That is precisely the case the quarantine lane exists for: "tests that are red on master's gating CI when the fix lives in another, open PR". Leaving them red is the option the docs explicitly rule out - ambiguous checks and error-prone merge decisions - and @disabled is the other, which loses the signal entirely. Quarantine keeps them running in the non-gating lane while taking them out of the gates. WHY THEY QUALIFY Rule 1, no quarantine without diagnosis - both are diagnosed, in docs/inflight/test-untracked-ci-flakes.md: PCMetricsTest.metricsRegisterBinding compares a registry gauge against an expectation built from a test-side counter snapshot taken earlier in the method. Two independently-advancing values, read at different instants, with nothing holding processing still between them. Seen as expected 203.0 but was 207.0 - four more records completed in the gap, so the metric was MORE current than the expectation testing it. OffsetEncodingBackPressureTest.backPressureShouldPreventTooManyMessagesBeing- QueuedForProcessing sleeps out the static retry delay instead of awaiting the retry event, then asserts on a count still in motion. Fails as ConditionTimeout, expected 139 but was 136 within 30 seconds, when the runner is loaded enough that the sleep expires first. Rule 2, master-state not PR-state - both fail regardless of PR content. This PR contains no Java and no pom.xml, and the pair fired on two consecutive runs of one unchanged commit, each time a different one of them. fixedBy is #265 for both, verified against its diff rather than its title: it replaces the Thread.sleep(1000) above the metrics assertions with an await().untilAsserted(...) on the trailing meters, and swaps sleepQuietly(DEFAULT_STATIC_RETRY_DELAY) for await().atMost(ofSeconds(30)).until(() -> attempts.get() >= 2). flapping = true on both. They pass most runs - the unit lane was green on eight consecutive PR runs across three branches the same day - so a pass proves nothing and must not post the merge-blocking "your fix landed" thread that deterministic quarantines get. WHY THEY ARE VISIBLE NOW #224 removed the surefire retry that was absorbing them. They were always failing at this rate; the retry was buying the green. This is the ledger entry from that PR getting its first real use. The copyright header on OffsetEncodingBackPressureTest gains the modifications line, required once an upstream-derived file is touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8He6kk23K3gN4qtojL9PE
Two enforced gates disagreed, and the registry could not satisfy both. `bin/check-quarantine-owners.sh` and `bin/quarantine-lane-report.sh` both parsed the owner marker with `grep -oE 'Owner: PR #[0-9]+'`, so the registry had to write a BARE number. `bin/check-issue-refs.sh` rejects a bare `#NN` on an added line, because the fork's numbers sit inside confluentinc's range and a bare one is a coin flip about which repo is meant. Adding the first two entries to this registry therefore failed the issue-ref gate by construction - not through carelessness, but because the two formats were mutually exclusive. The previous commit worked around that with the issue-ref gate's documented `issue-refs: N/A` opt-out. That was the wrong instrument: the opt-out exists for references that genuinely need no qualifier, not for a format conflict that can be fixed. An N/A left in place would also have quietly become permanent, since every future entry hits the same wall. Both parsers now accept `Owner: PR #NN`, `Owner: PR astubbs#NN`, and `Owner: PR astubbs/parallel-consumer#NN`, and the number is extracted from the `#NN` tail rather than the whole match, so a digit inside a qualifier cannot win. The bare form still parses, so existing entries keep working; the registry's format spec now asks for the qualified one and says why. Verified three ways rather than by inspection: qualified `Owner: PR #265` -> resolves, owner claim verified bare `Owner: PR #265` -> still resolves (back-compat intact) bogus `Owner: PR astubbs#999999` -> ERROR, PR does not exist The third is the one that matters. A parser that silently failed to match would also report no error, and would look identical to a pass - so the control had to show a wrong number being CAUGHT, not just a right number being accepted. Both parsers are changed together and carry a comment saying so; they read the same marker and drifting apart would mean the lane reporter attributing an entry to a different PR than the audit checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8He6kk23K3gN4qtojL9PE
… rule-1 exception The previous commit (cherry-picked from #286) quarantined both flakes under one diagnosis, and half of that diagnosis was wrong: #286 itself reverted the OffsetEncodingBackPressureTest quarantine after review showed the attributed cause - the retry-delay sleep - runs AFTER the failing assertion, so #265 cannot fix it. This commit re-lands that quarantine with metadata that tells the truth instead. WHAT THE ANNOTATION NOW SAYS The test is UNDIAGNOSED. It fails as ConditionTimeout at the getHighestSeenOffset() assertion: the committed high-water mark never reaches expectedHighestSeen (139), with a different actual each run (136 and 132 seen). No fixedBy - the entry is unowned, and check-quarantine-owners.sh flags it advisory ("diagnosed-but-unowned, find it an owner") rather than passing it silently. WHY QUARANTINE SOMETHING UNDIAGNOSED AT ALL Rule 1 says undiagnosed red stays red, on purpose. The repository owner decided to make an explicit exception: at 4/45 this is the most frequent tracked flake, and with the surefire retry gone (#224) it was failing outright and blocking every PR in the fork. The exception is recorded as an exception - in the annotation reason, the registry entry, and the ledger - so the pressure to diagnose is documented rather than quietly deleted. THE LEDGER KEEPS THE DIAGNOSIS ALIVE docs/inflight/test-untracked-ci-flakes.md is brought up to the #286 version (which records the wrong-diagnosis story and the void-control trap) and extended: the "do not quarantine it" verdict becomes "quarantined anyway, by explicit exception", and the open lead is written down - the UNVERIFIED hypothesis that expectedHighestSeen assumes all threshold-crossing extra records get polled while crossing the threshold is what stops intake, making the target unreachable rather than late. Falsification path included: if the high-water mark eventually reaches 139 given long enough, the hypothesis is dead. Verified: bin/check-quarantine-registry.sh green (2 entries), bin/check-quarantine-owners.sh green with the expected advisories, and the 38 quarantine-infrastructure tests (contract + script) pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F2MAyo5JDR5ZdWPGCxDCzY
…sync the rules to the mechanism The registry's rule 4 read "Every entry needs an owning fix PR", which is tighter than what the checks actually enforce: fixedBy defaults to empty, check-quarantine-owners.sh treats a missing owner as an ADVISORY and exits green, and only WRONG owner claims hard-fail (closed-unmerged owner, or merged owner with the test still quarantined). The repository owner judged the written rule too tight - unowned entries cannot be forgotten anyway, because the lane report surfaces every quarantined test on every PR and the release guard blocks releases while the registry is non-empty. Reworded in every place the rule is stated, so the docs describe the mechanism rather than a stricter fiction of it: - docs/quarantined-tests.md rule 4: owner is the goal, not a precondition; spells out what the checks hard-fail versus flag. - docs/testing.md: the owner-claim check applies "when an entry names one"; rule 3 gains the same goal-not-precondition note. - AGENTS.md ledger table: "and, when one exists, their owning fix PR". Rule 1 is synced in the same pass: the registry now contains a live owner-granted exception (the backpressure entry), so the rule's statement in the registry, docs/testing.md, and the @Quarantined javadoc records that the repository owner can grant one - the entry must declare itself an exception and keep the failure signature and open diagnosis. Verified: registry and owner checks green with the expected advisories, and the 38 quarantine contract/script tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F2MAyo5JDR5ZdWPGCxDCzY
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its |
![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: 94b95fd0b5
ℹ️ 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".
Owner
Author
|
@claude review this |
|
Claude finished @astubbs's task in 1m 47s —— View job Review complete
The mechanism and rewording changes are coherent overall (registry rule 4 wording now matches what the checks actually enforce, the two quarantine annotations match their 1. This PR's own fix, fixedBy = "astubbs#265",but the annotation-vs-registry cross-check still only recognizes the old unqualified form: declared=$(grep -oE 'fixedBy = "PR #[0-9]+' "$file" | grep -oE '[0-9]+' | head -1 || true)
2. "diagnosed-but-unowned" is the wrong label for a recorded rule-1 exception
which asserts the opposite of this entry's actual state and points at the wrong next step (find an owner, vs. finish the diagnosis). Neither issue affects the two hard-fail (ERROR) paths — they're both advisory-text/cross-check gaps — so they don't block the registry from doing its main job (catching orphaned/overdue owners), but they do undercut the honesty-of-the-audit-output goal this PR is explicitly going for. |
…undiagnosed entry diagnosed Two Codex review findings on #288, both correct. THE fixedBy CROSS-CHECK WAS STILL SILENTLY DISABLED The branch cherry-picked the Owner-marker parser fix from #286 but not the fixedBy one - that fix travelled in the same commit as the un-quarantine this PR deliberately skipped, and the handoff's claim that "the cross-check now accepts three forms" described that branch, not this one. So the extraction still matched only `fixedBy = "PR #NN"`, and PCMetricsTest's qualified `fixedBy = "#265"` parsed as empty - the annotation-versus-registry advisory could never fire, exactly the silent-disable #286 documented. The parser hunk is taken from #286 (8a7e1a0) verbatim, and this time it gets the regression test that commit lacked: a qualified fixedBy disagreeing with a qualified Owner line must produce the mismatch advisory. Against the old parser that test fails (empty declared, no advisory), so the silent-disable class is now fenced, not just fixed. THE ADVISORY REPORTED THE OPPOSITE OF THE ENTRY'S KEY STATE check-quarantine-owners.sh printed "diagnosed-but-unowned, find it an owner" for every unowned entry - including the backpressure one, whose defining property is being UNDIAGNOSED. Telling a maintainer to find an owner for a failure nobody understands points them at the wrong task. The no-owner branch now distinguishes: an entry recording a rule-1 exception reports "UNDIAGNOSED (recorded rule-1 exception); completing the diagnosis is the open task", and quarantined-test.sh's audit banner no longer claims every entry must be diagnosed. Both wordings are pinned by tests, including that the exception advisory does NOT contain "diagnosed-but-unowned". Verified: owner check green with the corrected advisories against the live registry, and the 40 quarantine contract/script tests pass (two new). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F2MAyo5JDR5ZdWPGCxDCzY
Owner
Author
|
Review response - both findings (flagged by Codex, independently confirmed by the Claude review) are fixed in e19d8d5:
Verified locally before push: owner check green with the corrected advisories against the live registry, and all 40 quarantine contract/script tests pass (38 + 2 new). Both Codex threads have in-thread replies and are resolved. |
…rep -q inverts its own answer The repo's own guard caught this one commit after it was written: the rule-1 exception detection added in e19d8d5 was `echo "$block" | grep -qi 'rule-1 exception'`, and bin/check-shell-sigpipe.sh fails bin/*.sh on exactly that shape. grep -q exits at its first match, the writer takes EPIPE, and pipefail promotes that to the pipeline's status - so the test reports failure precisely when it FINDS what it is looking for. It only bites once the writer has more than 64 KiB left to send, which is why a two-entry registry passed locally. Rewritten as the herestring the guard prescribes. Behaviour is unchanged and proven by the advisory it drives: the backpressure entry still reports "UNDIAGNOSED (recorded rule-1 exception)" and the diagnosed-but-unowned wording still fires for ordinary unowned entries. Searched the whole tree for other instances of the class - `| grep -q` in any form, across *.sh, *.bash and workflow YAML - and there are none: every remaining hit is either a comment warning about the pattern, a deliberate negative fixture in bin/test-check-shell-sigpipe.sh, or the guard's own detection pattern. Verified: bin/check-shell-sigpipe.sh green, and the 40 quarantine contract/script tests still pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F2MAyo5JDR5ZdWPGCxDCzY
Owner
Author
|
@claude review this |
|
Claude finished @astubbs's task in 1m 48s —— View job Review complete
This is a re-review of the branch after the two Codex-flagged issues were already fixed in e19d8d5 and 8168c95. I re-verified both fixes against the current code rather than trusting the commit messages, and checked for anything new. Both prior Codex findings are correctly fixed.
One extra bug surfaced and was self-caught within this same branch (8168c95): the first attempt at fix #2 wrote the detection as Consistency check passed: Not verified: I wasn't able to execute the shell scripts ( No new issues found beyond what's already been fixed. The mechanism/wording changes are coherent and the two quarantine entries are honest about their diagnosis state. |
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
… label it the weaker reviewer The gate refresh shipped on the dispatch route only. `claude-review` is produced by claude-code-review.yml, which triggers on `pull_request` alone, and a check run keeps its last conclusion until something re-runs it - so a review arriving by any other route posts, satisfies the gate's own rules, and leaves the required check red at whatever it recorded at push time. `claude.yml` had no `actions: write` and nothing that re-ran the gate, so the `@claude review this` route could never clear the check it needs to clear. That is not a corner: it is the documented fallback, and while the dispatched reviewer is broken on master it is the ONLY working route. Traced live on #288 - review posted 08:29 for the current head, check still red, and a hand-run `gh run rerun --failed` at 08:37 was the whole fix. So `claude.yml` gets the same two-job split: the `claude` job publishes the action's own conclusion, the resolved PR number and the head SHA as it was before Claude ran, and a checkout-free `refresh-gate` job holds `actions: write` alone. Same conditions as the dispatch route - refresh only after a review the ACTION concluded successfully, only on a PR, and only when the head has not moved underneath it. Refreshing unconditionally is how a failed run's error comment turns a required check green, which is the trap the dispatch route already refused. Two things stated rather than hidden. This makes an existing looseness reachable: the gate cannot tell a review from any other `@claude` answer, so a successful non-review reply on a PR will now refresh it too. That boundary was already recorded as social rather than technical, and the alternative - the only working review route unable to clear the gate - is worse. And the two refresh jobs are near-duplicates, because sharing the body would mean checking out the script, which is the one thing a job holding `actions: write` must not do. Left deliberately undone: the comment route reviews with NO tool grants, so it reads rather than runs. Observed on #288, on a PR whose whole diff was check scripts and their tests. Copying the dispatch route's curated allowlist across is a security decision - that route is triggered by comment text, which anyone who can comment can influence - so it is parked as an explicit open question and the degradation is documented instead, in docs/ci.md and in the in-flight tracker. Nobody should read this route's "no issues found" as equal weight while that holds.
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
… inline review threads Two decisions, one of which turned out to be smaller than it looked. THE ALLOWLIST. The comment route reviewed with no tool grants, so it read instead of running - visible on #288, where it reported "tool permissions blocked Bash execution" on a PR whose entire diff was check scripts and their tests. It now carries the dispatch reviewer's curated allowlist, copied verbatim and verified identical. The objection was that comment text is attacker-influencable where a dispatch is not, and `@claude` is matched by plain substring. True, and not the operative difference: BOTH routes execute PR-authored code. What protects them is that the list is curated to this repo's own scripts and that the job holding it has no write grant - not that the trigger is trusted. Granting less bought no safety and cost the only working reviewer the ability to check its own subject matter. Blanket Bash was never on the table, and a narrower subset was dropped because running the checks the PR changed is the entire value. `--allowedTools` in `claude_args` ADDS to tag mode's base tools rather than replacing them, so the tracking comment, the CI readers and the git tools all survive. That is the action's documented behaviour, not an assumption - I tried to prove it on the CLI first and could not, because every control arm in this environment passed, so the local experiment was void and the documentation is what the claim rests on. INLINE COMMENTS, which is the part worth reading. The regression is confined to the dispatch route and always was. The action installs its inline-comment server only for an ENTITY event; `workflow_dispatch` is not one, but `issue_comment` is - and `claude.yml` was already in tag mode, since the action picks tag mode for an `@claude` comment with no `prompt` set. So the comment route can open genuine review threads, and now does. It needs no `track_progress`: that input only FORCES tag mode where a prompt would otherwise select agent mode. That changes the guidance rather than merely softening it. Unresolved review threads are what mechanically gate a merge here, so a blocking finding raised by comment actually blocks, where the dispatch route's can only ask a human to act. Ask by comment when you want findings that hold the merge; dispatch when you want `-f focus` or the packaged procedure. Both lists must now stay in step and nothing enforces it, which is recorded with the shape of the check that would. Neither route's changes can be exercised before merging: a comment always runs the default branch's copy of `claude.yml`, so it is unverifiable for the same practical reason the dispatch route is, and both are now on the post-merge list.
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
…r every commit was
Reverses the strict head-freshness rule, and deletes what it cost.
The gate now asks one question: has `claude[bot]` posted a finished review on
this PR? Any such review satisfies it, whenever it was posted. A review of the
first commit vouches for the twentieth.
That is the weaker guarantee and it is chosen deliberately. Strict is not
wrong - a review of commit N genuinely does not vouch for commit N+1 - but the
coverage it was protecting already arrives from elsewhere, because a separate
reviewer reads every push. What strictness actually bought here was a second
copy of a guarantee we already had, and what it cost was:
- a timestamp comparison that had to choose between the contributor's own
committer date and the server-side check-suite time, handle same-second
ties, and paginate an endpoint with undocumented ordering - three rounds of
review findings, none about reviewing;
- the reviewed-SHA plumbing crossing job boundaries so the head-moved refusal
could compare against what was actually checked out.
Both deleted here, along with the `checks: read` scope that existed only to
read when a head first appeared.
WHAT SURVIVES, AND WHY. The completion rule: a reviewer that left unticked
boxes has said itself that it stopped partway, and that is orthogonal to
freshness (#275). The identity rule: `claude[bot]` per
the API, never the comment body. Both are mutation-tested - six mutations, all
killed, including one that re-introduces a freshness comparison, so silently
restoring strictness now breaks the suite.
THE FORK HOLE HAD TO MOVE. Leniency made it reachable: the gate asks only
whether a finished claude[bot] comment exists, and the comment route answers
`@claude` on fork PRs - so a maintainer asking an ordinary question would have
greened a fork PR's required check on the next push. Previously this was
awkward to hit; it was never actually closed, because the gate has always
judged comments rather than provenance. The gate now refuses a fork head
outright, before reading any comment, which is where the guard belonged.
WHAT DID NOT GO, AND THIS IS A CORRECTION. Both `refresh-gate` jobs stay, and
`actions: write` with them. The expectation was that leniency retires them,
because a push creates a fresh check run that greens immediately. That is true
of a push and it is not the case they exist for: the gate fires on
`pull_request` only, so the terminal flow - push, ask for review, merge -
raises no event after the review lands, and the check keeps the red it recorded
before the review existed. Observed on #288, where a
review at 08:29 left the check red until a hand-run rerun at 08:37. The write
scope is a cost of the check being produced by a `pull_request` workflow, not a
cost of strictness, so leniency cannot pay it off. Retiring it needs the
reviewer to raise a check run on the reviewed SHA, which is now the highest
value change left in this area and is tracked.
The strict implementation is archived at
`archive/review-gate-strict-head-freshness`, with the trade, the assumption it
rests on, and the trigger to restore it, in
docs/inflight/parked-strict-review-gate-freshness.md. The assumption is worth
naming here too: per-commit coverage comes from the auto-reviewer. If that ever
stops, it stops coming from anywhere, and nothing will announce it - the gate
keeps passing, because a review does still exist on the PR.
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
Found by the dispatched review this PR now makes possible - the first finding produced by the system this PR fixes, reviewing the PR that fixes it. The lead bullet of the tracker still said the gate "reds the PR until a review exists for the current head", two sentences before correctly describing the lenient behaviour. Same drift class as AGENTS.md, the docs/ci.md workflow overview and the dispatch workflow header, and it was not among the three Codex named, so it survived that sweep. A repo-wide grep now returns only descriptions of the REMOVED rule - "it does NOT prove the review covered the current head", "there used to be", "used to require" - plus the one narrative about #288's head. No live claim left. Also states the consequence the old wording hid: because the gate no longer asks about the head, a single satisfying comment counts for the life of the PR rather than until the next push.
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
…n out of it, and ask the gate a simpler question (#287) #284 split the code review into a cheap always-on gate and an on-demand reviewer. The gate worked. The reviewer did not: `track_progress` does not support `workflow_dispatch`, so the action refused before reviewing anything. With the auto-review already removed, that left no working route and a required check nothing could turn green. MAKING IT WORK The obvious fix - synthesise a `pull_request` payload so the action sees a supported event - is impossible, and was proved on a runner rather than argued. It passes locally: the action's own parser yields the right PR in tag mode. But the runner writes the `GITHUB_*` context over every step's environment AFTER the step's own `env:` block, and `event_name` is on that allowlist. The payload file is writable; the event name is not, and the name alone picks the mode. That commit was dropped. So the dispatch route runs in agent mode, and the three losses are paid out loud rather than discovered later: no tracking comment (the summary comment is now mandatory in the prompt), no inline-comment tool, and no CI MCP server (replaced with `gh run list -c <sha>` under a scope that is actually documented). The inline-comment loss turned out to be confined to that route. `claude.yml` was already in tag mode, and the inline-comment server installs for any ENTITY event - `issue_comment` is one, `workflow_dispatch` is not - so recovering it there was a one-item addition to a list this change was already writing. The guidance therefore inverts rather than degrades: ask by comment when you want findings that mechanically block (real threads, which required_review_thread_resolution enforces), dispatch when you want `-f focus` or the packaged procedure. TAKING THE WRITE TOKEN OUT OF REACH #284 shipped a privilege escalation. The reviewer job held `actions: write`, checked out `refs/pull/<n>/head` with credentials persisted by default, and by design executes this repo's `bin/` scripts from that checkout. A same-repo PR could edit an allowed script, recover the token from `.git/config`, and dispatch any workflow - `release.yml` has no actor gate. Now `actions: read` with `persist-credentials: false`, and `actions: write` isolated in a checkout-free `refresh-gate` job whose inputs cross as job outputs, so the head-moved refusal still compares against the SHA the reviewer actually read. The same gap existed on the comment route, traced live on #288: a review posted at 08:29 satisfied every rule and the required check sat red until a hand-run rerun at 08:37. That route now has the same split. Granting the curated allowlist there needed two carve-outs, both found in review: the execution tools are withheld from fork heads and from untrusted commenters. The first attempt at that guard was written as `${{ fork && '' || format(...) }}`, which needs a truthy middle operand - so it returned the FULL allowlist while the step above logged "WITHOUT execution tools". A guard that did not guard and said it had. ASKING THE GATE A SIMPLER QUESTION The gate required a review NEWER than the head. It now accepts any finished `claude[bot]` review on the PR. What that gives up is real - a review of the first commit vouches for the twentieth - and it is affordable only because Codex auto-reviews every push, so per-commit coverage comes from there. If that ever stops being true, per-commit coverage stops coming from anywhere; that trigger is recorded, with the strict implementation preserved at the tag `archive/review-gate-strict-head-freshness`. Deleted with it: the head/review timestamp comparison, preferring the server-side timestamp over the contributor-controlled committer date, same-second tie handling, the reviewed-SHA plumbing, the head-moved guard, and `checks: read`. The two surviving rules - identity and completion - are mutation-tested, six mutants all killed, including one that re-adds a freshness comparison, so restoring strictness silently now breaks the suite. One correction, because the first version of this change claimed otherwise: leniency does NOT retire `actions: write`. The gate fires on `pull_request` only, so the terminal flow - push, ask for review, merge - raises no event after the review lands. The refresh jobs stay. That scope is the cost of the check being produced by a `pull_request` workflow, not of strictness, and retiring it needs the reviewer to raise a check run on the reviewed SHA. Tracked. Leniency also opened a hole and closed it: the gate now asks only whether a finished review exists, and the comment route answers `@claude` on fork PRs - so a maintainer asking a QUESTION would have greened a fork PR. Fork heads are refused before any comment is read. ALSO - The action exits 0 on a workflow-validation refusal, so the job reported success. It now fails when the action's own `conclusion` output is empty. - The contract was stated in nine files, fourteen times, in nine different sentences. Collapsed to one canonical paragraph, with the rest reduced to pointers. Why no tool caught it is written up: `dups: clones` and `dups: similarity` scan `parallel-consumer-*/src` only, so docs, workflows and `bin/` are scanned by neither - and even aimed at them, token-based clone detection cannot see paraphrase. The scanners catch copy-paste; what bit here was restatement. - `.github/workflows/README.md`, one line per workflow, leading with the fact that `claude-code-review.yml` is the one that does not review. - A flapping `ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect` quarantined, pointing at its fix in #262. Note that #265 takes an incompatible approach to the same line - it deletes the assertion rather than anchoring the measurement - so whichever lands second will conflict there. - A cooperative-variant chaos sighting captured against the confluentinc#857 family (fork mirror #119) before its seed expired, recorded as consistent with that family rather than confirmed as it: the eager variant was already logged with a clean ProgressProbe kill, while this one shows only the shutdown-path timeout. Verified end to end on this PR: the review was requested by comment, posted as `claude[bot]`, and the gate went green - then stayed green across two further pushes with no new review. Under the old rule each push would have reddened it. This is the first PR in this series to merge without an admin bypass. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 17, 2026
Master moved a long way under this branch, and most of what it moved was this branch's own subject matter. Resolving the merge honestly leaves a one-file deletion, so the resolutions are recorded here rather than left to be inferred from a 37-conflict merge. Superseded on master, taken from master wholesale ------------------------------------------------------------------------------- - `.github/workflows/claude.yml` - #287 landed a superset of this branch's grant: the same curated allowlist as the dispatch route (REVIEW_TOOL_ALLOWLIST), a trusted-author gate, a fork carve-out, a 30-minute cap and `refresh-gate`. It also SETTLED this branch's stated open question - the action checks the PR branch out over master's checkout, so the granted scripts run against the PR's tree. Master's fork handling differs by design: it withholds the allowlist and still answers, where this branch refused the run outright. - `docs/quarantined-tests.md`, `bin/check-quarantine-owners.sh`, `bin/quarantine-lane-report.sh` - the qualified `Owner: PR astubbs#NN` marker this branch introduced is already on master via #288, parsers and registry prose included. - `docs/inflight/test-untracked-ci-flakes.md` - master's copy is a strict superset: it carries this branch's `PCMetricsTest` diagnosis and its "quarantined and removed again, the diagnosis was wrong" note about the backpressure test, plus a `ProducerManagerTest` entry, the rule-1 exception and a citation repair this branch predates. - `docs/inflight/ci-review-agent.md` - rewritten by #287 around the two-route model. Two of this branch's entries are now stale rather than merely older: `actionlint` IS granted, and "which tree does it run against" is answered. The quarantine this branch added is dropped, not re-resolved ------------------------------------------------------------------------------- `PCMetricsTest.metricsRegisterBinding` was quarantined here naming #265 as `fixedBy`. #265 has since MERGED, and it fixed this test causally: the `Thread.sleep(1000)` between the counter snapshot and the gauge read - which is the exact mechanism the quarantine reason describes - is now an `await().untilAsserted(...)` requiring the trailing meters to agree with the counters before the snapshot is asserted. #265 deleted the annotation and the registry entry in the same commit, per rule 3. Re-applying the entry would therefore be a hard failure, not a judgement call: `bin/check-quarantine-owners.sh` errors on a merged owner whose test is still quarantined ("re-enable overdue"). And a *fresh* sighting would need a fresh diagnosis under rule 1 - the old reason no longer describes code that exists. No such sighting: master's gating CI has been green on every run since #265 merged on 2026-08-13. If it flakes again, re-quarantining is the inverse of that commit, as #265's own message says - a new entry with a new diagnosis, not this one restored. Rename/rename, and the mis-pairing AGENTS.md warns about ------------------------------------------------------------------------------- Both sides had run `bin/rename-packages.sh` independently, so the tree conflicts were rename/rename and resolved to master throughout - this branch's Java delta was the quarantine annotation and nothing else. The predicted cross-module mis-pairing did surface: git paired the STREAMS module's `TestConventionsArchTest.java` against the METRICS module's. Asserted after resolution - all six arch tests present, one per module, and no `io/confluent` path left in the index. Verified: `bin/check-quarantine-registry.sh` (2 entries, consistent) and `bin/check-copyright-headers.sh` (241 files, 0 violations) both clean.
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 17, 2026
…merge Review feedback on #304. The citation repair in 3cc4d01 pointed at `cbd328746^` - a commit that exists only on this branch. Squash-merging this PR, or deleting the branch afterwards, would leave the repaired pointer unresolvable: a repair whose own pointer expires is not a repair, and it would have recreated the exact breakage it was written to fix. Both citations now name `b42733abef45e792df6fca1b3fb8d49d7dfc7946` - #288, the commit that RECORDED the rule, verified to be an ancestor of `master`. That is deliberately the commit that added the text rather than the one that removed it: the adding commit is on master permanently, while the removing commit is whichever branch happened to do the removal and is therefore exactly as ephemeral as the problem. The full 40-character SHA is used rather than an abbreviation. Verified the anchor resolves against that SHA: git show b42733a:docs/inflight/test-untracked-ci-flakes.md grep 'do not compare two moving values' Swept the rest of this PR's added content for the same defect class - a cited SHA that is branch-only - by resolving every SHA-shaped string it adds and testing each for ancestry against master. One other hit, `821a91af`, is pre-existing text relocated by this PR rather than a citation it authored, and it names which run failed rather than instructing a reader to `git show` it, so it is left alone.
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.
Context: #286 (where these quarantines were first attempted) and #265 (the owner of the PCMetrics fix). Tracking ledger:
docs/inflight/test-untracked-ci-flakes.md.Description
Quarantines the two unit-lane flakes so they stop blocking every PR, as a standalone PR that can merge ahead of the work that tripped over them.
PCMetricsTest.metricsRegisterBinding- diagnosed and owned. Cherry-picked unchanged from #286: the test compares a registry gauge against an expectation built from a test-side counter snapshot taken earlier, so two independently-advancing values are sampled at different instants. Owner: #265, whose newawait()sits upstream of the failing assertion and genuinely gates it. Per registry rule 3, when that PR merges it must delete the annotation and registry entry in the same commit -bin/check-quarantine-owners.shwill report "re-enable OVERDUE" if forgotten.OffsetEncodingBackPressureTest.backPressureShouldPreventTooManyMessagesBeingQueuedForProcessing- UNDIAGNOSED, quarantined as an explicit rule-1 exception by owner decision. Rule 1 says undiagnosed red stays red; at 4/45 this is the most frequent tracked flake and, with the surefire retry gone (#224), it blocked every PR. The exception is recorded as an exception, in the annotation, the registry entry, and the ledger:ConditionTimeoutat thegetHighestSeenOffset()assertion, high-water mark never reaches 139 (136 and 132 seen), cause unknown. NofixedBy- the audit flags it "diagnosed-but-unowned" (advisory) instead of passing silently.expectedHighestSeenassumes all threshold-crossing extra records get polled, while crossing the threshold is exactly what stops intake - plus its falsification path.Also cherry-picked from #286: the owner-marker/
fixedByparser fix (qualifiedastubbs#NNforms), without which any registry entry fails either the owner check orbin/check-issue-refs.shby construction.Interplay with #286: two of its commits are re-landed here verbatim, so once this merges, they drop out of that PR's net diff; its later un-quarantine commit makes no net change to the backpressure test against its base, so the honest re-quarantine here survives that merge.
Rule wording synced to the mechanism. Registry rule 4 claimed "every entry needs an owning fix PR", but the checks never enforced that - a missing owner is an advisory, and only wrong owner claims hard-fail. The owner judged the written rule too tight, so the wording now matches reality everywhere it is stated (registry rules,
docs/testing.md, the AGENTS.md ledger table, and the@Quarantinedjavadoc), and rule 1 records the owner-granted-exception path this PR exercises.Audit tooling corrected, so its output is honest about both states. Two defects found in review, both fixed here with regression tests:
fixedBycross-check parsed onlyfixedBy = "PR #NN", soPCMetricsTest's qualifiedastubbs#265matched nothing,declaredcame back empty, and the annotation-versus-registry advisory could never fire - a check that had silently stopped checking. It now accepts the same three forms as theOwner:line. The newqualifiedFixedByDisagreeingWithRegistryOwnerIsFlaggedfails against the old parser, so the silent-disable is fenced rather than merely fixed.bin/quarantined-test.sh's banner no longer claims every entry must be diagnosed.One self-caught defect worth recording. The first version of that advisory branch was
echo "$block" | grep -qi 'rule-1 exception', whichbin/check-shell-sigpipe.shcorrectly failed:grep -qexits at its first match, the writer takes EPIPE, andpipefailpromotes that to the pipeline's status - so the test reports failure precisely when it finds what it is looking for. It only bites once the writer has more than 64 KiB left to send, which is why a two-entry registry passed locally. Rewritten as the prescribed herestring. Swept the tree for other instances of the class and found none: every remaining hit is a warning comment, a deliberate negative fixture, or the guard's own detection pattern.Release note: per registry rule 5, releases are blocked while the registry is non-empty - the 0.6.0.0 release now waits on both tests being fixed and released from quarantine.
Verified locally before each push:
bin/check-quarantine-registry.shgreen (2 entries),bin/check-quarantine-owners.shgreen with the expected advisories (including the corrected UNDIAGNOSED wording),bin/check-issue-refs.shgreen,bin/check-shell-sigpipe.shgreen, and the quarantine contract/script tests pass (40, including the two new regression cases).Checklist
docs/quarantined-tests.md), ledger (docs/inflight/test-untracked-ci-flakes.md),docs/testing.md, AGENTS.md, and the@Quarantinedjavadoc@Quarantinedmechanism, whose 38 contract/script tests were run and passbin/check scripts touched🤖 Generated with Claude Code
https://claude.ai/code/session_01F2MAyo5JDR5ZdWPGCxDCzY