fix(browser): keep upload errors specific#101465
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a48a7026e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| timeoutMs: opts.timeoutMs, | ||
| }), | ||
| timeoutMs: 20000, | ||
| timeoutMs: resolveBrowserOperationRequestTimeoutMs(opts.timeoutMs), |
There was a problem hiding this comment.
When an atomic upload includes ref and a small timeoutMs (for example 1000ms), this changed watchdog becomes only timeoutMs + 5000, but the server route applies that timeout only to armFileUploadViaPlaywright; the subsequent clickViaPlaywright in extensions/browser/src/browser/routes/agent.act.hooks.ts is called without timeoutMs and falls back to the 8000ms interaction default from act-policy.ts. For a stale/hidden trigger ref, the client can still abort first and return the generic browser-control timeout this change is trying to avoid. Either include the trigger-click timeout/default in this outer timeout calculation or pass the upload timeout through to the trigger click.
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Reviewed July 7, 2026, 4:15 AM ET / 08:15 UTC. Summary PR surface: Source -1, Tests +71, Docs +2. Total +72 across 6 files. Reproducibility: no. high-confidence live reproduction was run in this review. Source inspection gives a clear reproduction for the review finding: an atomic upload with Review metrics: none identified. Stored data model Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the compatibility-preserving atomic upload direction, but make the hook watchdog cover both chooser arming and trigger click, remove the release-owned changelog edit, and add redacted live browser proof before merge. Do we have a high-confidence way to reproduce the issue? No high-confidence live reproduction was run in this review. Source inspection gives a clear reproduction for the review finding: an atomic upload with Is this the best way to solve the issue? No, not yet. The atomic guidance and hook slack are the right direction, but the implementation must include the trigger-click timeout/default in the watchdog calculation and provide live browser proof before merge. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 82b0f7cdc88d. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source -1, Tests +71, Docs +2. Total +72 across 6 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
6a48a70 to
b16eb37
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b16eb378cd
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| timeoutMs: opts.timeoutMs, | ||
| }), | ||
| timeoutMs: 20000, | ||
| timeoutMs: resolveBrowserOperationRequestTimeoutMs(opts.timeoutMs), |
There was a problem hiding this comment.
When action=upload uses inputRef or element with a short explicit timeoutMs, this new watchdog becomes timeoutMs + 5000, but the route does not pass that timeout into direct file-input uploads (extensions/browser/src/browser/routes/agent.act.hooks.ts:96-102), and setInputFilesViaPlaywright calls locator.setInputFiles(resolvedPaths) without options (extensions/browser/src/browser/pw-tools-core.interactions.ts:1495-1496). A slow/stale input can therefore still be cut off by the client first and return the generic browser-control timeout this change is trying to avoid. Either forward the timeout to setInputFiles or keep the outer watchdog above the direct-input operation timeout.
Useful? React with 👍 / 👎.
|
Land-ready proof for exact head What this solves:
Dependency contract checked directly against the installed Verification:
The refactor is bounded: five files, +84/-13 overall, and production code is net -1 line. No filesystem allowlist, sandbox permission, or browser networking boundary changed. |
|
Merged via squash.
|
What Problem This Solves
Closes #38844.
Browser uploads already support an atomic chooser path—send
pathsand the triggerrefin one request—but the agent-facing guidance still encouraged a split arm-then-click flow. On dynamic pages, that creates an avoidable window for a snapshot ref to go stale.The direct browser client also gave file-chooser and dialog hooks a fixed 20-second outer watchdog even though their advertised Playwright operation timeout defaults to 120 seconds. When the outer watchdog won, callers could receive a generic browser-control timeout/restart message instead of the specific stale-ref or Playwright operation error.
Current
mainhas already improved dispatcher error preservation and browser-act timeout slack since the original 2026.3.2 report. This change closes the remaining hook-specific mismatch and makes the atomic upload contract visible to agents and operators.Why This Change Was Made
refandpathsin the same upload call when available.inputRefandelementuploads unchanged.--refform and the compatibility rule.The exact pinned Playwright 1.61.1 contract was checked:
page.waitForEvent("filechooser", { timeout })owns the chooser timeout and rejects on page crash/close, whileFileChooser.setFiles()delegates to the file input operation. The outer client must therefore stay alive beyond that operation deadline rather than race it at 20 seconds.User Impact
Dynamic-page uploads are less likely to fail from a stale trigger ref, and real stale-ref/action errors stay actionable instead of being masked as browser-control reachability failures. Scripts that intentionally arm with paths first and click later continue to work.
Evidence
Remote provider: Blacksmith Testbox, lease
tbx_01kwxpxmb3x1g1prnaa9cs4230(silver-prawn).pnpm docs:list— completed before docs validation.pnpm test extensions/browser/src/browser/client-actions-core.hooks.test.ts extensions/browser/src/browser/client-actions-core.downloads.test.ts extensions/browser/src/browser/server.agent-contract-form-layout-act-commands.test.ts extensions/browser/src/browser/pw-tools-core.screenshots-element-selector.test.ts extensions/browser/src/browser-tool.test.ts— 130 passed.pnpm check:changed -- <touched browser/docs files>— production/test typechecks, changed-file lint, repository guards, docs lane, and import-cycle checks passed.gpt-5.5, high) reported no actionable findings.Regression tests prove explicit atomic uploads receive operation timeout plus transport slack, paths-only arming remains valid with the 120-second default, dialog hooks use the same deadline contract, and the agent description recommends the atomic form.
One broader
extensions/browser/src/browser/client.test.tsrun exposes an unrelated current-maintest-double failure (response.arrayBuffer is not a function) after the bounded-response reader change; 133 other tests in that run passed. This PR does not alter that stale test harness.Live Dev Gateway + real OpenAI agent + managed Chromium dynamic-page proof is still pending. This PR does not claim that proof yet and should not land until it is added in a follow-up comment.