Skip to content

claude-code-review's workflow_dispatch re-review posts a successful require-review check tied to the wrong commit SHA - #286

Merged
d-morrison merged 9 commits into
mainfrom
claude/issue-285-20260724-011755
Jul 24, 2026
Merged

claude-code-review's workflow_dispatch re-review posts a successful require-review check tied to the wrong commit SHA#286
d-morrison merged 9 commits into
mainfrom
claude/issue-285-20260724-011755

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

PR opened by @claude to address #285.

Triggered by workflow run.

Addresses #285.

Copy link
Copy Markdown
Collaborator

Working on this --- paws off until I'm done.

(The @claude agent run that opened this draft committed the actual fix locally but failed to push: refusing to allow a GitHub App to create or update workflow .github/workflows/claude-review.yml without workflows permission. Re-implementing and pushing from this session instead.)


Generated by Claude Code

claude added 2 commits July 24, 2026 16:02
gh workflow run's default ref (the repo's default branch) means every
workflow_dispatch re-review claude.yml/claude-review.yml triggers gets its
check-runs attributed to main's tip, not the PR's actual head commit --- so
a genuinely clean, current review can post while the PR's own
required-status-check view stays red, because the passing check landed on
the wrong SHA and never supersedes a stale cancelled run.

Pass --ref explicitly at every gh workflow run call site, using the PR
branch already resolved by the pr_checkout/issue_branch steps (or a fresh
API lookup in the two comment-triggered dispatch jobs that never check out
the repo). Addresses #285.
@d-morrison
d-morrison marked this pull request as ready for review July 24, 2026 16:04
Copilot AI review requested due to automatic review settings July 24, 2026 16:04
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@d-morrison
d-morrison requested a review from Copilot July 24, 2026 16:24
@d-morrison

Copy link
Copy Markdown
Collaborator

@claude review

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@github-actions

Copy link
Copy Markdown
Contributor Author

👀 Picked up by workflow run #30109028583. Setup runs first; Claude itself responds after that.

@github-actions
github-actions Bot requested a review from d-morrison July 24, 2026 16:28
@github-actions

Copy link
Copy Markdown
Contributor Author

💰 Cost: $0.6025288

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

.github/workflows/claude.yml:928

  • This step also runs under always(), but relies on PR_BRANCH coming from steps.pr_checkout.outputs.branch. If the checkout step failed earlier, PR_BRANCH will be empty and this dispatch attempt will still run. Guard against an empty PR_BRANCH before calling gh workflow run so the step degrades cleanly when checkout failed.
                  '[.[] | select((((.created_at // .submitted_at) // "") > $ts) and ((.body // "") | test("@claude review")) and ((.user.type // "") != "Bot"))] | length' ) || late=0
          if [ "${late:-0}" -gt 0 ]; then
            echo "Found $late late @claude review request(s) newer than $TRIGGER_TS; dispatching review."
            # --ref: see the comment on the earlier dispatch step above (#285).
            gh workflow run "$REVIEW_WF" --ref "$PR_BRANCH" -f pr_number="$PR_NUMBER" \
              || echo "::warning::Could not dispatch $REVIEW_WF for the late review request."

.github/workflows/claude.yml:959

  • Because this step is always() it may run even when the PR checkout failed; in that case PR_BRANCH is empty but the workflow still attempts a dispatch with --ref "$PR_BRANCH". Add a guard to skip the dispatch when PR_BRANCH is empty so the review re-dispatch can’t run against an unintended ref.
          PR_NUMBER="${{ github.event.pull_request.number || github.event.issue.number }}"
          echo "before=$SHA_BEFORE  after=$SHA_AFTER"
          if [ -n "$SHA_AFTER" ] && [ "$SHA_AFTER" != "$SHA_BEFORE" ]; then
            echo "Claude pushed new commits; re-requesting reviewer and dispatching code review."
            if [ -n "$REVIEWER" ]; then
              gh api -X POST "repos/${{ github.repository }}/pulls/$PR_NUMBER/requested_reviewers" -f "reviewers[]=$REVIEWER" \
                || echo "::warning::Could not re-request $REVIEWER as reviewer."
            fi
            # --ref: see the comment on the earlier dispatch step above (#285).
            gh workflow run "$REVIEW_WF" --ref "$PR_BRANCH" -f pr_number="$PR_NUMBER" \
              || echo "::warning::Could not dispatch $REVIEW_WF."

Comment thread .github/workflows/claude.yml Outdated
claude and others added 4 commits July 24, 2026 16:48
Copilot review on #286: the three PR-context dispatch steps read
PR_BRANCH from steps.pr_checkout.outputs.branch but run under always(),
so a failed "Checkout PR branch" step leaves PR_BRANCH empty while the
step still executes. Guard each dispatch so an empty PR_BRANCH skips
with a warning instead of calling `gh workflow run --ref ""`.
Self-review finding: the new changelog.d fragment used a raw em-dash
(U+2014), which this repo's ascii-punctuation-in-source convention bans
in all tracked source files, .md included, regardless of CI coverage.

Copy link
Copy Markdown
Collaborator

