From 504be9e41a6380199256fc1a92479b0e9eecb296 Mon Sep 17 00:00:00 2001 From: Eyejoker Date: Sun, 29 Mar 2026 23:23:30 +0900 Subject: [PATCH] refactor: use tree hash + git diff for post-approval change detection - Replace commit hash comparison with git tree hash (HEAD^{tree}) to avoid false re-review triggers on commit-only operations - Use git diff --quiet for actual file change detection - Update source_ref on failed reviewer execution with done verdict - Add tests for empty-commit finalize and code-change re-review - Document Codex reviewer bash mutation gap in reviewer-runtime --- runners/codex-runner/src/reviewer-runtime.ts | 3 + src/paired-execution-context.test.ts | 158 ++++++++++++++++++- src/paired-execution-context.ts | 73 ++++++--- 3 files changed, 213 insertions(+), 21 deletions(-) diff --git a/runners/codex-runner/src/reviewer-runtime.ts b/runners/codex-runner/src/reviewer-runtime.ts index 4947926..8ee4382 100644 --- a/runners/codex-runner/src/reviewer-runtime.ts +++ b/runners/codex-runner/src/reviewer-runtime.ts @@ -5,6 +5,9 @@ import path from 'path'; import type { RoomRoleContext } from './room-role-context.js'; +// Codex app-server does not expose a BashTool-style pre-use hook, so reviewer +// mode can only hard-block mutating git via PATH interception here. Non-git +// shell mutation commands remain a known gap when REVIEWER_AGENT_TYPE=codex. const BLOCKED_GIT_SUBCOMMANDS = new Set([ 'add', 'am', diff --git a/src/paired-execution-context.test.ts b/src/paired-execution-context.test.ts index 628a255..03e5b25 100644 --- a/src/paired-execution-context.test.ts +++ b/src/paired-execution-context.test.ts @@ -38,7 +38,12 @@ import { preparePairedExecutionContext, } from './paired-execution-context.js'; import * as pairedWorkspaceManager from './paired-workspace-manager.js'; -import type { RegisteredGroup, RoomRoleContext } from './types.js'; +import type { + PairedTask, + PairedWorkspace, + RegisteredGroup, + RoomRoleContext, +} from './types.js'; const group: RegisteredGroup = { name: 'Paired Room', @@ -85,6 +90,51 @@ function createCanonicalRepoWithCommit(commitMessage: string): string { return repoDir; } +function resolveTreeRef(repoDir: string): string { + return execFileSync('git', ['rev-parse', 'HEAD^{tree}'], { + cwd: repoDir, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + }).trim(); +} + +function buildPairedTask(overrides: Partial = {}): PairedTask { + return { + id: 'task-1', + chat_jid: 'dc:test', + group_folder: group.folder, + owner_service_id: 'codex-main', + reviewer_service_id: 'codex-review', + title: null, + source_ref: 'HEAD', + plan_notes: null, + review_requested_at: null, + round_trip_count: 0, + status: 'active', + created_at: '2026-03-28T00:00:00.000Z', + updated_at: '2026-03-28T00:00:00.000Z', + ...overrides, + }; +} + +function buildWorkspace( + role: 'owner' | 'reviewer', + workspaceDir: string, +): PairedWorkspace { + return { + id: `task-1:${role}`, + task_id: 'task-1', + role, + workspace_dir: workspaceDir, + snapshot_source_dir: role === 'reviewer' ? '/tmp/paired/task-1/owner' : null, + snapshot_ref: role === 'reviewer' ? 'fingerprint-1' : null, + status: 'ready', + snapshot_refreshed_at: role === 'reviewer' ? '2026-03-28T00:00:00.000Z' : null, + created_at: '2026-03-28T00:00:00.000Z', + updated_at: '2026-03-28T00:00:00.000Z', + }; +} + async function importExecutionContextForService(serviceId: string) { vi.resetModules(); @@ -363,6 +413,112 @@ describe('paired execution context', () => { // Should not throw; just logs. }); + it('completes owner finalize when only the commit object changed after approval', () => { + const repoDir = createCanonicalRepoWithCommit('reviewed'); + const approvedSourceRef = resolveTreeRef(repoDir); + execFileSync('git', ['commit', '--allow-empty', '-m', 'metadata only'], { + cwd: repoDir, + stdio: 'ignore', + }); + + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'merge_ready', + source_ref: approvedSourceRef, + }), + ); + vi.mocked(db.getPairedWorkspace).mockImplementation((_taskId, role) => + role === 'owner' ? buildWorkspace('owner', repoDir) : undefined, + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'owner', + status: 'succeeded', + summary: 'DONE', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ status: 'completed' }), + ); + expect(pairedWorkspaceManager.markPairedTaskReviewReady).not.toHaveBeenCalled(); + }); + + it('re-triggers review when owner changed code after approval', () => { + const repoDir = createCanonicalRepoWithCommit('reviewed'); + const approvedSourceRef = resolveTreeRef(repoDir); + fs.writeFileSync(path.join(repoDir, 'README.md'), 'changed\n'); + execFileSync('git', ['add', 'README.md'], { cwd: repoDir, stdio: 'ignore' }); + execFileSync('git', ['commit', '-m', 'code change'], { + cwd: repoDir, + stdio: 'ignore', + }); + + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'merge_ready', + source_ref: approvedSourceRef, + round_trip_count: 1, + }), + ); + vi.mocked(db.getPairedWorkspace).mockImplementation((_taskId, role) => + role === 'owner' ? buildWorkspace('owner', repoDir) : undefined, + ); + vi.mocked(pairedWorkspaceManager.markPairedTaskReviewReady).mockReturnValue({ + ownerWorkspace: buildWorkspace('owner', repoDir), + reviewerWorkspace: buildWorkspace('reviewer', '/tmp/paired/task-1/reviewer'), + }); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'owner', + status: 'succeeded', + summary: 'DONE', + }); + + expect(pairedWorkspaceManager.markPairedTaskReviewReady).toHaveBeenCalledWith( + 'task-1', + ); + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + round_trip_count: 2, + review_requested_at: expect.any(String), + }), + ); + }); + + it('records source_ref when reviewer verdict DONE arrives via failed fallback', () => { + const repoDir = createCanonicalRepoWithCommit('reviewed'); + const approvedSourceRef = resolveTreeRef(repoDir); + + vi.mocked(db.getPairedTaskById).mockReturnValue( + buildPairedTask({ + status: 'in_review', + source_ref: 'stale-ref', + }), + ); + vi.mocked(db.getPairedWorkspace).mockImplementation((_taskId, role) => + role === 'owner' ? buildWorkspace('owner', repoDir) : undefined, + ); + + completePairedExecutionContext({ + taskId: 'task-1', + role: 'reviewer', + status: 'failed', + summary: 'DONE', + }); + + expect(db.updatePairedTask).toHaveBeenCalledWith( + 'task-1', + expect.objectContaining({ + status: 'merge_ready', + source_ref: approvedSourceRef, + }), + ); + }); + it('returns ready for /review on the owner service', async () => { const { dbModule, diff --git a/src/paired-execution-context.ts b/src/paired-execution-context.ts index 82addc8..3f1d557 100644 --- a/src/paired-execution-context.ts +++ b/src/paired-execution-context.ts @@ -66,27 +66,45 @@ function classifyReviewerVerdict( // --------------------------------------------------------------------------- function resolveCanonicalSourceRef(workDir: string): string { + const treeHash = resolveCanonicalTreeHash(workDir); + return treeHash || 'HEAD'; +} + +function resolveCanonicalTreeHash(workDir: string): string | null { try { - const head = execFileSync('git', ['rev-parse', 'HEAD'], { + const treeHash = execFileSync('git', ['rev-parse', 'HEAD^{tree}'], { cwd: workDir, encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'], }).trim(); - return head || 'HEAD'; + return treeHash || null; } catch { - return 'HEAD'; + return null; } } -function resolveCanonicalHead(workDir: string): string | null { +function hasCodeChangesSinceRef( + workDir: string, + sourceRef: string | null | undefined, +): boolean | null { + if (!sourceRef) return null; try { - const head = execFileSync('git', ['rev-parse', 'HEAD'], { + execFileSync('git', ['diff', '--quiet', sourceRef, 'HEAD'], { cwd: workDir, - encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'], - }).trim(); - return head || null; - } catch { + }); + return false; + } catch (error) { + const exitCode = + typeof error === 'object' && + error !== null && + 'status' in error && + typeof (error as { status?: unknown }).status === 'number' + ? (error as { status: number }).status + : null; + if (exitCode === 1) { + return true; + } return null; } } @@ -290,12 +308,24 @@ export function completePairedExecutionContext(args: { verdict === 'needs_context' ) { const now = new Date().toISOString(); + const ownerWs = + verdict === 'done' ? getPairedWorkspace(taskId, 'owner') : null; + const approvedSourceRef = + verdict === 'done' && ownerWs?.workspace_dir + ? resolveCanonicalSourceRef(ownerWs.workspace_dir) + : task.source_ref; updatePairedTask(taskId, { status: verdict === 'done' ? 'merge_ready' : 'completed', + ...(verdict === 'done' ? { source_ref: approvedSourceRef } : {}), updated_at: now, }); logger.info( - { taskId, verdict, summary: args.summary?.slice(0, 100) }, + { + taskId, + verdict, + approvedSourceRef, + summary: args.summary?.slice(0, 100), + }, 'Reviewer verdict detected from failed execution — stopping ping-pong', ); return; @@ -320,13 +350,11 @@ export function completePairedExecutionContext(args: { // additional changes that need re-review. if (task.status === 'merge_ready') { const workspace = getPairedWorkspace(task.id, 'owner'); - const currentHead = workspace?.workspace_dir - ? resolveCanonicalHead(workspace.workspace_dir) + const hasNewChanges = workspace?.workspace_dir + ? hasCodeChangesSinceRef(workspace.workspace_dir, task.source_ref) : null; - const snapshotRef = task.source_ref; - const hasNewChanges = currentHead && currentHead !== snapshotRef; - if (!hasNewChanges) { + if (hasNewChanges === false) { // No code changes since reviewer approved → finalize complete updatePairedTask(taskId, { status: 'completed', updated_at: now }); logger.info( @@ -337,7 +365,11 @@ export function completePairedExecutionContext(args: { } // Owner made changes after approval → needs re-review logger.info( - { taskId, previousHead: snapshotRef, currentHead }, + { + taskId, + sourceRef: task.source_ref, + hasNewChanges, + }, 'Owner made changes after reviewer approval — re-triggering review', ); } @@ -380,12 +412,12 @@ export function completePairedExecutionContext(args: { // Record the current HEAD as source_ref so the finalize turn // can detect if the owner made additional code changes. const ownerWs = getPairedWorkspace(taskId, 'owner'); - const approvedHead = ownerWs?.workspace_dir - ? resolveCanonicalHead(ownerWs.workspace_dir) - : null; + const approvedSourceRef = ownerWs?.workspace_dir + ? resolveCanonicalSourceRef(ownerWs.workspace_dir) + : task.source_ref; updatePairedTask(taskId, { status: 'merge_ready', - source_ref: approvedHead ?? task.source_ref, + source_ref: approvedSourceRef, updated_at: now, }); logger.info( @@ -393,6 +425,7 @@ export function completePairedExecutionContext(args: { taskId, verdict, approvedHead, + approvedSourceRef, summary: args.summary?.slice(0, 100), }, 'Reviewer approved — owner gets final turn to finalize',