[Fix] Review public fork pull requests without fork access

[roomote-roomote]

Opened on behalf of Matt Rubens. Follow up by mentioning @roomote-roomote, in the web UI, or in Slack.

What changed

• Updated GitHub fork review fetches to force-refresh the local PR ref after contributor force-pushes.
• Revalidated the PR head once when the fetched ref and API head differ, avoiding blockers caused by a short GitHub propagation race.
• Preserved an explicit blocker path for unsupported cross-repository providers and expanded coverage across all review variants.

Why this change was made

Upstream GitHub App credentials may not read a fork directly, but GitHub serves the pull request head from the upstream repository. The workflow needs to follow that ref safely even when it was previously fetched or is briefly out of sync with the API.

Impact

Automated GitHub reviews can continue on public fork PRs after force-pushes without requesting extra credentials, while other providers retain a clear safe fallback.

[roomote-roomote]

No new code issues found. See task

_{Reviewed 14fab11}

[mrubens]

Overview

This rewrites the PR-checkout and diff-fetch instructions in review-code/SKILL.md across the four GitHub review appendices (review-github-pr, review-github-pr-with-approval, sync-github-pr-review, sync-github-pr-review-with-approval) so cross-repo PRs are fetched via the upstream refs/pull/<N>/head ref, verified against the API-reported head SHA, then checked out detached. Tests assert the new text in all four appendices.

The approach is right: refs/pull/<N>/head is served by the upstream repo, so the existing installation token suffices, and the rev-parse-vs-headSha check is a genuine safety property (it pins what gets reviewed to what the API says the head is). Detached checkout is correct for a read-only workflow.

Issues

1. Missing + on the fetch refspec (most likely real failure mode)

git fetch origin 'refs/pull/<N>/head:refs/remotes/origin/pr-<N>-head' refuses non-fast-forward updates to an existing local ref. If refs/remotes/origin/pr-<N>-head already exists in the workspace (snapshot reuse, or a re-run in a warm sandbox) and the contributor force-pushed the fork, which is the common case for a sync review, the fetch is rejected and the instruction then tells the agent to "report the blocker." That is a false blocker on exactly the flow this PR exists to fix. One character:

git fetch origin '+refs/pull/<PR_NUMBER>/head:refs/remotes/origin/pr-<PR_NUMBER>-head'

The SHA verification step already guards correctness, so forcing here loses nothing.

2. Non-GitHub fork PRs lost their instruction

The old sentence was provider-agnostic: "For cross-repository (fork) PRs whose source branch cannot be fetched with task credentials, report that blocker." The replacement handles same-repo and GitHub cross-repo. isCrossRepository is populated for GitLab, Bitbucket, Gitea, and Azure DevOps as well (see packages/sdk/src/server/lib/pull-requests/source-control-pull-request-reads.ts), and none of those serve refs/pull/<N>/head. A GitLab fork MR now falls through to the same-repo branch fetch with no stated fallback.

Suggest appending: "For a cross-repository PR on any other provider whose source branch cannot be fetched with task credentials, report that blocker instead of improvising credentials." (GitLab's equivalent is refs/merge-requests/<iid>/head if that is worth covering properly later.)

3. SHA mismatch is treated as terminal, but it has a benign race

refs/pull/<N>/head can lag a push by a short interval, and a push can land between get_pull_request and the fetch. Both produce a mismatch that is not a real blocker. Consider: on mismatch, re-call get_pull_request once, and report the blocker only if the SHAs still disagree. Otherwise sync reviews triggered by a fresh push will occasionally hard-fail for no reason.

Test coverage

Good: the appendix loop pins the checkout sentence in all four variants, so a partial revert fails.

Gap: the diff-fetch sentence is only asserted with expect(skillContent).toContain(...) against the whole file. That sentence appears in both initial-review appendices, so a regression in one still passes. Worth a second loop over ['review-github-pr', 'review-github-pr-with-approval'] asserting it per-appendix, matching the pattern introduced here.

Nits

• Placeholder style: the appendices define the variable as [PR_NUMBER] (see the "Determine the repository full name ... and [PR_NUMBER]" action), but the new text uses <PR_NUMBER>. <sourceBranch>/<headSha> justify angle brackets for git args, so it is defensible, but [PR_NUMBER] is the named variable and the mismatch is one more thing for the model to resolve.
• Out of scope but adjacent: packages/cloud-agents/src/server/workflows/skills/standard/fix-pr/SKILL.md (line 64) still carries the old blocker-only text. That one arguably should stay, since fix-pr needs write access to the fork, which this technique does not provide. A short clarifying note there would stop a future reader from "fixing" it by copying this pattern into a workflow that then cannot push.

Risk

Prompt-only, so there is no enforcement. Compliance depends on the model following a fairly long conditional instruction, and no test exercises a real fork PR end to end. The failure mode is graceful (blocker reported, no bad review posted), and the SHA check prevents reviewing the wrong tree. Nothing here needs to block merge except, in my view, item 1, which is a one-character change.

[roomote-roomote]

Addressed the requested feedback in 14fab11: fork refs now force-refresh, one API recheck absorbs short SHA races, non-GitHub fork handling remains explicit, and the targeted coverage verifies every affected review path.