fix(core): a commit rejected after leaving the group is no longer recorded as successful - #108
Merged
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Claude finished @astubbs's task in 7m 54s —— View job Code Review
SummaryThis is a correct, well-scoped fix, and I verified it empirically rather than just reading it. The fix logic checks out. Verified empirically, not just read:
Call-site coverage confirmed: Minor observations (non-blocking):
No issues found. Changelog/README entries accurately describe the change and are consistent with the diff. This is ready to merge once #100 lands and this branch is rebased onto it, per the PR notes. |
astubbs
added a commit
that referenced
this pull request
Aug 3, 2026
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
|
Claude finished @astubbs's task in 2m 51s —— View job Claude's reviewTasks
SummaryThis is a clean, well-scoped fix with the same shape as #100, and it holds up under a close read of the code paths involved.
Minor, non-blocking observations
Note on verificationI was unable to execute No blocking issues found — this looks ready to merge once #100 lands and this is rebased, per the PR's own note. |
✅ 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
✅ 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 |
astubbs
added a commit
that referenced
this pull request
Aug 3, 2026
…read (confluentinc#857 family) (#100) A commit landing while the consumer group is rebalancing killed the whole PC instance. Kafka signals this with RebalanceInProgressException and resolves it by completing the rebalance on the next poll() - it means "not yet", not "failed". ConsumerManager.commitSync has a deliberate ladder for exactly this family (CommitFailedException, TimeoutException, SaslAuthenticationException), but this one - the closest sibling of the first - was missing from it. The consequences were wildly out of proportion to the cause: 1. it escaped BrokerPollSystem.controlLoop(), which logs "Unknown error" and rethrows, permanently killing the broker-poll thread; 2. that thread is the ONLY producer of commit responses, so the control thread's commitAndWait() blocked for the full offsetCommitTimeout and then threw "Timeout waiting for commit response" - a symptom pointing nowhere near the cause; 3. the control thread died too, taking the instance with it, and the close path then failed to commit because the poll thread was gone. One retriable protocol blip killed the consumer. In production this is the confluentinc#857-family "locks forever until manual restart" signature. The fix is where it is on purpose. Catching this in ConsumerManager.commitSync, next to its siblings, is the obvious placement and is WRONG - it was tried first. AbstractOffsetCommitter.retrieveOffsetsAndCommit() calls onOffsetCommitSuccess() unconditionally once commitOffsets() returns, so swallowing it there makes PC record a commit that never reached the broker: partitions go clean, nothing is retried. The unit test caught that. So it is caught one layer up, in ConsumerOffsetCommitter, where the exception still aborts before the success marking. Offsets stay dirty and are genuinely re-committed next cycle. The commit response is sent even on deferral, so waiting committers are released immediately rather than waiting out offsetCommitTimeout for a commit that is not coming. Net main-code change: 4 functional lines. Found via the Chaos Pain Suite. W4 revoke-under-work went RED on every highcpu run for days, across branches with no related changes. Both arms failed, so it was not the cooperative variant's problem - but cooperative rebalancing EXPOSES it, because cooperative members keep committing during rebalances by design, which is exactly the race window. The bug itself is long-standing and inherited from upstream: the ladder arrived with 29795bf (upstream confluentinc#819) and no fork commit touched it. Not fixed by #80, which was verified present in a run that still went red. Verified: - chaos seed 8254214163208094917, the exact CI failure: RED -> GREEN - CI chaos, both W4 arms green: 181 deferrals handled, 0 poll-thread deaths, 0 commit-response timeouts, 0 unclassified-failure assertions. The exception still fires 181 times - that is the point. It is meant to; it is just no longer fatal. - MockConsumerRebalanceInProgressTest: broker-free reproducer that reduces a 2-minute chaos IT to a ~1-second unit test, failing in ~30s against unfixed code - full reactor: 11 modules, 742 tests, 0 failures The investigation is written up in docs/plans/2026-08-01-001-investigate-chaos-w4-red-report.md, including the load-starvation hypothesis it originally led with, recorded as FALSIFIED rather than quietly dropped. Follow-ups deliberately not in this PR, recorded in docs/inflight.md: ConsumerManager.commitSync's CommitFailedException handler has the same swallow-at-the-wrong-layer flaw (#108); commitAndWait interpolates DEFAULT_TIMEOUT into an error whose actual wait is offsetCommitTimeout, so every such message misstates it by 3x; and the highcpu lane's concurrency saturates its own runners. Relates to upstream confluentinc#857. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Base automatically changed from
fix/commit-rebalance-in-progress-kills-poll-thread
to
master
August 3, 2026 04:21
astubbs
changed the title
astubbs
force-pushed
the
fix/commitfailed-marks-uncommitted-offsets-clean
branch
from
August 3, 2026 04:54
777dce9 to
66ae845
Compare
|
Claude finished @astubbs's task in 3m 14s —— View job Claude's reviewTasks
|
5 tasks
4 tasks
astubbs
added a commit
that referenced
this pull request
Aug 4, 2026
…disabling all mutation testing (#110) ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect failed roughly 1 run in 6, and PIT refuses to score any mutants while a test is unstable without mutation - so that one race turned the whole Mutation (PIT, scoped) lane red and nothing was measured. The visible cost was never "a test is flaky"; it was that mutation testing measured nothing, and a green lane had become a proxy for suite stability rather than mutation coverage. Second test to do this - #101 fixed queuedMessagesNotProcessedOrCommittedIfSubmittedDuringShutdown for the same reason. The failure looked like an exactly-once bug. The commit captured offset=1 where the test wants 2, AFTER both records had been produced - a transaction holding two produced records while committing an offset saying the second was never consumed, which is the literal EOS violation this test's name forbids. The metadata was 'bgAA', the encoded incomplete-offset payload, so PC was not confused: at that instant offset 1 genuinely was incomplete. It was asked at the wrong moment. Worth taking seriously, because #100 and #108 were both "offsets recorded as committed when they were not" and both looked like flaky tests first; this one is on the producer path and could have been the third. It is the test that creates the moment. It hand-rolled its user function, acquired the produce lock against a MOCK context, and released it in its own finally - inside the user function, before the wrapper's addToMailbox. That opens a window where the produce lock is free but the work has not reached the controller's inbound queue, so the controller takes the commit lock, drains a mailbox missing the completion, and collects offsets one behind. Production has no such window, and WorkContainer#onPostAddToMailBox states the rule outright: unlock only once the work is safely back in the controller's inbound queue, precisely so the commit lock cannot be acquired until those offsets are in the commit payload. ParallelEoSStreamProcessor hands the lock to the real context and release happens post-mailbox. The test was at neither release point - and its own TODO said so: "this unlocks the produce lock too early - should be after WC returned." So the test now does what production does: acquire against the real context, hand the lock to it, no manual unlock. No main-code change - the exactly-once invariant was never violated in production. Diagnosed by controlled experiment rather than by the fix appearing to work. An identical 400ms delay injected AFTER the unlock failed 8/8; the same delay BEFORE it passed 8/8. Same latency, opposite side of one call, which rules out "the CI box is just slow" - the conclusion every previous look reached, and note the timeouts had already been widened for PIT in 9e133ce and the flake survived that. Also adds a guard, because removing today's instance invites tomorrow's: the test asserts the produce lock is still owned by the context when the user function returns, since that ownership is what defers release to onPostAddToMailBox. Reintroduce manual lock management and it fails deterministically instead of returning as a 1-in-6 flake. Verified by negative control - clearing the lock from the context makes the test fail. Verified: 12/12 green against a ~1/6 baseline, full unit suite 371 tests with 0 failures, and PIT now gets past the stage that used to abort ("Calculated coverage in 332 seconds" -> "Created 18 mutation test units", zero occurrences of "did not pass without mutation"). Documentation carries the reasoning, not just the outcome: docs/plans/2026-08-03-001-investigate-transactional-commit-flake.md has the full investigation; docs/inflight.md's entry is retired (its original Mockito-race diagnosis and its rerunFailingTestsCount suggestion were both wrong, and would have aimed the next reader at the wrong cause and the wrong remedy); and the double-unlock question this surfaced is promoted to a tracked item rather than left as a footnote - onPostAddToMailBox and cleanUpContext both release the same ProducingLock with nothing clearing it in between, which should throw IllegalMonitorStateException and does not, and this fix now drives that path for real where the mock-context version never did. Two false negatives worth not repeating, both of which nearly sent this the wrong way: ./mvnw -pl without -am fails the ReactorModuleConvergence enforcer, so the first run of the experiment silently tested a stale class and showed no effect; and surefire:test alone does not reprocess test resources, so a logback-test.xml change never reaches target/test-classes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
…orded as successful Kafka throws CommitFailedException when this consumer is no longer a member of the group, so the commit was rejected outright. ConsumerManager.commitSync caught it, logged that the poller would "seek commit later", and returned normally - which is precisely why nothing ever did. Returning normally lets AbstractOffsetCommitter.retrieveOffsetsAndCommit() carry on to onOffsetCommitSuccess(), marking the offsets clean. Once PC believes they are committed, collectCommitDataForDirtyPartitions() stops offering them and no commit is ever attempted again. This is the sibling of #100 and the opposite failure shape. #100 was loud and fatal: the exception escaped, killed the broker-poll thread, and took the instance down with a misleading commit-response timeout. This one is silent - no exception, no dead thread, no stall, just PC's bookkeeping running ahead of the broker. It surfaces later, when a partition changes hands and its new owner resumes behind records PC had already recorded as done. Fixed at the layer that already handles the rebalance case, since the layer is the whole point - the same catch one frame lower is the bug: - ConsumerManager.commitSync: drop the catch, so the exception reaches above onOffsetCommitSuccess() rather than being resolved below it. - ConsumerOffsetCommitter.commitDeferringOnRebalance: catch it beside RebalanceInProgressException. Offsets stay dirty and are re-committed next cycle; waiting committers are still released, so nothing blocks. The "Swallow" arm of that method's three-way javadoc is no longer hypothetical and now names this code as the case in point. MockConsumerCommitFailedTest verifies it broker-free, and is confirmed discriminating: with the two source files reverted, commit attempts stall at the rejection threshold and never resume. The stall IS the bug, so a "no exception escaped" test would have passed against the broken code. It also asserts the offsets are genuinely re-committed via mockConsumer.committed(), rather than inferring it from the attempt count. Both rejection tests are one scenario with one variable, so the duplicated setup is extracted into CommitRejectionTestBase; each subclass supplies only its exception, and a future third rejection reason has an obvious home. Verified: full unit suite green across all 11 modules (371 tests), with both rejection tests collected rather than silently skipped. Relates to upstream confluentinc#857 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
…nd let it run ./mvnw The grants added so the reviewer could run tests never matched anything. --allowedTools entries are PREFIX matches, not globs, so Bash(bin/ci-unit-test.sh:*) does not match ./bin/ci-unit-test.sh - and every natural invocation was denied. The review on this PR is the evidence: 10 permission denials across 41 turns, its own checklist left stopped at "Run tests to verify RED->GREEN claim (in progress)", no final review posted - and the check still went green. A grant that cannot match is worse than no grant, because it reads as a capability the reviewer does not have, and the failure is silent at both ends. Both spellings are now granted for every script. Enumerated per script rather than written as Bash(./bin/*.sh:*), because that glob would not match either - the same bug in a new coat. ./mvnw is granted too, for the one job the wrappers cannot do: running a single test class. Verifying a RED/GREEN claim means reverting the fix and re-running just the new test, which is a -Dtest run of seconds against a ~6 minute suite. Without it, the most valuable check a reviewer can make - does this test actually fail without the fix? - was unaffordable, and the earlier "no bespoke mvnw grant" comment ruled it out on the grounds that a hand-rolled invocation skips the group exclusions the wrappers pin. That objection is kept as guidance instead: whole suite -> wrapper, always; ./mvnw only for a scoped -Dtest run, whose result is never reported as "the suite is green". The system prompt says so, and also tells the reviewer to restore anything it stashes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
astubbs
force-pushed
the
fix/commitfailed-marks-uncommitted-offsets-clean
branch
from
August 4, 2026 01:18
a4f0090 to
9b503ad
Compare
astubbs
added a commit
that referenced
this pull request
Aug 4, 2026
…ntirely This was the generalisable lesson from #108's inflight note, which had to be resolved away during the rebase onto master once #110 rewrote the entry it was updating. The specific flake is fixed and documented; the pattern behind it was about to be lost, so it moves here where it belongs. PIT refuses to run while ANY test is unstable without mutation - it needs a green baseline to attribute kills to mutants rather than noise. So one unrelated flake does not degrade the signal, it switches the whole lane off: zero mutants scored anywhere, regardless of which class flaked or whether it relates to the code being mutated. That has happened twice, both times somewhere unrelated - #101's shutdown-commit flake and #110's produce-lock flake. Three consequences worth holding onto, now written down: - The lane's green-ness has been tracking SUITE STABILITY, not mutation coverage. Weaker signal than the one we thought we had. - rerunFailingTestsCount cannot rescue it. Surefire reruns hide a flake from the unit gate, but PIT does its own coverage run and sees the raw result, so a papered-over flake still kills mutation testing. It has to be fixed. - A red mutation lane usually means "something somewhere is flaky" rather than anything about mutants - check for the "did not pass without mutation" line before investigating mutation config. Filed as section 3.0 rather than appended, because it is a property of the lane as a whole and precedes the specific complaints about scope and targets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
5 tasks
astubbs
added a commit
that referenced
this pull request
Aug 4, 2026
docs/inflight.md appeared in 26 of the last 30 master commits. Unrelated PRs conflicted on it constantly - not because they disagreed, but because their notes were adjacent, and the merge that resolves such a conflict silently drops one side's update. It happened between #108 and #110. So each item is now its own file under docs/inflight/, named <category>-<slug>.md. Two PRs recording unrelated work touch disjoint files and cannot conflict, and finishing work is `git rm` - a deletion, which never conflicts with an edit elsewhere. The category prefix is the structure: `ls docs/inflight/` shows the shape of what is open without opening anything. No committed index. It would be edited by every PR, which is the problem this directory exists to solve; docs/TODO_INDEX.md is the cautionary case, generated and committed and stale until a reviewer caught it. `ls` and `grep -r` are the index. The rules that lived in the old file's header now live in docs/inflight/AGENTS.md, where an agent will find them: delete a file when its work lands rather than rewriting it into a FIXED narrative, delete it in the PR that resolves it rather than leaving a "delete when #NN merges" marker, never record what gh or git can answer, and record known defects here even when an issue exists. This was parked on docs/inflight-as-directory with migrating ~600 lines of stale entries named as the reason not to do it. This branch's audit already did that work, which is why it is cheap now. That parked note is one of the entries the split drops. docs/refactoring.md deliberately stays a single file - 2 of the last 30 commits touched it, so it has none of this problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 tasks
astubbs
added a commit
that referenced
this pull request
Aug 4, 2026
…s stop colliding (#112) The ledger had reached 677 lines and become a record of finished work. Entries told to "delete when #98 merges" outlived three merges; the rebalance-commit fix was still filed as awaiting PR after landing as #100; a static-state flake was still listed as deliberately-unfixed after #101 fixed it at source. Meanwhile #80, carrying the drain-zombie fix and the largest thing actually in flight, appeared only as a footnote about quarantine ownership. The file's own scope rule says entries vanish when work lands; it had stopped being applied. Every surviving claim is now checked against GitHub, git and the code. Obsolete entries are deleted rather than annotated: the jscpd cap is above baseline, no workflow has path filters so the docs-only inconsistency cannot happen, the ManagedPCInstance header carries its Modifications line, the CommitFailedException follow-up shipped in #108, and the stacked-PR gap is closed by a new all-branches ruleset. Upstream references now follow the convention the changelog already uses: fork numbering reaches #111, so every bare reference at or above #162 was silently upstream and read as ours. With the content correct, the file became a directory. It appeared in 26 of the last 30 master commits, so unrelated PRs conflicted on it constantly - not because they disagreed, but because their notes were adjacent, and the merge that resolves such a conflict silently drops one side (it happened between #108 and #110). Each item is now its own file, named <category>-<slug>.md; two PRs recording unrelated work touch disjoint files and cannot conflict, and finishing work is `git rm`, which never conflicts with an edit elsewhere. The prefix is the structure - `ls docs/inflight/` shows the shape of what is open without reading anything. There is deliberately no committed index: it would be edited by every PR, which is the problem the directory solves, and docs/TODO_INDEX.md is the cautionary case. This was parked on docs/inflight-as-directory with "migrating ~600 lines of existing entries" named as the reason not to do it. The audit is what made it cheap. docs/refactoring.md stays a single file - 2 of the last 30 commits touched it, so it has none of this problem. The manifest had drifted the same way, which matters more, because it is the declared source of truth that a future session trusts instead of re-deriving. Five entries disagreed with reality: bug-857 recorded no fork PR while #29 was open, #100 merged and #80 in review; fix-909 recorded no PR though #31 is open; bug-912 was in-progress when the schema's word for pushed-but-unPR'd is ready. Nothing catches this - upstream-map.py validate only checks the schema and upstream-sweep.sh only watches upstream, so "prs: []" beside an open PR passes every check we have. Hence the AGENTS.md rule to update it at every lifecycle transition, not only when starting. Four rules now live in docs/inflight/AGENTS.md so they are inherited rather than rediscovered: delete an entry in the PR that resolves it and never leave a "delete when #NN merges" marker on master; never record what gh or git can answer, which is why the open-PR table is gone; known code defects belong here even when an issue exists, because an agent scans this directory and will not read the tracker; and new guidance about how these notes are written goes into that file too. Four deferred code items moved to docs/refactoring.md, where deferred work lives: the SpotBugs thread-visibility findings, the produce-lock double release surfaced by #110, the commitAndWait message that misstates its own wait, and jacoco's single exec file under forked surefire. Also shares the agent-tooling gitignore rules that until now existed only in one checkout's .git/info/exclude, and deletes an empty duplicate-code-cross-check clone - a git init that never fetched - rather than ignoring it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 4, 2026
astubbs
added a commit
that referenced
this pull request
Aug 6, 2026
…changed Merging master brought three things that bear on this branch. The issue-reference gate (735b1d3) flags a bare `#NN` below #1000 on added lines, because the fork's numbers sit entirely inside confluentinc's range. This branch added 12 such refs across javadoc, tests and the inflight note. Now owner-qualified: #177/#100/#108/#186/#204 for the fork, confluentinc#833 for upstream. #177 is itself the mirror of confluentinc#833, so the pair reads correctly. Left `// fixes github issue confluentinc#809` alone - it is a pre-existing line, not one this PR adds, and rewriting it would be scope creep. The SIGPIPE gate bug this PR diagnosed is FIXED on master (cb8b118) and guarded repo-wide (7586b8c), so the inflight entry claiming it still needs its own PR was stale. Rewritten as resolved, keeping the measurements and the lesson: the failure is positional rather than size-monotonic, shellcheck does not catch it, and small fixtures never reproduce it. The manifest lost the backlink tooling and the `todo` subcommand, so this entry's `forwarded:` block and `todo:` list were writing into fields nothing reads any more. Dropped both; the release-time note moved into `notes:`, and `fork_issue: 177` added to match the shape master's entries now use. Worth recording: cb8b118 called out check-quarantine-owners.sh as latent with 351 bytes of headroom "on a file two open PRs are adding lines to". This is one of those PRs, and it takes AbstractParallelEoSStreamProcessor.java to 67,168 bytes - 1,632 past the 64 KiB buffer. Without that fix this branch would have failed as "annotation missing".
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.
Description
The sibling flaw #100 turned up, now rebuilt on
master(which has #100).#100 fixed
RebalanceInProgressExceptionescaping unhandled and killing the broker-poll thread. While tracing that,ConsumerManager.commitSyncwas found handlingCommitFailedExceptionin a way that looks like the answer but is the other failure this workstream is about:The poller was never going to "seek commit later." Catching there and returning normally means
AbstractOffsetCommitter.retrieveOffsetsAndCommit()carries straight on toonOffsetCommitSuccess(), which marks the offsets clean. Nothing re-commits an offset PC believes is already committed.How this differs from #100
Same root confusion - "Kafka refused this commit" treated as something other than defer and retry - but the two fail in opposite directions:
RebalanceInProgressExceptionCommitFailedException#100 announces itself. This one does not: there is no exception, no dead thread, no stall - only a divergence between PC's bookkeeping and the broker's, showing up later as unexplained duplicate delivery.
That is why the "Swallow" arm of the three-way framing in
commitDeferringOnRebalance's javadoc is no longer hypothetical - it names this code as the case in point.The fix
Handle it where the rebalance case is already handled, one layer up:
ConsumerManager.commitSync- remove the catch, so the exception reaches the layer aboveonOffsetCommitSuccess()instead of being resolved below it.ConsumerOffsetCommitter.commitDeferringOnRebalance- catch it besideRebalanceInProgressException. Offsets stay dirty and are re-committed next cycle; waiting committers are still released, so nothing blocks.Layer matters more than the catch: the same
catchone frame lower is the bug. The two exceptions are unrelated by inheritance, so catch order is not load-bearing.Verification
MockConsumerCommitFailedTest- broker-free, rejects the first 3 commits then accepts.Confirmed discriminating, by reverting the two source files and re-running:
The stall is the bug: once the offsets are marked clean,
collectCommitDataForDirtyPartitions()returns empty andcommitSyncis never called again. A "no exception escaped" test would pass against the broken code and guard nothing.Per review feedback, the test also asserts the offsets are genuinely re-committed -
mockConsumer.committed(tp).offset() == RECORDS- rather than inferring it from the attempt count.Duplication
The two tests are one scenario with one variable, so the ~36 duplicated lines the bot flagged are extracted into
CommitRejectionTestBase; each subclass now supplies only its exception.MockConsumerRebalanceInProgressTestkeeps its full diagnosis in the class javadoc. This also settles where a future third rejection reason goes.Also here: the reviewer's test grants were inert (
claude-code-review.yml)Added to this PR on request rather than as a separate one, because this PR is the evidence for it.
The grants meant to let the reviewer run tests never matched anything.
--allowedToolsentries are prefix matches, not globs, soBash(bin/ci-unit-test.sh:*)does not match./bin/ci-unit-test.sh- every natural invocation was denied.The review of this PR is the proof: 10 permission denials, its own checklist left at "Run tests to verify RED->GREEN claim (in progress)", no final review posted - and the check still reported success. A grant that cannot match is worse than no grant: it reads as a capability the reviewer does not have, and fails silently at both ends.
Bash(./bin/*.sh:*), because that glob would not match either - the same bug in a new coat../mvnwgranted, for the one thing the wrappers cannot express: running a single test class. Verifying a RED/GREEN claim means reverting the fix and re-running only the new test - seconds via-Dtest, against a ~6 minute suite. That turns the most valuable review check from theoretical into affordable../mvnwonly for a scoped-Dtestrun, whose result is never reported as "the suite is green" (it skips the-Dexcluded.groupsthe wrappers pin). The system prompt says so, and tells the reviewer to restore anything it stashes.Consequence: since this PR now modifies the review workflow,
claude-code-actionwill refuse to review it - it requires the workflow to match the default branch, and reports that skip as success. The grants take effect for the next PR after merge.The red PIT check is a known flake, not this PR
Mutation (PIT, scoped) (optional)fails onProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect, a pre-existing flake recorded indocs/inflight.mdsince 2026-07-28 (a Mockito interaction race, ~1/245 locally).It cannot be caused by this PR:
ConsumerManager.commitSynchas exactly one caller,ConsumerOffsetCommitter, and that test runsPERIODIC_TRANSACTIONAL_PRODUCER, which commits via the producer path and never enters it. The test passes inUnit Tests, intests, and in a full local suite on this commit.Worth its own note, though, and now recorded in
docs/inflight.md: one flaky test disables mutation testing repo-wide. PIT refuses to score any mutants while a test is unstable without mutation, so this lane's greenness has been a proxy for suite stability rather than for mutation coverage. This is the second test to do it - #101 fixedqueuedMessagesNotProcessedOrCommittedIfSubmittedDuringShutdownfor the same reason.rerunFailingTestsCountwould not help: surefire reruns hide a flake from the unit gate, but PIT runs its own coverage pass and sees the raw result.Checklist
CHANGELOG.adoc) - aFixesentry for the commit bug, and the existingBuild & CIreviewer entry extended rather than duplicated;README.adocregeneratedMockConsumerCommitFailedTest, verified RED without the fix; sharedCommitRejectionTestBasedocs/inflight.mdrecords that theProducerManagerTestflake disables the whole PIT laneubuntu-latest, not the self-hosted box. The grants do widen what the reviewer may execute, so it stays an enumerated allowlist rather thanBash(*): this job reads attacker-influencable text (diff, PR body, comments) and has no fork guard beyondsender.type != Bot../mvnwis the build wrapper every CI job already runs on the same checkout.changelog-ref: relates to upstream confluentinc#857
🤖 Generated with Claude Code