Skip to content

Commit b4fbf44

Browse files
detainclaude
detain
and
claude
committed
feat(github-code-review): review pushes/commits and honor submit options
Extend the review engine beyond pull requests. - web/github.php enqueues push events (skips deletes/empty/bots). - CodeReviewQueue gains a `kind` field (pr|push, missing => pr for back-compat) plus enqueuePush(). - processJob() dispatches to processPrJob()/processPushJob() which share runReviewAndPost(). Three baselines all resolve to _base..HEAD so the same `git diff _base HEAD` review + `git diff` fix-capture apply: PR (base+PR snapshot), PR --commit SHA (SHA^..SHA), and push (before..after, with shallow-clone SHA fetch/deepen, head^ for new branches, empty-tree for root commits). - Push findings post via the commit-comments API; PR posting is unchanged. - Honor submit options (comments only): audit_types scopes the composed prompt (review.txt stays generic); combine = one summary comment; split_changes = grouped summary comments (file|audit|severity|size). post_branch and issue_for_commits are logged no-op stubs that fall back to comments -- the bot creates no PRs, branches, or issues. - 42 new tests: push envelope/back-compat, push and commit-scoped baselines on real throwaway repos, prompt scoping, combine/split builders, route guards. Also folds in pending unrelated CLI test edits (ListCommand, GithubActivityService). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 3200acf commit b4fbf44

9 files changed

Lines changed: 1604 additions & 160 deletions

File tree

‎scripts/github-code-review.php‎

Lines changed: 845 additions & 144 deletions
Large diffs are not rendered by default.

‎src/CodeReviewQueue.php‎

Lines changed: 114 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,18 +2,26 @@
22
declare(strict_types=1);
33

44
/**
5-
* Redis-backed queue for GitHub PR code review jobs.
5+
* Redis-backed queue for GitHub code review jobs.
66
*
7-
* Envelopes are produced by web/github.php when a pull_request event fires
8-
* (opened, synchronize) and consumed by scripts/github-code-review.php.
7+
* Two kinds of job share one queue, distinguished by the "kind" field:
8+
*
9+
* - "pr" (default): produced by web/github.php on a pull_request event
10+
* (opened, synchronize) and by the `submit` CLI command.
11+
* - "push": produced by web/github.php on a push event — reviews a
12+
* before..after commit range with no PR involved.
13+
*
14+
* A dequeued envelope with NO "kind" field is treated as "pr" for
15+
* backward compatibility with envelopes enqueued before push support.
916
*
1017
* Queue key: codereview:queue (RPOP / BRPOP)
1118
*
12-
* Envelope shape:
19+
* PR envelope shape:
1320
* {
1421
* "v": 1,
1522
* "id": "uuid-v4",
1623
* "ts": 1715628000,
24+
* "kind": "pr",
1725
* "repo": "owner/repo",
1826
* "pr_number": 42,
1927
* "action": "opened", // opened | synchronize
@@ -25,6 +33,25 @@
2533
* "sha": "abc123...", // commit SHA (empty for opened)
2634
* "source": "webhooks/github.php"
2735
* }
36+
*
37+
* Push envelope shape:
38+
* {
39+
* "v": 1,
40+
* "id": "uuid-v4",
41+
* "ts": 1715628000,
42+
* "kind": "push",
43+
* "repo": "owner/repo",
44+
* "action": "push",
45+
* "ref": "main", // branch (refs/heads/ stripped)
46+
* "before_sha": "abc123...", // empty for a brand-new branch (use after^)
47+
* "after_sha": "def456...",
48+
* "author": "username",
49+
* "author_url": "https://github.com/username",
50+
* "is_bot": false,
51+
* "audit_types": ["full"],
52+
* "options": {},
53+
* "source": "webhooks/github.php"
54+
* }
2855
*/
2956
class CodeReviewQueue
3057
{
@@ -42,6 +69,16 @@ public static function getLastStatus(): string
4269
return self::$lastStatus;
4370
}
4471

