CI release-readiness audit

pkg/tools/browser.go

@@ -11,6 +11,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/mafredri/cdp"
 	"github.com/mafredri/cdp/devtool"
 	"github.com/mafredri/cdp/protocol/dom"
 	"github.com/mafredri/cdp/protocol/emulation"
@@ -405,21 +406,46 @@ func (b *BrowserTool) navigate(ctx context.Context, params map[string]interface{
 		return "", fmt.Errorf("failed to navigate: %w", err)
 	}
 
-	// Wait for page load
-	domLoaded, err := client.Page.DOMContentEventFired(ctx)
-	if err != nil {
-		b.log.Warn("Failed to wait for DOMContentLoaded",
+	// Wait for page load using a bounded readyState poll rather than a CDP event
+	// stream. This avoids CI flakes where the event listener can hang indefinitely.
+	if err := waitForPageReadyState(ctx, client, b.timeout); err != nil {
+		b.log.Warn("Failed to wait for readyState after navigation",
 			zap.Error(err))
-	} else {
-		defer func() {
-			_ = domLoaded.Close()
-		}()
-		_, _ = domLoaded.Recv()
 	}
 
 	return fmt.Sprintf("Navigated to: %s\nFrame ID: %s", urlStr, nav.FrameID), nil
 }
 
+func waitForPageReadyState(parent context.Context, client *cdp.Client, timeout time.Duration) error {
+	if timeout <= 0 {
+		timeout = 30 * time.Second
+	}
+	waitCtx, cancel := context.WithTimeout(parent, timeout)
+	defer cancel()
+
+	for {
+		evalArgs := runtime.NewEvaluateArgs("document.readyState").SetReturnByValue(true)
+		result, err := client.Runtime.Evaluate(waitCtx, evalArgs)
+		if err == nil && result.ExceptionDetails == nil {
+			var state string
+			if unmarshalErr := json.Unmarshal(result.Result.Value, &state); unmarshalErr == nil {
+				if state == "interactive" || state == "complete" {
+					return nil
+				}
+			}
+		}
+
+		select {
+		case <-waitCtx.Done():
+			if err != nil {
+				return err
+			}
+			return waitCtx.Err()
+		case <-time.After(100 * time.Millisecond):
+		}
+	}
+}
+
 func (b *BrowserTool) startMode(params map[string]interface{}) (BrowserConnectionMode, error) {
 	opts, err := b.startOptions(params)
 	if err != nil {

pkg/tools/browser_session.go

@@ -46,6 +46,7 @@ type BrowserSession struct {
 	connectFn         func(port int, timeout time.Duration) error
 	connectEndpointFn func(endpoint string, timeout time.Duration) error
 	launchFn          func(timeout time.Duration) error
+	findChromeFn      func() string
 	devtoolsFactory   func(endpoint string) browserDevTools
 }
 
@@ -95,6 +96,7 @@ func GetBrowserSession(log *logger.Logger) *BrowserSession {
 		browserSession.connectFn = browserSession.connect
 		browserSession.connectEndpointFn = browserSession.connectEndpoint
 		browserSession.launchFn = browserSession.launch
+		browserSession.findChromeFn = findChrome
 		browserSession.devtoolsFactory = func(endpoint string) browserDevTools {
 			return devtool.New(endpoint)
 		}
@@ -136,6 +138,9 @@ func (s *BrowserSession) StartWithOptions(timeout time.Duration, opts BrowserSta
 	if s.launchFn == nil {
 		s.launchFn = s.launch
 	}
+	if s.findChromeFn == nil {
+		s.findChromeFn = findChrome
+	}
 	if s.devtoolsFactory == nil {
 		s.devtoolsFactory = func(endpoint string) browserDevTools {
 			return devtool.New(endpoint)
@@ -190,8 +195,10 @@ func (s *BrowserSession) launch(timeout time.Duration) error {
 	s.cancel = cancel
 
 	// Try to find Chrome executable
-	chromePath := findChrome()
+	chromePath := s.findChromeFn()
 	if chromePath == "" {
+		cancel()
+		s.cancel = nil
 		return fmt.Errorf("chrome not found in PATH")
 	}
 
@@ -217,14 +224,12 @@ func (s *BrowserSession) launch(timeout time.Duration) error {
 
 	if err := s.cmd.Start(); err != nil {
 		cancel()
+		s.cancel = nil
 		return fmt.Errorf("failed to start chrome: %w", err)
 	}
 
-	// Wait for Chrome to be ready
-	time.Sleep(2 * time.Second)
-
-	if err := s.connect(9222, timeout); err != nil {
-		if stopErr := s.Stop(); stopErr != nil {
+	if err := s.connectWithRetry(9222, timeout); err != nil {
+		if stopErr := s.stopLocked(); stopErr != nil {
 			s.log.Warn("Failed to stop browser session after launch failure", zap.Error(stopErr))
 		}
 		return err
@@ -237,6 +242,28 @@ func (s *BrowserSession) launch(timeout time.Duration) error {
 	return nil
 }
 
+func (s *BrowserSession) connectWithRetry(port int, timeout time.Duration) error {
+	if timeout <= 0 {
+		timeout = s.timeout
+	}
+	connectFn := s.connectFn
+	if connectFn == nil {
+		connectFn = s.connect
+	}
+	deadline := time.Now().Add(timeout)
+	var lastErr error
+	for {
+		lastErr = connectFn(port, timeout)
+		if lastErr == nil {
+			return nil
+		}
+		if time.Now().After(deadline) {
+			return lastErr
+		}
+		time.Sleep(250 * time.Millisecond)
+	}
+}
+
 func (s *BrowserSession) connect(port int, timeout time.Duration) error {
 	return s.connectEndpoint(fmt.Sprintf("http://127.0.0.1:%d", port), timeout)
 }
@@ -264,12 +291,8 @@ func (s *BrowserSession) connectEndpoint(endpoint string, timeout time.Duration)
 	return nil
 }
 
-// Stop stops the browser session.
-func (s *BrowserSession) Stop() error {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	if !s.ready {
+func (s *BrowserSession) stopLocked() error {
+	if s.conn == nil && s.cancel == nil && s.cmd == nil && !s.ready {
 		return nil
 	}
 
@@ -292,13 +315,21 @@ func (s *BrowserSession) Stop() error {
 	s.client = nil
 	s.conn = nil
 	s.cmd = nil
+	s.cancel = nil
 	s.debugURL = ""
 	s.mode = BrowserModeAuto
 	s.endpoint = ""
 
 	return nil
 }
 
+// Stop stops the browser session.
+func (s *BrowserSession) Stop() error {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	return s.stopLocked()
+}
+
 // IsReady returns whether the session is ready.
 func (s *BrowserSession) IsReady() bool {
 	s.mu.RLock()

pkg/tools/browser_session_test.go

@@ -2,6 +2,8 @@ package tools
 
 import (
 	"context"
+	"strings"
+	"os/exec"
 	"errors"
 	"testing"
 	"time"
@@ -308,3 +310,69 @@ func TestBrowserSessionGetDevToolsReturnsFactoryForEndpoint(t *testing.T) {
 		t.Fatalf("expected devtools factory to be called once, got %d", called)
 	}
 }
+
+func TestBrowserSessionStartWithModeAutoCleansUpAfterLaunchFailure(t *testing.T) {
+	session := &BrowserSession{timeout: 5 * time.Second, log: newToolsTestLogger(t)}
+	session.connectFn = func(port int, timeout time.Duration) error {
+		return errors.New("not running")
+	}
+	cmd := exec.Command("bash", "-lc", "sleep 30")
+	session.launchFn = func(timeout time.Duration) error {
+		_, cancel := context.WithTimeout(context.Background(), timeout)
+		session.cancel = cancel
+		session.cmd = cmd
+		if err := session.cmd.Start(); err != nil {
+			return err
+		}
+		if err := session.stopLocked(); err != nil {
+			return err
+		}
+		return errors.New("failed to connect after launch")
+	}
+
+	err := session.StartWithMode(2*time.Second, BrowserModeAuto)
+	if err == nil {
+		t.Fatal("expected launch failure")
+	}
+	if session.cmd != nil || session.cancel != nil || session.conn != nil || session.client != nil || session.ready {
+		t.Fatalf("expected failed launch cleanup, got %+v", session.Status())
+	}
+}
+
+func TestBrowserSessionConnectWithRetryEventuallySucceeds(t *testing.T) {
+	session := &BrowserSession{timeout: 2 * time.Second, log: newToolsTestLogger(t)}
+	attempts := 0
+	session.connectFn = func(port int, timeout time.Duration) error {
+		attempts++
+		if attempts < 3 {
+			return errors.New("not ready")
+		}
+		session.ready = true
+		return nil
+	}
+
+	err := session.connectWithRetry(9222, 500*time.Millisecond)
+	if err != nil {
+		t.Fatalf("connectWithRetry returned error: %v", err)
+	}
+	if attempts != 3 {
+		t.Fatalf("expected 3 attempts before success, got %d", attempts)
+	}
+}
+
+func TestBrowserSessionLaunchWithoutChromeCleansUpCancel(t *testing.T) {
+	session := &BrowserSession{timeout: 5 * time.Second, log: newToolsTestLogger(t)}
+	session.findChromeFn = func() string { return "" }
+
+	err := session.launch(2 * time.Second)
+	if err == nil {
+		t.Fatal("expected chrome-not-found failure")
+	}
+	if !strings.Contains(err.Error(), "chrome not found in PATH") {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if session.cancel != nil || session.cmd != nil || session.conn != nil || session.client != nil || session.ready {
+		t.Fatalf("expected launch failure without chrome to leave no session state, got ready=%v cmd=%v cancel=%v conn=%v client=%v",
+			session.ready, session.cmd, session.cancel, session.conn, session.client)
+	}
+}

pkg/webui/server_goalruns_test.go

@@ -647,17 +647,15 @@ func TestGoalRunBrowserToolManualFlowE2E(t *testing.T) {
 
 	steps := []map[string]interface{}{
 		{"action": "execute_script", "script": `Array.from(document.querySelectorAll('button')).find(btn => btn.textContent?.includes('New goal run'))?.click(); true;`},
-		{"action": "type", "selector": "#goal-run-name", "text": "Browser tool e2e"},
-		{"action": "type", "selector": "#goal-run-goal", "text": "Verify browser-tool Goal Runs flow"},
-		{"action": "type", "selector": "#goal-run-criteria", "text": "confirm manually"},
+		{"action": "execute_script", "script": `(async function waitForDialog(deadline){ while (Date.now() < deadline) { if (document.querySelector('[role=\"dialog\"]')) { return true; } await new Promise(resolve => setTimeout(resolve, 50)); } throw new Error('goal run dialog did not render in time'); })(Date.now() + 4000);`},
+		{"action": "execute_script", "script": `(async function(){ const waitFor = async (selector) => { const deadline = Date.now() + 4000; while (Date.now() < deadline) { const el = document.querySelector(selector); if (el) return el; await new Promise(resolve => setTimeout(resolve, 50)); } throw new Error('missing ' + selector); }; const set = async (selector, value) => { const el = await waitFor(selector); el.value = value; el.dispatchEvent(new Event('input', { bubbles: true })); }; await set('#goal-run-name', 'Browser tool e2e'); await set('#goal-run-goal', 'Verify browser-tool Goal Runs flow'); await set('#goal-run-criteria', 'confirm manually'); return true; })();`},
 		{"action": "execute_script", "script": `Array.from(document.querySelectorAll('button')).find(btn => btn.textContent?.includes('Create run'))?.click(); true;`},
 		{"action": "wait", "duration": float64(1200)},
 		{"action": "execute_script", "script": `Array.from(document.querySelectorAll('button')).find(btn => btn.textContent?.includes('Confirm criteria'))?.click(); true;`},
 		{"action": "wait", "duration": float64(600)},
 		{"action": "execute_script", "script": `Array.from(document.querySelectorAll('button')).find(btn => btn.textContent?.includes('Start run'))?.click(); true;`},
 		{"action": "wait", "duration": float64(1200)},
-		{"action": "execute_script", "script": `const areas = Array.from(document.querySelectorAll('textarea')); if (areas.length > 0) { areas[areas.length - 1].focus(); } true;`},
-		{"action": "type", "selector": "textarea", "text": "browser tool e2e confirmed"},
+		{"action": "execute_script", "script": `(function(){ const areas = Array.from(document.querySelectorAll('textarea')); if (areas.length === 0) throw new Error('missing textarea'); const el = areas[areas.length - 1]; el.value = 'browser tool e2e confirmed'; el.dispatchEvent(new Event('input', { bubbles: true })); return true; })();`},
 		{"action": "execute_script", "script": `Array.from(document.querySelectorAll('button')).find(btn => btn.textContent?.includes('Approve criterion'))?.click(); true;`},
 		{"action": "wait", "duration": float64(1200)},
 	}