Context
Follow-up from #494 (no need to block that PR). The new upload_file action is solid; this is a small polish item about how its errors are communicated to the LLM.
Observation
The upload error codes (upload_disabled, upload_path_not_allowed, upload_path_not_file, upload_target_not_file_input, upload_path_required) are thrown as bare tokens:
throw new BrowserActionException("upload_file", "upload_path_not_allowed");BrowserActionException passes the second arg straight through as error.message (packages/core/src/errors.ts:73), and performActionWithValidation returns it verbatim to the model with isRecoverable: true (packages/core/src/tools/webActionTools.ts:278). So the agent receives the literal string "upload_path_not_allowed" and a "you may retry" signal — with no semantics telling it the rejection is a path-policy decision that a different element can't fix.
This is inconsistent with the other action errors, which give the model actionable prose, e.g.:
// packages/core/src/errors.ts:44 (InvalidRefException)
`Invalid element reference '${ref}'. The element does not exist on the current page.
Please check the page snapshot for valid element references.`Observed effect (minor)
When a path is rejected, the agent occasionally retries upload_file against a different element ref before aborting — which can never succeed, since the rejection is about the path, not the element. It's intermittent and clusters in cases where the path looks valid (e.g. a symlink whose name/existence checks out but resolves outside the allowlist):
| Scenario |
Runs |
Retried before abort |
| Plain outside-allowlist |
4 |
0 |
| Symlink escape |
3 |
1 |
No correctness or security impact — the firewall blocks every attempt. Cost is at most a couple of wasted LLM round-trips.
Suggested fix
Replace the bare codes with self-describing messages that name the failure as a policy decision and steer away from retrying a different element, e.g.:
upload_path_not_allowed: '<path>' is outside the configured upload allowlist. A different element will not help — stop, or ask the user to adjust upload_allowed_paths.
Pure string change at the BrowserActionException / failedActionResult call sites (packages/core/src/browser/playwrightBrowser.ts:563,571,575,591,603,1203 and packages/core/src/tools/webActionTools.ts:377). Leave isRecoverable alone — that flag drives broader loop behavior and isn't worth the risk for this.
Note: the retry is LLM behavior, so the wording change is expected to reduce it, not guarantee elimination. Worth a quick before/after check if anyone wants to confirm the payoff.
Context
Follow-up from #494 (no need to block that PR). The new
upload_fileaction is solid; this is a small polish item about how its errors are communicated to the LLM.Observation
The upload error codes (
upload_disabled,upload_path_not_allowed,upload_path_not_file,upload_target_not_file_input,upload_path_required) are thrown as bare tokens:BrowserActionExceptionpasses the second arg straight through aserror.message(packages/core/src/errors.ts:73), andperformActionWithValidationreturns it verbatim to the model withisRecoverable: true(packages/core/src/tools/webActionTools.ts:278). So the agent receives the literal string"upload_path_not_allowed"and a "you may retry" signal — with no semantics telling it the rejection is a path-policy decision that a different element can't fix.This is inconsistent with the other action errors, which give the model actionable prose, e.g.:
Observed effect (minor)
When a path is rejected, the agent occasionally retries
upload_fileagainst a different element ref before aborting — which can never succeed, since the rejection is about the path, not the element. It's intermittent and clusters in cases where the path looks valid (e.g. a symlink whose name/existence checks out but resolves outside the allowlist):No correctness or security impact — the firewall blocks every attempt. Cost is at most a couple of wasted LLM round-trips.
Suggested fix
Replace the bare codes with self-describing messages that name the failure as a policy decision and steer away from retrying a different element, e.g.:
Pure string change at the
BrowserActionException/failedActionResultcall sites (packages/core/src/browser/playwrightBrowser.ts:563,571,575,591,603,1203andpackages/core/src/tools/webActionTools.ts:377). LeaveisRecoverablealone — that flag drives broader loop behavior and isn't worth the risk for this.Note: the retry is LLM behavior, so the wording change is expected to reduce it, not guarantee elimination. Worth a quick before/after check if anyone wants to confirm the payoff.