claude-bot/EJClaw
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

refactor: use tree hash + git diff for post-approval change detection

Browse Source 
- 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
This commit is contained in:
Eyejoker
2026-03-29 23:23:30 +09:00
parent 2e4de9cca8
commit 504be9e41a
3 changed files with 213 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
runners/codex-runner/src/reviewer-runtime.ts
View File

@@ -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',

158
src/paired-execution-context.test.ts
View File

@@ -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> = {}): 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,

73
src/paired-execution-context.ts
View File

@@ -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',