Skip to content

Commit 0221084

Browse files
OAGrclaude
andcommitted
fix(pr-patrol): address CodeRabbit review comments
- Reset failure counter on successful fix (was monotonic/never cleared) - Handle timed-out reflections as incomplete instead of success - Add 'no-op' outcome for stop-early runs (heuristic detection) - No-op runs increment failure counter to avoid infinite retries - 8 new tests for looksLikeNoOp detection Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent a7a01cd commit 0221084

File tree

2 files changed

+121
-17
lines changed

2 files changed

+121
-17
lines changed

crux/pr-patrol/index.test.ts

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
findMergeCandidates,
55
detectIssues,
66
computeBudget,
7+
looksLikeNoOp,
78
type GqlPrNode,
89
} from './index.ts';
910

@@ -472,3 +473,43 @@ describe('detectIssues', () => {
472473
expect(result.issues).toEqual([]);
473474
});
474475
});
476+
477+
// ── looksLikeNoOp ───────────────────────────────────────────────────────────
478+
479+
describe('looksLikeNoOp', () => {
480+
it('detects "no action needed" in output tail', () => {
481+
expect(looksLikeNoOp('Analyzed the issue. No action needed for this PR.')).toBe(true);
482+
});
483+
484+
it('detects "requires human intervention"', () => {
485+
expect(looksLikeNoOp('The check-protected-paths check requires human intervention to add the label.')).toBe(true);
486+
});
487+
488+
it('detects "pre-existing failure"', () => {
489+
expect(looksLikeNoOp('This is a pre-existing failure also present on main.')).toBe(true);
490+
});
491+
492+
it('detects "also failing on main"', () => {
493+
expect(looksLikeNoOp('The CI check is also failing on main, so this is not introduced by this PR.')).toBe(true);
494+
});
495+
496+
it('detects "stopping early"', () => {
497+
expect(looksLikeNoOp('Stopping early because the issue cannot be resolved automatically.')).toBe(true);
498+
});
499+
500+
it('does NOT flag normal fix output', () => {
501+
expect(looksLikeNoOp('Fixed the TypeScript error in src/index.ts. All tests passing now.')).toBe(false);
502+
});
503+
504+
it('does NOT flag output with "no" in unrelated context', () => {
505+
expect(looksLikeNoOp('Added the missing test. No regressions found after running the suite.')).toBe(false);
506+
});
507+
508+
it('only checks last 1000 chars of output', () => {
509+
const longOutput = 'x'.repeat(2000) + 'No action needed.';
510+
expect(looksLikeNoOp(longOutput)).toBe(true);
511+
// Pattern in the first 1000 chars but not the last 1000
512+
const earlyMatch = 'No action needed.' + 'x'.repeat(2000);
513+
expect(looksLikeNoOp(earlyMatch)).toBe(false);
514+
});
515+
});

crux/pr-patrol/index.ts

Lines changed: 80 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ interface ScoredPr extends DetectedPr {
4848
score: number;
4949
}
5050

51-
export type FixOutcome = 'fixed' | 'max-turns' | 'timeout' | 'error' | 'dry-run';
51+
export type FixOutcome = 'fixed' | 'no-op' | 'max-turns' | 'timeout' | 'error' | 'dry-run';
5252

5353
export type MergeOutcome = 'merged' | 'dry-run' | 'error';
5454

@@ -204,6 +204,33 @@ function recordFailure(key: number | string): number {
204204
return count;
205205
}
206206

