diff --git a/CHANGELOG.md b/CHANGELOG.md index 73ee0ce0..43a97812 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,13 @@ Starting from 0.2.0, CLI / Extension / DSH Plugin share the same version number. ### Fixed +- Extension: `download` no longer silently reports a trace-folder path when a + coexisting extension won the `chrome.downloads.onDeterminingFilename` race. + Chromium lets only one extension name a download and never tells the losers, so + the per-call `BrowserSkill//` grouping was dropped while the result still + implied the folder layout. The result now carries a `note` when the completed file + is outside the trace folder, so the caller trusts the real path + ([#361](https://github.com/Tencent/BrowserSkill/issues/361)). - Extension: input to a background Agent Window tab no longer keeps failing with `input_not_ready` after Chrome drops the session's focus override without a detach ([#355](https://github.com/Tencent/BrowserSkill/issues/355)). The session's diff --git a/apps/extension/src/tools/__tests__/file-transfer.test.ts b/apps/extension/src/tools/__tests__/file-transfer.test.ts index 0cd7948b..3672ec95 100644 --- a/apps/extension/src/tools/__tests__/file-transfer.test.ts +++ b/apps/extension/src/tools/__tests__/file-transfer.test.ts @@ -80,10 +80,16 @@ function popupDownloadFakes() { new Promise((resolve) => { onDeterminingFilename.emit(item, resolve); }), - finish: (item: chrome.downloads.DownloadItem) => { + /** + * Completes the download, storing it at `finalPath` (defaults to flat in + * the download dir). Tests that care about the trace folder pass the stored + * path explicitly: honouring it, or losing it to a coexisting extension that + * won the `onDeterminingFilename` race. + */ + finish: (item: chrome.downloads.DownloadItem, finalPath?: string) => { const done = { ...item, - filename: `/profile/Downloads/${item.filename}`, + filename: finalPath ?? `/profile/Downloads/${item.filename}`, state: "complete", fileSize: 4, } as chrome.downloads.DownloadItem; @@ -1293,4 +1299,66 @@ describe("file transfer tools", () => { data: { effect_state: "unknown", phase: "trigger" }, }); }); + + it.each([ + ["Chrome honours the trace folder", "/profile/Downloads/BrowserSkill/tr_60/report-60.csv"], + // The uniquify suffix lands on the basename, so it must not mask an intact folder. + [ + "Chrome uniquifies the repeated name", + "/profile/Downloads/BrowserSkill/tr_60/report-60 (1).csv", + ], + ])("reports no dropped suggestion when %s", async (_label, finalPath) => { + const fakes = popupDownloadFakes(); + const download = fakes.item(60, "https://example.test/export?id=60"); + let suggested: Promise | undefined; + + 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); + suggested = fakes.offer(download); + fakes.finish(download, finalPath); + return { tab_id: 4, x: 10, y: 10 }; + }, + }); + + await expect(suggested).resolves.toEqual({ + filename: "BrowserSkill/tr_60/report-60.csv", + conflictAction: "overwrite", + }); + expect(result).toMatchObject({ item: { filename: finalPath } }); + expect(result).not.toHaveProperty("suggestionDropped"); + }); + + it("flags a dropped trace folder when another extension wins the name", async () => { + const fakes = popupDownloadFakes(); + const download = fakes.item(61, "https://example.test/export?id=61"); + + const result = await captureBrowserDownload({ + cdp: silentCdp(), + target: { tabId: 4 }, + downloads: fakes.downloads, + navigationTargets: fakes.navigationTargets, + browserRelativeDir: "BrowserSkill/tr_61", + timeoutMs: 1_000, + trigger: async (markDispatched) => { + markDispatched(); + fakes.popup(4, download.url); + fakes.offer(download); + fakes.finish(download, "/profile/Downloads/report-61.csv"); + return { tab_id: 4, x: 10, y: 10 }; + }, + }); + + expect(result).toMatchObject({ + item: { filename: "/profile/Downloads/report-61.csv" }, + suggestionDropped: true, + }); + }); }); diff --git a/apps/extension/src/tools/download-capture.ts b/apps/extension/src/tools/download-capture.ts index 519f306e..b61ee998 100644 --- a/apps/extension/src/tools/download-capture.ts +++ b/apps/extension/src/tools/download-capture.ts @@ -81,6 +81,12 @@ export interface DownloadCaptureOptions { export interface DownloadCaptureResult { click: ClickResult; item: chrome.downloads.DownloadItem; + /** + * Set when the completed path is not under `browserRelativeDir`, so another + * extension won the `chrome.downloads.onDeterminingFilename` race and the + * trace-scoped folder grouping was dropped. The download itself succeeded. + */ + suggestionDropped?: boolean; } interface DownloadIntent { @@ -102,6 +108,27 @@ function safeBasename(filename: string): string { return basename && basename !== "." && basename !== ".." ? basename : "download"; } +/** Path segments of a Chrome download path, ignoring separator flavour. */ +function pathSegments(path: string): string[] { + return path.split(/[\\/]+/).filter((segment) => segment.length > 0); +} + +/** + * Chromium lets only one extension answer `onDeterminingFilename`, and never + * tells the losers they lost. Chrome may also uniquify a repeated name with a + * `" (1)"` suffix, so compare the directory portion only: if the completed file + * is not under the trace-scoped `browserRelativeDir`, the suggestion was + * overridden elsewhere even though the download itself succeeded. + */ +function suggestionWasDropped(filename: string, browserRelativeDir: string): boolean { + const wanted = pathSegments(browserRelativeDir); + if (wanted.length === 0) return false; + const actual = pathSegments(filename).slice(0, -1); + return !wanted.every( + (segment, index) => actual[index + actual.length - wanted.length] === segment, + ); +} + function sameTarget(source: { tabId?: number; sessionId?: string }, target: CdpTarget): boolean { return source.tabId === target.tabId && source.sessionId === target.sessionId; } @@ -399,7 +426,8 @@ export async function captureBrowserDownload( click = triggered; const item = await completion; succeeded = true; - return { click, item }; + const suggestionDropped = suggestionWasDropped(item.filename, options.browserRelativeDir); + return { click, item, ...(suggestionDropped ? { suggestionDropped } : {}) }; } catch (err) { const effect: TransferEffectState = capturedId !== undefined diff --git a/apps/extension/src/tools/download.ts b/apps/extension/src/tools/download.ts index ae9d83b9..5b09747b 100644 --- a/apps/extension/src/tools/download.ts +++ b/apps/extension/src/tools/download.ts @@ -16,6 +16,15 @@ import { enforceAgentWindow, isRpcError, lookupSession, resolveTargetTab } from let downloadActive = false; +/** + * Result note explaining that the completed file landed outside the trace-scoped + * folder. Chromium lets only one extension answer + * `chrome.downloads.onDeterminingFilename` and never tells the losers they lost, + * so a coexisting download-manager extension silently overrides the suggestion. + */ +export const DOWNLOAD_SUGGESTION_DROPPED_NOTE = + "the completed download is not under the trace folder: another extension took over chrome.downloads.onDeterminingFilename, so the per-call folder grouping was dropped (the file itself downloaded fine) — disable that extension's download renaming to restore it"; + export type { DownloadsApi, NavigationTargetsApi } from "./download-capture"; export interface DownloadDeps extends InteractionDeps { @@ -71,6 +80,7 @@ export async function handleDownload( mime: item.mime || undefined, danger: item.danger, browser_path: item.filename, + ...(capture.suggestionDropped ? { note: DOWNLOAD_SUGGESTION_DROPPED_NOTE } : {}), }; } finally { downloadActive = false; diff --git a/apps/extension/src/transport/types.ts b/apps/extension/src/transport/types.ts index 37c01491..1827d8a9 100644 --- a/apps/extension/src/transport/types.ts +++ b/apps/extension/src/transport/types.ts @@ -723,6 +723,13 @@ export interface DownloadResult { danger?: string; browser_path?: string; transfer_id?: string; + /** + * Set when the completed file is not under the trace-scoped + * `browserRelativeDir`, meaning another extension won the + * `chrome.downloads.onDeterminingFilename` race. The download succeeded; only + * the per-call folder grouping was lost. + */ + note?: string; } // -------------------------------------------------------------------------- diff --git a/crates/bsk-cli/skill/references/help-and-recovery.md b/crates/bsk-cli/skill/references/help-and-recovery.md index ae041cb7..c287b8e5 100644 --- a/crates/bsk-cli/skill/references/help-and-recovery.md +++ b/crates/bsk-cli/skill/references/help-and-recovery.md @@ -33,6 +33,7 @@ substring is enough. | Unknown tab/session | List current tabs/sessions; never guess IDs or use another task's session. | | 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. | +| Download `note` about the trace folder | The file downloaded fine but landed outside the per-call folder: another extension took over `chrome.downloads.onDeterminingFilename`. Trust the reported path; do not assume the folder layout. Ask the user to disable that extension's download renaming to restore it. | | Unsupported operation | Use available capabilities; suggest updating only if the missing feature is needed. | Navigation alone (including deprecated help outcome `navigated`) is not completion. diff --git a/crates/bsk-protocol/src/tools/file_transfer.rs b/crates/bsk-protocol/src/tools/file_transfer.rs index e2cd97c8..53f49cf7 100644 --- a/crates/bsk-protocol/src/tools/file_transfer.rs +++ b/crates/bsk-protocol/src/tools/file_transfer.rs @@ -104,6 +104,12 @@ pub struct DownloadResult { /// Opaque id returned by the daemon to the CLI. #[serde(default, skip_serializing_if = "Option::is_none")] pub transfer_id: Option, + /// Set when the completed file is not under the trace-scoped + /// `browserRelativeDir`, meaning another extension won the + /// `chrome.downloads.onDeterminingFilename` race. The download succeeded; + /// only the per-call folder grouping was lost. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub note: Option, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize, JsonSchema)] @@ -210,4 +216,32 @@ mod tests { assert!(value.get("browser_relative_dir").is_none()); assert!(value.get("max_byte_size").is_none()); } + + fn download_result(note: Option) -> DownloadResult { + DownloadResult { + tab_id: 4, + used_ref: None, + used_selector: None, + suggested_filename: "report.csv".into(), + byte_size: 12, + mime: None, + danger: None, + browser_path: Some("/profile/Downloads/BrowserSkill/tr_1/report.csv".into()), + transfer_id: None, + note, + } + } + + #[test] + fn download_result_omits_the_note_when_the_suggestion_was_honoured() { + let value = serde_json::to_value(download_result(None)).unwrap(); + assert!(value.get("note").is_none()); + } + + #[test] + fn download_result_carries_the_note_when_the_suggestion_was_dropped() { + let value = + serde_json::to_value(download_result(Some("another extension won".into()))).unwrap(); + assert_eq!(value["note"], "another extension won"); + } } diff --git a/packages/dsh-plugin-browserskill/skill/references/help-and-recovery.md b/packages/dsh-plugin-browserskill/skill/references/help-and-recovery.md index 499f2edb..ba9d15a8 100644 --- a/packages/dsh-plugin-browserskill/skill/references/help-and-recovery.md +++ b/packages/dsh-plugin-browserskill/skill/references/help-and-recovery.md @@ -44,4 +44,8 @@ backends to bypass limits. Borrow confirmation still applies. - Timeout/unknown effect: inspect before retrying; the action may have happened. - Unconfirmed fill: read the field. Formatting may satisfy the goal; correct only a remaining difference instead of blindly refilling or requesting help. +- Download `note` about the trace folder: the file downloaded fine but landed outside + the per-call folder, because another extension took over + `chrome.downloads.onDeterminingFilename`. Trust the reported path; do not assume + the folder layout. Ask the user to disable that extension's download renaming. - Other errors: follow the hint; on unrecoverable failure, report and stop the owned session.