Conversation
|
Thanks for the PR. The approach looks sound: detecting the final-path mismatch and providing actionable recovery guidance is a useful improvement. Before merging, could you please address the following?
Please add the browser and extension versions, reproduction steps, and observed results to the PR description. The scope should remain clear: this PR improves conflict detection and recovery guidance; full compatibility with competing download managers can be addressed separately. The existing cleanup edge cases can also be tracked independently. Once the formatting is fixed and the real-browser validation confirms these behaviors, I’d be comfortable moving this PR toward merge. |
Thanks for the review. Both requested items are now complete:
Environment: Windows 11 build [The latest CI run](https://github.com/Tencent/BrowserSkill/actions/runs/36722168107) passes all checks, including both Windows installer test variants. The earlier pipe error did not recur. I also observed Chrono cancelling and restarting plain HTTP downloads, causing ambiguous attribution. That separate compatibility behavior remains outside this PR, consistent with the requested scope. |
Fixes #361
Problem
When BrowserSkill and Chrono both suggest a download filename, Chromium prioritizes the extension installed or updated most recently. If Chrono wins, the download can complete outside BrowserSkill’s
BrowserSkill/tr_<id>/directory.The daemon only imports a browser file whose parent matches the per-transfer directory. A file saved elsewhere therefore cannot become the requested
--outfile. Previously, the failure did not explain that the suggested directory had been ignored.Change
download_path_mismatchwitheffect_state: committedwhen its parent does not match the per-transfer directory. Provide recovery guidance for competing extensions, browser-rejected filenames, and different save locations. Preserve the completed browser file because its location may have been chosen by the user, and direct users to locate it in Downloads before retrying.The new parameterized test’s formatting was corrected in
edd71a6. Commitbcfddccadds a comment explaining why the completed path needs validation.Validation
Tests and CI
file-transfer,download-deadline,download-effect-state): 44 passed, including path mismatch and file preservation.git diff --check: passed.bcfddcc: all checks passed, including Biome and Windows installer tests under PowerShell 5.1 and 7.6.6.Real Windows Edge validation of this PR’s build
The tested BrowserSkill CLI, daemon, and extension were built locally from PR commit
edd71a6, which includes the #361 fix. The source tree’s package version remained0.3.1. Automatic CLI updates were disabled to keep those PR build artifacts fixed throughout validation.The current head,
bcfddcc, adds only an explanatory comment after the browser validation; the functional code is identical to the tested commit.Environment
10.0.26200, x64.154.0.4258.37, using a visible window and an isolated test profile.0.13.12, using unmodified files from its official Chrome Web Store package, loaded unpacked. Both extensions therefore used unpacked extension IDs.Validation exercised the complete PR-built CLI → PR-built daemon → PR-built extension → real Edge download flow. Download commands and browser download events were not mocked.
Reproduction
Build the BrowserSkill CLI and extension from
edd71a6, start the built daemon with an isolatedBSK_HOME, and load the built extension into Edge.Before adding Chrono, download a 63-byte text attachment from a local test page to establish the normal baseline.
Add Chrono after BrowserSkill, leaving its default download/naming settings enabled. Use a test-page link to a 1,484-byte HTTPS JSON attachment.
Observe the test tab to obtain the link reference, then run the PR-built CLI:
Disable Chrono in Edge’s extension manager, observe the same tab again, and repeat the identical command with the same session, tab, and output path.
The HTTPS attachment was
version.jsonfrom the repository’s release assets, used solely as download fixture data.Observed results
0; the requested output contained the expected 63 bytes.3,reason: download_path_mismatch,effect_state: committed, andphase: download. No requested--outfile was created. Edge reported the browser download as complete, and the file remained on disk outside the per-transfer directory.0;--outcontained 1,484 bytes matching the source attachment’s SHA-256. The original browser download from the conflict case also remained intact.Additional observation: With a plain HTTP attachment, Chrono cancelled the original download and started a replacement with the same URL. This produced ambiguous attribution (
download_capture_failed,effect_state: unknown) before final-path validation. Compatibility with that separate takeover behavior remains outside this PR.This PR improves conflict detection and recovery guidance. Full compatibility with competing download managers and existing cleanup edge cases remain separate follow-up work.