Skip to content

Commit 6df1719

Browse files
fix(review): address round-2 review issues
- Remove unused `cwd` param from reviewDiffWithAgent() - Fix JSON extraction: use indexOf/lastIndexOf instead of regex (handles braces in strings correctly) - Add task.error message when review rejects merge - Add review_started event assertions to scheduler tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 07135d0 commit 6df1719

4 files changed

Lines changed: 15 additions & 12 deletions

File tree

src/__tests__/agent-runner.test.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -423,7 +423,6 @@ describe("AgentRunner", () => {
423423
const result = await runner.reviewDiffWithAgent(
424424
"diff --git a/foo.test.ts b/foo.test.ts\n+it('works', () => {});",
425425
"claude", // task was by claude → review by codex → codex fails → fallback
426-
"/tmp",
427426
2, // 2 second timeout to keep test fast
428427
);
429428
assert.strictEqual(typeof result.approve, "boolean");

src/__tests__/scheduler.test.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -897,8 +897,11 @@ describe("Scheduler", () => {
897897
assert.ok(task.review);
898898
assert.strictEqual(task.review!.approve, false);
899899
assert.strictEqual(task.review!.score, 25);
900-
// Should have emitted review_rejected event
901-
assert.ok(events.some(e => e.type === "review_rejected"));
900+
// Error should explain why merge was blocked
901+
assert.ok(task.error.includes("review rejected"), "error should explain rejection");
902+
// Should have emitted review_started and review_rejected events
903+
assert.ok(events.some(e => e.type === "review_started"), "should emit review_started");
904+
assert.ok(events.some(e => e.type === "review_rejected"), "should emit review_rejected");
902905
});
903906

904907
it("review gate allows merge when review approves", async () => {
@@ -942,7 +945,8 @@ describe("Scheduler", () => {
942945
assert.strictEqual(mergeCalledWith, true);
943946
assert.ok(task.review);
944947
assert.strictEqual(task.review!.approve, true);
945-
assert.ok(events.some(e => e.type === "review_approved"));
948+
assert.ok(events.some(e => e.type === "review_started"), "should emit review_started");
949+
assert.ok(events.some(e => e.type === "review_approved"), "should emit review_approved");
946950
});
947951

948952
it("skips review when task has no diff", async () => {

src/agent-runner.ts

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,6 @@ export class AgentRunner {
112112
async reviewDiffWithAgent(
113113
diff: string,
114114
taskAgent: string,
115-
cwd: string,
116115
timeout: number = 60,
117116
): Promise<ReviewResult> {
118117
const reviewAgent = AgentRunner.pickReviewAgent(taskAgent);
@@ -194,12 +193,12 @@ export class AgentRunner {
194193
}
195194
} catch { /* not pure JSON */ }
196195

197-
// Try to find JSON object in the output (agent may wrap it in text)
198-
// Use [^{}]* to avoid greedily matching across multiple JSON objects
199-
const candidates = [...output.matchAll(/\{[^{}]*"approve"\s*:\s*(true|false)[^{}]*\}/g)];
200-
for (const match of candidates) {
196+
// Try to extract JSON by finding the outermost { } that contains "approve"
197+
const start = output.indexOf("{");
198+
const end = output.lastIndexOf("}");
199+
if (start !== -1 && end > start) {
201200
try {
202-
const obj = JSON.parse(match[0]);
201+
const obj = JSON.parse(output.slice(start, end + 1));
203202
if (typeof obj.approve === "boolean" && typeof obj.score === "number") {
204203
return {
205204
approve: obj.approve,
@@ -208,7 +207,7 @@ export class AgentRunner {
208207
suggestions: Array.isArray(obj.suggestions) ? obj.suggestions.map(String) : [],
209208
};
210209
}
211-
} catch { /* try next candidate */ }
210+
} catch { /* not valid JSON */ }
212211
}
213212

214213
return null;

src/scheduler.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -480,10 +480,11 @@ export class Scheduler {
480480
const taskAgent = task.agent ?? "claude";
481481
const reviewAgent = AgentRunner.pickReviewAgent(taskAgent);
482482
this.onEvent?.({ type: "review_started", taskId: task.id, reviewAgent });
483-
const review = await this.runner.reviewDiffWithAgent(diff, taskAgent, workerPath);
483+
const review = await this.runner.reviewDiffWithAgent(diff, taskAgent);
484484
task.review = review;
485485
if (!review.approve) {
486486
shouldMerge = false;
487+
task.error = `review rejected (score ${review.score}): ${review.issues.join("; ")}`;
487488
log("info", "cross-agent review rejected merge", {
488489
taskId: task.id,
489490
score: review.score,

0 commit comments

Comments
 (0)