feat(browser-mcp): structured results for screenshot, grep, and diff#1376
Conversation
- screenshot: always emit structuredContent {page, format, bytes, image,
annotations?} (was only {page, annotations}, and only when annotated).
- grep: emit {page, over, count, matches} on both match and no-match paths
(was just {count}); text output is unchanged so the CLI find command is unaffected.
- diff: add a "changed" boolean discriminator so the diff tool always returns
structuredContent, including {changed:false} on the no-change path.
tabs active/list already returned good structured output (cited as examples,
not gaps) and the Go CLI depends on active's page object, so they stay untouched.
|
Greptile (@greptileai) review |
|
Claude (@claude) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 2m 40s —— View job Code Review:
|
| Area | Verdict |
|---|---|
screenshot.ts |
✅ Correct — minor perf nit on Buffer.from allocation |
grep.ts |
✅ Correct — design question on raw content in structured vs. wrapping |
diff-format.ts |
✅ Clean |
| Tests | ✅ Solid coverage |
| Backward compat | ✅ Purely additive |
Good PR. The two items above are nits/design questions, not blockers.
✅ Tests passed — 1419/1423
|
Greptile SummaryThis PR completes structured output coverage for the three remaining
Confidence Score: 3/5The screenshot and diff changes are safe; grep's new structuredContent.matches exposes raw page text to LLM clients without the injection guard that the text path already has. The grep change puts unguarded page content into structuredContent.matches while the text path wraps the same data with wrapUntrusted. BrowserOS explicitly drives real logged-in sessions where scraped page text is an indirect-prompt-injection vector; diff-format.ts already sets the precedent of fencing content in structured output too. An LLM-driven MCP client consuming structuredContent.matches would receive raw page text with no injection-prevention markers. packages/browseros-agent/packages/browser-mcp/src/tools/grep.ts — the matches array in structuredContent needs the same trust-boundary treatment as the text path.
|
| Filename | Overview |
|---|---|
| packages/browseros-agent/packages/browser-mcp/src/tools/grep.ts | Adds page, over, count, and matches fields to structuredContent on both match and no-match paths; raw page content in matches is not wrapped with wrapUntrusted, unlike the text path and unlike diff-format.ts's structured output. |
| packages/browseros-agent/packages/browser-mcp/src/tools/screenshot.ts | structuredContent is now always emitted with page, format, bytes, image, and optional annotations; bytes is computed via a full Buffer allocation (minor inefficiency). |
| packages/browseros-agent/packages/browser-mcp/src/tools/diff-format.ts | Adds changed:false discriminator for the no-change path and changed:true for the change path; clean, additive, and consistent with existing wrapUntrusted usage in structured output. |
| packages/browseros-agent/apps/server/tests/tools/browser/grep.test.ts | New test file covering both match and no-match structured output for grep; thorough and well-structured. |
| packages/browseros-agent/apps/server/tests/tools/browser/register.test.ts | Updates existing screenshot tests to assert new structuredContent fields and adds a diff no-change assertion; all changes are correct and well-targeted. |
| packages/browseros-agent/apps/server/tests/tools/browser/screenshot-tool.test.ts | Extends existing screenshot unit tests to assert format, bytes, and image in structuredContent for both annotated and unannotated cases. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[MCP Tool Call] --> B{Tool}
B --> C[screenshot]
B --> D[grep]
B --> E[diff]
C --> C1[Capture image result.data base64]
C1 --> C2["structuredContent: { page, format, bytes, image, annotations? }"]
D --> D1{matches.length > 0?}
D1 -->|yes| D2["text: wrapUntrusted fenced\nstructuredContent: { page, over, count, matches }"]
D1 -->|no| D3["text: 'no matches'\nstructuredContent: { page, over, count:0, matches:[] }"]
D2 --> D4["⚠️ matches NOT wrapUntrusted-fenced"]
E --> E1{diff.changed?}
E1 -->|no| E2["structuredContent: { changed: false }"]
E1 -->|yes| E3["structuredContent: { changed: true, diff: wrappedDiff }"]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[MCP Tool Call] --> B{Tool}
B --> C[screenshot]
B --> D[grep]
B --> E[diff]
C --> C1[Capture image result.data base64]
C1 --> C2["structuredContent: { page, format, bytes, image, annotations? }"]
D --> D1{matches.length > 0?}
D1 -->|yes| D2["text: wrapUntrusted fenced\nstructuredContent: { page, over, count, matches }"]
D1 -->|no| D3["text: 'no matches'\nstructuredContent: { page, over, count:0, matches:[] }"]
D2 --> D4["⚠️ matches NOT wrapUntrusted-fenced"]
E --> E1{diff.changed?}
E1 -->|no| E2["structuredContent: { changed: false }"]
E1 -->|yes| E3["structuredContent: { changed: true, diff: wrappedDiff }"]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browseros-agent/packages/browser-mcp/src/tools/grep.ts:52-57
**Raw page content in `structuredContent.matches` bypasses the trust boundary**
The text path correctly fences matched lines with `wrapUntrusted(matches.join('\n'), origin)`, but the new `structuredContent.matches` array contains the same lines **unwrapped**. `diff-format.ts` sets the precedent: it puts `wrappedDiff` (the `wrapUntrusted`-fenced string) into its structured content too. An LLM-driven MCP client reading `structuredContent.matches` will receive raw page text with no indirect-prompt-injection markers; since BrowserOS drives real logged-in sessions, a hostile page can embed arbitrary instructions in its accessibility tree that flow directly to the model through this path.
### Issue 2 of 2
packages/browseros-agent/packages/browser-mcp/src/tools/screenshot.ts:92
`Buffer.from(..., 'base64').length` allocates an unnecessary buffer just to measure decoded byte length. Screenshots can be hundreds of KB to several MB, making this a non-trivial allocation on every call. The byte count can be derived directly from the base64 string length in O(1) without any allocation.
```suggestion
bytes: Math.floor((result.data.replace(/=+$/, '').length * 3) / 4),
```
Reviews (1): Last reviewed commit: "feat(browser-mcp): structured results fo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return textResult(wrapUntrusted(matches.join('\n'), origin), { | ||
| page: args.page, | ||
| over: args.over, | ||
| count: matches.length, | ||
| matches, | ||
| }) |
There was a problem hiding this comment.
structuredContent.matches bypasses the trust boundary
The text path correctly fences matched lines with wrapUntrusted(matches.join('\n'), origin), but the new structuredContent.matches array contains the same lines unwrapped. diff-format.ts sets the precedent: it puts wrappedDiff (the wrapUntrusted-fenced string) into its structured content too. An LLM-driven MCP client reading structuredContent.matches will receive raw page text with no indirect-prompt-injection markers; since BrowserOS drives real logged-in sessions, a hostile page can embed arbitrary instructions in its accessibility tree that flow directly to the model through this path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/packages/browser-mcp/src/tools/grep.ts
Line: 52-57
Comment:
**Raw page content in `structuredContent.matches` bypasses the trust boundary**
The text path correctly fences matched lines with `wrapUntrusted(matches.join('\n'), origin)`, but the new `structuredContent.matches` array contains the same lines **unwrapped**. `diff-format.ts` sets the precedent: it puts `wrappedDiff` (the `wrapUntrusted`-fenced string) into its structured content too. An LLM-driven MCP client reading `structuredContent.matches` will receive raw page text with no indirect-prompt-injection markers; since BrowserOS drives real logged-in sessions, a hostile page can embed arbitrary instructions in its accessibility tree that flow directly to the model through this path.
How can I resolve this? If you propose a fix, please make it concise.
Greptile SummaryThis PR completes structured output coverage for three
Confidence Score: 3/5Safe to merge for diff and screenshot; the grep change exposes raw page content in structuredContent without the prompt-injection fence that protects the text output path. The diff and screenshot changes are clean and additive with good test coverage. The grep change puts raw page-derived text into structuredContent.matches while the codebase explicitly wraps that same content with wrapUntrusted on the text path to guard against indirect prompt injection. BrowserOS drives real logged-in sessions, so page content is treated as a live injection vector throughout the codebase — skipping the fence on the structured path is a meaningful gap, not a theoretical one. packages/browseros-agent/packages/browser-mcp/src/tools/grep.ts — the matches array in structuredContent carries raw page content without the trust-boundary fence.
|
| Filename | Overview |
|---|---|
| packages/browseros-agent/packages/browser-mcp/src/tools/grep.ts | Adds page, over, count, and matches to structuredContent on both match and no-match paths; matches array contains raw page content that bypasses the wrapUntrusted trust boundary applied to the text output. |
| packages/browseros-agent/packages/browser-mcp/src/tools/screenshot.ts | Always emits structuredContent with page, format, bytes, image, and optional annotations; uses a redundant Buffer.from decode to compute bytes when arithmetic on the base64 length suffices. |
| packages/browseros-agent/packages/browser-mcp/src/tools/diff-format.ts | Adds changed: false discriminator to the no-change path and changed: true to all existing change paths — clean, additive, no regressions. |
| packages/browseros-agent/apps/server/tests/tools/browser/grep.test.ts | New test file covering both the match and no-match structured output paths for grep; clear and well-scoped. |
| packages/browseros-agent/apps/server/tests/tools/browser/register.test.ts | Extends existing screenshot and diff tests to assert the new structuredContent shapes; new diff no-change assertion is correctly isolated. |
| packages/browseros-agent/apps/server/tests/tools/browser/screenshot-tool.test.ts | Updates existing screenshot test assertions to expect the new format, bytes, and image fields in structuredContent; changes are accurate. |
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browseros-agent/packages/browser-mcp/src/tools/grep.ts:52-57
**`matches` in structuredContent bypasses the trust boundary**
The `matches` array is raw page content placed directly into `structuredContent`, but the text output correctly wraps the same lines with `wrapUntrusted`. Any MCP client (including an LLM-backed one) that consumes `structuredContent.matches` directly will receive page-derived text without the nonce-fenced untrusted-content marker, leaving it exposed to indirect prompt injection. A hostile page could embed instruction-like text in button labels or link text that appears verbatim in `matches`.
### Issue 2 of 2
packages/browseros-agent/packages/browser-mcp/src/tools/screenshot.ts:92
`Buffer.from(result.data, 'base64')` decodes the entire image into a new heap buffer just to read `.length`. For a 200 KB screenshot this allocates ~200 KB of memory that is immediately discarded. The byte length can be computed arithmetically from the base64 string itself.
```suggestion
bytes:
Math.floor(result.data.length * 3 / 4) -
(result.data.endsWith('==') ? 2 : result.data.endsWith('=') ? 1 : 0),
```
Reviews (2): Last reviewed commit: "feat(browser-mcp): structured results fo..." | Re-trigger Greptile
| return textResult(wrapUntrusted(matches.join('\n'), origin), { | ||
| page: args.page, | ||
| over: args.over, | ||
| count: matches.length, | ||
| matches, | ||
| }) |
There was a problem hiding this comment.
matches in structuredContent bypasses the trust boundary
The matches array is raw page content placed directly into structuredContent, but the text output correctly wraps the same lines with wrapUntrusted. Any MCP client (including an LLM-backed one) that consumes structuredContent.matches directly will receive page-derived text without the nonce-fenced untrusted-content marker, leaving it exposed to indirect prompt injection. A hostile page could embed instruction-like text in button labels or link text that appears verbatim in matches.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/packages/browser-mcp/src/tools/grep.ts
Line: 52-57
Comment:
**`matches` in structuredContent bypasses the trust boundary**
The `matches` array is raw page content placed directly into `structuredContent`, but the text output correctly wraps the same lines with `wrapUntrusted`. Any MCP client (including an LLM-backed one) that consumes `structuredContent.matches` directly will receive page-derived text without the nonce-fenced untrusted-content marker, leaving it exposed to indirect prompt injection. A hostile page could embed instruction-like text in button labels or link text that appears verbatim in `matches`.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| structuredContent: { | ||
| page: args.page, | ||
| format: args.format, | ||
| bytes: Buffer.from(result.data, 'base64').length, |
There was a problem hiding this comment.
Buffer.from(result.data, 'base64') decodes the entire image into a new heap buffer just to read .length. For a 200 KB screenshot this allocates ~200 KB of memory that is immediately discarded. The byte length can be computed arithmetically from the base64 string itself.
| bytes: Buffer.from(result.data, 'base64').length, | |
| bytes: | |
| Math.floor(result.data.length * 3 / 4) - | |
| (result.data.endsWith('==') ? 2 : result.data.endsWith('=') ? 1 : 0), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/packages/browser-mcp/src/tools/screenshot.ts
Line: 92
Comment:
`Buffer.from(result.data, 'base64')` decodes the entire image into a new heap buffer just to read `.length`. For a 200 KB screenshot this allocates ~200 KB of memory that is immediately discarded. The byte length can be computed arithmetically from the base64 string itself.
```suggestion
bytes:
Math.floor(result.data.length * 3 / 4) -
(result.data.endsWith('==') ? 2 : result.data.endsWith('=') ? 1 : 0),
```
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Summary
Close the remaining gaps so every
browser-mcptool returns clean, minimalstructuredContenton success (most already did;snapshot/diff/act/navigate/tabs/read/etc. were already structured).structuredContent—{ page, format, bytes, image, annotations? }(previously only{ page, annotations }, and only when annotations existed).bytesis the decoded image byte length;imageis the base64 string.{ page, over, count, matches }on both the match and no-match paths (previously just{ count }). The human/text output is byte-for-byte unchanged.changedboolean discriminator —{ changed: false }on the no-change path (previously nostructuredContentat all), andchanged: truealongsideadded/removedon the change path.Design
Surgical, additive edits through the existing
textResult/response.dataplumbing — no new abstractions, nooutputSchemadeclarations (onlyrundeclares one, by convention).tabs active/listwere intentionally left untouched: they already return good structured output (the request cited them as examples, not gaps), and the Go CLI'sactivecommand consumesstructuredContent.pageas a full page object and normalizes it itself (apps/cli/cmd/tabs.go), so reshaping it would breakactiveandsnap.Note on
screenshot.image: the base64 is intentionally included instructuredContent(as requested) even though it also appears in the image content block. This makes structured-only MCP clients self-sufficient; the trade-off is a larger payload on thebrowseros-cli screenshot --jsonpath that has no--out(the--outpath overrides structuredContent with{ path }). Easy to drop theimagefield later if that payload size matters.Test plan
bun run check(lint + typecheck + Fallow) — greenbun run test(full monorepo suite, incl. the Go-CLI integration test that launches BrowserOS) — greenscreenshot-tool.test.ts, newgrep.test.ts, diff no-change inregister.test.tsfindreads grep's text (unchanged),screenshot --outoverrides structured,diffchange is additive