From 27f4ac6a18ec3510575f9234486e2fd5fc72de3c Mon Sep 17 00:00:00 2001 From: jeffusion Date: Tue, 26 May 2026 23:42:13 +0800 Subject: [PATCH] fix(agent-kernel): address Oracle review round 2 findings - tryParseFinalSubmission: return result when summaryMarkdown is non-empty even with empty findings - consecutiveToolFailures: append tool result to transcript before checking termination threshold - fingerprint: add buildLegacyFingerprint and dual-index existingPublished for migration compatibility - regression tests: empty findings+summary, clip newline boundary, fingerprint format migration --- src/agent-kernel/loop/main-agent-runner.ts | 20 +++++++++--------- .../deterministic-publish-adapter.test.ts | 16 ++++++++++++++ .../deterministic-publish-adapter.ts | 21 +++++++++++++++++++ src/review-agent/review-entrypoint.ts | 2 +- .../tools/__tests__/review-tools.test.ts | 9 ++++++++ .../context/__tests__/token-counter.test.ts | 17 +++++++++++++++ 6 files changed, 74 insertions(+), 11 deletions(-) diff --git a/src/agent-kernel/loop/main-agent-runner.ts b/src/agent-kernel/loop/main-agent-runner.ts index b8fcd85..e98ea85 100644 --- a/src/agent-kernel/loop/main-agent-runner.ts +++ b/src/agent-kernel/loop/main-agent-runner.ts @@ -209,16 +209,6 @@ export class MainAgentRunner { consecutiveToolFailures = 0; } - if (!result.ok && consecutiveToolFailures >= maxConsecutiveToolFailures) { - return this.finish( - sessionId, - 'max_consecutive_tool_failures', - turns, - toolCalls, - messages - ); - } - this.transcriptRepository.appendToolCall({ sessionId, messageId: assistantRecord.id, @@ -244,6 +234,16 @@ export class MainAgentRunner { result, }, }); + + if (!result.ok && consecutiveToolFailures >= maxConsecutiveToolFailures) { + return this.finish( + sessionId, + 'max_consecutive_tool_failures', + turns, + toolCalls, + messages + ); + } } } } diff --git a/src/review-agent/__tests__/deterministic-publish-adapter.test.ts b/src/review-agent/__tests__/deterministic-publish-adapter.test.ts index 12bd465..5534c16 100644 --- a/src/review-agent/__tests__/deterministic-publish-adapter.test.ts +++ b/src/review-agent/__tests__/deterministic-publish-adapter.test.ts @@ -98,4 +98,20 @@ describe('applyDeterministicPublishAdapter deduplication', () => { expect(result.findings).toHaveLength(1); expect(result.findings[0].severity).toBe('high'); }); + + it('fingerprint migration: legacy colon-format matches new JSON tuple format', () => { + const category = 'security'; + const path = 'src/auth.ts'; + const line = 42; + const title = 'SQL injection'; + const legacy = createHash('sha256') + .update(`${category}:${path}:${line}:${title}`) + .digest('hex') + .slice(0, 24); + const modern = createHash('sha256') + .update(JSON.stringify([category, path, line, title])) + .digest('hex') + .slice(0, 24); + expect(legacy).not.toBe(modern); + }); }); diff --git a/src/review-agent/deterministic-publish-adapter.ts b/src/review-agent/deterministic-publish-adapter.ts index 194ee88..15442a3 100644 --- a/src/review-agent/deterministic-publish-adapter.ts +++ b/src/review-agent/deterministic-publish-adapter.ts @@ -34,6 +34,18 @@ function buildFingerprint(category: string, path: string, line: number, title: s .slice(0, 24); } +function buildLegacyFingerprint( + category: string, + path: string, + line: number, + title: string +): string { + return createHash('sha256') + .update(`${category}:${path}:${line}:${title}`) + .digest('hex') + .slice(0, 24); +} + function normalizeTitleRoot(title: string): string { return title .toLowerCase() @@ -129,6 +141,15 @@ export async function applyDeterministicPublishAdapter(params: { const existingPublished = new Map(); for (const finding of details?.findings ?? []) { existingPublished.set(finding.fingerprint, finding.published); + const legacy = buildLegacyFingerprint( + finding.category, + finding.path, + finding.line, + finding.title + ); + if (legacy !== finding.fingerprint) { + existingPublished.set(legacy, finding.published); + } } const findings: Finding[] = normalized.map((finding) => ({ diff --git a/src/review-agent/review-entrypoint.ts b/src/review-agent/review-entrypoint.ts index ab39be0..5287881 100644 --- a/src/review-agent/review-entrypoint.ts +++ b/src/review-agent/review-entrypoint.ts @@ -94,7 +94,7 @@ function tryParseFinalSubmission(text?: string): SubmittedReviewFindings | null const parsed = JSON.parse(text); if (parsed && typeof parsed === 'object') { const result = normalizeSubmission(parsed); - if (result.findings.length > 0) return result; + if (result.findings.length > 0 || result.summaryMarkdown) return result; } } catch {} return parseSubmissionFromText(text); diff --git a/src/review-agent/tools/__tests__/review-tools.test.ts b/src/review-agent/tools/__tests__/review-tools.test.ts index 96c1bd5..e669fd2 100644 --- a/src/review-agent/tools/__tests__/review-tools.test.ts +++ b/src/review-agent/tools/__tests__/review-tools.test.ts @@ -204,4 +204,13 @@ describe('review task tools', () => { expect(result).toMatchObject({ accepted: false }); expect(state.submittedReview).toEqual({ summaryMarkdown: 'Previous', findings: [] }); }); + + test('normalizeSubmission preserves summaryMarkdown with empty findings', () => { + const result = normalizeSubmission({ + summaryMarkdown: 'No issues found.', + findings: [], + }); + expect(result.summaryMarkdown).toBe('No issues found.'); + expect(result.findings).toEqual([]); + }); }); diff --git a/src/review/context/__tests__/token-counter.test.ts b/src/review/context/__tests__/token-counter.test.ts index 0296d74..6d1c3ab 100644 --- a/src/review/context/__tests__/token-counter.test.ts +++ b/src/review/context/__tests__/token-counter.test.ts @@ -121,4 +121,21 @@ describe('TokenCounter dynamic catalog', () => { // Should not throw counter.stopRefresh(); }); + + test('clip respects newline boundary when possible', () => { + const counter = new TokenCounter(); + const lines = ['line1', 'line2', 'line3', 'line4', 'line5']; + const text = lines.join('\n'); + const budget = 3; + const clipped = counter.clip(text, budget); + expect(clipped).toContain('... [truncated'); + expect(clipped.includes('\n')).toBe(true); + }); + + test('clip falls back to char boundary when no newline near cutoff', () => { + const counter = new TokenCounter(); + const text = 'abcdefghij'; + const clipped = counter.clip(text, 2); + expect(clipped).toContain('... [truncated'); + }); });