diff --git a/lib/agent/tools/__tests__/webFetchTool.test.ts b/lib/agent/tools/__tests__/webFetchTool.test.ts index 47fb75c9..5f430ec8 100644 --- a/lib/agent/tools/__tests__/webFetchTool.test.ts +++ b/lib/agent/tools/__tests__/webFetchTool.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; -import { webFetchTool } from "@/lib/agent/tools/webFetchTool"; +import { webFetchTool, MAX_BODY_LENGTH } from "@/lib/agent/tools/webFetchTool"; import { connectVercel } from "@/lib/sandbox/vercel/connect/connectVercel"; vi.mock("@/lib/sandbox/vercel/connect/connectVercel", () => ({ @@ -12,18 +12,21 @@ function makeSandbox(exec: ReturnType) { return { workingDirectory: "/sandbox/mono", exec }; } +/** The tool's stdout contract: `status\nbyteSize\n`. */ +function stdout(status: string, size: number, body: string) { + return `${status}\n${size}\n${body}`; +} + beforeEach(() => vi.clearAllMocks()); describe("webFetchTool", () => { - it("parses body + trailing status code on success", async () => { - // Body, then newline, then status code "200" (per the curl -w '%{http_code}' contract). + it("parses status, size and body on success", async () => { const sb = makeSandbox( vi.fn().mockResolvedValue({ success: true, exitCode: 0, - stdout: '{"ok":true}\n200', + stdout: stdout("200", 11, '{"ok":true}'), stderr: "", - truncated: false, }), ); vi.mocked(connectVercel).mockResolvedValue(sb as never); @@ -38,32 +41,67 @@ describe("webFetchTool", () => { }); }); - it("marks truncated:true on curl exit 23 (head -c cut off the body)", async () => { + it("preserves a body that itself contains newlines", async () => { const sb = makeSandbox( vi.fn().mockResolvedValue({ - success: false, - exitCode: 23, - stdout: "huge body fragment\n200", + success: true, + exitCode: 0, + stdout: stdout("200", 13, "line1\nline2\n"), + stderr: "", + }), + ); + vi.mocked(connectVercel).mockResolvedValue(sb as never); + const result = (await webFetchTool.execute!({ url: "https://example.com/multiline" }, { + experimental_context: ctx, + } as never)) as { body: string }; + expect(result.body).toBe("line1\nline2\n"); + }); + + it("marks truncated by BYTE SIZE, not by exit code", async () => { + const big = MAX_BODY_LENGTH + 512; + const sb = makeSandbox( + vi.fn().mockResolvedValue({ + success: true, + exitCode: 0, + stdout: stdout("200", big, "x".repeat(MAX_BODY_LENGTH)), stderr: "", - truncated: false, }), ); vi.mocked(connectVercel).mockResolvedValue(sb as never); const result = (await webFetchTool.execute!({ url: "https://example.com/huge" }, { experimental_context: ctx, - } as never)) as { success: boolean; truncated: boolean }; + } as never)) as { success: boolean; truncated: boolean; body: string }; expect(result.success).toBe(true); expect(result.truncated).toBe(true); + expect(result.body.length).toBe(MAX_BODY_LENGTH); }); - it("returns success:false on non-0, non-23 curl exit", async () => { + // The regression this fix exists for (chat#1918): a zero-byte write must never be + // reported as a truncated success — that made agents believe every URL was empty. + it("returns success:false when curl exits 23 with an empty body", async () => { + const sb = makeSandbox( + vi.fn().mockResolvedValue({ + success: false, + exitCode: 23, + stdout: stdout("200", 0, ""), + stderr: "curl: (23) Failure writing output to destination", + }), + ); + vi.mocked(connectVercel).mockResolvedValue(sb as never); + const result = (await webFetchTool.execute!({ url: "https://example.com" }, { + experimental_context: ctx, + } as never)) as { success: boolean; error: string }; + expect(result.success).toBe(false); + expect(result.error).toMatch(/23|writing output/i); + }); + + it("returns success:false on any nonzero exit with an empty body", async () => { const sb = makeSandbox( vi.fn().mockResolvedValue({ success: false, exitCode: 7, - stdout: "", + stdout: stdout("000", 0, ""), stderr: "Failed to connect", - truncated: false, }), ); vi.mocked(connectVercel).mockResolvedValue(sb as never); @@ -74,14 +112,32 @@ describe("webFetchTool", () => { expect(result.error).toMatch(/Failed to connect/); }); + it("does not use bash process substitution (unsupported in the sandbox shell)", async () => { + const sb = makeSandbox( + vi.fn().mockResolvedValue({ + success: true, + exitCode: 0, + stdout: stdout("200", 2, "ok"), + stderr: "", + }), + ); + vi.mocked(connectVercel).mockResolvedValue(sb as never); + await webFetchTool.execute!({ url: "https://example.com" }, { + experimental_context: ctx, + } as never); + const cmd = sb.exec.mock.calls[0]?.[0] as string; + expect(cmd).not.toContain(">("); + expect(cmd).not.toContain(">&3"); + expect(cmd).toContain("mktemp"); + }); + it("passes the request body for POST", async () => { const sb = makeSandbox( vi.fn().mockResolvedValue({ success: true, exitCode: 0, - stdout: "ok\n201", + stdout: stdout("201", 2, "ok"), stderr: "", - truncated: false, }), ); vi.mocked(connectVercel).mockResolvedValue(sb as never); diff --git a/lib/agent/tools/webFetchTool.ts b/lib/agent/tools/webFetchTool.ts index b395457f..41e8631c 100644 --- a/lib/agent/tools/webFetchTool.ts +++ b/lib/agent/tools/webFetchTool.ts @@ -63,7 +63,7 @@ USAGE: "--max-time", String(Math.ceil(FETCH_TIMEOUT_MS / 1000)), "-o", - `>(head -c ${MAX_BODY_LENGTH} >&3)`, + '"$tmp"', "-w", shellEscape("%{http_code}"), ]; @@ -78,15 +78,20 @@ USAGE: } args.push(shellEscape(url)); - // Use fd 3 to split curl's response body (truncated by `head -c`) from - // the status code written via `-w`. The body goes to stdout via fd 3 - // → fd 1, then we append the status code on its own newline. + // Write the body to a temp file rather than a process substitution: `>(…)` + // is a bash-ism that does not expand in the sandbox's exec shell, so curl + // wrote to a file literally named `>(head -c … >&3)`, failed, and exited 23 + // with an empty body — which the old exit-23 branch reported as a truncated + // success (chat#1918). stdout contract: `status\nbyteSize\n`, so the + // real size is known even though the body itself is capped for context. const command = [ - "exec 3>&1", + "tmp=$(mktemp)", `status=$(${args.join(" ")})`, "curlExit=$?", - "exec 3>&-", - "printf '\\n%s' \"$status\"", + 'size=$(wc -c < "$tmp" | tr -d " ")', + 'printf \'%s\\n%s\\n\' "$status" "$size"', + `head -c ${MAX_BODY_LENGTH} "$tmp"`, + 'rm -f "$tmp"', "exit $curlExit", ].join("\n"); @@ -96,25 +101,34 @@ USAGE: ...(recoupEnv ? { env: recoupEnv } : {}), }); - // exit 23 = curl wrote partial output (`head -c` cut it off — expected for large responses). - if (result.exitCode !== 0 && result.exitCode !== 23) { + const output = result.stdout ?? ""; + const firstNewline = output.indexOf("\n"); + const secondNewline = output.indexOf("\n", firstNewline + 1); + const statusText = firstNewline === -1 ? "" : output.slice(0, firstNewline).trim(); + const sizeText = + secondNewline === -1 ? "" : output.slice(firstNewline + 1, secondNewline).trim(); + const responseBody = secondNewline === -1 ? "" : output.slice(secondNewline + 1); + const status = /^\d+$/.test(statusText) && statusText !== "000" ? Number(statusText) : null; + const size = /^\d+$/.test(sizeText) ? Number(sizeText) : responseBody.length; + + // A nonzero exit that produced no bytes is a hard failure. Reporting it as + // an empty-bodied 200 is what made agents conclude the whole web was + // unreachable; surface it so they retry or report instead. + if (result.exitCode !== 0 && size === 0) { return { success: false, - error: `Fetch failed: ${result.stderr || result.stdout || "Unknown error"}`, + error: `Fetch failed (exit ${result.exitCode}): ${ + result.stderr || "no response body written" + }`, }; } - const output = result.stdout ?? ""; - const lastNewline = output.lastIndexOf("\n"); - const statusText = lastNewline !== -1 ? output.slice(lastNewline + 1).trim() : ""; - const responseBody = lastNewline !== -1 ? output.slice(0, lastNewline) : output; - const status = /^\d+$/.test(statusText) ? parseInt(statusText, 10) : null; - return { success: true, status, body: responseBody, - truncated: result.exitCode === 23, + // Truncation is a property of the response size, not of curl's exit code. + truncated: size > MAX_BODY_LENGTH, }; } catch (error) { const message = error instanceof Error ? error.message : String(error);