Repository navigation
ci(review): make the reviewer run on both routes, take the write token out of it, and ask the gate a simpler question - #287
Merged
Conversation
…code Round five's P1. The reviewer job held `actions: write`, checked out `refs/pull/<pr>/head`, and - by design, via --allowedTools - executes this repo's test and check scripts from that checkout. On a PR branch those scripts are whatever the PR says they are, so the job handed PR-authored code a token that can dispatch, cancel or re-run any workflow in the repo, `release.yml` among them. `actions/checkout` also defaults `persist-credentials: true`, writing that token into the .git/config of the very tree whose scripts run next. This is the fork-head refusal (24f9205) asked one level in. That guard settles WHOSE code runs beside the credential; it says nothing about WHAT that code could reach. A same-repository PR does mean a write-access author, but write access to a branch is not the same thing as a token that can fire a release. So the write grant moves out rather than being justified: - `review` drops to `actions: read` - still enough for the reviewer to read the PR's CI results, which the system prompt tells it to do - and its checkout sets `persist-credentials: false`. Nothing after the checkout pushes or fetches, so the persisted credential bought nothing. - `refresh-gate` is a new job holding `actions: write` alone, plus `pull-requests: read` to look up the head SHA. It checks NOTHING out and only talks to the API. The gate re-run step moves there unchanged. The re-run's inputs cross the job boundary as outputs (`reviewed_sha`, `review_outcome`) rather than being re-derived, so the head-moved refusal still compares against the SHA the reviewer actually checked out. `needs:` alone would already skip the job after a failed review; the explicit outcome test keeps that true if anyone later adds `continue-on-error`. Also round five, separately: `docs/data/testing-evidence.yaml`'s review-delivery entry still said the gate "verifies a review comment was posted for that run" and pointed readers at the gate workflow alone. Both went stale when the reviewer moved out of that file - there is no reviewer run in the gate any more, and the gate accepts any qualifying reviewer comment created at or after the head appeared. It now states the two-workflow contract and points at the script that implements the rule. This data feeds published documentation, so a stale posture there is a published claim that is not true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
… out loud
The reviewer has not run once since it moved to `workflow_dispatch`. Its first
live dispatch died before reading a line of the diff:
track_progress is only supported for events: pull_request, issues,
issue_comment, pull_request_review_comment, pull_request_review.
Current event: workflow_dispatch
With the gate strict and live and the auto-review gone from push, that left no
PR in the repo able to turn `claude-review` green.
WHY THE TIDY FIX DOES NOT EXIST. `claude-code-action` picks its behaviour from
the EVENT. On a PR event it runs in tag mode - tracking comment, inline-comment
server, CI server - and on anything else in agent mode with none of them. The
obvious escape is to hand it a `pull_request` payload, since `@actions/github`
reads its trigger from exactly `GITHUB_EVENT_NAME` and `GITHUB_EVENT_PATH`. That
was built and dispatched rather than reasoned about, and the runner discarded it:
it writes the `GITHUB_*` context variables into every step's environment AFTER
the step's own `env:` block. Run 31466525816 measured it -
GITHUB_EVENT_NAME as this step sees it: 'workflow_dispatch'
- and actions/runner agrees in source (`ScriptHandler.PrepareExecution` and
`NodeScriptActionHandler.PrepareExecution` both finish by writing the github
context over `Environment`, with `event_name` on its allowlist), as does GitHub's
own documentation. The payload FILE is writable; the event NAME is not, and the
name alone picks the mode. That route is closed, and the header says so, so the
next person does not spend an afternoon proving it again.
So the run is agent mode, and the three capabilities that costs are paid for
rather than left to be discovered:
1. NO TRACKING COMMENT, and therefore nothing posting a `claude[bot]` issue
comment by itself. The packaged procedure posts one ONLY when it finds
nothing; with findings it posts inline comments and stops. Since the gate
reads issue comments, the reviews that matter most would never have gone
green. The system prompt now makes the summary comment the reviewer's
mandatory last action, and says why: the check is satisfied by that comment
and by nothing else. The author is still `claude[bot]` - the action swaps
`GH_TOKEN` for its own App token before Claude runs, in every mode.
2. NO INLINE-COMMENT TOOL. The action installs that server only for a PR event,
so the grant is REMOVED rather than left inert - a grant that cannot match
reads as a capability the reviewer has, which is the #108 lesson. This is a
real loss and is labelled as one: blocking findings are now a marked section
at the top of the summary comment, and the reviewer is told to state plainly
that nothing mechanical will enforce them, because merges here gate on
unresolved review threads and it can no longer open one.
3. NO CI MCP SERVER. Replaced with `gh pr checks` / `gh run view` / `gh run list`
plus `additional_permissions: actions: read`, which is what gives the
reviewer's own token the scope those calls need. Grant and permission move
together or the calls 403.
`track_progress: false` is explicit, with the error it causes written beside it,
because turning it back on does not degrade the review - it aborts every one.
AND A DEFECT THE EXPERIMENT UNCOVERED. Dispatching this branch got
Skipping action due to workflow validation: The workflow file must exist and
have identical content to the version on the repository's default branch.
and the action exited 0. Every step reported success, the job was green in
eleven seconds having reviewed nothing, and `refresh-gate` re-ran the gate on
the strength of it. Only the gate itself, refusing to find a review comment,
kept the target PR honest. That is the failure class named in
docs/solutions/workflow-issues/a-check-that-reports-success-without-having-run.md.
The job now checks the action's own `conclusion` output - set only on the path
that actually ran Claude - and fails when it is empty, and `refresh-gate` keys
off that conclusion rather than the step's outcome.
That refusal is also a standing constraint, not a one-off: the guard means no
change to this workflow can be exercised before it is on master. Recorded in
docs/ci.md and docs/inflight/ci-review-agent.md so the next person plans for it
instead of rediscovering it.
`.github/actionlint.yaml` gains one narrowly scoped suppression: actionlint
type-checks action outputs against metadata baked into the binary, and its
snapshot predates `conclusion`. Scoped to that message in that file, with a note
to delete it when upstream catches up.
The `pull-requests: write` justification was rewritten: it claimed to be there
for inline review comments, which no longer happen. It stays at write because
the posting path is the action's own App token and nothing here has proven the
workflow token unused - narrowing it is a follow-up with a test behind it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
No new clones introduced by this PR. Powered by astubbs/duplicate-code-cross-check |
✅ SpotBugs ReportNo bugs found (new bugs only — baseline from base branch excluded). |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f77cc8c79d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…nd drop the grant that cannot work Three review findings, all correct on the mechanism. Editing this workflow does not cost you a review. The validation refusal is about which workflow file RUNS, not which code is READ: `--ref master` runs master's copy, so validation passes, while the checkout below fetches refs/pull/<n>/head. Master's reviewer therefore reads the PR's diff and posts the comment the gate wants. Only the PR's NEW reviewer behaviour is unexercisable before the merge, which is a different claim and stays. The bypass is real for exactly one shape of PR - one fixing a reviewer that is already broken on master, which is this PR - and that is now stated as the exception it is rather than as the rule. `gh pr checks` is removed from the grant list and the prompt. It reads check RUNS, which needs the `checks` scope, and this token cannot be given it: the action's own defaults are contents/pull_requests/issues, and `additional_permissions` is documented as supporting `actions: read` and nothing else while forwarding whatever key you write to the token exchange unvalidated. So `checks: read` would be a guess at an undocumented server contract in the critical path of every review, and the grant as it stood would 403. `gh run list -c <sha>` reaches the same runs under a scope that is actually granted. Leaving it listed would have been #108 again - a capability the reviewer appears to have and does not. The in-flight tracker goes back to holding only open work, per its own scoped rule: the settled track_progress question and the settled exits-0 guard are durable in docs/ci.md and are deleted here rather than rewritten into a DONE narrative. What stays is the inline-comment loss, the SHA-gating follow-up, and the one constraint the next editor of this file inherits.
…m, not a footnote The reviewer produced by this PR has never been observed completing a review start to finish - read a PR, post the summary as claude[bot], turn the gate green. Only the pieces were proved on runners. That is open work and belongs here as such, phrased as the first thing to do after the merge, rather than folded into a general note about editing the workflow.
… label it the weaker reviewer The gate refresh shipped on the dispatch route only. `claude-review` is produced by claude-code-review.yml, which triggers on `pull_request` alone, and a check run keeps its last conclusion until something re-runs it - so a review arriving by any other route posts, satisfies the gate's own rules, and leaves the required check red at whatever it recorded at push time. `claude.yml` had no `actions: write` and nothing that re-ran the gate, so the `@claude review this` route could never clear the check it needs to clear. That is not a corner: it is the documented fallback, and while the dispatched reviewer is broken on master it is the ONLY working route. Traced live on #288 - review posted 08:29 for the current head, check still red, and a hand-run `gh run rerun --failed` at 08:37 was the whole fix. So `claude.yml` gets the same two-job split: the `claude` job publishes the action's own conclusion, the resolved PR number and the head SHA as it was before Claude ran, and a checkout-free `refresh-gate` job holds `actions: write` alone. Same conditions as the dispatch route - refresh only after a review the ACTION concluded successfully, only on a PR, and only when the head has not moved underneath it. Refreshing unconditionally is how a failed run's error comment turns a required check green, which is the trap the dispatch route already refused. Two things stated rather than hidden. This makes an existing looseness reachable: the gate cannot tell a review from any other `@claude` answer, so a successful non-review reply on a PR will now refresh it too. That boundary was already recorded as social rather than technical, and the alternative - the only working review route unable to clear the gate - is worse. And the two refresh jobs are near-duplicates, because sharing the body would mean checking out the script, which is the one thing a job holding `actions: write` must not do. Left deliberately undone: the comment route reviews with NO tool grants, so it reads rather than runs. Observed on #288, on a PR whose whole diff was check scripts and their tests. Copying the dispatch route's curated allowlist across is a security decision - that route is triggered by comment text, which anyone who can comment can influence - so it is parked as an explicit open question and the degradation is documented instead, in docs/ci.md and in the in-flight tracker. Nobody should read this route's "no issues found" as equal weight while that holds.
astubbs
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0eb0c82bd8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review finding on the commit before this one, and a correct one. The comment route's new refresh-gate had no fork check, so it dissolved a boundary the repo documents on purpose. "A fork PR cannot turn the gate green" in docs/ci.md is a deliberate design point, not an oversight: the dispatched reviewer refuses fork heads outright, and the gate accepts only a claude[bot] comment, so a fork PR has no route to green and gets merged on a human's judgement or not at all. The comment route breaks the symmetry, because unlike the dispatched reviewer it will happily answer on a fork PR - it executes nothing, so there is no reason it should not. But the gate cannot tell a review from any other finished claude[bot] reply. So refreshing after that answer would have meant a maintainer merely ASKING a question on a fork PR turned its required check green: exactly the self-asserted escape docs/ci.md rejects by name, arriving by accident rather than by argument. Claude still answers on fork PRs. `pr-context` emits no PR number for a fork head, which is what refresh-gate skips on, and the refusal is repeated inside refresh-gate itself. Twice, because it is a security boundary whose failure mode is silent - a fork PR going green looks identical to a fork PR being reviewed - and the second check costs one jq read of a response that step already fetches.
… inline review threads Two decisions, one of which turned out to be smaller than it looked. THE ALLOWLIST. The comment route reviewed with no tool grants, so it read instead of running - visible on #288, where it reported "tool permissions blocked Bash execution" on a PR whose entire diff was check scripts and their tests. It now carries the dispatch reviewer's curated allowlist, copied verbatim and verified identical. The objection was that comment text is attacker-influencable where a dispatch is not, and `@claude` is matched by plain substring. True, and not the operative difference: BOTH routes execute PR-authored code. What protects them is that the list is curated to this repo's own scripts and that the job holding it has no write grant - not that the trigger is trusted. Granting less bought no safety and cost the only working reviewer the ability to check its own subject matter. Blanket Bash was never on the table, and a narrower subset was dropped because running the checks the PR changed is the entire value. `--allowedTools` in `claude_args` ADDS to tag mode's base tools rather than replacing them, so the tracking comment, the CI readers and the git tools all survive. That is the action's documented behaviour, not an assumption - I tried to prove it on the CLI first and could not, because every control arm in this environment passed, so the local experiment was void and the documentation is what the claim rests on. INLINE COMMENTS, which is the part worth reading. The regression is confined to the dispatch route and always was. The action installs its inline-comment server only for an ENTITY event; `workflow_dispatch` is not one, but `issue_comment` is - and `claude.yml` was already in tag mode, since the action picks tag mode for an `@claude` comment with no `prompt` set. So the comment route can open genuine review threads, and now does. It needs no `track_progress`: that input only FORCES tag mode where a prompt would otherwise select agent mode. That changes the guidance rather than merely softening it. Unresolved review threads are what mechanically gate a merge here, so a blocking finding raised by comment actually blocks, where the dispatch route's can only ask a human to act. Ask by comment when you want findings that hold the merge; dispatch when you want `-f focus` or the packaged procedure. Both lists must now stay in step and nothing enforces it, which is recorded with the shape of the check that would. Neither route's changes can be exercised before merging: a comment always runs the default branch's copy of `claude.yml`, so it is unverifiable for the same practical reason the dispatch route is, and both are now on the post-merge list.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/astubbs/parallel-consumer/blob/d7ba7a5960c20c6c9afb9a0edc505c40454e5d81/.claude/worktrees/review-b2d8/.github/workflows/claude.yml#L178
When a maintainer invokes @claude review this on a fork PR, pr-context deliberately exits successfully after blanking only the gate outputs, so the Claude step still runs with CLAUDE_CODE_OAUTH_TOKEN and id-token: write. This newly copied allowlist permits ./mvnw and the listed bin/ scripts from the PR-controlled checkout—the surrounding rationale explicitly says this route runs PR-authored code—so a fork can replace one of those executables and expose credentials when the reviewer follows the request to run checks. The earlier fork-gate finding does not cover this fresh execution grant: refuse fork heads before this step, or run fork reviews without secrets and code-execution tools.
https://github.com/astubbs/parallel-consumer/blob/d7ba7a5960c20c6c9afb9a0edc505c40454e5d81/.claude/worktrees/review-b2d8/docs/inflight/ci-review-agent.md#L44
Although the earlier SETTLED bullet was removed, this newly added DECIDED block is another resolved-history entry in a tracker restricted to open gaps; worse, the retained block at lines 127–143 still says claude.yml grants nothing and warns against the exact grant documented here, leaving agents with opposite instructions. Delete both settled narratives, keep the durable behavior in docs/ci.md, and retain only genuinely open follow-ups such as grant drift or credential exposure.
AGENTS.md reference: docs/inflight/AGENTS.md:L32-L35
https://github.com/astubbs/parallel-consumer/blob/d7ba7a5960c20c6c9afb9a0edc505c40454e5d81/.claude/worktrees/review-b2d8/.github/workflows/claude.yml#L178
This new allowlist lets the comment route launch the unit suite, integration suite, and arbitrary Maven goals, but its claude job has no timeout-minutes; the dispatch counterpart sets a 30-minute bound expressly because these commands can wedge. If a requested Docker, broker, or Maven operation hangs, this workflow can retain a hosted runner until GitHub's multi-hour default limit, and repeated @claude comments can start more such runs. Add a comparable job-level timeout before enabling these execution tools.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… the job Three review findings on the commit before this one. All correct, and the first is the one that mattered. FORK HEADS GET NO EXECUTION TOOLS. Copying the allowlist across gave the comment route `./mvnw` and the `bin/` scripts - and that route answers on fork PRs, in a job holding CLAUDE_CODE_OAUTH_TOKEN. A fork can replace an allowlisted script and wait for the reviewer to be asked to run the checks. That is precisely the combination the dispatched reviewer refuses fork heads to avoid, and the fork guard added earlier did not cover it: it only blanked the gate outputs, because at the time there was nothing to execute. The dispatch route answers this by refusing the review outright. This route answers it by still replying - a fork author asking a question is legitimate, and the reply itself is harmless - but with no execution tools, so there is nothing for replaced code to be run by. `pr-context` now emits `fork_pr`, and the allowlist is switched off by it. A 30-MINUTE CAP, matching the dispatch reviewer's, and for its stated reason: the allowlist can start the unit and integration suites and arbitrary Maven goals, and a wedged Docker or broker call would otherwise hold a hosted runner until GitHub's multi-hour default. Comments can be repeated, so several such runs can be started in a row. THE TRACKER GOES BACK TO OPEN WORK. The `DECIDED:` block added last commit was itself settled history, which is the same rule that removed the `SETTLED:` one two commits ago - deleted, with the durable reasoning in docs/ci.md where it belongs. Worse, and the reason this was P1 rather than tidying: a pre-existing block still asserted that `claude.yml` grants nothing and argued against ever mirroring the grants into it, so the tracker contradicted the workflow and would have told the next agent to undo this. Deleted too. What survives is open: grant drift between the two allowlists, credential exposure, fork PRs having neither a green path nor a runnable reviewer, and the OSS Index grants - which now have two destinations rather than one, making them the first live instance of the drift risk recorded directly above them.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f11a851a5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…earing to
The fork carve-out added last commit did nothing. Review finding, and correct.
It was written as the usual GitHub Actions ternary:
${{ fork_pr == 'true' && '' || format('--allowedTools "{0}"', ...) }}
That idiom is `a && b || c`, and it only behaves like a conditional when `b`
is truthy. The empty string is falsy, so on a fork head the expression
evaluated `true && ''` to `''`, fell through the `||`, and handed back the
full allowlist. The fork branch of the guard produced exactly the value it was
written to suppress, while the step above it logged that execution tools had
been withheld - so the failure would have read as working.
Worth stating how it was caught: in review, statically. Not by a runner, and
not by me. Nothing here can be exercised before it merges - an `issue_comment`
always runs the default branch's copy of this file - so an expression that is
wrong in a way logging actively conceals had no downstream chance of being
noticed.
The conditional now lives in `pr-context`, which emits the finished
`claude_args` string: the allowlist for a same-repo PR or an issue, the empty
string for a fork head. Ordinary shell, no truthiness subtleties, and both arms
were simulated against the real allowlist before committing - the fork arm
yields a zero-length value and the same-repo arm the full 1294-character list.
The comment at the call site records the trap so the ternary is not
reintroduced as a tidy-up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9aa94c8411
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…s too, and fix the stale header Two review findings. UNTRUSTED COMMENTERS. The workflow trigger is "a comment containing @claude", which anyone who can comment can write, and the allowlist it now attaches can run `./mvnw` with arbitrary goals in a job holding CLAUDE_CODE_OAUTH_TOKEN. The fork check does not help when the branch is in this repo. `claude-code-action` does already refuse actors without write access (src/github/validation/permissions.ts, via getCollaboratorPermissionLevel), and `allowed_non_write_users` cannot loosen it here because that bypass is only consulted when the `github_token` input is supplied, which this workflow does not do. So this is defence in depth rather than the only guard - added anyway, because "the action checks it" is exactly the class of assumption that has been wrong twice on this branch already. It withholds the ALLOWLIST rather than refusing the run, matching the fork carve-out. If the set of trusted associations is ever wrong the failure is a trusted user getting a read-only reviewer, which is visible and complainable, rather than `@claude` silently doing nothing for them. The two carve-outs are independent and either alone withholds execution; the truth table was simulated across OWNER/MEMBER/COLLABORATOR/CONTRIBUTOR/NONE/empty against both fork values before committing. THE HEADER WAS STALE. It still said this route has fewer tools and pointed at a "THIS ROUTE IS A DEGRADED REVIEWER" section deleted two commits ago when the allowlist was copied across. It now describes the real distinction: same execution tools, different procedure, and inline review comments that only this route can post.
…r every commit was
Reverses the strict head-freshness rule, and deletes what it cost.
The gate now asks one question: has `claude[bot]` posted a finished review on
this PR? Any such review satisfies it, whenever it was posted. A review of the
first commit vouches for the twentieth.
That is the weaker guarantee and it is chosen deliberately. Strict is not
wrong - a review of commit N genuinely does not vouch for commit N+1 - but the
coverage it was protecting already arrives from elsewhere, because a separate
reviewer reads every push. What strictness actually bought here was a second
copy of a guarantee we already had, and what it cost was:
- a timestamp comparison that had to choose between the contributor's own
committer date and the server-side check-suite time, handle same-second
ties, and paginate an endpoint with undocumented ordering - three rounds of
review findings, none about reviewing;
- the reviewed-SHA plumbing crossing job boundaries so the head-moved refusal
could compare against what was actually checked out.
Both deleted here, along with the `checks: read` scope that existed only to
read when a head first appeared.
WHAT SURVIVES, AND WHY. The completion rule: a reviewer that left unticked
boxes has said itself that it stopped partway, and that is orthogonal to
freshness (#275). The identity rule: `claude[bot]` per
the API, never the comment body. Both are mutation-tested - six mutations, all
killed, including one that re-introduces a freshness comparison, so silently
restoring strictness now breaks the suite.
THE FORK HOLE HAD TO MOVE. Leniency made it reachable: the gate asks only
whether a finished claude[bot] comment exists, and the comment route answers
`@claude` on fork PRs - so a maintainer asking an ordinary question would have
greened a fork PR's required check on the next push. Previously this was
awkward to hit; it was never actually closed, because the gate has always
judged comments rather than provenance. The gate now refuses a fork head
outright, before reading any comment, which is where the guard belonged.
WHAT DID NOT GO, AND THIS IS A CORRECTION. Both `refresh-gate` jobs stay, and
`actions: write` with them. The expectation was that leniency retires them,
because a push creates a fresh check run that greens immediately. That is true
of a push and it is not the case they exist for: the gate fires on
`pull_request` only, so the terminal flow - push, ask for review, merge -
raises no event after the review lands, and the check keeps the red it recorded
before the review existed. Observed on #288, where a
review at 08:29 left the check red until a hand-run rerun at 08:37. The write
scope is a cost of the check being produced by a `pull_request` workflow, not a
cost of strictness, so leniency cannot pay it off. Retiring it needs the
reviewer to raise a check run on the reviewed SHA, which is now the highest
value change left in this area and is tracked.
The strict implementation is archived at
`archive/review-gate-strict-head-freshness`, with the trade, the assumption it
rests on, and the trigger to restore it, in
docs/inflight/parked-strict-review-gate-freshness.md. The assumption is worth
naming here too: per-commit coverage comes from the auto-reviewer. If that ever
stops, it stops coming from anywhere, and nothing will announce it - the gate
keeps passing, because a review does still exist on the PR.
astubbs
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc2878a1ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…promised fork reply Three review findings on the leniency commit. Two corrections, one declined. AGENTS.MD STILL DESCRIBED THE OLD CONTRACT. It said `claude-review` "stays red until a review exists for the current head" - the rule the previous commit removed. That is the drift that matters most: the repo-wide instruction file is what an agent reads first, so it would have sent the next one to reinstate strictness as a bug fix. Now describes the actual contract, and names the two review routes and when to reach for each, which it never did. THE ARCHIVE PATH DID NOT WORK FROM A CI CHECKOUT. The tag does exist and is pushed - `refs/tags/archive/review-gate-strict-head-freshness` resolves to annotated tag c1365d6 over commit 38ccf88 - but a shallow checkout fetches no tags, so `git show <tag>` finds nothing there and the archive reads as lost. The entry now gives the commit SHA alongside the tag name and shows the `git fetch --tags` first, so it is recoverable either way. A FORK AUTHOR NEVER GETS THE REPLY I PROMISED THEM. Both docs/ci.md and the workflow said the comment route still answers on a fork PR, just without execution tools. Only half true: `claude-code-action` runs its own collaborator check and exits before Claude starts, so anyone without write access gets nothing at all, on any PR. The withholding logic is unchanged and still right - it is what stops a maintainer's question running fork-controlled scripts beside the token - but "a fork author can ask a question" was wrong, and the useful case is a MAINTAINER asking on a fork PR. Corrected in both places. DECLINED: restoring the current-head requirement. That is the change the previous commit deliberately made, with the trade, the assumption it rests on and the trigger to restore it recorded in docs/inflight/parked-strict-review-gate-freshness.md. The finding's evidence was real - it cited the AGENTS.md rule, which did still say the old thing - and that is fixed above, which is the part that was actually wrong.
🧪🔒 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a30c616ab1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…urrent-head sweep Two things, and they turned out to be the same thing seen from both ends. A ONE-LINE-PER-FILE INDEX IN .github/workflows/README.md. There was no documentation in that directory at all - the index lived in docs/ci.md, which is not where you are looking when you are looking at the files. Fifteen workflows, one sentence each. It leads with the three `claude*` files, because their names genuinely do not separate them and `claude-code-review.yml` is the one file that does NOT review. One judges, two review, and which reviewer you want depends on whether you need findings that block a merge - only the comment route can raise an inline thread. That distinction was discoverable only by reading three workflow headers. Also carries the two conventions that bite: job names are matched by name in the ruleset, so renaming one silently stops it being satisfied rather than failing; and most of these run PR-authored code, which is why the review jobs hold no write scope. THE REST OF THE CURRENT-HEAD SWEEP. The previous two commits changed the gate's contract and updated the sections that argue for it, but three summaries still promised the old one: the workflow index in docs/ci.md, its "fresh comment" wording, and the dispatch workflow's header. A maintainer reading any of them would expect a push to invalidate a review. All three now match. That is the third time this PR has shipped a contract change with a stale summary left behind it - AGENTS.md last commit, these three this one - which is the argument for the index existing at all: fewer places restating the same contract in their own words.
Owner
Author
|
@claude review this Focus on the review system itself, since that is what the PR changes:
Note this run is itself the test: this PR ships the lenient gate, and a |
|
Claude finished @astubbs's task in 4m 42s —— View job Review: the review system itself
This run is itself confirmation the lenient gate works as designed: this comment is a finished FindingsP2 — stale "current head" wording contradicts the lenient contract, in the same paragraph
That's the old, removed, per-commit-freshness contract. The very next sentence in the same bullet correctly describes the current lenient behavior ("the gate can be satisfied by any finished comment the reviewer bot posts on the PR, including its answer to some other What I checked and found solidIdentity + completion rules (
Fork refusal ( Three independent checks, all consistent:
I traced the specific scenario the design doc calls out — a maintainer asking an unrelated Execution allowlist + carve-outs (
I did not find a way to reach the execution allowlist on a fork head or as an untrusted commenter. Minor/optional, not blocking: |
Found by the dispatched review this PR now makes possible - the first finding produced by the system this PR fixes, reviewing the PR that fixes it. The lead bullet of the tracker still said the gate "reds the PR until a review exists for the current head", two sentences before correctly describing the lenient behaviour. Same drift class as AGENTS.md, the docs/ci.md workflow overview and the dispatch workflow header, and it was not among the three Codex named, so it survived that sweep. A repo-wide grep now returns only descriptions of the REMOVED rule - "it does NOT prove the review covered the current head", "there used to be", "used to require" - plus the one narrative about #288's head. No live claim left. Also states the consequence the old wording hid: because the gate no longer asks about the head, a single satisfying comment counts for the life of the PR rather than until the next push.
…t it was stated nine times Four review rounds found four stale restatements of the same contract, one at a time, and each was fixed as its own defect. The duplication was the defect. The contract - what satisfies `claude-review` - was written into nine files in nine different sentences. Changing it updated the places that ARGUE for it and missed the places that merely restate it in passing, which are the dangerous ones: they are what a reader skims, and in AGENTS.md's case what an agent reads first and would have "corrected" the code to match. Collapsed to one canonical paragraph in docs/ci.md, marked as canonical, with AGENTS.md, .github/workflows/README.md, the dispatch header, the workflow index entry and the in-flight tracker reduced to pointers. What remains elsewhere are six mentions that REASON about the rule in local context - the parked doc contrasting old and new, two comments explaining why the head-moved guard went, a test rationale - none of them an authoritative statement anyone would update. WHY THE DUPLICATION SCANNERS WERE GREEN THROUGHOUT, which is the part worth keeping. They cannot see this, for two independent reasons, and neither is bad luck. Scope: `dups: clones` and `dups: similarity` scan `parallel-consumer-*/src`, and the similarity job also filters `file_extensions: 'java'` - docs, .github, bin and AGENTS.md are scanned by neither, so they never looked at the files involved. Capability: even pointed at docs, a token-based clone engine finds repeated token SEQUENCES, and nine paraphrases of one rule share almost none; whole-file similarity puts an AGENTS.md and a workflow YAML far below its ignore floor. The scanners catch copy-paste. What bit here was restatement. That matters because the scanners exist precisely because agents struggle to stay DRY - so the defence has a hole exactly where that failure concentrates, agents duplicating prose and config as readily as code. The reviewers reported symptoms for a related reason: each round's diff held one or two stale sentences, so each produced findings about sentences. Counting instances repo-wide means stepping outside the diff. The fourth review named the class explicitly and still prescribed the sentence fix, which is the near-miss worth remembering. Written up in docs/solutions/documentation-gaps/, with both follow-ups recorded in the tracker and marked as non-substitutes: a clone engine over docs/.github catches verbatim copy-paste and not paraphrase, while a contract wants a narrow guard asserting its canonical phrasing lives in one file.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8367f73e0f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… the write-up where it belongs Two things, neither about the review system this PR otherwise changes. QUARANTINE ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirect. Met while babysitting this PR - which is the point, flakes get quarantined as they are met rather than waiting for the next sweep. It would not have been found by a sweep anyway: the 2026-08-07 scan read surefire `Flakes:` markers, which only exist when a retry re-ran a test and it then passed, and this one failed its run outright. Rule 1 is satisfied rather than excepted - the mechanism is known and the fix is already written. The defect is in the shared BlockedThreadAsserter#assertUnblocksAfter helper, not in this test: the unblocker is scheduled BEFORE the elapsed clock starts, so the measured window begins after the delay it is compared against and is short by however long arming the scheduler takes. Under load that exceeds a millisecond and `isAtLeast` fails a correct implementation. Seen as `expected to be at least PT20S but was PT19.998S` - two milliseconds on a twenty-second bound, which is the shape of a measurement error, not a behavioural one. Rule 2 is settled without needing a rate: it failed on a PR whose diff contained no Java at all, so nothing in the change could have caused it. Owner #262, which stamps the arming instant and measures from there; `flapping = true` because it passes most runs. Filed against the helper in the tracker, since any test using it can show the same signature - and noting that #265 deletes the same assertion instead, so whichever lands second has a real decision to make rather than a mechanical merge. Verified with both arms rather than assumed: under the gating exclusions the method reports `Tests run: 0`, with `-Dexcluded.groups=` it runs and passes in 21.04s, and the three sibling tests in the class still run in the gating lane - so the exclusion is method-granular and the lane still watches it. Registry and owner checks pass. FILE THE DUPLICATION WRITE-UP UNDER workflow-issues. It was in documentation-gaps, contradicting its own frontmatter, which already said `root_cause: inadequate_tooling_scope` and `resolution_type: workflow_improvement`. Nothing was missing from the docs - the opposite, one contract written nine times - and the causes are scanner scope and review granularity. It sits next to negative-results-need-an-instrument-that-could-have-said-yes.md, which it already cross-references as the same shape.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd98cbc919
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review finding, and the fifth instance of the drift class written up two commits ago - this time inside the correction itself, which is worth saying out loud rather than quietly fixing. When the strict rule went, four places recorded WHY, and each named the costs in its own words. Three of them charged freshness for the `actions: write` scope on the `refresh-gate` jobs. That was true of the first draft of the plan and false by the time it landed: the jobs survived, because the gate is produced by a `pull_request` workflow and a review landing after the last push raises no event to re-evaluate it - which is true under EITHER rule. The scope is a cost of how the check is produced, not of what it asks. `docs/ci.md` contradicted itself sixty lines apart, saying freshness cost the write token in one section and that the jobs survive it in another. The parked entry did the same thing in one page. `testing-evidence.yaml` and the script header carried the same claim. This is not pedantry about attribution. The parked entry exists so a future reader can weigh restoring strictness, and the inflated bill makes that trade look worse than it is: restoring it costs the timestamp machinery, not a new privilege-escalation surface. A reader talked out of it by a wrong price is exactly the failure that entry was written to prevent. Corrected in all four, with the freshness bill now naming only what it really bought - the contested timestamp comparison and the reviewed-SHA plumbing - and each place saying plainly that the write scope survives and why.
…ts seed expires UNRELATED TO THIS BRANCH'S SUBJECT, and recorded here anyway because the evidence is perishable and this branch's CI is where it surfaced. The entry itself says the replay seed is the part no command can recover once the log expires, and a chaos run randomises its seed, so waiting for a tidier home loses the datapoint. No product or workflow code changes; one ledger file. ChaosRevokeUnderWorkCooperativeIT.revokeUnderWorkStaysProtocolHonestWithCooperativeAssignor went red on job 93954713987, seed 3986919097693415295. Recorded with its uncertainty intact rather than filed as a stall. The 2026-08-11 sighting was a clean ProgressProbe kill - named partitions, lag numbers, 154s against a 150s bound. This one carries no probe verdict that could be extracted at all, only a shutdown-path symptom: the execution pool missing its PT10S termination await. That is compatible with work wedged behind the open deadlock and equally compatible with a chaos STOP_NO_DRAIN racing a slow shutdown, and nothing in the log separates them. The entry says which evidence would - a probe verdict in the replay, read from the surefire XML rather than the console tail that carried none. It matters because it is the cooperative sibling of the already-logged eager variant, one assignor apart. The previous sighting notes the cooperative variant passing in that same run and reasons from it to "eager-protocol- specific". A cooperative red is the first datapoint pointing the other way, and deliberately NOT treated as retiring that inference, because it is unconfirmed. Written so the replay decides it either way. Also warns off a re-diagnosis: the log is full of RebalanceInProgressException, and those lines are the LANDED #100 fix catching and deferring, not the old unhandled path. Someone grepping for that exception later would otherwise re-open something already closed. Nothing about the three-defect split, #29's status, or the structure of the entry is changed - this is a sighting appended to the sighting run.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47dfde8b7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| |---|---| | ||
| | `cancel-closed-pr-runs.yml` | Cancels in-flight runs for a PR once it closes, so a dead PR stops burning runners. | | ||
| | `chaos-pain.yml` | On-demand seeded chaos hunts for the stall bugs, `workflow_dispatch` with `seed`/`reps`. Self-hosted high-CPU lane. | | ||
| | `check-dependencies.yml` | Blocks a stacked PR until the PR it says it `depends on #N` has merged. | |
There was a problem hiding this comment.
When a maintainer follows this new workflow index, depends on #N produces a PR body that the issue-reference gate rejects for numbers below 1000. Document the accepted repository form, depends on astubbs/parallel-consumer#N, so the quick-reference guide does not direct users toward a failing check.
AGENTS.md reference: AGENTS.md:L383-L387
Useful? React with 👍 / 👎.
| `grep -rn` the claim across `docs/`, `.github/`, `bin/` and the instruction files. If it is stated | ||
| in more than one place, the duplication is the defect and the stale sentence is a symptom. | ||
|
|
||
| In `#287` this collapsed nine statements to one canonical paragraph in `docs/ci.md`, marked as |
There was a problem hiding this comment.
This second mention drops the repository qualification used earlier in the same document, making #287 ambiguous between the fork and upstream repositories. Keep it as astubbs#287 so the durable solution record follows the repository's issue-reference convention.
AGENTS.md reference: AGENTS.md:L284-L292
Useful? React with 👍 / 👎.
| @@ -0,0 +1,47 @@ | |||
| # Workflows - one line each | |||
There was a problem hiding this comment.
The commit subject is ci(review): make the reviewer run..., so it omits the required front-positioned issue reference even though the body identifies related repository work. Add the applicable #NNN after the scope before this becomes the squash title; otherwise the permanent history and its generated release-note input lose the subject-level link the repository format is designed to preserve.
AGENTS.md reference: AGENTS.md:L304-L310
Useful? React with 👍 / 👎.
Comment on lines
+290
to
+291
| <!-- CANONICAL: the gate contract. Nowhere else states what satisfies the gate - everything else | ||
| links here. If you change this paragraph, run bin/check-review-gate-contract.sh. --> |
There was a problem hiding this comment.
When a maintainer changes this canonical paragraph and follows the new instruction, bin/check-review-gate-contract.sh fails because that script does not exist. A repo-wide filename and text search, plus the paths recorded across all local commits, finds no implementation outside this comment; add and wire the promised guard, or remove the command rather than documenting an unavailable check.
Useful? React with 👍 / 👎.
astubbs
added a commit
that referenced
this pull request
Aug 11, 2026
Master landed the rebuilt review system (#287), which moved the reviewer out of the file this branch was editing. All three conflicts are on that seam, and none of them is a mechanical pick. .github/workflows/claude-code-review.yml - took master's side wholesale. Master rewrote it into a gate that invokes no Claude, so the `claude-code-action` step this branch was extending no longer exists in it. THE GRANTS WERE CARRIED, NOT KEPT BY LUCK: bin/check-ossindex-audit.sh, bin/test-check-ossindex-audit.sh, bin/check-cve-exclusions.sh and bin/test-check-cve-exclusions.sh - bare and ./ forms, eight entries - are re-added to BOTH claude-code-review-dispatch.yml and claude.yml, because master's own comment says the two allowlists must stay byte-identical apart from the inline-comment tool and nothing yet enforces it. A grant on one route only is a reviewer that silently cannot run the check it was asked about. All four scripts exist on this branch, so none of the grants is inert. This is the drift docs/inflight/ci-review-agent.md predicted would first bite right here. It also repairs an earlier instance of the same loss: this branch's previous merge (0cfe05b) dropped #273's check-docs-data grants from the allowlist without anyone noticing. Deleting this branch's copy of the list and adding to master's puts them back. docs/ci.md - kept both sides, minus one. This branch's cve-exclusions bullet stays under repo-hygiene; master's "The three claude* workflows" section replaces the old claude-code-review.yml bullet entirely. That bullet restated the review contract, and master marked a single paragraph canonical - so it is dropped rather than merged, and the surviving text points at the canonical statement instead of repeating it. docs/inflight/ci-review-agent.md - took master's rewritten bullets; all four of this branch's were superseded (credential exposure, the integration-test cap and the workflow-validation trap are all restated better there, and "claude.yml grants nothing" is simply no longer true). Master's "the OSS Index audit grants must be carried across at merge" entry is the instruction this commit just carried out, so it is retired and rewritten to keep only what is still open: that no reviewer has yet run any of the four scripts, making the grants asserted capability rather than demonstrated. .github/workflows/README.md is master's new per-workflow index and knew nothing of this branch, so dependency-audit.yml is added to it and repo-hygiene.yml's line now mentions the CVE exclusion expiry job. Verified: both self-tests pass, check-cve-exclusions reports the temporary jackson entries inside their window, actionlint clean on every touched workflow, check-action-versions / check-shell-sigpipe / check-issue-refs and both node gate self-tests green, mvn validate green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 12, 2026
…s allowlist The entry claimed the reviewer on run 31547154463 was refused four granted scripts by some layer beyond `--allowedTools`, and that the workflow allowlist was ruled out because the reviewer had checked it. Both halves were wrong, and we wrote them. That run was an `issue_comment`, which GitHub runs from the default branch, so the list in force was master's `REVIEW_TOOL_ALLOWLIST` - which has none of the four grants in it. The action prints the resolved list twice, in the step's `claude_args` and again in the SDK's `allowedTools` array, and it is absent from both. The reviewer's self-check cited `claude.yml:60` and `claude-code-review-dispatch.yml:394`, which are the same line numbers in master's copy and the PR's, and asserted the grants were present - so it had read the PR's copy over `gh pr diff` rather than the file underneath it, and mistook "granted in the diff" for "granted in the running copy". The generalisation is worth more than the incident: a grant a PR adds can never take effect on that PR, on any route. `docs/ci.md` already says so under "Editing the reviewer", so this entry points there rather than restating it - the canonical-statement rule from #287. What survives is the one genuinely open thing, that the grants stay unexercised until they merge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
astubbs
added a commit
that referenced
this pull request
Aug 12, 2026
…s done This branch's glob grants falsified bin/AGENTS.md, which still told agents the allowlist "is not a glob" and that a new script stays invisible until someone adds it by hand - binding guidance that would have had the next contributor undo the convention as an oversight. Shrinks bin/AGENTS.md rather than correcting it in place. That file is read by every agent touching the directory, so it should carry only what someone can get wrong by an action taken THERE: naming a script `check-*` or `test-check-*` grants it to the reviewer automatically, so a script that writes, publishes or deploys must not be named that way. Everything else about the allowlist now points at docs/ci.md. Net effect on the file is shorter than master, not longer. The rest moves to docs/ci.md -> "Editing the reviewer", canonical for the review system since #287, whose audience is someone editing the reviewer - rare, deliberate, and already in that file: - the boundary the allowlist actually is. The fork refusal settles whose code runs beside the credential and the review/refresh-gate split settles what the token can reach; neither touches what that code can talk the reviewer into RUNNING, which is the one the allowlist holds. - the naming convention as the new form of that boundary, and why the pattern stops at two prefixes. - what still needs a manual grant: both spellings, both files, and an earlier PR than the script. Drops the bin/test-check-review-posted.sh incident narrative. It was the evidence for a rule that no longer needs arguing - the naming convention now enforces it - and a file every bin/ agent reads should not pay context rent for a settled argument. It survives in git history. Swept the repo for the same staleness by the claim rather than the wording: docs/inflight/ci-review-agent.md, .github/workflows/README.md and both workflows' comments were already correct, and docs/ci.md's pattern bullet already was too. bin/AGENTS.md was the only hit. Also grepped every bold label removed from it - none was cited anywhere; "Scripts that guard other scripts", which is cited twice, is untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STG7ygSMtmFuu36BiqdA1m
astubbs
added a commit
that referenced
this pull request
Aug 12, 2026
…an (#279) Two changes wearing one coat, because the reviewer was being rebuilt underneath this branch while it was open. Read it as: a CVE lane, and the grants that lane needed. THE LANE `deps: whole-tree CVE scan` runs the OSS Index audit in one job. It is distinct from `deps: vulnerabilities`, which only inspects dependencies a PR changes: this one scans every module, on PRs and weekly, so an unchanged tree acquiring a new advisory is noticed. The novelty is that it can prove it ran. The plugin reports BUILD SUCCESS against an unreachable scanner - `fail` covers "vulnerable components found", not "the request failed" - so a job that trusts the exit code learns nothing. The guard has three distinguishable exits: 1 = could not prove the scan happened, 2 = it ran and found something, 3 = a temporary exclusion expired. Exit 1 wins when 1 and 2 both apply: findings recovered from an unprovable scan are evidence of nothing. Gating on the raw report would have shipped an escape hatch that silently did nothing. The plugin honours `excludeVulnerabilityIds` when deciding which components are "vulnerable", but the exported per-component `vulnerabilities` array is UNFILTERED, so an already-excluded id came straight back as a live finding and no pom edit could ever have cleared it. The guard filters per advisory id, and prints the suppression count on every run so the list stays reviewable. THE BACKLOG IT FOUND, AND WHAT KEEPS IT HONEST The first authenticated run failed on five components. Two were fixed in #281; three have no available remedy and are excluded by CVE id with a reason and a retirement condition. Two more arrived later against `jackson-databind:2.18.9` - fixed upstream in merged PRs milestoned 2.18.10, which is not released, so there is nothing to bump to on any line. Those two are marked `TEMPORARY-SINCE:` and expire. `bin/check-cve-exclusions.sh` fails when a temporary entry outlives 90 days - chosen from jackson's own release cadence, not ours, since our tags are years apart and irrelevant to when a fix ships. Undated, unparseable and future-dated entries fail rather than reading as not-yet-expired, and an id with no rationale comment fails too: unclassified would otherwise read as standing-forever. The Dependabot ignore for `jackson-bom` is narrowed to minor and major so the 2.18.10 patch that retires the exclusion can actually be proposed - a blanket ignore had been suppressing the very bump we were waiting for. THE GRANTS Reviewer allowlist entries for the new guards, which this branch needed and which moved file three times while #284 and #287 rebuilt the reviewer. They are enumerated no longer: `bin/check-*.sh` and `bin/test-check-*.sh` are granted by pattern, in both spellings, on both routes. The reason is structural. A grant a PR adds never applies to that PR - the dispatch reviewer runs the default branch's copy of the workflow by design, and an `issue_comment` run is served the default branch's copy by GitHub - so every new guard script needed its grant landed in a prior PR, and these four did not get one. A pattern makes the grant arrive before the script. That widening adds no capability a PR does not already have: on the dispatch route the checkout is `refs/pull/<n>/head`, so every already-granted script is the PR's own copy, and `Bash(./mvnw:*)` already runs PR-controlled Maven. It is a fifth door into a room with four open, and the exposure that matters - PR code executing beside CLAUDE_CODE_OAUTH_TOKEN - predates this and is tracked separately. What is new is that NAMING is now the grant: a script called `bin/check-*.sh` is granted automatically, so one that writes, deploys or publishes must not be named that way. Recorded where someone adding a script will read it. `Bash(git rev-parse:*)` is granted so the reviewer can establish which tree it is standing in rather than infer it, after one review reported a `rev-parse` result for a command it had no grant to run. ALSO - Published as evidence in `docs/data/testing-evidence.yaml`, holding that file's line: a green run means the audit ran, not that the tree is clean, and the exclusion list is a maintained artifact rather than proof of absence. - Corrected a claim this branch briefly published, that the `@claude` comment route runs the default branch's code. It does not - `claude-code-action`'s `setupBranch` fetches the PR branch and switches to it, including for forks. The belief came from finding zero `refs/pull` in a log, against an action that fetches by branch name: a negative result from a search that could not have produced a positive. - The scheduled `Automatic Dependency Submission` workflow is recorded as unable to work here rather than merely disabled: it probes with a bare `mvn validate`, which cannot resolve a reactor's inter-module jars, and no amount of publishing fixes that. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
4 tasks done
astubbs
added a commit
that referenced
this pull request
Aug 13, 2026
The gate has only ever asked whether a machine reviewed the PR. The half that actually matters to the person merging - "have I read this one myself yet?" - was carried in his head, across a dozen open PRs, and it was getting dropped: "I always need to do a review myself and add a LGTM ... sometimes I forget or get confused. It would be much easier if there was just an action that highlighted that." So the gate now asserts two things, and names them separately when they fail: an automated review exists and finished (bin/check-review-posted.sh, unchanged), and the repository owner has submitted a review saying LGTM (bin/check-human-lgtm.sh, new). Both halves run on every gate run even when the other has already failed, so one look at the checks list says which is outstanding rather than only the first one to go red. WHAT COUNTS, AND WHY IT WAS DECIDED BY LOOKING The matching rule was settled against every LGTM this repo has actually received rather than against taste, because a memory aid that rejects the habit it is meant to remind you of is a nuisance people route around. All 18 of them, from #210 to #292, are the lower-case bare word `lgtm`, submitted as a COMMENTED review - not an approval, and never in the ordinary comment box. So: a submitted pull-request review, by astubbs, whose body contains `lgtm` as a whole word in any case, outside code fences and blockquotes, not followed by `?` and not preceded by a negation. Review state is not consulted, except that a PENDING review is ignored. All sixteen historical LGTM PRs pass the new checker against live API data; #287, #296 and #297 - reviewed, or not, but never stamped - fail it. Two of those clauses are load-bearing rather than fussy. An APPROVED review with an empty body does NOT count, because a silent Approve is the reflex this exists to interrupt. A DISMISSED review DOES, because the master ruleset dismisses approvals on push: had the rule consulted state it would have been head-sensitive through the back door, going green and then quietly red again after an unrelated typo fix. HEAD-INSENSITIVE, DELIBERATELY An LGTM on any commit of the PR counts for the whole PR, permanently. That matches the automated half, which abandoned freshness for reasons recorded in docs/inflight/parked-strict-review-gate-freshness.md, and it matches what was asked for: "it doesn't need to be on the latest head because I won't say LGTM until I'm confident that we're near the merge point anyway". A stamp that evaporates on the next push trains you to stop typing it. Two self-test cases assert this as a positive rather than as deleted code, so re-adding freshness breaks the suite. RENAME, AND WHY THERE ARE TEMPORARILY TWO JOBS `claude-review` no longer describes a check that also wants a human, so the job is now named `review: bot + human LGTM` - repo convention for a required check (`shell: sigpipe`, `deps: vulnerabilities`), with the two operands naming the two halves so a red entry in the checks list says which question is open before you open anything. The job name is a required status check matched by name, so the rename cannot be done in one step: drop the old name and the ruleset requires a context nothing produces, which blocks every open PR including this one. A `needs:`-based mirror was rejected outright - a job that skips when its dependency fails reports `skipped`, which GitHub counts as satisfying a required check, so the mirror would have gone green exactly when the gate went red. Instead a transitional job keeps reporting `claude-review` under the old contract, with duplicated steps and a stated deletion condition. The ruleset is swapped in one atomic PUT while both names report, and only then is the old job deleted; the commands are in the PR description. The gate stays read-only (`contents: read`, `pull-requests: read` - the reviews endpoint needs no new scope), bot-raised PRs stay exempt, and a fork PR still cannot satisfy it. A `pull_request_review` trigger, which would clear the human half the moment the LGTM lands, is deliberately NOT added and the reasoning is written into the workflow: on that event the sender is the reviewer, so a bot-submitted review would skip the job on its existing sender guard, and a skipped job satisfies a required check. That is this repo's most-repeated defect class pointed the wrong way. The LGTM therefore needs the same re-run the automated half already documents, unless it is left before the review is asked for. The self-test carries 42 cases, over half of them negative controls, including four that read the workflow file and assert it still feeds this checker the reviews endpoint with the marker the checker parses - a gate fed the wrong stream would report success without having asked the question. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BhF637Ywr7MiKkxaR11Up3
astubbs
added a commit
that referenced
this pull request
Aug 17, 2026
Master moved a long way under this branch, and most of what it moved was this branch's own subject matter. Resolving the merge honestly leaves a one-file deletion, so the resolutions are recorded here rather than left to be inferred from a 37-conflict merge. Superseded on master, taken from master wholesale ------------------------------------------------------------------------------- - `.github/workflows/claude.yml` - #287 landed a superset of this branch's grant: the same curated allowlist as the dispatch route (REVIEW_TOOL_ALLOWLIST), a trusted-author gate, a fork carve-out, a 30-minute cap and `refresh-gate`. It also SETTLED this branch's stated open question - the action checks the PR branch out over master's checkout, so the granted scripts run against the PR's tree. Master's fork handling differs by design: it withholds the allowlist and still answers, where this branch refused the run outright. - `docs/quarantined-tests.md`, `bin/check-quarantine-owners.sh`, `bin/quarantine-lane-report.sh` - the qualified `Owner: PR astubbs#NN` marker this branch introduced is already on master via #288, parsers and registry prose included. - `docs/inflight/test-untracked-ci-flakes.md` - master's copy is a strict superset: it carries this branch's `PCMetricsTest` diagnosis and its "quarantined and removed again, the diagnosis was wrong" note about the backpressure test, plus a `ProducerManagerTest` entry, the rule-1 exception and a citation repair this branch predates. - `docs/inflight/ci-review-agent.md` - rewritten by #287 around the two-route model. Two of this branch's entries are now stale rather than merely older: `actionlint` IS granted, and "which tree does it run against" is answered. The quarantine this branch added is dropped, not re-resolved ------------------------------------------------------------------------------- `PCMetricsTest.metricsRegisterBinding` was quarantined here naming #265 as `fixedBy`. #265 has since MERGED, and it fixed this test causally: the `Thread.sleep(1000)` between the counter snapshot and the gauge read - which is the exact mechanism the quarantine reason describes - is now an `await().untilAsserted(...)` requiring the trailing meters to agree with the counters before the snapshot is asserted. #265 deleted the annotation and the registry entry in the same commit, per rule 3. Re-applying the entry would therefore be a hard failure, not a judgement call: `bin/check-quarantine-owners.sh` errors on a merged owner whose test is still quarantined ("re-enable overdue"). And a *fresh* sighting would need a fresh diagnosis under rule 1 - the old reason no longer describes code that exists. No such sighting: master's gating CI has been green on every run since #265 merged on 2026-08-13. If it flakes again, re-quarantining is the inverse of that commit, as #265's own message says - a new entry with a new diagnosis, not this one restored. Rename/rename, and the mis-pairing AGENTS.md warns about ------------------------------------------------------------------------------- Both sides had run `bin/rename-packages.sh` independently, so the tree conflicts were rename/rename and resolved to master throughout - this branch's Java delta was the quarantine annotation and nothing else. The predicted cross-module mis-pairing did surface: git paired the STREAMS module's `TestConventionsArchTest.java` against the METRICS module's. Asserted after resolution - all six arch tests present, one per module, and no `io/confluent` path left in the index. Verified: `bin/check-quarantine-registry.sh` (2 entries, consistent) and `bin/check-copyright-headers.sh` (241 files, 0 violations) both clean.
This was referenced Aug 17, 2026
Merged
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…hat its gap is accepted (#319) Automatic Codex review on every push was switched off on 2026-08-19, in the Codex account settings rather than anywhere in this repo. It now reviews on request only, via an `@codex review` comment. The reason is cost: a full review of every push spends more than the coverage is worth at current token prices. That is precisely the trigger `docs/inflight/parked-strict-review-gate-freshness.md` named for restoring the strict, head-fresh `claude-review` gate - so left alone, the entry would have read as an unnoticed regression. Five places asserted the same now-expired premise, each in its own words: the canonical gate contract in docs/ci.md, the gate's own header in bin/check-review-posted.sh, the "no longer asks" note in claude-code-review.yml, the review-delivery posture in docs/data/testing-evidence.yaml, and the parked entry itself. THE DECISION RECORDED IS NOT TO RESTORE STRICTNESS. Restoring it would make every push want a fresh Claude review, moving the per-push spend onto the more expensive reviewer - the exact cost that parking freshness and taking the reviewer off `pull_request` were both meant to remove. So the gap is stated plainly rather than covered: a PR reviewed at commit 1 can merge at commit 20 with commits 2-20 read by nobody, and every check stays green. What reopens the decision is the price of a review falling by roughly two orders of magnitude, not the rediscovery that the gap exists - written down so the next reader does not mistake it for an oversight and "fix" it. Contributor impact: no change to the library. Pushes are no longer reviewed automatically; ask for a review when the PR is ready, and again after a push that changes what the reviewer already looked at. Two stale claims fixed in passing, both inside comments this change rewrites: docs/ci.md cited `bin/check-review-gate-contract.sh` as the check to run when editing the canonical paragraph, and that script has never existed - the citation was born dangling in #287; and claude-code-review.yml still charged strictness for the `actions: write` scope, which docs/ci.md and the parked entry had both already corrected.
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…hat mattered docs/solutions/workflow-issues/duplication-scanners-do-not-look-where-agents-duplicate-2026-08-12.md tracked two follow-ups. (a) - point a clone engine at docs/, .github/ and bin/ - landed in this PR: both jobs scan `.` instead of a whitelist of Java module directories. Recorded so the tick is not mistaken for the problem being solved. Widening fixes SCOPE. The failure the write-up describes is CAPABILITY: nine sentences stating one contract, sharing almost no token runs, which no clone engine can see at any scope. This PR met the same wall from the other side - a real 22-line clone the engines are structurally unable to compare, because it was JavaScript embedded in a YAML string on one side and a shell heredoc on the other. So (b) is still open and is the one that would have caught #287: assert a contract's canonical phrasing appears in its one home and nowhere else. It tests the thing you care about rather than a proxy for it. Found because the prior art was read late. AGENTS.md lists `grep -rl <mechanism> docs/solutions/` as the second prior-art check, before forming a hypothesis; the scope problem had been diagnosed a week earlier, in a document that names both jobs and prescribes this exact change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
AGENTS.md has always asked authors to cite a path plus a greppable anchor and to "run the grep before you commit the citation". Nothing ran it, and docs/citations.md said so outright: "nothing in CI checks any of this, so the only thing standing between a reader and a confidently wrong pointer is the author having run it." The bill was docs/ci.md telling readers to run bin/check-review-gate-contract.sh when editing the canonical review-gate paragraph. That script has never existed - the citation was born dangling in #287 and survived every review since, because a path that looks right reads as right. That is why review does not catch this class and a check has to. WHOLE TREE, RATCHETED AGAINST THE BASE. Deleting a file changes no line in the documents that cite it, so a diff-scoped gate is blind to the commonest way a citation breaks - not a corner case, since two moves produced 59 of this repo's 87 dangling references. An earlier draft covered that with a second rule sweeping for citations of removed paths; scanning the tree subsumes it, because those citations simply stop resolving. Ratcheting is what makes whole-tree affordable. It fails only on findings the base did not already have, because master gains dangling references from ordinary work - 90 in a single day, about fifty of them documents describing modules and plans that live on feature branches. That is how this repo writes work down, and a gate red on every PR for inherited breakage would be switched off within a week. Deleting a cited file still fails the PR that deletes it: its citations resolved in the base and stop resolving here. Finding identity is document plus cited path, deliberately not the line number - the same reasoning that forbids citing a `file:line`. NARROW ON PURPOSE, because the failure mode is flagging prose, not missing a path. A token is a citation only if it carries a file extension, and it resolves three ways before being called dangling: from the repo root, relative to the citing document, or as the tail of a real path - the shorthand the anchor rule itself encourages. Globs, placeholders, URLs, absolute host paths and `<rev>:<path>` history pointers are not citations. A document carrying the history pointer docs/citations.md prescribes has its citations of that path satisfied, because a gate that fires on the documented repair teaches people to stop applying it. A paragraph may close with `<!-- file-refs: N/A - reason -->`, covering the paragraph above it and never below: an escape covering text the author never read is no narrower than having no gate. Every narrowing rule is pinned by a negative test case, 40 in all. Five defects were found by running it against this repo rather than against fixtures: URLs and absolute paths leaking their tail as a token; markdown links reporting each broken link twice; a bare `<!-- file-refs: N/A -->` passing because the comment terminator satisfied a "non-empty reason" rule; a per-line marker that did not survive prose, where the citation's sentence wraps; and a local oracle using fs.existsSync, which resolved git-IGNORED paths so a citation under target/ passed pre-commit and failed CI. The local mirror calls the same module rather than reimplementing the rule, and its base-tree reader is shared with the workflow - the byte-exact `cat-file --batch` slicing is not something to hold two copies of, and its failure mode is silent truncation. That framing is measured against real git, not assumed, with the command to re-measure recorded in the test. Costs 65ms, which is why the pre-commit hook registration is affordable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…te-ups they earned
Records only - no mechanism here. Each of these was met while doing the work in
the preceding commits, and each is something the next person would otherwise
rediscover.
SIGHTINGS
* simpleBatchTest failed in Mutiny after failing in Reactor on
#308 - same alias, same 30-second timeout, same
shared BatchTestMethods lambda in core, BOTH on branches whose diffs contain
no Java at all. Two sightings in different modules say the wrapper is not
the variable. The assertion is off by one in the informative direction,
`Expected size: 3 but was: 4`: the records arrived, in one batch too many,
so it is a batch-BOUNDARY question rather than a lost-work one. Recorded
with the two readings that need separating - a loaded runner spreading work
out so the batcher closes early, or the batcher splitting under a timing the
library should tolerate - because assuming either is how a real defect gets
written off as a flake. Not quarantined: quarantine is master-state and
needs a diagnosis, and there is not one yet.
* The clones the widened scan exposed. Pre-existing - base and head both
report 27 CPD / 70 jscpd - and never triaged, because nothing outside four
directories was ever scanned. Deliberately not listed here: that is a
command's answer. What is recorded is that nobody has read them, and the
trap that the example modules are near-copies ON PURPOSE, so triage means
deciding per clone and saying which.
* A dispatched review ran 5m47s, concluded SUCCESS and posted nothing, while
claude-review stayed green on an older comment. Already-known, but this
sighting narrows it: two comment-route reviews on the SAME PR that morning
posted normally, so it is the route, not the diff or the reviewer.
* The scanner follow-up marked landed, with the warning that it is not the one
that mattered. Widening fixes SCOPE; the failure the write-up describes is
CAPABILITY, and follow-up (b) - assert a contract's canonical phrasing
appears in one file and nowhere else - is still open and is what would have
caught #287.
WRITE-UPS
* What a clone detector cannot see once scope is fixed: code embedded in
another language's file. CPD parses per language, jscpd keys format off the
extension, so JavaScript in a YAML string is never compared with JavaScript
in a shell heredoc - and no directory list changes that. It also records the
ratio trap: widening found MORE clones at a LOWER percentage, which killed
an absolute threshold silently.
* A forward-pointer on the 2026-08-12 write-up, whose findings are
deliberately NOT rewritten - a dated record says what was known that day, and
a "See also" is a cross-reference rather than a changed claim.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
AGENTS.md has always asked authors to cite a path plus a greppable anchor and to "run the grep before you commit the citation". Nothing ran it, and docs/citations.md said so outright: "nothing in CI checks any of this, so the only thing standing between a reader and a confidently wrong pointer is the author having run it." The bill was docs/ci.md telling readers to run bin/check-review-gate-contract.sh when editing the canonical review-gate paragraph. That script has never existed - the citation was born dangling in #287 and survived every review since, because a path that looks right reads as right. That is why review does not catch this class and a check has to. WHOLE TREE, RATCHETED AGAINST THE BASE. Deleting a file changes no line in the documents that cite it, so a diff-scoped gate is blind to the commonest way a citation breaks - not a corner case, since two moves produced 59 of this repo's 87 dangling references. An earlier draft covered that with a second rule sweeping for citations of removed paths; scanning the tree subsumes it, because those citations simply stop resolving. Ratcheting is what makes whole-tree affordable. It fails only on findings the base did not already have, because master gains dangling references from ordinary work - 90 in a single day, about fifty of them documents describing modules and plans that live on feature branches. That is how this repo writes work down, and a gate red on every PR for inherited breakage would be switched off within a week. Deleting a cited file still fails the PR that deletes it: its citations resolved in the base and stop resolving here. Finding identity is document plus cited path, deliberately not the line number - the same reasoning that forbids citing a `file:line`. NARROW ON PURPOSE, because the failure mode is flagging prose, not missing a path. A token is a citation only if it carries a file extension, and it resolves three ways before being called dangling: from the repo root, relative to the citing document, or as the tail of a real path - the shorthand the anchor rule itself encourages. Globs, placeholders, URLs, absolute host paths and `<rev>:<path>` history pointers are not citations. A document carrying the history pointer docs/citations.md prescribes has its citations of that path satisfied, because a gate that fires on the documented repair teaches people to stop applying it. A paragraph may close with `<!-- file-refs: N/A - reason -->`, covering the paragraph above it and never below: an escape covering text the author never read is no narrower than having no gate. Every narrowing rule is pinned by a negative test case, 40 in all. Five defects were found by running it against this repo rather than against fixtures: URLs and absolute paths leaking their tail as a token; markdown links reporting each broken link twice; a bare `<!-- file-refs: N/A -->` passing because the comment terminator satisfied a "non-empty reason" rule; a per-line marker that did not survive prose, where the citation's sentence wraps; and a local oracle using fs.existsSync, which resolved git-IGNORED paths so a citation under target/ passed pre-commit and failed CI. The local mirror calls the same module rather than reimplementing the rule, and its base-tree reader is shared with the workflow - the byte-exact `cat-file --batch` slicing is not something to hold two copies of, and its failure mode is silent truncation. That framing is measured against real git, not assumed, with the command to re-measure recorded in the test. Costs 65ms, which is why the pre-commit hook registration is affordable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
astubbs
added a commit
that referenced
this pull request
Aug 19, 2026
…te-ups they earned
Records only - no mechanism here. Each of these was met while doing the work in
the preceding commits, and each is something the next person would otherwise
rediscover.
SIGHTINGS
* simpleBatchTest failed in Mutiny after failing in Reactor on
#308 - same alias, same 30-second timeout, same
shared BatchTestMethods lambda in core, BOTH on branches whose diffs contain
no Java at all. Two sightings in different modules say the wrapper is not
the variable. The assertion is off by one in the informative direction,
`Expected size: 3 but was: 4`: the records arrived, in one batch too many,
so it is a batch-BOUNDARY question rather than a lost-work one. Recorded
with the two readings that need separating - a loaded runner spreading work
out so the batcher closes early, or the batcher splitting under a timing the
library should tolerate - because assuming either is how a real defect gets
written off as a flake. Not quarantined: quarantine is master-state and
needs a diagnosis, and there is not one yet.
* The clones the widened scan exposed. Pre-existing - base and head both
report 27 CPD / 70 jscpd - and never triaged, because nothing outside four
directories was ever scanned. Deliberately not listed here: that is a
command's answer. What is recorded is that nobody has read them, and the
trap that the example modules are near-copies ON PURPOSE, so triage means
deciding per clone and saying which.
* A dispatched review ran 5m47s, concluded SUCCESS and posted nothing, while
claude-review stayed green on an older comment. Already-known, but this
sighting narrows it: two comment-route reviews on the SAME PR that morning
posted normally, so it is the route, not the diff or the reviewer.
* The scanner follow-up marked landed, with the warning that it is not the one
that mattered. Widening fixes SCOPE; the failure the write-up describes is
CAPABILITY, and follow-up (b) - assert a contract's canonical phrasing
appears in one file and nowhere else - is still open and is what would have
caught #287.
WRITE-UPS
* What a clone detector cannot see once scope is fixed: code embedded in
another language's file. CPD parses per language, jscpd keys format off the
extension, so JavaScript in a YAML string is never compared with JavaScript
in a shell heredoc - and no directory list changes that. It also records the
ratio trap: widening found MORE clones at a LOWER percentage, which killed
an absolute threshold silently.
* A forward-pointer on the 2026-08-12 write-up, whose findings are
deliberately NOT rewritten - a dated record says what was known that day, and
a "See also" is a cross-reference rather than a changed claim.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Follows #284, which shipped the on-demand reviewer. This makes it actually run, closes a privilege escalation it introduced, gives the comment route the same capabilities, and simplifies the gate it answers to.
Description
Master's review system is non-functional right now, and every PR in the repo needs an admin bypass until this lands. The auto-review no longer fires on push, the on-demand reviewer fails on every invocation, and the
claude-reviewgate is live - so nothing can turn it green.What is broken
The reviewer's first live dispatch (run 31464598166) died before reading a line of any diff:
claude-code-actionpicks its behaviour from the event, not from its inputs. On a pull-request event it runs in tag mode - tracking comment authored byclaude[bot], an MCP server for inline review comments, another for reading CI. On anything else it runs in agent mode with none of them.workflow_dispatchis not a pull-request event.The tidy fix does not exist, and that was established by experiment
The obvious escape is to hand the action a synthesised
pull_requestpayload, since@actions/githublearns its trigger from exactlyGITHUB_EVENT_NAMEandGITHUB_EVENT_PATH. That was built, pushed and dispatched. The runner discarded it - it writes theGITHUB_*context variables into every step's environment after the step's ownenv:block. Run 31466525816, from a step declaring both:actions/runneragrees in source (ScriptHandler.PrepareExecutionandNodeScriptActionHandler.PrepareExecutionboth finish by writing the github context overEnvironment, withevent_nameon its allowlist). The payload file is writable; the event name is not, and the name alone picks the mode. That commit was dropped, and the closed door is written into the workflow header so the next person does not spend an afternoon reopening it.So: agent mode, with what it costs paid for out loud
claude[bot]tracking comment the gate readsmcp__github_inline_comment__create_inline_commentgh run list/gh run viewplusadditional_permissions: actions: read. Notgh pr checks- reading check runs needs achecksscope this token cannot be grantedThe identity rule still holds: the action swaps
GH_TOKENfor its own GitHub App token before Claude runs, in every mode, sogh pr commentposts asclaude[bot].A defect the experiment uncovered
Dispatching a branch that edits this workflow gets
Skipping action due to workflow validation, and the action exits 0. In run 31466885778 every step reported success, the job was green in eleven seconds having reviewed nothing, and the gate re-ran on the strength of it. That is the class named indocs/solutions/workflow-issues/a-check-that-reports-success-without-having-run.md.The reviewer job now checks the action's own
conclusionoutput - set only on the path that actually ran Claude - and fails when it is empty.Privilege escalation, carried over from the branch it was written on
The
reviewjob heldactions: write, checked outrefs/pull/<pr>/head, and by design executes this repo'sbin/scripts from that checkout.actions/checkoutdefaultspersist-credentials: true, so that token was written into the.git/configof the very tree whose scripts then run. A same-repo PR could edit an allowed script, recover the token, and dispatch any workflow - includingrelease.yml, whoseworkflow_dispatchhas no actor gate.reviewdrops toactions: readwithpersist-credentials: false; the gate re-run moves to a separaterefresh-gatejob holdingactions: writealone and checking nothing out.The comment route is now a first-class reviewer
claude.yml- the@clauderoute - was the poor relation, and while the dispatched reviewer is broken it is the only route that works at all.It could not clear the gate.
claude-reviewis produced by apull_requestworkflow, and a check run keeps its last conclusion until something re-runs it, so a review arriving by comment posted, satisfied the rules, and left the required check red. Traced live on #288: review at 08:29, check still red, and a hand-rungh run rerun --failedat 08:37 was the entire fix. It now has the same two-job split as the dispatch route.It reviewed with no tools. It passed no
--allowed-tools, so it read rather than ran - on #288 it said so itself ("tool permissions blocked Bash execution"), on a PR whose entire diff was check scripts and their tests. It now carries the dispatch reviewer's curated allowlist, copied verbatim and verified identical.Two carve-outs withhold that allowlist, both found in review: a fork head, and an untrusted commenter (anything other than
OWNER/MEMBER/COLLABORATOR). Both withhold the tools rather than refusing the reply, so a wrong guess degrades the reviewer visibly instead of making@claudesilently do nothing. The fork case matters most: that job holdsCLAUDE_CODE_OAUTH_TOKEN, and a fork could otherwise replace an allowlisted script and wait to be asked to run the checks.It is the only route that can open inline review comments. The action installs that server for entity events;
issue_commentis one andworkflow_dispatchis not. Since unresolved review threads are what mechanically gate merges here, findings raised by comment can block a merge and findings raised by dispatch cannot. So the split is not "good reviewer, weak reviewer" - it is:-f focusor the packaged/code-review:code-reviewprocedure.The gate got simpler: reviewed, not re-reviewed
The gate used to require a review newer than the current head. It now asks only whether a finished
claude[bot]review exists on the PR.Strict is the stronger guarantee and it was not abandoned for being wrong - a review of commit 1 genuinely does not vouch for commit 20. It was abandoned because the coverage it protected already arrives from elsewhere (a separate reviewer reads every push), while enforcing it here cost:
All of that is deleted.
checks: readwent with it.What survives is mutation-tested. Identity (
claude[bot]per the API, never the comment body) and completion (an unticked task box is the reviewer's own record that it stopped partway - #275). Six mutations, all killed - including one that re-introduces a freshness comparison, so silently restoring strictness breaks the suite.The fork hole had to move. Leniency made it reachable: the gate asks only whether a finished
claude[bot]comment exists, and the comment route answers@claudeon fork PRs - so a maintainer asking an ordinary question would have greened a fork PR's required check. The gate now refuses a fork head outright, before reading any comment, which is where that guard belonged. It was never truly closed before; it was only hard to reach.One expectation did not survive contact, and it is worth stating plainly. Removing strictness was expected to delete both
refresh-gatejobs and the lastactions: writein the review system, on the reasoning that a push creates a fresh check run which greens immediately. That is true of a push, and it is not the case those jobs exist for: the gate fires onpull_requestonly, so the terminal flow - push, ask for a review, merge - raises no event after the review lands. Without the refresh, every PR would need a dummy push or a hand-run re-run. So the jobs stay, andactions: writewith them. The write scope is a cost of the check being produced by apull_requestworkflow, not a cost of strictness, and leniency cannot pay it off. Retiring it needs the reviewer to raise a check run on the reviewed SHA - now the highest-value change left here, and tracked.The strict implementation is archived at
archive/review-gate-strict-head-freshness. The trade, the assumption it rests on, and the trigger to restore it are indocs/inflight/parked-strict-review-gate-freshness.md. The assumption, named: per-commit coverage comes from the auto-reviewer. If that ever stops - disabled, rate-limited, quietly narrowed - it stops coming from anywhere, and nothing announces it, because the gate keeps passing.Also in this PR: one test quarantined, and master merged in
Two things that are not the review system, called out here so the squash commit does not hide them.
ProducerManagerTest.producedRecordsCantBeInTransactionWithoutItsOffsetDirectis quarantined.It failed on this PR at a point where the diff contained no Java at all, which settles quarantine
rule 2 (master-state, not PR-state) without needing a failure rate.
Rule 1 is satisfied rather than excepted - the mechanism is known and the fix is already written.
The defect is in the shared
BlockedThreadAsserter#assertUnblocksAfterhelper, not in the test: theunblocker is scheduled before the elapsed clock starts, so the measured window begins after the
delay it is compared against and is short by however long arming the scheduler takes. Under load
that exceeds a millisecond and
isAtLeastfails a correct implementation - seen asexpected to be at least PT20S but was PT19.998S, two milliseconds on a twenty-second bound. Owner#262,
flapping = true. Filed against the helper, since any test using itcan show the same signature. (#265 deletes the same assertion instead, so
whichever lands second faces a real decision, not a mechanical merge.)
Verified with both arms rather than assumed: under the gating exclusions the method reports
Tests run: 0; with-Dexcluded.groups=it runs and passes in 21.04s; and the three sibling testsin the class still run in the gating lane, so the exclusion is method-granular and the quarantine
lane still watches it. The lane's report on this PR lists it as
🟡🎲 passed (flapper).Master is merged in (#288), which brought its own quarantines of
PCMetricsTest.metricsRegisterBindingandOffsetEncodingBackPressureTest.backPressureShouldPreventTooManyMessagesBeingQueuedForProcessing-the other two flakes met while babysitting this PR.
Test evidence
Everything below is a real run against the live repo.
track_progressrejected onworkflow_dispatch. Nothing posted.GITHUB_EVENT_NAMEasworkflow_dispatch.refresh-gateskipped. One term changed, outcome flipped.Local, against the action's own code at the pinned v1 (SHA
5ef2e55): feeding the synthesised payload to its realparseGitHubContext/detectModeyieldspull_request/ tag mode, and the unmodifiedworkflow_dispatchpayload yields the production error. The gate's own rules are covered bybin/test-check-review-posted.sh(21 cases) and a mutation run over every surviving rule.What is NOT verified, and cannot be before this merges
No end-to-end review has been observed. No dispatched review has read a PR, posted a summary as
claude[bot], and turnedclaude-reviewgreen in one run; only the individual mechanisms were proved on runners.Neither route can be exercised first:
--ref <branch>refused,--ref masterruns the old copy);issue_commentalways runs the default branch's copy ofclaude.yml, so the comment route only looks testable.The first actions after merging:
Confirm: a comment appears, its author is
claude[bot], the comment route opens an inline thread, andclaude-reviewclears itself on both routes.This PR's own
claude-review- it can go green, and no bypass is neededEarlier drafts of this description said it could not. That was inherited from the strict era and is
now wrong, which is worth correcting rather than quietly dropping, because "this one needs a bypass"
is exactly the kind of claim that gets copied forward.
The dispatch route cannot help here.
--ref <branch>is refused by workflow validation, and--ref masterruns the broken copy this PR exists to fix. So there is nothing on master to dispatch.The comment route can. Three things make it work, each verified rather than assumed:
pull_requestworkflow uses the PR's copy ofthe workflow file, so the lenient gate is already live here - its current red says
"This PR has never been reviewed", which is the new message, not the old"not since <time>"one.@claudecomment route works today, because anissue_commentworkflow always runs thedefault branch's copy of
claude.yml- the one this PR has not changed yet. Onfix(quarantine): quarantine the two unit-lane flakes - one owned, one an explicit rule-1 exception #288 it posted twice, and both comments were authored by
claude[bot]with zero unticked task-list boxes - satisfying both surviving gate rules, identity and
completion.
So: comment
@claude review this, then let any subsequent gate run see it. That last step is neededbecause master's
claude.ymlhas norefresh-gateyet - this PR is what adds it - so the check willnot refresh itself; the next push, or one
gh run rerun, is enough.That is also a live demonstration of the change: under the strict rule, the review would have gone
stale the moment the next commit landed.
Checklist
docs/ci.md(what the dispatch trigger costs; why editing the reviewer cannot be tested pre-merge but can still be reviewed; the two routes and which to reach for; the new gate contract),docs/inflight/ci-review-agent.md(trimmed to genuinely open gaps),docs/inflight/parked-strict-review-gate-freshness.md(new - the archived strict gate and its restore trigger),docs/data/testing-evidence.yamldocs/features/-N/A - CI-internal review plumbing; it changes no shipped behaviour of the librarybin/test-check-review-posted.shrewritten for the new contract (21 cases), plus a mutation run over every surviving rule; one test quarantined with both arms verified and the registry/owner checks passing;bin/check-docs-data.sh,bin/check-quarantine-registry.sh,bin/check-quarantine-owners.sh,bin/check-shell-sigpipe.sh,bin/check-copyright-headers.sh,bin/check-action-versions.shandactionlintall run and pass