Repository navigation
fix(review): close the todo-index grant that let the reviewer rewrite the tree - #286
Conversation
… start it `.github/workflows/claude.yml` is the only reviewer available to a PR that edits `claude-code-review.yml` - the action refuses to run when its own workflow differs from the default branch, so the automatic reviewer skips itself there. That fallback passed no `--allowed-tools` at all, which is not permissive: an absent allowlist means Bash is not pre-approved, and a CI run has nobody to answer the approval prompt, so every script call was refused. So the reviewer that most needs to check a claim by running it was the one least able to. On #273 it verified a self-test's negative control by hand-tracing Python rather than executing a one-second script, and reported the block itself: `bash bin/test-check-docs-data.sh` and `./bin/check-docs-data.sh` were both refused while `git log`, `grep` and `python3 --version` ran unprompted. An earlier review round had concluded the opposite - that the absent restriction meant the block came from elsewhere and no workflow grant could clear it. That reasoning is wrong, and the next round disproved it in-session rather than by argument. Grants the read-only gates plus read-only `gh` queries. Not the Maven suites that `claude-code-review.yml` grants: those need a JDK and a warm cache this job does not set up, and a comment-triggered job holding secrets is the wrong place to run a build. THE GATE THAT COMES WITH IT Granting execution here is not the same trade as granting it in the review job, and the difference is easy to miss. A comment trigger receives repository secrets even on a FORK's pull request, where `pull_request` withholds them. The "bounded by fork PRs not receiving secrets" reasoning recorded in docs/inflight/ci-review-agent.md therefore never applied to this file: the OAuth token sits in a job any commenter could start. The job is now gated on `author_association` being OWNER, MEMBER or COLLABORATOR - enforcing the "trusted authors only" posture that entry already assumed, instead of leaving it an assumption. WHAT IS DELIBERATELY NOT IN HERE Which tree the granted scripts run against is unproven. The checkout names no `ref` and uses `fetch-depth: 1`, which for a comment trigger is the default branch - yet the #273 round described the fixed guard accurately, which master's copy could not have supported. A PR-head checkout in a comment-triggered job is a security change, not a convenience one, so it is left to be made knowing whether it is needed. Recorded as the open question; the first granted run settles it. Also deletes docs/inflight/ci-docs-data-audit-not-required.md, whose own delete-condition - the context appearing in ruleset 15055005 - is now met, and corrects two stale claims in ci-review-agent.md: bin/todo-index.sh is granted in both workflows now, and only actionlint remains missing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8He6kk23K3gN4qtojL9PE
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Claude finished @astubbs's task in 6m 29s —— View job Code review complete ✅
SummaryReviewed Three findings posted as inline comments:
|
✅ 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: dc14607d3f
ℹ️ 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".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Five review findings, all real. The first is a hole the original commit opened and did not close. REFUSE A FORK PR (P1, from the codex reviewer) The author gate authenticates the COMMENTER. It says nothing about who wrote the code. A maintainer typing "@claude" on an outside contributor's fork PR is a trusted trigger over untrusted content - and if the action checks out the PR head (the very thing this change admits it has not proven), the fork author chooses what `bin/check-docs-data.sh` contains and this job executes it with CLAUDE_CODE_OAUTH_TOKEN in the environment. Granting execution while checking only the commenter builds a confused deputy. A step before checkout now refuses any pull request whose `isCrossRepository` is not `false`, so untrusted refs are never even fetched. Fork PRs consequently cannot get an `@claude` review; they already could not get a useful automatic one, because `pull_request` withholds the token from forks, so this makes an existing gap explicit rather than creating one. GRANT bin/todo-index.sh ONLY WITH --check (P2, from the codex reviewer) Bare, it regenerates docs/todo-index.md - its own usage block says so. The grant was written as `Bash(bin/todo-index.sh:*)`, which permits the rewriting form, so a reviewer could modify the tree it was inspecting and then report a diff that included its own edit. Now granted only as `--check`, the verify mode. TWO COMMENTS THAT WERE WRONG (from the claude reviewer) "Read-only gates" was not true of the set: `bin/test-check-docs-data.sh` mutates a corpus file and restores it through a trap. claude-code-review.yml discloses that exception carefully at its own grant, and this file quietly dropped the disclosure while copying the grant. The comment now says which one writes and why the exception is considered. "Exactly one is non-null per event" was also wrong: an `issue_comment` payload carries both `comment.author_association` and the parent `issue.author_association`. The chain still selects correctly because `comment` is evaluated first, but the stated reason was false, and a future editor reordering the chain on the strength of it would silently begin trusting the wrong person. The comment now says the order is load-bearing. THE CHECKOUT AMBIGUITY, RESTATED RATHER THAN RESOLVED The reviewer independently reached the point this change already recorded, and sharpened it: before the grant, the unproven checkout ref was harmless because nothing in the tree was ever executed. The grant is what activates it. That remains the open question in docs/inflight/ci-review-agent.md, and the first granted run settles it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8He6kk23K3gN4qtojL9PE
|
Claude finished @astubbs's task in 3m 43s —— View job Code review complete ✅
SummaryThis is round 2, reviewing the state after
No new findings beyond what round 1 already surfaced and Note: this PR only touches |
Both were observed while watching #286, not reasoned about, and neither belongs in the workflow diff itself. WRITING ABOUT THE TRIGGER FIRES THE TRIGGER claude.yml matches with `contains(github.event.comment.body, '@claude')`, a plain substring test that knows nothing about backticks, code fences or quotation. Two of the review replies on this PR explained the trust model and quoted the trigger string; exactly two `Claude Code` runs fired on pull_request_review_comment and ran to completion. A one-to-one match. Prose about the feature invoked the feature, and nothing had asked for a review. Harmless while the fallback can run nothing. It stops being harmless the moment this PR grants script execution - the same argument that put the fork refusal here: the cost of the job starting when nobody asked for it goes up. A `startsWith` on the trimmed body would fix it, but that changes user-facing trigger semantics, so it is the repository owner's call rather than a silent tightening. Recorded with the related trap: comment-triggered workflows always run the copy on the DEFAULT branch, so neither the grant nor the fork refusal takes effect until merge, and a green Claude Code run on this PR is exercising master's file rather than this one. TWO TRACKED FLAKES FIRED ON CONSECUTIVE RUNS OF ONE COMMIT The unit lane went red twice on 821a91a, and the two failures were not the same failure: attempt 1 PCMetricsTest.metricsRegisterBinding:115 Failures: 1 expected 203.0, but was 207.0 attempt 2 OffsetEncodingBackPressureTest .backPressureShouldPreventTooManyMessagesBeingQueuedForProcessing:211 Errors: 1 ConditionTimeout, expected 139 but was 136 Both are already in this ledger - rows 3 and 1 - at 1/45 and 4/45. A code regression fails the same way twice; an unchanged commit failing two different tracked flakes in a row is the retry removal from #224 doing exactly what it was meant to do. PCMetricsTest goes from "one sighting is not a rate" to two. The mechanism behind the PCMetricsTest one is now known, which the ledger did not have. The test builds its expectation from a test-side counter snapshot and then asserts against a registry gauge, so two independently-advancing values are sampled at different instants with nothing holding the system still between them. Four more records completed in the gap. The metric was more current than the expectation written to test it. Same family as 16ac63b ("await the metric, not a counter that leads it") running the other way round. A CONTROL THAT WAS VOID, RECORDED BECAUSE IT WILL BE REACHED FOR AGAIN The first control tried here was "master at a797f75 passed the same suite 35 minutes earlier". It did not, and could not have: a push to master SKIPS the test matrix. Run 31459241709 shows `matrix.name: skipped`, with only `full build (master)` running; the unit lane exists on pull_request only. That control could not have failed, so its passing meant nothing - the "instrument that could have said yes" trap, from the write-up next door. The control that does work is other PR runs of the same lane: eight consecutive green unit runs across three branches that day, including this branch's own previous head, with only 821a91a red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8He6kk23K3gN4qtojL9PE
Both tests went red on this PR's own CI while it changed no Java at all, and both have a written fix sitting in an open PR. That is precisely the case the quarantine lane exists for: "tests that are red on master's gating CI when the fix lives in another, open PR". Leaving them red is the option the docs explicitly rule out - ambiguous checks and error-prone merge decisions - and @disabled is the other, which loses the signal entirely. Quarantine keeps them running in the non-gating lane while taking them out of the gates. WHY THEY QUALIFY Rule 1, no quarantine without diagnosis - both are diagnosed, in docs/inflight/test-untracked-ci-flakes.md: PCMetricsTest.metricsRegisterBinding compares a registry gauge against an expectation built from a test-side counter snapshot taken earlier in the method. Two independently-advancing values, read at different instants, with nothing holding processing still between them. Seen as expected 203.0 but was 207.0 - four more records completed in the gap, so the metric was MORE current than the expectation testing it. OffsetEncodingBackPressureTest.backPressureShouldPreventTooManyMessagesBeing- QueuedForProcessing sleeps out the static retry delay instead of awaiting the retry event, then asserts on a count still in motion. Fails as ConditionTimeout, expected 139 but was 136 within 30 seconds, when the runner is loaded enough that the sleep expires first. Rule 2, master-state not PR-state - both fail regardless of PR content. This PR contains no Java and no pom.xml, and the pair fired on two consecutive runs of one unchanged commit, each time a different one of them. fixedBy is #265 for both, verified against its diff rather than its title: it replaces the Thread.sleep(1000) above the metrics assertions with an await().untilAsserted(...) on the trailing meters, and swaps sleepQuietly(DEFAULT_STATIC_RETRY_DELAY) for await().atMost(ofSeconds(30)).until(() -> attempts.get() >= 2). flapping = true on both. They pass most runs - the unit lane was green on eight consecutive PR runs across three branches the same day - so a pass proves nothing and must not post the merge-blocking "your fix landed" thread that deterministic quarantines get. WHY THEY ARE VISIBLE NOW #224 removed the surefire retry that was absorbing them. They were always failing at this rate; the retry was buying the green. This is the ledger entry from that PR getting its first real use. The copyright header on OffsetEncodingBackPressureTest gains the modifications line, required once an upstream-derived file is touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8He6kk23K3gN4qtojL9PE
astubbs
changed the title
|
Claude finished @astubbs's task in 5m 9s —— View job Code review complete ✅
SummaryThis is round 3, scoped to what changed since the last review of Verified, all checked out:
One finding, verified rather than asserted:
I ran that exact grep against both files — it matches nothing in either, confirmed empty output. Since the check is only entered Impact is real but bounded: it doesn't break the registry-consistency gate ( Fix: change both to |
🧪🔒 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: 3f2f91a14e
ℹ️ 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".
Two enforced gates disagreed, and the registry could not satisfy both. `bin/check-quarantine-owners.sh` and `bin/quarantine-lane-report.sh` both parsed the owner marker with `grep -oE 'Owner: PR #[0-9]+'`, so the registry had to write a BARE number. `bin/check-issue-refs.sh` rejects a bare `#NN` on an added line, because the fork's numbers sit inside confluentinc's range and a bare one is a coin flip about which repo is meant. Adding the first two entries to this registry therefore failed the issue-ref gate by construction - not through carelessness, but because the two formats were mutually exclusive. The previous commit worked around that with the issue-ref gate's documented `issue-refs: N/A` opt-out. That was the wrong instrument: the opt-out exists for references that genuinely need no qualifier, not for a format conflict that can be fixed. An N/A left in place would also have quietly become permanent, since every future entry hits the same wall. Both parsers now accept `Owner: PR #NN`, `Owner: PR astubbs#NN`, and `Owner: PR astubbs/parallel-consumer#NN`, and the number is extracted from the `#NN` tail rather than the whole match, so a digit inside a qualifier cannot win. The bare form still parses, so existing entries keep working; the registry's format spec now asks for the qualified one and says why. Verified three ways rather than by inspection: qualified `Owner: PR #265` -> resolves, owner claim verified bare `Owner: PR #265` -> still resolves (back-compat intact) bogus `Owner: PR astubbs#999999` -> ERROR, PR does not exist The third is the one that matters. A parser that silently failed to match would also report no error, and would look identical to a pass - so the control had to show a wrong number being CAUGHT, not just a right number being accepted. Both parsers are changed together and carry a comment saying so; they read the same marker and drifting apart would mean the lane reporter attributing an entry to a different PR than the audit checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8He6kk23K3gN4qtojL9PE
|
Claude finished @astubbs's task in 3m 26s —— View job Code review complete ✅
SummaryThis is round 4, triggered by the new commit
Two findings from round 3 remain open — re-verified against current
New finding, independently verified (not just re-flagging codex's round-3 comment) — I traced the actual code path:
None of these three block the workflow-file change itself ( Branch · |
…was wrong Three review findings, all correct, two of them mine from the previous commit. THE QUARANTINE THAT SHOULD NOT HAVE HAPPENED OffsetEncodingBackPressureTest.backPressureShouldPreventTooManyMessagesBeing- QueuedForProcessing is un-quarantined, and its registry entry removed. It was tagged with the diagnosis "sleeps out the static retry delay instead of awaiting the retry event" and owned by #265, which replaces exactly that sleepQuietly(DEFAULT_STATIC_RETRY_DELAY) with an await. The narrative fitted. The line numbers do not. The failure is at line 211 of the commit CI ran: the waitAtMost(defaultTimeout).untilAsserted(...) block asserting committed offset metadata, specifically Truth8.assertThat(incompletes.getHighestSeenOffset()).hasValue(expectedHighestSeen) - the "value of: optional.get()" in the failure text is that Optional. It runs BEFORE the retry section #265 rewrites, and a change downstream of a failing assertion cannot fix it. So the real failure is a timeout waiting for the high-water mark to reach 139, stuck at 136, and nothing currently explains why three records never arrive. Rule 1 applies: no quarantine without diagnosis, undiagnosed red stays red and blocks, on purpose. Taking it out of the gating lane on a diagnosis that does not describe it is worse than leaving it red - it removes the pressure while pretending the cause is known. The error worth remembering: the fix PR was matched to the failure by subject-matter resemblance - both concern this test, both concern waiting - instead of by checking that the changed lines execute before the failing assertion. Match a fixedBy to a stack line, not to a theme. PCMetricsTest keeps its quarantine. Its diagnosis is a source-level read of two values sampled at different instants, and #265 inserts its await BEFORE the failing assertion, so it can actually gate it. A CROSS-CHECK THAT HAD SILENTLY STOPPED CHECKING check-quarantine-owners.sh compares the annotation's fixedBy against the registry's Owner line, and parsed it as `fixedBy = "PR #NN"`. The annotations added here write `fixedBy = "#265"`, so the grep matched nothing, the declared value came back empty, and the advisory could never fire. Adding the qualified form to one side quietly disabled the check that keeps the two sides honest. It now accepts the same three forms as the Owner marker. Proven by negative control rather than inspection: setting the annotation to astubbs#999 against a registry Owner of #265 now reports "annotation says fixedBy PR #999 but the registry Owner line says PR #265 - align them". Before the fix it reported nothing at all. A WITHDRAWN INFERENCE The ledger claimed the rerun failing at a DIFFERENT test was "the strongest evidence" against a regression, reasoning that a regression fails the same way twice. That does not hold: under concurrent execution one defect can perturb timing enough to surface different tests and different failure modes, so two dissimilar failures show only that the first did not reproduce. Withdrawn, with the load-bearing evidence restated as the source-level read it always should have been. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8He6kk23K3gN4qtojL9PE
There was a problem hiding this comment.
💡 Codex Review
https://github.com/astubbs/parallel-consumer/blob/de6e2c989d6a8e1e7244ffe0f1794f190e77817d/.claude/worktrees/review-7408/.github/workflows/claude.yml#L135
When the fallback reviewer invokes bin/check-quarantine-owners.sh, the script runs git fetch --depth=1 origin "$base" and fetches pull/$pr/merge at lines 99 and 110. This grant therefore reaches the network outside gh reads and mutates the checkout's FETCH_HEAD/shallow state inside the secret-bearing job; remove it from the allowlist or refactor the check to use only permitted read operations.
AGENTS.md reference: bin/AGENTS.md:L20-L24
https://github.com/astubbs/parallel-consumer/blob/de6e2c989d6a8e1e7244ffe0f1794f190e77817d/.claude/worktrees/review-7408/.github/workflows/claude.yml#L135
On this workflow's depth-1 checkout, invoking bin/check-copyright-headers.sh cannot normally reach its hard-coded fork-point commit; lines 72–83 of that script then print a warning, skip the entire check, and exit 0. The newly granted command can consequently appear successful without checking any headers, so fetch full history before running it or omit the grant rather than presenting a no-op as verification.
AGENTS.md reference: bin/AGENTS.md:L16-L18
ℹ️ 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".
|
Claude Code is working… I'll analyze this and get back to you. |
…-only Review found that `bin/check-quarantine-owners.sh` runs `git fetch --depth=1 origin <base>` and `git fetch --depth=1 origin pull/<n>/merge` and then reads `FETCH_HEAD`. So granting it gave the fallback reviewer network access outside `gh`, and moved the checkout fetch state underneath it - which makes anything it inspected afterwards untrustworthy. The same defect as granting bare `bin/todo-index.sh`, in the same file, one commit later. Its owner-claim verification belongs to CI's Quarantine Audit lane, not to an ad-hoc reviewer. `bin/check-quarantine-registry.sh` stays; it only reads the tree. Auditing the whole list rather than fixing only what was reported turned up two more inaccuracies in the comment above it: `bin/test-check-shell-sigpipe.sh` also writes - but only inside a `mktemp -d` it removes on exit, never the tree under review. The comment claimed exactly one grant wrote. `bin/todo-index.sh` can rewrite `docs/todo-index.md`, but is granted only as `--check`, so the rewriting form is not reachable through the allowlist. Worth stating, so a future editor does not "simplify" the grant back to bare. The comment now says which grants write and where, instead of asserting the set is read-only - a claim that has been wrong twice in this file now, both times because it was copied forward rather than re-derived from the scripts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R8He6kk23K3gN4qtojL9PE
|
P2 on Verified rather than accepted: lines 104 and 115 run That is the same defect as granting bare It is dropped rather than narrowed. Its owner-claim verification is the Quarantine Audit lane's job, and an ad-hoc reviewer has no reason to run it. So I audited the remaining eleven properly, and the report was worse than the finding:
The comment above the allowlist claimed exactly one grant wrote. That was wrong twice over — and it was wrong because I copied the sentence forward from Worth stating plainly: this PR's premise is that a reviewer which cannot execute anything reviews blind. Three rounds of review have now found three cases where I granted more capability than I described. The grant is still right; my accounting of it needed the scrutiny. |
|
Claude finished @astubbs's task in 1m 56s —— View job Code review complete ✅
SummaryThis is round 5, scoped to One finding — the same failure class the fix commit just addressed, in a grant the fix commit didn't touch:
Impact: if the fallback reviewer runs Fix options, in order of preference: (1) drop the grant, same reasoning as Branch · |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 427ed83d21
ℹ️ 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".
…undiagnosed entry diagnosed Two Codex review findings on #288, both correct. THE fixedBy CROSS-CHECK WAS STILL SILENTLY DISABLED The branch cherry-picked the Owner-marker parser fix from #286 but not the fixedBy one - that fix travelled in the same commit as the un-quarantine this PR deliberately skipped, and the handoff's claim that "the cross-check now accepts three forms" described that branch, not this one. So the extraction still matched only `fixedBy = "PR #NN"`, and PCMetricsTest's qualified `fixedBy = "#265"` parsed as empty - the annotation-versus-registry advisory could never fire, exactly the silent-disable #286 documented. The parser hunk is taken from #286 (8a7e1a0) verbatim, and this time it gets the regression test that commit lacked: a qualified fixedBy disagreeing with a qualified Owner line must produce the mismatch advisory. Against the old parser that test fails (empty declared, no advisory), so the silent-disable class is now fenced, not just fixed. THE ADVISORY REPORTED THE OPPOSITE OF THE ENTRY'S KEY STATE check-quarantine-owners.sh printed "diagnosed-but-unowned, find it an owner" for every unowned entry - including the backpressure one, whose defining property is being UNDIAGNOSED. Telling a maintainer to find an owner for a failure nobody understands points them at the wrong task. The no-owner branch now distinguishes: an entry recording a rule-1 exception reports "UNDIAGNOSED (recorded rule-1 exception); completing the diagnosis is the open task", and quarantined-test.sh's audit banner no longer claims every entry must be diagnosed. Both wordings are pinned by tests, including that the exception advisory does NOT contain "diagnosed-but-unowned". Verified: owner check green with the corrected advisories against the live registry, and the 40 quarantine contract/script tests pass (two new). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01F2MAyo5JDR5ZdWPGCxDCzY
…hanges) Directory move only, so every path is 100% similar and git's exact-rename detection cannot fail on it. The content edits follow in the next commit. Generated by bin/rename-packages.sh.
Text edits only. No file moves in this commit, so it cannot dilute the rename detection in its parent. Generated by bin/rename-packages.sh.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Two sharp edges on the review routes, one fixed and one recorded because its fix is a user-facing decision rather than a tightening. `bin/todo-index.sh` was granted without its argument ------------------------------------------------------------------------------- Both allowlists carried `Bash(bin/todo-index.sh:*)`. Bare, that script does not report - it REGENERATES `docs/todo-index.md` (`generate > "$OUT"` on its main path). So the reviewer was permitted to modify the working tree it had been asked to inspect, and any diff it reported afterwards was partly its own work. That is precisely the false-confidence class the allowlist exists to prevent, arriving through the allowlist itself: a reviewer that cannot tell its own edit from the PR's is worse than one that cannot run the script at all, because its output still reads as a finding. Granted now as `Bash(bin/todo-index.sh --check:*)` (both bare and `./` forms, per the as-written matching rule already documented there). `--check` is the read-only mode the script documents, and it answers the only question a reviewer has: is the committed index stale? Nothing else about the grant changes. It also makes an adjacent claim true again. The dispatch workflow's comment calls this family "fast, read-only VERIFICATION scripts" and names `bin/test-check-docs-data.sh` as "the one grant that writes", flagged as a considered exception. That was false while todo-index.sh was granted bare - there were two, and only one was disclosed. Applied to BOTH files, which is what their own `KEEP IN SYNC` note requires: `.github/workflows/claude.yml` (REVIEW_TOOL_ALLOWLIST) and `.github/workflows/claude-code-review-dispatch.yml` (`--allowedTools`). Prior art, as the PR-discipline rule asks: the defect class is "a grant whose default mode mutates". Swept the rest of both lists for it. `bin/check-*.sh` and `bin/test-check-*.sh` are the two read-only guard prefixes by convention; `bin/test-check-docs-data.sh` writes and restores through a trap and is already disclosed; the `ci-*-test.sh` wrappers and `./mvnw` write only to `target/`; the `gh` and `git rev-parse` grants are read-only. `bin/todo-index.sh` was the only undisclosed writer. Recorded, not fixed: writing about the trigger fires the trigger ------------------------------------------------------------------------------- `claude.yml` matches on `contains(github.event.comment.body, '@claude')` - a plain substring test that does not know about backticks, code fences or quotation - so a comment DISCUSSING the mechanism starts a billed job. Observed rather than theorised, on #286 itself: two review replies explained the trust model and quoted the trigger string in backticks, and exactly two `Claude Code` runs fired on `pull_request_review_comment` and ran to completion. One-to-one with the two replies containing it. Harmless while the fallback route could execute nothing. Not harmless now that it carries the curated allowlist: an unasked-for start costs a runner and a token-bearing job. The fix is cheap - `startsWith` on the trimmed body - but it would stop a mid-sentence mention working, which is a user-facing semantics change and therefore the repository owner's call. Filed in `docs/inflight/ci-review-agent.md` with that framing rather than applied. Verification ------------------------------------------------------------------------------- Both workflows parse (`yaml.safe_load`), and `bin/check-issue-refs.sh` is clean. `actionlint` is NOT installed on this machine and is Ansible-managed, so it was not run locally - the `workflows: action versions` lane covers it.
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: 2f3272be48
ℹ️ 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".
…well as the allowlist Review feedback on #286. The previous commit narrowed the reviewer's `bin/todo-index.sh` grant to `--check` and left the hole open. `Bash(bin/todo-index.sh --check:*)` is a PREFIX grant ------------------------------------------------------------------------------- `:*` is the trailing-wildcard form, so `bin/todo-index.sh --check=false` matched the "restricted" grant. The script's `[[ "${1:-}" == "--check" ]]` test then did NOT equal `--check=false`, left CHECK_MODE false, took the `generate > "$OUT"` branch and exited 0 - rewriting the tree the reviewer was inspecting, which is exactly what the narrowing was for. Reproduced before and after, rather than argued: $ echo MARKER >> docs/todo-index.md $ bin/todo-index.sh --check=false ; echo $? 0 # before: marker gone, file regenerated 2 # after: usage: bin/todo-index.sh [--check] Fixed on both sides, because they are independently wrong ------------------------------------------------------------------------------- 1. The grant is now EXACT - `Bash(bin/todo-index.sh --check)`, no `:*` - in `.github/workflows/claude.yml` and `.github/workflows/claude-code-review-dispatch.yml`, which carry an explicit KEEP IN SYNC contract. It is the only entry in either list without a trailing wildcard, so both comments now say so and say not to "tidy" it back for consistency with its neighbours. 2. `bin/todo-index.sh` REJECTS UNKNOWN ARGUMENTS instead of silently ignoring them, exiting 2 (1 stays "the index is stale"). This is the half that matters: an allowlist string in two workflow files is not where a guarantee should live, and the script is reachable by routes no allowlist governs. Verified that `--check=false`, `--check=true`, `--Check`, `-c` and `--check extra` are all refused, while `--check` and the bare regenerate mode still work. An allowlist-only fix would have been a fix to the exploit, not to the class: any future prefix-shaped grant of any argument would reopen it. Also here: the duplicated default-branch rule ------------------------------------------------------------------------------- `docs/inflight/ci-review-agent.md` gained a "Related trap" paragraph restating that comment-triggered workflows run the default branch's copy - a fact the same file already states in its comment-route entry, and which `docs/ci.md` owns under "Editing the reviewer". Replaced with a pointer naming that owner, keeping only the consequence specific to the trigger entry: the tightening cannot be exercised on the PR that makes it. Repo rule is never to state a fact twice. Verification ------------------------------------------------------------------------------- Both workflows parse; `bin/check-shell-sigpipe.sh`, `bin/check-issue-refs.sh`, `bin/check-copyright-headers.sh` and `bin/todo-index.sh --check` all clean. `shellcheck` and `actionlint` are not installed on this machine (Ansible-managed); the Repo Hygiene and action-version lanes cover them.
Leaving this one open for a decision — it is valid, it is live on master, and it also falsifies a claim I made in this PR. Confirmed. It violates a written rule. Why it survived this PR. The branch originally dropped the enumerated grant for it (commit It makes my own commit message wrong. Why I am not just fixing it: every option changes something outside this PR's scope.
My lean: (b). It keeps the prefix rule literally true — which is what makes it enforceable by a future Out of scope for this PR either way: it is a pre-existing master defect, not something this change introduced. |
…t the evidence it contradicted Review feedback on #304, plus a live defect the second finding uncovered. The closed entry was still a narrative ------------------------------------------------------------------------------- docs/inflight/AGENTS.md says not to rewrite a closed item into a "FIXED/DONE" narrative, and to shrink the file to its open follow-ups. The first commit closed `PCMetricsTest.metricsRegisterBinding` with an eight-line bullet carrying the mechanism, the generalised rule and a cross-reference - next to a three-line one for #260. That is the shape the rule forbids, however accurate it is: an agent scanning for current work still meets the test as though it were live. Now four lines: what fixed it, the rule it satisfied, and a pointer. The generalisation is NOT restated here - it already lives in `docs/solutions/test-flakiness/assert-the-commit-frontier-not-the-tick-path.md`, which cites it and observes that a ledger row was the wrong place to keep it. Deleting the restatement closes the loop that write-up opened instead of leaving the rule in two places to drift. The retained "Controls for these flakes" section no longer leans on the closed entry to justify itself. It is method for the two tests still open - the void control (a green push-to-master proves nothing; the unit lane is `pull_request`-only) and the working one - and now says so directly. `docs/data/testing-evidence.yaml` was contradicting the registry it cites ------------------------------------------------------------------------------- Found while answering the same reviewer on #286, where it was raised against a quarantine entry that PR no longer adds. The contradiction is real on master regardless: current_status: Empty - no test carries the annotation ... $ bin/check-quarantine-registry.sh Quarantine registry consistent (2 entr(ies), method-granularity checked). Two tests are held out of the gating lane - ProducerManagerTest and OffsetEncodingBackPressureTest - while the published evidence says the mechanism is unused. This file feeds release evidence and module-maturity claims, so it was overstating the suite's health in the one direction that matters. The fix is deliberately NOT a corrected count. That is what rotted: the field was true when written and nothing updates it when a test is quarantined or re-enabled, so a number would rot identically on the next change. It now names the registry as the live list and the only accurate count - which is what this block's own comment already demanded ("Do not restate them here; they would drift, and this file would then contradict the document it is citing as evidence"). `meaning` loses its "currently unused" clause for the same reason. Verification ------------------------------------------------------------------------------- `bin/check-docs-data.sh` (39 files structurally valid), a YAML parse of the evidence file, and `bin/check-issue-refs.sh` all clean. `current_status` has no consumer other than this file, so the wording change breaks no renderer.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 501998ce0d
ℹ️ 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".
…-fix Review feedback on #286: the previous commit closed a permission-boundary bug in `bin/todo-index.sh` and left nothing to stop it reopening. A repo-wide search finds no self-test for the script at all, and CI exercised only the valid `--check` path - so both a parser relaxation and a restored `:*` in either allowlist would go green. bin/AGENTS.md requires the case, and requires it to have failed: "a regression test that has never failed proves nothing". What it pins ------------------------------------------------------------------------------- `bin/test-todo-index.sh`, run from `pr-checklist.yml` immediately BEFORE the `bin/todo-index.sh --check` gate it protects, per the same doc. Nine cases: - `--check` on a current index passes; on a stale one reports 1 and rewrites nothing; bare regenerates. The valid paths, so a test that fails everything is distinguishable from one that fails the right thing. - `--check=false`, `--check=true`, `--Check`, `-c` and `--check extra` are each refused with exit 2 AND leave the index untouched. Both halves are asserted: the exit code proves the argument was rejected, the sentinel proves the rejection happened before the regenerate branch. - Both workflows grant the EXACT command, and neither carries a wildcard todo-index grant. This is the half a parser fix cannot cover: `:*` restored to either allowlist "for consistency" with its wildcard-bearing neighbours reopens the hole from the other side. The wildcard assertion reads grant lines only, since both files discuss the old forms in prose deliberately. Red before the fix, and only where it should be ------------------------------------------------------------------------------- Checked out `bin/todo-index.sh` and both workflows from 2f3272b (the revision whose grant was `Bash(bin/todo-index.sh --check:*)`) and ran it: ok: --check on a current index passes ok: --check on a stale index reports stale and does not rewrite ok: bare invocation regenerates the index FAIL: --check=false is refused and does not rewrite (expected '2 untouched', got '0 REWRITTEN') ... 9 assertion(s) failed `0 REWRITTEN` is the vulnerability stated as a test result: the allowed command regenerated the tree and reported success. The three valid-path cases still pass against that revision, so the failures are the boundary and not the harness. Safety of running it in the real checkout ------------------------------------------------------------------------------- Case 3 has to regenerate `docs/todo-index.md` to be worth anything, so the test backs the file up, restores through an EXIT trap, and refuses to start if the index already has uncommitted changes. Because CI runs it immediately before the `--check` gate, a silently failed restore would leave that gate inspecting a file this test had regenerated - which would pass by construction. So the restore is asserted (`cmp` against the backup) rather than assumed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a007ae1f5
ℹ️ 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 feedback on #286. The self-test added in 6a007ae pinned the todo-index grant against only the bare `bin/...` spelling, so half the permission contract was unguarded. A permission rule matches the command AS WRITTEN, which is why both workflows grant `bin/todo-index.sh --check` and `./bin/todo-index.sh --check` separately - the dot-prefixed invocation does not match the bare grant. Both predicates in the test recognised only `Bash(bin/todo-index.sh...)`, so removing or wildcarding just the `./` grant left every assertion green. Both spellings are now asserted independently, exact-grant and no-wildcard. Both mutations verified to fail, and to fail ONLY on the spelling mutated: # ./bin/ grant deleted from claude.yml FAIL: claude.yml grants the exact './bin/todo-index.sh --check' command # ./bin/ grant changed to :* FAIL: claude.yml grants the exact './bin/todo-index.sh --check' command FAIL: claude.yml has no wildcard './bin/todo-index.sh' grant The bare-spelling assertions stay green through both, which is what shows the two are independent rather than one predicate matching both by accident - the literal `Bash(` prefix is what stops `bin/` also matching `./bin/`. 17 assertions, all passing on the fixed tree.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 646ecc62c8
ℹ️ 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".
…track the networked checker Review feedback on #286. Two real gaps, both in work this PR added. The grant assertion tested the wrong invariant ------------------------------------------------------------------------------- It asked whether a wildcard rule existed whose text STARTED WITH the todo-index spelling. A broader neighbour re-covers the bare script without ever mentioning it: `Bash(bin/*.sh:*)` or `Bash(*:*)` would let the reviewer run the rewriting form again while every literal-spelling assertion still reported absent. The dispatch workflow's own allowlist comment names `Bash(bin/*.sh:*)` as precisely that hazard, so it was documented and untested. The assertion is now the actual security property: parse every `Bash(...)` rule on the grant line and require that NONE of them permits the bare command, in either spelling. Trailing-wildcard rules are matched as prefixes and plain rules exactly, with the pattern left unquoted so `*` globs the way the rules themselves do. When it fails it names the offending rule. Verified against three mutations, each caught: Bash(bin/*.sh:*) added -> permits no rule matching the bare 'bin/todo-index.sh' ... got 'bin/*.sh:*' Bash(*:*) added -> both spellings fail, got '*:*' ./ grant wildcarded -> exact-grant and bare-permission assertions both fail The new invariant subsumes the old one, so nothing is lost by replacing it. "Now tracked" was not true ------------------------------------------------------------------------------- The previous commit said the pre-existing `bin/check-quarantine-owners.sh` network/FETCH_HEAD problem was left open and "now tracked". The only record was a PR comment. docs/inflight/AGENTS.md is explicit that known problems belong in that directory precisely because the next session scans it and will not read every PR comment, so the claim was false in the way that matters. Added docs/inflight/ci-networked-checker-in-reviewer-grant.md with the evidence, the three options, the lean (rename), why an explicit deny is unavailable, and a delete-when condition. It also corrects a false claim that reached the PR description: `bin/check-cve-exclusions.sh` does NOT reach the network. Its `curl` is inside `cat <<'REPRO'`, printed as remediation guidance and never executed - a naive `grep curl` says otherwise, which is how it got asserted. A heredoc-aware sweep of every granted `check-*.sh`/`test-check-*.sh` finds `check-quarantine-owners.sh` to be the only instance.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca0d18f75e
ℹ️ 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".
…r defect its own note Review feedback on #286. Both findings are about where this PR put its words, not what it changed. Four copies of one explanation ------------------------------------------------------------------------------- The `--check=false` mechanism was written out at length in `claude.yml`, `claude-code-review-dispatch.yml`, `bin/todo-index.sh` and `bin/test-todo-index.sh` - plus twice more inline in the test. Six near-identical passages that a future change has to keep synchronised, when only the allowlist VALUES genuinely need duplicating. AGENTS.md forbids stating a fact twice precisely because the copies drift, and separately warns that a rule needing several paragraphs to defend itself is a rule that needs rewriting. Both apply. `bin/todo-index.sh`'s header is now the single owner - it is the parser that enforces the boundary, and AGENTS.md names an enforcing script's own header as a legitimate home for the detail. The two workflows keep a short local warning: the grant is exact, a wildcard reopens a tree-rewrite hole, do not tidy it to match its neighbours, full reasoning over there. The self-test's header says what each case asserts and defers the why. The local warnings are deliberately not bare pointers. A reader about to "tidy" the odd-looking entry needs the consequence at the point of temptation; what they do not need is the incident retold three times. A distinct defect inside an omnibus note ------------------------------------------------------------------------------- The trigger-substring problem went into `docs/inflight/ci-review-agent.md`, which already tracks many unrelated reviewer gaps. docs/inflight/AGENTS.md is explicit that the directory is one item per file, and that the prefix is the point: an agent listing the directory should see the shape of what is open without reading anything. Buried in an omnibus, the item is invisible to that listing, and any future work on it collides with unrelated edits to the same file. Moved to `docs/inflight/ci-claude-trigger-fires-on-prose.md` with its own delete-when condition. `ci-review-agent.md` keeps a two-line cross-reference so a reader arriving from the reviewer's gap list still finds it. Verification ------------------------------------------------------------------------------- bin/test-todo-index.sh (17 assertions), bin/check-shell-sigpipe.sh, bin/check-copyright-headers.sh and bin/check-issue-refs.sh all clean; both workflows parse.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
… a measurement `ci-disabled-jobs-and-runner-load.md` asked whether highcpu jobs still die of runner-lost-communication, and said to re-check before spending anything on a shared concurrency group. Chasing a red Chaos Pain Suite on #286 produced the answer, so it is recorded rather than left as an open question a second time. They still do. In one ~30-minute window the highcpu workflow failed on three unrelated branches - ci/claude-yml-script-grant, fix/concurrent-listener-registration and docs/inflight-note-currency - while succeeding on two of those same branches minutes either side. Unrelated branches failing together with interleaved successes is master-state under rule 2; no PR's diff explains it, and two of the three had no Java in them at all. The signature is the one the entry already names, and it is not a test failure. In run 32010207847 the `Chaos Pain Suite tests` step's log stops dead at 08:34:16 mid-scenario, the step does not complete until 08:40:40, and it fails with no BUILD FAILURE, no stack trace and no ##[error]. The process was killed; it did not report anything. That is what makes this class expensive: a reader grepping the log for a failing test finds nothing at all, and the natural next move is to suspect the PR. Also recorded, because it changes what the fix would have to address: several agent sessions were building against the same box concurrently. The load is not only CI's, so a shared concurrency group across workflows would only bound part of it. No action taken on the lane itself - Chaos Pain Suite is deliberately non-gating, so a red there is a finding rather than a merge blocker.
astubbs
changed the title
…ions Master landed #310's ranked revival of the 2022 micro-actor family the same day this branch wrote up the interrupt-overload finding, and the two were not pointed at each other. Forward: this branch's design read as greenfield - "add a nudge variant" - when a framework already exists, 537 lines in 4 files coupled to PC by one 16-line marker interface, with six ranked directions for reviving it. Both the solution write-up and the contract-debts note now say to read next-actor-revival.md first, and name survivor 5 (skeleton-first strangler) as the shape a payload-free nudge should land in rather than a rewrite. Back: survivor 6 is a concurrency mass budget - an ArchUnit ratchet on primitive counts, conversions graded on locks removed - and this branch has already taken a reading it can start from. Four meanings on one bit, four hand-clears, one of which does not clear and only warns that it cannot tell which meaning it got. That is a mass measurement with a documented failure behind it, which is more useful to a ratchet than a count taken cold. Also merges master: #310 and, in the previous merge, #304, #305, #307, #286 and #312. Core unit suite: Tests run 372, Failures 0, Errors 0, BUILD SUCCESS. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013ahSzHD72EvTGTYkmpTsx6
1 participant
Description
The review agent is granted
bin/todo-index.shso it can ask whether the committed TODO index isstale. It was granted as
Bash(bin/todo-index.sh:*), and that wildcard let it run the script bare— which regenerates
docs/todo-index.md.A reviewer that rewrites the tree it is inspecting then reports on its own edit. Any diff it describes
afterwards is untrustworthy, and nothing in the run says so.
Narrowing the allowlist is not enough on its own
The obvious fix — grant
--checkinstead of:*— does not hold, because the script's parser did notenforce it:
Anything unrecognised was silently ignored. So
bin/todo-index.sh --check=falsematched aprefix-shaped grant of
--check, leftCHECK_MODEfalse, fell through to the rewrite path, andexited 0. Found and reproduced by the review agent on this PR.
So the fix is two-sided:
Bash(bin/todo-index.sh --check)in both workflows. The absence of:*is load-bearing, and commented as such so it does not get "tidied" to match its neighbours.stale"). The guarantee now lives in the parser, where it holds however the caller was granted,
rather than in an allowlist string duplicated across two workflow files.
bin/test-todo-index.shpins the boundary and asserts both allowlists still grant the exactcommand rather than a prefix. Wired into
pr-checklist.ymlahead of the gate it protects.Recorded, not fixed
Two findings from reviewing this change get their own inflight notes rather than scope creep:
docs/inflight/ci-claude-trigger-fires-on-prose.md—claude.ymlstarts oncontains(body, '@claude'), a plain substring test, so a comment discussing the trigger starts abilled job. Observed here: two review replies quoted the string in backticks and fired exactly two
runs. The fix is a user-facing semantics change, so it is the owner's call.
docs/inflight/ci-networked-checker-in-reviewer-grant.md—bin/check-quarantine-owners.shfetchesand reads through
FETCH_HEAD, breaking thebin/AGENTS.mdrule that a granted prefix must notreach the network beyond
ghreads.Verification
bin/test-todo-index.shwas written red-before-fix: it fails against the pre-fix script, and againstmutations that reintroduce a
:*grant in either workflow.Checklist
ci-review-agent.mdcross-references thembin/test-todo-index.sh, wired intopr-checklist.yml