fix(browser): repair isolated download seam#64558
Conversation
14f5a18 to
dbb7793
Compare
Pearcekieser
changed the title
Greptile SummaryThis PR repairs three interlocked issues in the isolated browser download path: lazy auth-dep loading in
Confidence Score: 4/5Safe to merge after addressing the unhandled-rejection risk in the download event handler; the auth and CDP-readiness fixes are correct. One P1 finding: the floating extensions/browser/src/browser/pw-session.ts — the new Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/browser/src/browser/pw-session.ts
Line: 405-413
Comment:
**Unhandled rejection from `managedSave` IIFE**
`managedSave` is a floating promise — nothing attaches a rejection handler to the promise itself. Assigning `patched.path = async () => await managedSave` only handles the rejection when `path()` is actively called. If `download.saveAs()` fails before any caller awaits `path()` (e.g., disk full during `saveAs`, or the download is cancelled at the browser level immediately), Node.js 22 will emit `unhandledRejection` and, with the default `--unhandled-rejections=throw` mode, terminate the gateway process.
A minimal fix is to attach a no-op rejection handler to suppress the unhandled signal while still propagating the error to any caller who awaits `path()`:
```suggestion
const managedSave = (async () => {
await fs.mkdir(DEFAULT_DOWNLOAD_DIR, { recursive: true });
await fs.rm(managedPath, { force: true });
await download.saveAs(managedPath);
return managedPath;
})();
// Suppress unhandled-rejection warnings: the error is still propagated to
// any caller that awaits patched.path().
managedSave.catch(() => {});
const patched = download as typeof download & { path?: () => Promise<string> };
patched.path = async () => await managedSave;
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/browser/src/browser/pw-session.ts
Line: 396-413
Comment:
**Concurrent downloads with identical filenames will clobber each other**
`managedPath` is `DEFAULT_DOWNLOAD_DIR/{suggestedFilename}` — a flat, non-unique path. If two `page.on("download")` callbacks fire before either finishes (e.g., a page that triggers multiple downloads at once), both handlers will:
1. `fs.rm(managedPath, { force: true })` — interleave the deletions
2. `download.saveAs(managedPath)` — race to write the same file
The second `saveAs` could overwrite the first's file while the first caller still holds a reference to the path, or one `saveAs` could fail because the other just deleted the partially-written bytes via `rm`.
Consider generating a unique path (e.g., using a UUID prefix, similar to `buildTempDownloadPath` in `pw-tools-core.downloads.ts`) and then renaming/linking to `managedPath` when done, or at minimum documenting that concurrent same-name downloads are not supported here.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(browser): repair isolated download s..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d11cda81b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69d60f2924
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
c8a04fc to
e526c84
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f760fbbb88
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
572de40 to
ecb1124
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
obviyus
left a comment
obviyus
left a comment
There was a problem hiding this comment.
Thanks for digging into this. I think the isolated-profile download bug in #18598 looks real, but this PR still feels over-scoped for that issue: the Chrome download pref decoration looks plausibly relevant, while the lazy /act auth change and the Playwright download.path() patch read like separate seam fixes. Could you either split those pieces out, or add direct proof that they are required for the reported chrome://downloads filename/clickability bug on macOS?
|
Codex deep review: this is the better/current-path download-seam fix, and it supersedes #48132. Current I rechecked the earlier Greptile concern against the current diff: the branch now uses unique managed paths and attaches Remaining scope note: this should not close #38519 by itself. It repairs the underlying unmanaged download seam, but #38519 asks for agent-facing download capture/metadata (or a tool-level |
|
Codex follow-up deep review: still the right successor to #48132, but this branch is no longer direct-mergeable. Current
So the bug class is real and the final shape in this PR is still good. The remaining issue is freshness: the branch is now ~3.6k commits behind Best path now:
I would preserve contributor credit if this lands via a maintainer rewrite. |
|
Codex maintainer rewrite landed on Kept the useful pieces from this PR and left out the unrelated auth/availability changes:
Validation after rebase: |
3 participants
Summary
browserAct(...)could hit loopback bridge auth before headers were resolved, and plain browser-tool clicks exposed Playwright artifact download paths instead of the managed OpenClaw downloads dir.client-fetch.tsis now lazy so the browser client can attach auth headers without a module-init seam issue; isolated launches tolerate a short CDP-readiness wobble instead of immediately tearing Chrome down; and raw page download events are patched to save into the managed downloads dir unless an explicit waiter is already handling the download.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
client-fetch.tsneeded browser control auth while also importing modules that participated in the same startup graph, so loopback/actrequests could miss auth and fail withUnauthorized. Separately, even when the click succeeded, plain browser-tool download events still surfaced Playwright artifact paths instead of the managed downloads dir because only the explicit wait/download helper path normalized the saved location./actand then consumes the raw Playwrightdownloadevent.Regression Test Plan (if applicable)
extensions/browser/src/browser/pw-tools-core.downloads.browser-tool-seam.live.test.ts/actclick against an isolated browser download should not return a Playwright artifact path and should land in the managed OpenClaw downloads dir with the expected filename/body./act+ auth + isolated browser download event handling), not just inside a lower-level helper.User-visible / Behavior Changes
Diagram (if applicable)
Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation: N/ARepro + Verification
Environment
profile=openclaw-style flow via local bridge)Steps
<a href="/download">attachment link./actclick path.downloadevent path returned to the caller.Expected
/actclick works through the authenticated loopback browser client path.Actual
origin/main: fails, returning a Playwright artifact pathorigin/mainshowed the loopback browser client can still fail withUnauthorizedthroughbrowserAct(...), which is the auth seam cleaned up here.Evidence
Attach at least one:
Human Verification (required)
fix-18598-downloads@c59f857486fail,origin/main@3c03d41f13fail, fixed branch3d11cda81bpass).browserAct(...)harness onorigin/mainalso exposed the loopback auth seam asUnauthorized, confirming the earlier “download prefs only” story was incomplete.chrome://downloads, and the flaky broader browser-in-loop live variant was intentionally removed instead of being kept as misleading coverage.Review Conversations
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoRisks and Mitigations
Risk: patching raw page download events could interfere with the explicit wait/download helper path.
Risk: making launch readiness more tolerant could mask real Chrome startup failures.