From be63ba56d92a86d5fb36ed5d1636d1d0ff56118e Mon Sep 17 00:00:00 2001 From: "gfargo-horizon-agent[bot]" <294710345+gfargo-horizon-agent[bot]@users.noreply.github.com> Date: Fri, 14 Aug 2026 10:29:20 -0400 Subject: [PATCH] fix(parsers): route summarization failures through the logger (LIB-11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit summarizeFileDiff and summarizeDirectoryDiff both caught LLM errors, logged via bare console.error, and silently returned the unsummarized diff. console.error bypasses --quiet and the run's normal output channel entirely — and the silent fallback is exactly what blows the token budget the summarizer exists to protect, so a run that lost every summarization would go on to trim real content in enforcePromptBudget with no indication why. Both functions already had a logger threaded into their options (summarizeDirectoryDiff's is optional, hence the ?. guard) — routes the failure through logger.error instead, which reaches the run's normal output and survives testing via a mock rather than a console.error spy. --- .../default/utils/summarizeDiffs.test.ts | 59 +++++++++++++++++++ .../parsers/default/utils/summarizeDiffs.ts | 10 +++- .../default/utils/summarizeLargeFiles.test.ts | 34 ++++++++++- .../default/utils/summarizeLargeFiles.ts | 10 +++- 4 files changed, 109 insertions(+), 4 deletions(-) diff --git a/src/lib/parsers/default/utils/summarizeDiffs.test.ts b/src/lib/parsers/default/utils/summarizeDiffs.test.ts index a346b5f9..e1c087d9 100644 --- a/src/lib/parsers/default/utils/summarizeDiffs.test.ts +++ b/src/lib/parsers/default/utils/summarizeDiffs.test.ts @@ -112,6 +112,65 @@ describe('summarizeDirectoryDiff', () => { expect(result.diffs).toEqual(directory.diffs) // Original diffs preserved }) + // LIB-11: this failure used to go to bare console.error, bypassing the + // logger entirely (invisible under --quiet, untestable, and on a + // different channel than the rest of the run's output). + it('reports a directory summarization failure through the logger, not console.error (LIB-11)', async () => { + const summarizeMock = jest.requireMock('../../../langchain/chains/summarize').summarize + summarizeMock.mockRejectedValueOnce(new Error('provider timed out')) + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => undefined) + const mockLogger = { verbose: jest.fn(), error: jest.fn() } + + const directory = { + path: 'src/components', + diffs: [{ file: 'src/components/A.tsx', diff: 'a'.repeat(400), summary: 'A', tokenCount: 100 }], + tokenCount: 100, + } + + const result = await summarizeDirectoryDiff(directory, { + chain: mockChain, + textSplitter: mockTextSplitter, + tokenizer: mockTokenizer, + logger: mockLogger as never, + }) + + // Original directory diff preserved on failure. + expect(result).toEqual(directory) + expect(mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('src/components') + ) + expect(mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('provider timed out') + ) + expect(consoleErrorSpy).not.toHaveBeenCalled() + + consoleErrorSpy.mockRestore() + summarizeMock.mockImplementation(async (docs: unknown[]) => `Summary of ${docs.length} file(s)`) + }) + + it('does not throw when the failure occurs and no logger was supplied', async () => { + const summarizeMock = jest.requireMock('../../../langchain/chains/summarize').summarize + summarizeMock.mockRejectedValueOnce(new Error('provider timed out')) + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => undefined) + + const directory = { + path: 'src/components', + diffs: [{ file: 'src/components/A.tsx', diff: 'a'.repeat(400), summary: 'A', tokenCount: 100 }], + tokenCount: 100, + } + + await expect( + summarizeDirectoryDiff(directory, { + chain: mockChain, + textSplitter: mockTextSplitter, + tokenizer: mockTokenizer, + }) + ).resolves.toEqual(directory) + + consoleErrorSpy.mockRestore() + summarizeMock.mockImplementation(async (docs: unknown[]) => `Summary of ${docs.length} file(s)`) + }) + describe('diff-summary cache hit/miss reporting (#1958)', () => { const summarizeMock = jest.requireMock('../../../langchain/chains/summarize').summarize const cacheHitLogger = { verbose: jest.fn() } diff --git a/src/lib/parsers/default/utils/summarizeDiffs.ts b/src/lib/parsers/default/utils/summarizeDiffs.ts index 6d06b1d3..c2431add 100644 --- a/src/lib/parsers/default/utils/summarizeDiffs.ts +++ b/src/lib/parsers/default/utils/summarizeDiffs.ts @@ -143,7 +143,15 @@ export async function summarizeDirectoryDiff( tokenCount: newTokenTotal, } } catch (error) { - console.error(error) + // On error, return the original directory diff unchanged. Routed + // through the logger (not console.error) so it respects --quiet and + // reaches the same channel the rest of the run's output does — + // silently, this returns the UNSUMMARIZED diff, which is exactly + // what blows the token budget the summarizer exists to protect + // (LIB-11). + logger?.error?.( + `Failed to summarize directory "${directory.path}": ${error instanceof Error ? error.message : String(error)}`, + ) return directory } } diff --git a/src/lib/parsers/default/utils/summarizeLargeFiles.test.ts b/src/lib/parsers/default/utils/summarizeLargeFiles.test.ts index 449bc3a4..ca8cfe03 100644 --- a/src/lib/parsers/default/utils/summarizeLargeFiles.test.ts +++ b/src/lib/parsers/default/utils/summarizeLargeFiles.test.ts @@ -27,6 +27,7 @@ describe('summarizeLargeFiles', () => { const mockLogger = { verbose: jest.fn().mockReturnThis(), log: jest.fn().mockReturnThis(), + error: jest.fn().mockReturnThis(), startSpinner: jest.fn().mockReturnThis(), stopSpinner: jest.fn().mockReturnThis(), startTimer: jest.fn().mockReturnThis(), @@ -382,7 +383,6 @@ describe('summarizeLargeFiles', () => { }) it('keeps the raw diff when summarize() rejects with an empty-summary error (#1700)', async () => { - const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => undefined) mockSummarize.mockRejectedValueOnce( new Error('summarize: chain returned an empty summary') ) @@ -408,6 +408,36 @@ describe('summarizeLargeFiles', () => { // Raw diff preserved, not overwritten with an empty summary. expect(result[0]).toEqual(diffs[0]) + }) + + // LIB-11: this failure used to go to bare console.error, bypassing the + // logger entirely (invisible under --quiet, untestable, and on a + // different channel than the rest of the run's output). + it('reports a per-file summarization failure through the logger, not console.error (LIB-11)', async () => { + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => undefined) + mockSummarize.mockRejectedValueOnce(new Error('provider timed out')) + + const diffs: FileDiff[] = [ + { file: 'large.ts', diff: 'a'.repeat(2000), summary: 'large.ts', tokenCount: 600 }, + ] + + await summarizeLargeFiles(diffs, { + maxFileTokens: 500, + minTokensForSummary: 400, + maxConcurrent: 4, + tokenizer: mockTokenizer, + logger: mockLogger as never, + chain: mockChain, + textSplitter: mockTextSplitter, + }) + + expect(mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('large.ts') + ) + expect(mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('provider timed out') + ) + expect(consoleErrorSpy).not.toHaveBeenCalled() consoleErrorSpy.mockRestore() }) @@ -442,6 +472,7 @@ describe('diff-summary cache hit/miss reporting (#1958)', () => { const mockLogger = { verbose: jest.fn().mockReturnThis(), log: jest.fn().mockReturnThis(), + error: jest.fn().mockReturnThis(), startSpinner: jest.fn().mockReturnThis(), stopSpinner: jest.fn().mockReturnThis(), startTimer: jest.fn().mockReturnThis(), @@ -551,6 +582,7 @@ describe('preprocessLargeFiles', () => { const mockLogger = { verbose: jest.fn().mockReturnThis(), log: jest.fn().mockReturnThis(), + error: jest.fn().mockReturnThis(), startSpinner: jest.fn().mockReturnThis(), stopSpinner: jest.fn().mockReturnThis(), startTimer: jest.fn().mockReturnThis(), diff --git a/src/lib/parsers/default/utils/summarizeLargeFiles.ts b/src/lib/parsers/default/utils/summarizeLargeFiles.ts index a25469ef..d30959af 100644 --- a/src/lib/parsers/default/utils/summarizeLargeFiles.ts +++ b/src/lib/parsers/default/utils/summarizeLargeFiles.ts @@ -309,8 +309,14 @@ async function summarizeFileDiff( tokenCount: newTokenCount, } } catch (error) { - // On error, return original diff unchanged - console.error(`Failed to summarize file ${fileDiff.file}:`, error) + // On error, return original diff unchanged. Routed through the + // logger (not console.error) so it respects --quiet and reaches the + // same channel the rest of the run's output does — silently, this + // returns the UNSUMMARIZED diff, which is exactly what blows the + // token budget the summarizer exists to protect (LIB-11). + logger.error( + `Failed to summarize file ${fileDiff.file}: ${error instanceof Error ? error.message : String(error)}`, + ) return fileDiff } }