From d0911404fe143470c359da0ec285bdc6a63c7ced Mon Sep 17 00:00:00 2001 From: Christopher Collins Date: Mon, 10 Aug 2026 13:04:30 -1000 Subject: [PATCH] =?UTF-8?q?fix:=20413b=20hygiene=20=E2=80=94=20nine=20defe?= =?UTF-8?q?cts=20in=20AI=20paths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Makefile | 2 +- README.md | 2 - docs/plans/417-413b-hygiene.md | 43 +++++++ pkg/agent/agent.go | 3 +- pkg/agent/index.go | 6 +- pkg/agent/session.go | 54 ++++++--- pkg/agent/session_test.go | 111 ++++++++++++++++++ pkg/ai/bedrock.go | 19 ++++ pkg/ai/bedrock_test.go | 1 + pkg/ai/factory_test.go | 66 +++++++++++ pkg/ai/provider_test.go | 2 +- pkg/ai/tools/registry.go | 15 ++- pkg/ocm/client.go | 1 - pkg/ocm/fixtures.go | 27 ++--- pkg/ocm/ocm.go | 6 +- pkg/tui/approvals.go | 16 +-- pkg/tui/approvals_test.go | 17 +++ pkg/tui/approvals_update_test.go | 19 ++-- pkg/tui/ask_wiring_test.go | 123 ++++++++++++++++++++ pkg/tui/claude.go | 34 +++++- pkg/tui/claude_test.go | 171 ++++++++++++++++++++++++++++ pkg/tui/commands.go | 11 +- pkg/tui/commands_test.go | 2 + pkg/tui/investigation_test.go | 54 ++++++++- pkg/tui/model.go | 51 ++++++--- pkg/tui/model_test.go | 38 +------ pkg/tui/msgHandlers.go | 8 -- pkg/tui/stream.go | 7 +- pkg/tui/tui.go | 14 +++ pkg/tui/views.go | 1 - pkg/tui/watcher.go | 67 ++++++++++- pkg/tui/watcher_integration_test.go | 2 + pkg/tui/watcher_test.go | 52 +++++++++ testdata/fixtures/clusters.json | 3 +- 34 files changed, 910 insertions(+), 138 deletions(-) create mode 100644 docs/plans/417-413b-hygiene.md diff --git a/Makefile b/Makefile index 5818e39f..946c9794 100644 --- a/Makefile +++ b/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 diff --git a/README.md b/README.md index e5d6533f..9aabc55f 100644 --- a/README.md +++ b/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 diff --git a/docs/plans/417-413b-hygiene.md b/docs/plans/417-413b-hygiene.md new file mode 100644 index 00000000..31de03c6 --- /dev/null +++ b/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. diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index 12b4ca45..ba7e19d4 100644 --- a/pkg/agent/agent.go +++ b/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 } diff --git a/pkg/agent/index.go b/pkg/agent/index.go index 952a3e00..d30581fd 100644 --- a/pkg/agent/index.go +++ b/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 { diff --git a/pkg/agent/session.go b/pkg/agent/session.go index f0662e8a..d6271b8e 100644 --- a/pkg/agent/session.go +++ b/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 { diff --git a/pkg/agent/session_test.go b/pkg/agent/session_test.go index bbb051dd..e421952c 100644 --- a/pkg/agent/session_test.go +++ b/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") + } + }) + } +} diff --git a/pkg/ai/bedrock.go b/pkg/ai/bedrock.go index 8725755f..9db17c3f 100644 --- a/pkg/ai/bedrock.go +++ b/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 diff --git a/pkg/ai/bedrock_test.go b/pkg/ai/bedrock_test.go index 1e6fc8cd..f088ecb1 100644 --- a/pkg/ai/bedrock_test.go +++ b/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 { diff --git a/pkg/ai/factory_test.go b/pkg/ai/factory_test.go index 01ad2916..06f14609 100644 --- a/pkg/ai/factory_test.go +++ b/pkg/ai/factory_test.go @@ -4,6 +4,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewProvider_Anthropic(t *testing.T) { @@ -252,6 +253,71 @@ func TestValidateConfig_Invalid(t *testing.T) { } } +func TestNewProvider_BedrockNoRegion(t *testing.T) { + t.Setenv("AWS_REGION", "") + t.Setenv("AWS_DEFAULT_REGION", "") + + provider, err := NewProvider(Config{ + Provider: "anthropic-bedrock", + }) + + require.Error(t, err) + assert.Nil(t, provider) + assert.Contains(t, err.Error(), "region") + assert.Contains(t, err.Error(), "AWS_REGION") +} + +func TestNewProvider_BedrockRegionFromConfig(t *testing.T) { + t.Setenv("AWS_REGION", "") + t.Setenv("AWS_DEFAULT_REGION", "") + + provider, err := NewProvider(Config{ + Provider: "anthropic-bedrock", + Region: "us-west-2", + }) + + // Should pass region validation (may still fail on AWS auth, which is OK) + if err != nil { + assert.NotContains(t, err.Error(), "AWS_REGION", + "with Region set in config, region validation should pass") + } else { + assert.NotNil(t, provider) + } +} + +func TestNewProvider_BedrockRegionFromEnv(t *testing.T) { + t.Setenv("AWS_REGION", "eu-west-1") + t.Setenv("AWS_DEFAULT_REGION", "") + + provider, err := NewProvider(Config{ + Provider: "anthropic-bedrock", + }) + + // Should pass region validation (may still fail on AWS auth, which is OK) + if err != nil { + assert.NotContains(t, err.Error(), "AWS_REGION", + "with AWS_REGION set, region validation should pass") + } else { + assert.NotNil(t, provider) + } +} + +func TestNewProvider_BedrockRegionFromDefaultRegionEnv(t *testing.T) { + t.Setenv("AWS_REGION", "") + t.Setenv("AWS_DEFAULT_REGION", "ap-southeast-1") + + provider, err := NewProvider(Config{ + Provider: "anthropic-bedrock", + }) + + if err != nil { + assert.NotContains(t, err.Error(), "AWS_REGION", + "with AWS_DEFAULT_REGION set, region validation should pass") + } else { + assert.NotNil(t, provider) + } +} + func TestProviderRegistry_Defaults(t *testing.T) { tests := []struct { name string diff --git a/pkg/ai/provider_test.go b/pkg/ai/provider_test.go index b2fb7d03..a3266676 100644 --- a/pkg/ai/provider_test.go +++ b/pkg/ai/provider_test.go @@ -79,7 +79,7 @@ func TestResolvedModel(t *testing.T) { }) t.Run("bedrock provider yields inference-profile ID not bare model", func(t *testing.T) { - p, err := newBedrockProvider(Config{}) + p, err := newBedrockProvider(Config{Region: "us-east-1"}) assert.NoError(t, err) assert.Equal(t, bedrockDefaultModel, ResolvedModel(p), "Bedrock default must be the inference-profile ID, not the bare model") diff --git a/pkg/ai/tools/registry.go b/pkg/ai/tools/registry.go index c45f3f7e..13e2f534 100644 --- a/pkg/ai/tools/registry.go +++ b/pkg/ai/tools/registry.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "sync" + "unicode/utf8" anthropic "github.com/anthropics/anthropic-sdk-go" "github.com/clcollins/srepd/pkg/ai/policy" @@ -144,6 +145,8 @@ func (r *Registry) GatedBetaTools( } // Truncate shortens s to maxBytes, appending a truncation marker if cut. +// The cut point is backed up to the last valid UTF-8 rune boundary to +// avoid splitting multi-byte characters. func Truncate(s string, maxBytes int) string { if len(s) <= maxBytes { return s @@ -153,7 +156,15 @@ func Truncate(s string, maxBytes int) string { return "" } if maxBytes <= len(marker) { - return s[:maxBytes] + cut := maxBytes + for cut > 0 && !utf8.RuneStart(s[cut]) { + cut-- + } + return s[:cut] + } + cut := maxBytes - len(marker) + for cut > 0 && !utf8.RuneStart(s[cut]) { + cut-- } - return s[:maxBytes-len(marker)] + marker + return s[:cut] + marker } diff --git a/pkg/ocm/client.go b/pkg/ocm/client.go index 233e26e6..fafb535a 100644 --- a/pkg/ocm/client.go +++ b/pkg/ocm/client.go @@ -272,7 +272,6 @@ func clusterFromResponse(cluster *cmv1.Cluster) *ClusterInfo { State: string(cluster.State()), CloudProvider: cluster.CloudProvider().ID(), Version: cluster.OpenshiftVersion(), - CreatedAt: cluster.CreationTimestamp(), } if cluster.Region() != nil { diff --git a/pkg/ocm/fixtures.go b/pkg/ocm/fixtures.go index 62e8e5a7..df34bfc2 100644 --- a/pkg/ocm/fixtures.go +++ b/pkg/ocm/fixtures.go @@ -5,23 +5,21 @@ import ( "fmt" "os" "path/filepath" - "time" ) type fixtureCluster struct { - ID string `json:"id"` - ExternalID string `json:"external_id"` - Name string `json:"name"` - DisplayName string `json:"display_name"` - State string `json:"state"` - Region string `json:"region"` - CloudProvider string `json:"cloud_provider"` - Version string `json:"version"` - Hypershift bool `json:"hypershift"` - CCS bool `json:"ccs"` - Organization string `json:"organization"` - OrganizationID string `json:"organization_id"` - CreatedAt time.Time `json:"created_at"` + ID string `json:"id"` + ExternalID string `json:"external_id"` + Name string `json:"name"` + DisplayName string `json:"display_name"` + State string `json:"state"` + Region string `json:"region"` + CloudProvider string `json:"cloud_provider"` + Version string `json:"version"` + Hypershift bool `json:"hypershift"` + CCS bool `json:"ccs"` + Organization string `json:"organization"` + OrganizationID string `json:"organization_id"` } type fixtureServiceLog struct { @@ -86,7 +84,6 @@ func loadClusterFixtures(path string, mock *MockClient) error { CCS: fc.CCS, Organization: fc.Organization, OrganizationID: fc.OrganizationID, - CreatedAt: fc.CreatedAt, } } return nil diff --git a/pkg/ocm/ocm.go b/pkg/ocm/ocm.go index 50d5ea00..a075c521 100644 --- a/pkg/ocm/ocm.go +++ b/pkg/ocm/ocm.go @@ -1,9 +1,6 @@ package ocm -import ( - "context" - "time" -) +import "context" // ClusterInfo contains enriched cluster data from the OCM API. type ClusterInfo struct { @@ -19,7 +16,6 @@ type ClusterInfo struct { CCS bool Organization string OrganizationID string - CreatedAt time.Time } // ServiceLog represents a single service log entry. diff --git a/pkg/tui/approvals.go b/pkg/tui/approvals.go index 5c6f9cf7..078749e8 100644 --- a/pkg/tui/approvals.go +++ b/pkg/tui/approvals.go @@ -21,12 +21,14 @@ const ( // Ask represents a pending approval item from the AI watcher. type Ask struct { - ID string - Kind AskKind - Title string - Body string - Action func() tea.Cmd - CreatedAt time.Time + ID string + Kind AskKind + Title string + Body string + IncidentID string + IncidentTitle string + Action func() tea.Cmd + CreatedAt time.Time } // approvalsStrip manages the list of pending asks. @@ -150,7 +152,7 @@ func inferAskKind(action string) AskKind { switch { case strings.Contains(lower, "escalat") || strings.Contains(lower, "re-escalat"): return AskEscalationSuggestion - case strings.Contains(lower, "command") || strings.Contains(lower, "oc ") || strings.Contains(lower, "kubectl"): + case strings.Contains(lower, "command") || strings.HasPrefix(lower, "oc ") || strings.Contains(lower, " oc ") || strings.Contains(lower, "kubectl"): return AskSuggestedCommand default: return AskDraftNote diff --git a/pkg/tui/approvals_test.go b/pkg/tui/approvals_test.go index c5426696..ce9ca7c0 100644 --- a/pkg/tui/approvals_test.go +++ b/pkg/tui/approvals_test.go @@ -187,3 +187,20 @@ func TestApprovalsStrip_AcceptSelected(t *testing.T) { assert.Equal(t, "cmd1", called, "must accept the selected ask") assert.Equal(t, 1, strip.Count()) } + +func TestAskKindLabel(t *testing.T) { + tests := []struct { + kind AskKind + want string + }{ + {AskDraftNote, "Note"}, + {AskSuggestedCommand, "Command"}, + {AskEscalationSuggestion, "Escalation"}, + {AskKind(99), "Unknown"}, + } + for _, tt := range tests { + t.Run(tt.want, func(t *testing.T) { + assert.Equal(t, tt.want, askKindLabel(tt.kind)) + }) + } +} diff --git a/pkg/tui/approvals_update_test.go b/pkg/tui/approvals_update_test.go index 312126d2..0035f905 100644 --- a/pkg/tui/approvals_update_test.go +++ b/pkg/tui/approvals_update_test.go @@ -101,14 +101,17 @@ func TestUpdate_ApprovalsEnter_ReturnsCmdThatPostsNote(t *testing.T) { } noteContent := "Investigation: elevated error rate on cluster" - m.approvals.Add(Ask{ - Kind: AskDraftNote, - Title: "Post investigation note", - Body: noteContent, - Action: func() tea.Cmd { - return m.postAINoteCmd(noteContent) - }, + ask := m.buildAskFromVerdict(tools.Verdict{ + Tier: tools.TierActionable, + Summary: "Post investigation note", + Action: noteContent, }) + m.approvals.Add(ask) + + // Simulate user browsing to a different incident after ask creation + m.selectedIncident = &pagerduty.Incident{ + APIObject: pagerduty.APIObject{ID: "INC-DIFFERENT"}, + } m.approvalsExpanded = true m.table.Focus() @@ -130,6 +133,8 @@ func TestUpdate_ApprovalsEnter_ReturnsCmdThatPostsNote(t *testing.T) { require.NotNil(t, noteMsg.note) assert.Equal(t, noteContent, noteMsg.note.Content, "the posted note content must match the verdict action") + assert.Equal(t, "INC-M2-001", noteMsg.incidentID, + "note must target the snapshotted incident, not the current selection") assert.Equal(t, 1, mock.CallCounts["CreateIncidentNoteWithContext"], "PD mock CreateIncidentNoteWithContext must be called exactly once") } diff --git a/pkg/tui/ask_wiring_test.go b/pkg/tui/ask_wiring_test.go index dca53cac..8c38d0c6 100644 --- a/pkg/tui/ask_wiring_test.go +++ b/pkg/tui/ask_wiring_test.go @@ -148,6 +148,129 @@ func TestInferAskKind_Fallback_IsSensibleDefault(t *testing.T) { } } +func TestBuildAskFromVerdict_DraftNote_TargetsOriginalIncident(t *testing.T) { + mock := &pd.MockPagerDutyClient{} + m := createTestModel() + m.config = &pd.Config{Client: mock} + + incidentA := &pagerduty.Incident{ + APIObject: pagerduty.APIObject{ID: "INC-A"}, + Title: "Incident A", + } + incidentB := &pagerduty.Incident{ + APIObject: pagerduty.APIObject{ID: "INC-B"}, + Title: "Incident B", + } + + m.selectedIncident = incidentA + + verdict := tools.Verdict{ + Tier: tools.TierActionable, + Summary: "Post note", + Action: "Note content for incident A", + } + ask := m.buildAskFromVerdict(verdict) + + assert.Equal(t, "INC-A", ask.IncidentID, + "Ask must snapshot the incident ID at creation time") + assert.Equal(t, "Incident A", ask.IncidentTitle, + "Ask must snapshot the incident title at creation time") + + // Simulate user browsing to a different incident + m.selectedIncident = incidentB + + cmd := ask.Action() + require.NotNil(t, cmd) + msg := cmd() + + noteMsg, ok := msg.(addedIncidentNoteMsg) + require.True(t, ok, "expected addedIncidentNoteMsg, got %T", msg) + require.NoError(t, noteMsg.err) + + assert.Equal(t, "INC-A", noteMsg.incidentID, + "note must target incident A (snapshotted), not B (live selection)") +} + +func TestBuildAskFromVerdict_Escalation_TargetsOriginalIncident(t *testing.T) { + m := createTestModel() + + incidentA := pagerduty.Incident{ + APIObject: pagerduty.APIObject{ID: "INC-A"}, + Title: "Incident A", + } + incidentB := pagerduty.Incident{ + APIObject: pagerduty.APIObject{ID: "INC-B"}, + Title: "Incident B", + } + + m.selectedIncident = &incidentA + + verdict := tools.Verdict{ + Tier: tools.TierActionable, + Summary: "Re-escalate", + Action: "Re-escalate this incident", + } + ask := m.buildAskFromVerdict(verdict) + + assert.Equal(t, "INC-A", ask.IncidentID) + + // Simulate user browsing to a different incident + m.selectedIncident = &incidentB + + cmd := ask.Action() + require.NotNil(t, cmd) + msg := cmd() + + reescMsg, ok := msg.(unAcknowledgeIncidentsMsg) + require.True(t, ok, "expected unAcknowledgeIncidentsMsg, got %T", msg) + require.Len(t, reescMsg.incidents, 1) + assert.Equal(t, "INC-A", reescMsg.incidents[0].ID, + "escalation must target incident A (snapshotted), not B (live selection)") +} + +func TestBuildAskFromVerdict_NilSelectedIncident_NoAction(t *testing.T) { + m := createTestModel() + m.selectedIncident = nil + + verdict := tools.Verdict{ + Tier: tools.TierActionable, + Summary: "Re-escalate", + Action: "Re-escalate this incident", + } + ask := m.buildAskFromVerdict(verdict) + + assert.Empty(t, ask.IncidentID, "no incident selected means empty IncidentID") + require.NotNil(t, ask.Action) + + cmd := ask.Action() + require.NotNil(t, cmd) + msg := cmd() + + statusMsg, ok := msg.(setStatusMsg) + require.True(t, ok, "nil incident must produce setStatusMsg, got %T", msg) + assert.Contains(t, statusMsg.string, "no incident") +} + +func TestBuildAskFromVerdict_SanitizesControlSequences(t *testing.T) { + m := createTestModel() + m.selectedIncident = &pagerduty.Incident{ + APIObject: pagerduty.APIObject{ID: "INC-SANITIZE-001"}, + } + + verdict := tools.Verdict{ + Tier: tools.TierActionable, + Summary: "Clean\x1b]52;c;SGVsbG8=\x07title\x1b[31m", + Action: "Injected\x1b[2Jaction\x07text", + } + + ask := m.buildAskFromVerdict(verdict) + + assert.Equal(t, "Cleantitle", ask.Title, + "Ask.Title must have control sequences stripped") + assert.Equal(t, "Injectedactiontext", ask.Body, + "Ask.Body must have control sequences stripped") +} + func TestBuildAskFromVerdict_UnhandledKind_FallbackAction(t *testing.T) { mock := &pd.MockPagerDutyClient{} m := createTestModel() diff --git a/pkg/tui/claude.go b/pkg/tui/claude.go index bb6475c3..3e19dae3 100644 --- a/pkg/tui/claude.go +++ b/pkg/tui/claude.go @@ -173,6 +173,10 @@ func (m model) handleClaudePrompt(msg claudePromptMsg, lookPath func(string) (st return m, m.flashNotification(err.Error()) } + if m.claudeQuerying { + return m, m.flashNotification("agent query already in progress") + } + incidentID := "" if m.selectedIncident != nil { incidentID = m.selectedIncident.ID @@ -291,6 +295,7 @@ func (m model) handleAgentSessionEvent(msg agentSessionEventMsg) (tea.Model, tea return m, nil case agent.PermissionAsk: + // TODO(phase-2): wire interactive permission approval through the TUI m.setStatus("⚠ Agent needs permission — check terminal") return m, readAgentSessionCmd(msg.session) @@ -330,10 +335,11 @@ func (m model) handleClaudeResponse(msg claudeResponseMsg) (tea.Model, tea.Cmd) // truncatePrompt shortens a prompt string for display in the status bar func truncatePrompt(s string, maxLen int) string { - if len(s) <= maxLen { + r := []rune(s) + if len(r) <= maxLen { return s } - return s[:maxLen] + "..." + return string(r[:maxLen]) + "..." } // defaultLookPath wraps exec.LookPath for production use @@ -429,9 +435,24 @@ type agentSessionDoneMsg struct { } // readAgentSessionCmd drains events from a session and returns them -// as Bubble Tea messages. +// as Bubble Tea messages. Events are drained preferentially: Done() +// is only checked when the event channel has nothing buffered, so a +// final Result that arrives simultaneously with process exit is never +// discarded by Go's random select. func readAgentSessionCmd(s *agent.Session) tea.Cmd { return func() tea.Msg { + select { + case ev, ok := <-s.Events(): + if !ok { + return agentSessionDoneMsg{} + } + if ev.Kind == agent.Error { + return agentSessionDoneMsg{err: ev.Err} + } + return agentSessionEventMsg{event: ev, session: s} + default: + } + // No event buffered — wait for either. select { case ev, ok := <-s.Events(): if !ok { @@ -480,8 +501,11 @@ type agentStreamChunkMsg struct { ch <-chan streamEvent } +// agentStreamDoneMsg signals the agent stream finished. ch identifies which +// stream produced this message so stale Done events can be ignored. type agentStreamDoneMsg struct { err error + ch <-chan streamEvent } // readAgentStreamCmd drains one event from the stream channel and returns it @@ -490,10 +514,10 @@ func readAgentStreamCmd(ch <-chan streamEvent) tea.Cmd { return func() tea.Msg { ev, ok := <-ch if !ok { - return agentStreamDoneMsg{} + return agentStreamDoneMsg{ch: ch} } if ev.done { - return agentStreamDoneMsg{err: ev.err} + return agentStreamDoneMsg{err: ev.err, ch: ch} } return agentStreamChunkMsg{text: ev.text, ch: ch} } diff --git a/pkg/tui/claude_test.go b/pkg/tui/claude_test.go index 7b56010a..7d972946 100644 --- a/pkg/tui/claude_test.go +++ b/pkg/tui/claude_test.go @@ -868,6 +868,7 @@ func TestAgentStreamChunkMsg_AppendsText(t *testing.T) { ch := make(chan streamEvent, 1) ch <- streamEvent{text: "more"} + m.agentStreamCh = ch msg := agentStreamChunkMsg{text: "world", ch: ch} result, cmd := m.Update(msg) @@ -1115,6 +1116,176 @@ func TestHandleClaudePrompt_ContextInjectionNotLostOnFailedFirstSend(t *testing. // TestHandleAgentSessionEvent_InitOnce verifies that two Init events // (the synthetic one from startAgentSession and the real one from the // subprocess) produce only one marker line in the watcher buffer. +func TestReadAgentSessionCmd_PrefersEventsOverDone(t *testing.T) { + delivered := 0 + for i := 0; i < 100; i++ { + events := make(chan agent.Event, 1) + done := make(chan struct{}) + events <- agent.Event{Kind: agent.Result, Text: "final answer"} + close(done) + + s := &agent.Session{} + agent.SetTestChannels(s, events, done) + + cmd := readAgentSessionCmd(s) + msg := cmd() + if evMsg, ok := msg.(agentSessionEventMsg); ok { + if evMsg.event.Kind == agent.Result { + delivered++ + } + } + } + assert.Equal(t, 100, delivered, + "with a buffered Result and closed Done, all 100 reads must deliver the Result") +} + +func TestStaleAgentStreamDoneMsg_IgnoredWhenSuperseded(t *testing.T) { + m := createTestModel() + m.claudeQuerying = true + m.apiInProgress = true + + // Simulate an active (current) stream: store its channel and cancel func. + currentCh := make(chan streamEvent) + currentCancelCalled := false + m.agentStreamCancel = func() { currentCancelCalled = true } + m.agentStreamCh = currentCh + + // A stale Done arrives from a PREVIOUS (superseded) stream. + staleCh := make(chan streamEvent) + staleMsg := agentStreamDoneMsg{ch: staleCh} + + result, cmd := m.Update(staleMsg) + updated := result.(model) + + assert.True(t, updated.claudeQuerying, + "stale Done must NOT clear claudeQuerying — a new stream is active") + assert.True(t, updated.apiInProgress, + "stale Done must NOT clear apiInProgress") + assert.NotNil(t, updated.agentStreamCancel, + "stale Done must NOT nil the current stream's cancel func") + assert.False(t, currentCancelCalled, + "stale Done must NOT call the current stream's cancel func") + assert.Nil(t, cmd, + "stale Done must not produce any command") +} + +func TestStaleAgentStreamChunkMsg_IgnoredWhenSuperseded(t *testing.T) { + m := createTestModel() + m.claudeQuerying = true + m.agentStreamPartial = "current text" + m.watcherExpanded = true + + // The model's active stream channel + currentCh := make(chan streamEvent) + m.agentStreamCh = currentCh + + // A stale Chunk arrives from a superseded stream. + staleCh := make(chan streamEvent) + staleMsg := agentStreamChunkMsg{text: " stale chunk", ch: staleCh} + + result, cmd := m.Update(staleMsg) + updated := result.(model) + + assert.Equal(t, "current text", updated.agentStreamPartial, + "stale chunk must NOT append to the current stream's partial text") + assert.Nil(t, cmd, + "stale chunk must not continue reading the stale channel") +} + +func TestStaleWatcherStreamDoneMsg_IgnoredWhenSuperseded(t *testing.T) { + m := createTestModel() + m.watcherAnalyzing = true + m.apiInProgress = true + + currentCh := make(chan streamEvent) + currentCancelCalled := false + m.watcherStreamCancel = func() { currentCancelCalled = true } + m.watcherStreamCh = currentCh + + staleCh := make(chan streamEvent) + staleMsg := watcherStreamDoneMsg{ch: staleCh} + + result, cmd := m.Update(staleMsg) + updated := result.(model) + + assert.True(t, updated.watcherAnalyzing, + "stale Done must NOT clear watcherAnalyzing") + assert.True(t, updated.apiInProgress, + "stale Done must NOT clear apiInProgress") + assert.NotNil(t, updated.watcherStreamCancel, + "stale Done must NOT nil the current stream's cancel func") + assert.False(t, currentCancelCalled, + "stale Done must NOT call the current stream's cancel func") + assert.Nil(t, cmd, + "stale Done must not produce any command") +} + +func TestStaleWatcherStreamChunkMsg_IgnoredWhenSuperseded(t *testing.T) { + m := createTestModel() + m.watcherAnalyzing = true + m.watcherStreamPartial = "current text" + m.watcherExpanded = true + + currentCh := make(chan streamEvent) + m.watcherStreamCh = currentCh + + staleCh := make(chan streamEvent) + staleMsg := watcherStreamChunkMsg{text: " stale chunk", ch: staleCh} + + result, cmd := m.Update(staleMsg) + updated := result.(model) + + assert.Equal(t, "current text", updated.watcherStreamPartial, + "stale chunk must NOT append to the current stream's partial text") + assert.Nil(t, cmd, + "stale chunk must not continue reading the stale channel") +} + +func TestCurrentStreamDoneMsg_StillClearsState(t *testing.T) { + m := createTestModel() + m.claudeQuerying = true + m.apiInProgress = true + + currentCh := make(chan streamEvent) + m.agentStreamCancel = func() {} + m.agentStreamCh = currentCh + + // Done from the CURRENT stream — should clear state as before. + msg := agentStreamDoneMsg{ch: currentCh} + + result, _ := m.Update(msg) + updated := result.(model) + + assert.False(t, updated.claudeQuerying, + "current stream Done must clear claudeQuerying") + assert.False(t, updated.apiInProgress, + "current stream Done must clear apiInProgress") + assert.Nil(t, updated.agentStreamCancel, + "current stream Done must nil cancel func") +} + +func TestHandleClaudePrompt_RejectsWhileInFlight(t *testing.T) { + m := createTestModel() + m.agentCLICommand = "claude --print" + m.claudeQuerying = true + m.apiInProgress = true + + msg := claudePromptMsg{prompt: "second query while first is running"} + result, cmd := m.handleClaudePrompt(msg, func(s string) (string, error) { + return "/usr/bin/" + s, nil + }) + updated := result.(model) + + assert.True(t, updated.claudeQuerying, + "in-flight flag must remain set") + assert.True(t, updated.apiInProgress, + "apiInProgress must remain set") + assert.NotNil(t, cmd, + "must return a flash notification command") + assert.Contains(t, updated.status, "in progress", + "status must tell user a query is already running") +} + func TestHandleAgentSessionEvent_InitOnce(t *testing.T) { m := sizedTestModel(t) m.watcherExpanded = true diff --git a/pkg/tui/commands.go b/pkg/tui/commands.go index 94d3d5e2..f91a9a7b 100644 --- a/pkg/tui/commands.go +++ b/pkg/tui/commands.go @@ -799,9 +799,9 @@ type loginProcessExitedMsg struct { // sanitizeEnvValue removes or escapes characters that could cause issues // in environment variable values passed via -e flags to terminals or containers. func sanitizeEnvValue(s string) string { + s = stripControl(s) r := strings.NewReplacer( "\n", " ", - "\r", "", "\t", " ", "'", "", "\"", "", @@ -1101,8 +1101,9 @@ func silenceIncidents(incidents []pagerduty.Incident, policy *pagerduty.Escalati //lint:ignore U1000 - future proofing type addIncidentNoteMsg string type addedIncidentNoteMsg struct { - note *pagerduty.IncidentNote - err error + note *pagerduty.IncidentNote + err error + incidentID string } func addNoteToIncident(p *pd.Config, incident *pagerduty.Incident, file *os.File) tea.Cmd { @@ -1123,10 +1124,10 @@ func addNoteToIncident(p *pd.Config, incident *pagerduty.Incident, file *os.File if note != "" { n, err := pd.PostNote(p.Client, incident.ID, u, note) - return addedIncidentNoteMsg{n, err} + return addedIncidentNoteMsg{n, err, incident.ID} } - return addedIncidentNoteMsg{nil, errors.New(nilNoteErr)} + return addedIncidentNoteMsg{nil, errors.New(nilNoteErr), incident.ID} } } diff --git a/pkg/tui/commands_test.go b/pkg/tui/commands_test.go index 99d40e84..cd444862 100644 --- a/pkg/tui/commands_test.go +++ b/pkg/tui/commands_test.go @@ -2132,6 +2132,8 @@ func TestSanitizeEnvValue(t *testing.T) { {"with backticks", "alert `cmd` here", "alert cmd here"}, {"with dollar signs", "alert $VAR here", "alert VAR here"}, {"with backslashes", `alert \n here`, "alert n here"}, + {"strips ANSI escape", "alert \x1b[31mred\x1b[0m value", "alert red value"}, + {"strips OSC sequence", "title\x1b]0;pwned\x07value", "titlevalue"}, {"empty string", "", ""}, } for _, tt := range tests { diff --git a/pkg/tui/investigation_test.go b/pkg/tui/investigation_test.go index daedfc8c..b8bbe7cb 100644 --- a/pkg/tui/investigation_test.go +++ b/pkg/tui/investigation_test.go @@ -12,6 +12,7 @@ import ( anthropic "github.com/anthropics/anthropic-sdk-go" "github.com/anthropics/anthropic-sdk-go/option" + "github.com/clcollins/srepd/pkg/ai" "github.com/clcollins/srepd/pkg/ai/policy" "github.com/clcollins/srepd/pkg/ai/tools" "github.com/stretchr/testify/assert" @@ -358,5 +359,56 @@ func TestIsAnthropicFamily(t *testing.T) { func TestDefaultInvestigationConfig(t *testing.T) { cfg := defaultInvestigationConfig() assert.Equal(t, 6, cfg.maxToolTurns) - assert.Greater(t, cfg.timeout.Seconds(), float64(0)) + assert.Equal(t, 90*time.Second, cfg.timeout) + assert.Equal(t, policy.ModeInteractive, cfg.policyConfig.Mode) +} + +func TestExtractToolRunnerFactory_NilProvider(t *testing.T) { + assert.Nil(t, extractToolRunnerFactory(nil)) +} + +func TestExtractToolRunnerFactory_NonAnthropicProvider(t *testing.T) { + mock := &ai.MockProvider{ProviderName: "ollama"} + assert.Nil(t, extractToolRunnerFactory(mock), + "non-Anthropic provider must return nil factory") +} + +type mockBetaMessagesProvider struct { + ai.MockProvider +} + +func (m *mockBetaMessagesProvider) BetaMessages() *anthropic.BetaMessageService { + return nil +} + +func TestExtractToolRunnerFactory_WithBetaMessages(t *testing.T) { + mock := &mockBetaMessagesProvider{} + result := extractToolRunnerFactory(mock) + assert.Nil(t, result, + "BetaMessages() returns nil service so factory should be nil") +} + +type realBetaMessagesProvider struct { + ai.MockProvider + svc *anthropic.BetaMessageService +} + +func (r *realBetaMessagesProvider) BetaMessages() *anthropic.BetaMessageService { + return r.svc +} + +func TestExtractToolRunnerFactory_NonNilService_ReturnsUsableFactory(t *testing.T) { + svc := anthropic.NewBetaMessageService() + mock := &realBetaMessagesProvider{svc: &svc} + factory := extractToolRunnerFactory(mock) + require.NotNil(t, factory, + "anthropic-family provider with non-nil BetaMessages must yield a non-nil factory") + + runner := factory.NewToolRunner(nil, anthropic.BetaToolRunnerParams{ + BetaMessageNewParams: anthropic.BetaMessageNewParams{ + MaxTokens: 1, + }, + }) + assert.NotNil(t, runner, + "factory must be able to construct a BetaToolRunner") } diff --git a/pkg/tui/model.go b/pkg/tui/model.go index 7b3cc48b..4ef85592 100644 --- a/pkg/tui/model.go +++ b/pkg/tui/model.go @@ -190,9 +190,11 @@ type model struct { streamResponses bool watcherStreamPartial string watcherStreamCancel context.CancelFunc + watcherStreamCh <-chan streamEvent agentStreamPartial string agentStreamCancel context.CancelFunc + agentStreamCh <-chan streamEvent // Session-based agent state (Phase 1 AI rearchitecture) agentSessionEnabled bool @@ -956,55 +958,72 @@ func (m *model) buildAskFromVerdict(verdict tools.Verdict) Ask { kind := inferAskKind(verdict.Action) ask := Ask{ Kind: kind, - Title: verdict.Summary, - Body: verdict.Action, + Title: stripControl(verdict.Summary), + Body: stripControl(verdict.Action), + } + + // Snapshot the incident identity at creation time so that actions + // always target the incident that seeded the investigation, never + // whichever incident happens to be selected when the user accepts. + if m.selectedIncident != nil { + ask.IncidentID = m.selectedIncident.ID + ask.IncidentTitle = m.selectedIncident.Title } switch kind { case AskDraftNote: - noteContent := verdict.Action + noteContent := ask.Body + incidentID := ask.IncidentID ask.Action = func() tea.Cmd { - return m.postAINoteCmd(noteContent) + if incidentID == "" { + return func() tea.Msg { return setStatusMsg{"no incident selected for note"} } + } + return m.postAINoteToIncidentCmd(incidentID, noteContent) } case AskSuggestedCommand: - cmdText := verdict.Action + cmdText := ask.Body ask.Action = func() tea.Cmd { return m.copyToClipboardCmd(cmdText) } case AskEscalationSuggestion: + incidentID := ask.IncidentID + var snapshotIncident pagerduty.Incident + if m.selectedIncident != nil { + snapshotIncident = *m.selectedIncident + } ask.Action = func() tea.Cmd { - if m.selectedIncident == nil { + if incidentID == "" { return func() tea.Msg { return setStatusMsg{"no incident selected for re-escalation"} } } - incident := *m.selectedIncident return func() tea.Msg { - return unAcknowledgeIncidentsMsg{incidents: []pagerduty.Incident{incident}} + return unAcknowledgeIncidentsMsg{incidents: []pagerduty.Incident{snapshotIncident}} } } default: - noteContent := verdict.Action + noteContent := ask.Body + incidentID := ask.IncidentID ask.Action = func() tea.Cmd { - return m.postAINoteCmd(noteContent) + if incidentID == "" { + return func() tea.Msg { return setStatusMsg{"no incident selected for note"} } + } + return m.postAINoteToIncidentCmd(incidentID, noteContent) } } return ask } -func (m *model) postAINoteCmd(content string) tea.Cmd { +func (m *model) postAINoteToIncidentCmd(incidentID string, content string) tea.Cmd { return func() tea.Msg { if m.config == nil || m.config.Client == nil { return errMsg{fmt.Errorf("PagerDuty not configured")} } - if m.selectedIncident == nil { - return setStatusMsg{"no incident selected for note"} - } u, err := pd.GetCurrentUser(m.config.Client) if err != nil { return errMsg{err} } - n, err := pd.PostNote(m.config.Client, m.selectedIncident.ID, u, content) - return addedIncidentNoteMsg{n, err} + n, err := pd.PostNote(m.config.Client, incidentID, u, content) + return addedIncidentNoteMsg{n, err, incidentID} } } diff --git a/pkg/tui/model_test.go b/pkg/tui/model_test.go index 2c57c256..8c93cda2 100644 --- a/pkg/tui/model_test.go +++ b/pkg/tui/model_test.go @@ -1290,39 +1290,6 @@ func TestTabSwitch_TabKey(t *testing.T) { } } -func TestIncidentViewer_TopBottom(t *testing.T) { - m := createTestModel() - m.viewingIncident = true - m.selectedIncident = &pagerduty.Incident{ - APIObject: pagerduty.APIObject{ID: "Q123"}, - } - m.incidentViewer = newIncidentViewer() - m.incidentViewer.Height = 5 - lines := "" - for i := 0; i < 50; i++ { - lines += "line\n" - } - m.incidentViewer.SetContent(lines) - - // Scroll down first so we're not at the top - m.incidentViewer.GotoBottom() - assert.Greater(t, m.incidentViewer.YOffset, 0) - - t.Run("g jumps to top", func(t *testing.T) { - msg := tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'g'}} - result, _ := m.Update(msg) - updated := result.(model) - assert.Equal(t, 0, updated.incidentViewer.YOffset) - }) - - t.Run("G jumps to bottom", func(t *testing.T) { - msg := tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'G'}} - result, _ := m.Update(msg) - updated := result.(model) - assert.Greater(t, updated.incidentViewer.YOffset, 0) - }) -} - // captureLogOutput runs a function while capturing log output at the given level. // Returns the captured log output as a string. func captureLogOutput(level log.Level, fn func()) string { @@ -1410,8 +1377,9 @@ func TestSREActionsLogAtInfoLevel(t *testing.T) { { name: "note added logs at INFO", msg: addedIncidentNoteMsg{ - note: &pagerduty.IncidentNote{ID: "N123", Content: "test note"}, - err: nil, + note: &pagerduty.IncidentNote{ID: "N123", Content: "test note"}, + err: nil, + incidentID: "Q789", }, setupModel: func(m *model) { m.selectedIncident = &pagerduty.Incident{ diff --git a/pkg/tui/msgHandlers.go b/pkg/tui/msgHandlers.go index 6570d204..c164b4c2 100644 --- a/pkg/tui/msgHandlers.go +++ b/pkg/tui/msgHandlers.go @@ -917,14 +917,6 @@ func switchIncidentFocusMode(m model, msg tea.Msg) (tea.Model, tea.Cmd) { m.incidentViewer, _ = m.incidentViewer.Update(msg) return m, nil - case key.Matches(msg, defaultKeyMap.Top): - m.incidentViewer.GotoTop() - return m, nil - - case key.Matches(msg, defaultKeyMap.Bottom): - m.incidentViewer.GotoBottom() - return m, nil - // Tab/Shift+Tab: switch between tabs case key.Matches(msg, defaultKeyMap.TabNext): m.activeTab = (m.activeTab + 1) % tabCount diff --git a/pkg/tui/stream.go b/pkg/tui/stream.go index d79687c8..99e92c49 100644 --- a/pkg/tui/stream.go +++ b/pkg/tui/stream.go @@ -35,8 +35,11 @@ type watcherStreamChunkMsg struct { } // watcherStreamDoneMsg signals the stream finished (err is nil on success). +// ch identifies which stream produced this message so stale Done events from +// superseded streams can be ignored. type watcherStreamDoneMsg struct { err error + ch <-chan streamEvent } // streamWatcherCmd starts a streaming provider query. A background goroutine runs @@ -132,10 +135,10 @@ func readStreamCmd(ch <-chan streamEvent) tea.Cmd { return func() tea.Msg { ev, ok := <-ch if !ok { - return watcherStreamDoneMsg{} + return watcherStreamDoneMsg{ch: ch} } if ev.done { - return watcherStreamDoneMsg{err: ev.err} + return watcherStreamDoneMsg{err: ev.err, ch: ch} } return watcherStreamChunkMsg{text: ev.text, ch: ch} } diff --git a/pkg/tui/tui.go b/pkg/tui/tui.go index a1876921..ea60beac 100644 --- a/pkg/tui/tui.go +++ b/pkg/tui/tui.go @@ -628,6 +628,7 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.watcherStreamCancel() } m.watcherStreamCancel = msg.cancel + m.watcherStreamCh = msg.ch m.watcherStreamPartial = "" if !m.watcherExpanded { m.watcherExpanded = true @@ -638,6 +639,9 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, readStreamCmd(msg.ch) case watcherStreamChunkMsg: + if msg.ch != m.watcherStreamCh { + return m, nil + } // The first token is proof the provider answered — mark it healthy // now, not at end of stream (idempotent for subsequent chunks). m.aiHealth = aiHealthOK @@ -647,6 +651,9 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, readStreamCmd(msg.ch) case watcherStreamDoneMsg: + if msg.ch != m.watcherStreamCh { + return m, nil + } m.watcherAnalyzing = false m.apiInProgress = false m.watcherStreamCancel = nil @@ -1880,6 +1887,7 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.agentStreamCancel() } m.agentStreamCancel = msg.cancel + m.agentStreamCh = msg.ch m.agentStreamPartial = "" if !m.watcherExpanded { m.watcherExpanded = true @@ -1890,12 +1898,18 @@ func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, readAgentStreamCmd(msg.ch) case agentStreamChunkMsg: + if msg.ch != m.agentStreamCh { + return m, nil + } m.agentStreamPartial += msg.text m.watcherBuffer.SetLast(prefixLines(m.agentMarker, m.agentStreamPartial)) m.updateWatcherViewport() return m, readAgentStreamCmd(msg.ch) case agentStreamDoneMsg: + if msg.ch != m.agentStreamCh { + return m, nil + } m.claudeQuerying = false m.apiInProgress = false m.agentStreamCancel = nil diff --git a/pkg/tui/views.go b/pkg/tui/views.go index fa87d1dc..f8637820 100644 --- a/pkg/tui/views.go +++ b/pkg/tui/views.go @@ -589,7 +589,6 @@ func (m model) renderClusterTab() (string, error) { fmt.Fprintf(&content, "* CCS: %v\n", info.CCS) fmt.Fprintf(&content, "* Organization: %s\n", info.Organization) fmt.Fprintf(&content, "* Organization ID: %s\n", info.OrganizationID) - fmt.Fprintf(&content, "* Created: %s\n", info.CreatedAt.Format("2006-01-02 15:04:05 UTC")) if i < len(clusters)-1 { content.WriteString("\n---\n") } diff --git a/pkg/tui/watcher.go b/pkg/tui/watcher.go index 2933b8dc..642f994a 100644 --- a/pkg/tui/watcher.go +++ b/pkg/tui/watcher.go @@ -54,6 +54,7 @@ func newWatcherBuffer(capacity int) *watcherBuffer { } func (b *watcherBuffer) Append(entry string) { + entry = stripControl(entry) if len(b.entries) >= b.capacity { b.entries = b.entries[1:] } @@ -61,6 +62,7 @@ func (b *watcherBuffer) Append(entry string) { } func (b *watcherBuffer) SetLast(entry string) { + entry = stripControl(entry) if len(b.entries) == 0 { b.Append(entry) return @@ -403,8 +405,8 @@ func buildWatcherContext(m *model) string { break } content := n.Content - if len(content) > 300 { - content = content[:300] + "..." + if r := []rune(content); len(r) > 300 { + content = string(r[:300]) + "..." } parts = append(parts, fmt.Sprintf(" - %s", content)) } @@ -448,6 +450,67 @@ func buildClusterContext(m *model, clusterID string) []string { return parts } +// stripControl removes ANSI escape sequences (CSI, OSC, ESC) and C0 +// control characters from s, preserving only \n and \t. Applied at +// the buffer-append boundary so all AI output is sanitized in one +// place, preventing terminal injection from attacker-influenced data. +func stripControl(s string) string { + var b strings.Builder + b.Grow(len(s)) + i := 0 + for i < len(s) { + ch := s[i] + if ch == 0x1b { + i++ + if i >= len(s) { + break + } + switch s[i] { + case '[': + // CSI sequence: ESC [ ... final byte (0x40-0x7E) + i++ + for i < len(s) && (s[i] < 0x40 || (s[i] > 0x7E && s[i] < 0x80)) { + i++ + } + if i < len(s) { + i++ // skip final byte + } + case ']': + // OSC sequence: ESC ] ... (terminated by BEL or ST) + i++ + for i < len(s) { + if s[i] == 0x07 { + i++ + break + } + if s[i] == 0x1b && i+1 < len(s) && s[i+1] == '\\' { + i += 2 + break + } + i++ + } + default: + // Other ESC sequences (e.g., ESC c, ESC D): skip one char after ESC + i++ + } + continue + } + // Preserve \n and \t, strip all other C0 controls and DEL + if ch == '\n' || ch == '\t' { + b.WriteByte(ch) + i++ + continue + } + if ch < 0x20 || ch == 0x7f { + i++ + continue + } + b.WriteByte(ch) + i++ + } + return b.String() +} + func prefixLines(marker string, text string) string { lines := strings.Split(text, "\n") var result []string diff --git a/pkg/tui/watcher_integration_test.go b/pkg/tui/watcher_integration_test.go index 8492e6da..8e94d58d 100644 --- a/pkg/tui/watcher_integration_test.go +++ b/pkg/tui/watcher_integration_test.go @@ -633,6 +633,7 @@ func TestWatcherStreamChunkMsg_AccumulatesInPlace(t *testing.T) { m.watcherBuffer.Append(prefixLines(m.watcherMarker, "")) ch := make(chan streamEvent) + m.watcherStreamCh = ch result, _ := m.Update(watcherStreamChunkMsg{text: "Hello", ch: ch}) m = result.(model) result, _ = m.Update(watcherStreamChunkMsg{text: " world", ch: ch}) @@ -991,6 +992,7 @@ func TestWatcherStreamChunkMsg_FirstTokenSetsHealthOK(t *testing.T) { m.watcherBuffer.Append(prefixLines(m.watcherMarker, "")) ch := make(chan streamEvent) + m.watcherStreamCh = ch result, _ := m.Update(watcherStreamChunkMsg{text: "Hello", ch: ch}) updated := result.(model) diff --git a/pkg/tui/watcher_test.go b/pkg/tui/watcher_test.go index be4b6bde..746cb653 100644 --- a/pkg/tui/watcher_test.go +++ b/pkg/tui/watcher_test.go @@ -339,6 +339,58 @@ func TestBuildWatcherContext_NoIncident(t *testing.T) { assert.Contains(t, ctx, "svc-a") } +func TestStripControl(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + {"plain text", "hello world", "hello world"}, + {"preserves newline", "line1\nline2", "line1\nline2"}, + {"preserves tab", "col1\tcol2", "col1\tcol2"}, + {"strips ANSI color", "hello \x1b[31mred\x1b[0m world", "hello red world"}, + {"strips ANSI cursor move", "text\x1b[2Amoved", "textmoved"}, + {"strips OSC-52 clipboard", "text\x1b]52;c;SGVsbG8=\x07done", "textdone"}, + {"strips OSC with ST terminator", "text\x1b]0;title\x1b\\done", "textdone"}, + {"strips BEL", "hello\x07world", "helloworld"}, + {"strips C0 NUL", "hello\x00world", "helloworld"}, + {"strips C0 BS", "hello\x08world", "helloworld"}, + {"strips C0 CR", "hello\rworld", "helloworld"}, + {"strips mixed controls", "\x1b[31m\x07alert\x1b[0m\x00", "alert"}, + {"empty string", "", ""}, + {"only controls", "\x1b[31m\x07\x00", ""}, + {"unicode preserved", "hello 世界 🤖", "hello 世界 🤖"}, + {"CSI with params", "\x1b[38;5;196mcolored\x1b[0m", "colored"}, + // Truncated/malformed sequences — must not panic (R1) + {"truncated CSI params", "\x1b[999", ""}, + {"truncated CSI bare", "\x1b[", ""}, + {"lone ESC at end", "\x1b", ""}, + {"truncated OSC bare", "\x1b]", ""}, + {"truncated OSC-8 link", "\x1b]8;;", ""}, + {"lone ESC mid-string", "hello\x1bworld", "helloorld"}, + {"truncated CSI after text", "ok\x1b[999", "ok"}, + {"truncated OSC after text", "ok\x1b]title", "ok"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, stripControl(tt.input)) + }) + } +} + +func TestWatcherBuffer_StripsControlOnAppend(t *testing.T) { + buf := newWatcherBuffer(5) + buf.Append("safe \x1b[31mred\x1b[0m text") + assert.Equal(t, "safe red text", buf.Content()) +} + +func TestWatcherBuffer_StripsControlOnSetLast(t *testing.T) { + buf := newWatcherBuffer(5) + buf.Append("initial") + buf.SetLast("updated \x1b]52;c;SGVsbG8=\x07 content") + assert.Equal(t, "updated content", buf.Content()) +} + func TestBuildWatcherContext_WithIncident(t *testing.T) { m := createTestModel() m.selectedIncident = &pagerduty.Incident{ diff --git a/testdata/fixtures/clusters.json b/testdata/fixtures/clusters.json index a6146f38..d64f5230 100644 --- a/testdata/fixtures/clusters.json +++ b/testdata/fixtures/clusters.json @@ -11,8 +11,7 @@ "hypershift": false, "ccs": true, "organization": "Fake Aeronautical Ltd", - "organization_id": "1a2b3c4d5e6f7g8h9i0j", - "created_at": "2024-03-15T10:30:00Z" + "organization_id": "1a2b3c4d5e6f7g8h9i0j" }, "b2d4e6f8-fake-uuid-test-def012345678": { "id": "cluster-osd-002",