fix: insight generation auth failure and test isolation

.roborev.toml

@@ -1,6 +1,4 @@
-[review_guidelines]
-
-threat_model = """
+review_guidelines = """
 agentsview is a LOCAL-ONLY developer tool. It binds to 127.0.0.1
 by default and is not designed for multi-user or public deployment.
 

frontend/src/lib/components/insights/InsightsPage.svelte

@@ -315,6 +315,7 @@
                 <span class="task-phase">{task.phase}</span>
               {/if}
             </div>
+            <span class="task-agent">{task.agent}</span>
             <button
               class="task-dismiss"
               onclick={() => task.status === "error"
@@ -802,6 +803,15 @@
     text-overflow: ellipsis;
   }
 
+  .task-agent {
+    flex-shrink: 0;
+    font-size: 10px;
+    color: var(--text-muted);
+    font-family: var(--font-mono);
+    letter-spacing: -0.02em;
+    white-space: nowrap;
+  }
+
   .task-dismiss {
     flex-shrink: 0;
     width: 18px;

internal/insight/generate.go

@@ -7,6 +7,7 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"os"
 	"os/exec"
 	"strings"
 )
@@ -25,6 +26,12 @@ var ValidAgents = map[string]bool{
 	"gemini": true,
 }
 
+// GenerateFunc is the signature for insight generation,
+// allowing tests to substitute a stub.
+type GenerateFunc func(
+	ctx context.Context, agent, prompt string,
+) (Result, error)
+
 // Generate invokes an AI agent CLI to generate an insight.
 // The agent parameter selects which CLI to use (claude,
 // codex, gemini). The prompt is passed via stdin.
@@ -54,6 +61,60 @@ func Generate(
 	}
 }
 
+// allowedKeyPrefixes lists uppercase key prefixes that are
+// safe to pass to agent CLI subprocesses. Matched
+// case-insensitively so Windows-style casing (Path, ComSpec)
+// is handled correctly. Using an allowlist prevents leaking
+// secrets to child processes.
+var allowedKeyPrefixes = []string{
+	"PATH",
+	"HOME", "USERPROFILE",
+	"USER", "USERNAME", "LOGNAME",
+	"LANG", "LC_",
+	"TERM", "COLORTERM",
+	"TMPDIR", "TEMP", "TMP",
+	"XDG_",
+	"SHELL",
+	"SSL_CERT_", "CURL_CA_BUNDLE",
+	"HTTP_PROXY", "HTTPS_PROXY", "NO_PROXY",
+	"SYSTEMROOT", "COMSPEC", "PATHEXT", "WINDIR",
+	"HOMEDRIVE", "HOMEPATH",
+	"APPDATA", "LOCALAPPDATA", "PROGRAMDATA",
+}
+
+// envKeyAllowed reports whether key (case-insensitive) is
+// on the allowlist. Prefix entries ending with _ (LC_,
+// XDG_, SSL_CERT_) match any key starting with that prefix;
+// all others require an exact match.
+func envKeyAllowed(key string) bool {
+	upper := strings.ToUpper(key)
+	for _, p := range allowedKeyPrefixes {
+		if strings.HasSuffix(p, "_") {
+			if strings.HasPrefix(upper, p) {
+				return true
+			}
+		} else if upper == p {
+			return true
+		}
+	}
+	return false
+}
+
+// cleanEnv returns an allowlisted subset of the current
+// environment for agent CLI subprocesses, plus
+// CLAUDE_NO_SOUND=1.
+func cleanEnv() []string {
+	env := os.Environ()
+	filtered := make([]string, 0, len(env))
+	for _, e := range env {
+		k, _, _ := strings.Cut(e, "=")
+		if envKeyAllowed(k) {
+			filtered = append(filtered, e)
+		}
+	}
+	return append(filtered, "CLAUDE_NO_SOUND=1")
+}
+
 // generateClaude invokes `claude -p --output-format json`.
 func generateClaude(
 	ctx context.Context, path, prompt string,

internal/insight/generate_test.go

@@ -158,6 +158,93 @@ func TestParseStreamJSON_Empty(t *testing.T) {
 	}
 }
 