207+
function resetFailCount(key: number | string): void {
208+
const file = join(STATE_DIR, `failures-${key}`);
209+
if (existsSync(file)) writeFileSync(file, '0');
210+
}
211+
212+
/**
213+
* Detect when Claude exited cleanly but didn't actually fix anything
214+
* (e.g., followed "stop early" guidance for human-required issues).
215+
*/
216+
const NO_OP_PATTERNS = [
217+
/no action needed/i,
218+
/no code changes? needed/i,
219+
/requires? human intervention/i,
220+
/needs? human/i,
221+
/cannot be fixed automatically/i,
222+
/pre-existing.*(failure|issue|problem)/i,
223+
/also failing on main/i,
224+
/stopping early/i,
225+
/nothing to fix/i,
226+
];
227+
228+
export function looksLikeNoOp(output: string): boolean {
229+
// Only check the last portion of output (where the conclusion lives)
230+
const tail = output.slice(-1000);
231+
return NO_OP_PATTERNS.some((p) => p.test(tail));
232+
}
233+
207234
function isAbandoned(key: number | string): boolean {
208235
return getFailCount(key) >= 2;
209236
}
@@ -351,8 +378,16 @@ async function fixMainBranch(status: MainBranchStatus, config: PatrolConfig): Pr
351378
log(`${cl.red}✗ Main branch fix abandoned after ${failCount} failures${cl.reset}`);
352379
}
353380
} else if (result.exitCode === 0 && !result.hitMaxTurns) {
354-
outcome = 'fixed';
355-
log(`${cl.green}✓ Main branch CI fix processed${cl.reset} (${elapsedS}s)`);
381+
const isNoOp = looksLikeNoOp(result.output);
382+
outcome = isNoOp ? 'no-op' : 'fixed';
383+
if (isNoOp) {
384+
recordFailure(MAIN_BRANCH_KEY);
385+
reason = 'No-op: agent determined issue needs human intervention';
386+
log(`${cl.yellow}⚠ Main branch fix no-op — agent stopped early${cl.reset} (${elapsedS}s)`);
387+
} else {
388+
resetFailCount(MAIN_BRANCH_KEY);
389+
log(`${cl.green}✓ Main branch CI fix processed${cl.reset} (${elapsedS}s)`);
390+
}
356391
} else if (result.hitMaxTurns) {
357392
const failCount = recordFailure(MAIN_BRANCH_KEY);
358393
outcome = 'max-turns';
@@ -1323,8 +1358,20 @@ async function fixPr(pr: ScoredPr, config: PatrolConfig): Promise<void> {
13231358
).catch(() => log(' Warning: could not post abandonment comment'));
13241359
}
13251360
} else if (result.exitCode === 0 && !result.hitMaxTurns) {
1326-
log(`${cl.green}✓ PR #${pr.number} processed successfully${cl.reset} (${elapsedS}s)`);
1327-
outcome = 'fixed';
1361+
const isNoOp = looksLikeNoOp(result.output);
1362+
outcome = isNoOp ? 'no-op' : 'fixed';
1363+
1364+
if (isNoOp) {
1365+
// No-op: Claude determined the issue can't be fixed automatically.
1366+
// Don't reset fail count — treat like a soft failure so the PR
1367+
// gets skipped on future cycles instead of being retried forever.
1368+
const failCount = recordFailure(pr.number);
1369+
reason = `No-op: agent determined issue needs human intervention (attempt ${failCount})`;
1370+
log(`${cl.yellow}⚠ PR #${pr.number} no-op — agent stopped early${cl.reset} (${elapsedS}s)`);
1371+
} else {
1372+
resetFailCount(pr.number);
1373+
log(`${cl.green}✓ PR #${pr.number} processed successfully${cl.reset} (${elapsedS}s)`);
1374+
}
13281375

13291376
// Post summary comment
13301377
const summary = result.output.slice(-500);
@@ -1450,19 +1497,35 @@ ${recentEntries}
14501497
timeoutMinutes: 5, // Should complete quickly
14511498
});
14521499
const elapsedS = Math.floor((Date.now() - startTime) / 1000);
1453-
const filedIssue = /Created issue #|created.*#\d/.test(result.output);
1454-
1455-
appendJsonl(REFLECTION_FILE, {
1456-
cycle_number: cycleCount,
1457-
elapsed_s: elapsedS,
1458-
filed_issue: filedIssue,
1459-
exit_code: result.exitCode,
1460-
summary: result.output.slice(-500),
1461-
});
14621500

1463-
log(
1464-
`${cl.green}✓ Reflection complete${cl.reset} (${elapsedS}s, filed_issue=${filedIssue})`,
1465-
);
1501+
if (result.timedOut || result.hitMaxTurns) {
1502+
const reason = result.timedOut ? 'timeout' : 'max-turns';
1503+
appendJsonl(REFLECTION_FILE, {
1504+
cycle_number: cycleCount,
1505+
elapsed_s: elapsedS,
1506+
filed_issue: false,
1507+
exit_code: result.exitCode,
1508+
outcome: 'incomplete',
1509+
reason,
1510+
summary: result.output.slice(-500),
1511+
});
1512+
log(
1513+
`${cl.yellow}⚠ Reflection incomplete${cl.reset} (${elapsedS}s, ${reason})`,
1514+
);
1515+
} else {
1516+
const filedIssue = /Created issue #|created.*#\d/.test(result.output);
1517+
appendJsonl(REFLECTION_FILE, {
1518+
cycle_number: cycleCount,
1519+
elapsed_s: elapsedS,
1520+
filed_issue: filedIssue,
1521+
exit_code: result.exitCode,
1522+
outcome: 'complete',
1523+
summary: result.output.slice(-500),
1524+
});
1525+
log(
1526+
`${cl.green}✓ Reflection complete${cl.reset} (${elapsedS}s, filed_issue=${filedIssue})`,
1527+
);
1528+
}
14661529
} catch (e) {
14671530
const elapsedS = Math.floor((Date.now() - startTime) / 1000);
14681531
log(

0 commit comments

Comments
 (0)