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 } }