fix: 413b hygiene — eight defects in AI paths

Makefile

@@ -68,7 +68,7 @@ coverage: ## Generate test coverage report
 .PHONY: getlint
 getlint: ## Install golangci-lint if not already installed
 	@echo "Checking for golangci-lint..."
-	$(BIN_DIR)/golangci-lint >/dev/null 2>&1 || (echo "Installing golangci-lint..." && go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION))
+	@which golangci-lint >/dev/null 2>&1 || (echo "Installing golangci-lint..." && go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION))
 
 .PHONY: lint
 lint: getlint ## Run golangci-lint

README.md

@@ -238,8 +238,6 @@ Press `h` to toggle the help overlay inside srepd.
 | `ctrl+x ?` | Show chord help | | |
 | `Tab`/`Shift+Tab`/`←`/`→` | Switch tabs (incident view) | `↑`/`↓` | Scroll within tab |
 
-**Mouse:** Scroll wheel works in the incident table and detail views. To select text for copying, hold `Shift` while clicking and dragging.
-
 Chord commands use a configurable prefix (default `ctrl+x`) followed by a second key. Set `chord_prefix` in config to change.
 
 ### rosa-boundary Support

docs/plans/417-413b-hygiene.md

@@ -0,0 +1,43 @@
+# 417 — 413b Hygiene PR
+
+## Problem
+
+After plan 413 merged (commit `bccb0fb`), eight defects were identified
+in the AI paths through manual audit. Two are safety-critical (B1, B4),
+and the remainder are correctness/robustness issues that would surface
+under normal usage.
+
+## Defects and Fixes
+
+| Bug | Summary | Files Changed |
+|-----|---------|---------------|
+| B1 | Approval writes to wrong incident — `buildAskFromVerdict` captures live `m.selectedIncident`; user can switch incidents between creation and acceptance | `model.go`, `approvals.go`, `commands.go`, `ask_wiring_test.go` |
+| B4 | Terminal injection via AI output — ANSI CSI, OSC-52 clipboard writes, and C0 control chars pass through to the terminal | `watcher.go`, `watcher_test.go` |
+| B3 | `readAgentSessionCmd` drops final Result ~50% of the time — single `select` randomly picks `Done()` over `Events()` when both are ready | `claude.go`, `claude_test.go` |
+| B2+B8 | Spawn deadlock + pipe leak — detection `select` has no timeout/ctx case; retry-as-resume leaks stdin/stdout pipes | `session.go`, `session_test.go` |
+| B5 | Stale stream clobbers successor — Done/chunk messages carry no channel identity, so a superseded stream nils the new stream's cancel func | `stream.go`, `claude.go`, `tui.go`, `model.go`, `claude_test.go`, `watcher_integration_test.go` |
+| B6 | No in-flight guard on `:agent` — submitting while a query is active issues concurrent readers on the session channel | `claude.go`, `claude_test.go` |
+| B7 | Untested security gates — `ClaudeArgs`, `ValidateUserFlags`, `extractToolRunnerFactory`, `askKindLabel` had no direct unit tests | `session_test.go`, `investigation_test.go`, `approvals_test.go` |
+| B9 | Bedrock region not validated — `newBedrockProvider` doesn't check for a discoverable region, unlike Vertex | `bedrock.go`, `bedrock_test.go`, `factory_test.go`, `provider_test.go` |
+
+## Cleanup
+
+- `index.load`: warning said "truncated" (wrong), logged raw payload (no customer data in logs) — now says "ignored" and logs only byte length
+- UTF-8 truncation: four call sites used byte-level `s[:N]` which splits multi-byte runes — now rune-aware
+- `inferAskKind`: `"oc "` substring false-positived on `"doc "`, `"adhoc "` — now requires word boundary
+- Deleted write-only `Session.err` field and dead `LastUsed` from session index entry
+- Added `TODO(phase-2)` comment on `PermissionAsk` handler
+
+## Approach
+
+- B1 and B4 committed first (safety items)
+- TDD: failing test committed before each fix where possible; revert checks for B1, B4, B3, B7
+- Each fix is a separate commit with traceability to the bug ID
+
+## Lessons (for 413)
+
+Closures over live model state in deferred-action patterns (approvals,
+typed commands) are a recurring source of identity races. Snapshot the
+identity at the point of creation, not at the point of execution. The
+same principle applies to stream messages — every message must carry
+enough identity to be routed correctly even when superseded.

