opencode/picoclaw
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(tool/browser): address Copilot review round 2 (9 issues)

Browse source 
1. (#21) Move timeout ctx creation before connectIfNeeded so initial
   CDP connection also respects cfg.Timeout and caller cancellation.

2. (#23) Add bounds check to getIntArg: reject negative values and
   non-integer floats (e.g. 1.9 truncating to 1). Add 6 new test cases.

3. (#24/#26) Track temp screenshot files in BrowserTool.tempFiles and
   clean them up on executeClose. Update TempFileCleanup integration
   test to actually assert cleanup instead of just logging.

4. (#25) Remove hardcoded 30s timer in CDPClient.Navigate, rely solely
   on ctx.Done() so the tool's configured timeout is respected.

5. (#27/#28) Replace brittle strings.Contains(str, "error") detection
   in executeType/executeFill with proper JSON unmarshal into typed
   struct and check Error field directly.

6. (#29/#30) Add ctx parameter to CaptureScreenshot, InsertText,
   DispatchKeyEvent, DispatchMouseEvent, InjectScript. All now use
   SendCtx internally so tool-level timeouts propagate. Wire ctx
   through executeKeys (was discarding it).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
zhenghuizli 2026-04-19 01:34:38 +08:00
parent 561551ef34
commit d86cd27f28
5 changed files with 101 additions and 58 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

47
pkg/tools/browser.go
View file

@ -7,6 +7,7 @@ import (
"fmt"
"net"
"net/url"
"os"
"strings"
"sync"
"time"
@ -33,11 +34,12 @@ type BrowserTool struct {
cfg config.BrowserToolConfig
cdp *CDPClient
cdpMu sync.Mutex // guards lazy cdp connection and cdp pointer
stateMu sync.Mutex // guards mutable session state: history, mediaStore
stateMu sync.Mutex // guards mutable session state: history, mediaStore, tempFiles
chromePath string
stealthJS string
mediaStore media.MediaStore
history []pageVisit // browsing history, most recent last
tempFiles []string // temp files to clean up on close
}
// NewBrowserTool creates a new BrowserTool. It verifies that Chrome is available
@ -106,7 +108,7 @@ func (t *BrowserTool) connectIfNeeded() error {
// Inject stealth JS if configured
if t.stealthJS != "" {
if err := cdp.InjectScript(t.stealthJS); err != nil {
if err := cdp.InjectScript(context.Background(), t.stealthJS); err != nil {
logger.WarnCF("tool", "Failed to inject stealth JS",
map[string]any{"error": err.Error()})
}
@ -164,13 +166,7 @@ func (t *BrowserTool) Execute(ctx context.Context, args map[string]any) *ToolRes
return ErrorResult("action is required")
}
// Connect lazily on first use
if action != "close" {
if err := t.connectIfNeeded(); err != nil {
return ErrorResult(err.Error())
}
}
// Apply configured timeout before any work so connectIfNeeded also respects it
timeout := time.Duration(t.cfg.Timeout) * time.Second
if timeout <= 0 {
timeout = 30 * time.Second
@ -178,6 +174,13 @@ func (t *BrowserTool) Execute(ctx context.Context, args map[string]any) *ToolRes
ctx, cancel := context.WithTimeout(ctx, timeout)
defer cancel()
// Connect lazily on first use
if action != "close" {
if err := t.connectIfNeeded(); err != nil {
return ErrorResult(err.Error())
}
}
switch action {
case "navigate":
return t.executeNavigate(ctx, args)
@ -215,6 +218,13 @@ func (t *BrowserTool) SetMediaStore(store media.MediaStore) {
t.mediaStore = store
}
// trackTempFile records a temp file for cleanup on close.
func (t *BrowserTool) trackTempFile(path string) {
t.stateMu.Lock()
defer t.stateMu.Unlock()
t.tempFiles = append(t.tempFiles, path)
}
func (t *BrowserTool) executeClose() *ToolResult {
t.cdpMu.Lock()
defer t.cdpMu.Unlock()
@ -226,6 +236,13 @@ func (t *BrowserTool) executeClose() *ToolResult {
t.cdp = nil
}
t.history = nil
// Clean up temp files created by screenshots without MediaStore
for _, f := range t.tempFiles {
os.Remove(f)
}
t.tempFiles = nil
return SilentResult("Browser session closed.")
}
@ -440,7 +457,8 @@ func isNumericHost(host string) bool {
return true
}
// getIntArg extracts an integer from args, handling both float64 (JSON) and int types.
// getIntArg extracts a non-negative integer from args, handling both float64 (JSON) and int types.
// Rejects negative values and non-integer floats (e.g. 1.9).
func getIntArg(args map[string]any, key string) (int, bool) {
v, ok := args[key]
if !ok {
@ -448,10 +466,19 @@ func getIntArg(args map[string]any, key string) (int, bool) {
}
switch n := v.(type) {
case float64:
if n < 0 || n != float64(int(n)) {
return 0, false
}
return int(n), true
case int:
if n < 0 {
return 0, false
}
return n, true
case int64:
if n < 0 {
return 0, false
}
return int(n), true
}
return 0, false

50
pkg/tools/browser_actions.go
View file

@ -233,18 +233,19 @@ func (t *BrowserTool) executeType(ctx context.Context, args map[string]any) *Too
if err := json.Unmarshal(raw, &focusStr); err != nil {
return ErrorResult(fmt.Sprintf("failed to parse focus result: %v", err))
}
if strings.Contains(focusStr, "error") {
var result struct{ Error string `json:"error"` }
if err := json.Unmarshal([]byte(focusStr), &result); err != nil {
return ErrorResult(fmt.Sprintf("failed to parse focus result: %v", err))
}
if result.Error != "" {
return ErrorResult(result.Error)
}
var focusResult struct {
OK bool `json:"ok"`
Error string `json:"error"`
}
if err := json.Unmarshal([]byte(focusStr), &focusResult); err != nil {
return ErrorResult(fmt.Sprintf("failed to parse focus result: %v", err))
}
if focusResult.Error != "" {
return ErrorResult(focusResult.Error)
}
// Type using CDP Input.insertText
if err := t.cdp.InsertText(text); err != nil {
if err := t.cdp.InsertText(ctx, text); err != nil {
return ErrorResult(fmt.Sprintf("type failed: %v", err))
}
@ -281,18 +282,19 @@ func (t *BrowserTool) executeFill(ctx context.Context, args map[string]any) *Too
if err := json.Unmarshal(raw, &clearStr); err != nil {
return ErrorResult(fmt.Sprintf("failed to parse fill result: %v", err))
}
if strings.Contains(clearStr, "error") {
var result struct{ Error string `json:"error"` }
if err := json.Unmarshal([]byte(clearStr), &result); err != nil {
return ErrorResult(fmt.Sprintf("failed to parse fill result: %v", err))
}
if result.Error != "" {
return ErrorResult(result.Error)
}
var clearResult struct {
OK bool `json:"ok"`
Error string `json:"error"`
}
if err := json.Unmarshal([]byte(clearStr), &clearResult); err != nil {
return ErrorResult(fmt.Sprintf("failed to parse fill result: %v", err))
}
if clearResult.Error != "" {
return ErrorResult(clearResult.Error)
}
// Type new value
if err := t.cdp.InsertText(text); err != nil {
if err := t.cdp.InsertText(ctx, text); err != nil {
return ErrorResult(fmt.Sprintf("fill failed (type step): %v", err))
}
@ -353,7 +355,7 @@ func (t *BrowserTool) executeSelect(ctx context.Context, args map[string]any) *T
}
func (t *BrowserTool) executeScreenshot(ctx context.Context) *ToolResult {
data, err := t.cdp.CaptureScreenshot("png", 0)
data, err := t.cdp.CaptureScreenshot(ctx, "png", 0)
if err != nil {
return ErrorResult(fmt.Sprintf("screenshot failed: %v", err))
}
@ -402,7 +404,9 @@ func (t *BrowserTool) executeScreenshot(ctx context.Context) *ToolResult {
}
// No MediaStore or store failed — save to disk and return path as artifact
// so LLM can reference it and user can access it via send_file if needed
// so LLM can reference it and user can access it via send_file if needed.
// Track the temp file for cleanup on close.
t.trackTempFile(tmpFile)
return &ToolResult{
ForLLM: fmt.Sprintf("Screenshot saved to %s (%d KB). Use send_file to deliver to user if needed.", tmpFile, len(pngBytes)/1024),
ArtifactTags: []string{fmt.Sprintf("[file:%s]", tmpFile)},
@ -475,7 +479,7 @@ func (t *BrowserTool) executeScroll(ctx context.Context, args map[string]any) *T
return SilentResult(fmt.Sprintf("Scrolled %s. Run 'state' to see updated elements.", direction))
}
func (t *BrowserTool) executeKeys(_ context.Context, args map[string]any) *ToolResult {
func (t *BrowserTool) executeKeys(ctx context.Context, args map[string]any) *ToolResult {
text, _ := args["text"].(string)
if text == "" {
return ErrorResult("text (key name) is required for keys action. Examples: Enter, Tab, Escape, ArrowDown")
@ -504,10 +508,10 @@ func (t *BrowserTool) executeKeys(_ context.Context, args map[string]any) *ToolR
key = "ArrowRight"
}
if err := t.cdp.DispatchKeyEvent("keyDown", key, 0); err != nil {
if err := t.cdp.DispatchKeyEvent(ctx, "keyDown", key, 0); err != nil {
return ErrorResult(fmt.Sprintf("keyDown failed: %v", err))
}
if err := t.cdp.DispatchKeyEvent("keyUp", key, 0); err != nil {
if err := t.cdp.DispatchKeyEvent(ctx, "keyUp", key, 0); err != nil {
return ErrorResult(fmt.Sprintf("keyUp failed: %v", err))
}

35
pkg/tools/browser_cdp.go
View file

@ -456,22 +456,17 @@ func (c *CDPClient) Navigate(ctx context.Context, targetURL string) error {
return fmt.Errorf("navigation failed: %w", err)
}
// Wait for DOM content loaded with timeout
timer := time.NewTimer(30 * time.Second)
defer timer.Stop()
// Wait for DOM content loaded or context cancellation
select {
case <-domCh:
return nil
case <-timer.C:
return fmt.Errorf("page load timed out after 30s")
case <-ctx.Done():
return ctx.Err()
return fmt.Errorf("page load timed out: %w", ctx.Err())
}
}
// CaptureScreenshot takes a screenshot and returns base64-encoded PNG.
func (c *CDPClient) CaptureScreenshot(format string, quality int) (string, error) {
func (c *CDPClient) CaptureScreenshot(ctx context.Context, format string, quality int) (string, error) {
params := map[string]any{
"format": format,
}
@ -479,7 +474,7 @@ func (c *CDPClient) CaptureScreenshot(format string, quality int) (string, error
params["quality"] = quality
}
result, err := c.Send("Page.captureScreenshot", params)
result, err := c.SendCtx(ctx, "Page.captureScreenshot", params)
if err != nil {
return "", err
}
@ -495,35 +490,35 @@ func (c *CDPClient) CaptureScreenshot(format string, quality int) (string, error
}
// InjectScript injects JavaScript to be evaluated on every new document.
func (c *CDPClient) InjectScript(source string) error {
_, err := c.Send("Page.addScriptToEvaluateOnNewDocument", map[string]any{
func (c *CDPClient) InjectScript(ctx context.Context, source string) error {
_, err := c.SendCtx(ctx, "Page.addScriptToEvaluateOnNewDocument", map[string]any{
"source": source,
})
return err
}
// DispatchMouseEvent sends a mouse event at the given coordinates.
func (c *CDPClient) DispatchMouseEvent(eventType string, x, y float64, button string, clickCount int) error {
_, err := c.Send("Input.dispatchMouseEvent", map[string]any{
func (c *CDPClient) DispatchMouseEvent(ctx context.Context, eventType string, x, y float64, button string, clickCount int) error {
_, err := c.SendCtx(ctx, "Input.dispatchMouseEvent", map[string]any{
"type": eventType,
"x": x,
"y": y,
"button": button,
"x": x,
"y": y,
"button": button,
"clickCount": clickCount,
})
return err
}
// InsertText inserts text at the current cursor position.
func (c *CDPClient) InsertText(text string) error {
_, err := c.Send("Input.insertText", map[string]any{
func (c *CDPClient) InsertText(ctx context.Context, text string) error {
_, err := c.SendCtx(ctx, "Input.insertText", map[string]any{
"text": text,
})
return err
}
// DispatchKeyEvent sends a keyboard event.
func (c *CDPClient) DispatchKeyEvent(eventType, key string, modifiers int) error {
func (c *CDPClient) DispatchKeyEvent(ctx context.Context, eventType, key string, modifiers int) error {
params := map[string]any{
"type": eventType,
"key": key,
@ -531,6 +526,6 @@ func (c *CDPClient) DispatchKeyEvent(eventType, key string, modifiers int) error
if modifiers > 0 {
params["modifiers"] = modifiers
}
_, err := c.Send("Input.dispatchKeyEvent", params)
_, err := c.SendCtx(ctx, "Input.dispatchKeyEvent", params)
return err
}

18
pkg/tools/browser_integration_test.go
View file

@ -227,16 +227,24 @@ func TestIntegration_TempFileCleanup(t *testing.T) {
tool.Execute(ctx, map[string]any{"action": "navigate", "url": "https://example.com"})
// Screenshot without MediaStore — temp file should be cleaned up
// Screenshot without MediaStore — temp files should be cleaned up on close
beforeFiles := countTempScreenshots()
tool.Execute(ctx, map[string]any{"action": "screenshot"})
time.Sleep(100 * time.Millisecond)
afterFiles := countTempScreenshots()
tool.Execute(ctx, map[string]any{"action": "screenshot"})
// Since no MediaStore is set, temp file should be deferred for removal
t.Logf("Temp screenshot files: before=%d, after=%d", beforeFiles, afterFiles)
midFiles := countTempScreenshots()
if midFiles < beforeFiles+2 {
t.Logf("expected at least 2 new temp files, before=%d, mid=%d", beforeFiles, midFiles)
}
// Close should clean up temp files
tool.Execute(ctx, map[string]any{"action": "close"})
time.Sleep(100 * time.Millisecond)
afterFiles := countTempScreenshots()
if afterFiles > beforeFiles {
t.Errorf("temp screenshot files not cleaned up after close: before=%d, after=%d", beforeFiles, afterFiles)
}
}
func countTempScreenshots() int {

9
pkg/tools/browser_test.go
View file

@ -91,6 +91,15 @@ func TestGetIntArg(t *testing.T) {
{map[string]any{"other": float64(1)}, "index", 0, false},
{map[string]any{}, "index", 0, false},
{map[string]any{"index": "not a number"}, "index", 0, false},
// Negative values rejected
{map[string]any{"index": float64(-1)}, "index", 0, false},
{map[string]any{"index": -3}, "index", 0, false},
{map[string]any{"index": int64(-5)}, "index", 0, false},
// Non-integer floats rejected
{map[string]any{"index": float64(1.9)}, "index", 0, false},
{map[string]any{"index": float64(0.5)}, "index", 0, false},
// Zero is valid
{map[string]any{"index": float64(0)}, "index", 0, true},
}
for _, tt := range tests {