refactor(github-code-review): commit change snapshot so agent fixes are a clean diff

Review the changes on a committed baseline instead of an uncommitted working
tree, so the reviewer's in-place fixes can be captured as their own diff.

- processJob(): after applyPRDiff, record _base and commit the change as a
  snapshot (commitPrSnapshot). HEAD=base+change, _base=base — so
  `git diff _base HEAD` is the change under review and a later `git diff` is
  only the agent's fixes.
- Capture fixes from git (captureAgentFixes/getAgentFileDiff) instead of a
  second opencode "improve" pass. Remove runOpencodeImprove, parseImproveOutput,
  extractFixedContent, getFileDiff and OPENCODE_IMPROVE_CMD.
- resetCheckoutToCleanState wipes the snapshot commit and the _base ref so
  cached checkouts can't drift.
- review.txt: the agent now fixes issues in place (scoped to the changed lines,
  no git writes) and reports in the parseable format. Worded generically so it
  serves commit reviews as well as PRs.
- tests: add CommitBaselineTest; drop tests for the removed functions.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: detain

scripts/github-code-review.php

@@ -1,102 +1,82 @@
-# Code Review Philosophy
-
-## TL;DR
-Systematic code review across 4 layers with severity classification. Only report findings with ≥80% confidence. Include file:line references for all issues.
-
-## When to Use This Skill
-- Before reporting implementation completion
-- When explicitly asked to review code
-- When using the `/review` command
-- As an independent audit after code changes
-
-## The 4 Review Layers
-
-### Layer 1: Correctness
-- Logic errors and edge cases
-- Error handling completeness
-- Type safety and null checks
-- Algorithm correctness
-- Off-by-one errors
-
-### Layer 2: Security
-- No hardcoded secrets or API keys
-- Input validation and sanitization
-- Injection vulnerability prevention (SQL, XSS, command)
-- Authentication and authorization checks
-- Sensitive data not logged
-- OWASP Top 10 awareness
-
-### Layer 3: Performance
-- No N+1 query patterns
-- Appropriate caching strategies
-- No unnecessary re-renders (React/frontend)
-- Lazy loading where appropriate
-- Memory leak prevention
-- Algorithmic complexity concerns
-
-### Layer 4: Style & Maintainability
-- Adherence to project conventions (check AGENTS.md)
-- Code duplication (DRY violations)
-- Complexity management (cyclomatic complexity)
-- Documentation completeness
-- Test coverage gaps
-
-## Severity Classification
-
-| Severity | Icon | Criteria | Action Required |
-|----------|------|----------|-----------------|
-| Critical | 🔴 | Security vulnerabilities, crashes, data loss, corruption | Must fix before merge |
-| Major | 🟠 | Bugs, performance issues, missing error handling | Should fix |
-| Minor | 🟡 | Code smells, maintainability issues, test gaps | Nice to fix |
-| Nitpick | 🟢 | Style preferences, naming suggestions, documentation | Optional |
-
-## Confidence Threshold
-**Only report findings with ≥80% confidence.**
-
-If uncertain about an issue:
-- State the uncertainty explicitly: "Potential issue (70% confidence): ..."
-- Suggest investigation rather than assert a problem
-- Prefer false negatives over false positives (reduce noise)
-
-## Review Process
-
-1. **Initial Scan** - Identify all files in scope, understand the change
-2. **Deep Analysis** - Apply all 4 layers systematically to each file
-3. **Context Evaluation** - Consider surrounding code, project patterns, existing conventions
-4. **Philosophy Check** - Verify against code-philosophy (5 Laws) if applicable
-5. **Synthesize Findings** - Group by severity, deduplicate, prioritize
-
-## Output Format
-
-Structure your review as:
-
-1. **Files Reviewed** - List all files analyzed
-2. **Overall Assessment** - APPROVE | REQUEST_CHANGES | NEEDS_DISCUSSION
-3. **Summary** - 2-3 sentence overview
-4. **Critical Issues** (🔴) - With file:line references
-5. **Major Issues** (🟠) - With file:line references
-6. **Minor Issues** (🟡) - With file:line references
-7. **Positive Observations** (🟢) - What's done well (always include at least one)
-8. **Philosophy Compliance** - Checklist results if applicable
-
-## What NOT to Do
-
-- Do NOT report low-confidence findings as definite issues
-- Do NOT provide vague feedback without file:line references
-- Do NOT skip any of the 4 layers
-- Do NOT forget to note positive observations
-- Do NOT modify any files during review
-- Do NOT approve without completing the full review process
-
-## Adherence Checklist
-
-Before completing a review, verify:
-- [ ] All 4 layers analyzed (Correctness, Security, Performance, Style)
-- [ ] Severity assigned to each finding
-- [ ] Confidence ≥80% for all reported issues (or uncertainty stated)
-- [ ] File names and line numbers included for all findings
-- [ ] Positive observations noted
-- [ ] Output follows the standard format
-
-
-Start this now and continu till its finished.
+You are an automated Code Review agent. Your job is to REVIEW a set of code
+changes AND fix the problems you find by editing the affected files in place.
+The changes may come from a pull request, a pushed commit, or a range of commits
+— the review works the same way regardless of the source.
+
+## What changed
+
+The changes under review have been committed as a snapshot on top of the code as
+it was before them. To see EXACTLY what changed, run:
+
+    git diff _base HEAD
+
+`_base` is the baseline (the code before the change); `HEAD` is the baseline plus
+the changes under review. That diff — and only that diff — is your review scope.
+Read the surrounding code for context, but do not go looking for unrelated
+problems elsewhere in the repository.
+
+## How to fix
+
+For each real issue in the changed lines, edit the file in place to fix it:
+
+- Make MINIMAL, SURGICAL changes. Touch only what is needed to fix the specific
+  issue, scoped to the lines the change added or modified.
+- Do NOT refactor, reformat, rename, or "improve" code that is unrelated to the
+  issue. No drive-by cleanups.
+- Preserve `declare(strict_types=1);` at the top of every PHP file.
+- Follow the existing style: PSR-2 plus PHP 7.4 conventions. Match the file's
+  existing indentation and formatting.
+- Leave your edits UNCOMMITTED in the working tree. Do NOT run `git commit`,
+  `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.
+
+## Confidence
+
+Only report issues you are at least 80% confident are real. Prefer silence over
+noise: a false positive wastes the author's time.
+
+## Output format — follow EXACTLY
+
+After you finish editing, print a report. Each issue MUST be a single heading line
+in this exact shape (the worker parses these lines):
+
+    #### <emoji> <title> — <path>:<line>
+
+Where:
+
+- `<emoji>` is exactly one of these severity markers:
+    🔴 critical  — security holes, crashes, data loss/corruption
+    🟠 major     — bugs, missing error handling, real correctness problems
+    🟡 minor     — code smells, maintainability, small correctness risks
+    🟢 nitpick   — style, naming, documentation
+- `<title>` is a short summary of the issue (a few words).
+- `<path>` is the repository-relative file path (no spaces, no colons).
+- `<line>` is the line number in the changed (HEAD) version of the file.
+- Use a spaced em dash ` — ` between the title and `<path>:<line>`.
+
+Immediately below each heading, write ONE short paragraph. It MUST state whether
+you FIXED the issue in place or only FLAGGED it, and briefly why. Start that
+paragraph with the word `FIXED` or `FLAGGED`.
+
+Separate issues with a blank line. Example:
+
+#### 🔴 SQL injection in user lookup — include/Api/Users.php:207
+FIXED: the username was concatenated straight into the query; switched it to a
+parameterized `$db->real_escape()` call.
+
+#### 🟠 Unhandled null from getService() — include/Api/Users.php:88
+FLAGGED: getService() can return null here, but guarding it correctly needs a
+public signature change, so it is out of scope for an in-place fix.
+
+After all issues, end the report with a single summary line, on its own line,
+in exactly this form (N = issues you fixed, M = total issues you reported):
+
+Fixed N of M issues.
+
+If you find no issues at all, print exactly:
+
+Fixed 0 of 0 issues.

scripts/prompts/review.txt

@@ -33,7 +33,7 @@ public function testBaseBranchFallbackFlow(): void
         $checkoutCalls = [];
         $running = true;
 
-        $mockCheckoutBranch = function(string $repo, string $branch, string $path) use (&$checkoutCalls, &$running): string {
+        $mockCheckoutBranch = function(string $repo, string $branch, string $path) use (&$checkoutCalls): string {
             $checkoutCalls[] = ['repo' => $repo, 'branch' => $branch, 'path' => $path];
             if ($branch === 'main') {
                 return 'gh repo clone failed: main branch not found';
@@ -44,10 +44,7 @@ public function testBaseBranchFallbackFlow(): void
             return 'unknown branch';
         };
 
-        $mockGetPRBaseBranch = function(string $repo, int $prNumber) use (&$running): ?string {
-            if (!$running) {
-                return null;
-            }
+        $mockGetPRBaseBranch = function(string $repo, int $prNumber): string {
             return 'master';
         };
 
@@ -86,7 +83,7 @@ public function testBaseBranchFallbackFailsWhenActualBranchCheckoutAlsoFails():
             return 'gh repo clone failed: branch not found';
         };
 
-        $mockGetPRBaseBranch = function(string $repo, int $prNumber): ?string {
+        $mockGetPRBaseBranch = function(string $repo, int $prNumber): string {
             return 'master';
         };
 
@@ -124,7 +121,7 @@ public function testGetPRBaseBranchIsCalledOnlyWhenFirstCheckoutFails(): void
             return 'true';
         };
 
-        $mockGetPRBaseBranch = function(string $repo, int $prNumber) use (&$getPRBaseBranchCalls): ?string {
+        $mockGetPRBaseBranch = function(string $repo, int $prNumber) use (&$getPRBaseBranchCalls): string {
             $getPRBaseBranchCalls[] = ['repo' => $repo, 'prNumber' => $prNumber];
             return 'master';
         };
@@ -164,7 +161,7 @@ public function testGetPRBaseBranchNotCalledWhenShutdownBeforeFallback(): void
             return 'shutdown';
         };
 
-        $mockGetPRBaseBranch = function(string $repo, int $prNumber) use (&$getPRBaseBranchCalls): ?string {
+        $mockGetPRBaseBranch = function(string $repo, int $prNumber) use (&$getPRBaseBranchCalls): string {
             $getPRBaseBranchCalls[] = ['repo' => $repo, 'prNumber' => $prNumber];
             return 'master';
         };
@@ -244,7 +241,7 @@ public function testActualBaseBranchSameAsBaseBranchDoesNotRetry(): void
             return 'gh repo clone failed: main branch not found';
         };
 
-        $mockGetPRBaseBranch = function(string $repo, int $prNumber): ?string {
+        $mockGetPRBaseBranch = function(string $repo, int $prNumber): string {
             return 'main';
         };
 

tests/phpunit/unit/CodeReviewWorker/BaseBranchFallbackTest.php

@@ -0,0 +1,138 @@
+<?php
+declare(strict_types=1);
+
+namespace Webhooks\Tests\Unit\CodeReviewWorker;
+
+use PHPUnit\Framework\TestCase;
+
+/**
+ * Tests for the commit-baseline redesign in scripts/github-code-review.php:
+ * commitPrSnapshot(), captureAgentFixes(), getAgentFileDiff(), and
+ * resetCheckoutToCleanState(). Each test drives a real throwaway git repo so
+ * the git plumbing is exercised end to end.
+ */
+class CommitBaselineTest 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_baseline_' . 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));
+        }
+    }
+
+    /** Run a git command in the test repo and return trimmed stdout+stderr. */
+    private function git(string $args): string
+    {
+        return trim((string)shell_exec('cd ' . escapeshellarg($this->dir) . ' && git ' . $args . ' 2>&1'));
+    }
+
+    /**
+     * Create an initial commit on a `main` branch with the given files and
+     * return the base commit SHA.
+     *
+     * @param array<string, string> $files path => contents
+     */
+    private function initBaseRepo(array $files): string
+    {
+        $this->git('init -q');
+        foreach ($files as $path => $contents) {
+            file_put_contents($this->dir . '/' . $path, $contents);
+        }
+        $this->git('add -A');
+        $this->git('-c user.email=setup@test -c user.name=Setup commit -q -m base');
+        $this->git('branch -M main');
+        return $this->git('rev-parse HEAD');
+    }
+
+    public function testCommitPrSnapshotRecordsBaseAndCommitsPr(): void
+    {
+        $baseSha = $this->initBaseRepo(['a.php' => "<?php\necho 1;\n"]);
+
+        // Apply a "PR": modify a tracked file and add a new one.
+        file_put_contents($this->dir . '/a.php', "<?php\necho 2;\n");
+        file_put_contents($this->dir . '/b.php', "<?php\necho 'new';\n");
+
+        $this->assertTrue(commitPrSnapshot($this->dir, 42));
+
+        // _base pins the base commit; HEAD has advanced to the snapshot.
+        $this->assertSame($baseSha, $this->git('rev-parse _base'), '_base must pin the base commit');
+        $this->assertNotSame($baseSha, $this->git('rev-parse HEAD'), 'HEAD must advance to the snapshot');
+        $this->assertSame('PR #42 snapshot', $this->git('log -1 --pretty=%s'));
+
+        // Working tree is clean and the PR shows up as _base..HEAD.
+        $this->assertSame('', $this->git('status --porcelain'), 'tree must be clean after snapshot');
+        $changed = $this->git('diff --name-only _base HEAD');
+        $this->assertStringContainsString('a.php', $changed);
+        $this->assertStringContainsString('b.php', $changed);
+    }
+
+    public function testCaptureAgentFixesReturnsOnlyAgentEdits(): void
+    {
+        $this->initBaseRepo(['a.php' => "<?php\necho 1;\n"]);
+        file_put_contents($this->dir . '/a.php', "<?php\necho 2;\n");
+        file_put_contents($this->dir . '/b.php', "<?php\necho 'new';\n");
+        $this->assertTrue(commitPrSnapshot($this->dir, 7));
+
+        // Agent edits one file in place, leaving it uncommitted.
+        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']);
+
+        // Per-file diff has hunks only for the edited file.
+        $this->assertStringContainsString('fixed by agent', getAgentFileDiff($this->dir, 'a.php'));
+        $this->assertSame('', trim(getAgentFileDiff($this->dir, 'b.php')));
+    }
+
+    public function testCaptureAgentFixesEmptyWhenTreeClean(): void
+    {
+        $this->initBaseRepo(['a.php' => "<?php\necho 1;\n"]);
+        file_put_contents($this->dir . '/a.php', "<?php\necho 2;\n");
+        $this->assertTrue(commitPrSnapshot($this->dir, 1));
+
+        $fixes = captureAgentFixes($this->dir);
+        $this->assertSame([], $fixes['files']);
+        $this->assertSame('', $fixes['diff']);
+    }
+
+    public function testResetCheckoutToCleanStateWipesEditsSnapshotAndBaseRef(): void
+    {
+        $baseSha = $this->initBaseRepo(['a.php' => "v1\n"]);
+
+        // Simulate a previous job's leftovers: a committed PR snapshot (which a
+        // fresh checkoutBranch() would already have re-pointed the branch away
+        // from — emulated here by resetting main back to the base commit), a
+        // stale _base ref, an in-place tracked edit, and an untracked file.
+        file_put_contents($this->dir . '/a.php', "v2\n");
+        file_put_contents($this->dir . '/b.php', "added by PR\n");
+        $this->assertTrue(commitPrSnapshot($this->dir, 99));
+        $this->git('reset --hard ' . $baseSha); // emulate checkoutBranch sync to fresh base
+        file_put_contents($this->dir . '/a.php', "agent-edit\n"); // leftover tracked edit
+        file_put_contents($this->dir . '/c.php', "agent-untracked\n"); // leftover untracked file
+
+        $this->assertTrue(resetCheckoutToCleanState($this->dir, 'main'));
+
+        // Tree is pristine at the base commit.
+        $this->assertSame($baseSha, $this->git('rev-parse HEAD'));
+        $this->assertSame('', $this->git('status --porcelain'), 'working tree must be clean');
+        $this->assertSame("v1\n", file_get_contents($this->dir . '/a.php'), 'tracked edit reverted');
+        $this->assertFileDoesNotExist($this->dir . '/c.php', 'untracked file removed');
+        $this->assertFileDoesNotExist($this->dir . '/b.php', 'snapshot-only file removed');
+
+        // The scratch _base ref is gone so a cached checkout cannot drift on it.
+        $this->assertSame('', $this->git('branch --list _base'), '_base ref must be deleted');
+    }
+}