From 418e5699a4d01e0742b82882a3193887492748af Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Wed, 18 Mar 2026 19:28:57 +0000 Subject: [PATCH] fix(agents): add observability to work-item image download in fetchWorkItemStep Silent null returns from downloadAttachment were completely invisible in run logs, making image delivery failures impossible to diagnose. Add INFO logs before and after the parallel download, plus a WARN when a download returns null (with query params stripped from the URL to avoid leaking Trello API credentials). Also fix a pre-existing bug where the exception-path WARN logged the full URL including query-param credentials, and correct a stray lowercase 'warn' log level in builderFactory.ts to match the codebase-wide 'WARN' convention. Co-Authored-By: Claude Sonnet 4.6 --- src/agents/definitions/contextSteps.ts | 20 ++++++- src/agents/shared/builderFactory.ts | 2 +- .../agents/definitions/contextSteps.test.ts | 57 +++++++++++++++++-- 3 files changed, 71 insertions(+), 8 deletions(-) diff --git a/src/agents/definitions/contextSteps.ts b/src/agents/definitions/contextSteps.ts index 858fef0b..78e9623a 100644 --- a/src/agents/definitions/contextSteps.ts +++ b/src/agents/definitions/contextSteps.ts @@ -124,6 +124,11 @@ export async function fetchWorkItemStep(params: FetchContextParams): Promise r !== null); + params.logWriter('INFO', 'fetchWorkItemStep: image download complete', { + workItemId: params.input.workItemId, + attempted: limited.length, + downloaded: images.length, + skipped: limited.length - images.length, + }); if (images.length > 0) { injection.images = images; } diff --git a/src/agents/shared/builderFactory.ts b/src/agents/shared/builderFactory.ts index 78f9d5bb..ebe9a5b7 100644 --- a/src/agents/shared/builderFactory.ts +++ b/src/agents/shared/builderFactory.ts @@ -144,7 +144,7 @@ export async function createConfiguredBuilder(options: CreateBuilderOptions): Pr builder = builder.withBudget(remainingBudgetUsd); } catch (err) { if (err instanceof BudgetPricingUnavailableError) { - logWriter('warn', 'Budget enforcement unavailable for model', { model }); + logWriter('WARN', 'Budget enforcement unavailable for model', { model }); } else { throw err; } diff --git a/tests/unit/agents/definitions/contextSteps.test.ts b/tests/unit/agents/definitions/contextSteps.test.ts index 96998075..86602498 100644 --- a/tests/unit/agents/definitions/contextSteps.test.ts +++ b/tests/unit/agents/definitions/contextSteps.test.ts @@ -275,12 +275,16 @@ describe('fetchWorkItemStep', () => { expect(mockTrelloDownload).not.toHaveBeenCalled(); }); - it('skips failed downloads gracefully and logs warning', async () => { + it('logs WARN and skips when download returns null, stripping query params from URL', async () => { mockReadWorkItemWithMedia.mockResolvedValue({ text: '# Card', media: [ { url: 'https://trello.com/ok.png', mimeType: 'image/png', source: 'description' }, - { url: 'https://trello.com/fail.png', mimeType: 'image/png', source: 'description' }, + { + url: 'https://trello.com/fail.png?key=secret&token=abc', + mimeType: 'image/png', + source: 'description', + }, ], }); mockGetPMProviderOrNull.mockReturnValue({ type: 'trello' } as never); @@ -294,12 +298,25 @@ describe('fetchWorkItemStep', () => { // Only 1 successful image expect(result[0].images).toHaveLength(1); expect(result[0].images?.[0].base64Data).toBe(Buffer.from('ok').toString('base64')); + + // WARN for the null return, URL sanitized (no query params) + expect(params.logWriter).toHaveBeenCalledWith( + 'WARN', + 'fetchWorkItemStep: image download returned null', + { url: 'https://trello.com/fail.png' }, + ); }); - it('logs warning when download throws an exception', async () => { + it('logs warning when download throws an exception, stripping query params from URL', async () => { mockReadWorkItemWithMedia.mockResolvedValue({ text: '# Card', - media: [{ url: 'https://trello.com/err.png', mimeType: 'image/png', source: 'description' }], + media: [ + { + url: 'https://trello.com/err.png?key=secret&token=abc', + mimeType: 'image/png', + source: 'description', + }, + ], }); mockGetPMProviderOrNull.mockReturnValue({ type: 'trello' } as never); mockTrelloDownload.mockRejectedValue(new Error('network failure')); @@ -311,7 +328,37 @@ describe('fetchWorkItemStep', () => { expect(params.logWriter).toHaveBeenCalledWith( 'WARN', 'fetchWorkItemStep: failed to download image', - expect.objectContaining({ error: 'network failure' }), + { url: 'https://trello.com/err.png', error: 'network failure' }, + ); + }); + + it('emits INFO logs before and after download with correct counts', async () => { + mockReadWorkItemWithMedia.mockResolvedValue({ + text: '# Card', + media: [ + { url: 'https://trello.com/a.png', mimeType: 'image/png', source: 'description' }, + { url: 'https://trello.com/b.png', mimeType: 'image/png', source: 'description' }, + { url: 'https://trello.com/c.png', mimeType: 'image/png', source: 'description' }, + ], + }); + mockGetPMProviderOrNull.mockReturnValue({ type: 'trello' } as never); + mockTrelloDownload + .mockResolvedValueOnce({ buffer: Buffer.from('data'), mimeType: 'image/png' }) + .mockResolvedValueOnce(null) // returns null + .mockRejectedValueOnce(new Error('timeout')); // throws + + const params = makeParams({ workItemId: 'card-1' }); + await fetchWorkItemStep(params); + + expect(params.logWriter).toHaveBeenCalledWith( + 'INFO', + 'fetchWorkItemStep: downloading work item images', + { workItemId: 'card-1', count: 3 }, + ); + expect(params.logWriter).toHaveBeenCalledWith( + 'INFO', + 'fetchWorkItemStep: image download complete', + { workItemId: 'card-1', attempted: 3, downloaded: 1, skipped: 2 }, ); });