fix(ci): review the PR head, not the default branch

[thecodedrift]

What you saw

Nothing. Every on-demand @claude /review finished green and posted a plausible-looking review.

What was actually happening

.github/workflows/claude-code-review-on-demand.yml checked out with no ref::

- name: Checkout repository
  uses: actions/checkout@v4
  with:
    fetch-depth: 1

Neither issue_comment nor pull_request_review_comment is a PR event, so actions/checkout falls back to the default branch. Confirmed in the log for job 96140869912: git checkout --progress --force -B main refs/remotes/origin/main, with head_branch: main and head_sha: 5596e390. The reviewer was reading main while being told it was reviewing a PR.

The git hash-object stack traces in #118 are cosmetic — upstream catches the failure, warns, sets sha: "unknown", and the job concludes success. That is why nothing ever went red.

The real damage is silent degradation. Across the four PRs reviewed at 16:18 on 2026-08-19 — every one checked out main:

PR	Files invisible to its own review
#113	changeset only — review sound
#114	both .github/scripts/openspec-visibility.cjs and its test — the entire new script
#117	packages/cli/test/ast-grep-vendor-contract.test.ts — the ~500-line contract that is the PR
#115	25 files — the whole renamed tree

#115's posted review claimed it "spot-checked every topic name against real files in packages/cli/src/agent/" — a directory that does not exist on main. A review asserting verification it could not have performed is worse than no review.

The fix

Resolve the PR number before checkout and pin the checkout to it:

- name: Checkout PR head
  uses: actions/checkout@v4
  with:
    ref: refs/pull/${{ steps.prep.outputs.pr }}/head
    fetch-depth: 1

The Prepare review context step already branches on github.event_name because the PR number lives in issue.number on issue_comment and pull_request.number on pull_request_review_comment, each absent on the other event. Reusing it makes the reviewed tree and the PR NUMBER: in the prompt a single source of truth, rather than duplicating a || fallback over a null.

/head, not /merge. /head is the tree the author pushed; it matches what gh pr diff returns and what the inline-comment line anchors are computed against, so a comment lands on the line the reviewer read. /merge would also fail to exist on a conflicted PR — and a conflicted PR is exactly when you still want a review.

fetch-depth: 1 is kept: the prompt forbids running the project's build/lint/test, and every allowed tool reads the diff through gh (the API), not through local history.

Security properties are unchanged — no contents: write, same maintainer-only author_association gate, same restricted --allowedTools, still issue_comment/pull_request_review_comment and not pull_request_target, no permissions added.

The header comment now records which tree gets reviewed and why, alongside the security model it already documented. This bug was invisible for as long as it was because nothing in the file said what the checkout was for.

Verification

Verified: the file parses as YAML and passes Prettier; the run log for the successful issue_comment run 32275202719 shows the rendered script as echo "pr=117" / echo "pr=", i.e. github.event.pull_request.number is empty on issue_comment and issue.number is populated; pull_request_review_comment runs carry the PR's own head_branch (e.g. run 32291103997 → openspec/nightly-1-workflow-split), and GitHub's webhook payload reference confirms that event carries a top-level pull_request and no issue.

Reasoned, not executed: the workflow itself cannot be run from this branch, so the corrected checkout is confirmed by ref semantics and the payload evidence above rather than by a green run. The first real invocation after merge is the actual proof.

No changeset — this changes no published package. Labeled skip-changeset.

Fixes #118

[Copilot]

Pull request overview

Updates the on-demand Claude review workflow to ensure it checks out and reviews the pull request head ref (instead of unintentionally reviewing the default branch) when triggered by comment-based events.

Changes:

• Resolve the PR number in a prep step based on the triggering event payload.
• Pin actions/checkout to refs/pull/<pr>/head to ensure the reviewed working tree matches the PR being reviewed.
• Expand the workflow header comment to document the checkout rationale and constraints.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[thecodedrift]

@claude /review

[claude]

Claude finished @thecodedrift's task in 3m 54s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude finished @thecodedrift's task in 3m 15s —— View job

---

Review: fix(ci): review the PR head, not the default branch

• Read the diff and the full workflow file for context
• Traced the prep → checkout → claude-code-action step ordering and the pr value's provenance for both trigger events
• Checked for correctness bugs (bad ref interpolation, wrong event field, ordering hazards)
• Checked security implications of the changed checkout behavior
• Posted an inline comment with the one substantive finding

Overall assessment

This is a correct, well-reasoned fix for the actual bug (checkout silently falling back to main on non-PR trigger events). Specifically verified:

• Ref resolution is correct. issue_comment only reaches this job when github.event.issue.pull_request is truthy (see the job-level if: on lines 38–44), so github.event.issue.number is always populated when used. pull_request_review_comment is PR-native, so github.event.pull_request.number is always populated too. No path produces an empty pr output that would turn into a broken refs/pull//head.
• Step ordering is safe. Prepare review context (lines 58–71) only reads github.event.* context expressions and shell-echoes them — it does no git operations, so running it before the new checkout step is fine.
• refs/pull/N/head vs /merge reasoning holds up. /head matches what gh pr diff/gh pr view return and what inline-comment line anchors are computed against, and it still exists on a conflicted PR where /merge would not.
• fetch-depth: 1 is still adequate — nothing downstream reads git history; --allowedTools (line 109) restricts Claude to gh pr comment/diff/view plus the inline-comment MCP tool, all of which go through the API rather than local history.
• No test suite/CI harness exists for these workflow files in this repo, so the PR body's "reasoned, not executed" verification approach (ref semantics + payload evidence from real run logs) is the appropriate substitute; not treating that as a gap.

One finding posted inline (line 80): checking out the PR head is now checking out potentially attacker-influenced content (any PR a maintainer comments @claude /review on, including forks), and the new actions/checkout step doesn't set persist-credentials: false. The job carries pull-requests: write / issues: write / id-token: write and a real OAuth secret, so leaving credential persistence at its default is an easy one-line hardening gap now that the checkout target actually matters. Mitigated by the tight --allowedTools allowlist (no general Bash/file-read tool for the model), but that leaves the guarantee resting on claude-code-action's internals never touching .git/config, which this diff doesn't establish. Low severity, cheap fix.

Everything else — the header comment, the contents: read-is-enough note, the /head-not-/merge reasoning, the fetch-depth: 1 justification — matches the actual workflow behavior and is accurate.