fix(github-code-review): pin PR review to head commit, fix posting auth, fix trivial issues

detain · claude · detain · commit 32f5530b34c5 · 2026-07-17T20:45:25.000-04:00
- Pin the normal PR review to a specific head commit instead of the live
  `gh pr diff`: resolvePrHeadSha() (job sha, else API), setupPrHeadBaseline()
  (merge-base from the compare API -&gt; setupCommitRangeBaseline(_base=mergeBase,
  HEAD=headSha)). `git diff _base HEAD` now reproduces the PR's Files-changed
  view at that exact commit, and the posted commit_id matches what was reviewed.
  Falls back to the existing getPRDiff+applyPRDiff+commitPrSnapshot flow when the
  SHA/merge-base can't be fetched. Push and commit-scoped paths unchanged.

- Fix comment posting 401 "Bad credentials": posting uses raw curl with an
  explicit token, but only GITHUB_TOKEN was honored. Now resolve
  GITHUB_TOKEN -&gt; GH_TOKEN -&gt; `gh auth token` (gh's own credential, which reads
  already rely on), so `gh auth login` alone is enough to post.

- review.txt: default to FIXING trivial, unambiguous issues (typos, misspelled
  literals/constants, obvious wrong values) in place instead of flagging them;
  reserve FLAGGED-only for changes needing human judgment.

- New PrHeadBaselineTest (network-free, real throwaway repos) covers the pinned
  baseline + agent-fix isolation, job-sha preference, and the fallback trigger.

Co-Authored-By: Claude Opus 4.8 &lt;noreply@anthropic.com&gt;
diff --git a/scripts/github-code-review.php b/scripts/github-code-review.php
@@ -9,8 +9,11 @@
  * with `_base` and HEAD set so that `git diff _base HEAD` is exactly the change
  * under review and a later plain `git diff` is exactly the agent's in-place fixes:
  *
- *   - kind=pr (no options.commit): clone the PR base branch, apply the PR diff,
- *     and commit it as a snapshot (HEAD = base+PR, _base = base commit).
+ *   - kind=pr (no options.commit): clone the PR base branch and pin the review to
+ *     the PR head commit — _base = GitHub's 3-dot merge-base, HEAD = head SHA — so
+ *     a later synchronize push or a moving base branch cannot shift the tree under
+ *     review. Falls back to applying the live PR diff as a snapshot (HEAD =
+ *     base+PR, _base = base commit) only when the head SHA cannot be pinned.
  *   - kind=pr WITH options.commit=SHA: review just that one commit
  *     (_base = SHA^, HEAD = SHA); results still post to the PR.
  *   - kind=push: review a before..after commit range with no PR; the commits
@@ -696,6 +699,46 @@ function getPRBaseBranch(string $repo, int $prNumber): ?string
     return getPRRef($repo, $prNumber, 'base');
 }
 
