From acfdf92c5d1e2b6caa54e574254a38316c6177ce Mon Sep 17 00:00:00 2001 From: Yuhan Lei Date: Tue, 24 Mar 2026 17:46:15 +0800 Subject: [PATCH 1/6] chore: ignore worktree directory --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index c017617b..224d549d 100644 --- a/.gitignore +++ b/.gitignore @@ -22,3 +22,4 @@ docs/.vitepress/cache # Database files *.db +.worktrees/ From f1ccd8703ad3fb0d7e29981c021a259e3d140b13 Mon Sep 17 00:00:00 2001 From: Yuhan Lei Date: Tue, 24 Mar 2026 22:53:37 +0800 Subject: [PATCH 2/6] fix(pipeline): check HTTP status in fetch step --- src/pipeline/steps/fetch.test.ts | 72 ++++++++++++++++++++++++++++++++ src/pipeline/steps/fetch.ts | 9 ++++ 2 files changed, 81 insertions(+) create mode 100644 src/pipeline/steps/fetch.test.ts diff --git a/src/pipeline/steps/fetch.test.ts b/src/pipeline/steps/fetch.test.ts new file mode 100644 index 00000000..f65e2c51 --- /dev/null +++ b/src/pipeline/steps/fetch.test.ts @@ -0,0 +1,72 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import type { IPage } from '../../types.js'; +import { stepFetch } from './fetch.js'; + +afterEach(() => { + vi.restoreAllMocks(); + vi.unstubAllGlobals(); +}); + +describe('stepFetch', () => { + it('throws on non-ok HTTP responses without a browser session', async () => { + const jsonMock = vi.fn().mockResolvedValue({ error: 'rate limited' }); + const fetchMock = vi.fn().mockResolvedValue({ + ok: false, + status: 429, + statusText: 'Too Many Requests', + json: jsonMock, + }); + vi.stubGlobal('fetch', fetchMock); + + await expect(stepFetch(null, { url: 'https://api.example.com/items' }, null, {})).rejects.toThrow( + 'HTTP 429 Too Many Requests', + ); + expect(jsonMock).not.toHaveBeenCalled(); + }); + + it('throws on non-ok HTTP responses inside the browser session', async () => { + const jsonMock = vi.fn().mockResolvedValue({ error: 'auth required' }); + const fetchMock = vi.fn().mockResolvedValue({ + ok: false, + status: 401, + statusText: 'Unauthorized', + json: jsonMock, + }); + vi.stubGlobal('fetch', fetchMock); + + // Execute the injected browser-side function so the test observes the real fetch logic. + const page = { + evaluate: vi.fn(async (js: string) => Function(`return (${js})`)()()), + } as unknown as IPage; + + await expect(stepFetch(page, { url: 'https://api.example.com/items' }, null, {})).rejects.toThrow( + 'HTTP 401 Unauthorized', + ); + expect(jsonMock).not.toHaveBeenCalled(); + }); + + it('returns per-item HTTP errors for batch browser fetches', async () => { + const jsonMock = vi.fn().mockResolvedValue({ error: 'upstream unavailable' }); + const fetchMock = vi.fn().mockResolvedValue({ + ok: false, + status: 503, + statusText: 'Service Unavailable', + json: jsonMock, + }); + vi.stubGlobal('fetch', fetchMock); + + const page = { + evaluate: vi.fn(async (js: string) => Function(`return (${js})`)()()), + } as unknown as IPage; + + await expect(stepFetch( + page, + { url: 'https://api.example.com/items/${{ item.id }}' }, + [{ id: 1 }], + {}, + )).resolves.toEqual([ + { error: 'HTTP 503 Service Unavailable from https://api.example.com/items/1' }, + ]); + expect(jsonMock).not.toHaveBeenCalled(); + }); +}); diff --git a/src/pipeline/steps/fetch.ts b/src/pipeline/steps/fetch.ts index a771bb35..72254821 100644 --- a/src/pipeline/steps/fetch.ts +++ b/src/pipeline/steps/fetch.ts @@ -28,6 +28,9 @@ async function fetchSingle( if (page === null) { const resp = await fetch(finalUrl, { method: method.toUpperCase(), headers: renderedHeaders }); + if (!resp.ok) { + throw new Error(`HTTP ${resp.status} ${resp.statusText} from ${finalUrl}`); + } return resp.json(); } @@ -39,6 +42,9 @@ async function fetchSingle( const resp = await fetch(${urlJs}, { method: ${methodJs}, headers: ${headersJs}, credentials: "include" }); + if (!resp.ok) { + throw new Error('HTTP ' + resp.status + ' ' + resp.statusText + ' from ' + ${urlJs}); + } return await resp.json(); } `); @@ -71,6 +77,9 @@ async function fetchBatchInBrowser( const i = idx++; try { const resp = await fetch(urls[i], { method, headers, credentials: "include" }); + if (!resp.ok) { + throw new Error('HTTP ' + resp.status + ' ' + resp.statusText + ' from ' + urls[i]); + } results[i] = await resp.json(); } catch (e) { results[i] = { error: e.message }; From 74188cacfe32645eaa3b4e8c52b653ac99ae24d0 Mon Sep 17 00:00:00 2001 From: Yuhan Lei Date: Tue, 24 Mar 2026 23:40:25 +0800 Subject: [PATCH 3/6] fix(pipeline): align fetch error semantics --- src/pipeline/steps/fetch.test.ts | 57 ++++++++++++++++++++++++++++++-- src/pipeline/steps/fetch.ts | 9 +++-- 2 files changed, 61 insertions(+), 5 deletions(-) diff --git a/src/pipeline/steps/fetch.test.ts b/src/pipeline/steps/fetch.test.ts index f65e2c51..82d0b0c7 100644 --- a/src/pipeline/steps/fetch.test.ts +++ b/src/pipeline/steps/fetch.test.ts @@ -39,9 +39,30 @@ describe('stepFetch', () => { evaluate: vi.fn(async (js: string) => Function(`return (${js})`)()()), } as unknown as IPage; - await expect(stepFetch(page, { url: 'https://api.example.com/items' }, null, {})).rejects.toThrow( - 'HTTP 401 Unauthorized', - ); + await expect(stepFetch(page, { url: 'https://api.example.com/items' }, null, {})).rejects.toMatchObject({ + message: 'HTTP 401 Unauthorized from https://api.example.com/items', + }); + expect(jsonMock).not.toHaveBeenCalled(); + }); + + it('returns per-item HTTP errors for batch fetches without a browser session', async () => { + const jsonMock = vi.fn().mockResolvedValue({ error: 'upstream unavailable' }); + const fetchMock = vi.fn().mockResolvedValue({ + ok: false, + status: 503, + statusText: 'Service Unavailable', + json: jsonMock, + }); + vi.stubGlobal('fetch', fetchMock); + + await expect(stepFetch( + null, + { url: 'https://api.example.com/items/${{ item.id }}' }, + [{ id: 1 }], + {}, + )).resolves.toEqual([ + { error: 'HTTP 503 Service Unavailable from https://api.example.com/items/1' }, + ]); expect(jsonMock).not.toHaveBeenCalled(); }); @@ -69,4 +90,34 @@ describe('stepFetch', () => { ]); expect(jsonMock).not.toHaveBeenCalled(); }); + + it('stringifies non-Error batch browser failures consistently', async () => { + vi.stubGlobal('fetch', vi.fn().mockRejectedValue('socket hang up')); + + const page = { + evaluate: vi.fn(async (js: string) => Function(`return (${js})`)()()), + } as unknown as IPage; + + await expect(stepFetch( + page, + { url: 'https://api.example.com/items/${{ item.id }}' }, + [{ id: 1 }], + {}, + )).resolves.toEqual([ + { error: 'socket hang up' }, + ]); + }); + + it('stringifies non-Error batch non-browser failures consistently', async () => { + vi.stubGlobal('fetch', vi.fn().mockRejectedValue('socket hang up')); + + await expect(stepFetch( + null, + { url: 'https://api.example.com/items/${{ item.id }}' }, + [{ id: 1 }], + {}, + )).resolves.toEqual([ + { error: 'socket hang up' }, + ]); + }); }); diff --git a/src/pipeline/steps/fetch.ts b/src/pipeline/steps/fetch.ts index 72254821..9acd4c96 100644 --- a/src/pipeline/steps/fetch.ts +++ b/src/pipeline/steps/fetch.ts @@ -82,7 +82,8 @@ async function fetchBatchInBrowser( } results[i] = await resp.json(); } catch (e) { - results[i] = { error: e.message }; + const message = e instanceof Error ? e.message : String(e); + results[i] = { error: message }; } } } @@ -129,7 +130,11 @@ export async function stepFetch(page: IPage | null, params: unknown, data: unkno // Non-browser: use concurrent pool (already optimized) return mapConcurrent(data, concurrency, async (item, index) => { const itemUrl = String(render(urlTemplate, { args, data, item, index })); - return fetchSingle(null, itemUrl, method, queryParams, headers, args, data); + try { + return await fetchSingle(null, itemUrl, method, queryParams, headers, args, data); + } catch (error) { + return { error: error instanceof Error ? error.message : String(error) }; + } }); } const url = render(urlOrObj, { args, data }); From c466e6e515fdf5596483e6fa4b8687e2a1e906f5 Mon Sep 17 00:00:00 2001 From: Yuhan Lei Date: Wed, 25 Mar 2026 01:19:04 +0800 Subject: [PATCH 4/6] fix(pipeline): use CliError and add warn logging in fetch step - Replace bare Error with CliError('FETCH_ERROR') for consistent CLI output - Return error status from browser evaluate instead of throwing inside it - Add log.warn() for batch item failures in both browser and non-browser paths --- src/pipeline/steps/fetch.test.ts | 74 ++++++++++++++++++++++++++++---- src/pipeline/steps/fetch.ts | 27 +++++++++--- 2 files changed, 87 insertions(+), 14 deletions(-) diff --git a/src/pipeline/steps/fetch.test.ts b/src/pipeline/steps/fetch.test.ts index 82d0b0c7..8b97b313 100644 --- a/src/pipeline/steps/fetch.test.ts +++ b/src/pipeline/steps/fetch.test.ts @@ -1,4 +1,5 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; +import { CliError } from '../../errors.js'; import type { IPage } from '../../types.js'; import { stepFetch } from './fetch.js'; @@ -8,7 +9,8 @@ afterEach(() => { }); describe('stepFetch', () => { - it('throws on non-ok HTTP responses without a browser session', async () => { + // W1 + W4: non-browser single fetch throws CliError with FETCH_ERROR code and full message + it('throws CliError with FETCH_ERROR code on non-ok responses without a browser session', async () => { const jsonMock = vi.fn().mockResolvedValue({ error: 'rate limited' }); const fetchMock = vi.fn().mockResolvedValue({ ok: false, @@ -18,13 +20,15 @@ describe('stepFetch', () => { }); vi.stubGlobal('fetch', fetchMock); - await expect(stepFetch(null, { url: 'https://api.example.com/items' }, null, {})).rejects.toThrow( - 'HTTP 429 Too Many Requests', - ); + const err = await stepFetch(null, { url: 'https://api.example.com/items' }, null, {}).catch((e: unknown) => e); + expect(err).toBeInstanceOf(CliError); + expect((err as CliError).code).toBe('FETCH_ERROR'); + expect((err as CliError).message).toBe('HTTP 429 Too Many Requests from https://api.example.com/items'); expect(jsonMock).not.toHaveBeenCalled(); }); - it('throws on non-ok HTTP responses inside the browser session', async () => { + // W1 + W3: browser single fetch returns error status from evaluate, outer code throws CliError + it('throws CliError with FETCH_ERROR code on non-ok responses inside the browser session', async () => { const jsonMock = vi.fn().mockResolvedValue({ error: 'auth required' }); const fetchMock = vi.fn().mockResolvedValue({ ok: false, @@ -34,14 +38,15 @@ describe('stepFetch', () => { }); vi.stubGlobal('fetch', fetchMock); - // Execute the injected browser-side function so the test observes the real fetch logic. + // Simulate real CDP behavior: evaluate returns a value, errors are thrown outside const page = { evaluate: vi.fn(async (js: string) => Function(`return (${js})`)()()), } as unknown as IPage; - await expect(stepFetch(page, { url: 'https://api.example.com/items' }, null, {})).rejects.toMatchObject({ - message: 'HTTP 401 Unauthorized from https://api.example.com/items', - }); + const err = await stepFetch(page, { url: 'https://api.example.com/items' }, null, {}).catch((e: unknown) => e); + expect(err).toBeInstanceOf(CliError); + expect((err as CliError).code).toBe('FETCH_ERROR'); + expect((err as CliError).message).toBe('HTTP 401 Unauthorized from https://api.example.com/items'); expect(jsonMock).not.toHaveBeenCalled(); }); @@ -120,4 +125,55 @@ describe('stepFetch', () => { { error: 'socket hang up' }, ]); }); + + // W2: batch item failures emit a warning log + it('logs a warning for each failed batch item in non-browser mode', async () => { + const { log } = await import('../../logger.js'); + const warnSpy = vi.spyOn(log, 'warn'); + + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ + ok: false, + status: 503, + statusText: 'Service Unavailable', + json: vi.fn(), + })); + + await stepFetch( + null, + { url: 'https://api.example.com/items/${{ item.id }}' }, + [{ id: 1 }, { id: 2 }], + {}, + ); + + expect(warnSpy).toHaveBeenCalledTimes(2); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('https://api.example.com/items/1')); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('https://api.example.com/items/2')); + }); + + it('logs a warning for each failed batch item in browser mode', async () => { + const { log } = await import('../../logger.js'); + const warnSpy = vi.spyOn(log, 'warn'); + + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ + ok: false, + status: 502, + statusText: 'Bad Gateway', + json: vi.fn(), + })); + + const page = { + evaluate: vi.fn(async (js: string) => Function(`return (${js})`)()()), + } as unknown as IPage; + + await stepFetch( + page, + { url: 'https://api.example.com/items/${{ item.id }}' }, + [{ id: 1 }, { id: 2 }], + {}, + ); + + expect(warnSpy).toHaveBeenCalledTimes(2); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('https://api.example.com/items/1')); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('https://api.example.com/items/2')); + }); }); diff --git a/src/pipeline/steps/fetch.ts b/src/pipeline/steps/fetch.ts index 9acd4c96..b88ad797 100644 --- a/src/pipeline/steps/fetch.ts +++ b/src/pipeline/steps/fetch.ts @@ -2,6 +2,8 @@ * Pipeline step: fetch — HTTP API requests. */ +import { CliError } from '../../errors.js'; +import { log } from '../../logger.js'; import type { IPage } from '../../types.js'; import { render } from '../template.js'; @@ -29,7 +31,7 @@ async function fetchSingle( if (page === null) { const resp = await fetch(finalUrl, { method: method.toUpperCase(), headers: renderedHeaders }); if (!resp.ok) { - throw new Error(`HTTP ${resp.status} ${resp.statusText} from ${finalUrl}`); + throw new CliError('FETCH_ERROR', `HTTP ${resp.status} ${resp.statusText} from ${finalUrl}`); } return resp.json(); } @@ -37,17 +39,23 @@ async function fetchSingle( const headersJs = JSON.stringify(renderedHeaders); const urlJs = JSON.stringify(finalUrl); const methodJs = JSON.stringify(method.toUpperCase()); - return page.evaluate(` + // Return error status instead of throwing inside evaluate to avoid CDP wrapper rewriting the message + const result = await page.evaluate(` async () => { const resp = await fetch(${urlJs}, { method: ${methodJs}, headers: ${headersJs}, credentials: "include" }); if (!resp.ok) { - throw new Error('HTTP ' + resp.status + ' ' + resp.statusText + ' from ' + ${urlJs}); + return { __fetchError: true, status: resp.status, statusText: resp.statusText, url: ${urlJs} }; } return await resp.json(); } `); + if (result && typeof result === 'object' && '__fetchError' in result) { + const { status, statusText, url: errorUrl } = result as { status: number; statusText: string; url: string }; + throw new CliError('FETCH_ERROR', `HTTP ${status} ${statusText} from ${errorUrl}`); + } + return result; } /** @@ -124,7 +132,14 @@ export async function stepFetch(page: IPage | null, params: unknown, data: unkno // BATCH IPC: if browser is available, batch all fetches into a single evaluate() call if (page !== null) { - return fetchBatchInBrowser(page, urls, method.toUpperCase(), renderedHeaders, concurrency); + const results = await fetchBatchInBrowser(page, urls, method.toUpperCase(), renderedHeaders, concurrency); + for (let i = 0; i < results.length; i++) { + const r = results[i]; + if (r && typeof r === 'object' && 'error' in r) { + log.warn(`Batch fetch failed for ${urls[i]}: ${(r as { error: string }).error}`); + } + } + return results; } // Non-browser: use concurrent pool (already optimized) @@ -133,7 +148,9 @@ export async function stepFetch(page: IPage | null, params: unknown, data: unkno try { return await fetchSingle(null, itemUrl, method, queryParams, headers, args, data); } catch (error) { - return { error: error instanceof Error ? error.message : String(error) }; + const message = error instanceof Error ? error.message : String(error); + log.warn(`Batch fetch failed for ${itemUrl}: ${message}`); + return { error: message }; } }); } From ecd40c02c3dd6e63ced66c5fba93c3c5ca2ed40a Mon Sep 17 00:00:00 2001 From: Yuhan Lei Date: Wed, 25 Mar 2026 01:19:47 +0800 Subject: [PATCH 5/6] chore: remove unrelated .worktrees/ from .gitignore --- .gitignore | 1 - 1 file changed, 1 deletion(-) diff --git a/.gitignore b/.gitignore index 224d549d..c017617b 100644 --- a/.gitignore +++ b/.gitignore @@ -22,4 +22,3 @@ docs/.vitepress/cache # Database files *.db -.worktrees/ From 205c66cde4c66b4930501b7f935ba2601a007a68 Mon Sep 17 00:00:00 2001 From: jackwener Date: Wed, 25 Mar 2026 13:09:28 +0800 Subject: [PATCH 6/6] refactor(fetch): use getErrorMessage(), unify sentinel naming to __httpError - Use project's existing getErrorMessage() utility instead of manual instanceof checks - Rename sentinel from __fetchError to __httpError for consistency with other adapters - Simplify sentinel structure (url already available in outer scope, no need to pass through evaluate) - Add comment explaining why getErrorMessage() can't be used inside evaluate() - Add comment explaining CDP error message rewriting behavior --- src/pipeline/steps/fetch.ts | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/pipeline/steps/fetch.ts b/src/pipeline/steps/fetch.ts index b88ad797..edecda63 100644 --- a/src/pipeline/steps/fetch.ts +++ b/src/pipeline/steps/fetch.ts @@ -2,7 +2,7 @@ * Pipeline step: fetch — HTTP API requests. */ -import { CliError } from '../../errors.js'; +import { CliError, getErrorMessage } from '../../errors.js'; import { log } from '../../logger.js'; import type { IPage } from '../../types.js'; import { render } from '../template.js'; @@ -39,21 +39,22 @@ async function fetchSingle( const headersJs = JSON.stringify(renderedHeaders); const urlJs = JSON.stringify(finalUrl); const methodJs = JSON.stringify(method.toUpperCase()); - // Return error status instead of throwing inside evaluate to avoid CDP wrapper rewriting the message + // Return error status instead of throwing inside evaluate to avoid CDP wrapper + // rewriting the message (CDP prepends "Evaluate error: " to thrown errors). const result = await page.evaluate(` async () => { const resp = await fetch(${urlJs}, { method: ${methodJs}, headers: ${headersJs}, credentials: "include" }); if (!resp.ok) { - return { __fetchError: true, status: resp.status, statusText: resp.statusText, url: ${urlJs} }; + return { __httpError: resp.status, statusText: resp.statusText }; } return await resp.json(); } `); - if (result && typeof result === 'object' && '__fetchError' in result) { - const { status, statusText, url: errorUrl } = result as { status: number; statusText: string; url: string }; - throw new CliError('FETCH_ERROR', `HTTP ${status} ${statusText} from ${errorUrl}`); + if (result && typeof result === 'object' && '__httpError' in result) { + const { __httpError: status, statusText } = result as { __httpError: number; statusText: string }; + throw new CliError('FETCH_ERROR', `HTTP ${status} ${statusText} from ${finalUrl}`); } return result; } @@ -90,8 +91,8 @@ async function fetchBatchInBrowser( } results[i] = await resp.json(); } catch (e) { - const message = e instanceof Error ? e.message : String(e); - results[i] = { error: message }; + results[i] = { error: e instanceof Error ? e.message : String(e) }; + // Note: getErrorMessage() is a Node.js utility — can't use it inside evaluate() } } } @@ -148,7 +149,7 @@ export async function stepFetch(page: IPage | null, params: unknown, data: unkno try { return await fetchSingle(null, itemUrl, method, queryParams, headers, args, data); } catch (error) { - const message = error instanceof Error ? error.message : String(error); + const message = getErrorMessage(error); log.warn(`Batch fetch failed for ${itemUrl}: ${message}`); return { error: message }; }