diff --git a/apps/extension/src/tools/__tests__/file-transfer.test.ts b/apps/extension/src/tools/__tests__/file-transfer.test.ts index 16e1eef..182744b 100644 --- a/apps/extension/src/tools/__tests__/file-transfer.test.ts +++ b/apps/extension/src/tools/__tests__/file-transfer.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi } from "vitest"; import { SessionManager } from "@/session-manager/manager"; import { type DownloadsApi, handleDownload } from "../download"; +import { uploadThroughFileChooser } from "../file-chooser"; import type { CdpRunner } from "../shared"; import { handleUpload } from "../upload"; @@ -37,7 +38,7 @@ function fakeEvent void>() { } describe("file transfer tools", () => { - it("arms chooser interception before clicking and injects only staged paths", async () => { + it("enables chooser observation before clicking and injects only staged paths", async () => { const manager = sessions(); const ctx = await manager.start("s1"); ctx.refStore.set("e3", 123, { tabId: 4 }); @@ -82,13 +83,135 @@ describe("file transfer tools", () => { expect(result).toMatchObject({ tab_id: 4, file_names: ["one.png", "two.png"] }); expect(calls[0]).toMatchObject({ - method: "Page.setInterceptFileChooserDialog", - params: { enabled: true }, + method: "Page.enable", + params: { enableFileChooserOpenedEvent: true }, }); expect(calls).toContainEqual({ method: "DOM.setFileInputFiles", params: { files: ["/private/stage/one", "/private/stage/two"], backendNodeId: 456 }, }); + expect(calls.at(-1)).toEqual({ + method: "Page.enable", + params: { enableFileChooserOpenedEvent: false }, + }); + }); + + it("reports File System Access pickers as an explicit manual fallback", async () => { + let eventHandler: Parameters>[0] | undefined; + const cdp: CdpRunner = { + onEvent: (handler) => { + eventHandler = handler; + return { dispose: vi.fn() }; + }, + send: vi.fn(async () => ({})) as unknown as CdpRunner["send"], + }; + + const result = await uploadThroughFileChooser({ + cdp, + target: { tabId: 4 }, + files: ["/private/stage/one"], + timeoutMs: 100, + trigger: async () => { + eventHandler?.({ tabId: 4 }, "Page.fileChooserOpened", { mode: "selectSingle" }); + return { tab_id: 4, x: 10, y: 10 }; + }, + }); + + expect(result).toEqual({ + code: "unsupported", + message: "this file chooser is not backed by an input element", + data: { + reason: "unsupported_file_chooser", + chooser_kind: "non_input", + manual_fallback_available: true, + }, + }); + }); + + it("bounds a stuck chooser-control command and resets uncertain CDP state", async () => { + const detach = vi.fn(async () => {}); + const cdp: CdpRunner = { + onEvent: () => ({ dispose: vi.fn() }), + send: vi.fn(() => new Promise(() => {})) as unknown as CdpRunner["send"], + detach, + }; + + const result = await uploadThroughFileChooser({ + cdp, + target: { tabId: 4 }, + files: ["/private/stage/one"], + timeoutMs: 5, + trigger: vi.fn(), + }); + + expect(result).toMatchObject({ + code: "timeout", + data: { reason: "file_chooser_control_failed", phase: "enable_events" }, + }); + expect(detach).toHaveBeenCalledWith(4); + }); + + it("distinguishes a completed click that did not open a chooser", async () => { + const send = vi.fn(async () => ({})) as unknown as CdpRunner["send"]; + const cdp: CdpRunner = { + onEvent: () => ({ dispose: vi.fn() }), + send, + }; + + const result = await uploadThroughFileChooser({ + cdp, + target: { tabId: 4 }, + files: ["/private/stage/one"], + timeoutMs: 5, + trigger: async () => ({ tab_id: 4, x: 10, y: 10 }), + }); + + expect(result).toMatchObject({ + code: "timeout", + data: { reason: "file_chooser_not_opened", phase: "await_event" }, + }); + expect(send).toHaveBeenLastCalledWith(4, "Page.enable", { + enableFileChooserOpenedEvent: false, + }); + }); + + it("accepts chooser events only from the exact CDP target", async () => { + let eventHandler: Parameters>[0] | undefined; + const setFiles = vi.fn(); + const cdp: CdpRunner = { + onEvent: (handler) => { + eventHandler = handler; + return { dispose: vi.fn() }; + }, + send: vi.fn(async () => ({})) as unknown as CdpRunner["send"], + sendToTarget: vi.fn(async (_target, method, params) => { + if (method === "DOM.setFileInputFiles") setFiles(params); + return {}; + }) as unknown as NonNullable, + }; + + const result = await uploadThroughFileChooser({ + cdp, + target: { tabId: 4, sessionId: "child-a" }, + files: ["/private/stage/one"], + timeoutMs: 100, + trigger: async () => { + eventHandler?.({ tabId: 4, sessionId: "child-b" }, "Page.fileChooserOpened", { + backendNodeId: 111, + }); + eventHandler?.({ tabId: 4, sessionId: "child-a" }, "Page.fileChooserOpened", { + backendNodeId: 222, + }); + return { tab_id: 4, x: 10, y: 10 }; + }, + }); + + expect(result).toMatchObject({ click: { tab_id: 4 } }); + expect(setFiles).toHaveBeenCalledOnce(); + expect(setFiles).toHaveBeenCalledWith({ + files: ["/private/stage/one"], + backendNodeId: 222, + }); }); it("captures the one download created under daemon staging", async () => { diff --git a/apps/extension/src/tools/file-chooser.ts b/apps/extension/src/tools/file-chooser.ts new file mode 100644 index 0000000..ab72241 --- /dev/null +++ b/apps/extension/src/tools/file-chooser.ts @@ -0,0 +1,265 @@ +// One file-chooser transaction for an upload. Chrome's extension debugger +// bridge may leave a command pending indefinitely, so every state-changing +// command is bounded and an uncertain target is detached before returning. + +import type { CdpTarget } from "@/browser-driver/frame-graph"; +import type { ClickResult, RpcError } from "@/transport/types"; +import { type CdpRunner, isRpcError, sendToCdpTarget } from "./shared"; + +const CLEANUP_TIMEOUT_MS = 1_000; + +type ChooserPhase = "enable_events" | "trigger" | "await_event" | "set_files"; + +interface FileChooserOpened { + source: { tabId?: number; sessionId?: string }; + backendNodeId?: number; + mode?: string; +} + +export interface FileChooserUploadOptions { + cdp: CdpRunner; + target: CdpTarget; + files: string[]; + timeoutMs: number; + signal?: AbortSignal; + trigger(timeoutMs: number): Promise; +} + +export interface FileChooserUploadResult { + click: ClickResult; + mode?: string; +} + +class BoundedWaitError extends Error { + constructor( + readonly kind: "timeout" | "aborted", + message: string, + ) { + super(message); + } +} + +function remainingMs(deadline: number): number { + return Math.max(0, deadline - Date.now()); +} + +async function waitBounded( + promise: Promise, + deadline: number, + signal: AbortSignal | undefined, + timeoutMessage: string, +): Promise { + const remaining = remainingMs(deadline); + if (signal?.aborted) throw new BoundedWaitError("aborted", "file chooser operation aborted"); + if (remaining === 0) throw new BoundedWaitError("timeout", timeoutMessage); + + let timer: ReturnType | undefined; + let onAbort: (() => void) | undefined; + const boundary = new Promise((_resolve, reject) => { + timer = setTimeout(() => reject(new BoundedWaitError("timeout", timeoutMessage)), remaining); + if (signal) { + onAbort = () => reject(new BoundedWaitError("aborted", "file chooser operation aborted")); + signal.addEventListener("abort", onAbort, { once: true }); + } + }); + try { + return await Promise.race([promise, boundary]); + } finally { + if (timer) clearTimeout(timer); + if (signal && onAbort) signal.removeEventListener("abort", onAbort); + } +} + +function chooserError( + code: RpcError["code"], + message: string, + reason: "file_chooser_control_failed" | "file_chooser_not_opened", + phase: ChooserPhase, +): RpcError { + return { code, message, data: { reason, phase } }; +} + +function sameTarget(source: FileChooserOpened["source"], target: CdpTarget): boolean { + return source.tabId === target.tabId && source.sessionId === target.sessionId; +} + +async function resetTarget(cdp: CdpRunner, tabId: number): Promise { + if (!cdp.detach) return; + try { + await waitBounded( + cdp.detach(tabId), + Date.now() + CLEANUP_TIMEOUT_MS, + undefined, + "CDP detach timed out", + ); + } catch { + // The target may already have detached or closed. + } +} + +export async function uploadThroughFileChooser( + options: FileChooserUploadOptions, +): Promise { + const deadline = Date.now() + options.timeoutMs; + let resolveOpened!: (value: FileChooserOpened) => void; + const opened = new Promise((resolve) => { + resolveOpened = resolve; + }); + const subscription = options.cdp.onEvent?.((source, method, raw) => { + if (method !== "Page.fileChooserOpened" || !sameTarget(source, options.target)) return; + const event = raw as { backendNodeId?: unknown; mode?: unknown }; + resolveOpened({ + source, + ...(typeof event.backendNodeId === "number" ? { backendNodeId: event.backendNodeId } : {}), + ...(typeof event.mode === "string" ? { mode: event.mode } : {}), + }); + }); + if (!subscription) { + return { code: "unsupported", message: "CDP event subscription unavailable" }; + } + + let eventsEnabled = false; + let targetStateUnknown = false; + let outcome: FileChooserUploadResult | RpcError; + let aborted: BoundedWaitError | undefined; + try { + try { + await waitBounded( + sendToCdpTarget(options.cdp, options.target, "Page.enable", { + enableFileChooserOpenedEvent: true, + }), + deadline, + options.signal, + "enabling file chooser events timed out", + ); + eventsEnabled = true; + } catch (err) { + if (err instanceof BoundedWaitError) { + targetStateUnknown = true; + if (err.kind === "aborted") throw err; + outcome = chooserError( + "timeout", + err.message, + "file_chooser_control_failed", + "enable_events", + ); + } else { + outcome = chooserError( + "cdp_failed", + err instanceof Error ? err.message : String(err), + "file_chooser_control_failed", + "enable_events", + ); + } + return outcome; + } + + let click: ClickResult | RpcError; + try { + click = await waitBounded( + options.trigger(remainingMs(deadline)), + deadline, + options.signal, + "upload trigger timed out", + ); + } catch (err) { + if (err instanceof BoundedWaitError) { + targetStateUnknown = true; + if (err.kind === "aborted") throw err; + return chooserError("timeout", err.message, "file_chooser_control_failed", "trigger"); + } + return chooserError( + "cdp_failed", + err instanceof Error ? err.message : String(err), + "file_chooser_control_failed", + "trigger", + ); + } + if (isRpcError(click)) return click; + + let chooser: FileChooserOpened; + try { + chooser = await waitBounded( + opened, + deadline, + options.signal, + "upload trigger did not open a file chooser", + ); + } catch (err) { + if (err instanceof BoundedWaitError && err.kind === "aborted") throw err; + return chooserError( + "timeout", + err instanceof Error ? err.message : String(err), + "file_chooser_not_opened", + "await_event", + ); + } + + if (chooser.backendNodeId === undefined) { + return { + code: "unsupported", + message: "this file chooser is not backed by an input element", + data: { + reason: "unsupported_file_chooser", + chooser_kind: "non_input", + manual_fallback_available: true, + }, + }; + } + if (chooser.mode === "selectSingle" && options.files.length !== 1) { + return { code: "invalid_params", message: "file chooser accepts exactly one file" }; + } + + try { + await waitBounded( + sendToCdpTarget(options.cdp, options.target, "DOM.setFileInputFiles", { + files: options.files, + backendNodeId: chooser.backendNodeId, + }), + deadline, + options.signal, + "setting file input files timed out", + ); + } catch (err) { + if (err instanceof BoundedWaitError) { + targetStateUnknown = true; + if (err.kind === "aborted") throw err; + return chooserError("timeout", err.message, "file_chooser_control_failed", "set_files"); + } + return chooserError( + "cdp_failed", + err instanceof Error ? err.message : String(err), + "file_chooser_control_failed", + "set_files", + ); + } + outcome = { click, ...(chooser.mode ? { mode: chooser.mode } : {}) }; + } catch (err) { + if (err instanceof BoundedWaitError && err.kind === "aborted") aborted = err; + else throw err; + outcome = { code: "cancelled", message: "file chooser operation aborted" }; + } finally { + subscription.dispose(); + if (targetStateUnknown) { + await resetTarget(options.cdp, options.target.tabId); + } else if (eventsEnabled) { + try { + await waitBounded( + sendToCdpTarget(options.cdp, options.target, "Page.enable", { + enableFileChooserOpenedEvent: false, + }), + Date.now() + CLEANUP_TIMEOUT_MS, + undefined, + "restoring file chooser events timed out", + ); + } catch { + await resetTarget(options.cdp, options.target.tabId); + } + } + } + if (aborted) { + const error = new DOMException(aborted.message, "AbortError"); + throw error; + } + return outcome; +} diff --git a/apps/extension/src/tools/shared.ts b/apps/extension/src/tools/shared.ts index 3b2cb79..9483abf 100644 --- a/apps/extension/src/tools/shared.ts +++ b/apps/extension/src/tools/shared.ts @@ -53,6 +53,7 @@ export type { DialogCursor }; export interface CdpRunner { send(tabId: number, method: string, params?: object): Promise; sendToTarget?(target: CdpTarget, method: string, params?: object): Promise; + detach?(tabId: number): Promise; getFrameGraph?(tabId: number): Promise; ensureAttachedToUrl?(tabId: number, expectedUrl: string | undefined): Promise; trackSessionTab?(sessionId: string, tabId: number): void; diff --git a/apps/extension/src/tools/upload.ts b/apps/extension/src/tools/upload.ts index afee918..e131392 100644 --- a/apps/extension/src/tools/upload.ts +++ b/apps/extension/src/tools/upload.ts @@ -1,10 +1,10 @@ -// Atomic file-chooser upload: arm interception before clicking, then attach -// daemon-staged files to the chooser node. No local path is accepted from an -// agent-facing call; staged_path is injected by the daemon. +// Upload orchestration: validate the session-scoped request, resolve its click +// target, then delegate the browser protocol transaction to file-chooser.ts. import type { CdpTarget } from "@/browser-driver/frame-graph"; import type { SessionManager } from "@/session-manager/manager"; import type { ClickParams, RpcError, UploadParams, UploadResult } from "@/transport/types"; +import { uploadThroughFileChooser } from "./file-chooser"; import { handleClick, type InteractionDeps } from "./interaction"; import { type CdpRunner, @@ -12,7 +12,6 @@ import { isRpcError, lookupSession, resolveTargetTab, - sendToCdpTarget, } from "./shared"; import { resolveSnapshotRef } from "./snapshot-ref"; @@ -60,89 +59,28 @@ export async function handleUpload( const cdpTarget = chooserTarget(params, target.tabId, manager); if (isRpcError(cdpTarget)) return cdpTarget; const timeoutMs = params.timeout_ms ?? DEFAULT_TIMEOUT_MS; - let timer: ReturnType | undefined; - let subscription: { dispose(): void } | undefined; - let resolveChooser!: (value: { - source: { tabId?: number; sessionId?: string }; - backendNodeId: number; - mode?: string; - }) => void; - let rejectChooser!: (error: Error) => void; - const chooser = new Promise<{ - source: { tabId?: number; sessionId?: string }; - backendNodeId: number; - mode?: string; - }>((resolve, reject) => { - resolveChooser = resolve; - rejectChooser = reject; - }); - subscription = deps.cdp.onEvent((source, method, raw) => { - if (method !== "Page.fileChooserOpened" || source.tabId !== target.tabId) return; - if (cdpTarget.sessionId && source.sessionId !== cdpTarget.sessionId) return; - const event = raw as { backendNodeId?: unknown; mode?: unknown }; - if (typeof event.backendNodeId !== "number") { - rejectChooser(new Error("file chooser did not expose an input backendNodeId")); - return; - } - resolveChooser({ - source, - backendNodeId: event.backendNodeId, - ...(typeof event.mode === "string" ? { mode: event.mode } : {}), - }); - }); - timer = setTimeout( - () => rejectChooser(new Error("file chooser did not open before timeout")), + const transaction = await uploadThroughFileChooser({ + cdp: deps.cdp, + target: cdpTarget, + files: params.files.map((file) => file.staged_path as string), timeoutMs, - ); - const onAbort = () => rejectChooser(new DOMException("aborted", "AbortError")); - deps.signal?.addEventListener("abort", onAbort, { once: true }); - try { - await sendToCdpTarget(deps.cdp, cdpTarget, "Page.setInterceptFileChooserDialog", { - enabled: true, - }); - const clickParams: ClickParams = { - session_id: params.session_id, - ref: params.ref, - selector: params.selector, - tab_id: params.tab_id, - timeout_ms: params.timeout_ms, - }; - const clicked = await handleClick(manager, clickParams, deps); - if (isRpcError(clicked)) { - void chooser.catch(() => undefined); - return clicked; - } - const opened = await chooser; - if (opened.mode === "selectSingle" && params.files.length !== 1) { - return { code: "invalid_params", message: "file chooser accepts exactly one file" }; - } - const sourceTarget: CdpTarget = { - tabId: target.tabId, - ...(opened.source.sessionId ? { sessionId: opened.source.sessionId } : {}), - }; - await sendToCdpTarget(deps.cdp, sourceTarget, "DOM.setFileInputFiles", { - files: params.files.map((file) => file.staged_path as string), - backendNodeId: opened.backendNodeId, - }); - return { - tab_id: target.tabId, - used_ref: clicked.used_ref, - used_selector: clicked.used_selector, - file_names: params.files.map((file) => file.name), - }; - } catch (err) { - if (err instanceof DOMException && err.name === "AbortError") throw err; - return { code: "cdp_failed", message: err instanceof Error ? err.message : String(err) }; - } finally { - if (timer) clearTimeout(timer); - subscription?.dispose(); - deps.signal?.removeEventListener("abort", onAbort); - try { - await sendToCdpTarget(deps.cdp, cdpTarget, "Page.setInterceptFileChooserDialog", { - enabled: false, - }); - } catch { - // Best-effort compensation; the target may have navigated/closed. - } - } + 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); + }, + }); + if (isRpcError(transaction)) return transaction; + return { + tab_id: target.tabId, + used_ref: transaction.click.used_ref, + used_selector: transaction.click.used_selector, + file_names: params.files.map((file) => file.name), + }; } diff --git a/apps/extension/src/transport/types.ts b/apps/extension/src/transport/types.ts index 3590257..c161169 100644 --- a/apps/extension/src/transport/types.ts +++ b/apps/extension/src/transport/types.ts @@ -33,6 +33,9 @@ export type RpcErrorReason = | "restricted_tab_url" | "borrow_conflict" | "screenshot_capture_failed" + | "file_chooser_control_failed" + | "file_chooser_not_opened" + | "unsupported_file_chooser" | "cleanup_failed"; export interface RpcErrorData { diff --git a/crates/bsk-cli/skill/SKILL.md b/crates/bsk-cli/skill/SKILL.md index fb6e338..0e5aee1 100644 --- a/crates/bsk-cli/skill/SKILL.md +++ b/crates/bsk-cli/skill/SKILL.md @@ -217,7 +217,13 @@ The agent/harness decides whether a file transfer is appropriate and which local BrowserSkill enforces the mechanical boundary: files are staged under a session-scoped opaque transfer, only daemon-minted paths reach the extension, upload/download still obey Agent Window tab checks, and transfers are chunk/size bounded. Download staging is removed after the CLI copies 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; use these commands. Ask for help only when the intended upload file is unavailable/unreadable to the agent, the page requires genuinely human-only input, or the browser reports the transfer mechanism as unsupported. +Do not use `request-help` merely because a native file chooser or browser download is involved; try these commands first. For upload failures, use the structured error instead of retrying blindly: + +- `reason=unsupported_file_chooser` means the page opened a non-input picker such as `window.showOpenFilePicker()`. Do not retry `upload`; if human help is available, call `request-help` and tell the user the exact original file path to choose. The staged daemon path is an internal implementation detail and must not be shown to the user. +- `reason=file_chooser_control_failed` means BrowserSkill could not establish or complete the browser-side transaction. Do not retry the same action blindly; use `request-help` when available. +- `reason=file_chooser_not_opened` means the click completed without opening a chooser. Observe once to verify that the target is the actual upload action (for example, after expanding a menu), then retry with the corrected target or ask for help. + +If `request-help` returns `outcome="disabled"`, do not retry it. Stop gracefully and report that the workflow requires a non-input file picker or unavailable local file. ### Scripting & timing diff --git a/crates/bsk-cli/src/cli/render_error.rs b/crates/bsk-cli/src/cli/render_error.rs index db52f47..2be4682 100644 --- a/crates/bsk-cli/src/cli/render_error.rs +++ b/crates/bsk-cli/src/cli/render_error.rs @@ -49,6 +49,9 @@ pub mod reason { pub const TAB_NOT_ACTIVE: &str = "tab_not_active"; pub const BORROW_CONFLICT: &str = "borrow_conflict"; pub const SCREENSHOT_CAPTURE_FAILED: &str = "screenshot_capture_failed"; + pub const FILE_CHOOSER_CONTROL_FAILED: &str = "file_chooser_control_failed"; + pub const FILE_CHOOSER_NOT_OPENED: &str = "file_chooser_not_opened"; + pub const UNSUPPORTED_FILE_CHOOSER: &str = "unsupported_file_chooser"; pub const SESSION_BUSY: &str = crate::rpc_reason::SESSION_BUSY; pub const RECORD_START_PAGE_UNREACHABLE: &str = "record_start_page_unreachable"; } @@ -280,6 +283,29 @@ pub fn info_for_error(code: ErrorCode, data: Option<&serde_json::Value>) -> Rend ), exit_code: base.exit_code, }, + (ErrorCode::Unsupported, reason::UNSUPPORTED_FILE_CHOOSER) => RenderInfo { + summary: "the page opened a non-input file picker", + hint: Some( + "do not retry upload; use `bsk request-help` and tell the user the exact original local path to select, or stop if human help is disabled", + ), + exit_code: base.exit_code, + }, + (ErrorCode::Timeout | ErrorCode::CdpFailed, reason::FILE_CHOOSER_CONTROL_FAILED) => { + RenderInfo { + summary: "the browser file-chooser transaction could not complete", + hint: Some( + "do not retry the same upload blindly; use `bsk request-help` when human help is available", + ), + exit_code: base.exit_code, + } + } + (ErrorCode::Timeout, reason::FILE_CHOOSER_NOT_OPENED) => RenderInfo { + summary: "the upload action did not open a file chooser", + hint: Some( + "observe once after expanding any upload menu, then retry with the actual upload action or use `bsk request-help`", + ), + exit_code: base.exit_code, + }, _ => base, } } @@ -419,6 +445,27 @@ mod tests { assert_eq!(info.exit_code, 3); } + #[test] + fn file_chooser_reasons_render_actionable_fallbacks() { + let unsupported = serde_json::json!({ "reason": reason::UNSUPPORTED_FILE_CHOOSER }); + let info = info_for_error(ErrorCode::Unsupported, Some(&unsupported)); + assert_eq!(info.summary, "the page opened a non-input file picker"); + assert!(info.hint.unwrap().contains("exact original local path")); + + let control = serde_json::json!({ "reason": reason::FILE_CHOOSER_CONTROL_FAILED }); + let info = info_for_error(ErrorCode::Timeout, Some(&control)); + assert!(info.summary.contains("transaction could not complete")); + assert!(info.hint.unwrap().contains("do not retry")); + + let not_opened = serde_json::json!({ "reason": reason::FILE_CHOOSER_NOT_OPENED }); + let info = info_for_error(ErrorCode::Timeout, Some(¬_opened)); + assert_eq!( + info.summary, + "the upload action did not open a file chooser" + ); + assert!(info.hint.unwrap().contains("expanding any upload menu")); + } + #[test] fn ref_not_found_overrides_not_found_copy() { let data = serde_json::json!({ "reason": reason::REF_NOT_FOUND }); diff --git a/crates/bsk-cli/src/daemon/ipc.rs b/crates/bsk-cli/src/daemon/ipc.rs index 38ee666..64a03d3 100644 --- a/crates/bsk-cli/src/daemon/ipc.rs +++ b/crates/bsk-cli/src/daemon/ipc.rs @@ -62,6 +62,10 @@ pub type RpcHandler = Arc< >; const DEFAULT_RPC_TIMEOUT: Duration = Duration::from_secs(15); +// The upload transaction owns its operation deadline and may need a bounded +// cleanup before it can return a useful structured error. Keep only that +// transport alive slightly longer so it does not replace the result. +const EXTENSION_RESPONSE_GRACE: Duration = Duration::from_secs(2); /// Upper bound on `wait_for_browser_ms` accepted over IPC. const MAX_BROWSER_WAIT: Duration = Duration::from_secs(60); // `session.stop` fast-fails while another tool is active; this budget @@ -332,7 +336,7 @@ async fn handle_tool_dispatch( data: None, }); } - let timeout = match tool_dispatch_timeout(¶ms) { + let timeout = match tool_dispatch_transport_timeout(&method, ¶ms) { Ok(timeout) => timeout, Err(err) => return ResponseBody::Err(err), }; @@ -696,6 +700,16 @@ fn tool_dispatch_timeout(params: &Value) -> Result { Ok(Duration::from_millis(u64::from(ms))) } +fn tool_dispatch_transport_timeout(method: &Method, params: &Value) -> Result { + tool_dispatch_timeout(params).map(|timeout| { + if method == &Method::ToolUpload { + timeout.saturating_add(EXTENSION_RESPONSE_GRACE) + } else { + timeout + } + }) +} + // Local CLI-facing shapes. Intentionally distinct from // `bsk_protocol::tools::SessionStart*` (which describes the WS-facing // `tool.session_*` round-trip with the extension) so the two sides can @@ -1494,6 +1508,30 @@ mod tests { assert_eq!(got, std::time::Duration::from_millis(300_000)); } + #[test] + fn extension_transport_outlives_the_operation_deadline() { + let params = serde_json::json!({ + "session_id": "abcd", + "timeout_ms": 60_000, + }); + let dispatch_timeout = + tool_dispatch_transport_timeout(&Method::ToolUpload, ¶ms).unwrap(); + + assert_eq!(dispatch_timeout, Duration::from_secs(62)); + } + + #[test] + fn extension_response_grace_does_not_change_other_tools() { + let params = serde_json::json!({ + "session_id": "abcd", + "timeout_ms": 60_000, + }); + let dispatch_timeout = + tool_dispatch_transport_timeout(&Method::ToolClick, ¶ms).unwrap(); + + assert_eq!(dispatch_timeout, Duration::from_secs(60)); + } + #[test] fn tool_dispatch_timeout_rejects_zero_timeout_ms() { let params = serde_json::json!({ diff --git a/docs/architecture.md b/docs/architecture.md index 23db98e..2560016 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -148,7 +148,7 @@ flowchart LR - 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. It injects only its own staging paths into extension RPCs and validates that a completed download remains inside its minted directory. -- The extension only performs the browser-side mechanism: intercept one chooser or capture one download, scoped to the requested session and Agent Window tab. It does not classify content or make policy decisions. +- The extension only performs the browser-side mechanism: handle one file-chooser transaction or capture one download, scoped to the requested session and exact Agent Window target. Automatic upload is limited to choosers backed by ``; non-input pickers such as `window.showOpenFilePicker()` return a structured unsupported result for the agent to handle. 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. ## Repository layout diff --git a/skill/SKILL.md b/skill/SKILL.md index fb6e338..0e5aee1 100644 --- a/skill/SKILL.md +++ b/skill/SKILL.md @@ -217,7 +217,13 @@ The agent/harness decides whether a file transfer is appropriate and which local BrowserSkill enforces the mechanical boundary: files are staged under a session-scoped opaque transfer, only daemon-minted paths reach the extension, upload/download still obey Agent Window tab checks, and transfers are chunk/size bounded. Download staging is removed after the CLI copies 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; use these commands. Ask for help only when the intended upload file is unavailable/unreadable to the agent, the page requires genuinely human-only input, or the browser reports the transfer mechanism as unsupported. +Do not use `request-help` merely because a native file chooser or browser download is involved; try these commands first. For upload failures, use the structured error instead of retrying blindly: + +- `reason=unsupported_file_chooser` means the page opened a non-input picker such as `window.showOpenFilePicker()`. Do not retry `upload`; if human help is available, call `request-help` and tell the user the exact original file path to choose. The staged daemon path is an internal implementation detail and must not be shown to the user. +- `reason=file_chooser_control_failed` means BrowserSkill could not establish or complete the browser-side transaction. Do not retry the same action blindly; use `request-help` when available. +- `reason=file_chooser_not_opened` means the click completed without opening a chooser. Observe once to verify that the target is the actual upload action (for example, after expanding a menu), then retry with the corrected target or ask for help. + +If `request-help` returns `outcome="disabled"`, do not retry it. Stop gracefully and report that the workflow requires a non-input file picker or unavailable local file. ### Scripting & timing