feat: add browser tool powered by playwright-go#187
feat: add browser tool powered by playwright-go#187strelov1 wants to merge 2 commits intosipeed:mainfrom
Conversation
Replace chromedp with playwright-go to support two connection protocols: - CDP (Chromium/Browserless) via ConnectOverCDP - Playwright Wire Protocol (Firefox/Camoufox) via Firefox.Connect 13 actions: navigate, click, type, screenshot, get_text, evaluate, wait, scroll, hover, select, pdf, cookies, close. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
strelov1
force-pushed
the
feat/browser-playwright
branch
from
394356b to
7142079
Compare
Changes SummaryThis PR adds browser automation capabilities to the picoclaw agent using playwright-go. It introduces a new browser tool supporting both CDP (Chrome DevTools Protocol) for Chromium/Browserless and Playwright Wire Protocol for Firefox/Camoufox, with 13 browser actions including navigation, screenshots, form interaction, and content extraction. Type: feature Components Affected: tools, config, agent, dependencies Files Changed
Architecture Impact
Risk Areas: Connection management: browser connections are maintained as stateful resources with mutex protection but no explicit timeout/cleanup beyond close action, Resource leaks: if browser.Close() or pw.Stop() fail, resources may leak; errors are collected but connection marked as closed anyway, Security: JavaScript evaluation via 'evaluate' action allows arbitrary code execution in browser context, Cookie manipulation: delete action uses clear-and-restore pattern which could fail partially leaving cookies in inconsistent state, Text truncation: get_text action truncates at 50000 chars which could lose important data without clear indication to LLM beyond [truncated] note, Concurrent access: mutex protects connection state but individual page operations are not synchronized, potential race conditions if tool called concurrently Suggestions
Full review in progress... | Powered by diffray |
Review Summary
Validated 81 issues: 23 kept, 58 filtered Issues Found: 23💬 See 23 individual line comment(s) for details. 📊 16 unique issue type(s) across 23 location(s) 📋 Full issue list (click to expand)🔴 CRITICAL - Race condition: page accessed without mutex protectionAgent: bugs Category: bug File: Description: Action methods (doNavigate, doClick, etc.) access t.page without mutex after ensureConnected() returns. If doClose() runs concurrently, t.page becomes nil causing panic. Suggestion: Add t.mu.Lock()/defer t.mu.Unlock() at start of each action method, or hold mutex during entire Execute() call. Confidence: 90% Rule: 🔴 CRITICAL - Integration tests depend on external browser servicesAgent: testing Category: testing File: Description: Integration tests require real external browser services via environment variables. These will skip in CI/CD without access to these services. Unit tests for action handlers would improve coverage. Suggestion: Create mocks for playwright-go Browser, Page, and Playwright interfaces. Use dependency injection to inject mocked playwright instances for unit testing action handlers. Confidence: 85% Rule: 🟠 HIGH - Ignored error when re-adding cookies in delete operation (2 occurrences)Agent: bugs Category: bug 📍 View all locations
Rule: 🟠 HIGH - Resource cleanup missing defer for playwright.Stop() on Connect errorAgent: go Category: bug File: Description: When Firefox.Connect fails, pw.Stop() is called manually. If panic occurs between Run() and error check, playwright process leaks. Suggestion: Use defer pattern with cleanup flag: success := false; defer func() { if !success { pw.Stop() } }(); then set success = true before return nil. Confidence: 75% Rule: 🟠 HIGH - Slice access without bounds checkAgent: go Category: bug File: Description: The code accesses contexts[0].Pages()[0] without storing the Pages() result. While it checks len(contexts[0].Pages()) > 0, calling Pages() again could return a different slice due to race conditions, causing a panic if the second call returns an empty slice. Suggestion: Store the result of contexts[0].Pages() in a variable before checking length and accessing index 0: Confidence: 85% Rule: 🟠 HIGH - Unbounded screenshot data allocation (3 occurrences)Agent: performance Category: performance 📍 View all locations
Rule: 🟠 HIGH - Arbitrary JavaScript Execution via LLM OutputAgent: security Category: security File: Description: The doEvaluate function accepts an 'expression' parameter and executes it directly via page.Evaluate() without any validation. This allows arbitrary JS execution in the browser context if LLM output is manipulated. Suggestion: Consider implementing a whitelist of allowed JavaScript patterns, restricting to read-only operations, or adding a warning about the security implications in documentation. Confidence: 85% Rule: 🟠 HIGH - Server-Side Request Forgery (SSRF) via Unvalidated URL Navigation (2 occurrences)Agent: security Category: security 📍 View all locations
Rule: 🟠 HIGH - Missing unit test coverage for error paths in action handlers (2 occurrences)Agent: testing Category: testing 📍 View all locations
Rule: 🟡 MEDIUM - Unchecked error return from os.MkdirAllAgent: bugs Category: bug File: Description: Error from os.MkdirAll(workspace, 0755) is not checked. If directory creation fails, subsequent file operations will fail. Suggestion: Check the error: if err := os.MkdirAll(workspace, 0755); err != nil { return nil, fmt.Errorf("failed to create workspace: %w", err) } Confidence: 85% Rule: 🟡 MEDIUM - BrowserConfig has overlapping fields (CdpURL and WsURL)Agent: architecture Category: quality File: Description: Both CdpURL and WsURL exist with undocumented fallback logic. WsURL takes precedence over CdpURL (seen in loop.go:91-94) but this is not documented. Suggestion: Document precedence in struct comments or use single URL field with protocol determining interpretation. Confidence: 70% Rule: 🟡 MEDIUM - JSON marshalling error ignored in doEvaluate (2 occurrences)Agent: go Category: quality 📍 View all locations
Rule: 🟡 MEDIUM - URL parameter concatenation lacks proper encodingAgent: quality Category: quality File: Description: The buildCdpURL function manually constructs URL query parameters by string concatenation without proper URL encoding. The token and launch JSON parameters are not URL-encoded, which could cause issues with special characters. Suggestion: Use url.Values from the net/url package to properly encode query parameters instead of manual string concatenation. Confidence: 80% Rule: 🟡 MEDIUM - Sensitive Cookie Data Exposure via LLM Context (2 occurrences)Agent: security Category: security 📍 View all locations
Rule: 🔵 LOW - Custom string contains function reimplements stdlibAgent: quality Category: quality File: Description: Test file implements custom contains/containsSubstring functions instead of using strings.Contains from standard library. Suggestion: Replace with strings.Contains: import "strings" and use strings.Contains(s, substr) Confidence: 90% Rule: 🔵 LOW - Error ignored when getting user home directoryAgent: go Category: quality File: Description: os.UserHomeDir() error is ignored in expandHome. If getting the home directory fails, an empty string is used which could lead to incorrect path resolution. Suggestion: Check the error and either return the original path unchanged, log a warning, or document that empty home is acceptable. Consider: home, err := os.UserHomeDir(); if err != nil { return path } Confidence: 65% Rule: 🔇 3 low-severity issue(s) not posted (min: medium)
📝 10 additional issue(s) shown in summary only (max: 10 inline)
Review ID: |
- Handle ignored error from AddCookies in cookie delete operation - Check os.MkdirAll error in workspace directory creation - Store Pages() result in variable to avoid redundant calls - Document CdpURL/WsURL field precedence in BrowserConfig Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@Zepan This PR addresses roadmap issue #293 (Autonomous Browser Operations — priority: high) using Consideration: PR #318 also targets the same roadmap item but uses a CLI subprocess approach ( Recommendation: Compare with #318 before deciding. For a project targeting $10 hardware with <10MB RAM, the subprocess approach may be more appropriate than embedding playwright-go. |
3 participants
Summary
playwright-goConnectOverCDP) for Chromium/BrowserlessFirefox.Connect) for Firefox/CamoufoxBrowserConfigto config with protocol, URLs, token, stealth, timeoutsConfiguration
New dependency
github.com/playwright-community/playwright-gov0.5200.1Test plan
go build ./pkg/...— compiles without errorsgo test ./pkg/...— all existing tests passBROWSER_TEST_CDP_URL=ws://... go test ./pkg/tools/ -run TestBrowserTool_Integration_CDP -vBROWSER_TEST_PW_URL=ws://... go test ./pkg/tools/ -run TestBrowserTool_Integration_Playwright -v🤖 Generated with Claude Code