feat(server): add wait and custom code tools#980
Conversation
✅ Tests passed — 1232/1236
|
Greptile SummaryThis PR adds two new agent tools —
Confidence Score: 3/5The two new tools are broadly correct, but the wait_for handler has a logic gap that silently discards the time field whenever it is combined with any other condition, which could confuse the LLM agent driving the browser. The wait_for handler accepts time alongside text/textGone/selector/selectorGone without error, then silently ignores it — an LLM caller following the schema will get no feedback that its time constraint was dropped. This is an observable wrong-result path on a core navigation tool that is now being re-enabled in production. The selectorGone false-positive on pages that never rendered the target element is a secondary concern worth addressing before the tool is widely used. packages/browseros-agent/apps/server/src/tools/navigation.ts and packages/browseros-agent/apps/server/src/browser/browser.ts warrant a closer look before merging. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[wait_for called] --> B{any condition\nprovided?}
B -- no --> ERR1[error: provide a condition]
B -- yes --> C{time provided AND\nother condition?}
C -- yes --> SILENTDROP["⚠️ time silently dropped\n(bug: should error)"]
SILENTDROP --> F
C -- no --> D{time only?}
D -- yes --> E[setTimeout for time ms\nreturn found:true]
D -- no --> F[browser.waitFor loop]
F --> G{poll: text / textGone\nselector / selectorGone}
G -- any condition true --> H[return true]
G -- deadline exceeded --> I[return false]
H --> J{found?}
J -- yes --> K[response.text + snapshot]
J -- no --> L[response.error timeout]
M[browser_run_code called] --> N["wrap code in\nasync(args)=>{ code }"]
N --> O[CDP Runtime.evaluate\nawaitPromise:true]
O --> P{exceptionDetails?}
P -- yes --> Q[response.error]
P -- no --> R[response.text + data]
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/browseros-agent/apps/server/src/tools/navigation.ts:349-368
When `time` is provided alongside any of `text`, `textGone`, `selector`, or `selectorGone`, the fixed-wait branch is skipped and `time` is silently discarded — only the timeout-based wait runs. There is no validation or feedback to the caller. An LLM agent that passes `{ time: 2000, text: "hello" }` (intending a minimum-pause) will have the 2 000 ms wait entirely ignored and the tool will proceed straight to the `waitFor` loop.
```suggestion
if (
!args.text &&
!args.textGone &&
!args.selector &&
!args.selectorGone &&
args.time === undefined
) {
response.error(
'Provide text, textGone, selector, selectorGone, or time to wait for.',
)
return
}
if (
args.time !== undefined &&
(args.text || args.textGone || args.selector || args.selectorGone)
) {
response.error(
'time cannot be combined with text, textGone, selector, or selectorGone. Use time alone for a fixed wait.',
)
return
}
if (
args.time !== undefined &&
!args.text &&
!args.textGone &&
!args.selector &&
!args.selectorGone
) {
```
### Issue 2 of 3
packages/browseros-agent/apps/server/src/tools/navigation.ts:336-347
**Misleading `target` when multiple conditions are provided**
When multiple conditions are supplied (e.g., `textGone: "Loading"` and `selectorGone: ".spinner"`), `target` is resolved via a priority chain that always picks the first non-undefined field — even if a lower-priority field triggered the actual match. If `.spinner` disappears first but `textGone` is the higher-priority field, the response says `target: 'text "Loading" to disappear'`, which misreports what was actually matched. The success message sent to the agent is equally misleading ("Found text 'Loading' to disappear on page" when it was the selector that disappeared).
### Issue 3 of 3
packages/browseros-agent/apps/server/src/browser/browser.ts:697-703
**`selectorGone` returns `true` immediately on a page where the selector never existed**
The expression `!document.querySelector(selector)` returns `true` whenever the selector is absent, including on blank pages or pages that never had the element. On a freshly navigated page that hasn't yet injected a loading spinner, `selectorGone: ".spinner"` would resolve to `true` on the very first poll rather than waiting for the element to appear and then disappear. If the intent is to confirm that a previously-visible element has been removed, callers relying on `selectorGone` as "wait until element appears then disappears" will get a misleading success on pages that never rendered the element at all.
Reviews (1): Last reviewed commit: "feat: add wait and custom code browser t..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ( | ||
| !args.text && | ||
| !args.textGone && | ||
| !args.selector && | ||
| !args.selectorGone && | ||
| args.time === undefined | ||
| ) { | ||
| response.error( | ||
| 'Provide text, textGone, selector, selectorGone, or time to wait for.', | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| if ( | ||
| args.time !== undefined && | ||
| !args.text && | ||
| !args.textGone && | ||
| !args.selector && | ||
| !args.selectorGone | ||
| ) { |
There was a problem hiding this comment.
time is provided alongside any of text, textGone, selector, or selectorGone, the fixed-wait branch is skipped and time is silently discarded — only the timeout-based wait runs. There is no validation or feedback to the caller. An LLM agent that passes { time: 2000, text: "hello" } (intending a minimum-pause) will have the 2 000 ms wait entirely ignored and the tool will proceed straight to the waitFor loop.
| if ( | |
| !args.text && | |
| !args.textGone && | |
| !args.selector && | |
| !args.selectorGone && | |
| args.time === undefined | |
| ) { | |
| response.error( | |
| 'Provide text, textGone, selector, selectorGone, or time to wait for.', | |
| ) | |
| return | |
| } | |
| if ( | |
| args.time !== undefined && | |
| !args.text && | |
| !args.textGone && | |
| !args.selector && | |
| !args.selectorGone | |
| ) { | |
| if ( | |
| !args.text && | |
| !args.textGone && | |
| !args.selector && | |
| !args.selectorGone && | |
| args.time === undefined | |
| ) { | |
| response.error( | |
| 'Provide text, textGone, selector, selectorGone, or time to wait for.', | |
| ) | |
| return | |
| } | |
| if ( | |
| args.time !== undefined && | |
| (args.text || args.textGone || args.selector || args.selectorGone) | |
| ) { | |
| response.error( | |
| 'time cannot be combined with text, textGone, selector, or selectorGone. Use time alone for a fixed wait.', | |
| ) | |
| return | |
| } | |
| if ( | |
| args.time !== undefined && | |
| !args.text && | |
| !args.textGone && | |
| !args.selector && | |
| !args.selectorGone | |
| ) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/apps/server/src/tools/navigation.ts
Line: 349-368
Comment:
When `time` is provided alongside any of `text`, `textGone`, `selector`, or `selectorGone`, the fixed-wait branch is skipped and `time` is silently discarded — only the timeout-based wait runs. There is no validation or feedback to the caller. An LLM agent that passes `{ time: 2000, text: "hello" }` (intending a minimum-pause) will have the 2 000 ms wait entirely ignored and the tool will proceed straight to the `waitFor` loop.
```suggestion
if (
!args.text &&
!args.textGone &&
!args.selector &&
!args.selectorGone &&
args.time === undefined
) {
response.error(
'Provide text, textGone, selector, selectorGone, or time to wait for.',
)
return
}
if (
args.time !== undefined &&
(args.text || args.textGone || args.selector || args.selectorGone)
) {
response.error(
'time cannot be combined with text, textGone, selector, or selectorGone. Use time alone for a fixed wait.',
)
return
}
if (
args.time !== undefined &&
!args.text &&
!args.textGone &&
!args.selector &&
!args.selectorGone
) {
```
How can I resolve this? If you propose a fix, please make it concise.| const target = | ||
| args.text !== undefined | ||
| ? `text "${args.text}"` | ||
| : args.textGone !== undefined | ||
| ? `text "${args.textGone}" to disappear` | ||
| : args.selector !== undefined | ||
| ? `selector "${args.selector}"` | ||
| : args.selectorGone !== undefined | ||
| ? `selector "${args.selectorGone}" to disappear` | ||
| : args.time !== undefined | ||
| ? `${args.time}ms` | ||
| : '' |
There was a problem hiding this comment.
target when multiple conditions are provided
When multiple conditions are supplied (e.g., textGone: "Loading" and selectorGone: ".spinner"), target is resolved via a priority chain that always picks the first non-undefined field — even if a lower-priority field triggered the actual match. If .spinner disappears first but textGone is the higher-priority field, the response says target: 'text "Loading" to disappear', which misreports what was actually matched. The success message sent to the agent is equally misleading ("Found text 'Loading' to disappear on page" when it was the selector that disappeared).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/apps/server/src/tools/navigation.ts
Line: 336-347
Comment:
**Misleading `target` when multiple conditions are provided**
When multiple conditions are supplied (e.g., `textGone: "Loading"` and `selectorGone: ".spinner"`), `target` is resolved via a priority chain that always picks the first non-undefined field — even if a lower-priority field triggered the actual match. If `.spinner` disappears first but `textGone` is the higher-priority field, the response says `target: 'text "Loading" to disappear'`, which misreports what was actually matched. The success message sent to the agent is equally misleading ("Found text 'Loading' to disappear on page" when it was the selector that disappeared).
How can I resolve this? If you propose a fix, please make it concise.| if (opts.selectorGone) { | ||
| const result = await session.Runtime.evaluate({ | ||
| expression: `!document.querySelector(${JSON.stringify(opts.selectorGone)})`, | ||
| returnByValue: true, | ||
| }) | ||
| if (result.result?.value === true) return true | ||
| } |
There was a problem hiding this comment.
selectorGone returns true immediately on a page where the selector never existed
The expression !document.querySelector(selector) returns true whenever the selector is absent, including on blank pages or pages that never had the element. On a freshly navigated page that hasn't yet injected a loading spinner, selectorGone: ".spinner" would resolve to true on the very first poll rather than waiting for the element to appear and then disappear. If the intent is to confirm that a previously-visible element has been removed, callers relying on selectorGone as "wait until element appears then disappears" will get a misleading success on pages that never rendered the element at all.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/apps/server/src/browser/browser.ts
Line: 697-703
Comment:
**`selectorGone` returns `true` immediately on a page where the selector never existed**
The expression `!document.querySelector(selector)` returns `true` whenever the selector is absent, including on blank pages or pages that never had the element. On a freshly navigated page that hasn't yet injected a loading spinner, `selectorGone: ".spinner"` would resolve to `true` on the very first poll rather than waiting for the element to appear and then disappear. If the intent is to confirm that a previously-visible element has been removed, callers relying on `selectorGone` as "wait until element appears then disappears" will get a misleading success on pages that never rendered the element at all.
How can I resolve this? If you propose a fix, please make it concise.
|
Refinery rejected this merge request after review gate. Greptile found branch-caused wait_for behavior defects: time is silently ignored when combined with text/textGone/selector/selectorGone, and selectorGone can report success before the selector ever existed. Source issue bosmain-6fd has been reopened for rework; this branch is not merged. |
1 participant
Fixes #422
Verification: