From 58c7f38a05e9c4b2c40937c56928a73d3097331b Mon Sep 17 00:00:00 2001 From: Ljy-0827 Date: Fri, 21 Aug 2026 16:31:47 +0800 Subject: [PATCH] feat(bsk): harden browser file transfer transactions --- apps/extension/PRIVACY.md | 2 +- .../src/tools/__tests__/dispatcher.test.ts | 13 + .../src/tools/__tests__/file-transfer.test.ts | 300 ++++++++++- apps/extension/src/tools/download-capture.ts | 256 ++++++--- apps/extension/src/tools/download.ts | 23 +- apps/extension/src/tools/errors.ts | 28 +- .../src/tools/file-input-transaction.ts | 494 +++++++++++------- apps/extension/src/tools/interaction.ts | 46 +- apps/extension/src/tools/upload.ts | 23 +- apps/extension/src/transport/types.ts | 9 + crates/bsk-cli/Cargo.toml | 1 + crates/bsk-cli/skill/SKILL.md | 5 +- crates/bsk-cli/src/cli/atomic_output.rs | 75 +++ crates/bsk-cli/src/cli/download.rs | 20 +- crates/bsk-cli/src/cli/mod.rs | 1 + crates/bsk-cli/src/cli/render_error.rs | 25 + crates/bsk-cli/src/cli/upload.rs | 65 +-- crates/bsk-cli/src/daemon/file_transfer.rs | 133 ++++- crates/bsk-cli/src/daemon/ipc.rs | 3 +- crates/bsk-cli/src/daemon/queue.rs | 253 ++++++++- .../schema/tool_download_params.json | 9 + .../bsk-protocol/src/tools/file_transfer.rs | 6 + docs/architecture.md | 9 +- skill/SKILL.md | 5 +- 24 files changed, 1397 insertions(+), 407 deletions(-) create mode 100644 crates/bsk-cli/src/cli/atomic_output.rs diff --git a/apps/extension/PRIVACY.md b/apps/extension/PRIVACY.md index 38e7968..bfe4675 100644 --- a/apps/extension/PRIVACY.md +++ b/apps/extension/PRIVACY.md @@ -51,7 +51,7 @@ The Extension requests the following Chrome permissions. Each is used solely for - **`alarms`** — Periodically wake the service worker to keep the local WebSocket connection alive. - **`idle`** — Detect when the device returns from idle/locked so the Extension can promptly re-establish the local WebSocket connection after the machine wakes. No idle data is stored or transmitted. - **`notifications`** — Show a system notification to obtain user approval before the agent borrows a user-owned tab. -- **`downloads`** — Observe and, on cancellation, stop the one browser download initiated by an active `bsk download` command. It is not used to enumerate download history. +- **`downloads`** — Correlate and route the one browser download initiated by an active `bsk download` command. If that claimed transaction fails, BrowserSkill cancels an in-progress file or removes its completed temporary browser file. It is not used to enumerate download history or alter unclaimed downloads. - **`storage`** — Persist a random instance ID and optional label in `chrome.storage.local`. - **Host permission ``** — Inject a small status overlay (showing "Agent Active") on pages controlled by the agent, and enable automation across whatever sites the user directs the agent to. The Extension does **not** read or transmit page content from sites the agent is not actively driving. diff --git a/apps/extension/src/tools/__tests__/dispatcher.test.ts b/apps/extension/src/tools/__tests__/dispatcher.test.ts index 113fc20..ee566f5 100644 --- a/apps/extension/src/tools/__tests__/dispatcher.test.ts +++ b/apps/extension/src/tools/__tests__/dispatcher.test.ts @@ -258,7 +258,20 @@ describe("ToolDispatcher", () => { if (method === "DOM.getContentQuads") { return { quads: [[0, 0, 20, 0, 20, 20, 0, 20]] } as T; } + if (method === "DOM.resolveNode") { + return { object: { objectId: "trigger-object" } } as T; + } if (method === "DOM.describeNode") return { node: { backendNodeId: 456 } } as T; + if (method === "Runtime.callFunctionOn") { + const declaration = (params as { functionDeclaration?: string }).functionDeclaration ?? ""; + if (declaration.includes("count: state.inputs.length")) { + return { result: { value: { count: 1, multiple: false } } } as T; + } + if (declaration.includes("inputs[0]")) { + return { result: { objectId: "input-object" } } as T; + } + return { result: { value: true } } as T; + } if (method === "Runtime.evaluate") { const expression = (params as { expression?: string }).expression ?? ""; if (expression.includes("overlayDetails")) { diff --git a/apps/extension/src/tools/__tests__/file-transfer.test.ts b/apps/extension/src/tools/__tests__/file-transfer.test.ts index 75d0b7e..81dbe01 100644 --- a/apps/extension/src/tools/__tests__/file-transfer.test.ts +++ b/apps/extension/src/tools/__tests__/file-transfer.test.ts @@ -3,6 +3,7 @@ import { SessionManager } from "@/session-manager/manager"; import { type DownloadsApi, handleDownload } from "../download"; import { captureBrowserDownload } from "../download-capture"; import { uploadThroughActivatedFileInput } from "../file-input-transaction"; +import type { ResolvedActionTarget } from "../interaction"; import type { CdpRunner } from "../shared"; import { handleUpload } from "../upload"; @@ -36,37 +37,76 @@ function fakeEvent unknown>() { }; } -function uploadCdp(options: { inputCount?: number; multiple?: boolean; pickerCall?: string } = {}) { +function actionTarget(frameId?: string): ResolvedActionTarget { + return { + tab: { tabId: 4, windowId: 100, active: true }, + backendNodeId: 123, + cdpTarget: { tabId: 4 }, + ...(frameId ? { frameId } : {}), + usedRef: "e3", + }; +} + +function uploadCdp( + options: { + inputCount?: number; + multiple?: boolean; + chooser?: { frameId?: string; backendNodeId?: number; mode?: string }; + pendingResolve?: boolean; + } = {}, +) { const calls: Array<{ method: string; params?: object }> = []; + let cdpEvent: Parameters>[0] | undefined; const send = vi.fn(async (_tabId: number, method: string, params?: object) => { calls.push({ method, params }); + if (method === "Page.setInterceptFileChooserDialog") return {}; if (method === "Page.getLayoutMetrics") return { cssLayoutViewport: { clientWidth: 1280, clientHeight: 720 } }; if (method === "DOM.getContentQuads") return { quads: [[0, 0, 20, 0, 20, 20, 0, 20]] }; - if (method === "Runtime.evaluate") { - const expression = (params as { expression?: string }).expression ?? ""; - if (expression.includes("count:")) { + if (method === "DOM.resolveNode") { + if (options.pendingResolve) return new Promise(() => {}); + return { object: { objectId: "trigger-object" } }; + } + if (method === "Runtime.callFunctionOn") { + const declaration = (params as { functionDeclaration?: string }).functionDeclaration ?? ""; + if (declaration.includes("Object.defineProperty")) return { result: { value: true } }; + if (declaration.includes("count: state.inputs.length")) { return { result: { value: { count: options.inputCount ?? 1, multiple: options.multiple ?? true, - pickerCall: options.pickerCall, }, }, }; } - if (expression.includes("?.inputs[0]")) { + if (declaration.includes("inputs[0]")) { return { result: { objectId: "input-object" } }; } return { result: { value: true } }; } if (method === "DOM.describeNode") return { node: { backendNodeId: 456 } }; + if ( + method === "Input.dispatchMouseEvent" && + (params as { type?: string }).type === "mousePressed" && + options.chooser + ) { + cdpEvent?.({ tabId: 4 }, "Page.fileChooserOpened", options.chooser); + } return {}; }); + const cdp: CdpRunner = { + send: send as unknown as CdpRunner["send"], + onEvent: (handler) => { + cdpEvent = handler; + return { dispose: vi.fn() }; + }, + }; return { calls, - cdp: { send: send as unknown as CdpRunner["send"] } satisfies CdpRunner, + emitChooser: (event: { frameId?: string; backendNodeId?: number; mode?: string }) => + cdpEvent?.({ tabId: 4 }, "Page.fileChooserOpened", event), + cdp, }; } @@ -91,15 +131,14 @@ describe("file transfer tools", () => { ); expect(result).toMatchObject({ tab_id: 4, file_names: ["one.png", "two.png"] }); - const armCall = calls.find( - (call) => - call.method === "Runtime.evaluate" && - (call.params as { expression?: string }).expression?.includes("showOpenFilePicker"), - ); - expect(armCall).toBeDefined(); - expect((armCall?.params as { expression?: string }).expression).toContain( - "event.preventDefault()", - ); + expect(calls[0]).toEqual({ + method: "Page.setInterceptFileChooserDialog", + params: { enabled: true }, + }); + expect(calls).toContainEqual({ + method: "Page.setInterceptFileChooserDialog", + params: { enabled: false, cancel: true }, + }); expect(calls).toContainEqual({ method: "DOM.setFileInputFiles", params: { files: ["/private/stage/one", "/private/stage/two"], backendNodeId: 456 }, @@ -110,7 +149,7 @@ describe("file transfer tools", () => { const { cdp } = uploadCdp({ inputCount: 0 }); const result = await uploadThroughActivatedFileInput({ cdp, - target: { tabId: 4 }, + actionTarget: actionTarget(), files: ["/private/stage/one"], timeoutMs: 100, trigger: async () => ({ tab_id: 4, x: 10, y: 10 }), @@ -122,31 +161,80 @@ describe("file transfer tools", () => { }); }); - it("reports a File System Access picker without waiting for a timeout", async () => { - const { cdp } = uploadCdp({ inputCount: 0, pickerCall: "showOpenFilePicker" }); + it("fails before clicking when chooser interception is unavailable", async () => { + const trigger = vi.fn(async () => ({ tab_id: 4, x: 10, y: 10 })); + const cdp: CdpRunner = { + send: vi.fn(async (_tabId: number, method: string) => { + if (method === "Page.setInterceptFileChooserDialog") { + throw new Error("method unavailable"); + } + return {}; + }) as CdpRunner["send"], + }; + const result = await uploadThroughActivatedFileInput({ cdp, - target: { tabId: 4 }, + actionTarget: actionTarget(), files: ["/private/stage/one"], timeoutMs: 100, - trigger: async () => ({ tab_id: 4, x: 10, y: 10 }), + trigger, + }); + + expect(result).toMatchObject({ + code: "cdp_failed", + data: { effect_state: "none", phase: "arm_interception" }, + }); + expect(trigger).not.toHaveBeenCalled(); + }); + + it("uses an exact chooser event as an independent input-location signal", async () => { + const { cdp, calls, emitChooser } = uploadCdp({ inputCount: 0 }); + const result = await uploadThroughActivatedFileInput({ + cdp, + actionTarget: actionTarget("f1"), + files: ["/private/stage/one"], + timeoutMs: 100, + trigger: async () => { + emitChooser({ frameId: "f1", backendNodeId: 789, mode: "selectSingle" }); + return { tab_id: 4, x: 10, y: 10 }; + }, + }); + + expect(result).toMatchObject({ multiple: false }); + expect(calls).toContainEqual({ + method: "DOM.setFileInputFiles", + params: { files: ["/private/stage/one"], backendNodeId: 789 }, + }); + }); + + it("reports a File System Access picker without waiting for a timeout", async () => { + const { cdp, emitChooser } = uploadCdp({ + inputCount: 0, + }); + const result = await uploadThroughActivatedFileInput({ + cdp, + actionTarget: actionTarget("f1"), + files: ["/private/stage/one"], + timeoutMs: 100, + trigger: async () => { + emitChooser({ frameId: "f1", mode: "selectSingle" }); + return { tab_id: 4, x: 10, y: 10 }; + }, }); expect(result).toMatchObject({ code: "unsupported", - message: "upload trigger invoked showOpenFilePicker instead of an input[type=file]", + message: "upload trigger invoked a non-input file picker", data: { reason: "file_input_not_activated", phase: "resolve_input" }, }); }); it("bounds a stuck file-input probe", async () => { - const cdp: CdpRunner = { - send: vi.fn(() => new Promise(() => {})) as unknown as CdpRunner["send"], - }; + const { cdp } = uploadCdp({ pendingResolve: true }); const result = await uploadThroughActivatedFileInput({ cdp, - target: { tabId: 4 }, + actionTarget: actionTarget(), files: ["/private/stage/one"], timeoutMs: 5, trigger: vi.fn(), @@ -158,6 +246,31 @@ describe("file transfer tools", () => { }); }); + it("marks a timed-out file assignment unknown and detaches browser state", async () => { + const fixture = uploadCdp(); + const originalSend = fixture.cdp.send; + const detach = vi.fn(async () => {}); + fixture.cdp.detach = detach; + fixture.cdp.send = vi.fn((tabId: number, method: string, params?: object) => { + if (method === "DOM.setFileInputFiles") return new Promise(() => {}); + return originalSend(tabId, method, params); + }) as CdpRunner["send"]; + + const result = await uploadThroughActivatedFileInput({ + cdp: fixture.cdp, + actionTarget: actionTarget(), + files: ["/private/stage/one"], + timeoutMs: 10, + trigger: async () => ({ tab_id: 4, x: 10, y: 10 }), + }); + + expect(result).toMatchObject({ + code: "timeout", + data: { effect_state: "unknown", phase: "set_files" }, + }); + expect(detach).toHaveBeenCalledWith(4); + }); + it("routes one exact-target download through a browser-relative capability", async () => { const manager = sessions(); const ctx = await manager.start("s1"); @@ -194,6 +307,7 @@ describe("file transfer tools", () => { onDeterminingFilename, search: vi.fn(async () => [completed]), cancel: vi.fn(async () => {}), + removeFile: vi.fn(async () => {}), }; let cdpEvent: Parameters>[0] | undefined; let suggested: chrome.downloads.DownloadFilenameSuggestion | undefined; @@ -261,6 +375,7 @@ describe("file transfer tools", () => { onDeterminingFilename, search: vi.fn(async () => []), cancel: vi.fn(async () => {}), + removeFile: vi.fn(async () => {}), }; let cdpEvent: Parameters>[0] | undefined; const cdp: CdpRunner = { @@ -304,4 +419,137 @@ describe("file transfer tools", () => { data: { reason: "download_capture_failed" }, }); }); + + it("correlates a filename candidate that arrives before the CDP intent", async () => { + const onCreated = fakeEvent<(item: chrome.downloads.DownloadItem) => void>(); + const onChanged = fakeEvent<(delta: chrome.downloads.DownloadDelta) => void>(); + const onDeterminingFilename = + fakeEvent< + ( + item: chrome.downloads.DownloadItem, + suggest: (suggestion?: chrome.downloads.DownloadFilenameSuggestion) => void, + ) => void | true + >(); + const initial = { + id: 21, + url: "https://example.test/candidate-first.bin", + finalUrl: "https://example.test/candidate-first.bin", + filename: "candidate-first.bin", + state: "in_progress", + fileSize: -1, + totalBytes: 4, + bytesReceived: 0, + } as chrome.downloads.DownloadItem; + const complete = { ...initial, state: "complete", fileSize: 4 } as chrome.downloads.DownloadItem; + const downloads: DownloadsApi = { + onCreated, + onChanged, + onDeterminingFilename, + search: vi.fn(async () => [complete]), + cancel: vi.fn(async () => {}), + removeFile: vi.fn(async () => {}), + }; + let cdpEvent: Parameters>[0] | undefined; + const cdp: CdpRunner = { + send: vi.fn(async () => ({})) as CdpRunner["send"], + onEvent: (handler) => { + cdpEvent = handler; + return { dispose: vi.fn() }; + }, + }; + let suggestion: chrome.downloads.DownloadFilenameSuggestion | undefined; + + const result = await captureBrowserDownload({ + cdp, + target: { tabId: 4 }, + downloads, + browserRelativeDir: "BrowserSkill/tr_21", + timeoutMs: 1_000, + trigger: async () => { + const suggested = new Promise((resolve) => { + onDeterminingFilename.emit(initial, (value) => { + suggestion = value; + resolve(); + }); + }); + cdpEvent?.({ tabId: 4 }, "Page.downloadWillBegin", { + url: initial.url, + suggestedFilename: initial.filename, + }); + await suggested; + onCreated.emit(complete); + return { tab_id: 4, x: 10, y: 10 }; + }, + }); + + expect(suggestion).toEqual({ + filename: "BrowserSkill/tr_21/candidate-first.bin", + conflictAction: "overwrite", + }); + expect(result).toMatchObject({ item: { id: 21, state: "complete" } }); + }); + + it("rejects ambiguous attribution without cancelling either unclaimed download", async () => { + const onCreated = fakeEvent<(item: chrome.downloads.DownloadItem) => void>(); + const onChanged = fakeEvent<(delta: chrome.downloads.DownloadDelta) => void>(); + const onDeterminingFilename = + fakeEvent< + ( + item: chrome.downloads.DownloadItem, + suggest: (suggestion?: chrome.downloads.DownloadFilenameSuggestion) => void, + ) => void | true + >(); + const downloads: DownloadsApi = { + onCreated, + onChanged, + onDeterminingFilename, + search: vi.fn(async () => []), + cancel: vi.fn(async () => {}), + removeFile: vi.fn(async () => {}), + }; + let cdpEvent: Parameters>[0] | undefined; + const cdp: CdpRunner = { + send: vi.fn(async () => ({})) as CdpRunner["send"], + onEvent: (handler) => { + cdpEvent = handler; + return { dispose: vi.fn() }; + }, + }; + const defaults: number[] = []; + const candidate = (id: number) => + ({ + id, + url: "https://example.test/same.bin", + finalUrl: "https://example.test/same.bin", + filename: "same.bin", + state: "in_progress", + }) as chrome.downloads.DownloadItem; + + const result = await captureBrowserDownload({ + cdp, + target: { tabId: 4 }, + downloads, + browserRelativeDir: "BrowserSkill/tr_ambiguous", + timeoutMs: 100, + trigger: async () => { + cdpEvent?.({ tabId: 4 }, "Page.downloadWillBegin", { + url: "https://example.test/same.bin", + suggestedFilename: "same.bin", + }); + for (const id of [31, 32]) { + onDeterminingFilename.emit(candidate(id), (value) => { + if (value === undefined) defaults.push(id); + }); + } + return { tab_id: 4, x: 10, y: 10 }; + }, + }); + + expect(defaults.sort()).toEqual([31, 32]); + expect(downloads.cancel).not.toHaveBeenCalled(); + expect(result).toMatchObject({ + code: "cdp_failed", + data: { effect_state: "unknown", phase: "attribution" }, + }); + }); }); diff --git a/apps/extension/src/tools/download-capture.ts b/apps/extension/src/tools/download-capture.ts index afab991..6d909b2 100644 --- a/apps/extension/src/tools/download-capture.ts +++ b/apps/extension/src/tools/download-capture.ts @@ -1,12 +1,16 @@ -// Transaction-scoped capture for one web download. Chrome owns the actual -// download and writes only beneath its Downloads root; BrowserSkill supplies a -// daemon-minted relative directory and reports the completed absolute path -// back to the daemon for validated import. +// Order-independent coordinator for one browser download. CDP supplies the +// exact target/frame intent while chrome.downloads supplies the download id +// and filename routing hook; neither event is assumed to arrive first. import type { CdpTarget } from "@/browser-driver/frame-graph"; -import type { ClickResult, RpcError } from "@/transport/types"; +import type { ClickResult, RpcError, TransferEffectState } from "@/transport/types"; +import { transferError } from "./errors"; import { type CdpRunner, isRpcError } from "./shared"; +const CORRELATION_GRACE_MS = 750; +const UNIQUE_SETTLE_MS = 50; +const SIZE_POLL_MS = 250; + type DeterminingFilenameListener = ( item: chrome.downloads.DownloadItem, suggest: (suggestion?: chrome.downloads.DownloadFilenameSuggestion) => void, @@ -23,6 +27,7 @@ export interface DownloadsApi { onDeterminingFilename: ListenerEvent; search(query: chrome.downloads.DownloadQuery): Promise; cancel(downloadId: number): Promise; + removeFile(downloadId: number): Promise; } export const chromeDownloadsApi: DownloadsApi = { @@ -37,13 +42,16 @@ export const chromeDownloadsApi: DownloadsApi = { }, search: (query) => chrome.downloads.search(query), cancel: (id) => chrome.downloads.cancel(id), + removeFile: (id) => chrome.downloads.removeFile(id), }; export interface DownloadCaptureOptions { cdp: CdpRunner; target: CdpTarget; + expectedFrameId?: string; downloads: DownloadsApi; browserRelativeDir: string; + maxByteSize?: number; timeoutMs: number; signal?: AbortSignal; trigger(): Promise; @@ -54,43 +62,70 @@ export interface DownloadCaptureResult { item: chrome.downloads.DownloadItem; } +interface DownloadIntent { + url: string; + suggestedFilename: string; + frameId?: string; +} + +interface DownloadCandidate { + item: chrome.downloads.DownloadItem; + suggest: (suggestion?: chrome.downloads.DownloadFilenameSuggestion) => void; + suggested: boolean; + graceTimer: ReturnType; +} + function safeBasename(filename: string): string { const basename = filename.split(/[\\/]/).pop()?.trim(); return basename && basename !== "." && basename !== ".." ? basename : "download"; } -function captureError(message: string): RpcError { - return { - code: "cdp_failed", - message, - data: { reason: "download_capture_failed" }, - }; -} - function sameTarget(source: { tabId?: number; sessionId?: string }, target: CdpTarget): boolean { return source.tabId === target.tabId && source.sessionId === target.sessionId; } -function matchesIntent( - item: chrome.downloads.DownloadItem, - intent: { url: string; suggestedFilename: string }, -): boolean { +function matchesIntent(item: chrome.downloads.DownloadItem, intent: DownloadIntent): boolean { const urlMatches = item.url === intent.url || item.finalUrl === intent.url; return urlMatches && safeBasename(item.filename) === safeBasename(intent.suggestedFilename); } +function knownSize(item: chrome.downloads.DownloadItem): number | undefined { + if (item.fileSize >= 0) return item.fileSize; + if (item.totalBytes >= 0) return item.totalBytes; + return undefined; +} + +function captureError( + message: string, + effectState: TransferEffectState, + phase: string, + cleanupFailed = false, +): RpcError { + return transferError("cdp_failed", "download_capture_failed", message, { + effectState, + phase, + ...(cleanupFailed ? { cleanupState: "failed" } : {}), + }); +} + export async function captureBrowserDownload( options: DownloadCaptureOptions, ): Promise { + let click: ClickResult | undefined; + let intent: DownloadIntent | undefined; let capturedId: number | undefined; - let intent: { url: string; suggestedFilename: string } | undefined; - const createdItems = new Map(); + let capturedItem: chrome.downloads.DownloadItem | undefined; let settled = false; let succeeded = false; - let timer: ReturnType | undefined; - let rejectCompletion!: (error: Error) => void; - let resolveCompletion!: (item: chrome.downloads.DownloadItem) => void; + let failureResult: RpcError | undefined; + let uniquenessTimer: ReturnType | undefined; + let operationTimer: ReturnType | undefined; + let sizePoll: ReturnType | undefined; + const candidates = new Map(); + const createdItems = new Map(); + let resolveCompletion!: (item: chrome.downloads.DownloadItem) => void; + let rejectCompletion!: (error: Error) => void; const completion = new Promise((resolve, reject) => { resolveCompletion = resolve; rejectCompletion = reject; @@ -102,40 +137,95 @@ export async function captureBrowserDownload( }; const complete = (item: chrome.downloads.DownloadItem) => { if (settled) return; + const size = knownSize(item); + if (size !== undefined && options.maxByteSize !== undefined && size > options.maxByteSize) { + fail(new Error(`download exceeds transfer limit ${options.maxByteSize}`)); + return; + } settled = true; + capturedItem = item; resolveCompletion(item); }; - const claim = (item: chrome.downloads.DownloadItem): boolean => { - if (capturedId === undefined) { - capturedId = item.id; - return true; + const suggestDefault = (candidate: DownloadCandidate) => { + if (candidate.suggested) return; + candidate.suggested = true; + candidate.suggest(); + }; + const matchingCandidates = (): DownloadCandidate[] => { + const currentIntent = intent; + return currentIntent + ? [...candidates.values()].filter( + (candidate) => !candidate.suggested && matchesIntent(candidate.item, currentIntent), + ) + : []; + }; + + const claimUnique = () => { + uniquenessTimer = undefined; + if (settled || capturedId !== undefined || !intent) return; + const matches = matchingCandidates(); + if (matches.length !== 1) { + if (matches.length > 1) { + for (const candidate of matches) suggestDefault(candidate); + fail(new Error("download attribution is ambiguous")); + } + return; + } + const candidate = matches[0]; + candidate.suggested = true; + clearTimeout(candidate.graceTimer); + capturedId = candidate.item.id; + capturedItem = candidate.item; + candidate.suggest({ + filename: `${options.browserRelativeDir}/${safeBasename(intent.suggestedFilename)}`, + conflictAction: "overwrite", + }); + const size = knownSize(candidate.item); + if (size !== undefined && options.maxByteSize !== undefined && size > options.maxByteSize) { + fail(new Error(`download exceeds transfer limit ${options.maxByteSize}`)); + return; + } + const created = createdItems.get(candidate.item.id); + if (created?.state === "interrupted") { + fail(new Error(created.error ?? "download interrupted")); + } else if (created?.state === "complete") { + complete(created); + } + }; + const reconcile = () => { + if (settled || capturedId !== undefined || !intent) return; + const matches = matchingCandidates(); + if (matches.length > 1) { + for (const candidate of matches) suggestDefault(candidate); + fail(new Error("download attribution is ambiguous")); + return; + } + if (matches.length === 1 && !uniquenessTimer) { + uniquenessTimer = setTimeout(claimUnique, UNIQUE_SETTLE_MS); } - if (capturedId === item.id) return true; - void options.downloads.cancel(item.id).catch(() => undefined); - fail(new Error("download trigger produced more than one file")); - return false; }; const determiningListener: DeterminingFilenameListener = (item, suggest) => { - try { - if (!intent || !matchesIntent(item, intent) || !claim(item)) { - suggest(); - return; - } - suggest({ - filename: `${options.browserRelativeDir}/${safeBasename(intent.suggestedFilename)}`, - conflictAction: "overwrite", - }); - const created = createdItems.get(item.id); - if (created?.state === "complete") complete(created); - } catch (err) { - suggest(); - fail(err instanceof Error ? err : new Error(String(err))); - } + const candidate: DownloadCandidate = { + item, + suggest, + suggested: false, + graceTimer: setTimeout(() => { + suggestDefault(candidate); + candidates.delete(item.id); + if (intent && matchesIntent(item, intent) && capturedId === undefined) { + fail(new Error("download correlation grace elapsed before unique attribution")); + } + }, CORRELATION_GRACE_MS), + }; + candidates.set(item.id, candidate); + reconcile(); + return true; }; const createdListener = (item: chrome.downloads.DownloadItem) => { createdItems.set(item.id, item); if (capturedId !== item.id) return; + capturedItem = item; if (item.state === "interrupted") { fail(new Error(item.error ?? "download interrupted")); } else if (item.state === "complete") { @@ -158,54 +248,100 @@ export async function captureBrowserDownload( } } }; - const onAbort = () => { - fail(new DOMException("aborted", "AbortError")); - }; + const onAbort = () => fail(new DOMException("aborted", "AbortError")); const cdpSubscription = options.cdp.onEvent?.((source, method, raw) => { if (method !== "Page.downloadWillBegin" || !sameTarget(source, options.target)) return; - const event = raw as { url?: unknown; suggestedFilename?: unknown }; + const event = raw as { url?: unknown; suggestedFilename?: unknown; frameId?: unknown }; if (typeof event.url !== "string" || typeof event.suggestedFilename !== "string") return; + if (options.expectedFrameId && event.frameId !== options.expectedFrameId) { + fail(new Error("download originated from a different frame")); + return; + } if (intent) { fail(new Error("download trigger produced more than one browser download intent")); return; } - intent = { url: event.url, suggestedFilename: event.suggestedFilename }; + intent = { + url: event.url, + suggestedFilename: event.suggestedFilename, + ...(typeof event.frameId === "string" ? { frameId: event.frameId } : {}), + }; + reconcile(); }); if (!cdpSubscription) { - return captureError("CDP download intent subscription unavailable"); + return captureError("CDP download intent subscription unavailable", "none", "arm"); } options.downloads.onDeterminingFilename.addListener(determiningListener); options.downloads.onCreated.addListener(createdListener); options.downloads.onChanged.addListener(changedListener); options.signal?.addEventListener("abort", onAbort, { once: true }); - timer = setTimeout( + operationTimer = setTimeout( () => fail(new Error("download did not complete before timeout")), options.timeoutMs, ); + sizePoll = setInterval(() => { + if (capturedId === undefined || settled || options.maxByteSize === undefined) return; + void options.downloads + .search({ id: capturedId }) + .then(([item]) => { + if (!item || settled) return; + capturedItem = item; + if (item.bytesReceived > (options.maxByteSize as number)) { + fail(new Error(`download exceeds transfer limit ${options.maxByteSize}`)); + } + }) + .catch((err) => fail(err instanceof Error ? err : new Error(String(err)))); + }, SIZE_POLL_MS); try { const triggered = await options.trigger(); if (isRpcError(triggered)) { void completion.catch(() => undefined); - return triggered; + const effect: TransferEffectState = capturedId !== undefined ? "committed" : intent ? "unknown" : "none"; + failureResult = { + ...triggered, + data: { ...triggered.data, effect_state: effect, phase: "trigger" }, + }; + return failureResult; } + click = triggered; const item = await completion; succeeded = true; - return { click: triggered, item }; + return { click, item }; } catch (err) { - if (err instanceof DOMException && err.name === "AbortError") throw err; - return captureError(err instanceof Error ? err.message : String(err)); + const effect: TransferEffectState = capturedId !== undefined ? "committed" : click ? "unknown" : "none"; + failureResult = captureError( + err instanceof Error ? err.message : String(err), + effect, + capturedId !== undefined ? "download" : "attribution", + ); + return failureResult; } finally { settled = true; - if (timer) clearTimeout(timer); - if (!succeeded && capturedId !== undefined) { - await options.downloads.cancel(capturedId).catch(() => undefined); - } + if (operationTimer) clearTimeout(operationTimer); + if (uniquenessTimer) clearTimeout(uniquenessTimer); + if (sizePoll) clearInterval(sizePoll); options.signal?.removeEventListener("abort", onAbort); options.downloads.onDeterminingFilename.removeListener(determiningListener); options.downloads.onCreated.removeListener(createdListener); options.downloads.onChanged.removeListener(changedListener); cdpSubscription.dispose(); + for (const candidate of candidates.values()) { + clearTimeout(candidate.graceTimer); + if (candidate.item.id !== capturedId) suggestDefault(candidate); + } + if (!succeeded && capturedId !== undefined) { + const item = capturedItem ?? createdItems.get(capturedId); + try { + if (item?.state === "complete") { + await options.downloads.removeFile(capturedId); + } else { + await options.downloads.cancel(capturedId); + } + } catch { + if (failureResult?.data) failureResult.data.cleanup_state = "failed"; + } + } } } diff --git a/apps/extension/src/tools/download.ts b/apps/extension/src/tools/download.ts index d337315..fb2c586 100644 --- a/apps/extension/src/tools/download.ts +++ b/apps/extension/src/tools/download.ts @@ -2,9 +2,13 @@ // browser-global chrome.downloads transaction to download-capture.ts. import type { SessionManager } from "@/session-manager/manager"; -import type { ClickParams, DownloadParams, DownloadResult, RpcError } from "@/transport/types"; +import type { DownloadParams, DownloadResult, RpcError } from "@/transport/types"; import { captureBrowserDownload, chromeDownloadsApi, type DownloadsApi } from "./download-capture"; -import { handleClick, type InteractionDeps, resolveBackendNode } from "./interaction"; +import { + clickResolvedTarget, + type InteractionDeps, + resolveActionTarget, +} from "./interaction"; import { enforceAgentWindow, isRpcError, lookupSession, resolveTargetTab } from "./shared"; let downloadActive = false; @@ -32,7 +36,7 @@ export async function handleDownload( if (!params.browser_relative_dir) { return { code: "invalid_params", message: "download requires a daemon capability directory" }; } - const address = await resolveBackendNode(deps.cdp, ctx, target, params, "download"); + const address = await resolveActionTarget(deps.cdp, ctx, target, params, "download"); if (isRpcError(address)) return address; const capture = await captureBrowserDownload({ @@ -40,18 +44,11 @@ export async function handleDownload( target: address.cdpTarget, downloads: deps.downloads ?? chromeDownloadsApi, browserRelativeDir: params.browser_relative_dir, + maxByteSize: params.max_byte_size, timeoutMs: params.timeout_ms ?? 120_000, signal: deps.signal, - trigger: () => { - const clickParams: ClickParams = { - session_id: params.session_id, - ref: params.ref, - selector: params.selector, - tab_id: params.tab_id, - timeout_ms: params.timeout_ms, - }; - return handleClick(manager, clickParams, deps); - }, + expectedFrameId: address.frameId, + trigger: () => clickResolvedTarget(ctx, address, {}, deps), }); if (isRpcError(capture)) return capture; const { click, item } = capture; diff --git a/apps/extension/src/tools/errors.ts b/apps/extension/src/tools/errors.ts index 88cfd78..800a765 100644 --- a/apps/extension/src/tools/errors.ts +++ b/apps/extension/src/tools/errors.ts @@ -2,10 +2,23 @@ // rendering. Extension handlers attach reasons here; human-facing copy // lives in bsk-cli `render_error.rs`. -import type { ErrorCode, RpcError, RpcErrorData, RpcErrorReason } from "@/transport/types"; +import type { + ErrorCode, + RpcError, + RpcErrorData, + RpcErrorReason, + TransferCleanupState, + TransferEffectState, +} from "@/transport/types"; export type { RpcErrorData, RpcErrorReason }; +export interface TransferErrorOptions { + effectState: TransferEffectState; + phase: string; + cleanupState?: TransferCleanupState; +} + export function rpcError( code: ErrorCode, reason: RpcErrorReason, @@ -15,3 +28,16 @@ export function rpcError( const data: RpcErrorData = { reason, ...extra }; return { code, message, data }; } + +export function transferError( + code: ErrorCode, + reason: RpcErrorReason, + message: string, + options: TransferErrorOptions, +): RpcError { + return rpcError(code, reason, message, { + effect_state: options.effectState, + phase: options.phase, + ...(options.cleanupState ? { cleanup_state: options.cleanupState } : {}), + }); +} diff --git a/apps/extension/src/tools/file-input-transaction.ts b/apps/extension/src/tools/file-input-transaction.ts index 50297ad..fb3901f 100644 --- a/apps/extension/src/tools/file-input-transaction.ts +++ b/apps/extension/src/tools/file-input-transaction.ts @@ -1,35 +1,43 @@ -// One upload transaction, independent of Page.fileChooserOpened delivery. -// A transaction-scoped DOM listener records the file input actually activated -// by the requested click and cancels its native default action. File System -// Access picker entry points are replaced only for the same transaction, so a -// non-input picker fails promptly without opening an OS dialog. +// One upload transaction with an explicit browser-side commit boundary. +// Chrome interception prevents a native chooser from escaping automation; +// chooser events and a frame-scoped DOM probe are independent input-location +// signals, so event delivery is not required for standard file inputs. -import type { CdpTarget } from "@/browser-driver/frame-graph"; -import type { ClickResult, RpcError } from "@/transport/types"; +import type { ClickResult, RpcError, TransferEffectState } from "@/transport/types"; +import { transferError } from "./errors"; +import type { ResolvedActionTarget } from "./interaction"; import { type CdpRunner, isRpcError, sendToCdpTarget } from "./shared"; const CLEANUP_TIMEOUT_MS = 1_000; +const ACTIVATION_GRACE_MS = 1_000; +const PROBE_INTERVAL_MS = 20; -type UploadPhase = "arm_input_probe" | "trigger" | "resolve_input" | "set_files"; +type UploadPhase = + | "arm_interception" + | "arm_input_probe" + | "trigger" + | "resolve_input" + | "set_files" + | "cleanup"; interface RuntimeReply { - result?: { - value?: unknown; - objectId?: string; - }; - exceptionDetails?: { - text?: string; - exception?: { description?: string }; - }; + result?: { value?: unknown; objectId?: string }; + exceptionDetails?: { text?: string; exception?: { description?: string } }; +} + +interface ChooserEvent { + frameId?: string; + backendNodeId?: number; + mode?: "selectSingle" | "selectMultiple"; } export interface FileInputTransactionOptions { cdp: CdpRunner; - target: CdpTarget; + actionTarget: ResolvedActionTarget; files: string[]; timeoutMs: number; signal?: AbortSignal; - trigger(timeoutMs: number): Promise; + trigger(): Promise; } export interface FileInputTransactionResult { @@ -38,10 +46,7 @@ export interface FileInputTransactionResult { } class BoundedWaitError extends Error { - constructor( - readonly kind: "timeout" | "aborted", - message: string, - ) { + constructor(readonly kind: "timeout" | "aborted", message: string) { super(message); } } @@ -77,15 +82,6 @@ async function waitBounded( } } -function uploadError( - code: RpcError["code"], - message: string, - reason: "file_input_probe_failed" | "file_input_not_activated" | "set_file_input_failed", - phase: UploadPhase, -): RpcError { - return { code, message, data: { reason, phase } }; -} - function runtimeError(reply: RuntimeReply, fallback: string): Error | null { if (!reply.exceptionDetails) return null; return new Error( @@ -93,25 +89,124 @@ function runtimeError(reply: RuntimeReply, fallback: string): Error | null { ); } +function transferFailure( + code: RpcError["code"], + reason: "file_input_probe_failed" | "file_input_not_activated" | "set_file_input_failed", + message: string, + effectState: TransferEffectState, + phase: UploadPhase, +): RpcError { + return transferError(code, reason, message, { effectState, phase }); +} + +function enrichFailure(error: RpcError, effectState: TransferEffectState, phase: UploadPhase): RpcError { + return { + ...error, + data: { ...error.data, effect_state: effectState, phase }, + }; +} + +function sameTarget( + source: { tabId?: number; sessionId?: string }, + target: { tabId: number; sessionId?: string }, +): boolean { + return source.tabId === target.tabId && source.sessionId === target.sessionId; +} + +async function delay(ms: number, signal?: AbortSignal): Promise { + await waitBounded( + new Promise((resolve) => setTimeout(resolve, ms)), + Date.now() + ms + 1, + signal, + "upload activation probe timed out", + ); +} + +async function callOnTrigger( + options: FileInputTransactionOptions, + objectId: string, + functionDeclaration: string, + args: unknown[], + returnByValue: boolean, +): Promise { + return sendToCdpTarget(options.cdp, options.actionTarget.cdpTarget, "Runtime.callFunctionOn", { + objectId, + functionDeclaration, + arguments: args.map((value) => ({ value })), + returnByValue, + awaitPromise: false, + }); +} + export async function uploadThroughActivatedFileInput( options: FileInputTransactionOptions, ): Promise { const deadline = Date.now() + options.timeoutMs; + const target = options.actionTarget.cdpTarget; const objectGroup = `bsk-upload-${crypto.randomUUID()}`; const stateKey = `__bskUpload_${crypto.randomUUID().replaceAll("-", "")}`; - const stateKeyLiteral = JSON.stringify(stateKey); - let probeAttempted = false; + const chooserEvents: ChooserEvent[] = []; + let interceptionArmed = false; + let probeArmed = false; + let triggerObjectId: string | undefined; + let outcome: FileInputTransactionResult | RpcError = transferFailure( + "protocol_error", + "file_input_probe_failed", + "upload transaction ended without an outcome", + "none", + "cleanup", + ); + + const chooserSubscription = options.cdp.onEvent?.((source, method, raw) => { + if (method !== "Page.fileChooserOpened" || !sameTarget(source, target)) return; + const event = raw as ChooserEvent; + chooserEvents.push(event); + }); try { try { - probeAttempted = true; + await waitBounded( + sendToCdpTarget(options.cdp, target, "Page.setInterceptFileChooserDialog", { + enabled: true, + }), + deadline, + options.signal, + "arming native file chooser interception timed out", + ); + interceptionArmed = true; + } catch (err) { + outcome = transferFailure( + err instanceof BoundedWaitError ? "timeout" : "cdp_failed", + "file_input_probe_failed", + err instanceof Error ? err.message : String(err), + "none", + "arm_interception", + ); + return outcome; + } + + try { + const resolved = await waitBounded( + sendToCdpTarget<{ object?: { objectId?: string } }>(options.cdp, target, "DOM.resolveNode", { + backendNodeId: options.actionTarget.backendNodeId, + objectGroup, + }), + deadline, + options.signal, + "resolving upload trigger timed out", + ); + triggerObjectId = resolved.object?.objectId; + if (!triggerObjectId) throw new Error("DOM.resolveNode returned no trigger objectId"); + const armed = await waitBounded( - sendToCdpTarget(options.cdp, options.target, "Runtime.evaluate", { - expression: `(() => { - const key = ${stateKeyLiteral}; - const owner = globalThis; - const doc = document; - const state = { inputs: [], listener: null, pickerCalls: [], pickers: [] }; + callOnTrigger( + options, + triggerObjectId, + `function(key) { + const doc = this.ownerDocument; + const owner = doc.defaultView; + if (!owner) return false; + const state = { inputs: [], listener: null }; Object.defineProperty(owner, key, { value: state, configurable: true }); state.listener = event => { const path = typeof event.composedPath === "function" ? event.composedPath() : []; @@ -123,143 +218,169 @@ export async function uploadThroughActivatedFileInput( } }; doc.addEventListener("click", state.listener, true); - const win = doc.defaultView; - for (const name of ["showOpenFilePicker", "showSaveFilePicker", "showDirectoryPicker"]) { - if (!win || typeof win[name] !== "function") continue; - const hadOwn = Object.prototype.hasOwnProperty.call(win, name); - const descriptor = Object.getOwnPropertyDescriptor(win, name); - state.pickers.push({ name, hadOwn, descriptor }); - Object.defineProperty(win, name, { - configurable: true, - enumerable: descriptor?.enumerable ?? true, - writable: true, - value: () => { - state.pickerCalls.push(name); - return Promise.reject( - new DOMException("Picker intercepted by BrowserSkill", "AbortError") - ); - }, - }); - } return true; - })()`, - objectGroup, - returnByValue: true, - }), + }`, + [stateKey], + true, + ), deadline, options.signal, - "arming file input probe timed out", + "arming frame-scoped file input probe timed out", ); const armError = runtimeError(armed, "failed to arm file input probe"); - if (armError) throw armError; + if (armError || armed.result?.value !== true) { + throw armError ?? new Error("file input probe did not arm"); + } + probeArmed = true; } catch (err) { - if (err instanceof BoundedWaitError && err.kind === "aborted") throw err; - return uploadError( + outcome = transferFailure( err instanceof BoundedWaitError ? "timeout" : "cdp_failed", - err instanceof Error ? err.message : String(err), "file_input_probe_failed", + err instanceof Error ? err.message : String(err), + "none", "arm_input_probe", ); + return outcome; } let click: ClickResult | RpcError; try { click = await waitBounded( - options.trigger(remainingMs(deadline)), + options.trigger(), deadline, options.signal, "upload trigger timed out", ); } catch (err) { - if (err instanceof BoundedWaitError && err.kind === "aborted") throw err; - return uploadError( + outcome = transferFailure( err instanceof BoundedWaitError ? "timeout" : "cdp_failed", - err instanceof Error ? err.message : String(err), "file_input_probe_failed", + err instanceof Error ? err.message : String(err), + "none", "trigger", ); + return outcome; + } + if (isRpcError(click)) { + outcome = enrichFailure(click, "none", "trigger"); + return outcome; } - if (isRpcError(click)) return click; try { - const summary = await waitBounded( - sendToCdpTarget(options.cdp, options.target, "Runtime.evaluate", { - expression: `(() => { - const s = globalThis[${stateKeyLiteral}]; - return s - ? { count: s.inputs.length, multiple: s.inputs[0]?.multiple === true, - pickerCall: s.pickerCalls[0] } + const activationDeadline = Math.min(deadline, Date.now() + ACTIVATION_GRACE_MS); + let summary: { count: number; multiple: boolean } = { count: 0, multiple: false }; + while (Date.now() < activationDeadline) { + if (chooserEvents.length > 0) break; + const reply = await callOnTrigger( + options, + triggerObjectId, + `function(key) { + const state = this.ownerDocument.defaultView?.[key]; + return state + ? { count: state.inputs.length, multiple: state.inputs[0]?.multiple === true } : { count: 0, multiple: false }; - })()`, - returnByValue: true, - }), - deadline, - options.signal, - "resolving activated file input timed out", - ); - const summaryError = runtimeError(summary, "failed to inspect activated file input"); - if (summaryError) throw summaryError; - const value = summary.result?.value as - | { count?: unknown; multiple?: unknown; pickerCall?: unknown } - | undefined; - const count = typeof value?.count === "number" ? value.count : 0; - const multiple = value?.multiple === true; - if (typeof value?.pickerCall === "string") { - return uploadError( - "unsupported", - `upload trigger invoked ${value.pickerCall} instead of an input[type=file]`, - "file_input_not_activated", - "resolve_input", + }`, + [stateKey], + true, ); - } - if (count !== 1) { - return uploadError( - "unsupported", - count === 0 - ? "upload trigger did not activate an input[type=file]" - : "upload trigger activated more than one input[type=file]", - "file_input_not_activated", - "resolve_input", - ); - } - if (!multiple && options.files.length !== 1) { - return { code: "invalid_params", message: "file input accepts exactly one file" }; + const summaryError = runtimeError(reply, "failed to inspect activated file input"); + if (summaryError) throw summaryError; + const value = reply.result?.value as { count?: unknown; multiple?: unknown } | undefined; + summary = { + count: typeof value?.count === "number" ? value.count : 0, + multiple: value?.multiple === true, + }; + if (summary.count > 0) break; + await delay(Math.min(PROBE_INTERVAL_MS, remainingMs(activationDeadline)), options.signal); } - const input = await waitBounded( - sendToCdpTarget(options.cdp, options.target, "Runtime.evaluate", { - expression: `globalThis[${stateKeyLiteral}]?.inputs[0]`, - objectGroup, - returnByValue: false, - }), - deadline, - options.signal, - "resolving activated file input object timed out", - ); - const inputError = runtimeError(input, "failed to resolve activated file input object"); - if (inputError) throw inputError; - const inputObjectId = input.result?.objectId; - if (!inputObjectId) throw new Error("activated file input returned no objectId"); + if (chooserEvents.length > 1) { + outcome = transferFailure( + "unsupported", + "file_input_not_activated", + "upload trigger activated more than one file chooser", + "none", + "resolve_input", + ); + return outcome; + } - const described = await waitBounded( - sendToCdpTarget<{ node?: { backendNodeId?: number } }>( + const chooser = chooserEvents[0]; + let backendNodeId: number | undefined; + let multiple = summary.multiple; + if (chooser) { + if ( + options.actionTarget.frameId && + chooser.frameId !== options.actionTarget.frameId + ) { + outcome = transferFailure( + "unsupported", + "file_input_not_activated", + "upload trigger activated a file chooser in a different frame", + "none", + "resolve_input", + ); + return outcome; + } + if (typeof chooser.backendNodeId !== "number") { + outcome = transferFailure( + "unsupported", + "file_input_not_activated", + "upload trigger invoked a non-input file picker", + "none", + "resolve_input", + ); + return outcome; + } + backendNodeId = chooser.backendNodeId; + multiple = chooser.mode === "selectMultiple"; + } else { + if (summary.count !== 1) { + outcome = transferFailure( + "unsupported", + "file_input_not_activated", + summary.count === 0 + ? "upload trigger did not activate an input[type=file]" + : "upload trigger activated more than one input[type=file]", + "none", + "resolve_input", + ); + return outcome; + } + const input = await callOnTrigger( + options, + triggerObjectId, + `function(key) { return this.ownerDocument.defaultView?.[key]?.inputs[0]; }`, + [stateKey], + false, + ); + const inputError = runtimeError(input, "failed to resolve activated file input object"); + if (inputError) throw inputError; + if (!input.result?.objectId) throw new Error("activated file input returned no objectId"); + const described = await sendToCdpTarget<{ node?: { backendNodeId?: number } }>( options.cdp, - options.target, + target, "DOM.describeNode", - { objectId: inputObjectId }, - ), - deadline, - options.signal, - "describing activated file input timed out", - ); - const backendNodeId = described.node?.backendNodeId; - if (typeof backendNodeId !== "number") { - throw new Error("DOM.describeNode returned no file input backendNodeId"); + { objectId: input.result.objectId }, + ); + backendNodeId = described.node?.backendNodeId; + if (typeof backendNodeId !== "number") { + throw new Error("DOM.describeNode returned no file input backendNodeId"); + } + } + + if (!multiple && options.files.length !== 1) { + outcome = enrichFailure( + { code: "invalid_params", message: "file input accepts exactly one file" }, + "none", + "resolve_input", + ); + return outcome; } try { await waitBounded( - sendToCdpTarget(options.cdp, options.target, "DOM.setFileInputFiles", { + sendToCdpTarget(options.cdp, target, "DOM.setFileInputFiles", { files: options.files, backendNodeId, }), @@ -268,72 +389,87 @@ export async function uploadThroughActivatedFileInput( "setting file input files timed out", ); } catch (err) { - if (err instanceof BoundedWaitError && err.kind === "aborted") throw err; - return uploadError( + outcome = transferFailure( err instanceof BoundedWaitError ? "timeout" : "cdp_failed", - err instanceof Error ? err.message : String(err), "set_file_input_failed", + err instanceof Error ? err.message : String(err), + "unknown", "set_files", ); + return outcome; } - return { click, multiple }; + outcome = { click, multiple }; + return outcome; } catch (err) { - if (err instanceof BoundedWaitError && err.kind === "aborted") throw err; - return uploadError( + outcome = transferFailure( err instanceof BoundedWaitError ? "timeout" : "cdp_failed", - err instanceof Error ? err.message : String(err), "file_input_probe_failed", + err instanceof Error ? err.message : String(err), + "none", "resolve_input", ); + return outcome; } - } catch (err) { - if (err instanceof BoundedWaitError && err.kind === "aborted") { - return { code: "cancelled", message: "upload transaction aborted" }; - } - throw err; } finally { - if (probeAttempted) { + chooserSubscription?.dispose(); + let cleanupFailed = false; + if (probeArmed && triggerObjectId) { try { await waitBounded( - sendToCdpTarget(options.cdp, options.target, "Runtime.evaluate", { - expression: `(() => { - const key = ${stateKeyLiteral}; - const state = globalThis[key]; - if (state?.listener) { - document.removeEventListener("click", state.listener, true); - } - const win = document.defaultView; - if (win) { - for (const picker of state?.pickers || []) { - try { - if (picker.hadOwn && picker.descriptor) { - Object.defineProperty(win, picker.name, picker.descriptor); - } else { - delete win[picker.name]; - } - } catch {} - } - } - delete globalThis[key]; - })()`, - }), + callOnTrigger( + options, + triggerObjectId, + `function(key) { + const owner = this.ownerDocument.defaultView; + const state = owner?.[key]; + if (state?.listener) this.ownerDocument.removeEventListener("click", state.listener, true); + if (owner) delete owner[key]; + }`, + [stateKey], + true, + ), Date.now() + CLEANUP_TIMEOUT_MS, undefined, "cleaning file input probe timed out", ); } catch { - // Navigation may have invalidated the object; its document is gone too. + cleanupFailed = true; + } + } + if (interceptionArmed) { + try { + await waitBounded( + sendToCdpTarget(options.cdp, target, "Page.setInterceptFileChooserDialog", { + enabled: false, + cancel: true, + }), + Date.now() + CLEANUP_TIMEOUT_MS, + undefined, + "disabling file chooser interception timed out", + ); + } catch { + cleanupFailed = true; } } try { await waitBounded( - sendToCdpTarget(options.cdp, options.target, "Runtime.releaseObjectGroup", { - objectGroup, - }), + sendToCdpTarget(options.cdp, target, "Runtime.releaseObjectGroup", { objectGroup }), Date.now() + CLEANUP_TIMEOUT_MS, undefined, "releasing upload object group timed out", ); - } catch {} + } catch { + cleanupFailed = true; + } + + const effect = isRpcError(outcome) + ? (outcome.data?.effect_state as TransferEffectState | undefined) + : "committed"; + if (effect === "unknown" || cleanupFailed) { + await options.cdp.detach?.(target.tabId); + } + if (cleanupFailed && isRpcError(outcome)) { + outcome.data = { ...outcome.data, cleanup_state: "failed" }; + } } } diff --git a/apps/extension/src/tools/interaction.ts b/apps/extension/src/tools/interaction.ts index 3ea1373..54646ed 100644 --- a/apps/extension/src/tools/interaction.ts +++ b/apps/extension/src/tools/interaction.ts @@ -40,6 +40,7 @@ import { enforceAgentWindow, isRpcError, lookupSession, + type ResolvedTargetTab, resolveTargetTab, } from "./shared"; import { resolveSnapshotRef } from "./snapshot-ref"; @@ -56,6 +57,15 @@ export interface InteractionDeps { keepOverlayBypassAfterHover?: boolean; } +export interface ResolvedActionTarget { + tab: ResolvedTargetTab; + backendNodeId: number; + cdpTarget: CdpTarget; + frameId?: string; + usedRef?: string; + usedSelector?: string; +} + const DEFAULT_TIMEOUT_MS = 30_000; const DEFAULT_HOVER_SETTLE_MS = 200; @@ -215,6 +225,17 @@ export async function resolveBackendNode( } } +export async function resolveActionTarget( + cdp: CdpRunner, + ctx: SessionContext, + target: ResolvedTargetTab, + params: { ref?: string; selector?: string }, + toolName: string, +): Promise { + const node = await resolveBackendNode(cdp, ctx, target, params, toolName); + return isRpcError(node) ? node : { tab: target, ...node }; +} + // --------------------------------------------------------------------------- // tool.click // --------------------------------------------------------------------------- @@ -233,10 +254,19 @@ export async function handleClick( if (isRpcError(target)) return target; const denied = enforceAgentWindow(ctx, target, "click"); if (denied) return denied; - const dialogCursor = markDialogCursor(deps.cdp, target.tabId); + const resolved = await resolveActionTarget(deps.cdp, ctx, target, params, "click"); + if (isRpcError(resolved)) return resolved; + return clickResolvedTarget(ctx, resolved, params, deps); +} - const node = await resolveBackendNode(deps.cdp, ctx, target, params, "click"); - if (isRpcError(node)) return node; +export async function clickResolvedTarget( + ctx: SessionContext, + resolved: ResolvedActionTarget, + params: Pick, + deps: InteractionDeps, +): Promise { + const { tab: target } = resolved; + const dialogCursor = markDialogCursor(deps.cdp, target.tabId); if (throwIfAborted(deps.signal)) { return { code: "cancelled", message: "click aborted" }; @@ -247,9 +277,9 @@ export async function handleClick( deps.cdp, target.tabId, { - target: node.cdpTarget, - backendNodeId: node.backendNodeId, - ...(node.frameId ? { frameId: node.frameId } : {}), + target: resolved.cdpTarget, + backendNodeId: resolved.backendNodeId, + ...(resolved.frameId ? { frameId: resolved.frameId } : {}), }, { scrollIntoView: true }, ); @@ -337,8 +367,8 @@ export async function handleClick( return attachDialogs(deps.cdp, target.tabId, dialogCursor, { tab_id: target.tabId, - used_ref: node.usedRef, - used_selector: node.usedSelector, + used_ref: resolved.usedRef, + used_selector: resolved.usedSelector, x: centre.x, y: centre.y, }); diff --git a/apps/extension/src/tools/upload.ts b/apps/extension/src/tools/upload.ts index 6622bc4..58370e4 100644 --- a/apps/extension/src/tools/upload.ts +++ b/apps/extension/src/tools/upload.ts @@ -3,9 +3,13 @@ // transaction module. import type { SessionManager } from "@/session-manager/manager"; -import type { ClickParams, RpcError, UploadParams, UploadResult } from "@/transport/types"; +import type { RpcError, UploadParams, UploadResult } from "@/transport/types"; import { uploadThroughActivatedFileInput } from "./file-input-transaction"; -import { handleClick, type InteractionDeps, resolveBackendNode } from "./interaction"; +import { + clickResolvedTarget, + type InteractionDeps, + resolveActionTarget, +} from "./interaction"; import { type CdpRunner, enforceAgentWindow, @@ -38,25 +42,16 @@ export async function handleUpload( ) { return { code: "invalid_params", message: "upload requires daemon-staged files" }; } - const address = await resolveBackendNode(deps.cdp, ctx, target, params, "upload"); + const address = await resolveActionTarget(deps.cdp, ctx, target, params, "upload"); if (isRpcError(address)) return address; const timeoutMs = params.timeout_ms ?? DEFAULT_TIMEOUT_MS; const transaction = await uploadThroughActivatedFileInput({ cdp: deps.cdp, - target: address.cdpTarget, + actionTarget: address, files: params.files.map((file) => file.staged_path as string), timeoutMs, signal: deps.signal, - trigger: (remaining) => { - const clickParams: ClickParams = { - session_id: params.session_id, - ref: params.ref, - selector: params.selector, - tab_id: params.tab_id, - timeout_ms: Math.max(1, remaining), - }; - return handleClick(manager, clickParams, deps); - }, + trigger: () => clickResolvedTarget(ctx, address, {}, deps), }); if (isRpcError(transaction)) return transaction; return { diff --git a/apps/extension/src/transport/types.ts b/apps/extension/src/transport/types.ts index 4504ff1..7143cb1 100644 --- a/apps/extension/src/transport/types.ts +++ b/apps/extension/src/transport/types.ts @@ -37,10 +37,18 @@ export type RpcErrorReason = | "file_input_not_activated" | "set_file_input_failed" | "download_capture_failed" + | "transfer_outcome_unknown" + | "transfer_timeout" | "cleanup_failed"; +export type TransferEffectState = "none" | "committed" | "unknown"; +export type TransferCleanupState = "complete" | "failed"; + export interface RpcErrorData { reason?: RpcErrorReason; + effect_state?: TransferEffectState; + phase?: string; + cleanup_state?: TransferCleanupState; [key: string]: unknown; } @@ -533,6 +541,7 @@ export interface DownloadParams { tab_id?: number; timeout_ms?: number; browser_relative_dir?: string; + max_byte_size?: number; } export interface DownloadResult { diff --git a/crates/bsk-cli/Cargo.toml b/crates/bsk-cli/Cargo.toml index 1475e4b..e24ef88 100644 --- a/crates/bsk-cli/Cargo.toml +++ b/crates/bsk-cli/Cargo.toml @@ -64,6 +64,7 @@ windows-sys = { version = "0.59", features = [ "Win32_Foundation", "Win32_System_Threading", "Win32_Security", + "Win32_Storage_FileSystem", ] } [dev-dependencies] diff --git a/crates/bsk-cli/skill/SKILL.md b/crates/bsk-cli/skill/SKILL.md index 4233d1f..2cb5d80 100644 --- a/crates/bsk-cli/skill/SKILL.md +++ b/crates/bsk-cli/skill/SKILL.md @@ -215,7 +215,7 @@ Both capture from the moment the tab is attached and read a bounded per-tab buff The agent/harness decides whether a file transfer is appropriate and which local path belongs to the task. Treat upload as disclosure of that file to the current website, and download as accepting website-controlled bytes onto the local filesystem. Use only paths that are necessary for the user's bounded goal. -BrowserSkill enforces the mechanical boundary: files are staged under a session-scoped opaque transfer, only daemon-minted capabilities reach the extension, upload/download still obey Agent Window tab checks, and transfers are chunk/size bounded. Upload captures the file input actually activated by the requested click and assigns only the staged file paths. Download requires an exact-target browser intent before it claims one Chrome download, routes it through a daemon-minted relative directory, and lets the daemon validate and import it. Upload staging remains available for a later form submission and is removed when the session ends. Downloads cannot overwrite an existing destination unless `--overwrite` is explicit. BrowserSkill does not inspect file content or decide whether its meaning is sensitive. +BrowserSkill enforces the mechanical boundary: files are staged under a session-scoped opaque transfer, only daemon-minted capabilities reach the extension, upload/download still obey Agent Window tab checks, and transfers are chunk/size bounded. Upload intercepts the native chooser for one transaction, locates the input activated in the resolved target's document, and assigns only the staged file paths. Download uniquely correlates one exact-target browser intent with one Chrome download in either event order, routes it through a daemon-minted relative directory, and lets the daemon validate and import it. Upload staging remains available for a later form submission and is removed when the session ends. Downloads cannot overwrite an existing destination unless `--overwrite` is explicit. BrowserSkill does not inspect file content or decide whether its meaning is sensitive. Do not use `request-help` merely because a native file chooser or browser download is involved; try these commands first. For transfer failures, use the structured error instead of retrying blindly: @@ -223,6 +223,9 @@ Do not use `request-help` merely because a native file chooser or browser downlo - `reason=file_input_probe_failed` means BrowserSkill could not safely establish the browser-side upload transaction. Do not repeat the same action; use `request-help` when available. - `reason=set_file_input_failed` means BrowserSkill found the activated file input but Chrome rejected the staged path or assignment. Check the extension's file-URL access permission; otherwise use `request-help`. - `reason=download_capture_failed` means BrowserSkill could not attribute exactly one completed download to the requested target. Do not retry blindly or accept an unrelated browser download; use `request-help` when available. +- `effect_state=none` means BrowserSkill confirmed that no file-transfer effect was committed. Follow the accompanying reason; a corrected target or explicit human fallback may be attempted. +- `effect_state=unknown` means the browser may already have attached or created the file. Do not repeat the transfer. Observe the page if that can establish the result; otherwise stop and report the uncertainty. +- `effect_state=committed` means the browser-side effect occurred even if later completion or cleanup failed. Do not repeat it; continue only after verifying the resulting page/download state. If `request-help` returns `outcome="disabled"`, do not retry it. Stop gracefully and report the transfer mechanism that requires human intervention. diff --git a/crates/bsk-cli/src/cli/atomic_output.rs b/crates/bsk-cli/src/cli/atomic_output.rs new file mode 100644 index 0000000..5efacf2 --- /dev/null +++ b/crates/bsk-cli/src/cli/atomic_output.rs @@ -0,0 +1,75 @@ +//! Atomic visibility boundary for files received into a same-directory temp. + +use std::path::Path; + +pub fn commit(temp: &Path, out: &Path, overwrite: bool) -> std::io::Result<()> { + if !overwrite { + std::fs::hard_link(temp, out)?; + std::fs::remove_file(temp)?; + return Ok(()); + } + replace(temp, out) +} + +#[cfg(unix)] +fn replace(temp: &Path, out: &Path) -> std::io::Result<()> { + // POSIX rename replaces an existing non-directory destination atomically. + std::fs::rename(temp, out) +} + +#[cfg(windows)] +fn replace(temp: &Path, out: &Path) -> std::io::Result<()> { + use std::os::windows::ffi::OsStrExt; + use windows_sys::Win32::Storage::FileSystem::{ + MOVEFILE_REPLACE_EXISTING, MOVEFILE_WRITE_THROUGH, MoveFileExW, + }; + + let from: Vec = temp.as_os_str().encode_wide().chain(Some(0)).collect(); + let to: Vec = out.as_os_str().encode_wide().chain(Some(0)).collect(); + // SAFETY: both buffers are NUL-terminated and remain alive for the call. + let ok = unsafe { + MoveFileExW( + from.as_ptr(), + to.as_ptr(), + MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH, + ) + }; + if ok == 0 { + Err(std::io::Error::last_os_error()) + } else { + Ok(()) + } +} + +#[cfg(not(any(unix, windows)))] +fn replace(temp: &Path, out: &Path) -> std::io::Result<()> { + std::fs::rename(temp, out) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn no_replace_never_overwrites_existing_output() { + let dir = tempfile::tempdir().unwrap(); + let temp = dir.path().join("new.part"); + let out = dir.path().join("out.bin"); + std::fs::write(&temp, b"new").unwrap(); + std::fs::write(&out, b"old").unwrap(); + assert!(commit(&temp, &out, false).is_err()); + assert_eq!(std::fs::read(&out).unwrap(), b"old"); + } + + #[test] + fn overwrite_replaces_existing_output_without_predelete() { + let dir = tempfile::tempdir().unwrap(); + let temp = dir.path().join("new.part"); + let out = dir.path().join("out.bin"); + std::fs::write(&temp, b"new").unwrap(); + std::fs::write(&out, b"old").unwrap(); + commit(&temp, &out, true).unwrap(); + assert_eq!(std::fs::read(&out).unwrap(), b"new"); + assert!(!temp.exists()); + } +} diff --git a/crates/bsk-cli/src/cli/download.rs b/crates/bsk-cli/src/cli/download.rs index 043b4d1..5a97b28 100644 --- a/crates/bsk-cli/src/cli/download.rs +++ b/crates/bsk-cli/src/cli/download.rs @@ -15,6 +15,7 @@ use bsk_protocol::tools::{ use clap::Args; use uuid::Uuid; +use crate::cli::atomic_output; use crate::cli::ensure_daemon::ensure_daemon; use crate::cli::error::{CliError, Format}; use crate::cli::interaction::split_target; @@ -56,6 +57,7 @@ pub fn dispatch(args: DownloadArgs, format: Format) -> Result<(), CliError> { tab_id: args.tab_id, timeout_ms: Some(args.timeout), browser_relative_dir: None, + max_byte_size: None, }; let reply: DownloadResult = crate::cli::business_rpc::call( info.sock_path.clone(), @@ -130,22 +132,8 @@ fn write_transfer(sock: &Path, id: &str, out: &Path, overwrite: bool) -> Result< } file.sync_all().map_err(|e| CliError::Local(e.into()))?; drop(file); - if !overwrite { - std::fs::hard_link(&temp, out) - .with_context(|| format!("commit download without overwriting {}", out.display())) - .map_err(CliError::Local)?; - std::fs::remove_file(&temp) - .with_context(|| format!("remove temporary download {}", temp.display())) - .map_err(CliError::Local)?; - return Ok(()); - } - if out.exists() { - std::fs::remove_file(out) - .with_context(|| format!("replace existing output {}", out.display())) - .map_err(CliError::Local)?; - } - std::fs::rename(&temp, out) - .with_context(|| format!("commit download to {}", out.display())) + atomic_output::commit(&temp, out, overwrite) + .with_context(|| format!("atomically commit download to {}", out.display())) .map_err(CliError::Local) })(); if result.is_err() { diff --git a/crates/bsk-cli/src/cli/mod.rs b/crates/bsk-cli/src/cli/mod.rs index 249f556..4776ebe 100644 --- a/crates/bsk-cli/src/cli/mod.rs +++ b/crates/bsk-cli/src/cli/mod.rs @@ -2,6 +2,7 @@ use std::time::Duration; +mod atomic_output; pub mod browser_wait; pub mod browsers; pub mod business_rpc; diff --git a/crates/bsk-cli/src/cli/render_error.rs b/crates/bsk-cli/src/cli/render_error.rs index bb2a0a6..05db2b7 100644 --- a/crates/bsk-cli/src/cli/render_error.rs +++ b/crates/bsk-cli/src/cli/render_error.rs @@ -53,6 +53,8 @@ pub mod reason { pub const FILE_INPUT_NOT_ACTIVATED: &str = "file_input_not_activated"; pub const SET_FILE_INPUT_FAILED: &str = "set_file_input_failed"; pub const DOWNLOAD_CAPTURE_FAILED: &str = "download_capture_failed"; + pub const TRANSFER_OUTCOME_UNKNOWN: &str = "transfer_outcome_unknown"; + pub const TRANSFER_TIMEOUT: &str = "transfer_timeout"; pub const SESSION_BUSY: &str = crate::rpc_reason::SESSION_BUSY; pub const RECORD_START_PAGE_UNREACHABLE: &str = "record_start_page_unreachable"; } @@ -201,6 +203,20 @@ pub fn info_for_error(code: ErrorCode, data: Option<&serde_json::Value>) -> Rend return base; }; match (code, reason) { + (_, reason::TRANSFER_OUTCOME_UNKNOWN) => RenderInfo { + summary: "the file transfer outcome could not be confirmed", + hint: Some( + "do not retry the transfer: the browser may already have applied it; inspect the page or stop safely", + ), + exit_code: base.exit_code, + }, + (ErrorCode::Timeout, reason::TRANSFER_TIMEOUT) => RenderInfo { + summary: "the file transfer timed out after browser dispatch", + hint: Some( + "do not retry when effect_state is unknown or committed; inspect the page before taking another action", + ), + exit_code: base.exit_code, + }, (ErrorCode::PermissionDenied, reason::ELEMENT_NOT_VISIBLE) => RenderInfo { summary: "target element has no visible geometry", hint: Some( @@ -474,6 +490,15 @@ mod tests { let download = serde_json::json!({ "reason": reason::DOWNLOAD_CAPTURE_FAILED }); let info = info_for_error(ErrorCode::CdpFailed, Some(&download)); assert!(info.summary.contains("download could not be attributed")); + + let unknown = serde_json::json!({ "reason": reason::TRANSFER_OUTCOME_UNKNOWN }); + let info = info_for_error(ErrorCode::ProtocolError, Some(&unknown)); + assert!(info.summary.contains("outcome could not be confirmed")); + assert!(info.hint.unwrap().contains("do not retry")); + + let timeout = serde_json::json!({ "reason": reason::TRANSFER_TIMEOUT }); + let info = info_for_error(ErrorCode::Timeout, Some(&timeout)); + assert!(info.summary.contains("timed out after browser dispatch")); } #[test] diff --git a/crates/bsk-cli/src/cli/upload.rs b/crates/bsk-cli/src/cli/upload.rs index 9614e0b..7636453 100644 --- a/crates/bsk-cli/src/cli/upload.rs +++ b/crates/bsk-cli/src/cli/upload.rs @@ -49,38 +49,43 @@ pub fn dispatch(args: UploadArgs, format: Format) -> Result<(), CliError> { let info = ensure_daemon().context("ensure daemon is running")?; let (ref_, selector) = split_target(args.target, args.ref_, args.selector)?; let mut staged = Vec::new(); - let result = (|| { - for path in &args.files { - staged.push(stage_file(&info.sock_path, &args.session, path)?); - } - let params = UploadParams { - session_id: args.session, - ref_, - selector, - tab_id: args.tab_id, - files: staged - .iter() - .map(|(id, name)| UploadFile { - transfer_id: id.clone(), - name: name.clone(), - staged_path: None, - }) - .collect(), - timeout_ms: Some(args.timeout), - }; - crate::cli::business_rpc::call::<_, UploadResult>( - info.sock_path.clone(), - "upload", - Method::ToolUpload, - Some(params), - ipc_timeout(args.timeout), - ) - })(); - if result.is_err() { - for (id, _) in &staged { - let _ = release(&info.sock_path, id); + for path in &args.files { + match stage_file(&info.sock_path, &args.session, path) { + Ok(file) => staged.push(file), + Err(err) => { + for (id, _) in &staged { + let _ = release(&info.sock_path, id); + } + return Err(err); + } } } + let params = UploadParams { + session_id: args.session, + ref_, + selector, + tab_id: args.tab_id, + files: staged + .iter() + .map(|(id, name)| UploadFile { + transfer_id: id.clone(), + name: name.clone(), + staged_path: None, + }) + .collect(), + timeout_ms: Some(args.timeout), + }; + let result = crate::cli::business_rpc::call::<_, UploadResult>( + info.sock_path.clone(), + "upload", + Method::ToolUpload, + Some(params), + ipc_timeout(args.timeout), + ); + // Once tool.upload is dispatched, staging ownership belongs to the + // session. A transport timeout cannot prove that Chrome did not attach + // the file, so releasing here could invalidate a late successful attach. + // Session teardown remains the single cleanup boundary after dispatch. let reply = result?; match format { Format::Json => println!("{}", serde_json::to_string_pretty(&reply).unwrap()), diff --git a/crates/bsk-cli/src/daemon/file_transfer.rs b/crates/bsk-cli/src/daemon/file_transfer.rs index b63ca50..397e30b 100644 --- a/crates/bsk-cli/src/daemon/file_transfer.rs +++ b/crates/bsk-cli/src/daemon/file_transfer.rs @@ -40,6 +40,67 @@ struct Entry { ready: bool, } +/// Browser download path after the daemon has verified that it belongs to +/// the capability minted for this transfer. From this point onward the daemon +/// owns cleanup on every error path; unvalidated arbitrary paths are never +/// removed. +#[derive(Debug)] +struct ValidatedBrowserFile { + path: PathBuf, + parent: PathBuf, + cleanup_armed: bool, +} + +impl ValidatedBrowserFile { + fn validate(reported_path: &Path, transfer_id: &str) -> Result { + if !reported_path.is_absolute() { + return Err(permission( + "download path is outside its browser capability", + )); + } + let expected_parent = Path::new("BrowserSkill").join(transfer_id); + let parent = reported_path + .parent() + .ok_or_else(|| permission("download path has no parent directory"))?; + if !parent.ends_with(&expected_parent) { + return Err(permission( + "download escaped its browser capability directory", + )); + } + for path in [reported_path, parent] { + if fs::symlink_metadata(path) + .map_err(io_error)? + .file_type() + .is_symlink() + { + return Err(permission("download capability path contains a symlink")); + } + } + Ok(Self { + path: reported_path.to_path_buf(), + parent: parent.to_path_buf(), + cleanup_armed: true, + }) + } + + fn remove_source(mut self) -> Result<(), RpcError> { + remove_if_present(&self.path).map_err(io_error)?; + remove_dir_if_present(&self.parent).map_err(io_error)?; + self.cleanup_armed = false; + Ok(()) + } +} + +impl Drop for ValidatedBrowserFile { + fn drop(&mut self) { + if !self.cleanup_armed { + return; + } + let _ = remove_if_present(&self.path); + let _ = remove_dir_if_present(&self.parent); + } +} + #[derive(Debug)] pub struct DownloadStaging { pub transfer_id: String, @@ -259,30 +320,11 @@ impl TransferRegistry { let entry = entries .get_mut(id) .ok_or_else(|| not_found("download transfer not found"))?; - if entry.direction != Direction::Download || !reported_path.is_absolute() { - return Err(permission( - "download path is outside its browser capability", - )); + if entry.direction != Direction::Download { + return Err(permission("transfer is not a download capability")); } - let expected_parent = Path::new("BrowserSkill").join(id); - let reported_parent = reported_path - .parent() - .ok_or_else(|| permission("download path has no parent directory"))?; - if !reported_parent.ends_with(&expected_parent) { - return Err(permission( - "download escaped its browser capability directory", - )); - } - for path in [reported_path, reported_parent] { - if fs::symlink_metadata(path) - .map_err(io_error)? - .file_type() - .is_symlink() - { - return Err(permission("download capability path contains a symlink")); - } - } - let canonical = reported_path.canonicalize().map_err(io_error)?; + let browser_file = ValidatedBrowserFile::validate(reported_path, id)?; + let canonical = browser_file.path.canonicalize().map_err(io_error)?; let meta = fs::metadata(&canonical).map_err(io_error)?; if !meta.is_file() || meta.len() > MAX_TRANSFER_BYTES { return Err(invalid( @@ -310,8 +352,7 @@ impl TransferRegistry { entry.written = copied; entry.expected_size = Some(copied); entry.ready = true; - let _ = fs::remove_file(&canonical); - let _ = fs::remove_dir(reported_parent); + browser_file.remove_source()?; Ok(copied) } @@ -391,6 +432,22 @@ fn set_private_file(file: &File) -> std::io::Result<()> { Ok(()) } +fn remove_if_present(path: &Path) -> std::io::Result<()> { + match fs::remove_file(path) { + Ok(()) => Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(err) => Err(err), + } +} + +fn remove_dir_if_present(path: &Path) -> std::io::Result<()> { + match fs::remove_dir(path) { + Ok(()) => Ok(()), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(err) => Err(err), + } +} + fn safe_upload_basename(name: &str) -> Result<&str, RpcError> { if name.is_empty() || name.contains(['/', '\\', '\0']) { return Err(invalid("upload name must be a safe basename")); @@ -570,6 +627,7 @@ mod tests { .import_download(&staging.transfer_id, &outside) .is_err() ); + assert_eq!(fs::read(&outside).unwrap(), b"secret"); let browser_dir = temp .path() @@ -601,6 +659,31 @@ mod tests { ); } + #[test] + fn validated_oversized_browser_download_is_removed_on_rejection() { + let (temp, registry) = registry(); + let staging = registry.begin_download("s1").unwrap(); + let browser_dir = temp + .path() + .join("Downloads") + .join("BrowserSkill") + .join(&staging.transfer_id); + fs::create_dir_all(&browser_dir).unwrap(); + let inside = browser_dir.join("oversized.bin"); + File::create(&inside) + .unwrap() + .set_len(MAX_TRANSFER_BYTES + 1) + .unwrap(); + + assert!( + registry + .import_download(&staging.transfer_id, &inside) + .is_err() + ); + assert!(!inside.exists()); + assert!(!browser_dir.exists()); + } + #[test] fn releasing_a_session_removes_all_staging() { let (_temp, registry) = registry(); diff --git a/crates/bsk-cli/src/daemon/ipc.rs b/crates/bsk-cli/src/daemon/ipc.rs index 9a6d0bd..186cade 100644 --- a/crates/bsk-cli/src/daemon/ipc.rs +++ b/crates/bsk-cli/src/daemon/ipc.rs @@ -382,6 +382,7 @@ async fn handle_tool_dispatch( Err(err) => return ResponseBody::Err(err), }; download.browser_relative_dir = Some(staging.browser_relative_dir); + download.max_byte_size = Some(super::file_transfer::MAX_TRANSFER_BYTES); download_transfer_id = Some(staging.transfer_id); params = serde_json::to_value(download).unwrap_or(Value::Null); } @@ -702,7 +703,7 @@ fn tool_dispatch_timeout(params: &Value) -> Result { fn tool_dispatch_transport_timeout(method: &Method, params: &Value) -> Result { tool_dispatch_timeout(params).map(|timeout| { - if method == &Method::ToolUpload { + if matches!(method, Method::ToolUpload | Method::ToolDownload) { timeout.saturating_add(EXTENSION_RESPONSE_GRACE) } else { timeout diff --git a/crates/bsk-cli/src/daemon/queue.rs b/crates/bsk-cli/src/daemon/queue.rs index 05f09e8..6b7366a 100644 --- a/crates/bsk-cli/src/daemon/queue.rs +++ b/crates/bsk-cli/src/daemon/queue.rs @@ -438,6 +438,7 @@ async fn dispatch_with_sender( lifecycle_cancel: Option, cancel_cleanup_timeout: Duration, ) -> Result { + let effect_aware_transfer = is_effect_aware_transfer(&method); let (respond_tx, respond_rx) = oneshot::channel(); let job = ToolJob { method, @@ -474,15 +475,21 @@ async fn dispatch_with_sender( mpsc::error::TrySendError::Closed(_) => DispatchError::QueueClosed, }); } - let mut response_timeout = timeout.saturating_add(Duration::from_secs(1)); - if lifecycle_cancellable { - // A lifecycle cancel can arrive at the original RPC deadline and - // intentionally keeps the worker alive while extension cleanup - // settles. Do not let this outer waiter abandon that reconciliation - // early, which could reopen a daemon session whose window is gone. - response_timeout = response_timeout.saturating_add(cancel_cleanup_timeout); - } - let waited = tokio::time::timeout(response_timeout, respond_rx).await; + // Lifecycle teardown and effect-aware transfers both keep the worker busy + // for bounded compensation after their original deadline. Keep the outer + // waiter alive for the same grace period so it cannot abandon reconciliation. + let response_grace = if lifecycle_cancellable || effect_aware_transfer { + cancel_cleanup_timeout + } else { + Duration::ZERO + }; + let waited = tokio::time::timeout( + timeout + .saturating_add(response_grace) + .saturating_add(Duration::from_secs(1)), + respond_rx, + ) + .await; match waited { Ok(Ok(Ok(v))) => Ok(v), Ok(Ok(Err(rpc))) => Err(DispatchError::Rpc(rpc)), @@ -636,44 +643,85 @@ async fn forward_one( .lifecycle_cancel .as_ref() .map(|_| &forward_lifecycle_cancel as &(dyn Fn() + Sync)); + let send_deadline_cancel = || { + client + .sink + .send(Frame::Request(RequestFrame { + id: format!("deadline-cancel-{rpc_id}"), + method: Method::Cancel, + params: Some(serde_json::json!({ "rpc_id": rpc_id })), + })) + .is_ok() + }; + let deadline_cancel: Option<&(dyn Fn() -> bool + Sync)> = + is_effect_aware_transfer(&job.method).then_some(&send_deadline_cancel); let waited = await_with_optional_cancel( job.timeout, job.cancel_cleanup_timeout, waiter, cancel_token.as_ref(), on_abort, + deadline_cancel, ) .await; let response = match waited { WaitOutcome::Response(resp) => resp, WaitOutcome::CancelledAfterResponse(resp) => { if job.method == Method::ToolSessionStop - && let ResponseBody::Ok(value) = resp.body + && let ResponseBody::Ok(value) = &resp.body { // Teardown crossed its final irreversible boundary before // the cancel landed. Commit the real extension result so the // daemon does not retain a session whose window is gone. - return Ok(value); + return Ok(value.clone()); } - // Cancellation wins the external verdict, but only after the - // extension's original RPC has settled. Preserve any non-cancel - // error so compensation failures remain explicit instead of - // being hidden behind a generic cancelled result. - if let ResponseBody::Err(err) = resp.body - && !matches!(err.code, ErrorCode::Cancelled | ErrorCode::UserAborted) - { - return Err(err); + // File transfer commits are irreversible. A late cancel cannot + // overwrite a confirmed success, and an unknown transfer effect + // must remain explicit so callers do not retry or release upload + // staging as though nothing happened. + if is_effect_aware_transfer(&job.method) { + match &resp.body { + ResponseBody::Ok(_) => resp, + ResponseBody::Err(err) + if transfer_effect(err) == Some("unknown") + || transfer_effect(err) == Some("committed") => + { + return Err(err.clone()); + } + ResponseBody::Err(err) + if !matches!(err.code, ErrorCode::Cancelled | ErrorCode::UserAborted) => + { + return Err(err.clone()); + } + ResponseBody::Err(_) => { + return Err(cancelled_error( + job.inflight.as_deref(), + "tool dispatch cancelled after extension cleanup", + )); + } + } + } else { + // For ordinary tools cancellation keeps the existing verdict, + // while non-cancel errors still expose compensation failures. + if let ResponseBody::Err(err) = resp.body + && !matches!(err.code, ErrorCode::Cancelled | ErrorCode::UserAborted) + { + return Err(err); + } + return Err(cancelled_error( + job.inflight.as_deref(), + "tool dispatch cancelled after extension cleanup", + )); } - return Err(cancelled_error( - job.inflight.as_deref(), - "tool dispatch cancelled after extension cleanup", - )); } WaitOutcome::TimedOutAfterResponse(resp) => match resp.body { - ResponseBody::Ok(value) if job.method == Method::ToolSessionStop => { + ResponseBody::Ok(value) + if job.method == Method::ToolSessionStop + || is_effect_aware_transfer(&job.method) => + { // The close crossed its irreversible boundary during the - // timeout cleanup grace. Reconcile daemon state instead of - // reopening a session whose window is already gone. + // timeout cleanup grace, or the transfer committed before its + // deadline cancel settled. Preserve the irreversible result. return Ok(value); } ResponseBody::Err(err) @@ -681,6 +729,9 @@ async fn forward_one( { return Err(err); } + ResponseBody::Err(err) if is_effect_aware_transfer(&job.method) => { + return Err(timed_out_transfer_error(&err)); + } _ => { return Err(RpcError { code: ErrorCode::Timeout, @@ -691,6 +742,14 @@ async fn forward_one( }, WaitOutcome::CleanupTimeout => { client.pending.lock().unwrap().cancel(&rpc_id); + if is_effect_aware_transfer(&job.method) { + return Err(unknown_transfer_error( + ErrorCode::Timeout, + "cancelled file transfer did not confirm its outcome before cleanup timed out", + "cleanup", + true, + )); + } return Err(RpcError { code: ErrorCode::Timeout, message: format!( @@ -700,8 +759,25 @@ async fn forward_one( data: Some(serde_json::json!({ "reason": "cancel_cleanup_timeout" })), }); } + WaitOutcome::TimeoutCleanupFailed => { + client.pending.lock().unwrap().cancel(&rpc_id); + return Err(unknown_transfer_error( + ErrorCode::Timeout, + "file transfer timed out and cleanup could not be confirmed", + "cleanup", + true, + )); + } WaitOutcome::WaiterClosed => { client.pending.lock().unwrap().cancel(&rpc_id); + if is_effect_aware_transfer(&job.method) { + return Err(unknown_transfer_error( + ErrorCode::ProtocolError, + "file transfer transport closed after dispatch; outcome is unknown", + "transport", + false, + )); + } return Err(RpcError { code: ErrorCode::ProtocolError, message: "transport closed mid-call".into(), @@ -710,6 +786,14 @@ async fn forward_one( } WaitOutcome::Timeout => { client.pending.lock().unwrap().cancel(&rpc_id); + if is_effect_aware_transfer(&job.method) { + return Err(unknown_transfer_error( + ErrorCode::Timeout, + "file transfer timed out after dispatch; outcome is unknown", + "transport", + false, + )); + } return Err(RpcError { code: ErrorCode::Timeout, message: format!("tool RPC timed out after {:?}", job.timeout), @@ -723,12 +807,65 @@ async fn forward_one( } } +fn is_effect_aware_transfer(method: &Method) -> bool { + matches!(method, Method::ToolUpload | Method::ToolDownload) +} + +fn transfer_effect(err: &RpcError) -> Option<&str> { + err.data.as_ref()?.get("effect_state")?.as_str() +} + +fn unknown_transfer_error( + code: ErrorCode, + message: impl Into, + phase: &str, + cleanup_failed: bool, +) -> RpcError { + let mut data = serde_json::json!({ + "reason": "transfer_outcome_unknown", + "effect_state": "unknown", + "phase": phase, + }); + if cleanup_failed { + data["cleanup_state"] = serde_json::json!("failed"); + } + RpcError { + code, + message: message.into(), + data: Some(data), + } +} + +fn timed_out_transfer_error(err: &RpcError) -> RpcError { + let data = err.data.as_ref(); + RpcError { + code: ErrorCode::Timeout, + message: "file transfer timed out after dispatch".into(), + data: Some(serde_json::json!({ + "reason": "transfer_timeout", + "effect_state": data + .and_then(|value| value.get("effect_state")) + .and_then(Value::as_str) + .unwrap_or("unknown"), + "phase": data + .and_then(|value| value.get("phase")) + .and_then(Value::as_str) + .unwrap_or("cleanup"), + "cleanup_state": data + .and_then(|value| value.get("cleanup_state")) + .and_then(Value::as_str) + .unwrap_or("complete"), + })), + } +} + #[derive(Debug)] enum WaitOutcome { Response(bsk_protocol::ResponseFrame), CancelledAfterResponse(bsk_protocol::ResponseFrame), TimedOutAfterResponse(bsk_protocol::ResponseFrame), CleanupTimeout, + TimeoutCleanupFailed, WaiterClosed, Timeout, } @@ -739,6 +876,7 @@ async fn await_with_optional_cancel( mut waiter: oneshot::Receiver, cancel: Option<&super::abort::AbortToken>, on_abort: Option<&(dyn Fn() + Sync)>, + on_deadline: Option<&(dyn Fn() -> bool + Sync)>, ) -> WaitOutcome { match cancel { Some(token) => { @@ -764,6 +902,16 @@ async fn await_with_optional_cancel( Err(_) => WaitOutcome::WaiterClosed, }, _ = &mut deadline => { + if let Some(send_cancel) = on_deadline { + return if send_cancel() { + match tokio::time::timeout(cleanup_timeout, &mut waiter).await { + Ok(Ok(resp)) => WaitOutcome::TimedOutAfterResponse(resp), + Ok(Err(_)) | Err(_) => WaitOutcome::TimeoutCleanupFailed, + } + } else { + WaitOutcome::TimeoutCleanupFailed + }; + } let Some(on_abort) = on_abort else { return WaitOutcome::Timeout; }; @@ -779,7 +927,16 @@ async fn await_with_optional_cancel( None => match tokio::time::timeout(timeout, &mut waiter).await { Ok(Ok(resp)) => WaitOutcome::Response(resp), Ok(Err(_)) => WaitOutcome::WaiterClosed, - Err(_) => WaitOutcome::Timeout, + Err(_) => match on_deadline { + Some(send_cancel) if send_cancel() => { + match tokio::time::timeout(cleanup_timeout, &mut waiter).await { + Ok(Ok(resp)) => WaitOutcome::TimedOutAfterResponse(resp), + Ok(Err(_)) | Err(_) => WaitOutcome::TimeoutCleanupFailed, + } + } + Some(_) => WaitOutcome::TimeoutCleanupFailed, + None => WaitOutcome::Timeout, + }, }, } } @@ -864,6 +1021,7 @@ mod await_with_optional_cancel_tests { rx, Some(&token), None, + None, ) .await; assert!( @@ -887,6 +1045,7 @@ mod await_with_optional_cancel_tests { rx, Some(&token), None, + None, ) .await; match outcome { @@ -907,6 +1066,7 @@ mod await_with_optional_cancel_tests { rx, None, None, + None, ) .await; assert!(matches!(outcome, WaitOutcome::Response(_))); @@ -924,6 +1084,7 @@ mod await_with_optional_cancel_tests { rx, Some(&token), None, + None, ) .await }); @@ -948,6 +1109,7 @@ mod await_with_optional_cancel_tests { rx, Some(&token), None, + None, ) .await; assert!(matches!(outcome, WaitOutcome::CleanupTimeout)); @@ -972,12 +1134,49 @@ mod await_with_optional_cancel_tests { rx, Some(&token), Some(&forward_abort), + None, ) .await; assert!(abort_forwarded.load(Ordering::SeqCst)); assert!(matches!(outcome, WaitOutcome::TimedOutAfterResponse(_))); } + + #[tokio::test] + async fn transfer_deadline_sends_cancel_and_waits_for_cleanup_response() { + let (tx, rx) = oneshot::channel(); + tokio::spawn(async move { + tokio::time::sleep(Duration::from_millis(5)).await; + tx.send(dummy_response()).unwrap(); + }); + let send_cancel = || true; + let outcome = await_with_optional_cancel( + Duration::from_millis(1), + Duration::from_secs(1), + rx, + None, + None, + Some(&send_cancel), + ) + .await; + assert!(matches!(outcome, WaitOutcome::TimedOutAfterResponse(_))); + } + + #[tokio::test] + async fn transfer_deadline_is_unknown_when_cancel_cannot_be_sent() { + let (_tx, rx) = oneshot::channel(); + let send_cancel = || false; + let outcome = await_with_optional_cancel( + Duration::from_millis(1), + Duration::from_secs(1), + rx, + None, + None, + Some(&send_cancel), + ) + .await; + assert!(matches!(outcome, WaitOutcome::TimeoutCleanupFailed)); + } } #[cfg(test)] diff --git a/crates/bsk-protocol/schema/tool_download_params.json b/crates/bsk-protocol/schema/tool_download_params.json index b6487cc..3bfbba7 100644 --- a/crates/bsk-protocol/schema/tool_download_params.json +++ b/crates/bsk-protocol/schema/tool_download_params.json @@ -13,6 +13,15 @@ "null" ] }, + "max_byte_size": { + "description": "Daemon-injected authoritative transfer size limit. The extension uses it only for early cancellation; daemon import remains the final check.", + "type": [ + "integer", + "null" + ], + "format": "uint64", + "minimum": 0.0 + }, "ref": { "type": [ "string", diff --git a/crates/bsk-protocol/src/tools/file_transfer.rs b/crates/bsk-protocol/src/tools/file_transfer.rs index f2517ce..9677785 100644 --- a/crates/bsk-protocol/src/tools/file_transfer.rs +++ b/crates/bsk-protocol/src/tools/file_transfer.rs @@ -63,6 +63,10 @@ pub struct DownloadParams { /// Daemon-injected relative directory beneath Chrome's Downloads root. #[serde(default, skip_serializing_if = "Option::is_none")] pub browser_relative_dir: Option, + /// Daemon-injected authoritative transfer size limit. The extension uses + /// it only for early cancellation; daemon import remains the final check. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub max_byte_size: Option, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] @@ -163,8 +167,10 @@ mod tests { tab_id: None, timeout_ms: None, browser_relative_dir: None, + max_byte_size: None, }) .unwrap(); assert!(value.get("browser_relative_dir").is_none()); + assert!(value.get("max_byte_size").is_none()); } } diff --git a/docs/architecture.md b/docs/architecture.md index d86748d..cfaf9e5 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -146,10 +146,11 @@ flowchart LR ### File-transfer boundary - The invoking agent/harness decides whether a transfer is authorized and supplies the task-local source or destination path. -- The CLI is the only component that reads an upload source or writes the final download destination. The extension never receives either agent-facing path. -- The daemon issues opaque, session-scoped transfer IDs and stages bounded chunks in a private runtime directory. For upload it injects only its private staged paths. For download it mints a relative Chrome directory capability, validates the reported completed path against that capability, rejects symlinks and oversized/non-regular files, then imports the bytes into private staging. -- The extension only performs the browser-side mechanism, scoped to the requested session and exact Agent Window target. Upload installs a transaction-scoped DOM guard that records the one `` actually activated by the requested click while cancelling its native default action, then uses `DOM.setFileInputFiles`. File System Access picker entry points are intercepted only for that transaction and return a structured fallback. Download requires an exact-target `Page.downloadWillBegin` intent before a matching `chrome.downloads` item is routed into the daemon-minted relative directory. The extension does not classify content or make policy decisions. -- Download staging is released after the CLI commits the file. Upload staging remains until session teardown because the page may read an attached file only on a later form submission. All staging is released on session stop/browser disconnect and on daemon startup after a crash. Existing download destinations are not overwritten unless the CLI caller explicitly opts in. +- The CLI is the only component that reads an upload source or writes the final download destination. Before browser dispatch it owns rollback of partially staged uploads; after dispatch, ownership moves to the session because a transport timeout cannot prove that Chrome did not attach the file. Download output becomes visible through one atomic commit, and replacement is opt-in without a pre-delete window. The extension never receives either agent-facing path. +- The daemon is the authority for storage capabilities and limits. It issues opaque session-scoped transfer IDs, stages bounded chunks in a private runtime directory, and injects only private staged upload paths. For download it mints one relative Chrome directory capability. Only after validating the reported path, file type, symlink boundary, and authoritative byte limit does it take ownership of browser-file cleanup and import the bytes. +- The extension owns only the browser transaction. Both tools resolve one `ResolvedActionTarget` and use that same target for protocol setup and the triggering click. Upload arms Chrome's chooser interception before clicking, then accepts either an exact `Page.fileChooserOpened` input node or an independent probe anchored in the trigger node's own document; one verified input is committed with `DOM.setFileInputFiles`. A non-input picker is rejected immediately as unsupported. Download correlates exact-target CDP intent and `chrome.downloads` filename candidates in either arrival order, claims only one unique match, and never cancels an unclaimed candidate. +- Browser-side operations report `effect_state` (`none`, `committed`, or `unknown`), `phase`, and `cleanup_state`. Confirmed success wins over a late cancel; an unknown effect is preserved across timeout or transport loss and must not be retried blindly. A transfer deadline sends cancellation to the extension and keeps the session queue occupied for bounded compensation rather than abandoning an in-flight browser effect. +- Download staging is released after CLI commit. Upload staging remains until session teardown because the page may read an attached file only on a later form submission. Remaining staging is released on session stop/browser disconnect and on daemon startup after a crash. BrowserSkill does not inspect content or decide whether a transfer is appropriate. ## Repository layout diff --git a/skill/SKILL.md b/skill/SKILL.md index 4233d1f..2cb5d80 100644 --- a/skill/SKILL.md +++ b/skill/SKILL.md @@ -215,7 +215,7 @@ Both capture from the moment the tab is attached and read a bounded per-tab buff The agent/harness decides whether a file transfer is appropriate and which local path belongs to the task. Treat upload as disclosure of that file to the current website, and download as accepting website-controlled bytes onto the local filesystem. Use only paths that are necessary for the user's bounded goal. -BrowserSkill enforces the mechanical boundary: files are staged under a session-scoped opaque transfer, only daemon-minted capabilities reach the extension, upload/download still obey Agent Window tab checks, and transfers are chunk/size bounded. Upload captures the file input actually activated by the requested click and assigns only the staged file paths. Download requires an exact-target browser intent before it claims one Chrome download, routes it through a daemon-minted relative directory, and lets the daemon validate and import it. Upload staging remains available for a later form submission and is removed when the session ends. Downloads cannot overwrite an existing destination unless `--overwrite` is explicit. BrowserSkill does not inspect file content or decide whether its meaning is sensitive. +BrowserSkill enforces the mechanical boundary: files are staged under a session-scoped opaque transfer, only daemon-minted capabilities reach the extension, upload/download still obey Agent Window tab checks, and transfers are chunk/size bounded. Upload intercepts the native chooser for one transaction, locates the input activated in the resolved target's document, and assigns only the staged file paths. Download uniquely correlates one exact-target browser intent with one Chrome download in either event order, routes it through a daemon-minted relative directory, and lets the daemon validate and import it. Upload staging remains available for a later form submission and is removed when the session ends. Downloads cannot overwrite an existing destination unless `--overwrite` is explicit. BrowserSkill does not inspect file content or decide whether its meaning is sensitive. Do not use `request-help` merely because a native file chooser or browser download is involved; try these commands first. For transfer failures, use the structured error instead of retrying blindly: @@ -223,6 +223,9 @@ Do not use `request-help` merely because a native file chooser or browser downlo - `reason=file_input_probe_failed` means BrowserSkill could not safely establish the browser-side upload transaction. Do not repeat the same action; use `request-help` when available. - `reason=set_file_input_failed` means BrowserSkill found the activated file input but Chrome rejected the staged path or assignment. Check the extension's file-URL access permission; otherwise use `request-help`. - `reason=download_capture_failed` means BrowserSkill could not attribute exactly one completed download to the requested target. Do not retry blindly or accept an unrelated browser download; use `request-help` when available. +- `effect_state=none` means BrowserSkill confirmed that no file-transfer effect was committed. Follow the accompanying reason; a corrected target or explicit human fallback may be attempted. +- `effect_state=unknown` means the browser may already have attached or created the file. Do not repeat the transfer. Observe the page if that can establish the result; otherwise stop and report the uncertainty. +- `effect_state=committed` means the browser-side effect occurred even if later completion or cleanup failed. Do not repeat it; continue only after verifying the resulting page/download state. If `request-help` returns `outcome="disabled"`, do not retry it. Stop gracefully and report the transfer mechanism that requires human intervention.