+/**
+ * Resolve the head commit SHA to review for a PR job. Prefers a non-empty
+ * $job['sha'] (the pr.head.sha captured at enqueue time) so no network call is
+ * needed in the common case; otherwise it looks the SHA up from the GitHub API.
+ * Returns '' when the SHA cannot be resolved (the caller then falls back to the
+ * live diff-apply path).
+ *
+ * @param array<string, mixed> $job
+ */
+function resolvePrHeadSha(array $job, string $repo, int $prNumber): string
+{
+    $sha = trim((string)($job['sha'] ?? ''));
+    if ($sha !== '') {
+        return $sha;
+    }
+
+    // No SHA on the job envelope — look it up. Skip the API entirely when we
+    // have nothing to query with (keeps callers/tests network-free).
+    if ($repo === '' || $prNumber <= 0) {
+        return '';
+    }
+
+    $cmd = sprintf(
+        'gh api %s --jq .head.sha 2>&1',
+        escapeshellarg("repos/{$repo}/pulls/{$prNumber}")
+    );
+    $output = [];
+    $ret = 0;
+    exec($cmd, $output, $ret);
+    verbose_raw('gh_api_pr_head_sha', "cmd: {$cmd}\noutput: " . implode("\n", $output) . "\nret: {$ret}");
+    if ($ret === 0 && !empty($output)) {
+        $resolved = trim(implode("\n", $output));
+        // Guard against error text / non-SHA output leaking through 2>&1.
+        if (preg_match('/^[0-9a-f]{7,40}$/i', $resolved) === 1) {
+            return $resolved;
+        }
+    }
+    return '';
+}
+
 /**
  * Get the diff for a PR using gh pr diff
  *
@@ -820,7 +863,10 @@ function applyPRDiff(string $checkoutPath, ?string $diff): bool
 // root commit that has no parent (brand-new branch's very first commit).
 const EMPTY_TREE_SHA = '4b825dc642cb6eb9a060e54bf8d69288fbee4904';
 
-$githubToken = getenv(GITHUB_TOKEN) ?: '';
+// Comment posting uses raw curl with an explicit token (reads go through the
+// gh CLI's own auth). Honor GITHUB_TOKEN, then GH_TOKEN, then (in the direct-run
+// block below) fall back to whatever `gh` is authenticated with.
+$githubToken = getenv(GITHUB_TOKEN) ?: (getenv('GH_TOKEN') ?: '');
 $ghToken = getenv('GH_TOKEN') ?: '';
 $checkoutRoot = getenv(CHECKOUT_ROOT) ?: '/tmp/pr-checkouts';
 $opencodeAnalyzeCmd = getenv(OPENCODE_ANALYZE_CMD) ?: 'sh -c "cd {dir} && opencode run \"Analyze all PHP files in this directory tree. Find issues and return JSON array with fields: file, line, severity (error/warning/info), message for each issue\" --format json" 2>&1';
@@ -833,6 +879,16 @@ function applyPRDiff(string $checkoutPath, ?string $diff): bool
         exit(1);
     }
 
+    // No token was exported, but `gh` is authenticated (checked above) — reuse
+    // its credential so posting works with `gh auth login` alone instead of
+    // 401ing with "Bad credentials".
+    if ($githubToken === '') {
+        $ghAuthToken = trim((string)shell_exec('gh auth token 2>/dev/null'));
+        if ($ghAuthToken !== '') {
+            $githubToken = $ghAuthToken;
+        }
+    }
+
     // Pass tokens to child processes if set
     if ($githubToken !== '') {
         putenv("GITHUB_TOKEN={$githubToken}");
@@ -1145,25 +1201,48 @@ function processPrJob(array $job, string $repo, string $jobId, array $auditTypes
             // Anchor inline comments on the reviewed commit.
             $sha = $commitSha;
         } else {
-            // Get the PR diff, apply it, and commit it as a snapshot. After a
-            // successful apply the working tree holds the PR changes on top of
-            // the base commit; we record the base commit as `_base` and commit
-            // the PR changes so HEAD = base+PR with a clean tree.
-            $diff = getPRDiff($repo, $prNumber);
-            if ($diff !== null && $diff !== '') {
-                verbose_log("applying PR diff for {$repo}#{$prNumber}", 2);
-                if (applyPRDiff($checkoutPath, $diff)) {
-                    if (commitPrSnapshot($checkoutPath, $prNumber)) {
-                        $baselineReady = true;
-                        verbose_log("PR snapshot committed (HEAD=base+PR, _base=base)", 2);
+            // Pin the review to a specific head commit (mirrors the push path)
+            // so a later synchronize push or a base branch that moves between
+            // enqueue and processing cannot make us review the wrong tree while
+            // anchoring comments to a stale SHA. Resolve the head SHA, then pin
+            // _base = GitHub's 3-dot merge-base and HEAD = head SHA.
+            $headSha = resolvePrHeadSha($job, $repo, $prNumber);
+            if ($headSha !== '') {
+                verbose_log("pinning PR review to head commit {$headSha} for {$repo}#{$prNumber}", 2);
+                $baseErr = setupPrHeadBaseline($checkoutPath, $repo, $baseBranch, $headSha);
+                if ($baseErr === 'true') {
+                    $baselineReady = true;
+                    // Anchor inline comments on the EXACT reviewed commit.
+                    $sha = $headSha;
+                    verbose_log("PR head baseline pinned (_base=merge-base, HEAD={$headSha})", 2);
+                } else {
+                    verbose_log("PR head baseline failed ({$baseErr}); falling back to live diff apply", 1);
+                }
+            } else {
+                verbose_log("could not resolve PR head SHA; falling back to live diff apply", 1);
+            }
+
+            // Fallback: head SHA unresolved or the merge-base/head SHA could not
+            // be fetched. Use the live `gh pr diff`, apply it, and commit it as a
+            // snapshot (HEAD = base+PR, _base = base commit). This path is NOT
+            // pinned to a specific commit — it reviews the current PR state.
+            if (!$baselineReady) {
+                $diff = getPRDiff($repo, $prNumber);
+                if ($diff !== null && $diff !== '') {
+                    verbose_log("applying PR diff for {$repo}#{$prNumber}", 2);
+                    if (applyPRDiff($checkoutPath, $diff)) {
+                        if (commitPrSnapshot($checkoutPath, $prNumber)) {
+                            $baselineReady = true;
+                            verbose_log("PR snapshot committed (HEAD=base+PR, _base=base)", 2);
+                        } else {
+                            verbose_log("failed to commit PR snapshot - agent fixes cannot be captured", 1);
+                        }
                     } else {
-                        verbose_log("failed to commit PR snapshot - agent fixes cannot be captured", 1);
+                        verbose_log("failed to apply PR diff - analyzing base branch only", 2);
                     }
                 } else {
-                    verbose_log("failed to apply PR diff - analyzing base branch only", 2);
+                    verbose_log("no diff retrieved - analyzing base branch only", 2);
                 }
-            } else {
-                verbose_log("no diff retrieved - analyzing base branch only", 2);
             }
         }
 
@@ -1964,6 +2043,64 @@ function setupCommitScopedBaseline(string $checkoutPath, string $repo, string $s
     return setupCommitRangeBaseline($checkoutPath, $repo, '', $sha);
 }
 
+/**
+ * Resolve GitHub's true merge-base for a PR head against its base branch using
+ * the compare API's 3-dot semantics — the same base the PR "Files changed" view
+ * diffs against. Prefers .merge_base_commit.sha, falls back to .base_commit.sha
+ * (a.k.a. .base.sha). Returns '' when the API is unavailable so the caller can
+ * fall back to the base branch tip.
+ */
+function getPrMergeBaseSha(string $repo, string $baseBranch, string $headSha): string
+{
+    if ($repo === '' || $baseBranch === '' || $headSha === '') {
+        return '';
+    }
+    $cmd = sprintf(
+        'gh api %s 2>&1',
+        escapeshellarg("repos/{$repo}/compare/{$baseBranch}...{$headSha}")
+    );
+    $output = [];
+    $ret = 0;
+    exec($cmd, $output, $ret);
+    if ($ret !== 0) {
+        verbose_log('getPrMergeBaseSha: compare API failed: ' . substr(implode("\n", $output), 0, 200), 2);
+        return '';
+    }
+    $json = json_decode(implode("\n", $output), true);
+    if (!is_array($json)) {
+        return '';
+    }
+    foreach (['merge_base_commit', 'base_commit', 'base'] as $key) {
+        $sha = $json[$key]['sha'] ?? '';
+        if (is_string($sha) && $sha !== '') {
+            return $sha;
+        }
+    }
+    return '';
+}
+
+/**
+ * Pin a normal (non-commit-scoped) PR review to a specific head commit, mirroring
+ * the push path. Resolves GitHub's 3-dot merge-base so `git diff _base HEAD`
+ * matches the PR's Files-changed view exactly, then materializes
+ * _base = merge-base and HEAD = $headSha via setupCommitRangeBaseline(). When the
+ * merge-base cannot be fetched from the API, falls back to the base branch tip.
+ * Returns 'true' or an error string.
+ */
+function setupPrHeadBaseline(string $checkoutPath, string $repo, string $baseBranch, string $headSha): string
+{
+    if ($headSha === '') {
+        return 'missing head sha';
+    }
+    $mergeBase = getPrMergeBaseSha($repo, $baseBranch, $headSha);
+    if ($mergeBase === '') {
+        // API unavailable: use the base branch tip as the baseline. An empty
+        // baseBranch degrades to headSha^ inside setupCommitRangeBaseline().
+        $mergeBase = $baseBranch;
+    }
+    return setupCommitRangeBaseline($checkoutPath, $repo, $mergeBase, $headSha);
+}
+
 /**
  * Strip ANSI escape sequences (colors, cursor movement, OSC titles) from text.
  * opencode --format default may emit these; they break JSON/markdown parsing.
diff --git a/scripts/prompts/review.txt b/scripts/prompts/review.txt
@@ -17,7 +17,13 @@ problems elsewhere in the repository.
 
 ## How to fix
 
-For each real issue in the changed lines, edit the file in place to fix it:
+For each real issue in the changed lines, edit the file in place to fix it.
+
+**Default to FIXING.** A fix that is small, safe, and unambiguous — a typo, a
+misspelled string literal or constant (e.g. `'pendapprova'` should be
+`'pendapproval'`), an obviously wrong value, a missing null check on a changed
+line — MUST be fixed in place, never merely flagged. Only fall back to
+FLAGGED-only when a fix genuinely needs human judgment.
 
 - Make MINIMAL, SURGICAL changes. Touch only what is needed to fix the specific
   issue, scoped to the lines the change added or modified.
@@ -30,9 +36,10 @@ For each real issue in the changed lines, edit the file in place to fix it:
   `git add`, `git reset`, `git stash`, `git checkout`, or any other command that
   stages, commits, or discards changes. The worker captures your edits with a
   plain `git diff` afterwards.
-- If you cannot fix an issue safely with a small in-place change (for example it
-  would require an API change, a new dependency, or broad restructuring), leave
-  the code as-is and only FLAG it.
+- Only FLAG instead of fixing when the fix genuinely needs human judgment — it
+  would require an API or signature change, a new dependency, broad
+  restructuring, or you are not confident the change is correct. Do NOT flag a
+  trivial, unambiguous fix; make it.
 
 ## Confidence
 
diff --git a/tests/phpunit/unit/CodeReviewWorker/PrHeadBaselineTest.php b/tests/phpunit/unit/CodeReviewWorker/PrHeadBaselineTest.php
@@ -0,0 +1,134 @@
+<?php
+declare(strict_types=1);
+
+namespace Webhooks\Tests\Unit\CodeReviewWorker;
+
+use PHPUnit\Framework\TestCase;
+
+/**
+ * Tests for the pinned normal-PR review path added to
+ * scripts/github-code-review.php: resolvePrHeadSha(), setupPrHeadBaseline() and
+ * the underlying setupCommitRangeBaseline() plumbing it relies on.
+ *
+ * Every test is network-free: the git-plumbing case drives
+ * setupCommitRangeBaseline() directly with local SHAs (the merge-base and head),
+ * and the resolver/guard cases never reach the GitHub API.
+ */
+class PrHeadBaselineTest extends TestCase
+{
+    private string $dir = '';
+
+    protected function setUp(): void
+    {
+        require_once __DIR__ . '/../../../../scripts/github-code-review.php';
+        $GLOBALS['verbose'] = 0;
+        $GLOBALS['logFile'] = null;
+
+        $this->dir = sys_get_temp_dir() . '/crw_prhead_' . bin2hex(random_bytes(6));
+        mkdir($this->dir, 0777, true);
+    }
+
+    protected function tearDown(): void
+    {
+        if ($this->dir !== '' && is_dir($this->dir)) {
+            exec('rm -rf ' . escapeshellarg($this->dir));
+        }
+    }
+
+    private function git(string $args): string
+    {
+        return trim((string)shell_exec('cd ' . escapeshellarg($this->dir) . ' && git ' . $args . ' 2>&1'));
+    }
+
+    /**
+     * Build a merge-base commit + a feature (head) commit on main and return
+     * [mergeBaseSha, headSha]. In this linear history the merge-base is simply
+     * the commit the feature was built on — exactly what GitHub's 3-dot compare
+     * would report for a PR.
+     *
+     * @return array{0: string, 1: string}
+     */
+    private function initMergeBaseAndHead(): array
+    {
+        $this->git('init -q');
+        file_put_contents($this->dir . '/a.php', "<?php\necho 1;\n");
+        $this->git('add -A');
+        $this->git('-c user.email=setup@test -c user.name=Setup commit -q -m base');
+        $this->git('branch -M main');
+        $mergeBase = $this->git('rev-parse HEAD');
+
+        file_put_contents($this->dir . '/a.php', "<?php\necho 2;\n");
+        file_put_contents($this->dir . '/b.php', "<?php\necho 'feature';\n");
+        $this->git('add -A');
+        $this->git('-c user.email=setup@test -c user.name=Setup commit -q -m feature');
+        $head = $this->git('rev-parse HEAD');
+
+        return [$mergeBase, $head];
+    }
+
+    /**
+     * The plumbing setupPrHeadBaseline() delegates to: driven directly with the
+     * local merge-base and head SHAs so no network fetch happens. `git diff
+     * _base HEAD` must be exactly the PR change, and a later agent edit must be
+     * the only thing captureAgentFixes() reports.
+     */
+    public function testCommitRangeBaselinePinsMergeBaseAndHead(): void
+    {
+        [$mergeBase, $head] = $this->initMergeBaseAndHead();
+
+        $this->assertSame('true', setupCommitRangeBaseline($this->dir, 'owner/repo', $mergeBase, $head));
+
+        $this->assertSame($mergeBase, $this->git('rev-parse _base'), '_base pins the merge-base commit');
+        $this->assertSame($head, $this->git('rev-parse HEAD'), 'HEAD lands on the head commit');
+
+        // The PR change is exactly _base..HEAD.
+        $changed = $this->git('diff --name-only _base HEAD');
+        $this->assertStringContainsString('a.php', $changed);
+        $this->assertStringContainsString('b.php', $changed);
+
+        // Agent edits one file in place -> captureAgentFixes returns ONLY that.
+        file_put_contents($this->dir . '/a.php', "<?php\necho 2; // fixed by agent\n");
+        $fixes = captureAgentFixes($this->dir);
+        $this->assertSame(['a.php'], $fixes['files'], 'only the agent-edited file is listed');
+        $this->assertStringContainsString('fixed by agent', $fixes['diff']);
+    }
+
+    /**
+     * resolvePrHeadSha() prefers a non-empty $job['sha'] and returns it verbatim
+     * (trimmed) without any GitHub API lookup.
+     */
+    public function testResolvePrHeadShaPrefersJobSha(): void
+    {
+        $sha = '0123456789abcdef0123456789abcdef01234567';
+        $job = ['sha' => $sha, 'pr_number' => 42, 'base_branch' => 'main'];
+
+        $this->assertSame($sha, resolvePrHeadSha($job, 'owner/repo', 42));
+    }
+
+    public function testResolvePrHeadShaTrimsJobSha(): void
+    {
+        $job = ['sha' => "  abc1234  \n"];
+        $this->assertSame('abc1234', resolvePrHeadSha($job, 'owner/repo', 42));
+    }
+
+    /**
+     * With no job SHA and nothing to query against (pr_number 0), the resolver
+     * returns '' without touching the network so the caller takes the fallback.
+     */
+    public function testResolvePrHeadShaEmptyWhenNoShaAndNoPrNumber(): void
+    {
+        $this->assertSame('', resolvePrHeadSha(['sha' => ''], 'owner/repo', 0));
+        $this->assertSame('', resolvePrHeadSha([], 'owner/repo', 0));
+        $this->assertSame('', resolvePrHeadSha(['sha' => ''], '', 42));
+    }
+
+    /**
+     * setupPrHeadBaseline() short-circuits on an empty head SHA before any API
+     * call, so the guard is exercised network-free.
+     */
+    public function testSetupPrHeadBaselineMissingHeadShaReturnsError(): void
+    {
+        $this->git('init -q');
+        $this->assertSame('missing head sha', setupPrHeadBaseline($this->dir, 'owner/repo', 'main', ''));
+    }
+}
