Repository navigation
ci: quarantine lane - @Quarantined tag + enforced registry so green checks mean mergeable - #84
Merged
Merged
Conversation
…failing-on-master tests
Makes green checks mean 'mergeable'. When a test is red on master's gating CI
and its fix lives in another open PR, manual triage ('is this red the known
flake or a real break?') is error-prone. But @disabled would lose the signal
entirely - and a 'known flake' can be a real product bug (the drain-zombie
investigation started exactly that way).
The quarantine lane is the middle path:
- @Quarantined(reason, tracking, fixedBy) - meta-tagged 'quarantined'; reason
and tracking are compiler-required, so 'no quarantine without diagnosis' is
enforced by the type system. Lives in core's shared test sources (test-jar),
usable by every module.
- Gating suites exclude the tag (pom default + ci-unit/ci-integration/ci-build
scripts), so required checks go green.
- docs/QUARANTINED_TESTS.md - the live registry / task list of quarantined
tests. Enforced, not advisory: bin/check-quarantine-registry.sh fails on any
drift vs the annotations, in both directions (missing entry OR stale entry),
run locally by bin/quarantined-test.sh and in CI.
- New non-gating 'Quarantined Tests' CI job runs ONLY the tag on every PR:
registry drift is the one thing allowed to turn it red (real, actionable);
quarantined-test failures never do (step-level continue-on-error - the
lesson from the Kafka-compat job's noise). Step summary carries pass/fail,
the audit of every quarantined test + its owning fix PR, and per-test
results. All-green is flagged as 'fixes may have landed - check if
annotations can be deleted'.
- Re-enabling = the owning fix PR deletes the annotation AND the registry
entry in the same commit after merging master, atomically restoring the
test to the gating lane.
Rules (AGENTS.md): no quarantine without diagnosis - undiagnosed red stays red
and blocks, on purpose; quarantine is master-state, not PR-state.
First occupant: PartitionStateCommittedOffsetIT.committedOffsetRemoved - the
[latest] nudge race, owner PR #80 (awaitWithTopicNudge, 20/20 clean).
Verified: targeted run under gating groups = 0 tests; under the lane = all 3
params run; registry check passes and correctly fails on seeded drift in both
directions; full unit suite green across the reactor.
Also parks the user-facing upstream-issue-mirroring plan in docs/inflight.md
(one issue per invocation, first-class dry-run).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
Dependency ReviewThe following issues were found:
License Issues.github/workflows/quarantine-lane.yml
OpenSSF Scorecard
Scanned Files
|
✅ 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)
|
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
…must exist, stay open, and eventually remove the quarantine bin/check-quarantine-owners.sh closes the last gap in the loop: the registry CLAIMS 'PR #NN will fix this', and now CI checks that claim against reality on every PR: - ERROR (turns the non-gating job red - real, actionable): owning PR does not exist; owning PR closed without merging (orphaned entry, needs a new owner); owning PR MERGED but the test is still quarantined (re-enable overdue). - ADVISORY: entry unowned; open owner whose merge preview does not yet remove the annotation (normal until it merges master); preview n/a because the quarantine has not reached the owner's base branch yet (checked explicitly to avoid a false 'removes it' verdict). gh is pinned to the origin remote's repo (fork gotcha: unpinned gh resolves PR numbers against upstream). Run in CI with github.token; locally via bin/quarantined-test.sh when gh is authenticated (skip note otherwise). Verified live: PR #80 correctly resolved as open with base ci/grumpy-runner-workflow, preview check correctly reported n/a (quarantine not on that base yet). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
release.yml gains a guard beside 'refuse to release a red master': if any @Quarantined annotation exists, a real release hard-fails with a pointer to docs/QUARANTINED_TESTS.md; dry runs warn instead so a rehearsal surfaces that a real release would block. Rationale: the quarantine lane keeps PR merges honest - it must not let a release ship while known-failing tests are held out of the gating suites. Snapshots (publish.yml) deliberately still publish: they are dev artifacts, master is always -SNAPSHOT, and freezing snapshot publishing for an entire quarantine window would be over-reach. Registry rule 5, AGENTS.md, and the CHANGELOG entry document the behaviour. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
The quarantine mechanism only works while otherwise-unrelated string sites agree (annotation meta-tag, pom group exclusion, gating scripts, lane runner, workflow job, release guard). Any one drifting in a refactor breaks the lane SILENTLY - worst case quarantined tests vanish from BOTH lanes: no red anywhere, coverage just gone. Captured as ordinary gating unit tests: - QuarantinedAnnotationContractTest (8 tests): @tag meta-annotation == TAG, RUNTIME retention, TYPE+METHOD targets, pom excluded.groups, all three gating scripts' exclusions, lane runner's include+exclude-clear, workflow wiring of all three quarantine scripts, release.yml guard presence, registry file location. - QuarantineRegistryScriptTest (5 tests): runs the real registry check against temp fixtures via a new QUARANTINE_CHECK_ROOT override - happy path, missing-entry drift, stale-entry drift, empty lane, and string literal mentions must NOT count. - RepoRoot: shared walk-up helper (DRY). Writing them exposed a real bug, now fixed: the enforcement greps matched the literal '@Quarantined(' anywhere, and the self-test files themselves contain that string in literals - the release guard would have blocked releases FOREVER and the registry check would have reported phantom drift. All greps are now anchored to annotation usage ('^[[:space:]]*@Quarantined\(') across both check scripts, the lane runner, maven.yml, and release.yml; the literal-mention test case pins the distinction. Also parks in inflight: evaluate extracting the lane as its own FOSS project (prior art is mostly commercial SaaS - Trunk.io/BuildPulse/Datadog/Develocity; the CI-enforced closed loop looks novel). All 13 self-tests green; repo-level registry + owner checks re-verified green with the self-test files present. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
astubbs
added a commit
that referenced
this pull request
Jul 30, 2026
… the pom The gating scripts hardcode -Dexcluded.groups (overriding the pom default), so adding 'chaos' only to the pom let ChaosChurnStormIT leak into the required Integration Tests check - where it promptly went RED on this branch's master base (the detector detecting; but it must not gate). Same class of bug the quarantine-lane PR #84's contract test now pins for the 'quarantined' group. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
…g forever The mac-laptop runner is offline/unused for the foreseeable future, so every PR showed three eternally-pending 'Build & Test - ... (macOS self-hosted, optional)' checks, polluting the at-a-glance checks list. Disabled at the TRIGGER level (pull_request commented out with the repo's 'was:' re-enable pattern), not job-level if:false like test-kafka-compat: this workflow's only job is the mac job, and trigger removal makes the checks disappear from PRs entirely instead of listing skipped entries. workflow_dispatch is preserved - manual runs work whenever the runner is up. self-hosted-tests.yml (Linux performance runner, schedule/dispatch only) and everything highcpu are untouched. Validated: YAML parses (trigger set now workflow_dispatch only); quarantine contract self-tests green (13/13). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
The single job bundled two responsibilities: the enforcement checks (registry
drift + owner claims - seconds) and actually RUNNING the quarantined tests
(full reactor build + TestContainers - ~10min), making a fast audit look
like a slow suite on every PR.
Split:
- 'Quarantine Audit' (per-PR, seconds): checkout + registry check + owner
claims. These are the teeth - real, actionable errors that go red. No
Java, no Maven, no tests.
- 'Quarantined Tests (nightly, non-gating)': the lane run moves off PRs to a
nightly schedule against master + workflow_dispatch. More principled too:
quarantine is master-state - the lane's signal ('did the fixes land?')
only changes when master does, so per-PR runs were pure runner noise. The
same rule checks run fail-fast at the top before any test spend.
Self-tests green (13/13 - the contract test's workflow-wiring assertions
still hold). Docs (registry header, AGENTS.md, CHANGELOG) reworded.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
… runs The nightly job's if-condition allowed workflow_dispatch but the workflow never declared the trigger - manual dispatch was impossible. Caught while writing the end-to-end validation plan for the audit/nightly split. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
…d entry on PRs First-run validation of the audit/nightly split showed the nightly job listed as a grey 'skipped' check on every PR run (it shared maven.yml with the PR jobs). Same spirit as the macOS-runner cleanup: checks that will never run shouldn't appear at all. The lane run now lives in quarantine-nightly.yml (schedule 03:17 + workflow_dispatch, fail-fast rule checks kept at the top); maven.yml keeps only the seconds-fast per-PR Quarantine Audit (validated live: green in 5s). Contract tests updated: maven.yml must NOT contain the lane run (pinning this exact regression); quarantine-nightly.yml must contain the run, the fail-fast checks, and declare both of its triggers. 14/14 green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
On the PR checks list (especially mobile) the 'Build and Test / ' workflow
prefix consumed the row, truncating every actual job name to a few chars.
- Workflow renamed 'Build and Test' -> 'CI' (say-nothing minimum; the prefix
appears on every row so it should carry no information).
- Gating job names byte-identical ('Unit Tests', 'Integration Tests',
'Performance Tests') - the master ruleset matches check names, so required
checks are unaffected.
- Auxiliary jobs get terse group labels: 'quarantine: audit', 'dups: clones',
'dups: similarity', 'static: spotbugs', 'static: spotbugs baseline',
'deps: vulnerabilities', 'compat: kafka 4.x (experimental)', 'cache',
'full build (master)'; review workflow -> 'review'.
- Workflow-name consumers updated in lockstep: publish.yml workflow_run
trigger and release.yml's refuse-red-master guard both now reference 'CI'
(they matched on the old name and would have silently broken).
Quarantine self-tests green (14/14).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
…- watch the audit fail fast Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
… test - watch the audit fail fast" This reverts commit 7d45a60.
…-tests 14 -> 31) P1 fixes, each with proof: 1. Surefire never bound the group properties - quarantine filtering was a SILENT NO-OP for unit tests (only failsafe was wired; a @Quarantined unit test would keep running in the gating Unit Tests lane). Fixed in the pom; proven behaviorally both directions with a deliberately-failing quarantined unit test (0 runs under gating groups; executes + fails under the lane's groups); pinned by a pom-level contract assertion (both plugins must bind <groups>/<excludedGroups>). 2. Detection regex blind spots - '@test @Quarantined(...)' same-line and fully-qualified forms were invisible to the shell-side checks while JUnit still excluded the test from gating: silent loss from BOTH lanes. Broadened ERE (stacked annotations + FQ form) that still rejects string literals; fixture tests for every variant. 3. Method-granularity drift - two annotated methods rode along on one registry entry, and stale Class.method entries survived (both reproduced by the review). The registry check now compares per-class annotation COUNTS against entry counts and verifies each entry's named method still exists; both cases are fixtures now. 4. Transient gh failures were conflated with 'PR does not exist', so rate limits/5xx would red the audit repo-wide, eroding 'red here is real'. gh calls now retry with backoff and classify stderr: confirmed not-found is the only ERROR; infra weather is ADVISORY. Covered by stubbed tests. P2/P3: - bin/lib/quarantine-common.sh: THE single home of the detection pattern + registry parsing (was duplicated ~8x across 5 files); all callers (both check scripts, lane runner, nightly workflow, release guard) source it. Registry text is now parsed fixed-string (awk index()), never as a dynamic pattern. - Owners script: QUARANTINE_CHECK_ROOT test seam + CheckQuarantineOwnersScriptTest (10 cases, PATH-stubbed gh/git: MISSING/CLOSED/MERGED/TRANSIENT/OPEN sub-branches, renamed-file preview, unowned, fixedBy-vs-Owner mismatch). - Lane runner: QUARANTINE_SKIP_CHECKS=1 in CI (gating steps already ran the checks; re-running them inside continue-on-error swallowed failures); the surefire fix means the lane no longer runs the full unit suite unfiltered. - Blank reason/tracking now rejected by a reflective contract scan (empty strings compiled fine - 'no quarantine without diagnosis' wasn't enforced). - Preview file-missing (renamed by fix PR) -> ADVISORY instead of false OK; guarded base-branch query; || true on the non-gating summary grep. - PIT's coincidental exclusion of the tag documented in ci-mutation-test.sh; dangling diagnosis-doc references repaired; registry doc gains usage notes (standalone fast check, gh auth requirement, manual lane dispatch). Full unit suite green across the reactor; 31/31 quarantine self-tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
Owner
Author
ce-review remediation (commit ac6b093) - how each finding was handledAll 4 P1s confirmed real and fixed, with proof; P2/P3 tail addressed. Self-tests grew 14 → 31. P1
P2/P3
Validation
🤖 Generated with Claude Code |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…ghtly lane job on GitHub pre-merge Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
'Quarantine is master-state, the signal only changes when master does' - followed to its conclusion: master changes ON MERGES, so push-to-master is the natural trigger (user's framing: run it on every PR merge like any other integration test - just non-gating and invisible to PR checks). Every merge now immediately shows whether it made the quarantined tests start passing (fixes landed - delete the annotations) or worsen; an owner PR that merged without re-enabling is caught within minutes, not at 03:17. The nightly cron stays as a backstop for merge-free stretches; dispatch stays for on-demand. Also drops the temporary DEMO pull_request trigger. Self-tests 31/31. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
…agnosis + fix options Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
…Test The duplicate-code check correctly flagged this PR's two script tests sharing 24 lines of process-runner scaffolding (ProcessBuilder + readFully + Result). Extracted to AbstractQuarantineScriptTest: fixture root, registry writer, runScript() with a customizeEnvironment() hook (the owners test uses it for its PATH-stubbed gh/git). Script tests now contain only fixtures and assertions. 31/31 self-tests green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
…rop redundant cron Per-PR runs give PRE-MERGE attribution - a PR that accidentally fixes or breaks a quarantined test learns before merging; the push-to-master run keeps the canonical master-state record; workflow_dispatch stays for on-demand. The nightly cron is gone: the signal cannot change without a code change, and both kinds are now covered. File renamed quarantine-nightly.yml -> quarantine-lane.yml to match behavior; contract test pins all three triggers; docs aligned. Self-tests 31/31. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
…d pass; flapping attribute The lane now closes its own loop on every PR run: - Sticky per-test report comment (🧪🔒, marker-upserted, never spams): 🔴 failing-as-expected / 🟡🎲 flapper passed (proves nothing) / 🚨 PASSED - ACTION REQUIRED / ⚪ not run, each with its owning PR. - Strict-xfail gating via conversation resolution (repo already requires it): when a DETERMINISTIC quarantined test passes, the fix landed - reality no longer matches the ledger - so the lane opens a merge-blocking review thread demanding the annotation + registry entry be deleted (or the thread consciously resolved, e.g. one lucky pass -> mark it flapping). Anchored at the annotation's file when the PR touches it, else file-level on the first changed file with a pointer to the annotation location (GitHub cannot anchor threads outside the diff). Thread is marker-deduped across runs. - @Quarantined gains 'flapping = true' (compile-checked) for tests that pass unreliably - their passes are report-only. committedOffsetRemoved is marked (only the [latest] param under broker load fails). - The job's conclusion stays reserved for rule violations - test outcomes never red it; the thread is the gate, with a human-judgment resolve button as the escape valve. bin/quarantine-lane-report.sh does the work (classification from surefire/failsafe XML incl. parameterized entries + single-line testcase shapes; DRY_RUN test seam). New QuarantineLaneReportScriptTest (5 cases) on the shared script-test harness. Self-tests 36/36; workflow gains pull-requests:write. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
🧪🔒 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 |
- Job renamed 'Quarantined Tests (non-gating)' -> 'tests': the row renders as 'Quarantine Lane / tests' - the workflow name already carries the identity, the old name double-said quarantine and truncated on mobile. - Lane-leak self-check (user-mandated coverage that the lane runs ONLY the quarantined tests): the reporter now cross-checks every testcase in the surefire/failsafe reports against the registry - any non-quarantined test executing in the lane means group filtering regressed (the surefire-binding P1 class) and FAILS the job with a LANE_LEAK diagnosis. Verified against the live run: only the quarantined IT executed (3 params; all other modules 0). Two new fixture tests (leaked test -> exit 1; clean run -> self-check pass). Self-tests 38/38. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18
astubbs
added a commit
that referenced
this pull request
Jul 31, 2026
…t program Unblocks pull_request CI on PR #83 (a conflicted PR runs no workflows at all - the merge ref can't be built - which is why chaos-pain never fired). Conflict resolutions: - bin/ci-*.sh: keep this branch's inherit-from-pom form (no explicit -Dexcluded.groups; the pom is the single source of truth). - pom.xml excluded.groups: union of both sides - performance,chaos (this branch) + quarantined (master's #84 quarantine lane). Known follow-up (rostered in inflight): ManagedPCInstance needs the Modifications Copyright line to pass the new Copyright Headers check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013ALhTf1TRA2S6Ue5rkiQdK
astubbs
added a commit
that referenced
this pull request
Aug 14, 2026
…correct the survey it was wrong about Review findings on this branch, all four verified by running before and after. CORRECTION TO THE PREVIOUS COMMIT ON THIS BRANCH. Its message says "18 owner LGTMs" in one place and "sixteen" in another. Both are wrong, and so was every copy of that claim in the tree. The real figure, re-derived from repos/astubbs/parallel-consumer/pulls/<n>/reviews over all 181 PRs, is 50 owner LGTM reviews across 38 PRs, from #63 to #292. That commit cannot be rewritten, so this one states the correction. 1. THE SELF-TEST COULD NOT SEE A DISABLED STEP, ONLY A DELETED ONE The four coupling cases were `grep -F` substring searches over the workflow TEXT, and a substring search cannot tell a step that RUNS from one that merely APPEARS. Measured on a scratch copy: `if: false` on the step, `|| true` on its pipeline, `; true`, `set +e`, `continue-on-error: true`, and commenting the entire step out ALL left the suite green at 42/42. One case - `grep -F 'bin/check-human-lgtm.sh'` - was satisfied by two header comments alone, so it stayed green with the step deleted outright. That is the failure class in docs/solutions/workflow-issues/a-check-that-reports-success-without-having-run.md occurring inside the guard written to prevent it, which is why it is worth more than its blast radius suggests. The coupling section now parses the step out of the workflow - comments dropped first, so a commented-out step reads as an absent one - and asserts on its structure: it exists, it reads the reviews endpoint, its marker matches, its `if:` is neither a never-true constant nor changed from the intended guard, nothing in it swallows the checker's exit status, and the checker invocation is its last command. All eight sabotages above now go red; the unsabotaged copy stays green. Deliberately awk rather than python3 + PyYAML, though PyYAML does import here. This suite runs as a step of a REQUIRED check, ahead of the gate it protects, so a dependency of it is a thing that can brick every open PR by being absent from a runner image. The indentation rules it needs are the only YAML involved. 2. THE EMPIRICAL CLAIM JUSTIFYING CASE-INSENSITIVITY WAS FALSE, IN FOUR PLACES Claimed: 18 LGTMs, #210 to #292, all the lower-case bare word. Actual: 50, across 38 PRs, #63 to #292. Forty-nine are `lgtm`; ONE, on #84, is `Lgtm`. Forty-six are the bare word alone on a line; four carry a trailing clause, one of which - #73's "lgtm, @claude how about you?" - ends in a question mark. The true data argues for the design harder than the false data did. `Lgtm` is a live counterexample proving case-insensitivity is load-bearing rather than merely generous: without it this repo has a stamp on record that the checker would call missing. #73 is a real passing body containing a `?`, which is why the `?` clause rejects only a `?` touching the token. Both are now self- test cases (8c, 8d). The one clause that WAS true and is load-bearing - all 50 are COMMENTED reviews, not approvals - is kept. The count also sat inside an assertion LABEL, so a passing test printed a false claim on every run. Labels now name the PR the body came from instead. 3. README DROPPED `claude-review` FROM "JOB NAMES ARE AN API" That list named only the new job, while `claude-review` is the context ruleset 15055005 actually requires throughout cutover steps 1-3. Someone reading it before performing step 5 could conclude the old name is no longer an API and delete the transitional job BEFORE the ruleset swap - the exact bricking the workflow header warns about, arrived at by following the docs. Restored, marked transitional, with the ordering spelled out. 4. THE CANONICAL CONTRACT WAS OWNED TWICE docs/ci.md declares itself canonical and says everything else links to it; this branch then added a seven-clause restatement of a rule bin/check-human-lgtm.sh heads "THE MATCHING RULE, IN FULL". Both cannot own it, and the drift was not hypothetical - the false survey above was pasted into four files and rotted in all four inside one PR. Split by altitude: docs/ci.md owns the GATE contract (what satisfies the gate), bin/check-human-lgtm.sh owns the MATCHING RULE (what satisfies the human half). The script wins the second because it is the executable truth - its prose sits beside the awk implementing it, and prose and code in one file cannot drift unnoticed. AGENTS.md's substantive clause, which sat two lines above the sentence forbidding exactly that, is reduced to rule-plus-pointer. FOUR CHECKER BUGS, EACH PINNED BY A CASE PROVEN TO FAIL WITHOUT THE FIX - A marker line bearing this run's token but a lost field was read as more of the PREVIOUS review's body, merging the next reviewer's words into the previous segment. A six-field marker for `mallory` after an `astubbs` segment reported "astubbs submitted a review containing LGTM". Not reachable from today's --jq, but one `; next` removes the forgery path. (21c) - scan() advanced PAST each token it examined, discarding the character the next candidate needs to see, so glued repeats walked through the whole-word guard: `LGTMLGTM` passed a rule under which neither half of it does, and `xLGTMLGTM` passed one that refuses `xLGTM`. (11b, 11c) - A trailing `\r` defeated the fence-close test, so a CRLF body never closed a fence and swallowed every LGTM after it. Not live - 0 of 365 owner bodies carry a CR - which is why it needed a case rather than a wait. (12f) - Three clauses survived deletion with the suite green: the marker NAME check, the fence info-string clause, and the fence-character clause (no `~~~` fence appeared anywhere in the suite). Inputs added that flip under each. (21b, 12d, 12e) Also: docs/ci.md said there were three human-half reds when there are four - the catch-all "Could not scan this PR's reviews" does not start with NO HUMAN LGTM ON THIS PR, which defeated the "tell them apart without opening the job" promise in the same paragraph. The fourth is now listed, and named as the one that means the instrument broke rather than the work is outstanding. DELIBERATELY NOT CHANGED. `not LGTM/LGTM` returns 0. The negator rule considers only the word TOUCHING the token, and the second token's preceding character is a slash; making it 1 needs a negation detector that reads past an intervening token, which is the unbounded cleverness the script refuses by name. Pinned as case 10e so nobody widens it while fixing the glued-repeat bug beside it. VERIFIED. 55 self-test cases green. Both gate self-tests, check-shell-sigpipe, check-copyright-headers, check-issue-refs, check-docs-data, check-action- versions: all pass. Eight workflow sabotages red after, six of them green before. Seven checker mutations each turn exactly the intended new case red. Replayed against live API data for all 78 PRs carrying reviews: 38 green, zero false positives, zero false negatives against an independently written matcher. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BhF637Ywr7MiKkxaR11Up3
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 14, 2026
593 lines of checker and 855 of self-test, to answer "did Antony say lgtm". It parsed code fences, blockquotes, negation forms, glued repeats, CRLF line endings and typographic apostrophes - defences against an attacker who is also the only person the check protects. The last thing the previous round was doing was widening a bracket class because a typographic apostrophe is three UTF-8 bytes. The rule as stated: a review by the owner whose body contains lgtm, any case, anywhere. That is now what the code says. The marker machinery went with it, and that is the interesting part. It existed because the workflow streamed every review as flat text - marker line, body, marker line, body - so a body could forge a segment header and mint an owner LGTM out of a stranger's comment. Hence an unguessable token per run. Filtering on .user.login with jq BEFORE any text is looked at removes the attack, so the token defends nothing and is gone. The reviews endpoint gives "a review, not a comment" for free. Behaviour is unchanged where it matters: case-insensitive, anywhere in the body, submitted reviews only, and still not head-sensitive - review state is not consulted, because the ruleset dismisses stale reviews on push and consulting state would silently un-stamp a PR the owner had already stamped. Verified against the real data rather than fixtures alone: PR #206 reads LGTM-present, #298 and #299 read absent, which is correct in all three cases. The self-test keeps the two real spellings the repo's history contains - Lgtm on #84, and the mid-sentence form ending in a question mark on #73 - plus a negative control proving the check can fail at all. 15 lines and 11 cases, from 593 and ~55.
astubbs
added a commit
that referenced
this pull request
Aug 14, 2026
…298) Two merged PRs, #264 and #277, carry no LGTM in any form - no review, no comment. The requirement is that the owner reviews everything that goes in, and the gap was not a lapse of intent but of visibility: "have I read this one myself yet?" is a thing to carry across a dozen open PRs, and carrying it does not work. So it becomes a check with its own name. `bin/check-human-lgtm.sh` asserts one thing: the owner has left a REVIEW whose body contains "lgtm", any case, anywhere in the body. The reviews endpoint is the rule rather than an implementation detail - "lgtm" typed into the ordinary comment box is not the deliberate act being asked for - and the endpoint gives that distinction for free. The matching rule was settled by looking rather than by taste. All 50 owner LGTMs this repo has received, across 38 PRs from #63 to #292, are COMMENTED reviews; 49 are lower-case `lgtm` and one, on #84, is `Lgtm`. Four carry a trailing clause, one of them ending in a question mark. A case-sensitive or whole-word rule would have gone red on real history, so both of those real spellings are pinned as cases. NOT HEAD-SENSITIVE, deliberately: an LGTM on any commit counts for the whole PR, permanently, because the owner only stamps a PR once it is near merge. That is why review STATE is not consulted at all - the master ruleset sets dismiss_stale_reviews_on_push, so reading state would silently un-stamp a PR the owner had already stamped. A SEPARATE JOB, NOT A SECOND STEP IN claude-review, and that is the substance of this change rather than a packaging choice. Two checks say WHICH half is missing straight from the checks list - "claude-review" red means no automated review, "review: human LGTM" red means the owner has not read it yet - and one combined check is red either way, which is exactly the question the human half exists to answer. It also leaves claude-review byte-identical to master: no rename, so no transitional duplicate job and no ordered ruleset swap, and a required check matched by name cannot be left pointing at nothing. Making the new context required is additive, once it has merged and is reporting. NO BOT EXEMPTION, unlike the automated half. A Dependabot PR does not need an AUTOMATED review, but it is still a change going in. Having no guard also means there is no job to skip - and a skipped job satisfies a required check, so a guard keyed on github.event.sender would have let a human PR synchronized by a bot go green having asserted nothing. The checker is 15 lines. An earlier revision was 593, with 855 lines of self-test, parsing code fences, blockquotes, negation forms, glued repeats, CRLF and typographic apostrophes - defences against an attacker who is also the only person the check protects. Filtering the reviews with jq on .user.login before any text is read removed both the attack and the unguessable per-run marker token it needed, since a stranger's body can no longer be attributed to the owner. Verified against real data, not only fixtures: #206 reads LGTM-present, #298 and #299 read absent. The self-test carries a negative control proving the check can fail at all.
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.
Makes green checks mean "mergeable". Today, when a test is red on master's gating CI but its fix lives in another open PR, every merge decision requires manual triage: "is this red the known flake, or did this PR break something?" That's error-prone - and the alternative,
@Disabled, loses the signal entirely (a "known flake" can be a real product bug: the drain-zombie investigation started as exactly that dismissal).The quarantine lane
@Quarantined(reason, tracking, fixedBy)(core shared test sources, meta-taggedquarantined):reasonandtrackingare compiler-required, so no quarantine without diagnosis is enforced by the type system.Gating suites exclude the tag (pom default +
ci-unit/ci-integration/ci-buildscripts) - required checks go green.docs/QUARANTINED_TESTS.md- the live registry / task list of quarantined tests. Enforced, not advisory, by two checks:bin/check-quarantine-registry.shfails on any drift vs the annotations in both directions (missing entry OR stale entry), andbin/check-quarantine-owners.shverifies each entry's owner claim against reality via gh - owning PR must exist and be open (closed-unmerged = orphaned = error; merged-but-still-quarantined = re-enable overdue = error), and once the quarantine reaches the owner's base branch, its merge preview is checked for actually removing the annotation (advisory until it does; the base-not-updated case is detected explicitly to avoid a false "removes it" verdict).Non-gating "Quarantined Tests" CI job keeps running the lane on every PR: skips when empty; registry drift is the only thing allowed to turn it red (real + actionable); quarantined-test failures never do (step-level
continue-on-error- the lesson from the Kafka-compat job's red-X noise). Its step summary shows pass/fail, the ownership audit, and per-test results; all-green is flagged as "fixes may have landed - check whether annotations can be deleted."Re-enable = the owning fix PR deletes the annotation AND the registry entry in one commit after merging master - atomically restoring the test to the gating lane.
Releases are blocked while any test is quarantined -
release.ymlgains a guard beside "refuse to release a red master": real releases hard-fail while the lane is non-empty; dry runs warn (so a rehearsal surfaces the block). Snapshots deliberately still publish (dev artifacts; master is always-SNAPSHOT).Rules (in AGENTS.md + the annotation javadoc)
First occupant
PartitionStateCommittedOffsetIT.committedOffsetRemoved- the[latest]nudge race (pre-await tail-nudge leapfrogged by a slowauto.offset.reset=latestresolution → await unwinnable at any timeout). Owner: #80 (awaitWithTopicNudge, 20/20 clean acceptance) - #80 deletes the annotation + registry entry once it has master merged in.Verification
OffsetResetStrategyparams run.Merge order
Queue-jumps like #83: this is what makes every other open PR's checks trustworthy, and #80 needs it merged first (its re-enable step depends on it). Known trivial downstream conflicts: #83's
excluded.groupspom line (performance,chaosvsperformance,quarantined→ union) and possibly an adjacent hunk in #80's edit of the same test file.Also parks the user-facing upstream-issue-mirroring plan in
docs/inflight.md(one issue per invocation, first-class dry-run).🤖 Generated with Claude Code
https://claude.ai/code/session_01VbtaCN1Re14pJ99VFW4R18