Repository navigation
fix(claude-code-review): request checks: read so the reviewer can read check runs (closes #829) - #830
Merged
Merged
Conversation
…d check runs (closes #829) actions: read covers workflow runs, not check runs: GET /repos/{owner}/{repo}/commits/{ref}/check-runs needs checks: read. Without it the reviewer, and check-pr-fully-clean.py which it runs, got "Resource not accessible by integration (HTTP 403)" on Morrison-Lab/bcs#964 and reported a clean diff as blocked on human review. Both the claude-review and post-review jobs request it now, the file header and README list it, and the README says callers must grant it too, since a nested job cannot request more than its caller allows. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rojsq7df4i6yQ2yE9wjb2g
Collaborator
Author
|
Self-review (fallback): this PR edits a workflow file, so the repo's own AI review skips it. An
VerdictClean. Reviewed commit: 65e0715 Posted by Claude Code (AI agent) --- not written by a human. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…py, and only where it is read Review findings on #830. The example caller stub, website/permissions.qmd, and website/reference/claude-code-review.qmd (prose and example) restate the caller grant list and had not been swept; a consumer copying the stub would have kept the 403 this PR fixes. post-review reads no check runs, so its checks: read is dropped. run-review-job-split-tests.py now asserts checks: read parity across the example stub, README, website, and the model job itself, so the next drift fails offline. Two lines packing more than one sentence are split for the semantic-break gate. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rojsq7df4i6yQ2yE9wjb2g
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…e the parity test fail loudly Round-2 self-review findings on #830: website/workflows.qmd and this repo's own dogfood caller .github/workflows/claude-review.yml restated the grant list without checks: read; the reference-example regex could match a grant anywhere after `permissions:`, so it now admits only the block's own `key: value` lines; and a missing doc target used to skip its assertions silently, so it now records a failure. The parity suite also covers workflows.qmd's scope list and the dogfood caller. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rojsq7df4i6yQ2yE9wjb2g
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rojsq7df4i6yQ2yE9wjb2g
Contributor
|
Claude finished review — View run The merge commit (5db8472) introduces no changes beyond 89cb6f6 — it's a clean fast-forward-equivalent merge with an empty diff. The full change set since the last review round is exactly these three cosmetic line-wraps. I checked all four findings from the round-1 review (65e0715) and confirmed they were resolved in round 2 (0ca7406), which was verdicted "Ready for merge" at that comment (posted 2026-09-06T00:41:34Z, run 34001818332). This round's diff (082bb7a..5db8472) only rewraps three prose lines in The prior round's "Ready for merge" verdict stands; this round adds nothing new to review. VerdictReady for merge Structured Review Data (JSON){
"schema_version": "1.1",
"reviewer": "claude",
"commit_sha": "5db84725327775adacbdfa47e2273eee7a21ee50",
"verdict": "CLEAN",
"findings": [],
"detailed_assessment": "The only changes since the prior CLEAN verdict are three cosmetic line rewraps in website/permissions.qmd, website/reference/claude-code-review.qmd, and website/workflows.qmd to satisfy the 80-column qmd lint, with no wording or semantic content altered.",
"holistic_assessment": "All four blocking findings from round one were already resolved and confirmed clean in round two; this round introduces no functional, documentation-drift, or scope regressions requiring further validation."
}Reviewed commit: 89cb6f6 |
Contributor
Contributor
2026-09-06 00:57 UTC |
Collaborator
Author
|
Post-merge incident: sliding Posted by Claude Code (AI agent) --- not written by a human. |
This was referenced Sep 6, 2026
Closed
d-morrison
added a commit
to Morrison-Lab/ai-config
that referenced
this pull request
Sep 7, 2026
…3313) Every claude-review run in this repo has ended in startup_failure since 2026-09-06T06:50Z, across four different branches. The last success was 2026-09-05T18:09Z. Cause, already documented as ai-config#3303: Morrison-Lab/gha#830 added 'checks: read' to the reusable claude-code-review.yml's review job, and v2 was slid to it. A called job may not request more than its caller grants, so every consumer whose caller lacked that grant broke at parse time. gha@v2 line 550 declares it; this caller granted contents, pull-requests, issues, id-token and actions, but not checks. The failure is silent in the way that matters: a workflow that never starts contributes no failing check run, so a PR shows green checks and no red one while nothing has reviewed it. check-pr-fully-clean.py correctly reports no review found, which reads as 'not yet' rather than 'broken'. This unblocks every open PR in the repo, not only the one that surfaced it. Refs #3303, #3309. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 7, 2026
d-morrison
added a commit
to Morrison-Lab/ai-config
that referenced
this pull request
Sep 7, 2026
…ected claims (#3323) * ums: a missing review, an unchained mutation, and two agy/classifier gotchas Four learnings from the 2026-09-06/07 session, each extending an existing memory entry rather than adding a new file: - gha-reusable-workflow-permissions.md: the checks: read startup_failure (ai-config#3303, Morrison-Lab/gha#830) as a fourth occurrence of this file's permission-mismatch class, plus the new insight that workflow_dispatch runs the default branch's workflow file, so a caller-permission fix on main heals every open PR via redispatch with no branch sync. - git-worktrees.md: extends the existing refspec-push entry with the detached-checkout form for creating new commits on a branch checked out elsewhere, and records flag-unchained-branch-switch.py firing correctly on a live incident (three of four checkouts failed, the unchained merge ran anyway and reported false success). - delegation.md: extends the agy headless-permission section with the command-vs-write_file asymmetry and the agy -p "" empty-prompt fast-fail, distinguishing it from a genuine stall. - mistake-patterns.md Pattern 43: records the new symptom that the auto-mode classifier's escalation spreads to unrelated innocuous commands, and that a same-session identical retry (no restart) recovers the goal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * ums: correct a wrong redispatch claim, plus five new learnings Corrects gha-reusable-workflow-permissions.md's prior "redispatch heals every open PR" entry: workflow_dispatch does run the default branch's fixed caller and does post a review comment, so check-pr-fully-clean.py correctly goes green, but the run attaches its check to main's SHA, not the PR head, so the required review/require-review check never appears and the PR stays unmergeable despite the clean instrument verdict (measured on ai-config#3305). Also corrects a second, untested claim in the same entry: a pull_request event's merge-ref semantics do not substitute for a push -- #3305's reopen (head SHA unchanged) hit startup_failure while #3312's push succeeded, with neither branch carrying the checks: read fix, so branch content was not the variable. New learnings, each extending an existing file: - review-verdict-pitfalls.md: no-push-without-self-review.py's VERDICT_LINE regex accepts only "Ready for merge"/"Needs (more) work" and reads a report's "CLEAN" (the vocabulary check-pr-fully-clean.py and fully-clean.md use) as no verdict at all (ai-config#3018). - mistake-patterns.md Pattern 52: a variable cd target on a guarded push now resolves to a wrong commit and reports it as fact rather than admitting it could not be resolved, worse than the case #2680 fixed (ai-config#3319). - dont-reinvent-wheel.md: "reuse the shared module" is a correctness claim about that module and should be measured -- scripts/lib/shellcmd.py has a false-positive wrapper-lookahead bug and a false-negative escaped- heredoc bug on top of its already-fixed heredoc-scanner history (ai-config#3321). - algorithmatize-checks.md: a warn-only guard's false positive and false negative are not equally expensive (over-warn by default), while a blocking guard's two directions both cost real work and want precision instead (ai-config#3302, ai-config#3304). - derive-dont-enumerate.md: enumerating review findings for a subagent hands over a summary instead of the primary source; briefing with the deriving command let a subagent rebut a finding with measured evidence instead of complying with an unchecked summary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ums: cite the report that actually reproduces the verdict-vocabulary bug Review finding: the entry's 'observed live' example named the report that carried BOTH vocabularies -- prose 'Ready for merge' beside JSON '"verdict": "CLEAN"'. That report satisfies VERDICT_LINE on its prose half, so it is not an instance of this bug, and the refusal recorded against it on ai-config#3018's opening post has a separate, still-unverified cause. The reproduction is the 2026-09-06 comment on the same issue, whose prose verdict read solely '### Verdict: CLEAN'. Cites that instead, and adds the discriminator so the wrong report is not picked up again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
d-morrison
added a commit
to Morrison-Lab/rme
that referenced
this pull request
Sep 7, 2026
Morrison-Lab/gha's v2 tag moved on 2026-09-06T00:56:53Z to c07f7d45,
which added `checks: read` to the reusable review job so it can call
/repos/{owner}/{repo}/commits/{ref}/check-runs (Morrison-Lab/gha#830).
A called reusable workflow cannot request a permission its caller
withheld, so from the next review trigger onward every Claude Code Review
run ended in `startup_failure` -- on PR branches and on main alike, with
no job created and therefore no check run to go red. Reviews stopped
happening and nothing said so.
Swept all twelve Morrison-Lab/gha callers across .github/workflows/,
comparing each callee's declared permission keys at @v2 against what the
caller grants. This is the only one with a gap; with it closed, all
twelve report ok.
Note the sweep had to glob both `*.yml` and `*.yaml`: an earlier pass
globbed only `*.yml`, examined eleven callers, and reported that count as
if it were the population. It missed check-spelling.yaml entirely. That
caller turns out to be fine, so the fix was never wrong -- but the
completeness claim was, which is the more dangerous half.
Closes #1142
Co-authored-by: d-morrison <2474437+d-morrison@users.noreply.github.com>
d-morrison
added a commit
that referenced
this pull request
Sep 7, 2026
…fails callers A reusable workflow's job cannot request a permission its caller lacks: the run ends in startup_failure before any job starts. #830 added checks: read to the claude-review job, and the v2 slide onto c07f7d4 killed review dispatch in every consumer that had not granted it. The parity test now pins the model job's permission SET rather than one key, so the next addition of any name fails offline instead of at a consumer's next PR. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d-morrison
added a commit
that referenced
this pull request
Sep 7, 2026
R5-F4 is the substantive one. README already carries 'Widening permissions is a breaking change', with the identical prior incident measured (#685, when #638 added issues: read to ai-code-review.yml and the v2 slide delivered it). My CLAUDE.md addition re-derived that rule without citing it, and prescribed a WEAKER remedy: grep the diff, where README says sweep REVDEPS.md and PR the caller grants BEFORE sliding. An operator following CLAUDE.md alone would have shipped #830 exactly as it shipped. I found this by grepping for 'checks: read' rather than for the concept -- the corpus-gap-from-a-grep trap. CLAUDE.md now defers to README as the authority, adds only the check that makes it fire, and records the actual lesson: the rule was written down with a worked precedent and was not consulted, so the check belongs at the readiness bar that made it feel safe. R5-F1 Four sites still said checks: read is 'not yet used by @v2'. v2 points at the commit that DOES request it, so the grant is currently the only thing keeping a caller alive -- and one of the four is gha's own caller, whose grant is why this repo is the 1-of-18 still working. A maintainer trimming an 'unused' permission would have broken it. R5-F2 The workflow comment still named #831 as the live tracker of a moving count, and this PR closes #831. Retargeted to #833; #831's frozen figure is labelled as frozen and dated. R5-F3 '@v2' meant the tag as it resolves today in one sentence and the tag after the slide six lines later, unmarked. Both now say which. R5-F5 'currently points at' and 'required by the currently-tagged @v2' were undated volatile claims that go false at the slide. Dated. R5-F6 The same point was made three times and #833 linked twice in one paragraph. Duplicate sentence dropped. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d-morrison
added a commit
that referenced
this pull request
Sep 7, 2026
…fails every caller without it (closes #831) (#832) * chore: open PR for #831 (v2 checks: read outage) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(claude-code-review): drop the callee's checks: read that startup-fails callers A reusable workflow's job cannot request a permission its caller lacks: the run ends in startup_failure before any job starts. #830 added checks: read to the claude-review job, and the v2 slide onto c07f7d4 killed review dispatch in every consumer that had not granted it. The parity test now pins the model job's permission SET rather than one key, so the next addition of any name fails offline instead of at a consumer's next PR. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: state that checks: read is caller-recommended and callee-unrequested at v2 Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address adversarial review round 1 on #832 F1 The comment claimed the outage ended 'until the tag moved back'. v2 is still c07f7d4; the rollback never happened. Says so, and says plainly that merging fixes nothing until v2 is slid onto the merge commit. F2 '18 consumers' was the count of repos pinning @v2, not the count that broke. Derived: 16 of 18. gha and ucdavis/bcs already had the grant. F3 CLAUDE.md's tag-slide readiness bar still said green-and-none-pending, which c07f7d4 satisfied -- the policy that permitted the incident was the one site not updated. Adds the permission-addition gate, with a command verified to flag the real incident (898a3e2..c07f7d4). F4 The example stub and dogfood caller comments, and a test message, still said the model job reads check runs. It no longer can. F5 The workflows.qmd needle could not show the list 'ends' -- appending the grant left it green. Anchored on the closing paren; mutation-confirmed. Also adds the missing self-test case for an ADDED scope, which nothing in the suite demonstrated; confirmed non-vacuous by weakening the assertion. F6 'the v3 that will request it' now cites #833 at all five sites. F7 Rewrapped the header sentence at a clause boundary. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address adversarial review round 2 on #832 R2-F1 The '18 consumer repositories' the round-1 sweep missed, in the test file's own comment -- invisible to a line-based grep because it spans a line break. Swept by joining comment continuations instead. R2-F2 CLAUDE.md said consumers 'lost review dispatch until gha#832'. That PR is unmerged and merging it is not what ends the outage; the same diff says so twelve lines away. Claim removed. R2-F3 '16 broke' was the count STILL broken. bcs#966 merged 19:14Z, hours after the slide, so bcs broke and was then repaired: 17 broke, 16 remain. Corrected at all six sites. R2-F4 The gate command resolved the tag from a LOCAL ref, which this same file documents as stale after a slide. Now resolves via ls-remote and diffs against FETCH_HEAD. R2-F5 The gate flagged this very PR: a caller's comment-only change on an unchanged key. Now scoped to workflow_call callees, and labelled a prompt rather than a gate, with its residual imprecision stated and #836 filed for the parsed instrument. R2-F6 'the one breaking change CI cannot see' was a false universal; it is a family, and the siblings are named. R2-F7 The round-1 rewrap produced a 128-char header line. Rewrapped. R2-F8 'recommended but not yet used: ... so the reads fail' read as though granting stops the 403. Says plainly that at @v2 the reads fail either way, because the model job does not request the scope. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address adversarial review round 3 on #832 R3-F1 The residual count decayed while the PR was being written -- ai-config granted checks: read 15 minutes before the review, making '16 still broken, only bcs has added it' false on both halves. The consumer docs and changelog now carry only the fixed historical fact (17 of 18 broke) and point at #831 for the number that moves; the workflow comment dates its figure. #833's own '18' corrected too. R3-F2 website/workflows.qmd was the one doc that never got round 2's F8 correction, so it still implied a caller's grant fixes the 403. R3-F3 The gate globbed *.yml only -- the exact drift workflow_discovery.py exists for, and untestable here since the tree has no *.yaml workflow -- and enumerated callees from the working tree while diffing FETCH_HEAD. Both fixed or disclosed, and the limitation list now says it is partial. R3-F4 'changing a required input's default' cannot break a caller: the default is unreachable. Replaced with the two that do break -- changing an OPTIONAL input's default, and making an optional input required. R3-F5 checks: read sat in a must-grant list while the prose below called it inert; marked '(recommended, see below)'. The reference page's example stub was the third copy and lacked the comment the other two gained. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address adversarial review round 4 on #832 R4-F1 The docs asserted present-tense that the 403 happens whether or not the caller grants the scope. v2 still points at the commit that DOES request it, so until the slide a grant is exactly what makes a caller work -- bcs has been green since 19:15Z on the strength of it. As written a repaired consumer could read the page and remove the grant. Now states what holds before the slide, after it, and at the v3. R4-F2 Three docs named #831 as the live tracker of the remaining count, and this PR closes #831. Retargeted to #833, which stays open and is where consumer adoption is the v3 precondition. R4-F3 '18 repositories pin @v2' names a strictly larger population than the one measured -- ucdavis/hac.sap pins quarto-publish@v2 and was never affected. Says 'this workflow at @v2' at all six sites. R4-F4 The closing-paren anchor guarded only workflows.qmd; the identical parenthetical in three other docs was unguarded. Now looped over all four, mutation-confirmed red in each. R4-F5 perm_block tolerated a trailing comment on the target line but not on the intermediate ones, so a pure reorder of two correct grants turned the suite red. Fixed; reorder now passes. R4-F6 'gha#836 carries the full list' was backwards -- CLAUDE.md's list is a superset. Reworded to 'carries the reasoning'. R4-F7 The CI-invisible family omitted renaming or removing a JOB, which is the one this repo's own reference pages expose consumers to by telling them to gate branch protection on job names. R4-F8 '(recommended, see below)' was a forward reference. Replaced with a self-contained qualifier. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address adversarial review round 5 on #832 R5-F4 is the substantive one. README already carries 'Widening permissions is a breaking change', with the identical prior incident measured (#685, when #638 added issues: read to ai-code-review.yml and the v2 slide delivered it). My CLAUDE.md addition re-derived that rule without citing it, and prescribed a WEAKER remedy: grep the diff, where README says sweep REVDEPS.md and PR the caller grants BEFORE sliding. An operator following CLAUDE.md alone would have shipped #830 exactly as it shipped. I found this by grepping for 'checks: read' rather than for the concept -- the corpus-gap-from-a-grep trap. CLAUDE.md now defers to README as the authority, adds only the check that makes it fire, and records the actual lesson: the rule was written down with a worked precedent and was not consulted, so the check belongs at the readiness bar that made it feel safe. R5-F1 Four sites still said checks: read is 'not yet used by @v2'. v2 points at the commit that DOES request it, so the grant is currently the only thing keeping a caller alive -- and one of the four is gha's own caller, whose grant is why this repo is the 1-of-18 still working. A maintainer trimming an 'unused' permission would have broken it. R5-F2 The workflow comment still named #831 as the live tracker of a moving count, and this PR closes #831. Retargeted to #833; #831's frozen figure is labelled as frozen and dated. R5-F3 '@v2' meant the tag as it resolves today in one sentence and the tag after the slide six lines later, unmarked. Both now say which. R5-F5 'currently points at' and 'required by the currently-tagged @v2' were undated volatile claims that go false at the slide. Dated. R5-F6 The same point was made three times and #833 linked twice in one paragraph. Duplicate sentence dropped. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address adversarial review round 6 on #832 R6-F1 The four-doc parity needle was a whitespace-intolerant substring, so a semantic-line-break reflow -- the edit this repo's own review item 7 asks for -- turned it red claiming the model job holds checks: read. Regex now tolerates the reflow; still red on an appended grant. R6-F2 The gate flagged every permission line of a callee ADDED since the tag, which has no callers to break (measured on check-code-similarity.yml, new in #728). --diff-filter=M drops those; the rename consequence is disclosed. R6-F3 'the model job stops requesting it' was stated twice, six lines apart, in three files. Merged, with the rationale attached to the first. R6-F4 workflows.qmd was the one prose site left undated, and the three consumer-copied stubs justified the grant by a fact that goes false at the slide -- and then lives forever in every repo that copied them. Reason is now durable: the v3 reinstates it on the callee. R6-F5 The exact-set assertion fires on a REMOVAL too but reported it as an addition needing a v3, contradicting CLAUDE.md. R6-F6 Breaks fell mid-noun-phrase ('a called' / 'workflow'). R6-F7 The PR body still said 16 and called bcs unaffected -- written before 17 was derived. bcs broke for ~18h. Body rewritten. R6-F8 'gha#831 ... freezes at 15' made the issue the subject; and the population count was undated while the figure it counts was dated. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address adversarial review round 7 on #832 R7-F1 is the substantive one. My bullet's stop action was 'go do the REVDEPS sweep', and REVDEPS.md lists 4 repos of which 1 names this workflow, while its owner-scoped fallback omits Morrison-Lab and Lacaedemon -- the org this repo now lives in. A sweep run during the outage would have found 1 of 18 consumers and reported the rest safe, so the remedy I pointed at would have reproduced the incident. REVDEPS.md now leads with the unscoped per-workflow search that actually worked, records why the scoped list is a fallback, and adds the two missing owners; CLAUDE.md carries the derivation inline rather than delegating it. R7-F2 'only additions break callers' missed value escalation: contents: read to contents: write adds no key and startup-fails every read-only caller. The grep surfaces it and the prose said to dismiss it. R7-F4 The workflow comment attributed the figure 15 to #831, whose body says 18 and whose comment said 16; #833 and #829 still said 18. Derivation posted to all three. R7-F5 perm_block tolerated trailing comments and reflows but not a full-line comment inside the block -- which the sibling stub this PR touches already uses. Mutation-confirmed both ways. R7-F6 workflows.qmd kept the duplication fixed in the other three files, and an 'It' whose nearest antecedent was the wrong noun. R7-F7 'all three phases:' enumerated two; the middle one is now named. R7-F8 The stub comment gave only the post-slide reason, so it omitted that the tagged @v2 makes the grant mandatory today -- the mirror of the round-6 error. R7-F3 (branch unpushed) resolves with this push; the pre-push guard held it until a clean verdict. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address adversarial review round 8 on #832 R8-F1 and R8-F2 are both on my own REVDEPS edit, and F1 is the same class this PR keeps fixing: I asserted the old owner-scoped sweep would have found '1 of the 18' without deriving it. Measured, it returns 10 of 18 -- the figure 1 belonged to the consumer table, a different list. Both figures are now derived and dated. R8-F2 The fallback queries could never work: GitHub code search reads a leading 'uses:' as a qualifier and drops the term, so both returned 0 hits under every owner list -- indistinguishable from no consumers. Measured 0 with the prefix, 30 without. Prefix dropped. R8-F3 The tag-slide section's closing Do/Don't still presented green CI as the whole gate, which is what the new bullet exists to deny. R8-F4 The consumer-search snippet hard-coded @v2 while the same section says to derive the major -- and #833 cuts a v3 that would be missed. R8-F5 CLAUDE.md's enumeration of what run-review-job-split-tests.py pins omitted this change's central guard, describing the suite as weaker than it is. R8-F6 README and website/versioning.qmd -- the sections the new bullet calls the authority -- still prescribed the REVDEPS sweep this PR documents as having missed most affected repos. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address adversarial review round 9 on #832 Every finding this round was on a numeric claim I wrote into documentation without deriving it -- the third round running where that is the dominant class, and the same failure the PR itself is about. R9-F1 'the 6 under Morrison-Lab and Lacaedemon plus 2 more' invented a residual category. Derived: the old five-owner list misses exactly 8, all under those two owners (Morrison-Lab 7, Lacaedemon 1), and the corrected list returns all 18. R9-F2 README and versioning.qmd said 'its owner-scoped fallback returned 10', dropping REVDEPS.md's own 'pre-2026-09-06' qualifier, so they discredited the fallback this PR had just fixed. Both now attribute the 10 to the retired list and say the current one returns 18. R9-F3 CLAUDE.md still summarized README's remedy as 'sweep REVDEPS.md', which this PR removed from README -- and contradicted its own text 35 lines later. R9-F4 The search snippet interpolated $major before the block that assigns it. Run verbatim it does not error: it drops to an unpinned query, measured 34 hits against 29, silently widening a blast-radius check. The assignment moved into the same block. R9-F6 The --diff-filter=M rationale cited a false positive that cannot be observed: check-code-similarity.yml predates the current tag, and the v2..main range adds no workflow at all (11 M, 0 A). The comment now says the filter drops nothing today and names a range that shows the shape. R9-F5 The owner-scoped fallbacks omitted --limit, so gh's default of 30 truncates silently -- the exact failure the surrounding note warns about. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address adversarial review round 10 on #832 R10-F1 'The list below ... returns all 18' pointed at the BROAD fallback, not the pinned query the 18 was derived from. Measured 2026-09-07: that fallback returns 100 hits across 4 repositories at --limit 100, and 304 across 28 at --limit 1000 -- so it was truncating exactly as silently as the default 30, in the block whose own comment claimed --limit fixed that. Cap raised, the measurement recorded, the sentence scoped to the pinned query, and a re-raise instruction added for when the hit count equals the cap. R10-F2 The pre-slide grep matched only the values read and write, so a block collapsing to permissions: write-all -- a widening that startup-fails every narrower caller, and a spelling the suite's own fixture uses -- was invisible. Pattern and prose both widened; verified against a synthetic write-all line, and confirmed still clean on the live range and still red on the incident range. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address adversarial review round 11 on #832 R11-F1 The 'permissions: (read|write)-all' alternative I added last round was dead code: the value pattern is unanchored, so 'write' already matches the prefix of 'write-all'. Verified -- the full pattern and the first alternative alone match an identical set. Removed, and the caveat list now records what the pattern really does: an INDENTED write-all is caught, a workflow-level one at column 0 is not, and that gap is unreachable today because every callee job declares its own block. R11-F2 'the pre-2026-09-06 owner list' asserted a supersession that never happened -- git says the OWNERS line last changed 2026-07-28 and the names 2026-06-18, and the list is replaced by THIS commit. Three sites now say 'the list this change replaces' and date it from history. README and versioning.qmd additionally narrated this diff's own edit in the past tense, which is false on main. R11-F3 The pre-slide caller search carried --limit 100 with no truncation caveat, on the one surface where truncation costs an outage. REVDEPS spells the hazard out; CLAUDE.md now does too, with the measured busiest count (claude.yml, 29) so the cap is checkable. R11-F4 REVDEPS's note still framed itself around moving @v1 and cutting @v2. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address adversarial review round 12 on #832 R12-F1 is the one that matters, and it inverts my own recommendation. The unscoped per-workflow search I had been telling operators to prefer returns 17 of the 18 consumers: reproduced across three runs, it omits d-morrison/rme, a public non-archived repo whose caller does pin this workflow at @v2. The owner-scoped form finds it. Neither form is complete, neither reports dropping anything, and the --limit guard cannot see it because 28 never equals the cap. Following my own instructions before the next slide would therefore have PRed 17 callers, slid the tag, and startup-failed the eighteenth -- the gha#831 outage reproduced by the procedure written to prevent it. Every site now says to run both forms and union them, with the measurement recorded. R12-F2 'the busiest workflow was claude.yml at 29 hits' was wrong on both identity and count -- 29 is claude.yml's UNPINNED figure, while claude-code-review.yml measures 28 pinned and 34 unpinned. The adjacent 34-against-29 pair mixed a scoped and an unscoped count. The specific ceiling is dropped in favour of the mechanical check, since a remembered ceiling is the same defect one level down. Also refreshes the dated consumer figure from the union query: 14 of 18 lack the grant as of 2026-09-07 02:24 PDT (rme added it since the last reading). Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address adversarial review round 13 on #832 Round 12's headline finding was my own, and it was wrong. I claimed the unscoped per-workflow search structurally omits a consumer, on three repeated runs. Re-measured: d-morrison/rme's caller was pushed at 01:55 PDT (rme#1143), my readings were at ~02:24, and by 02:53 the same query returns 29 hits across 18 with rme present, stable over four runs. The omission was code-search INDEX LAG, and my three repetitions were three samples of one stale index inside a ~30-minute window -- repetition is not independent sampling when the thing sampled is a cache. All four sites now carry the true lesson instead: the index lags a recent push, so union the two query forms, treat any count as a floor, and re-run after a delay when a caller may have changed recently. The owner-list half of the evidence was solid and is kept. R13-F2 'Measured 2026-09-06: 0 with the prefix, 30 without' cited 30, which is gh's default cap -- the exact truncation the next code block in the same file warns about. Re-measured at --limit 1000: 0 and 304 across 28 repositories. The artifact is now named as one. The adjacent '28 pinned' figure was stale at 29; the comment no longer depends on either number. Also corrects a prose overclaim the reviewer raised as non-blocking: CLAUDE.md said the model job requests those four keys 'at read and nothing more', but the exact-set assertion is over KEYS, with values pinned separately against write. Mutation-confirmed: pull-requests: none passes. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: address adversarial review round 14 on #832 R14-F1 The union instruction referenced ${OWNERS[@]} three lines before the array was assigned, under a header introducing a different sweep. Followed in order, bash expands an unset array to zero words, so the 'owner-scoped' per-workflow query is byte-identical to the unscoped one already run and the union silently collapses to a single form -- the exact failure the surrounding note exists to prevent, reproduced inside the instrument written to prevent it. The block is reordered so OWNERS is assigned first (both sweeps use it), the two per-workflow forms are spelled out as real adjacent commands rather than one command and a comment, and the reason for the ordering is stated so a later edit does not undo it. Run verbatim in order, both forms now execute: 29 hits / 18 repos each. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: correct a misleading REVDEPS clause (review round 15) 'returns all 18 -- under the PINNED per-workflow query, not under the broad fallback beside it' reads as saying the broad sweep would miss consumers the pinned query finds. Measured twice on 2026-09-07: the broad sweep returns 304 hits across 28 repositories, and that set is a strict superset of the 18 (pin - broad is empty). The true distinction is not coverage but specificity: the broad form cannot say WHICH callers pin the workflow being changed. Only the truncated --limit 100 run omits any of the 18, and the truncation note beside it already covers that. Reworded to say what actually separates the two forms. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: make the pre-slide snippet work on a shallow clone (@claude review) The @claude review's one finding, non-blocking but real and reproduced: the snippet resolved $tagsha with git ls-remote, which reads the remote without fetching anything, then diffed against that sha after fetching only main. On a shallow clone -- what actions/checkout gives by default, and what the review job itself runs in -- every loop iteration dies with 'fatal: bad object'. Reproduced on a --depth 1 clone and fixed by fetching the tag OBJECT (git fetch --tags --force) and diffing against the tag ref rather than a bare sha. Verified on both a shallow and a full clone: the loop completes, stays clean on v2..main, and still fires on the incident range. --force because a slide moves the tag, which this file already documents elsewhere. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: make the stub comment's reason durable (post-merge review) 'required by the tagged @v2 now' dates itself with a word rather than a date, and becomes false the moment v2 is slid onto this merge -- which this PR's own body says must happen immediately. These three files are the ones consumers copy verbatim and then keep indefinitely, so a reason that expires is the worst place to put one. The reviewer's failure scenario is the outage this PR exists to prevent: a maintainer auditing unused permissions weeks later checks the stated reason, finds it false, drops the grant, and gets startup-failed by the v3. Reworded to a reason that holds through all three phases. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: correct the code-search index mechanism (post-merge review round 2) I attributed the rme observation to 'a caller edited minutes ago may not be indexed yet'. The measurement rules that out: rme#1143 added only a comment and a checks: read line, leaving the uses: line the query matches on byte-identical since 2026-07-28 (verified from the commit's own patch -- the uses: line appears in neither the + nor the - side). So the document had matched for six weeks and was dropped anyway. That is a reindex DROP of an already-indexed file, not a not-yet-indexed new one, and the difference changes what a reader concludes. Under the old wording, a count that FELL between two readings reads as a consumer having removed its pin, because the stated hazard only covers new callers. All four sites now say a push to any caller's file can drop it until reindexed, and name the falling-count reading explicitly. The remedy was already right -- union the forms, treat counts as a floor, re-run after a delay -- so only the cause and its scope change. Refs #831 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
d-morrison
added a commit
to Morrison-Lab/ai-config
that referenced
this pull request
Sep 9, 2026
…kflow job permission (closes #3303) (#3304) * feat(hooks): deny a slide-major-tag dispatch that adds a reusable-workflow job permission (closes #3303) 2026-09-05 incident: Morrison-Lab/gha#830 added `checks: read` to reusable claude-code-review.yml's claude-review job and dispatched slide-major-tag, breaking un-updated consumers with startup_failure (Morrison-Lab/bcs#966; same class as gha#685). A called workflow job may not request permissions its caller does not grant; sliding a major tag is a release to all consumers rather than a post-merge chore. Clearable with ALLOW_BREAKING_SLIDE=1. * chore: regenerate the plugin hooks manifest for the slide guard Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rojsq7df4i6yQ2yE9wjb2g * fix(hooks): structurally parse reusable-workflow permissions in guard-slide-major-tag (refs #3303, #3304) - Switch from line-oriented diff regex heuristic to structural YAML parsing with block-context tracking fallback - Isolate permissions mappings at both workflow root and job levels, avoiding false-positive denials on step inputs (e.g. access: write in with:) - Detect permission additions at workflow root as well as under jobs - Add regression fixtures for both false-positive and false-negative reproductions - Update README hook catalog description to cover both job-level and workflow-level permissions * fix(hooks): fail closed when YAML cannot be parsed in guard-slide-major-tag (refs #3303, #3304) - Eliminate ad-hoc _extract_permissions_fallback parser that failed open on valid quoted keys and quoted values - Fail closed by issuing an actionable denial when PyYAML is unavailable or workflow YAML is unparseable - Ensure native PyYAML parsing handles quoted keys, quoted values, and flow mappings - Add tests in test-guard-slide-major-tag.py covering quoted keys/values, PyYAML absence, and malformed YAML syntax * fix(hooks): pre-filter workflow files for workflow_call before parse-or-deny in guard-slide-major-tag (refs #3303, #3304) * fix(hooks): reorder workflow parsing before raw scan in guard-slide-major-tag (refs #3303, #3304) * fix(hooks): compare permission escalation over PERM_RANK in guard-slide-major-tag (refs #3303, #3304) * fix(hooks): drop the unreachable PERM_RANK raise that failed open (refs #3304) The raise KeyError added alongside PERM_RANK was specified by my own brief as a way to 'fail loudly rather than silently comparing as equal'. It did the opposite. main() wraps evaluate() in a blanket 'except Exception: return 0', and for a PreToolUse hook return 0 with empty stdout is ALLOW, so an internal logic error became permission to proceed in a guard whose purpose is failing closed. A reviewer reproduced that end-to-end. Removed rather than converted to a deny, because RX_PERM_VAL already filters every value through ^(read|write|none)$ before it can reach the comparison, so the raise guarded an unreachable state. It was a second, weaker check standing where the authoritative one already suffices -- the shape that has recurred in every round of this PR. The test asserting it 'fails loudly' asserted something false and is replaced by one covering the invariant that does hold: an out-of-rank value is filtered upstream, so it neither reaches the comparison nor produces a spurious deny. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(guard-slide-major-tag): assert on stderr so 11d can fail Review finding: test 11d asserted only verdict == allow and discarded stderr, but allow is reachable two ways -- the invariant holding and the value being filtered upstream, or the invariant breaking so PERM_RANK raises KeyError and main()'s blanket handler converts it to exit 0 with empty stdout, which is also allow. The test could not tell those apart, so it covered nothing. Adds an assertion that stderr carries no 'could not evaluate command'. Mutation-tested, which is the point of the change. With RX_PERM_VAL widened to .* the new assertion fails with exactly: guard-slide-major-tag: could not evaluate command ('admin') while the old verdict-only assertion stayed green under the same mutant. Restored: 43/43. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: rank the shorthand permission forms so a downgrade is not a deny _find_added_permissions compared the shorthand form (read-all/write-all) with a plain inequality while the dict form used PERM_RANK, so a strict downgrade read as an added permission and denied a safe slide. _find_added_permissions({'workflow': 'write-all'}, {'workflow': 'read-all'}) -> [('workflow', 'read-all')] The same failure class sat in the dict branch, which the review did not name: a shorthand baseline was coerced to {} there, so every key of an explicit dict looked newly added. write-all -> {contents: read} # a strict downgrade -> [('workflow', 'contents: read')] Both now rank on one scale. SHORTHAND_RANK maps read-all/write-all onto PERM_RANK's read/write, and _shorthand_floor turns a shorthand baseline into a floor the keys it covers are compared against. The floor does NOT extend to every key, and getting that wrong opened a worse hole than the one being fixed. `id-token` accepts only `write` or `none` and never `read` (GitHub's workflow-syntax reference), so read-all provably cannot grant it and write-all's coverage of it is documented nowhere. A floor equal to PERM_RANK's maximum made every dict key unreachable after a write-all baseline, so write-all -> {id-token: write} stopped being reported -- a real escalation, silently allowed, that the pre-fix code denied correctly. SHORTHAND_UNCOVERED_KEYS holds id-token out of the floor so it keeps a zero baseline and stays reportable. For a guard, a scope it cannot rank must be compared against zero. Other asymmetries kept deliberately: - dict -> shorthand still reports. read-all grants read on scopes the dict never listed, so it widens even where one named key drops. - An unrecognized shorthand reports rather than ranking to 0. Fourteen cases added (43 -> 57). Five fail against the pre-fix hook, and one -- write-all -> {id-token: write} -- fails against the intermediate version that had the floor covering every key, so both directions are pinned rather than only the one the finding named. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: list the scopes a shorthand covers, not the ones it does not Round 2 review of 26b1544 found the carve-out pointed the wrong way, and it is right. SHORTHAND_UNCOVERED_KEYS was an exclusion list, so a scope nobody had added to it inherited a full floor and its explicit grant stopped being reported -- a silent allow, the direction this guard must never fail in. GitHub added `vulnerability-alerts` on 2026-09-03, five days before that commit, and it already exercised the gap. Inverted to SHORTHAND_COVERED_KEYS: the fourteen scopes GitHub documents as `read|write|none`, taken from github/docs data/reusables/actions/github-token-available-permissions.md. Anything absent -- `id-token` (write|none), `vulnerability-alerts` (read|none), and any scope added after this list was written -- gets a zero floor and stays reportable. The cost of a new scope is now a spurious deny that names itself, rather than an escalation that says nothing. That is the trade this guard should make, and it is the same reasoning the previous round applied to id-token, applied to the shape of the check instead of to one key. Also fixes the no-op the same review found: the unrecognized-shorthand branch reported a value identical to the baseline, so _find_added_permissions({'j': 'admin-all'}, {'j': 'admin-all'}) -> [('j', 'admin-all')] contradicting the docstring's claim that a no-op is never reported. An equality test now short-circuits ahead of the recognized/unrecognized split. Three cases added, 57 -> 60. All three fail against 26b1544: vulnerability-alerts and an unknown future scope both read as covered there, and the no-op reported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
5 tasks done
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
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.
Closes #829.
What
claude-code-review.yml'sclaude-reviewandpost-reviewjobs now requestchecks: read; the file header and the README's caller-permissions list nameit, and the README explains why callers must grant it as well.
Why
actions: readcovers workflow runs, not check runs:GET /repos/{owner}/{repo}/commits/{ref}/check-runsneedschecks: read.On ucdavis/bcs#964 (2026-09-05) the reviewer found no defects, then every
check-status read -- including
check-pr-fully-clean.py, which it ran --failed with
Resource not accessible by integration (HTTP 403), and the rundeclared the diff blocked on human review for a reason unrelated to the
diff. Re-running cannot help while the token is the same.
Consumer side: ucdavis/bcs#965 (the caller adds
checks: readonce this isreleased).
This is a workflow-file edit, so the repo's own AI review skips it; an
agyread-only self-review is running (verdict to follow in a comment). Pushed
with
ALLOW_UNREVIEWED_PUSH=1(Morrison-Lab/ai-config#3209).🤖 Generated with Claude Code
https://claude.ai/code/session_01Rojsq7df4i6yQ2yE9wjb2g