From 845311cf97ecfc6c66de713a7df241bc4e01d101 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 10 Jul 2026 17:40:14 +0100 Subject: [PATCH] fix(browser): prevent stale handles from switching duplicate-URL tabs (#103816) * fix(browser): reject ambiguous tab replacements * fix(browser): guide stale tab recovery --- docs/tools/browser-control.md | 7 +- ....remote-profile-tab-ops.playwright.test.ts | 41 ++++++++++++ .../browser/server-context.selection.test.ts | 9 +++ .../src/browser/server-context.selection.ts | 11 ++-- .../src/browser/server-context.tab-ops.ts | 51 ++++++++------- ...server-context.tab-selection-state.test.ts | 65 +++++++++++++++++++ 6 files changed, 152 insertions(+), 32 deletions(-) diff --git a/docs/tools/browser-control.md b/docs/tools/browser-control.md index 24185581f15a..3056e12c352f 100644 --- a/docs/tools/browser-control.md +++ b/docs/tools/browser-control.md @@ -281,9 +281,10 @@ Notes: - `upload` can also set file inputs directly via `--input-ref` or `--element`. Stable tab ids and labels survive Chromium raw-target replacement when OpenClaw -can prove the replacement tab, such as same URL or a single old tab becoming a -single new tab after form submission. Raw target ids are still volatile; prefer -`suggestedTargetId` from `tabs` in scripts. +can prove the replacement tab, such as a unique old/new pair for the same URL or +a single old tab becoming a single new tab after form submission. Ambiguous +duplicate-URL replacements receive fresh handles. Raw target ids are still +volatile; prefer `suggestedTargetId` from `tabs` in scripts. Snapshot flags at a glance: diff --git a/extensions/browser/src/browser/server-context.remote-profile-tab-ops.playwright.test.ts b/extensions/browser/src/browser/server-context.remote-profile-tab-ops.playwright.test.ts index abe799ee9b37..97c9c3069476 100644 --- a/extensions/browser/src/browser/server-context.remote-profile-tab-ops.playwright.test.ts +++ b/extensions/browser/src/browser/server-context.remote-profile-tab-ops.playwright.test.ts @@ -189,6 +189,47 @@ describe("browser remote profile tab ops via Playwright", () => { expect(state.profiles.get("remote")?.lastTargetId).toBe("A"); }); + it("migrates only unique URL groups alongside ambiguous duplicate groups", async () => { + let currentPages = [ + page("A", "https://unique.example"), + page("B", "https://duplicate.example"), + page("C", "https://duplicate.example"), + ]; + const listPagesViaPlaywright = vi.fn(async () => currentPages); + + vi.spyOn(deps.pwAiModule, "getPwAiModule").mockResolvedValue({ + listPagesViaPlaywright, + } as unknown as Awaited>); + + const { state, remote } = deps.createRemoteRouteHarness(); + + expect((await remote.listTabs()).map((tab) => [tab.targetId, tab.tabId])).toEqual([ + ["A", "t1"], + ["B", "t2"], + ["C", "t3"], + ]); + await remote.labelTab("t1", "unique"); + state.profiles.get("remote")!.lastTargetId = "A"; + + currentPages = [ + page("D", "https://unique.example"), + page("E", "https://duplicate.example"), + page("F", "https://duplicate.example"), + ]; + + await expect(remote.listTabs()).resolves.toEqual([ + expect.objectContaining({ + targetId: "D", + tabId: "t1", + label: "unique", + suggestedTargetId: "unique", + }), + expect.objectContaining({ targetId: "E", tabId: "t4", suggestedTargetId: "t4" }), + expect.objectContaining({ targetId: "F", tabId: "t5", suggestedTargetId: "t5" }), + ]); + expect(state.profiles.get("remote")?.lastTargetId).toBe("D"); + }); + it("prefers lastTargetId for remote profiles when targetId is omitted", async () => { const responses = [ [ diff --git a/extensions/browser/src/browser/server-context.selection.test.ts b/extensions/browser/src/browser/server-context.selection.test.ts index b3fdbfd3b0de..d9d26117ecfd 100644 --- a/extensions/browser/src/browser/server-context.selection.test.ts +++ b/extensions/browser/src/browser/server-context.selection.test.ts @@ -199,6 +199,15 @@ describe("browser profile tab selection", () => { await expect(selection.ensureTabAvailable()).resolves.toEqual(sticky); }); + it("rejects when a sticky target disappears instead of selecting another tab", async () => { + const other = tab("OTHER", "ws://127.0.0.1/devtools/page/OTHER"); + const { selection, profileState } = createSelectionHarness({ snapshots: [[other]] }); + profileState.lastTargetId = "STALE"; + + await expect(selection.ensureTabAvailable()).rejects.toThrow(/use action=tabs/i); + expect(profileState.lastTargetId).toBe("STALE"); + }); + it("keeps polling after a transient tab-list rejection", async () => { vi.useFakeTimers(); const withoutWs = tab("RECOVERED"); diff --git a/extensions/browser/src/browser/server-context.selection.ts b/extensions/browser/src/browser/server-context.selection.ts index dc3b023bdfbf..5a5ec3f17368 100644 --- a/extensions/browser/src/browser/server-context.selection.ts +++ b/extensions/browser/src/browser/server-context.selection.ts @@ -195,15 +195,16 @@ export function createProfileSelectionOps({ return candidates.find((t) => t.targetId === resolved.targetId) ?? null; }; + const stickyTargetId = normalizeOptionalString(profileState.lastTargetId); const pickDefault = () => { - const last = normalizeOptionalString(profileState.lastTargetId) ?? ""; + const last = stickyTargetId ?? ""; const lastResolved = last ? resolveById(last, { exactTargetId: true }) : null; if (lastResolved && lastResolved !== "AMBIGUOUS") { return lastResolved; } - // Chrome MCP identity is authoritative. Once a selected target disappears, - // require a fresh explicit choice instead of guessing another tab. - if (last && capabilities.usesChromeMcp) { + // Sticky selection is an identity promise. If it disappears without a proven + // alias migration, require a fresh explicit choice instead of guessing a tab. + if (last) { return null; } // Prefer a real page tab first (avoid service workers/background targets). @@ -217,7 +218,7 @@ export function createProfileSelectionOps({ throw new BrowserTargetAmbiguousError(); } if (!chosen) { - throw new BrowserTabNotFoundError(targetId ? { input: targetId } : undefined); + throw new BrowserTabNotFoundError({ input: targetId ?? stickyTargetId }); } profileState.lastTargetId = chosen.targetId; return chosen; diff --git a/extensions/browser/src/browser/server-context.tab-ops.ts b/extensions/browser/src/browser/server-context.tab-ops.ts index 9452a1462410..73fa26d43c27 100644 --- a/extensions/browser/src/browser/server-context.tab-ops.ts +++ b/extensions/browser/src/browser/server-context.tab-ops.ts @@ -121,18 +121,33 @@ function assignTabAlias(params: { }; } -function isConfidentReplacement(params: { - staleEntry: { url?: string }; - tab: BrowserTab; - staleCount: number; - newCandidateCount: number; -}): boolean { - const staleUrl = params.staleEntry.url?.trim(); - const tabUrl = params.tab.url?.trim(); - if (staleUrl && tabUrl && staleUrl === tabUrl) { - return true; +type TabAliasEntry = NonNullable["byTargetId"][string]; + +function normalizeReplacementUrl(url: string | undefined): string | undefined { + return url?.trim() || undefined; +} + +function findConfidentReplacement(params: { + staleEntry: TabAliasEntry; + staleEntries: Array<[targetId: string, entry: TabAliasEntry]>; + newCandidates: BrowserTab[]; +}): BrowserTab | undefined { + const { staleEntry, staleEntries, newCandidates } = params; + // Preserve shipped form-submit continuity when the replacement set is one-for-one. + if (staleEntries.length === 1 && newCandidates.length === 1) { + return newCandidates[0]; } - return params.staleCount === 1 && params.newCandidateCount === 1; + + const url = normalizeReplacementUrl(staleEntry.url); + if (!url) { + return undefined; + } + const staleMatches = staleEntries.filter( + ([, entry]) => normalizeReplacementUrl(entry.url) === url, + ); + const candidates = newCandidates.filter((tab) => normalizeReplacementUrl(tab.url) === url); + // Duplicate URL buckets have no ordering contract, so only migrate an exact 1:1 bucket. + return staleMatches.length === 1 && candidates.length === 1 ? candidates[0] : undefined; } function assignTabAliases( @@ -146,26 +161,14 @@ function assignTabAliases( ([targetId]) => !liveTargetIds.has(targetId), ); const newCandidates = tabs.filter((tab) => !aliases.byTargetId[tab.targetId]); - const claimedTargetIds = new Set(); - if (migrateReplacements) { for (const [oldTargetId, staleEntry] of staleEntries) { - const candidate = newCandidates.find( - (tab) => - !claimedTargetIds.has(tab.targetId) && - isConfidentReplacement({ - staleEntry, - tab, - staleCount: staleEntries.length, - newCandidateCount: newCandidates.length, - }), - ); + const candidate = findConfidentReplacement({ staleEntry, staleEntries, newCandidates }); if (!candidate) { continue; } aliases.byTargetId[candidate.targetId] = staleEntry; delete aliases.byTargetId[oldTargetId]; - claimedTargetIds.add(candidate.targetId); if (profileState.lastTargetId === oldTargetId) { profileState.lastTargetId = candidate.targetId; } diff --git a/extensions/browser/src/browser/server-context.tab-selection-state.test.ts b/extensions/browser/src/browser/server-context.tab-selection-state.test.ts index 958493edb953..b1fa7b3fc7e4 100644 --- a/extensions/browser/src/browser/server-context.tab-selection-state.test.ts +++ b/extensions/browser/src/browser/server-context.tab-selection-state.test.ts @@ -565,6 +565,71 @@ describe("browser server-context tab selection state", () => { expect(state.profiles.get("openclaw")?.lastTargetId).toBe("NEW_RAW"); }); + it("expires aliases when duplicate-URL targets are replaced ambiguously", async () => { + let targets = [ + { id: "OLD_LEFT", title: "Left", url: "https://app.example/same" }, + { id: "OLD_RIGHT", title: "Right", url: "https://app.example/same" }, + ]; + const fetchMock = vi.fn(async (url: unknown) => { + const value = String(url); + if (!value.includes("/json/list")) { + throw new Error(`unexpected fetch: ${value}`); + } + return { + ok: true, + json: async () => + targets.map((target) => ({ + id: target.id, + title: target.title, + url: target.url, + webSocketDebuggerUrl: `ws://127.0.0.1/devtools/page/${target.id}`, + type: "page", + })), + } as unknown as Response; + }); + + global.fetch = withBrowserFetchPreconnect(fetchMock); + const state = makeState("openclaw"); + const ctx = createTestBrowserRouteContext({ getState: () => state }); + const openclaw = ctx.forProfile("openclaw"); + + expect((await openclaw.listTabs()).map((tab) => [tab.targetId, tab.tabId])).toEqual([ + ["OLD_LEFT", "t1"], + ["OLD_RIGHT", "t2"], + ]); + await openclaw.labelTab("t1", "left"); + await openclaw.labelTab("t2", "right"); + state.profiles.get("openclaw")!.lastTargetId = "OLD_LEFT"; + + targets = [ + { id: "NEW_RIGHT", title: "Right", url: "https://app.example/same" }, + { id: "NEW_LEFT", title: "Left", url: "https://app.example/same" }, + ]; + + await expect(openclaw.listTabs()).resolves.toEqual([ + expect.objectContaining({ targetId: "NEW_RIGHT", tabId: "t3", suggestedTargetId: "t3" }), + expect.objectContaining({ targetId: "NEW_LEFT", tabId: "t4", suggestedTargetId: "t4" }), + ]); + expect(state.profiles.get("openclaw")?.lastTargetId).toBe("OLD_LEFT"); + await expect(openclaw.ensureTabAvailable("left")).rejects.toThrow(/tab not found/i); + await expect(openclaw.ensureTabAvailable()).rejects.toThrow(/tab not found/i); + + await openclaw.labelTab("t3", "fresh-right"); + targets = [ + { id: "NEW_LEFT", title: "Left", url: "https://app.example/same" }, + { id: "NEWER_RIGHT", title: "Right", url: "https://app.example/same" }, + ]; + await expect(openclaw.listTabs()).resolves.toEqual([ + expect.objectContaining({ targetId: "NEW_LEFT", tabId: "t4" }), + expect.objectContaining({ + targetId: "NEWER_RIGHT", + tabId: "t3", + label: "fresh-right", + suggestedTargetId: "fresh-right", + }), + ]); + }); + it("resolves friendly tab references before backend focus and close calls", async () => { const fetchMock = vi.fn(async (url: unknown) => { const value = String(url);