Skip to content

Reinstate checks: read on the claude-review model job as v3 - #882

Open
d-morrison wants to merge 2 commits into
mainfrom
claude/reinstate-checks-read-v3
Open

d-morrison wants to merge 2 commits into
mainfrom
claude/reinstate-checks-read-v3

Conversation

@d-morrison

Copy link
Copy Markdown
Collaborator

Summary

Closes #833.

claude-code-review.yml's claude-review (model) job holds checks: read again, so the reviewer's GET /repos/{owner}/{repo}/commits/{ref}/check-runs reads stop 403ing (gha#829, ucdavis/bcs#964). #832 had dropped that scope to un-break the 17 of 18 @v2 consumers that hadn't yet granted checks: read at the caller level (gha#831) — but that also reopened the 403 for everyone, so a review can currently report a genuinely clean, all-green PR as blocked because it can't verify CI status at all, not because anything is actually wrong.

Real-world trigger: Morrison-Lab/lbt#11 — content review clean, every substantive CI check green, but require-clean-verdict failed because the model job's own token couldn't read check-runs. Retrying the review job produced the identical failure, confirming it's structural rather than transient.

Why this is a new major tag, not a v2 slide

Per this repo's own documented rule (README's "Widening permissions is a breaking change", and the extensive history already in claude-code-review.yml's comments): a reusable workflow's job cannot request a permission its caller lacks, or the whole run ends in startup_failure before any job begins. Adding checks: read back is exactly that kind of addition, so it belongs in a new tag a caller opts into, never a silent slide of @v2.

I have not cut the v3 tag itself — that's a maintainer call, and #833 also asks to "confirm first that consumers have picked up the caller-side grant" before it's worth widely recommending. This PR only lands the code, tests, and docs so that check is unblocked whenever you're ready to look at it.

Changes

  • .github/workflows/claude-code-review.yml: checks: read back on the claude-review job's permissions:, comment rewritten with the full v2→v3 history for the next person who touches this.
  • .github/workflows/scripts/tests/run-review-job-split-tests.py: the exact-set permission assertion, the synthetic good_wf fixture, and the --self-test mutation suite all updated to the new v3 baseline — added_scope now tests an unrelated extra scope (security-events: read) instead of reusing checks: read, since that's no longer an addition; added a mirror dropped_checks mutation to guard the other direction (accidentally losing the scope again).
  • README.md, CLAUDE.md, website/permissions.qmd, website/reference/claude-code-review.qmd, website/workflows.qmd: prose brought in line with the new state (all four copies of the "model-scope parenthetical" the test suite pins).
  • New changelog fragment (breaking).

examples/claude-code-review.yml and run-audit-callee-permissions-tests.py are intentionally untouched (the former already grants checks: read at the caller level; the latter uses it only as an unrelated synthetic example). No existing changelog.d/*.md file was touched.

Test plan

  • python3 .github/workflows/scripts/tests/run-review-job-split-tests.py — 243/243 assertions pass
  • python3 .github/workflows/scripts/tests/run-review-job-split-tests.py --self-test — 54/54 mutation cases pass
  • python3 .github/workflows/scripts/tests/run-audit-capability-versioning-docs-tests.py — unaffected, still passes
  • YAML re-parses cleanly; only the claude-review job's permissions changed, no other job disturbed
  • Adversarial subagent review (foreground, separate context) confirmed the above independently rather than trusting the commit message's own claims

🤖 Generated with Claude Code

d-morrison and others added 2 commits September 15, 2026 23:52
Closes #833. #832 dropped this scope from the model job to un-break the
17 of 18 repositories pinning claude-code-review.yml@v2 that had not yet
granted checks: read at the caller level (#831), but that also reopened
the HTTP 403 on GET .../commits/{ref}/check-runs the scope existed to
fix in the first place (#829, ucdavis/bcs#964): the reviewer (and any
caller-side check-pr-fully-clean.py) can no longer verify CI status and
reports an unverifiable diff as blocked, even when every real check is
green. Measured on Morrison-Lab/lbt#11 (2026-09-16): content review
clean, every substantive CI check green, but require-clean-verdict still
failed because claude-review's own token couldn't read check-runs.

Reinstating the scope now is a major-tag bump rather than a v2 slide, per
the rule this repo's own README and CLAUDE.md already document: adding a
job permission breaks every caller lacking the grant at parse time. A
caller still pinned at @v2 is unaffected -- it only sees what its pinned
tag resolves to. A caller that already grants checks: read (the README,
examples/claude-code-review.yml, and website docs have asked for it since
#832) moves to @V3 at no cost.

Updates run-review-job-split-tests.py's exact-set assertion, its good_wf
fixture, and its mutation self-tests to match the new v3 baseline
(added-scope now tests an unrelated extra scope instead of checks: read,
plus a new dropped-checks mutation guards the other direction), and
brings README.md/CLAUDE.md/website docs' model-scope prose in line with
the new state. All 243 run-review-job-split-tests.py assertions and all
54 of its --self-test mutation cases pass locally.

Cutting the actual v3 tag is left to a maintainer -- this PR only lands
the code and docs; see the PR description for the adoption-count caveat
noted in #833.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
check-new-line-breaks flagged one line in the new changelog fragment
that packed two sentences together. Rewrapped to one clause/sentence
per line, matching this repo's own convention (and the rest of the
fragment, which was already compliant).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@d-morrison

d-morrison commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed the new-line-breaks/diff-scoped-guard failure (a changelog-fragment line packed two sentences together).

The remaining lint-checkout-tokens failure is expected and inherent to this PR, not a bug: it's audit_callee_permissions.py --base-ref v2 correctly reporting that this PR adds checks: read to the model job relative to the currently-slid v2 tag. That's exactly what a major-tag-bump PR does by definition, and per this repo's own versioning docs the major tag never advances automatically on a merge to main -- advancing it (or cutting a new v3.0.0 release so resolve-major-tag.sh starts resolving v3 at all) is a separate, deliberate workflow_dispatch step. I don't think this check can go green before that release exists, and I haven't cut it -- per #833 itself, confirming consumer adoption first and deciding when to cut v3 is a maintainer call, not something I should do unilaterally from here.

Posted by Claude Code (AI agent) --- not written by a human.

@github-actions

Copy link
Copy Markdown
Contributor

Claude finished reviewView run


Confirmed unchanged: the v2 tag (27721bd6) still requests no checks: read anywhere in claude-code-review.yml — same result as the prior round. The stale claim in the four doc files stands.

Review: PR #882 — Reinstate checks: read on claude-review as @v3

What changed since the last review round (b646cce6..97e100f7): one commit, da970b7 ("Fix semantic-line-break violation in the changelog fragment"), reflowing changelog.d/reinstate-claude-review-checks-read-v3.breaking.md onto clause-length lines. No other file changed (verified via git diff --stat and a targeted git diff on the four doc files below, both empty).

Prior finding — still open (not addressed this round)

The 2026-09-16T07:03:55Z review flagged a factual staleness: four doc locations assert checks: read is "required by @v2 at startup," which is false for the currently-tagged @v2 (commit 27721bd6, a descendant of #832's fix, requests no checks: read anywhere in claude-code-review.yml — re-verified above with git show 27721bd6:.github/workflows/claude-code-review.yml, zero live grants, only comments). That requirement applied only to the transient c07f7d45 slide (2026-09-05–2026-09-07) that #832 already fixed.

This diff touched only the changelog fragment, so the claim is unchanged and still present verbatim:

  • README.md:165`checks: read` (grant it; required by `@v2` at startup, and by the model
  • README.md:197Keep granting checks: readregardless: it is required by@v2 at
  • website/permissions.qmd:71 and :101 — identical phrasing
  • website/reference/claude-code-review.qmd:55 and :87 — identical phrasing
  • website/workflows.qmd:344The tagged @v2 requires it at startup regardless of whether the

Same fix suggestion as before: reword to something scoped correctly, e.g. "optional under the currently-tagged @v2 (which no longer requests it, per #832) but required by the model job once you adopt @v3."

Resolved this round

The minor semantic-line-break finding (changelog.d/reinstate-claude-review-checks-read-v3.breaking.md:7, two sentences packed onto one line) is fixed — the fragment now breaks at clause boundaries throughout, confirmed by reading the current file content.

Verdict

Needs more work — the one substantive finding from the prior round (the stale "@v2 requires checks: read at startup" claim, false for the currently-tagged @v2, repeated in 4 files) remains unaddressed in this diff; the changelog line-break fix from this round is correct and complete.

Structured Review Data (JSON)
{
  "schema_version": "1.1",
  "reviewer": "claude",
  "commit_sha": "97e100f7ef795fa43b2fdd5a4a0f785221f6d89b",
  "verdict": "NOT_CLEAN",
  "findings": [
    {"file": "README.md", "line": 165, "category": "doc", "message": "Claims checks: read is 'required by @v2 at startup', but the currently-tagged @v2 (27721bd6, a descendant of #832's fix) requests no checks: read anywhere in claude-code-review.yml -- re-verified via git show against the live tag. Carried over unaddressed from the prior review round; this diff did not touch this file."},
    {"file": "README.md", "line": 197, "category": "doc", "message": "Same stale claim repeated: 'it is required by @v2 at startup'. Carried over unaddressed from the prior review round."},
    {"file": "website/permissions.qmd", "line": 71, "category": "doc", "message": "Same stale '@v2 at startup' claim. Carried over unaddressed from the prior review round."},
    {"file": "website/permissions.qmd", "line": 101, "category": "doc", "message": "Same stale '@v2 at startup' claim. Carried over unaddressed from the prior review round."},
    {"file": "website/reference/claude-code-review.qmd", "line": 55, "category": "doc", "message": "Same stale '@v2 at startup' claim. Carried over unaddressed from the prior review round."},
    {"file": "website/reference/claude-code-review.qmd", "line": 87, "category": "doc", "message": "Same stale '@v2 at startup' claim. Carried over unaddressed from the prior review round."},
    {"file": "website/workflows.qmd", "line": 344, "category": "doc", "message": "'The tagged @v2 requires it at startup regardless of whether the model job uses it' -- false for the currently-tagged @v2. Carried over unaddressed from the prior review round."}
  ],
  "detailed_assessment": "This round's only change is a semantic-line-break reflow of the changelog fragment, which correctly resolves the prior round's minor style finding, but the four doc files carrying the false 'checks: read required by @v2 at startup' claim were not touched and remain factually inaccurate against the live v2 tag.",
  "holistic_assessment": "Scope stays appropriately limited to a cosmetic fix this round; the substantive doc-accuracy regression from the prior round persists and should be corrected before merge per this repo's own fact-checking and staleness standards."
}

Reviewed commit: da970b7

@github-actions

Copy link
Copy Markdown
Contributor

💰 Cost: $0.8294 (review) — run

@github-actions

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
🚀 Preview available at: https://Morrison-Lab.github.io/gha/pr-preview/pr-882/

Built to branch gh-pages at 2026-09-16 07:06 UTC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reinstate checks: read on the reviewer's model job as a v3, not a v2 slide

1 participant