diff --git a/pkg/tools/browser.go b/pkg/tools/browser.go index b75401aed..4c6b85a6e 100644 --- a/pkg/tools/browser.go +++ b/pkg/tools/browser.go @@ -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 diff --git a/pkg/tools/browser_actions.go b/pkg/tools/browser_actions.go index 267b99bc8..65746a79f 100644 --- a/pkg/tools/browser_actions.go +++ b/pkg/tools/browser_actions.go @@ -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)) } diff --git a/pkg/tools/browser_cdp.go b/pkg/tools/browser_cdp.go index 612353b63..a2478ef21 100644 --- a/pkg/tools/browser_cdp.go +++ b/pkg/tools/browser_cdp.go @@ -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 } diff --git a/pkg/tools/browser_integration_test.go b/pkg/tools/browser_integration_test.go index 8a736b010..d4b7a9d1b 100644 --- a/pkg/tools/browser_integration_test.go +++ b/pkg/tools/browser_integration_test.go @@ -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 { diff --git a/pkg/tools/browser_test.go b/pkg/tools/browser_test.go index de4454912..cd2094600 100644 --- a/pkg/tools/browser_test.go +++ b/pkg/tools/browser_test.go @@ -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 {