diff --git a/apps/extension/src/tools/__tests__/file-transfer.test.ts b/apps/extension/src/tools/__tests__/file-transfer.test.ts index 0cd7948b..306b670d 100644 --- a/apps/extension/src/tools/__tests__/file-transfer.test.ts +++ b/apps/extension/src/tools/__tests__/file-transfer.test.ts @@ -41,6 +41,7 @@ function fakeEvent unknown>() { function popupDownloadFakes() { const onCreated = fakeEvent<(item: chrome.downloads.DownloadItem) => void>(); + const onChanged = fakeEvent<(delta: chrome.downloads.DownloadDelta) => void>(); const onDeterminingFilename = fakeEvent< ( @@ -53,7 +54,7 @@ function popupDownloadFakes() { const completed = new Map(); const downloads: DownloadsApi = { onCreated, - onChanged: fakeEvent<(delta: chrome.downloads.DownloadDelta) => void>(), + onChanged, onDeterminingFilename, search: vi.fn(async ({ id }: chrome.downloads.DownloadQuery) => { const item = id === undefined ? undefined : completed.get(id); @@ -80,16 +81,31 @@ function popupDownloadFakes() { new Promise((resolve) => { onDeterminingFilename.emit(item, resolve); }), - finish: (item: chrome.downloads.DownloadItem) => { + finish: ( + item: chrome.downloads.DownloadItem, + filename = `/profile/Downloads/BrowserSkill/tr_${item.id}/${item.filename}`, + ) => { const done = { ...item, - filename: `/profile/Downloads/${item.filename}`, + filename, state: "complete", fileSize: 4, } as chrome.downloads.DownloadItem; completed.set(item.id, done); onCreated.emit(done); }, + finishChanged: (item: chrome.downloads.DownloadItem, filename: string) => { + completed.set(item.id, { + ...item, + filename, + state: "complete", + fileSize: 4, + } as chrome.downloads.DownloadItem); + onChanged.emit({ + id: item.id, + state: { current: "complete" }, + } as chrome.downloads.DownloadDelta); + }, popup: (sourceTabId: number, url: string) => onCreatedNavigationTarget.emit({ sourceTabId, @@ -669,7 +685,7 @@ describe("file transfer tools", () => { } as chrome.downloads.DownloadItem; const completed = { ...initial, - filename: "/profile/Downloads/BrowserSkill/tr_1/result.zip", + filename: "C:\\Users\\tester\\Downloads\\BrowserSkill\\tr_1\\result.zip", state: "complete", fileSize: 12, } as chrome.downloads.DownloadItem; @@ -833,6 +849,7 @@ describe("file transfer tools", () => { } as chrome.downloads.DownloadItem; const complete = { ...initial, + filename: "/profile/Downloads/BrowserSkill/tr_21/candidate-first.bin", state: "complete", fileSize: 4, } as chrome.downloads.DownloadItem; @@ -1142,10 +1159,42 @@ describe("file transfer tools", () => { expect(result).toMatchObject({ tab_id: 4, suggested_filename: "report-50.csv", - browser_path: "/profile/Downloads/report-50.csv", + browser_path: "/profile/Downloads/BrowserSkill/tr_50/report-50.csv", }); }); + it.each([ + "C:\\Users\\tester\\Downloads\\report-60.csv", + "C:\\Users\\tester\\Documents\\report-60.csv", + ])("rejects a download outside its transfer directory without deleting %s", async (filename) => { + const fakes = popupDownloadFakes(); + const download = fakes.item(60, "https://example.test/export?id=60"); + + const result = await captureBrowserDownload({ + cdp: silentCdp(), + target: { tabId: 4 }, + downloads: fakes.downloads, + navigationTargets: fakes.navigationTargets, + browserRelativeDir: "BrowserSkill/tr_60", + timeoutMs: 1_000, + trigger: async (markDispatched) => { + markDispatched(); + fakes.popup(4, download.url); + await fakes.offer(download); + fakes.finishChanged(download, filename); + return { tab_id: 4, x: 10, y: 10 }; + }, + }); + + expect(result).toMatchObject({ + code: "cdp_failed", + data: { reason: "download_path_mismatch", effect_state: "committed" }, + }); + expect(fakes.downloads.removeFile).not.toHaveBeenCalled(); + expect(fakes.downloads.cancel).not.toHaveBeenCalled(); + expect(result).not.toMatchObject({ data: { cleanup_state: "failed" } }); + }); + it.each([ "navigation-first", "candidate-first", diff --git a/apps/extension/src/tools/download-capture.ts b/apps/extension/src/tools/download-capture.ts index 519f306e..b25734de 100644 --- a/apps/extension/src/tools/download-capture.ts +++ b/apps/extension/src/tools/download-capture.ts @@ -102,6 +102,12 @@ function safeBasename(filename: string): string { return basename && basename !== "." && basename !== ".." ? basename : "download"; } +function isInBrowserRelativeDir(filename: string, browserRelativeDir: string): boolean { + const parent = filename.split(/[\\/]/).slice(0, -1); + const expected = browserRelativeDir.split("/"); + return parent.slice(-expected.length).join("/") === browserRelativeDir; +} + function sameTarget(source: { tabId?: number; sessionId?: string }, target: CdpTarget): boolean { return source.tabId === target.tabId && source.sessionId === target.sessionId; } @@ -171,6 +177,7 @@ export async function captureBrowserDownload( let capturedId: number | undefined; let settled = false; let succeeded = false; + let preserveOutsideDownload = false; let failureResult: RpcError | undefined; let uniquenessTimer: ReturnType | undefined; let operationTimer: ReturnType | undefined; @@ -398,6 +405,18 @@ export async function captureBrowserDownload( } click = triggered; const item = await completion; + // Another extension can override our suggestion, so validate the completed path. + if (!isInBrowserRelativeDir(item.filename, options.browserRelativeDir)) { + // The final location may be user-selected, so leave the file in place. + preserveOutsideDownload = true; + failureResult = transferError( + "cdp_failed", + "download_path_mismatch", + "the browser did not use the requested download directory", + { effectState: "committed", phase: "download" }, + ); + return failureResult; + } succeeded = true; return { click, item }; } catch (err) { @@ -432,7 +451,7 @@ export async function captureBrowserDownload( const cleanupDeadline = Date.now() + CLEANUP_TIMEOUT_MS; const cleanups: Promise[] = []; if (options.cleanupTrigger) cleanups.push(options.cleanupTrigger(cleanupDeadline)); - if (!succeeded && capturedId !== undefined) { + if (!succeeded && !preserveOutsideDownload && capturedId !== undefined) { cleanups.push(cleanupClaimedDownload(options.downloads, capturedId)); } try { diff --git a/apps/extension/src/transport/types.ts b/apps/extension/src/transport/types.ts index 37c01491..cacb4040 100644 --- a/apps/extension/src/transport/types.ts +++ b/apps/extension/src/transport/types.ts @@ -74,6 +74,7 @@ export type RpcErrorReason = | "file_drop_target_unavailable" | "file_drop_failed" | "download_capture_failed" + | "download_path_mismatch" | "transfer_outcome_unknown" | "transfer_timeout" | "cleanup_failed"; diff --git a/crates/bsk-cli/skill/references/files.md b/crates/bsk-cli/skill/references/files.md index 58843e99..28ca76b7 100644 --- a/crates/bsk-cli/skill/references/files.md +++ b/crates/bsk-cli/skill/references/files.md @@ -17,5 +17,7 @@ Use agent-local paths, not browser-internal staging paths. A successful drop proves dispatch, not site acceptance; observe the attachment. - Download refuses overwrite by default; add `--overwrite` only when replacement is intended. Consult each command's help for other flags. +- For download directory conflicts with other extensions, see + [human steps and recovery](help-and-recovery.md). Remote upload/download are unsupported. diff --git a/crates/bsk-cli/skill/references/help-and-recovery.md b/crates/bsk-cli/skill/references/help-and-recovery.md index ae041cb7..bdf5f152 100644 --- a/crates/bsk-cli/skill/references/help-and-recovery.md +++ b/crates/bsk-cli/skill/references/help-and-recovery.md @@ -34,6 +34,12 @@ substring is enough. | Timeout or unknown effect | Inspect current state before retrying; the action may already have happened. | | `fill_value_mismatch` | Read the field: formatting may still satisfy the request. Correct only a remaining difference; no blind refill or immediate handoff. | | Unsupported operation | Use available capabilities; suggest updating only if the missing feature is needed. | +| `download_path_mismatch` | The browser completed the download outside BrowserSkill's transfer directory, so `bsk download` did not produce `--out`. The browser file is left in place; find it in the Downloads list and decide whether to use or remove it before retrying. A download manager such as Chrono may have overridden the filename suggestion; when one is present, disable or pause its renaming or download takeover. The browser may also have rejected the suggested filename or used a different save location; check both before retrying. Retry only if `--out` is still needed. | + +If both extensions suggest a name, updating or reinstalling BrowserSkill after +the other extension can make BrowserSkill's folder suggestion win. This changes +which extension wins the conflict; a later update to the other extension may +reverse it. Navigation alone (including deprecated help outcome `navigated`) is not completion. For other errors, follow the returned hint and inspect the current state. diff --git a/crates/bsk-cli/src/cli/render_error.rs b/crates/bsk-cli/src/cli/render_error.rs index 1063ff58..a5c9a242 100644 --- a/crates/bsk-cli/src/cli/render_error.rs +++ b/crates/bsk-cli/src/cli/render_error.rs @@ -71,6 +71,7 @@ pub mod reason { pub const FILE_DROP_TARGET_UNAVAILABLE: &str = "file_drop_target_unavailable"; pub const FILE_DROP_FAILED: &str = "file_drop_failed"; pub const DOWNLOAD_CAPTURE_FAILED: &str = "download_capture_failed"; + pub const DOWNLOAD_PATH_MISMATCH: &str = "download_path_mismatch"; 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; @@ -549,6 +550,13 @@ pub fn info_for_error(code: ErrorCode, data: Option<&serde_json::Value>) -> Rend ), exit_code: base.exit_code, }, + (ErrorCode::CdpFailed, reason::DOWNLOAD_PATH_MISMATCH) => RenderInfo { + summary: "the browser did not use the requested download directory", + hint: Some( + "find the saved file in the browser's Downloads list; check for a competing rename, a browser-rejected filename, or a different save location before retrying", + ), + exit_code: base.exit_code, + }, _ => base, } } @@ -817,6 +825,15 @@ mod tests { let info = info_for_error(ErrorCode::CdpFailed, Some(&download)); assert!(info.summary.contains("download could not be attributed")); + let path_mismatch = serde_json::json!({ + "reason": reason::DOWNLOAD_PATH_MISMATCH, + "effect_state": "committed", + "phase": "download" + }); + let info = info_for_error(ErrorCode::CdpFailed, Some(&path_mismatch)); + assert!(info.summary.contains("requested download directory")); + assert!(info.hint.unwrap().contains("browser-rejected filename")); + 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"));