Attach screenshots (CDP) + ESM build fix + idempotent claim#37
Open
jddunn wants to merge 1 commit into
Open
Conversation
…ild fix browser_attach_screenshot writes a PNG of the agent tab for research evidence, implemented over CDP Page.captureScreenshot (page pixels — no coordinate math, no focus steal, no other windows). Threaded through the daemon protocol/client/ surface so it works on the IPC lane too. The JXA lane's screencapture fallback is present but DISABLED by default: its -R crop works in display points while Chrome bounds + Retina scale disagree, so the region drifted onto neighbouring windows in live testing — unacceptable for an evidence file. Opt in with WUNDERLAND_ATTACH_JXA_SHOTS=1 only. Also fixes two real defects found while proving this live: - JxaBackend.claimAgentTab is now idempotent: probeIdentity() claims a tab first, so the controller's own claim consumed a SECOND about:blank and failed NO_BLANK_TAB on every attach. - package/tsconfig emit ESM (type: module, Node16) matching sibling curated extensions; the CommonJS emit made every dist import fail at runtime.
Reviewer's GuideImplements a CDP-based screenshot capture pipeline for the browser-automation attach system, fixes idempotency of tab claiming in the JXA backend, and converts the package to a proper ESM/Node16 build configuration with protocol and test updates. Sequence diagram for CDP-based browser_attach_screenshot pipelinesequenceDiagram
actor Agent
participant AttachScreenshotTool
participant AttachSurface as DaemonAttachSurface
participant DaemonClient as AttachDaemonClient
participant Daemon as AttachDaemon
participant Controller as AttachController
participant Backend as RawCdpBackend
Agent->>AttachScreenshotTool: execute({ path, fullPage })
AttachScreenshotTool->>AttachSurface: screenshot(path, fullPage)
AttachSurface->>DaemonClient: screenshot(path, fullPage)
DaemonClient->>Daemon: request('screenshot', { path, fullPage })
Daemon->>Controller: screenshot(a.path, a.fullPage)
Controller->>Backend: screenshotTab(tab, fullPage)
Backend->>Backend: send('Page.captureScreenshot', { format: 'png', captureBeyondViewport })
Backend-->>Controller: base64 PNG data
Controller->>Controller: writeFileSync(filePath, buf)
Controller-->>Daemon: { path, bytes }
Daemon-->>DaemonClient: { path, bytes }
DaemonClient-->>AttachSurface: { path, bytes }
AttachSurface-->>AttachScreenshotTool: { path, bytes }
AttachScreenshotTool-->>Agent: tool envelope with result
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThe attach package adds screenshot capture through controller, CDP and JXA backends, daemon transport, and a new ChangesScreenshot capture
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AttachScreenshotTool
participant DaemonAttachSurface
participant AttachDaemonClient
participant AttachDaemon
participant AttachController
participant RawCdpBackend
AttachScreenshotTool->>DaemonAttachSurface: screenshot(path, fullPage)
DaemonAttachSurface->>AttachDaemonClient: screenshot(path, fullPage)
AttachDaemonClient->>AttachDaemon: queue screenshot operation
AttachDaemon->>AttachController: screenshot(path, fullPage)
AttachController->>RawCdpBackend: screenshotTab(tab, fullPage)
RawCdpBackend-->>AttachController: base64 PNG
AttachController-->>AttachDaemon: path and bytes
AttachDaemon-->>AttachScreenshotTool: screenshot result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
AttachBackend.screenshotTaband related surface/tool docs describe the capability as CDP-only and non-focusing, but the JXA backend now implementsscreenshotTaband explicitly focuses the window; consider reconciling these comments with the actual behavior or scoping the JXA implementation differently. - In the daemon
screenshothandler, invalid or missingpathcurrently returns anUNKNOWNerror code; using a more specific validation-style code (e.g., something akin toBAD_REQUEST/INVALID_ARG) would make error handling and diagnostics clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `AttachBackend.screenshotTab` and related surface/tool docs describe the capability as CDP-only and non-focusing, but the JXA backend now implements `screenshotTab` and explicitly focuses the window; consider reconciling these comments with the actual behavior or scoping the JXA implementation differently.
- In the daemon `screenshot` handler, invalid or missing `path` currently returns an `UNKNOWN` error code; using a more specific validation-style code (e.g., something akin to `BAD_REQUEST`/`INVALID_ARG`) would make error handling and diagnostics clearer.
## Individual Comments
### Comment 1
<location path="registry/curated/system/browser-automation/src/attach/backends/jxa.ts" line_range="221-230" />
<code_context>
+ const file = join(mkdtempSync(join(tmpdir(), 'attach-shot-')), 'shot.png');
</code_context>
<issue_to_address>
**suggestion (performance):** Temporary directories created for screenshots are never removed, which can accumulate over time.
Each screenshot call creates a new temp directory via `mkdtempSync`, but only `shot.png` is deleted, leaving the directory in `/tmp`. Over many runs this can lead to unnecessary disk usage, especially for long-lived processes. Please also remove the temp directory returned by `mkdtempSync` (e.g. `rmSync(dir, { recursive: true, force: true })`) in the `finally` block.
Suggested implementation:
```typescript
const { mkdtempSync, readFileSync, existsSync, rmSync } = await import('node:fs');
const { tmpdir } = await import('node:os');
const dir = mkdtempSync(join(tmpdir(), 'attach-shot-'));
const file = join(dir, 'shot.png');
```
You’ll also need to update the surrounding error-handling/cleanup logic:
1. In the `finally` block that currently deletes `shot.png` (if present), add `rmSync(dir, { recursive: true, force: true });` to remove the temporary directory.
2. If there is no `finally` block yet, wrap the screenshot capture and file reading logic in a `try`/`finally` and in `finally`:
- Check `existsSync(file)` and delete it as currently done.
- Call `rmSync(dir, { recursive: true, force: true });` to ensure the temp directory is removed even if the file was never written.
3. Ensure `dir` is in scope for that `finally` block (i.e. declared in the same function scope rather than inside a narrower block).
</issue_to_address>
### Comment 2
<location path="registry/curated/system/browser-automation/src/attach/tools.ts" line_range="118-122" />
<code_context>
+ 'Capture a PNG screenshot of the agent tab to a local file path — visual evidence for a research report. Requires the CDP transport (the AppleScript/JXA transport has no pixel access and refuses with UNSUPPORTED_OP). Read-only: never navigates or mutates the page.';
+ readonly category = 'browser';
+ readonly version = '0.1.0';
+ readonly hasSideEffects = false;
+ readonly inputSchema = {
+ type: 'object' as const,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The screenshot tool writes a file to disk, which is a side effect despite being read-only from the page’s perspective.
Although the operation is read-only for page state, `browser_attach_screenshot` still writes a PNG to disk. If `hasSideEffects` is meant to capture any external state changes (not just page mutations), this should be `true` or the semantics of `hasSideEffects` should be clarified to avoid upstream tools misclassifying this call as purely observational.
```suggestion
readonly description =
'Capture a PNG screenshot of the agent tab to a local file path — visual evidence for a research report. Requires the CDP transport (the AppleScript/JXA transport has no pixel access and refuses with UNSUPPORTED_OP). Read-only with respect to page state (never navigates or mutates the page), but writes a PNG file to disk.';
readonly category = 'browser';
readonly version = '0.1.0';
readonly hasSideEffects = true;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+221
to
+230
| const file = join(mkdtempSync(join(tmpdir(), 'attach-shot-')), 'shot.png'); | ||
| // Crop to the agent WINDOW's bounds. A full-display capture would include | ||
| // every other window the user has open — private content must never land | ||
| // in a research evidence shot. | ||
| let region: string | undefined; | ||
| try { | ||
| const b = JSON.parse(raised) as { x: number; y: number; width: number; height: number }; | ||
| if ([b.x, b.y, b.width, b.height].every((n) => typeof n === 'number' && Number.isFinite(n)) && b.width > 0 && b.height > 0) { | ||
| region = `${Math.max(0, Math.round(b.x))},${Math.max(0, Math.round(b.y))},${Math.round(b.width)},${Math.round(b.height)}`; | ||
| } |
There was a problem hiding this comment.
suggestion (performance): Temporary directories created for screenshots are never removed, which can accumulate over time.
Each screenshot call creates a new temp directory via mkdtempSync, but only shot.png is deleted, leaving the directory in /tmp. Over many runs this can lead to unnecessary disk usage, especially for long-lived processes. Please also remove the temp directory returned by mkdtempSync (e.g. rmSync(dir, { recursive: true, force: true })) in the finally block.
Suggested implementation:
const { mkdtempSync, readFileSync, existsSync, rmSync } = await import('node:fs');
const { tmpdir } = await import('node:os');
const dir = mkdtempSync(join(tmpdir(), 'attach-shot-'));
const file = join(dir, 'shot.png');You’ll also need to update the surrounding error-handling/cleanup logic:
- In the
finallyblock that currently deletesshot.png(if present), addrmSync(dir, { recursive: true, force: true });to remove the temporary directory. - If there is no
finallyblock yet, wrap the screenshot capture and file reading logic in atry/finallyand infinally:- Check
existsSync(file)and delete it as currently done. - Call
rmSync(dir, { recursive: true, force: true });to ensure the temp directory is removed even if the file was never written.
- Check
- Ensure
diris in scope for thatfinallyblock (i.e. declared in the same function scope rather than inside a narrower block).
Comment on lines
+118
to
+122
| readonly description = | ||
| 'Capture a PNG screenshot of the agent tab to a local file path — visual evidence for a research report. Requires the CDP transport (the AppleScript/JXA transport has no pixel access and refuses with UNSUPPORTED_OP). Read-only: never navigates or mutates the page.'; | ||
| readonly category = 'browser'; | ||
| readonly version = '0.1.0'; | ||
| readonly hasSideEffects = false; |
There was a problem hiding this comment.
suggestion (bug_risk): The screenshot tool writes a file to disk, which is a side effect despite being read-only from the page’s perspective.
Although the operation is read-only for page state, browser_attach_screenshot still writes a PNG to disk. If hasSideEffects is meant to capture any external state changes (not just page mutations), this should be true or the semantics of hasSideEffects should be clarified to avoid upstream tools misclassifying this call as purely observational.
Suggested change
| readonly description = | |
| 'Capture a PNG screenshot of the agent tab to a local file path — visual evidence for a research report. Requires the CDP transport (the AppleScript/JXA transport has no pixel access and refuses with UNSUPPORTED_OP). Read-only: never navigates or mutates the page.'; | |
| readonly category = 'browser'; | |
| readonly version = '0.1.0'; | |
| readonly hasSideEffects = false; | |
| readonly description = | |
| 'Capture a PNG screenshot of the agent tab to a local file path — visual evidence for a research report. Requires the CDP transport (the AppleScript/JXA transport has no pixel access and refuses with UNSUPPORTED_OP). Read-only with respect to page state (never navigates or mutates the page), but writes a PNG file to disk.'; | |
| readonly category = 'browser'; | |
| readonly version = '0.1.0'; | |
| readonly hasSideEffects = true; |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92372f54b4
ℹ️ 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".
| 'Capture a PNG screenshot of the agent tab to a local file path — visual evidence for a research report. Requires the CDP transport (the AppleScript/JXA transport has no pixel access and refuses with UNSUPPORTED_OP). Read-only: never navigates or mutates the page.'; | ||
| readonly category = 'browser'; | ||
| readonly version = '0.1.0'; | ||
| readonly hasSideEffects = false; |
There was a problem hiding this comment.
When a mission uses the documented deny-side-effects policy, this tool remains callable even though AttachController.screenshot() creates directories and overwrites the caller-selected path with writeFileSync. Because the path can name any file writable by the agent process, an unapproved invocation can truncate user files while bypassing the policy intended to gate writes; mark this tool as side-effecting and consider constraining or refusing existing destinations.
Useful? React with 👍 / 👎.
Comment on lines
+218
to
+220
| 'Page.captureScreenshot', | ||
| { format: 'png', captureBeyondViewport: !!fullPage }, | ||
| this.sessionId!, |
There was a problem hiding this comment.
When fullPage is true on a document taller than the viewport, this still captures only the viewport: captureBeyondViewport permits a requested clip to extend outside the viewport, but it does not expand the default screenshot area by itself. Query the layout/content dimensions and pass an appropriate clip (or temporarily resize the viewport) so the advertised full-page option includes the entire scrollable page.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@registry/curated/system/browser-automation/src/attach/AttachController.ts`:
- Around line 287-302: Constrain the file-writing logic in screenshot to a
configured evidence directory before creating the output: reject traversal,
symlinked paths, special files, and existing non-regular targets, and create the
result exclusively and atomically. Preserve dry-run behavior and return the
resolved safe destination and byte count after writing.
In `@registry/curated/system/browser-automation/src/attach/backends/jxa.ts`:
- Around line 203-245: Remove the WUNDERLAND_ATTACH_JXA_SHOTS opt-in escape
hatch from screenshotTab so the JXA screenshot implementation always rejects the
operation. Keep the existing AttachError response and do not execute raise,
bounds resolution, or screencapture while this unsafe fallback remains
unavailable.
In `@registry/curated/system/browser-automation/src/attach/backends/raw-cdp.ts`:
- Around line 215-221: Update screenshotTab to cap full-page capture dimensions
before calling Page.captureScreenshot, using the page layout or pixel bounds to
prevent oversized PNG responses. Preserve normal viewport screenshots, and
ensure the cap is applied before the base64 result is held or converted to a
Buffer.
In `@registry/curated/system/browser-automation/src/attach/tools.ts`:
- Around line 119-122: Update the screenshot tool’s hasSideEffects property to
indicate that capturing a screenshot is side-effecting because it creates
directories and writes a PNG file. Keep the existing category and version
unchanged so confirmation and policy controls apply to this tool’s local
filesystem writes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a65f1ff-4303-428c-9a31-bda684aa0535
📒 Files selected for processing (12)
registry/curated/system/browser-automation/package.jsonregistry/curated/system/browser-automation/src/attach/AttachController.tsregistry/curated/system/browser-automation/src/attach/__tests__/pack-registration.test.tsregistry/curated/system/browser-automation/src/attach/__tests__/tools.test.tsregistry/curated/system/browser-automation/src/attach/backends/jxa.tsregistry/curated/system/browser-automation/src/attach/backends/raw-cdp.tsregistry/curated/system/browser-automation/src/attach/daemon/client.tsregistry/curated/system/browser-automation/src/attach/daemon/daemon.tsregistry/curated/system/browser-automation/src/attach/daemon/protocol.tsregistry/curated/system/browser-automation/src/attach/daemon/surface.tsregistry/curated/system/browser-automation/src/attach/tools.tsregistry/curated/system/browser-automation/tsconfig.json
Comment on lines
+287
to
+302
| async screenshot(filePath: string, fullPage?: boolean): Promise<{ path: string; bytes: number }> { | ||
| this.requireReady(); | ||
| requireLease(this.opts.leaseFile, this.lease!.nonce); | ||
| const fn = this.backend.screenshotTab?.bind(this.backend); | ||
| if (!fn) { | ||
| throw new AttachError('UNSUPPORTED_OP', `${this.backend.kind} backend has no screenshot capability (CDP transport required)`); | ||
| } | ||
| if (this.dryRun) return { path: filePath, bytes: 0 }; | ||
| this.machine.to('reading'); | ||
| try { | ||
| const b64 = await withDeadline(fn(this.tab!, fullPage), this.deadline, 'screenshot'); | ||
| const { writeFileSync, mkdirSync } = await import('node:fs'); | ||
| const { dirname } = await import('node:path'); | ||
| mkdirSync(dirname(filePath), { recursive: true }); | ||
| const buf = Buffer.from(b64, 'base64'); | ||
| writeFileSync(filePath, buf); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Constrain screenshot destinations before writing.
Line 300 writes a claimant-supplied path as the daemon user. Since daemon validation only checks that path is non-empty, a caller can overwrite arbitrary files or target special files. Restrict output to a configured evidence directory and reject traversal, symlinks, and non-regular targets; use exclusive/atomic creation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@registry/curated/system/browser-automation/src/attach/AttachController.ts`
around lines 287 - 302, Constrain the file-writing logic in screenshot to a
configured evidence directory before creating the output: reject traversal,
symlinked paths, special files, and existing non-regular targets, and create the
result exclusively and atomically. Preserve dry-run behavior and return the
resolved safe destination and byte count after writing.
Comment on lines
+203
to
+245
| async screenshotTab(tab: string, _fullPage?: boolean): Promise<string> { | ||
| // OFF BY DEFAULT. `screencapture -R` works in display POINTS while Chrome's | ||
| // window bounds and Retina backing scale disagree, so the cropped region | ||
| // drifted onto NEIGHBOURING windows in live testing (2026-07-24) — which | ||
| // would silently put the user's unrelated, private windows into a research | ||
| // evidence file. Correct page-pixel capture is CDP `Page.captureScreenshot` | ||
| // (see RawCdpBackend); this lane stays refused unless explicitly opted in | ||
| // via WUNDERLAND_ATTACH_JXA_SHOTS=1 for a single-display debug session. | ||
| if (process.env.WUNDERLAND_ATTACH_JXA_SHOTS !== '1') { | ||
| throw new AttachError( | ||
| 'UNSUPPORTED_OP', | ||
| 'screenshots require the CDP transport (WUNDERLAND_ATTACH_TRANSPORT=cdp); the JXA display-capture lane is disabled because its crop can include other windows', | ||
| ); | ||
| } | ||
| const [wid, tid] = tab.split(/\s+/); | ||
| const raised = await this.exec('raise', [wid, tid]); | ||
| const { mkdtempSync, readFileSync, existsSync, rmSync } = await import('node:fs'); | ||
| const { tmpdir } = await import('node:os'); | ||
| const file = join(mkdtempSync(join(tmpdir(), 'attach-shot-')), 'shot.png'); | ||
| // Crop to the agent WINDOW's bounds. A full-display capture would include | ||
| // every other window the user has open — private content must never land | ||
| // in a research evidence shot. | ||
| let region: string | undefined; | ||
| try { | ||
| const b = JSON.parse(raised) as { x: number; y: number; width: number; height: number }; | ||
| if ([b.x, b.y, b.width, b.height].every((n) => typeof n === 'number' && Number.isFinite(n)) && b.width > 0 && b.height > 0) { | ||
| region = `${Math.max(0, Math.round(b.x))},${Math.max(0, Math.round(b.y))},${Math.round(b.width)},${Math.round(b.height)}`; | ||
| } | ||
| } catch { | ||
| /* older driver without bounds → refuse rather than capture the screen */ | ||
| } | ||
| if (!region) { | ||
| throw new AttachError('UNSUPPORTED_OP', 'could not resolve agent-window bounds; refusing a full-display capture'); | ||
| } | ||
| try { | ||
| await pExecFile('screencapture', ['-x', '-o', '-t', 'png', '-R', region, file], { timeout: 20_000 }); | ||
| if (!existsSync(file)) { | ||
| throw new AttachError('UNSUPPORTED_OP', 'screencapture produced no file (Screen Recording permission?)'); | ||
| } | ||
| return readFileSync(file).toString('base64'); | ||
| } catch (err) { | ||
| if (err instanceof AttachError) throw err; | ||
| throw new AttachError('UNSUPPORTED_OP', `screencapture failed: ${err instanceof Error ? err.message : String(err)}`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not expose the known-unsafe JXA capture fallback.
The implementation acknowledges that its point/pixel mismatch can capture neighboring private windows, then enables that same path via an environment variable. Keep screenshotTab unavailable for JXA until bounds can be converted and validated reliably; an opt-in flag is not sufficient containment for an exposed evidence-writing tool.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@registry/curated/system/browser-automation/src/attach/backends/jxa.ts` around
lines 203 - 245, Remove the WUNDERLAND_ATTACH_JXA_SHOTS opt-in escape hatch from
screenshotTab so the JXA screenshot implementation always rejects the operation.
Keep the existing AttachError response and do not execute raise, bounds
resolution, or screencapture while this unsafe fallback remains unavailable.
Comment on lines
+215
to
+221
| async screenshotTab(tab: string, fullPage?: boolean): Promise<string> { | ||
| this.requireTab(tab); | ||
| const res = (await this.send( | ||
| 'Page.captureScreenshot', | ||
| { format: 'png', captureBeyondViewport: !!fullPage }, | ||
| this.sessionId!, | ||
| 45_000, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== raw-cdp.ts around screenshotTab ==\n'
sed -n '180,280p' registry/curated/system/browser-automation/src/attach/backends/raw-cdp.ts
printf '\n== search for screenshot-related guards and metrics ==\n'
rg -n "captureBeyondViewport|Page\.getLayoutMetrics|layoutMetrics|screenshotTab|captureScreenshot|clip|pixel budget|fullPage" registry/curated/system/browser-automation/src -SRepository: framerslab/agentos-extensions
Length of output: 8311
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' registry/curated/system/browser-automation/src/attach/backends/raw-cdp.tsRepository: framerslab/agentos-extensions
Length of output: 10155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== raw-cdp.ts around screenshotTab ==\n'
sed -n '180,280p' registry/curated/system/browser-automation/src/attach/backends/raw-cdp.ts
printf '\n== search for screenshot-related guards and metrics ==\n'
rg -n "captureBeyondViewport|Page\.getLayoutMetrics|layoutMetrics|screenshotTab|captureScreenshot|clip|pixel budget|fullPage" registry/curated/system/browser-automation/src -SRepository: framerslab/agentos-extensions
Length of output: 8311
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== AttachController screenshot flow ==\n'
sed -n '260,340p' registry/curated/system/browser-automation/src/attach/AttachController.ts
printf '\n== daemon/client screenshot path ==\n'
sed -n '100,150p' registry/curated/system/browser-automation/src/attach/daemon/client.ts
sed -n '240,290p' registry/curated/system/browser-automation/src/attach/daemon/daemon.ts
printf '\n== search for screenshot limits / byte caps ==\n'
rg -n "screenshot.*(limit|max|cap|budget|bytes|size)|base64|Buffer\\.from\\(.*base64|captureBeyondViewport|fullPage" registry/curated/system/browser-automation/src -SRepository: framerslab/agentos-extensions
Length of output: 13686
Cap full-page screenshot output.
captureBeyondViewport can turn a tall page into a huge PNG, and this path immediately holds the result as base64 before writing it as a Buffer. Add a layout/pixel cap before allowing full-page capture so a single request can’t exhaust Chrome or the daemon.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@registry/curated/system/browser-automation/src/attach/backends/raw-cdp.ts`
around lines 215 - 221, Update screenshotTab to cap full-page capture dimensions
before calling Page.captureScreenshot, using the page layout or pixel bounds to
prevent oversized PNG responses. Preserve normal viewport screenshots, and
ensure the cap is applied before the base64 result is held or converted to a
Buffer.
Comment on lines
+119
to
+122
| 'Capture a PNG screenshot of the agent tab to a local file path — visual evidence for a research report. Requires the CDP transport (the AppleScript/JXA transport has no pixel access and refuses with UNSUPPORTED_OP). Read-only: never navigates or mutates the page.'; | ||
| readonly category = 'browser'; | ||
| readonly version = '0.1.0'; | ||
| readonly hasSideEffects = false; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Mark the screenshot tool as side-effecting.
This tool creates directories and writes a PNG, but Line 122 declares hasSideEffects = false. That can bypass tool confirmation or policy controls intended for local writes.
Proposed fix
- readonly hasSideEffects = false;
+ readonly hasSideEffects = true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 'Capture a PNG screenshot of the agent tab to a local file path — visual evidence for a research report. Requires the CDP transport (the AppleScript/JXA transport has no pixel access and refuses with UNSUPPORTED_OP). Read-only: never navigates or mutates the page.'; | |
| readonly category = 'browser'; | |
| readonly version = '0.1.0'; | |
| readonly hasSideEffects = false; | |
| 'Capture a PNG screenshot of the agent tab to a local file path — visual evidence for a research report. Requires the CDP transport (the AppleScript/JXA transport has no pixel access and refuses with UNSUPPORTED_OP). Read-only: never navigates or mutates the page.'; | |
| readonly category = 'browser'; | |
| readonly version = '0.1.0'; | |
| readonly hasSideEffects = true; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@registry/curated/system/browser-automation/src/attach/tools.ts` around lines
119 - 122, Update the screenshot tool’s hasSideEffects property to indicate that
capturing a screenshot is side-effecting because it creates directories and
writes a PNG file. Keep the existing category and version unchanged so
confirmation and policy controls apply to this tool’s local filesystem writes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
browser_attach_screenshotover CDPPage.captureScreenshot(threaded through the daemon protocol/client/surface). The JXA display-capture fallback is deliberately disabled by default — its crop drifted onto neighbouring windows in live testing, which would leak private windows into evidence files; CDP is the correct page-pixel path.Two real defects found while proving it live:
claimAgentTabwas not idempotent → every attach failedNO_BLANK_TAB(probeIdentity already claimed the tab).type: module+ Node16, matching sibling curated exts.Summary by Sourcery
Introduce a CDP-backed screenshot capability for the browser attach surface and wire it through tools and daemon, while fixing attach tab claiming and aligning the package with ESM/Node16 builds.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests:
Summary by CodeRabbit