feat(server): add wait and custom code tools#984
Conversation
|
Claude (@claude) review |
|
Greptile (@greptileai) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 2m 6s —— View job Code Review: feat(server): add wait and custom code tools
Overall this is a solid PR. The Bug:
|
✅ Tests passed — 1238/1242
|
Greptile SummaryThis PR registers two new MCP tools —
Confidence Score: 5/5Safe to merge — the new tools are well-guarded, the was-present logic is correct, and the test coverage is thorough. The core polling logic, was-present guards for both textGone and selectorGone, runCode error propagation, and the single-condition validation gate are all correct. Tests cover false-positive suppression, fixed-time waits, error paths, and value serialisation. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent
participant wait_for tool
participant Browser.waitFor
participant Page (CDP)
Agent->>wait_for tool: { page, text/textGone/selector/selectorGone/time }
alt time only
wait_for tool->>wait_for tool: setTimeout(time)
wait_for tool-->>Agent: { found: true, timeout: time }
else text / selector
wait_for tool->>Browser.waitFor: { text/selector, timeout }
loop every 500ms until deadline
Browser.waitFor->>Page (CDP): Runtime.evaluate(innerText.includes / querySelector)
Page (CDP)-->>Browser.waitFor: true / false
end
Browser.waitFor-->>wait_for tool: found (bool)
wait_for tool-->>Agent: { found, target, timeout } + snapshot if found
else textGone / selectorGone
wait_for tool->>Browser.waitFor: { textGone/selectorGone, timeout }
loop every 500ms until deadline
Browser.waitFor->>Page (CDP): Runtime.evaluate
Page (CDP)-->>Browser.waitFor: present (bool)
Note over Browser.waitFor: Set wasPresent=true on first true, return true after wasPresent && absent
end
Browser.waitFor-->>wait_for tool: found (bool)
wait_for tool-->>Agent: { found, target, timeout } + snapshot if found
end
Agent->>browser_run_code tool: { page, code, args }
browser_run_code tool->>Browser.runCode: (page, code, args)
Browser.runCode->>Page (CDP): Runtime.evaluate(async (args) => { code })(args)
Page (CDP)-->>Browser.runCode: result / exceptionDetails
Browser.runCode-->>browser_run_code tool: { value, description } or { error }
browser_run_code tool-->>Agent: text + structured value, or error
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browseros-agent/apps/server/src/tools/navigation.ts:351-356
The error message only calls out `time` as the disallowed combination partner, which implies to a caller (or an LLM) that combining two non-time conditions like `{ text, selector }` is valid. Since the guard rejects any `conditionCount > 1`, the message should reflect the actual rule.
```suggestion
if (conditionCount > 1) {
response.error(
'Provide exactly one wait condition: text, textGone, selector, selectorGone, or time.',
)
return
}
```
### Issue 2 of 2
packages/browseros-agent/apps/server/src/tools/navigation.ts:335
Zod's `.default(10000)` on the `timeout` field guarantees `args.timeout` is always a number in the handler, so the `?? 10_000` fallback is dead code. This is worth removing under the team rule about keeping dead code out of the codebase.
```suggestion
const timeout = args.timeout
```
Reviews (2): Last reviewed commit: "fix: require prior presence for textGone..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (opts.textGone) { | ||
| const result = await session.Runtime.evaluate({ | ||
| expression: `!(document.body?.innerText?.includes(${JSON.stringify(opts.textGone)}) ?? false)`, | ||
| returnByValue: true, | ||
| }) | ||
| if (result.result?.value === true) return true | ||
| } |
There was a problem hiding this comment.
textGone returns true prematurely on an unloaded page
The expression !(document.body?.innerText?.includes(...) ?? false) evaluates to true when document.body is null or undefined (page still loading). The optional chain short-circuits to undefined, undefined ?? false becomes false, and !false is true, so the condition reports the text as "gone" even before any content has rendered. Contrast with the text check which uses the same pattern affirmatively — it correctly returns false (not found) when the body is absent.
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: 682-688
Comment:
**`textGone` returns true prematurely on an unloaded page**
The expression `!(document.body?.innerText?.includes(...) ?? false)` evaluates to `true` when `document.body` is `null` or `undefined` (page still loading). The optional chain short-circuits to `undefined`, `undefined ?? false` becomes `false`, and `!false` is `true`, so the condition reports the text as "gone" even before any content has rendered. Contrast with the `text` check which uses the same pattern affirmatively — it correctly returns `false` (not found) when the body is absent.
How can I resolve this? If you propose a fix, please make it concise.| if (opts.textGone) { | ||
| const result = await session.Runtime.evaluate({ | ||
| expression: `!(document.body?.innerText?.includes(${JSON.stringify(opts.textGone)}) ?? false)`, | ||
| returnByValue: true, | ||
| }) | ||
| if (result.result?.value === true) return true | ||
| } |
There was a problem hiding this comment.
textGone lacks the "was-present" guard that selectorGone has
selectorGone only reports success after the element was observed at least once (via selectorGoneWasPresent), preventing a false positive when the selector simply never existed. textGone has no equivalent guard and returns true on the very first poll if the text is already absent. A caller doing waitFor({ textGone: 'Loading…' }) before the page has rendered any text would receive found: true immediately, even though the text never appeared.
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: 682-688
Comment:
**`textGone` lacks the "was-present" guard that `selectorGone` has**
`selectorGone` only reports success after the element was observed at least once (via `selectorGoneWasPresent`), preventing a false positive when the selector simply never existed. `textGone` has no equivalent guard and returns `true` on the very first poll if the text is already absent. A caller doing `waitFor({ textGone: 'Loading…' })` before the page has rendered any text would receive `found: true` immediately, even though the text never appeared.
How can I resolve this? If you propose a fix, please make it concise.
|
Addressed the correctness item in 5960bce: now mirrors by requiring prior presence before disappearance succeeds. Also cleaned up the gone-condition success message and removed the timer monkey-patch from the browser wait tests. |
|
Greptile (@greptileai) review |
|
Claude (@claude) review |
|
Follow-up pushed in 5960bce: textGone now mirrors selectorGone with a prior-presence guard; gone-condition success text now uses Condition met; wait-for tests use real polling instead of timer monkey-patching. |
|
Claude finished Nikhil (@shadowfax92)'s task in 1m 27s —— View job Claude finished Nikhil (@shadowfax92)'s task — View job Code Review: feat(server): add wait and custom code tools
The fixes in 5960bce look correct. All items from the previous review have been addressed.
|
1 participant
Fixes #422
Summary
wait_forandbrowser_run_codeMCP tools for BrowserOS server clients.wait_forvalidation so ambiguous condition combinations fail loudly, and makeselectorGonewait for prior selector presence before reporting disappearance.Test plan
bun --env-file=.env.development test ./tests/tools/registry.test.ts ./tests/browser/wait-for.test.tsbun run test:browserbunx biome check apps/server/src/agent/prompt.ts apps/server/src/browser/browser.ts apps/server/src/tools/navigation.ts apps/server/src/tools/registry.ts apps/server/src/tools/snapshot.ts apps/server/src/tools/tool-label-registry.ts apps/server/tests/tools/navigation.test.ts apps/server/tests/tools/observation.test.ts apps/server/tests/tools/registry.test.ts apps/server/tests/browser/wait-for.test.tsbun run typecheckfrompackages/browseros-agent/apps/serverLocal blockers
FORCE_COLOR=1 bun scripts/build/server.ts --target=darwin-arm64 --ciis blocked locally becauseapps/server/.env.productionis missing.bun run test:toolsis blocked locally because/Applications/BrowserOS.app/Contents/MacOS/BrowserOSis not installed in this worktree environment; the run was stopped after repeated identical launch failures.