diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 4811b747c..27f97cb6e 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -10,6 +10,7 @@ ### Fixed - Fixed plan-mode re-entry after approval reopening a fresh `local://PLAN.md` instead of the approved titled plan artifact, which could duplicate plan content and fail approval on an existing destination. +- Fixed `read` URL reader mode aborting after a stalled Jina request instead of falling back to trafilatura/lynx/native: Jina (and Parallel extract) now have their own per-attempt sub-budget capped at 10s, the catch handler honours only real user cancellation, and the in-process native renderer is always attempted on already-loaded HTML ([#1449](https://github.com/can1357/oh-my-pi/issues/1449)) ## [15.5.6] - 2026-05-27 ### Added diff --git a/packages/coding-agent/src/tools/fetch.ts b/packages/coding-agent/src/tools/fetch.ts index e1a15c0b8..274699b95 100644 --- a/packages/coding-agent/src/tools/fetch.ts +++ b/packages/coding-agent/src/tools/fetch.ts @@ -562,9 +562,22 @@ function parseFeedToMarkdown(content: string, maxItems = 10): string { } /** - * Render HTML to markdown using Parallel, jina, trafilatura, lynx (in order of preference) + * Cap on any single remote reader-mode request (Parallel, Jina) so a stalled + * remote endpoint cannot consume the whole reader-mode budget and starve the + * local fallback renderers (trafilatura, lynx, native). See #1449. */ -async function renderHtmlToText( +const REMOTE_READER_MAX_MS = 10_000; + +/** + * Render HTML to markdown using Parallel, jina, trafilatura, lynx, then the + * in-process native converter. The overall `timeout` budget bounds the call, + * but remote reader requests are additionally capped at `REMOTE_READER_MAX_MS` + * so that a hung remote endpoint cannot prevent local fallbacks from running. + * Only a real `userSignal` cancellation aborts the chain — remote per-attempt + * timeouts and the overall reader-mode timeout still allow later renderers + * (especially the purely-local native converter) to be tried. + */ +export async function renderHtmlToText( url: string, html: string, timeout: number, @@ -572,14 +585,15 @@ async function renderHtmlToText( userSignal: AbortSignal | undefined, storage: AgentStorage | null, ): Promise<{ content: string; ok: boolean; method: string }> { - const signal = ptree.combineSignals(userSignal, timeout * 1000); + const overallSignal = ptree.combineSignals(userSignal, timeout * 1000); const execOptions = { mode: "group" as const, allowNonZero: true, allowAbort: true, stderr: "full" as const, - signal, + signal: overallSignal, }; + const remoteBudgetMs = Math.min(timeout * 1000, REMOTE_READER_MAX_MS); // Try Parallel extract first when credentials are configured if (settings.get("providers.parallelFetch") && findParallelApiKey(storage)) { @@ -590,7 +604,7 @@ async function renderHtmlToText( objective: "Extract the main content", excerpts: true, fullContent: false, - signal, + signal: ptree.combineSignals(userSignal, remoteBudgetMs), }, storage, ); @@ -602,17 +616,18 @@ async function renderHtmlToText( } } } catch { - // Parallel extract failed, continue to next method - signal?.throwIfAborted(); + // Parallel extract failed or stalled; honour real cancellation only. + userSignal?.throwIfAborted(); } } - // Try jina first (reader API) + // Try jina reader API with its own sub-budget so a stall cannot starve + // later fallbacks (#1449). try { const jinaUrl = `https://r.jina.ai/${url}`; const response = await fetch(jinaUrl, { headers: { Accept: "text/markdown" }, - signal, + signal: ptree.combineSignals(userSignal, remoteBudgetMs), }); if (response.ok) { const content = await response.text(); @@ -621,37 +636,50 @@ async function renderHtmlToText( } } } catch { - // Jina failed, continue to next method - signal?.throwIfAborted(); + // Jina failed or stalled; honour real cancellation only. + userSignal?.throwIfAborted(); } // Try trafilatura (auto-install via uv/pip) - const trafilatura = await ensureTool("trafilatura", { signal, silent: true }); - if (trafilatura) { - const result = await ptree.exec([trafilatura, "-u", url, "--output-format", "markdown"], execOptions); - if (result.ok && result.stdout.trim().length > 100) { - return { content: result.stdout, ok: true, method: "trafilatura" }; + try { + const trafilatura = await ensureTool("trafilatura", { signal: overallSignal, silent: true }); + if (trafilatura) { + const result = await ptree.exec([trafilatura, "-u", url, "--output-format", "markdown"], execOptions); + if (result.ok && result.stdout.trim().length > 100) { + return { content: result.stdout, ok: true, method: "trafilatura" }; + } } + } catch { + // trafilatura unavailable or stalled; continue to next method. + userSignal?.throwIfAborted(); } // Try lynx (can't auto-install, system package) - const lynx = hasCommand("lynx"); - if (lynx) { - const result = await ptree.exec(["lynx", "-dump", "-nolist", "-width", "250", url], execOptions); - if (result.ok) { - return { content: result.stdout, ok: true, method: "lynx" }; + try { + const lynx = hasCommand("lynx"); + if (lynx) { + const result = await ptree.exec(["lynx", "-dump", "-nolist", "-width", "250", url], execOptions); + if (result.ok) { + return { content: result.stdout, ok: true, method: "lynx" }; + } } + } catch { + // lynx failed or stalled; continue to native converter. + userSignal?.throwIfAborted(); } - // Fall back to native converter (fastest, no network/subprocess) + // Fall back to native converter (purely local, no network/subprocess). + // Always attempted: even if remote renderers and subprocesses were aborted + // by the overall reader-mode timeout, this still works on already-loaded + // HTML (#1449). try { const content = await htmlToMarkdown(html, { cleanContent: true }); if (content.trim().length > 100 && !isLowQualityOutput(content)) { return { content, ok: true, method: "native" }; } } catch { - // Native converter failed, continue to next method - signal?.throwIfAborted(); + // Native converter failed; nothing else to try. + userSignal?.throwIfAborted(); } return { content: "", ok: false, method: "none" }; } diff --git a/packages/coding-agent/test/tools/fetch-jina-stall.test.ts b/packages/coding-agent/test/tools/fetch-jina-stall.test.ts new file mode 100644 index 000000000..33fbee1be --- /dev/null +++ b/packages/coding-agent/test/tools/fetch-jina-stall.test.ts @@ -0,0 +1,101 @@ +import { afterEach, describe, expect, it } from "bun:test"; +import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; +import { renderHtmlToText } from "@oh-my-pi/pi-coding-agent/tools/fetch"; +import { hookFetch } from "@oh-my-pi/pi-utils"; + +/** + * Regression test for #1449: a stalled Jina reader request must not prevent + * local fallback renderers (trafilatura/lynx/native) from running within the + * overall reader-mode budget. + */ +describe("renderHtmlToText: jina stall does not starve local fallbacks (#1449)", () => { + afterEach(() => { + // Nothing to restore — `using` handles fetch hook cleanup per-test. + }); + + it("falls back to native renderer when jina hangs until aborted", async () => { + const settings = Settings.isolated({ "providers.parallelFetch": false }); + // Substantive HTML so the native converter produces >100 chars and + // `isLowQualityOutput` does not reject it. + const paragraphs = Array.from({ length: 6 }, (_, i) => `

Paragraph number ${i + 1} carries some real content for the article body so the native renderer has enough text to satisfy the length threshold.

`).join(""); + const html = `Example

Example article

${paragraphs}
`; + + using _hook = hookFetch((input, _init, _next) => { + const url = String(input); + // Hang on the Jina reader endpoint until aborted, mirroring the + // real bug: r.jina.ai stalls indefinitely. + if (url.startsWith("https://r.jina.ai/")) { + return new Promise((_resolve, reject) => { + const signal = _init?.signal; + if (!signal) return; // never settles + if (signal.aborted) { + reject(new DOMException("aborted", "AbortError")); + return; + } + signal.addEventListener("abort", () => { + reject(new DOMException("aborted", "AbortError")); + }); + }); + } + return new Response("", { status: 404 }); + }); + + const started = Date.now(); + // `timeout: 2` keeps the overall budget tight — the test must complete + // within ~2s even though Jina would otherwise hang for the full budget. + const result = await renderHtmlToText( + "https://example.com/article", + html, + 2, + settings, + undefined, + null, + ); + const elapsedMs = Date.now() - started; + + expect(result.ok).toBe(true); + // Native converter is the only deterministic local fallback; trafilatura + // and lynx may or may not be installed in CI, but native always works. + // If trafilatura or lynx happened to succeed first, that's also a valid + // non-aborted outcome. + expect(["native", "trafilatura", "lynx"]).toContain(result.method); + // Must finish well before the overall budget elapses: the remote + // sub-budget caps Jina at min(timeout, REMOTE_READER_MAX_MS), so the + // remaining ~1s of the 2s budget is enough for the native renderer. + expect(elapsedMs).toBeLessThan(2_500); + }); + + it("re-throws when the user signal is aborted, not when Jina sub-budget expires", async () => { + const settings = Settings.isolated({ "providers.parallelFetch": false }); + const html = "

short

"; + + using _hook = hookFetch((_input, init, _next) => { + return new Promise((_resolve, reject) => { + const signal = init?.signal; + if (!signal) return; // Defensive: never settles otherwise. + if (signal.aborted) { + reject(new DOMException("aborted", "AbortError")); + return; + } + signal.addEventListener("abort", () => { + reject(new DOMException("aborted", "AbortError")); + }); + }); + }); + + const controller = new AbortController(); + const pending = renderHtmlToText( + "https://example.com/article", + html, + 30, + settings, + controller.signal, + null, + ).catch(err => err); + + controller.abort(); + const outcome = await pending; + expect(outcome).toBeInstanceOf(Error); + expect((outcome as Error).name === "AbortError" || (outcome as Error).message.toLowerCase().includes("abort")).toBe(true); + }); +});