Self-review (the automated claude-code-review reviewer can't run on this PR — it edits its own dispatch workflow, so the Skip self-review when the PR edits this workflow guard in claude-code-review.yml intentionally skips it until merge; require-review stays green because it treats that skip as passing).

Findings:

  1. Fixed: the new changelog.d/fix-review-dispatch-wrong-sha.fixed.md fragment had a raw em-dash (U+2014), which this repo's ascii-punctuation-in-source convention bans in all tracked source, .md included. Replaced with -- in 89d7f8d.
  2. Accepted as-is: the PR_BRANCH=$(gh api ... --jq '.head.ref') lookup is duplicated between claude-review.yml and its examples/claude-code-review.yml mirror. Treating this as expected wrapper/example-stub duplication (per this repo's own convention of keeping the two in sync) rather than extracting a new composite action for one gh api line.
  3. Deferred: filed Review-dispatch --ref lookup can hard-fail for a fork-originated PR #289.head.ref has no owner prefix, so the new --ref would fail to resolve for a fork-originated PR (hard-failing claude-review.yml's dispatch-on-comment job specifically, since that call site has no || echo "::warning::..." fallback). Out of scope for this PR; claude-code-review's workflow_dispatch re-review posts a successful require-review check tied to the wrong commit SHA #286's fix is correct for the common same-repo case regardless.

Verified the --ref fix itself works empirically: this PR's own dispatched review run (30109069032) shows head_sha correctly matching the PR's actual head commit, not main's tip.


Generated by Claude Code

@d-morrison
d-morrison merged commit 71e9de9 into main Jul 24, 2026
26 checks passed
@d-morrison
d-morrison deleted the claude/issue-285-20260724-011755 branch July 24, 2026 16:57
d-morrison pushed a commit that referenced this pull request Jul 24, 2026
The @claude agent's own attempt at this fix (workflow run 30110560206)
implemented the change but failed to push -- GitHub rejected a GitHub-App
token update to .github/workflows/claude-review.yml without the
workflows permission. Re-implementing and pushing from a session with
adequate credentials, per the same recovery pattern used for #286.

Addresses #289: .head.ref has no owner prefix, so for a fork-originated
PR it names a branch that doesn't exist in this repo. gh workflow run
--ref would fail to resolve it there, hard-failing claude-review.yml's
/review dispatch job (no warning fallback on that call site) and
degrading the other three claude.yml call sites to a lost dispatch.
Each dispatch site now compares the PR's head repo against the base
repo and omits --ref for a fork PR, falling back to the pre-#285
wrong-SHA-attribution behavior rather than failing the dispatch.
d-morrison added a commit to Morrison-Lab/ai-config that referenced this pull request Jul 24, 2026
…edoing

Learned on Morrison-Lab/gha#286, 2026-07-24: a webhook-delivered review
reply claimed a fix was already pushed, in a session that hadn't made
it. Verified real (a second live session, not injection) by
cross-checking the PR's actual commit list before either trusting the
claim or duplicating the work.
d-morrison pushed a commit that referenced this pull request Jul 24, 2026
…290)

* Start @claude work on #289

* fix: skip --ref for fork-originated PRs in review dispatch

The @claude agent's own attempt at this fix (workflow run 30110560206)
implemented the change but failed to push -- GitHub rejected a GitHub-App
token update to .github/workflows/claude-review.yml without the
workflows permission. Re-implementing and pushing from a session with
adequate credentials, per the same recovery pattern used for #286.

Addresses #289: .head.ref has no owner prefix, so for a fork-originated
PR it names a branch that doesn't exist in this repo. gh workflow run
--ref would fail to resolve it there, hard-failing claude-review.yml's
/review dispatch job (no warning fallback on that call site) and
degrading the other three claude.yml call sites to a lost dispatch.
Each dispatch site now compares the PR's head repo against the base
repo and omits --ref for a fork PR, falling back to the pre-#285
wrong-SHA-attribution behavior rather than failing the dispatch.

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>
d-morrison added a commit to Morrison-Lab/ai-config that referenced this pull request Jul 24, 2026
…edoing (#676)

Learned on Morrison-Lab/gha#286, 2026-07-24: a webhook-delivered review
reply claimed a fix was already pushed, in a session that hadn't made
it. Verified real (a second live session, not injection) by
cross-checking the PR's actual commit list before either trusting the
claim or duplicating the work.
d-morrison added a commit that referenced this pull request Jul 24, 2026
…cause (#294)

* docs: note the in-workflow self-mod guard as a third self-verify-gap cause

Complements #291's @v2-floating-tag explanation with the mechanism this
session actually hit on #286: claude-code-review.yml's own 'Skip
self-review when the PR edits this workflow' step, which skips every
downstream step (not just runs a stale pre-fix version) when the PR
touches the caller review workflow's own file. Both claude-review /
require-review report success with no verdict ever posted -- easy to
mistake for a real review unless the job's step list (not just its
conclusion) is checked.

* Fix overstated file list in self-mod-guard note

Review finding on #294: the guard checks exactly one path
(github.workflow_ref, the caller's own stub -- claude-review.yml in
this repo's dogfooding setup), not all three files previously listed.
claude.yml has no analogous self-mod check at all; examples/claude-
code-review.yml never executes as a workflow here, so github.workflow_ref
can't resolve to it either. Clarify scope and correct the trigger
condition.

---------

Co-authored-by: Claude <noreply@anthropic.com>
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.

3 participants