Migrate PR review workflows to Morrison-Lab/gha's claude-code-review.yml@v2 - #110
Conversation
|
…yml@v2 Replaces the two hand-rolled review workflows with one caller stub targeting the reusable workflow, retiring the manually-synced `claude_args` allowlist that both copies had to keep in step by hand. - claude-code-review.yml becomes a caller stub (pull_request + issue_comment `/review` + workflow_dispatch). - claude-review-comment.yml is deleted; its on-demand path is now the `/review` slash command, and its fork-PR checkout trick never executed once (5/5 runs skipped, 2026-08-01 through 2026-08-24). Refs #101
- Stop dispatching a review for a fork PR. The reusable workflow's dispatch guard blocks forks, so dispatching posted ":mag: dispatched a review" and delivered nothing, with no red check to contradict it. Post an explicit fork notice linking #90 instead. - Add `pull-requests: read` to dispatch-on-comment. It calls `gh api repos/../pulls/N`, and declaring any scope zeroes the unlisted ones. - Document the three skip gates the reusable workflow adds (draft, fork, bot-sender) and the #84 red-to-gray interaction. - Correct three comments that asserted capabilities this repo does not have until #100 lands: claude.yml cannot dispatch a review, and `@claude review` does not produce one. - Restore the deleted rationale for `pull-requests: write` (#91) and for `--ref` on the dispatch. - State the inherited upstream defaults (report-cost, use-ai-config, lab-manual, require-review) instead of accepting them silently. - Document the exact accepted `/review` comment form. - Narrow the gha#437 and `secrets: inherit` claims to what each source says. Refs #101
Keeps the `with:` block carrying only deviations, per the same convention used in the claude.yml stub. All four values equal upstream's defaults. Refs #101
- setup-r: false, against gha's default. This repo's own
copilot-setup-steps.yml already recorded the same conclusion ("we don't
have any R code to run yet"), and it still holds: zero .qmd files contain
an R chunk and _quarto.yml is html-only. Enabling it would also run
`local::.` on every invocation -- an install nothing on main exercises,
against a DESCRIPTION with `LazyData: true` and no data/ directory -- and
a failure there happens before Claude runs, killing every @claude run.
- Document the tag-mode to agent-mode switch and the tool-surface narrowing
it brings. gha sets a `prompt:`, and its default claude-args REPLACES the
allowlist rather than extending it, denying git push, gh pr create, and
the write forms of gh api.
- Correct the `actions: write` comment: the review dispatch it enables only
resolves once #110 adds a workflow_dispatch trigger to
claude-code-review.yml.
- Correct the enumerated upstream defaults. `reviewer` defaults to
'd-morrison' rather than being unset, so d-morrison is re-requested on
every Claude push -- new behaviour worth naming.
- Carry across gha's WORKFLOW_TOKEN caveat (a PAT push fires other
push-based workflows) and its upside (it fires `synchronize`, so Claude's
commits would get reviewed even without the dispatch path).
- Note the retained `assigned` trigger's changed cost, the unconditional
TinyTeX install, and the omission of SUBMODULES_TOKEN.
- Restate the `secrets: inherit` rule as GitHub documents it (same
organization or enterprise).
Refs #100
This comment has been minimized.
This comment has been minimized.
|
Context for reviewers: this is one of a pair of migrations driven by a standing instruction to move repos onto Two things found while doing it that are worth reading alongside the diff:
Five gaps found upstream while diffing against gha and filed there: Morrison-Lab/gha#608, #609, #610, #612, #613. The last two are bugs in gha's own example stub for this workflow, both fixed consumer-side here. |
Self-review (fallback under
|
Fixes a blocking defect introduced by the previous round's own fix, and a factual claim I published in three places. - BLOCKING: the dispatch always passed `--ref "$PR_BRANCH"`, and a workflow_dispatch runs the workflow file as it exists at that ref. This PR is what ADDS the workflow_dispatch trigger, so every branch cut before it lacks it -- three of the four open same-repo PRs. Either the dispatch is rejected and `set -e` kills the step before the acknowledgment posts, or it runs a copy that selects no job and the acknowledgment posts anyway, reintroducing the false promise the fork short-circuit was added to prevent. Now tries the PR branch, falls back to a no-ref dispatch, and acknowledges only if one actually started a run -- reporting the failure on the PR otherwise. Removing upstream's REF_ARGS array last round had also removed the only no-ref path, which is what left a deleted head branch with no fallback. - FACTUAL: "it never executed once (5/5 runs skipped)" was wrong. That came from reading `--limit 5` and generalizing to the population. The real history is 25 runs: 23 skipped, one still in flight, and one success -- run 30687108541 on 2026-08-01, against fork PR #77, the exact case the workflow was written for. It executed and posted nothing because its prompt never carried `--comment` (#96), which #97 fixed for the other review workflow and never for this one. The justification for deleting it is that it was non-functional, not that it never ran. - FACTUAL: the user-facing fork notice blamed the OIDC exchange. That is the `pull_request`-path story (#90); a dispatched run executes in base-repo context with no fork token involved, and upstream describes its guard as failing a mistaken dispatch fast rather than closing a hole. - "produces no run at all" corrected -- a skipped run is still recorded. - `pull-requests: read` restated as declared rather than required, since a public repo would permit the read anyway. - The `pull-requests: write` rationale covered only the pull_request trigger; added what bounds the workflow_dispatch path. - Disambiguated "Set this to true", which sat five lines from its referent with `require-review` as the nearer antecedent, and split the three unrelated paragraphs sharing one heading. - report-cost's description corrected to "appends". Refs #101
This comment has been minimized.
This comment has been minimized.
Self-review, round 2 --- one blocking defect and a correction to my own published claimSecond adversarial round against Blocking: the dispatch would have failed on three of four open PRsRound 1 had me remove upstream's Both possible outcomes are bad, so it does not matter which GitHub applies: either the dispatch is rejected and Removing Fixed by trying the PR branch, falling back to a no-ref dispatch against the default branch, and acknowledging only if one of them actually started a run --- reporting the failure on the PR otherwise. Correction: "it never executed once" was wrong, and I published it three timesI wrote, here and in the PR body and a commit message, that the deleted The success is run 30687108541, 2026-08-01, That is #96 --- which #97 fixed for the other review workflow and never for this one. So the honest justification for deleting it is that it was exercised once and non-functional, not that it was never exercised. The conclusion is unchanged and arguably better supported; the evidence I gave for it was wrong, and it was the evidence offered for dropping fork-review coverage without a replacement. The PR body and the workflow comment now say the accurate thing. Also corrected
Verified clean this roundShell semantics of the fork short-circuit ( One regression check I ran unprompted: deleting the StatusNo known open findings. Still blocked on an external verdict rather than ready --- the self-modification guard means no automated reviewer can weigh in until this merges, and Copilot has not answered. |
|
Warning No review ran --- this PR edits
|
1 participant
Closes #101
Replaces this repo's two hand-rolled PR-review workflows with a single caller stub for
Morrison-Lab/gha's reusableclaude-code-review.yml@v2.claude-code-review.yml+claude-review-comment.ymlclaude-code-review.ymlonlyanthropics/claude-code-action@v1directly, twiceMorrison-Lab/gha/.github/workflows/claude-code-review.yml@v2claude_argsallowlistWhat is preserved
pull_requesttrigger, and the reason for it. The old file's load-bearing comment --- do not switch topull_request_target, because Anthropic's token-exchange endpoint rejects OIDC tokens minted for that event (OIDC token exchange rejects pull_request_target events anthropics/claude-code-action#713) --- is carried forward verbatim in the new stub, with the same issue link. This repo learned that the hard way across Fix claude-review workflow: switch to pull_request_target for fork PR support #83 / Revert claude-review to the pull_request trigger #89; discarding the reasoning would have reintroduced it.[opened, synchronize, ready_for_review, reopened], unchanged.pull-requests: write. The old file's comment explains it is safe underpull_requestbecause GitHub forces a read-only token for fork PRs regardless. Still true, still granted./reviewrather than@claude review(see below).What changes
/reviewreplaces@claude reviewas the comment trigger. Deliberate, and gha documents why: any@claudesubstring also wakesclaude.yml, so a mention would dispatch two paid review runs of possibly the same PR. TheOWNER/MEMBER/COLLABORATORgate is unchanged.workflow_dispatchis added. Required soclaude.ymlcan re-dispatch a review of commits an@clauderun pushes. This is what creates the merge-order constraint with Migrate claude.yml (@claude agent) to Morrison-Lab/gha's claude.yml@v2 #111.concurrency:block. The reusable workflow manages per-PR concurrency internally (group: claude-review-<PR>); a PR-scoped group in the caller deadlocks against the nested job and cancels the run (gha#437).secrets: inherit. GitHub only inherits secrets into a reusable workflow owned by the same org/user, and this is a UCD-SERG repo calling a Morrison-Lab workflow ---inheritwould hand it an empty token.What the reusable workflow does NOT cover
Fork PRs are still not reviewed. This does not close #90.
I checked both paths in the reusable workflow rather than assuming:
gha blocks forks on both, and states the reason: forks cannot read the OAuth token.
The deleted
claude-review-comment.ymlwas this repo's attempt at a fork path --- anissue_commenttrigger checking outrefs/pull/<n>/headin base-repo context. Removing it is a capability loss on paper. In practice it is not, because it was exercised once and was non-functional:The one success is run 30687108541, 2026-08-01,
event: issue_comment--- on fork PR #77, the exact case the workflow was written for. It ran every step to completion and posted nothing, because its prompt never carried--comment:That is #96 --- fixed for the other review workflow by #97, and never for this one. So the path was tried on a real fork PR and could not have posted anything, which is consistent with #98 saying it has never been confirmed to post a review end-to-end. This trades a non-functional path for a maintained one, and #90 and #98 stay open on their own terms.
/reviewon a fork PR now says so, rather than promising a review. An adversarial self-review of this diff caught that the stub (copied from gha's example) detected the fork, dispatched anyway, and posted:mag: dispatched a Claude review of this PR. The dispatch guard then blocks the run, and both jobs skip rather than fail --- so the PR would carry a promise, no review, and no red check to contradict it. The stub now short-circuits with an explicit fork notice linking #90. Filed upstream as Morrison-Lab/gha#613, since gha's own example stub has the same bug.The dispatch itself is now failure-aware, after round 2 found that the round-1 fix broke it. Removing gha's
REF_ARGSfallback left the dispatch always passing--ref "$PR_BRANCH"--- and aworkflow_dispatchruns the workflow file as it exists at that ref. This PR is what adds theworkflow_dispatchtrigger, so three of the four open same-repo PR branches do not have it. Either the dispatch is rejected andset -euo pipefailkills the step before the acknowledgment posts, or it runs a copy that matches no job and the acknowledgment posts anyway --- the same false promise, through a different door. It now tries the PR branch, falls back to a no-ref dispatch, and acknowledges only if one actually started a run. Details in the round-2 self-review comment.Three new skip gates arrive with the reusable workflow, and the trigger list being unchanged hides them. The hand-rolled workflow had no job-level
if:at all; the reusable one skips draft PRs, fork PRs, and bot-sender events. The third turns #84 (Copilot-authored PRs 401 on token exchange) from a red failure into a gray skip --- better signal, not a fix. All three are documented in the file; #84 stays open.On #105 (
claude-reviewhas never posted a review)Two things, and I want to be careful to separate the one I verified from the one I did not.
Verified: the root cause named in #96/#105 cannot recur under this workflow. #105's root cause was that the upstream
code-reviewplugin's step 7 gates its owngh pr commenton a--commentargument the workflow had to remember to pass. gha's reusable workflow does not rely on the plugin to post at all --- it captures the review to a file and posts it itself from the workflow:COMMENT_URL="$(gh pr comment "$PR_NUMBER" --repo "$REPO" --body-file "$BODY_FILE")"So the "plugin decided not to post" failure mode is structurally absent, not merely fixed.
Not verified: whether a review will actually appear. #105's most recent comment (2026-08-24) documents runs that completed with
is_error: false, 4--6 turns, real cost, and posted nothing --- and names two remaining candidate causes (the plugin's step-1 eligibility gate, or a denied tool call) that it could not separate. A different posting mechanism removes one of those candidates. It does not establish that the other is gone. This PR does not close #105, and the test is the same one #105 already describes: push to an open PR that does not touch this workflow file, and read the result.What it does add is the lever #105 asks for. Its "concrete next step" is:
The reusable workflow exposes exactly that as an input, described upstream for exactly this symptom. It is wired in this stub at
show-full-output: false(gha's default), with a comment saying when to flip it. That turns #105's next step from a workflow edit into a one-word change.Why no automated review ran on this PR
The self-modification guard, and only that. This PR edits
.github/workflows/claude-code-review.yml, andclaude-code-actionrequires that file to match the default branch or its token exchange fails. The bot said so itself at 16:38:30Z:So the skip is by design, it lifts only on merge, and re-running cannot clear it. The warning prescribes the remedy directly: merge on a self-review or a human review.
This is not #105, despite the shared symptom. #105 is a different failure --- a review that genuinely executes and posts nothing. It did not apply here, because no review executed at all. A clean reproduction of #105 did land during this work, on #111, which touches no workflow file and so is not subject to this guard; it is written up on #105. Keeping the two apart matters, since they call for opposite responses: this one resolves on merge, and #105 does not.
What gha improves here is legibility rather than outcome. The warning above, and
require-reviewreporting gray instead of green, are both new. Under the hand-rolled workflow this case was a silent green, which is what made #89, #91 and #97 hard to read at the time --- #105 lists it as one of the three distinct meanings a greenclaude-reviewcheck currently carries.check-pr-fully-clean.pyreports this PR clean, and that report is wrong. It exits 0 while printing:latest = NONEmeans nothing reviewed this head. The one item it examined is the self-modification skip notice, which is admitted as a review item and states no verdict --- exactly the false-clean that line exists to expose. #111 reports the same underlying state honestly, exiting 1 with "No automated review comments or reviews found".This PR is therefore blocked on an external verdict, not ready. Per
self-review-fallback, the fallback is an adversarial self-review (posted separately on this PR), with a cross-vendor reviewer requested in parallel. A human approval is required to merge regardless: themainruleset requires 1 approving review,require_last_push_approval, and thread resolution.Merge order
Merge this before #111. gha's
claude.ymldispatches the review workflow viaworkflow_dispatch, and the current bespokeclaude-code-review.ymlhas no such trigger, so #111's dispatch would fail until this lands. The two PRs touch disjoint files (gh pr diff --name-onlyon each: this one isclaude-code-review.yml+claude-review-comment.yml, #111 isclaude.yml), so there is no conflict --- only an ordering constraint.Other self-review fixes
pull-requests: readadded todispatch-on-comment. It callsgh api repos/../pulls/N, and declaring any permission scope zeroes every unlisted one. gha's own example has the same gap; filed as examples/claude-code-review.yml: dispatch-on-comment reads a PR without declaring pull-requests: read Morrison-Lab/gha#612.claude.ymlcannot currently dispatch a review (it holdsactions: read), and@claude reviewdoes not currently produce one.pull-requests: writeis safe underpull_request(Restore pull-requests: write for claude-review #91), and what--refon the dispatch is for./reviewform --- it must be the first characters of the comment, which is narrower than the deleted workflow's match-anywhere behaviour.secrets: inheritbeing same-organization-or-enterprise).Validation
actionlintandyamllintclean on the new file, and it is ASCII-only.Note that
workflow_dispatchand the/reviewpath only become usable once this file is onmain---gh workflow runresolves against the default branch. Thepull_requestpath works from the PR branch immediately.Related
Same class of gha-caller defect, found from a separate probe today: UCD-SERG/serocalculator#663 and UCD-SERG/serodynamics#297, both gha callers that fail to forward
WORKFLOW_TOKEN. This PR is unaffected --- no gha review workflow references that secret, and the reviewer never pushes, so omitting it here is correct.The standing rule authorizing this migration is Morrison-Lab/ai-config#2127, merged 2026-08-24 at 16:42Z.