72+
/**
73+
* Resolve the kind of a (possibly legacy) envelope. Anything other than an
74+
* explicit "push" is treated as "pr" — this keeps envelopes enqueued before
75+
* push support (which have no "kind" field) working as PR jobs.
76+
*/
77+
public static function jobKind(array $envelope): string
78+
{
79+
return (($envelope['kind'] ?? 'pr') === 'push') ? 'push' : 'pr';
80+
}
81+
4582
/**
4683
* Enqueue a PR for code review.
4784
*
@@ -85,6 +122,49 @@ public static function enqueue(array $job): bool
85122
}
86123
}
87124

125+
/**
126+
* Enqueue a push (before..after commit range) for code review.
127+
*
128+
* @param array{
129+
* repo: string,
130+
* ref: string,
131+
* before_sha?: string,
132+
* after_sha: string,
133+
* author?: string,
134+
* author_url?: string,
135+
* is_bot?: bool,
136+
* audit_types?: array<string>,
137+
* options?: array<string, mixed>,
138+
* } $job
139+
*/
140+
public static function enqueuePush(array $job): bool
141+
{
142+
$envelope = self::buildPushEnvelope($job);
143+
144+
$r = self::redis();
145+
if ($r === null) {
146+
self::$lastStatus = 'failed_no_redis';
147+
error_log('CodeReviewQueue: no Redis client available');
148+
return false;
149+
}
150+
151+
try {
152+
$json = json_encode($envelope, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE);
153+
if ($json === false) {
154+
self::$lastStatus = 'failed_json_encode';
155+
return false;
156+
}
157+
$r->lpush(self::QUEUE_KEY, [$json]);
158+
$r->incr(self::METRICS_KEY);
159+
self::$lastStatus = 'queued';
160+
return true;
161+
} catch (\Throwable $e) {
162+
error_log('CodeReviewQueue lpush failed: ' . $e->getMessage());
163+
self::$lastStatus = 'failed_redis_exception';
164+
return false;
165+
}
166+
}
167+
88168
/**
89169
* Dequeue a single job from the queue (blocking RPOP).
90170
*
@@ -162,12 +242,13 @@ public static function requeue(array $envelope): bool
162242
}
163243
}
164244

165-
private static function buildEnvelope(array $job): array
245+
public static function buildEnvelope(array $job): array
166246
{
167247
return [
168248
'v' => self::ENVELOPE_VERSION,
169249
'id' => self::uuidV4(),
170250
'ts' => time(),
251+
'kind' => 'pr',
171252
'repo' => $job['repo'],
172253
'pr_number' => (int)$job['pr_number'],
173254
'action' => $job['action'],
@@ -177,6 +258,34 @@ private static function buildEnvelope(array $job): array
177258
'author' => $job['author'],
178259
'author_url' => $job['author_url'],
179260
'sha' => $job['sha'] ?? '',
261+
'is_bot' => (bool)($job['is_bot'] ?? false),
262+
'source' => 'webhooks/github.php',
263+
'retry_count' => 0,
264+
];
265+
}
266+
267+
/**
268+
* Build a push job envelope (kind=push). Push jobs review a before..after
269+
* commit range with no PR; an empty before_sha signals a brand-new branch
270+
* so the worker falls back to after^ as the baseline.
271+
*/
272+
public static function buildPushEnvelope(array $job): array
273+
{
274+
return [
275+
'v' => self::ENVELOPE_VERSION,
276+
'id' => self::uuidV4(),
277+
'ts' => time(),
278+
'kind' => 'push',
279+
'repo' => $job['repo'],
280+
'action' => 'push',
281+
'ref' => $job['ref'] ?? '',
282+
'before_sha' => $job['before_sha'] ?? '',
283+
'after_sha' => $job['after_sha'] ?? '',
284+
'author' => $job['author'] ?? '',
285+
'author_url' => $job['author_url'] ?? '',
286+
'is_bot' => (bool)($job['is_bot'] ?? false),
287+
'audit_types' => $job['audit_types'] ?? ['full'],
288+
'options' => $job['options'] ?? [],
180289
'source' => 'webhooks/github.php',
181290
'retry_count' => 0,
182291
];

‎tests/phpunit/unit/Cli/Command/ListCommandTest.php‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,6 @@
1212

1313
class ListCommandTest extends TestCase
1414
{
15-
private CommandTester $commandTester;
16-
17-
protected function setUp(): void
18-
{
19-
$application = new CliApplication();
20-
$command = $application->find('list');
21-
$this->commandTester = new CommandTester($command);
22-
}
23-
2415
public function testConfigureSetsNameAndDescription(): void
2516
{
2617
$application = new CliApplication();

‎tests/phpunit/unit/Cli/Service/GithubActivityServiceTest.php‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,6 @@ public function testGetUserReturnsNullWhenGhNotAvailable(): void
4343
if ($user === null) {
4444
// gh not authenticated - this is valid behavior
4545
$this->markTestSkipped('gh is available but not authenticated');
46-
return;
4746
}
4847

4948
// If authenticated, validate the response structure

‎tests/phpunit/unit/CodeReviewQueue/CodeReviewQueueTest.php‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,114 @@
1+
<?php
2+
declare(strict_types=1);
3+
4+
namespace Webhooks\Tests\Unit\CodeReviewQueue;
5+
6+
use PHPUnit\Framework\TestCase;
7+
8+
/**
9+
* Tests for the push-support additions to CodeReviewQueue: the "kind" field,
10+
* buildPushEnvelope(), and the jobKind() back-compat resolver. These exercise
11+
* only pure envelope/kind logic — no Redis is touched.
12+
*/
13+
class CodeReviewQueueTest extends TestCase
14+
{
15+
protected function setUp(): void
16+
{
17+
require_once __DIR__ . '/../../../../src/CodeReviewQueue.php';
18+
}
19+
20+
public function testBuildEnvelopeMarksPrKind(): void
21+
{
22+
$envelope = \CodeReviewQueue::buildEnvelope([
23+
'repo' => 'owner/repo',
24+
'pr_number' => 42,
25+
'action' => 'opened',
26+
'head_branch' => 'feature/x',
27+
'base_branch' => 'main',
28+
'pr_url' => 'https://github.com/owner/repo/pull/42',
29+
'author' => 'octocat',
30+
'author_url' => 'https://github.com/octocat',
31+
'sha' => 'abc123',
32+
]);
33+
34+
$this->assertSame('pr', $envelope['kind']);
35+
$this->assertSame('owner/repo', $envelope['repo']);
36+
$this->assertSame(42, $envelope['pr_number']);
37+
$this->assertSame(0, $envelope['retry_count']);
38+
}
39+
40+
public function testBuildPushEnvelopeShape(): void
41+
{
42+
$envelope = \CodeReviewQueue::buildPushEnvelope([
43+
'repo' => 'owner/repo',
44+
'ref' => 'main',
45+
'before_sha' => 'aaaaaaa',
46+
'after_sha' => 'bbbbbbb',
47+
'author' => 'octocat',
48+
'author_url' => 'https://github.com/octocat',
49+
'is_bot' => false,
50+
]);
51+
52+
$this->assertSame('push', $envelope['kind']);
53+
$this->assertSame('push', $envelope['action']);
54+
$this->assertSame('owner/repo', $envelope['repo']);
55+
$this->assertSame('main', $envelope['ref']);
56+
$this->assertSame('aaaaaaa', $envelope['before_sha']);
57+
$this->assertSame('bbbbbbb', $envelope['after_sha']);
58+
$this->assertSame('octocat', $envelope['author']);
59+
$this->assertFalse($envelope['is_bot']);
60+
$this->assertSame(['full'], $envelope['audit_types']);
61+
$this->assertSame([], $envelope['options']);
62+
$this->assertSame(0, $envelope['retry_count']);
63+
// No PR fields on a push envelope.
64+
$this->assertArrayNotHasKey('pr_number', $envelope);
65+
}
66+
67+
public function testBuildPushEnvelopeDefaultsBeforeShaEmpty(): void
68+
{
69+
$envelope = \CodeReviewQueue::buildPushEnvelope([
70+
'repo' => 'owner/repo',
71+
'ref' => 'feature/new',
72+
'after_sha' => 'bbbbbbb',
73+
]);
74+
75+
// Brand-new branch: before_sha absent -> empty (worker uses after^).
76+
$this->assertSame('', $envelope['before_sha']);
77+
$this->assertSame('bbbbbbb', $envelope['after_sha']);
78+
}
79+
80+
public function testBuildPushEnvelopeCarriesAuditTypesAndOptions(): void
81+
{
82+
$envelope = \CodeReviewQueue::buildPushEnvelope([
83+
'repo' => 'owner/repo',
84+
'ref' => 'main',
85+
'after_sha' => 'bbbbbbb',
86+
'audit_types' => ['security', 'logic'],
87+
'options' => ['combine' => true],
88+
]);
89+
90+
$this->assertSame(['security', 'logic'], $envelope['audit_types']);
91+
$this->assertSame(['combine' => true], $envelope['options']);
92+
}
93+
94+
public function testJobKindDefaultsToPrWhenMissing(): void
95+
{
96+
// Back-compat: envelopes enqueued before push support have no "kind".
97+
$this->assertSame('pr', \CodeReviewQueue::jobKind(['repo' => 'owner/repo', 'pr_number' => 1]));
98+
}
99+
100+
public function testJobKindExplicitPr(): void
101+
{
102+
$this->assertSame('pr', \CodeReviewQueue::jobKind(['kind' => 'pr']));
103+
}
104+
105+
public function testJobKindPush(): void
106+
{
107+
$this->assertSame('push', \CodeReviewQueue::jobKind(['kind' => 'push']));
108+
}
109+
110+
public function testJobKindUnknownFallsBackToPr(): void
111+
{
112+
$this->assertSame('pr', \CodeReviewQueue::jobKind(['kind' => 'something-else']));
113+
}
114+
}

0 commit comments

Comments
 (0)