ci(review): let the PR reviewer verify claims instead of trusting them - #102
Conversation
… author The review workflow fires on pull_request open/synchronize - BEFORE the CI test jobs finish - so 'just read the CI results' was never available to it. With no execution grants it could only restate whatever the author's PR description claimed. PR #100 is the cautionary case: a regression test was named MockConsumerTestWithRebalanceInProgressException, which matches none of surefire's default include patterns, so it was never collected. The suite was green precisely because the new test never ran, and a review that read the author's '622 tests passing' table would have waved it through. Only running the suite - and checking WHICH tests ran - catches that. - grants bin/ci-unit-test.sh and bin/ci-integration-test.sh (the repo's own wrappers, so the reviewer runs what CI runs, with the same group exclusions and profile) rather than blanket Bash(./mvnw:*) - adds JDK 17 + Maven cache, without which any invocation would fail or burn its budget re-downloading dependencies - fetch-depth 0, since comparing against the PR base needs history - appends a system prompt telling the reviewer it MAY execute tests, that CI results are not available to it, to verify a new test was actually collected, and to say so plainly if it could not verify rather than implying it did - caps the job at 30 minutes so a wedged suite cannot hold a runner Chaos is deliberately NOT granted: it needs Docker and takes ~2 minutes per scenario arm, far too slow for a per-PR review, and it is non-gating. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
Dependency ReviewThe following issues were found:
License Issues.github/workflows/claude-code-review.yml
OpenSSF Scorecard
Scanned Files
|
Per AGENTS.md's reference convention ('link the PR/issue'). Added at
PR-creation time, since the entry necessarily predates the number.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
✅ 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 |
|
@claude please review |
|
Claude finished @astubbs's task in 3m 40s —— View job Review:
|
Review feedback on #102: a finding that should block the merge has to be an inline review comment, because the master ruleset sets required_review_thread_resolution - an unresolved thread is the ONLY thing that actually gates a merge here. As written, this workflow had the reviewer post a single PR-level summary comment, which nothing forces anyone to resolve. A genuine blocking finding could be merged straight past. The inline-comment tool was already granted; the agent was never told when to use it. The system prompt now requires blocking findings (correctness bugs, data loss or corruption, security holes, broken public contracts, or a test that cannot do what it claims) to go inline via create_inline_comment, one thread per finding, and explains WHY - so the instruction survives a model that reasons rather than pattern-matches. Deliberately NOT converting the whole review to inline threads: every routine nit would then be merge-blocking, you would resolve a dozen threads to land a docs change, and the gate would stop meaning 'something is actually wrong'. Gates that are routinely dismissed get dismissed when it matters. Scarcity is what keeps them meaningful. There is an explicit tie-breaker for the ambiguous case: if unsure, summarise and say why, rather than blocking on a maybe. Also bumps to pull-requests: write, since creating inline comments writes to the PR. It may be unnecessary - the summary comment posts fine under read, suggesting the action uses its own app token - but a silently failing inline comment would mean a blocking finding never gates the merge, which defeats the purpose. Rationale is in a comment beside the permission; drop it back if redundant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
A #103 reviewer could not run bin/todo-index.sh --check to confirm the generated index was regenerated - it had the test wrappers granted but not the repo's own verification scripts - so it had to infer freshness from the diff. That is the exact 'author asserts vs actually verified' gap this whole workstream is about. Grants the fast, read-only VERIFICATION scripts: bin/check-copyright- headers.sh, bin/test-check-copyright-headers.sh, bin/check-quarantine- registry.sh, bin/check-quarantine-owners.sh, bin/todo-index.sh. All are executable with shebangs, so the bare-path grant matches the existing ci-*-test.sh convention. The system prompt now tells the reviewer they exist and to prefer them over trusting the PR body. NOT the quarantine/test RUNNERS (quarantined-test.sh, quarantine-lane-report.sh): those execute suites, are slow, and test execution is already covered by the ci-*-test.sh wrappers. Deliberately still an allowlist, not blanket Bash(*): this job has no fork guard beyond sender.type != Bot - fork PRs are currently protected only by not receiving secrets, so the action fails rather than runs - and it reads attacker-influencable text (diff, PR body, comments). An enumerable allowlist is the margin against injection-into-execution; blanket shell here plus the pull-requests: write added for inline comments would be a pwn-request waiting for a trigger change. Reasoning recorded in the workflow comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
|
@claude please review again, paying special attention to the changes since last review |
|
Claude finished @astubbs's task in 2m 25s —— View job Review:
|
Both findings were correct. 1. Bash(bin/todo-index.sh:*) referenced a script that does not exist on this branch. It is added by #103 (docs/todo-index), which is not merged, so from here the grant is inert and the prompt instruction and changelog line claimed a capability that does not exist. The reviewer was right to check with find/git log rather than take the commit message's word for it; it was wrong only about the script never existing anywhere, since it lives on the unmerged #103 branch. Landing an unverified claim in a PR whose whole thesis is 'do not let assertions go unverified' is exactly the failure this workstream exists to catch. Dropped from the grant list, the system prompt, and the changelog; the grant belongs in #103 alongside the script. 2. Bash(./mvnw --batch-mode -Pci -pl parallel-consumer-core test:*) contradicted the comment sitting directly above it, which says the reviewer runs the repo's wrappers so it runs what CI runs. That bespoke invocation skipped -Dexcluded.groups=performance,chaos, quarantined - which the wrappers hardcode because it was once a silent no-op for unit tests, a real P1, now pinned by QuarantinedAnnotationContractTest for the wrappers but NOT for a workflow-embedded command - was core-only, and dropped clean, -Dlicense.skip and rerunFailingTestsCount. Removed: if the reviewer runs tests, it runs what CI runs. Adds a comment recording both rules, so the next person does not reintroduce either: every grant must be a script that exists on THIS branch, and there is deliberately no bespoke mvnw grant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
|
Both findings were correct and are fixed in the latest push. Thank you for checking with 1.
|
|
@claude last review please |
|
Claude finished @astubbs's task in 2m 6s —— View job Review:
|
Four things #102 does not resolve, so they are not lost at merge: - A green 'review' check can mean the reviewer never ran. claude-code-action refuses to run when the workflow file differs from the default branch's copy, and reports that skip as SUCCESS. Any PR editing a validated workflow therefore gets a green review check that verified nothing - it bit #102 and #104, both workflow changes. The control itself is correct (it stops a PR rewriting the reviewer's own grants and having that version execute), but it is invisible unless you read the job log, so it needs saying out loud. - Credential exposure is unresolved, not cleared: the job runs PR-authored build code alongside CLAUDE_CODE_OAUTH_TOKEN, and #102 widened the blast radius to pull-requests: write. Bounded by fork PRs not getting secrets and by the allowlist - neither of which answers the actual question about token scrubbing. - The reviewer cannot lint workflows: actionlint is not granted, so on a workflow PR it can only eyeball YAML, as it said itself on #102 ('trusted, not verified'). Cheap to grant, but it must land in a non-workflow PR or the validation skip means it is never exercised. - bin/ci-integration-test.sh may not fit the 30-minute cap, and none of #102's setup (cache, Docker, the grants) has actually executed yet - same reason. Filed under CI reliability / gate issues rather than refactoring.md: these are in-flight gate concerns tied to open PRs, not deferred code work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
astubbs
changed the title
…esn't exist Two findings from the #101 review rounds. 1. gotK1 was write-only state, left behind when the old while (!gotK0 && !gotK1) loop condition was replaced by the event-based wait. Removed. 2. docs/inflight.md claimed the remaining awaitForSomeLoopCycles/sleep waits in this class were 'filed under Test infrastructure - timing-based waits in docs/refactoring.md'. That section exists only on #103's unmerged branch - zero occurrences here or on master - so the ledger cited a reference to nothing, in the PR whose own thesis is 'do not let something look tracked when it is not'. The reviewer flagged it three rounds running. Reworded to say #103 adds that entry and to file them there once it merges, rather than claiming they already are. Same mistake as the bin/todo-index.sh grant on #102: cross-referencing content that lives on another unmerged branch. Full reactor after both: 11 modules, 740 tests, 0 failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
Review on #103 noted the scan covers .java/.sh/.xml/.yml/.yaml but not docs, and that the omission was reasonable but unexplained in the script's own scope comment. It was the missing rationale that was the gap, so this adds it - with evidence rather than assertion, because 'surely docs could contain a TODO too' is a reasonable first reaction. Scanning them was tried. It added 4 hits and not one was a marker: three were docs/refactoring.md QUOTING markers the index already lists from their .java source, so the same work appeared twice and the count inflated, and the fourth was prose ('seeded from a code scan (TODO/FIXME + ...)'). That is structural, not bad luck - refactoring.md is where marker triage lives, and AGENTS.md / inflight.md / CHANGELOG.adoc describe this tool, so scanning docs means indexing the index-of-work. The generated index is the reductio: on its own it accounts for ~95 self-referential hits. Also keeps two things the experiment surfaced, needed regardless: - a filter for compound names (todo-index.sh as a FILENAME, not a marker). This is live, not theoretical: merging master brought in #102's workflow comment naming the script, and the scanner was counting it as a 92nd marker. --check caught it, which is a decent self-validation of the tool. - excluded belt-and-braces, so a future widening cannot make the index index itself. Index back to 91 markers, --check clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QqHpNSXC39ANv9kG1ZvUzn
…actoring backlog (#103) 91 TODO/FIXME/XXX markers were scattered across the tree with nothing tracking them. This adds a generated inventory, and triages the useful ones into the backlog that already exists rather than a new one. docs/TODO_INDEX.md - generated inventory bin/todo-index.sh walks the tracked tree and writes every marker, grouped by module with file and line. --check exits non-zero when the committed index is stale, so it can be wired into CI later. This does not overlap docs/refactoring.md's existing // TODO(refactor): convention - that captures 3 markers; this inventories all 91, a population nothing was tracking. Triage goes in docs/refactoring.md An earlier revision of this PR added a separate docs/TODO_TRIAGE.md. That was wrong and has been removed. docs/refactoring.md was already a 295-line backlog with a release-gated 'Breaking changes queued for next major version' section (already listing the deprecated commitInterval options I had 'discovered'), a 'Remove static state' section carrying upstream refs (confluentinc#405, #126, #143) and design branches I did not have, and per-file entries for #233, the encoders and WorkContainer. My triage re-derived that taxonomy from scratch with worse provenance. The root error: concluding 'no breaking-changes list exists' after grepping only docs/inflight.md. Too narrow a search to support the conclusion drawn from it. Two findings genuinely absent from refactoring.md were folded into it: the loop-cycle waits in ParallelEoSStreamProcessorTest (not a style point - that class is on the known intermittent-failure list, and cycle-counting IS the flake mechanism), and the MockConsumer .groupMetadata() workaround copy-pasted across four example tests (one Apache Kafka defect, four times; not ours to fix, but the duplication is). The guardrail is the durable part AGENTS.md gains a 'Where things live' table - one row per document, what it owns and what it is explicitly NOT for, since the boundaries are what get guessed wrong. There was no docs index at all: pointers were scattered across six sections, and docs/SELF_HOSTED_RUNNER.md was linked from nowhere. That layout only works if you already know which section to read, which is circular - and is how this PR went wrong. An earlier guardrail attempt lived inside the 'Refactoring backlog' section, so it only reached someone who had already found the file it was warning them about. The warning is now at the top, before the mistake rather than after it. Why .adoc/.md are not scanned Raised in review, and recorded as a comment in list_files() rather than left as an unexplained omission. It was tried: scanning docs added 4 hits and not one was a marker. Three were docs/refactoring.md QUOTING markers the index already lists from their .java source - so the same work appeared twice and the count inflated - and the fourth was prose. That is structural, not bad luck: refactoring.md is where marker triage lives, and AGENTS.md / inflight.md / CHANGELOG.adoc describe this tool, so scanning docs means indexing the index-of-work. The generated index is the reductio - alone it accounts for ~95 self-referential hits. The comment also says what to do instead: if a genuine marker lands in prose, move it into the code it concerns, where the scan will find it. Marker detection 101 on the first pass, 91 real. Excluded with reasons in the script: shell variables (todo=(), ${todo[*]}), YAML field keys (upstream-map .yaml uses todo: as a schema field), string literals, prose referencing a marker elsewhere, and compound names where the word is part of a filename (todo-index.sh). That last is live, not theoretical: merging master brought in #102's workflow comment naming the script and the scanner counted it as a 92nd marker until filtered - caught by --check, a fair self-validation of the tool. One self-inflicted bug found while building it: the anchored exclusions were applied to grep -n output, so ^ never matched and they silently did nothing. Noted in the script so it is not reintroduced. Not wired into CI as a gate. --check exists for it, but making it gating means every added or removed marker fails the build until regenerated - worth deciding deliberately, not as a side effect here. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
AGENTS.md already said changelog entries must link the PR ("a bare #NN
refers to this fork ... and link the PR/issue"). It was missed anyway,
for a structural reason rather than carelessness: the entry is written
BEFORE the PR exists, so at authoring time there is no number to cite,
and nothing afterwards prompts you to return. #100, #101, #102 and #103
each shipped an entry with no self-reference, and each was caught one at
a time by a human reading the diff. That is the profile of a rule worth
making mechanical.
The gate: when a PR ADDS a new CHANGELOG.adoc entry, at least one added
entry must reference this PR (pull/NN or #NN).
Deliberately narrow, to avoid false failures:
- only genuinely new asciidoc bullets count, so reflows and edits to
older entries are ignored;
- word-bounded matching, so #100 does not match inside #1000;
- opt-out for a PR that touches the changelog without adding an entry of
its own: `changelog-ref: N/A - <reason>` on its own line in the body,
reason required, mirroring the checklist's existing N/A convention;
- bots exempt, consistent with the checklist gate;
- no changelog change at all means nothing to check - whether an entry
was NEEDED is already the checklist item's job.
The logic lives in .github/scripts/changelog-ref-gate.js rather than
inline in the workflow, and is unit tested (28 tests, plain node, no
runner) with the tests running BEFORE the gate, so a regression fails
the job loudly instead of silently mis-judging PRs. That is not
gold-plating: this logic shipped two real misjudgements during review -
an opt-out regex a body merely QUOTING the syntax could trip, and
positional add/remove pairing that mis-read a block holding both an edit
and a new entry. Mirrors the existing copyright.yml self-test step.
Pairing is by CONTENT, not position: an added bullet reads as an edit of
a removed one when they share a (#NN) citation, or failing that when
their wording overlaps enough. Position alone turns -A,-B,+A',+NEW,+B'
into exactly the wrong answer - +NEW consumed as an edit and never
checked.
KNOWN LIMITATION, raised in review and confirmed by construction. When
NEITHER bullet carries a citation, pairing falls back to word overlap -
and this changelog is full of same-template entries ("build(deps): Bump
<lib> to <version>"). Given a removed "Bump Testcontainers to 1.20.0", a
genuinely new "Bump Kafka to 3.9.0", and a reworded edit of the
Testcontainers line, the gate pairs the NEW line as the edit: it escapes
the citation check while the real edit is falsely flagged. Wrong both
ways, and the escaping half is the false negative this gate exists to
prevent.
Not tuned blind - the threshold is a considered choice and a guessed
replacement could easily be worse. Instead it is pinned by a regression
test that states outright it records ACCEPTED rather than DESIRED
behaviour (a failure caused by the gate getting stricter is an
improvement), and documented next to EDIT_THRESHOLD with the direction
to take if it bites: FAIL CLOSED, so an uncertain pair is treated as a
new entry needing a citation. That trades a silent miss for a visible
false alarm - and the false alarm has an escape hatch, whereas the miss
has nothing.
Needs pull-requests: read to fetch the patch, and an actions/checkout
(persist-credentials: false, since nothing there consumes secrets) so
the module and its tests are on disk.
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Description
The automated PR reviewer could never check a test claim — it can now run the suites itself, and its blocking findings now actually gate the merge.
This workflow triggers on
pull_request(opened/synchronize), which fires before the CI test jobs finish. So "read the CI results" was never available to the reviewer, and with no execution grants it could only restate whatever the author's PR description asserted.That is not hypothetical. On #100 the reviewer said as much in its own review:
And it mattered: that PR added a regression test named
MockConsumerTestWithRebalanceInProgressException, which matches none of surefire's default include patterns. The suite was green because the new test never ran. A review that trusted the author's totals would have waved through a permanent no-op. Running the suite — and checking which tests ran — is the only thing that catches that class of bug.What changed
Execution
bin/ci-unit-test.sh,bin/ci-integration-test.sh— rather than blanketBash(./mvnw:*): the wrappers pin theciprofile and the group exclusions, so the reviewer runs what CI runs instead of some variant of it.bin/check-copyright-headers.sh,bin/test-check-copyright-headers.sh,bin/check-quarantine-registry.sh,bin/check-quarantine-owners.sh— so the reviewer can re-run what a PR claims rather than inferring it from the diff.fetch-depth: 0. Without these the grants would be useless — the job had no JDK at all, so every invocation would have failed outright or spent its budget re-downloading dependencies, andfetch-depth: 1cannot reach the PR base to diff against.Blocking findings now gate the merge
masterruleset setsrequired_review_thread_resolution, so an unresolved review thread is the only thing that actually gates a merge. A blocking finding written only in a PR-level summary comment can be — and would be — merged straight past.create_inline_comment, one thread per finding.pull-requests: write(wasread). It may be unnecessary — the summary comment posts fine underread, suggesting the action uses its own app token — but a silently failing inline comment would mean a blocking finding never gates the merge. The rationale sits beside the permission; drop it back if it proves redundant.Bounds
ci-*-test.shwrappers.Bash(*). This job has no fork guard beyondsender.type != Bot— fork PRs are protected only by not receiving secrets, so the action fails rather than runs — and the reviewer reads attacker-influencable text (diff, PR body, comments). The allowlist is the margin against injection becoming execution.Two defects the reviewer caught in its own review
Both fixed here, and both worth recording since this PR's thesis is "don't let assertions go unverified":
bin/todo-index.sh— a script that exists only on unmerged docs: index every TODO/FIXME/XXX marker, triage into the existing refactoring backlog #103, so on this branch the grant was inert while the changelog advertised it as shipped. Dropped; it belongs in docs: index every TODO/FIXME/XXX marker, triage into the existing refactoring backlog #103 alongside the script. The workflow now carries a standing rule: every grant must be a script that exists on THIS branch../mvnw --batch-mode -Pci -pl parallel-consumer-core testgrant that contradicted the comment directly above it, skipping the-Dexcluded.groups=performance,chaos,quarantinedthe wrappers hardcode. That exclusion was once a silent no-op for unit tests — a real P1 — andQuarantinedAnnotationContractTestpins it for the wrapper scripts but cannot pin a command embedded in a workflow string. Removed.Known limitation — this PR cannot verify itself
claude-code-actionrefuses to run when the workflow file differs from the copy on the default branch ("Workflow validation failed … must have identical content to the version on the repository's default branch") and reports that skip as success.That is a deliberate and correct control — it stops a PR rewriting the reviewer's own grants and having that version execute — but it means:
reviewcheck verified nothing, and a silent skip is indistinguishable from a clean pass;@claudementions, which run a different, unmodified workflow.Open follow-ups are recorded in
docs/inflight.mdunder CI reliability / gate issues: whetherCLAUDE_CODE_OAUTH_TOKENis reachable from a Bash subprocess running PR-authored build code (unresolved either way, andpull-requests: writewidened the blast radius); thatactionlintis not granted, so the reviewer cannot lint the very artefact this PR changes; and whetherbin/ci-integration-test.shfits the 30-minute cap.Notes
actionlint.claude_argsvalues are kept to single lines on purpose, so the change doesn't depend on how the action parses multi-line quoted arguments.Checklist
CHANGELOG.adoc) - under== Unreleased→ Build & CI, citing this PR;README.adocregenerated from itdocs/inflight.mdgains the open review-agent follow-ups under CI reliability / gate issues; the workflow's own inline comments carry the per-decision rationaleactionlint, and see the known limitation above: the behaviour itself cannot be exercised until after mergeubuntu-latest(GitHub-hosted, not the self-hosted box). Grants are six repo-owned scripts rather than arbitrary Maven or shell, and the job is time-capped.pull-requests: writeis added for inline comments. The unresolved credential-exposure question (PR-authored build code running alongside the OAuth token) is recorded indocs/inflight.mdrather than claimed as handled.🤖 Generated with Claude Code