pkg/agent/agent.go

@@ -258,7 +258,8 @@ func summarizeToolInput(input json.RawMessage) string {
 	}
 	s := string(input)
 	if len(s) > 100 {
-		return s[:100] + "..."
+		truncated := string([]rune(s)[:100])
+		return truncated + "..."
 	}
 	return s
 }

pkg/agent/index.go

@@ -16,7 +16,6 @@ type sessionEntry struct {
 	IncidentID string    `json:"incident_id"`
 	SessionID  string    `json:"session_id"`
 	Created    time.Time `json:"created"`
-	LastUsed   time.Time `json:"last_used"`
 }
 
 type sessionIndex struct {
@@ -79,9 +78,9 @@ func (idx *sessionIndex) load() {
 
 	if lastLine != nil {
 		charlog.Warn("agent.index.load",
-			"msg", "corrupt trailing line truncated",
+			"msg", "corrupt trailing line ignored",
 			"line", lineNum,
-			"content", string(lastLine))
+			"len", len(lastLine))
 	}
 }
 
@@ -122,7 +121,6 @@ func (idx *sessionIndex) record(incidentID string, sessionID uuid.UUID) {
 		IncidentID: incidentID,
 		SessionID:  sessionID.String(),
 		Created:    time.Now(),
-		LastUsed:   time.Now(),
 	}
 	data, err := json.Marshal(entry)
 	if err != nil {

pkg/agent/session.go

@@ -174,7 +174,6 @@ type Session struct {
 	done         chan struct{}
 	doneOnce     sync.Once
 	closed       bool
-	err          error
 
 	useStreamEvents bool
 
@@ -207,6 +206,13 @@ func (s *Session) Done() <-chan struct{} {
 	return s.done
 }
 
+// SetTestChannels replaces the event and done channels for testing.
+// Only for use in tests — the session must not be spawned.
+func SetTestChannels(s *Session, events chan Event, done chan struct{}) {
+	s.events = events
+	s.done = done
+}
+
 // Send writes a user turn to the session's stdin. On the first call it
 // spawns (or resumes) the Claude Code process.
 func (s *Session) Send(ctx context.Context, text string) error {
@@ -295,6 +301,10 @@ func (s *Session) spawn(ctx context.Context) error {
 	// On success the child writes system/init to stdout immediately.
 	// On duplicate-ID rejection the child exits non-zero with no stdout.
 	// This is event-driven: no timer, no delay on the happy path.
+	//
+	// The select includes ctx.Done so a hung child (auth prompt, stuck MCP
+	// server) cannot block spawn indefinitely. Without this, Close/CloseAll
+	// deadlocks because Send holds s.mu while spawn blocks.
 	if !s.resumed {
 		exitCh := make(chan error, 1)
 		go func() { exitCh <- wait() }()
@@ -306,19 +316,29 @@ func (s *Session) spawn(ctx context.Context) error {
 			peekResult <- err
 		}()
 
+		spawnDetectTimeout := 30 * time.Second
+		timer := time.NewTimer(spawnDetectTimeout)
+		defer timer.Stop()
+
+		retryAsResume := func() error {
+			_ = stdin.Close()
+			_ = stdout.Close()
+			cancel()
+			log.Info("agent.session.spawn",
+				"msg", "session ID already in use, retrying with --resume",
+				"session_id", s.id.String())
+			s.resumed = true
+			return s.spawn(ctx)
+		}
+
 		select {
 		case exitErr := <-exitCh:
 			if exitErr != nil {
-				_ = stdout.Close() // unblock peek goroutine; real exec.Wait already closes pipes
+				_ = stdout.Close()
 				<-peekResult
 				if stderrBuf != nil &&
 					strings.Contains(stderrBuf.String(), "already in use") {
-					cancel()
-					log.Info("agent.session.spawn",
-						"msg", "session ID already in use, retrying with --resume",
-						"session_id", s.id.String())
-					s.resumed = true
-					return s.spawn(ctx)
+					return retryAsResume()
 				}
 			} else {
 				if peekErr := <-peekResult; peekErr == nil {
@@ -331,17 +351,22 @@ func (s *Session) spawn(ctx context.Context) error {
 				exitErr := <-exitCh
 				if exitErr != nil && stderrBuf != nil &&
 					strings.Contains(stderrBuf.String(), "already in use") {
-					cancel()
-					log.Info("agent.session.spawn",
-						"msg", "session ID already in use, retrying with --resume",
-						"session_id", s.id.String())
-					s.resumed = true
-					return s.spawn(ctx)
+					return retryAsResume()
 				}
 				exitCh <- exitErr
 			} else {
 				stdout = &prefixedReadCloser{prefix: peekBuf[:1], inner: stdout}
 			}
+		case <-ctx.Done():
+			_ = stdin.Close()
+			_ = stdout.Close()
+			cancel()
+			return fmt.Errorf("spawn: context cancelled while waiting for child: %w", ctx.Err())
+		case <-timer.C:
+			_ = stdin.Close()
+			_ = stdout.Close()
+			cancel()
+			return fmt.Errorf("spawn: child produced no output within %s", spawnDetectTimeout)
 		}
 
 		waitFn = func() error { return <-exitCh }
@@ -387,7 +412,6 @@ func (s *Session) readLoop(stdout io.ReadCloser, wait func() error) {
 		if err := wait(); err != nil {
 			s.mu.Lock()
 			deliberate := s.closing
-			s.err = err
 			s.mu.Unlock()
 			if !deliberate {
 				select {

pkg/agent/session_test.go

@@ -1063,3 +1063,114 @@ func TestSessionManager_CloseAllReapsChildren(t *testing.T) {
 		}
 	}
 }
+
+func TestSpawn_HungChildReturnsWithinTimeout(t *testing.T) {
+	// A child that neither prints nor exits should not block forever.
+	// The spawn detection select must have a timeout/ctx.Done case.
+	executor := &callbackExecutor{
+		startFn: func(ctx context.Context, _ string, _ []string, _ []string) (io.WriteCloser, io.ReadCloser, *bytes.Buffer, func() error, error) {
+			stdoutR, stdoutW := io.Pipe()
+			go func() {
+				<-ctx.Done()
+				_ = stdoutW.Close()
+			}()
+			return &mockStdin{}, stdoutR, &bytes.Buffer{}, func() error {
+				<-ctx.Done()
+				return fmt.Errorf("signal: killed")
+			}, nil
+		},
+	}
+
+	cfg := Config{CLICommand: "claude", SessionEnabled: true}
+	s := NewSession(cfg, "INC-001", executor, nil)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+	defer cancel()
+
+	err := s.Send(ctx, "hello")
+	require.Error(t, err, "spawn must return an error when child neither prints nor exits")
+	assert.Contains(t, err.Error(), "spawn",
+		"error should mention spawn")
+
+	// Close must not block
+	done := make(chan struct{})
+	go func() {
+		_ = s.Close()
+		close(done)
+	}()
+	select {
+	case <-done:
+	case <-time.After(2 * time.Second):
+		t.Fatal("Close() blocked after hung spawn — deadlock")
+	}
+}
+
+func TestClaudeArgs(t *testing.T) {
+	tests := []struct {
+		name   string
+		fields []string
+		want   []string
+	}{
+		{"bare claude", []string{"claude", "--print"}, []string{"--print"}},
+		{"absolute path", []string{"/usr/bin/claude", "--model", "opus"}, []string{"--model", "opus"}},
+		{"toolbox wrapper", []string{"toolbox", "run", "-c", "devtools", "claude", "--print"}, []string{"--print"}},
+		{"flatpak-spawn wrapper", []string{"flatpak-spawn", "--host", "claude", "--verbose"}, []string{"--verbose"}},
+		{"backward scan anchors on last claude", []string{"toolbox", "run", "claude", "--bare", "/usr/bin/claude"}, []string{}},
+		{"/usr/bin/claude as value does not trick backward scan",
+			[]string{"toolbox", "run", "claude", "--model", "opus"},
+			[]string{"--model", "opus"}},
+		{"no claude token falls back to fields[1:]", []string{"my-agent", "--flag"}, []string{"--flag"}},
+		{"single element returns nil", []string{"my-agent"}, nil},
+		{"empty returns nil", []string{}, nil},
+		{"claude with no args", []string{"claude"}, []string{}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := ClaudeArgs(tt.fields)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
+func TestValidateUserFlags(t *testing.T) {
+	tests := []struct {
+		name    string
+		tokens  []string
+		wantErr string
+	}{
+		{"nil tokens", nil, ""},
+		{"empty tokens", []string{}, ""},
+		{"allowed flags", []string{"--model", "opus", "--verbose", "--print"}, ""},
+		{"--bare denied", []string{"--bare"}, "--bare"},
+		{"--bare=true denied", []string{"--bare=true"}, "--bare"},
+		{"--dangerously-skip-permissions denied", []string{"--dangerously-skip-permissions"}, "--dangerously-skip-permissions"},
+		{"--permission-mode denied", []string{"--permission-mode", "bypassPermissions"}, "--permission-mode"},
+		{"--allowedTools denied", []string{"--allowedTools", "Bash"}, "--allowedTools"},
+		{"--disallowedTools denied", []string{"--disallowedTools", "Read"}, "--disallowedTools"},
+		{"--session-id denied", []string{"--session-id", "abc"}, "--session-id"},
+		{"--session-id=abc denied", []string{"--session-id=abc"}, "--session-id"},
+		{"--resume denied", []string{"--resume", "id"}, "--resume"},
+		{"-r short alias denied", []string{"-r", "id"}, "-r"},
+		{"--continue denied", []string{"--continue"}, "--continue"},
+		{"-c short alias denied", []string{"-c"}, "-c"},
+		{"--fork-session denied", []string{"--fork-session"}, "--fork-session"},
+		{"--input-format denied", []string{"--input-format", "text"}, "--input-format"},
+		{"--output-format denied", []string{"--output-format", "json"}, "--output-format"},
+		{"denied flag mid-args", []string{"--model", "opus", "--bare", "--verbose"}, "--bare"},
+		{"--flag=value form", []string{"--output-format=text"}, "--output-format"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := ValidateUserFlags(tt.tokens)
+			if tt.wantErr == "" {
+				assert.NoError(t, err)
+			} else {
+				assert.Error(t, err)
+				assert.Contains(t, err.Error(), tt.wantErr)
+				assert.Contains(t, err.Error(), "denied flag")
+			}
+		})
+	}
+}

pkg/ai/bedrock.go

@@ -3,6 +3,7 @@ package ai
 import (
 	"context"
 	"fmt"
+	"os"
 
 	"github.com/anthropics/anthropic-sdk-go"
 	"github.com/anthropics/anthropic-sdk-go/bedrock"
@@ -16,7 +17,25 @@ import (
 // foundation-model ID cannot be invoked directly. See docs/llm-providers.md.
 const bedrockDefaultModel = "us.anthropic.claude-sonnet-4-6"
 
+func resolveBedrockRegion(cfg Config) string {
+	if cfg.Region != "" {
+		return cfg.Region
+	}
+	for _, env := range []string{"AWS_REGION", "AWS_DEFAULT_REGION"} {
+		if v := os.Getenv(env); v != "" {
+			log.Debug("ai.bedrock", "msg", "region from env", "env", env, "region", v)
+			return v
+		}
+	}
+	return ""
+}
+
 func newBedrockProvider(cfg Config) (p *anthropicProvider, err error) {
+	region := resolveBedrockRegion(cfg)
+	if region == "" {
+		return nil, fmt.Errorf("ai: anthropic-bedrock requires region (set llm_api.region, AWS_REGION, or AWS_DEFAULT_REGION)")
+	}
+
 	defer func() {
 		if r := recover(); r != nil {
 			p = nil

pkg/ai/bedrock_test.go

@@ -9,6 +9,7 @@ import (
 func TestNewBedrockProvider_AuthPanicRecovery(t *testing.T) {
 	cfg := Config{
 		Provider: "anthropic-bedrock",
+		Region:   "us-east-1",
 	}
 	_, err := newBedrockProvider(cfg)
 	if err != nil {