From 0251e8ef7f38006df7d25ebd64f1c04b7df8af99 Mon Sep 17 00:00:00 2001 From: rhclaw Date: Mon, 6 Jul 2026 10:55:39 -0400 Subject: [PATCH] fix(browser): preserve HTTP status in node-proxied browser errors (#89086) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(browser): preserve HTTP status in node-proxied browser errors The node browser proxy (runBrowserProxyCommand) collapsed a >=400 browser-route response into new Error(), dropping the HTTP status. That error crosses the node.invoke boundary as a plain string (Error properties are not preserved over the RPC), so the gateway's stale-target retry classifier — which keys off a leading : token (msg.includes("404:") && msg.includes("tab not found")) — never matches a node-proxied "tab not found". The drop-targetId retry never fires and the stale-targetId error surfaces to the agent instead. Prefix the status onto the message ("404: tab not found", "403: action targetId must match request targetId") so the existing gateway classification and retry work through the node proxy. Pure formatting change in the >=400 branch; validation/timeout error paths are untouched. Tests: extensions/browser invoke-browser suite — 14/14 pass. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(browser): harden node proxy status errors * docs(changelog): credit browser proxy status fix * chore: defer browser proxy release note --------- Co-authored-by: rhclaw <260109027+rhclaw@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) Co-authored-by: Peter Steinberger --- extensions/browser/src/browser-tool.test.ts | 42 +++++++++++++++++++ .../src/node-host/invoke-browser.test.ts | 17 +++++--- .../browser/src/node-host/invoke-browser.ts | 10 +++-- 3 files changed, 59 insertions(+), 10 deletions(-) diff --git a/extensions/browser/src/browser-tool.test.ts b/extensions/browser/src/browser-tool.test.ts index 7539111a2347..7be9fe9a9de9 100644 --- a/extensions/browser/src/browser-tool.test.ts +++ b/extensions/browser/src/browser-tool.test.ts @@ -1898,6 +1898,48 @@ describe("browser tool act stale target recovery", () => { expect((result?.details as { ok?: unknown } | undefined)?.ok).toBe(true); }); + it("retries stale targetIds returned through the node browser proxy", async () => { + mockSingleBrowserProxyNode(); + setResolvedBrowserProfiles({ + user: { driver: "existing-session", attachOnly: true, color: "#00AA00" }, + }); + gatewayMocks.callGatewayTool + .mockRejectedValueOnce(new Error("INVALID_REQUEST: Error: 404: tab not found")) + .mockResolvedValueOnce({ + ok: true, + payload: { result: { tabs: [{ targetId: "only-tab" }] } }, + }) + .mockResolvedValueOnce({ + ok: true, + payload: { result: { ok: true, targetId: "only-tab" } }, + }); + + const tool = createBrowserTool(); + const result = await tool.execute?.("call-1", { + action: "act", + target: "node", + profile: "user", + request: { + kind: "wait", + targetId: "stale-tab", + timeMs: 1, + }, + }); + + expect(gatewayMocks.callGatewayTool).toHaveBeenCalledTimes(3); + expect(nodeInvokeCall(0).request.params).toMatchObject({ + path: "/act", + body: { kind: "wait", targetId: "stale-tab", timeMs: 1 }, + }); + expect(nodeInvokeCall(1).request.params?.path).toBe("/tabs"); + expect(nodeInvokeCall(2).request.params).toMatchObject({ + path: "/act", + body: { kind: "wait", timeMs: 1 }, + }); + expect(nodeInvokeCall(2).request.params?.body).not.toHaveProperty("targetId"); + expect(result?.details).toMatchObject({ ok: true, targetId: "only-tab" }); + }); + it("does not retry mutating user-browser act requests without targetId", async () => { browserActionsMocks.browserAct.mockRejectedValueOnce(new Error("404: tab not found")); browserClientMocks.browserTabs.mockResolvedValueOnce([{ targetId: "only-tab" }]); diff --git a/extensions/browser/src/node-host/invoke-browser.test.ts b/extensions/browser/src/node-host/invoke-browser.test.ts index 9d13d9c7b265..f6b0005f162c 100644 --- a/extensions/browser/src/node-host/invoke-browser.test.ts +++ b/extensions/browser/src/node-host/invoke-browser.test.ts @@ -289,11 +289,16 @@ describe("runBrowserProxyCommand", () => { await result; }); - it("keeps non-timeout browser errors intact", async () => { - dispatcherMocks.dispatch.mockResolvedValue({ - status: 500, - body: { error: "tab not found" }, - }); + it.each([ + { status: 500, body: { error: "tab not found" }, expected: "500: tab not found" }, + { + status: 404, + body: { error: 'tab not found: browser tab "abc"' }, + expected: "404: tab not found", + }, + { status: 503, body: { error: "" }, expected: "HTTP 503" }, + ])("preserves browser response status in errors: $expected", async (response) => { + dispatcherMocks.dispatch.mockResolvedValue(response); await expect( runBrowserProxyCommand( @@ -304,7 +309,7 @@ describe("runBrowserProxyCommand", () => { timeoutMs: 50, }), ), - ).rejects.toThrow("tab not found"); + ).rejects.toThrow(response.expected); }); it("rejects unauthorized query.profile when allowProfiles is configured", async () => { diff --git a/extensions/browser/src/node-host/invoke-browser.ts b/extensions/browser/src/node-host/invoke-browser.ts index ec19f00523f4..31f5945c3b9e 100644 --- a/extensions/browser/src/node-host/invoke-browser.ts +++ b/extensions/browser/src/node-host/invoke-browser.ts @@ -315,11 +315,13 @@ export async function runBrowserProxyCommand(paramsJSON?: string | null): Promis ); } if (response.status >= 400) { - const message = + // node.invoke preserves only Error.message; keep the status there so gateway + // retry classifiers see the same error shape as direct browser requests. + const detail = response.body && typeof response.body === "object" && "error" in response.body - ? String((response.body as { error?: unknown }).error) - : `HTTP ${response.status}`; - throw new Error(message); + ? String((response.body as { error?: unknown }).error).trim() + : ""; + throw new Error(detail ? `${response.status}: ${detail}` : `HTTP ${response.status}`); } const result = response.body;