+func TestCleanEnv(t *testing.T) {
+	t.Setenv("ANTHROPIC_API_KEY", "sk-secret")
+	t.Setenv("CLAUDECODE", "1")
+	t.Setenv("HOME", "/home/test")
+	t.Setenv("AWS_SECRET_ACCESS_KEY", "s3cret")
+	t.Setenv("PATH", "/usr/bin")
+	t.Setenv("LANG", "en_US.UTF-8")
+	t.Setenv("UNKNOWN_VAR", "should-be-dropped")
+
+	env := cleanEnv()
+
+	// Normalize keys to uppercase for cross-platform assertions
+	// (Windows may return Path instead of PATH).
+	envMap := make(map[string]string, len(env))
+	for _, e := range env {
+		k, v, _ := strings.Cut(e, "=")
+		envMap[strings.ToUpper(k)] = v
+	}
+
+	// Secrets and unknown vars must not pass through.
+	for _, blocked := range []string{
+		"ANTHROPIC_API_KEY", "CLAUDECODE",
+		"AWS_SECRET_ACCESS_KEY", "UNKNOWN_VAR",
+	} {
+		if _, ok := envMap[blocked]; ok {
+			t.Errorf("%s should not be in env", blocked)
+		}
+	}
+
+	// Allowed system vars must pass through.
+	for _, allowed := range []string{
+		"HOME", "PATH", "LANG",
+	} {
+		if _, ok := envMap[allowed]; !ok {
+			t.Errorf("%s should be preserved", allowed)
+		}
+	}
+
+	if v, ok := envMap["CLAUDE_NO_SOUND"]; !ok || v != "1" {
+		t.Errorf(
+			"CLAUDE_NO_SOUND should be 1, got %q", v,
+		)
+	}
+}
+
+func TestEnvKeyAllowed(t *testing.T) {
+	tests := []struct {
+		key  string
+		want bool
+	}{
+		{"PATH", true},
+		{"Path", true}, // Windows-style
+		{"path", true}, // lowercase
+		{"HOME", true},
+		{"Home", true},
+		{"COMSPEC", true},
+		{"ComSpec", true}, // Windows-style
+		{"LC_ALL", true},  // prefix match
+		{"XDG_CONFIG_HOME", true},
+		{"SSL_CERT_FILE", true},
+		{"HTTP_PROXY", true},
+		{"APPDATA", true},
+		{"AppData", true},
+		{"LOCALAPPDATA", true},
+		{"PROGRAMDATA", true},
+		{"PATHEXT", true},
+		{"PathExt", true},
+		{"WINDIR", true},
+		{"HOMEDRIVE", true},
+		{"HOMEPATH", true},
+		{"ANTHROPIC_API_KEY", false},
+		{"AWS_SECRET_ACCESS_KEY", false},
+		{"DATABASE_URL", false},
+		{"", false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.key, func(t *testing.T) {
+			if got := envKeyAllowed(tt.key); got != tt.want {
+				t.Errorf(
+					"envKeyAllowed(%q) = %v, want %v",
+					tt.key, got, tt.want,
+				)
+			}
+		})
+	}
+}
+
 func TestValidAgents(t *testing.T) {
 	for _, agent := range []string{
 		"claude", "codex", "gemini",

internal/server/deadline_internal_test.go

@@ -13,7 +13,6 @@ func TestHandlers_Internal_DeadlineExceeded(t *testing.T) {
 	s := testServer(t, 30*time.Second)
 
 	// Seed a session just in case handlers check for existence before context.
-	// We'll use the public methods on db to seed.
 	started := "2025-01-15T10:00:00Z"
 	sess := db.Session{
 		ID:        "s1",
@@ -52,7 +51,8 @@ func TestHandlers_Internal_DeadlineExceeded(t *testing.T) {
 
 			w := httptest.NewRecorder()
 
-			// Call handler directly, bypassing middleware
+			// Call handler directly, bypassing middleware.
+			// handleContextError writes 504 for deadline exceeded.
 			tt.handler(w, req)
 
 			assertRecorderStatus(t, w, http.StatusGatewayTimeout)

internal/server/deadline_test.go

@@ -1,7 +1,6 @@
 package server_test
 
 import (
-	"net/http"
 	"net/http/httptest"
 	"testing"
 )
@@ -35,8 +34,7 @@ func TestMiddleware_Timeout(t *testing.T) {
 			w := httptest.NewRecorder()
 			te.handler.ServeHTTP(w, req)
 
-			assertTimeoutResponse(t, w,
-				http.StatusServiceUnavailable, "request timed out")
+			assertTimeoutRace(t, w)
 		})
 	}
 }

internal/server/insights.go

@@ -202,7 +202,7 @@ func (s *Server) handleGenerateInsight(
 	)
 	defer cancel()
 
-	result, err := insight.Generate(
+	result, err := s.generateFunc(
 		genCtx, req.Agent, prompt,
 	)
 	if err != nil {

internal/server/insights_test.go

@@ -1,11 +1,14 @@
 package server_test
 
 import (
+	"context"
 	"fmt"
 	"net/http"
 	"testing"
 
 	"github.com/wesm/agentsview/internal/db"
+	"github.com/wesm/agentsview/internal/insight"
+	"github.com/wesm/agentsview/internal/server"
 )
 
 func TestListInsights_Empty(t *testing.T) {
@@ -181,7 +184,17 @@ func TestGenerateInsight_InvalidAgent(t *testing.T) {
 }
 
 func TestGenerateInsight_DefaultAgent(t *testing.T) {
-	te := setup(t)
+	stubGen := func(
+		_ context.Context, agent, _ string,
+	) (insight.Result, error) {
+		if agent != "claude" {
+			t.Errorf("expected default agent claude, got %q", agent)
+		}
+		return insight.Result{}, fmt.Errorf("stub: no CLI")
+	}
+	te := setupWithServerOpts(t, []server.Option{
+		server.WithGenerateFunc(stubGen),
+	})
 
 	w := te.post(t, "/api/v1/insights/generate",
 		`{"type":"daily_activity","date_from":"2025-01-15","date_to":"2025-01-15"}`)

internal/server/response.go

@@ -24,15 +24,20 @@ func writeError(w http.ResponseWriter, status int, msg string) {
 	writeJSON(w, status, map[string]string{"error": msg})
 }
 
-// handleContextError handles context.Canceled and context.DeadlineExceeded
-// errors by writing the appropriate HTTP response. Returns true if the
-// error was handled (i.e., it was a context error), false otherwise.
+// handleContextError checks for context.Canceled and
+// context.DeadlineExceeded. On cancellation it returns true
+// silently (client disconnected). On deadline exceeded it
+// writes a 504 and returns true. Behind withTimeout the 504
+// goes into the TimeoutHandler buffer and is discarded if
+// the middleware fires first.
 func handleContextError(w http.ResponseWriter, err error) bool {
 	if errors.Is(err, context.Canceled) {
 		return true
 	}
 	if errors.Is(err, context.DeadlineExceeded) {
-		writeError(w, http.StatusGatewayTimeout, "gateway timeout")
+		writeError(
+			w, http.StatusGatewayTimeout, "gateway timeout",
+		)
 		return true
 	}
 	return false

internal/server/search.go

@@ -1,8 +1,6 @@
 package server
 
 import (
-	"context"
-	"errors"
 	"net/http"
 	"strings"
 
@@ -63,11 +61,7 @@ func (s *Server) handleSearch(
 
 	page, err := s.db.Search(r.Context(), filter)
 	if err != nil {
-		if errors.Is(err, context.Canceled) {
-			return
-		}
-		if errors.Is(err, context.DeadlineExceeded) {
-			writeError(w, http.StatusGatewayTimeout, "search timed out")
+		if handleContextError(w, err) {
 			return
 		}
 		writeError(w, http.StatusInternalServerError, err.Error())

internal/server/server.go

@@ -14,6 +14,7 @@ import (
 
 	"github.com/wesm/agentsview/internal/config"
 	"github.com/wesm/agentsview/internal/db"
+	"github.com/wesm/agentsview/internal/insight"
 	"github.com/wesm/agentsview/internal/sync"
 	"github.com/wesm/agentsview/internal/web"
 )
@@ -35,8 +36,9 @@ type Server struct {
 	httpSrv *http.Server
 	version VersionInfo
 
-	spaFS      fs.FS
-	spaHandler http.Handler
+	generateFunc insight.GenerateFunc
+	spaFS        fs.FS
+	spaHandler   http.Handler
 }
 
 // New creates a new Server.
@@ -50,12 +52,13 @@ func New(
 	}
 
 	s := &Server{
-		cfg:        cfg,
-		db:         database,
-		engine:     engine,
-		mux:        http.NewServeMux(),
-		spaFS:      dist,
-		spaHandler: http.FileServerFS(dist),
+		cfg:          cfg,
+		db:           database,
+		engine:       engine,
+		mux:          http.NewServeMux(),
+		generateFunc: insight.Generate,
+		spaFS:        dist,
+		spaHandler:   http.FileServerFS(dist),
 	}
 	for _, opt := range opts {
 		opt(s)
@@ -72,6 +75,16 @@ func WithVersion(v VersionInfo) Option {
 	return func(s *Server) { s.version = v }
 }
 
+// WithGenerateFunc overrides the insight generation function,
+// allowing tests to substitute a stub. Nil is ignored.
+func WithGenerateFunc(f insight.GenerateFunc) Option {
+	return func(s *Server) {
+		if f != nil {
+			s.generateFunc = f
+		}
+	}
+}
+
 func (s *Server) routes() {
 	// API v1 routes
 	s.mux.Handle("GET /api/v1/sessions", s.withTimeout(s.handleListSessions))