fix: 413b hygiene — eight defects in AI paths

[clcollins]

Summary

Post-merge audit of plan 413 (commit bccb0fb) found eight defects in AI paths. Two are safety-critical (B1: wrong-incident writes, B4: terminal injection), and the remainder are correctness/robustness issues.

• B1: Approval actions captured live m.selectedIncident — user switching incidents between creation and acceptance wrote to the wrong incident. Fixed by snapshotting incident identity into Ask at creation time.
• B4: ANSI CSI, OSC-52 clipboard writes, and C0 control chars in AI output passed through to the terminal. Fixed by stripping at the buffer-append boundary.
• B3: readAgentSessionCmd dropped the final Result ~50% of the time due to Go's random select. Fixed with two-phase select (non-blocking Events first).
• B2+B8: Spawn detection had no timeout/ctx case (deadlock); retry-as-resume leaked stdin/stdout pipes. Fixed both.
• B5: Stale stream Done messages nilled the successor stream's cancel func. Fixed by tagging messages with channel identity.
• B6: Submitting :agent while a query was in flight issued concurrent readers. Fixed with in-flight guard.
• B7: ClaudeArgs, ValidateUserFlags, extractToolRunnerFactory, askKindLabel had no direct unit tests. Added 50+ test cases.
• B9: Bedrock region not validated at construction. Added resolveBedrockRegion mirroring Vertex pattern.
• Cleanup: UTF-8 truncation, index.load warning/logging, inferAskKind false-positive, dead fields, PermissionAsk TODO.

R1-R5 follow-up fixes (second round)

• R1: stripControl panicked on truncated CSI sequences due to operator precedence bug (&& vs ||). Fixed with parentheses.
• R2: buildAskFromVerdict passed verdict Summary/Action to Ask Title/Body unsanitized, bypassing the B4 buffer-boundary choke point. Fixed by applying stripControl at Ask creation and using sanitized ask.Body in all action closures.
• R3: All three extractToolRunnerFactory tests asserted Nil — a return nil stub passed them all. Added positive-case test with non-nil BetaMessageService.
• R4: B1 traceability row cited three nonexistent test names. Fixed with mechanically-verified names.
• R5: Dead postAINoteCmd (zero production callers) still read m.selectedIncident live — the B1 bug. Deleted it and rewrote its test to exercise buildAskFromVerdict snapshot path.

Traceability

Fix	Test Functions	File	Commit
B1	TestBuildAskFromVerdict_DraftNote_TargetsOriginalIncident, TestBuildAskFromVerdict_Escalation_TargetsOriginalIncident, TestBuildAskFromVerdict_NilSelectedIncident_NoAction	pkg/tui/ask_wiring_test.go	a66f4e6, c23666d
B4	TestStripControl (16+8 cases), TestWatcherBuffer_StripsControlOnAppend, TestWatcherBuffer_StripsControlOnSetLast	pkg/tui/watcher_test.go	bf918ae, d6646c4
B3	TestReadAgentSessionCmd_PrefersEventsOverDone (100 iterations)	pkg/tui/claude_test.go	c85be57, c6d9791
B2+B8	TestSpawn_HungChildReturnsWithinTimeout	pkg/agent/session_test.go	a891870
B5	TestStaleAgentStreamDoneMsg_IgnoredWhenSuperseded, TestStaleAgentStreamChunkMsg_IgnoredWhenSuperseded, TestStaleWatcherStreamDoneMsg_IgnoredWhenSuperseded, TestStaleWatcherStreamChunkMsg_IgnoredWhenSuperseded, TestCurrentStreamDoneMsg_StillClearsState	pkg/tui/model_test.go	d948f82
B6	TestHandleClaudePrompt_RejectsWhileInFlight	pkg/tui/claude_test.go	06eadbb
B7	TestClaudeArgs (10), TestValidateUserFlags (20), TestExtractToolRunnerFactory_* (3), TestAskKindLabel (4), TestDefaultInvestigationConfig (real values)	pkg/tui/investigation_test.go, pkg/tui/claude_test.go, pkg/tui/approvals_test.go	d8190a3
B9	TestNewProvider_BedrockNoRegion, TestNewProvider_BedrockRegionFromConfig, TestNewProvider_BedrockRegionFromEnv, TestNewProvider_BedrockRegionFromDefaultRegionEnv	pkg/ai/provider_test.go	ec308c1
R1	TestStripControl/truncated_CSI_params, TestStripControl/truncated_CSI_bare, TestStripControl/lone_ESC_at_end, TestStripControl/truncated_OSC_bare, TestStripControl/truncated_OSC-8_link, TestStripControl/lone_ESC_mid-string, TestStripControl/truncated_CSI_after_text, TestStripControl/truncated_OSC_after_text	pkg/tui/watcher_test.go
R2	TestBuildAskFromVerdict_SanitizesControlSequences	pkg/tui/ask_wiring_test.go
R3	TestExtractToolRunnerFactory_NonNilService_ReturnsUsableFactory	pkg/tui/investigation_test.go
R5	TestUpdate_ApprovalsEnter_ReturnsCmdThatPostsNote (rewritten to use buildAskFromVerdict snapshot path)	pkg/tui/approvals_update_test.go

Traceability verification transcript

$ grep -rn 'func TestBuildAskFromVerdict_DraftNote_TargetsOriginalIncident(' pkg/
pkg/tui/ask_wiring_test.go:151:func TestBuildAskFromVerdict_DraftNote_TargetsOriginalIncident(t *testing.T) {

$ grep -rn 'func TestBuildAskFromVerdict_Escalation_TargetsOriginalIncident(' pkg/
pkg/tui/ask_wiring_test.go:194:func TestBuildAskFromVerdict_Escalation_TargetsOriginalIncident(t *testing.T) {

$ grep -rn 'func TestBuildAskFromVerdict_NilSelectedIncident_NoAction(' pkg/
pkg/tui/ask_wiring_test.go:231:func TestBuildAskFromVerdict_NilSelectedIncident_NoAction(t *testing.T) {

$ grep -rn 'func TestBuildAskFromVerdict_SanitizesControlSequences(' pkg/
pkg/tui/ask_wiring_test.go:254:func TestBuildAskFromVerdict_SanitizesControlSequences(t *testing.T) {

$ grep -rn 'func TestExtractToolRunnerFactory_NonNilService_ReturnsUsableFactory(' pkg/
pkg/tui/investigation_test.go:400:func TestExtractToolRunnerFactory_NonNilService_ReturnsUsableFactory(t *testing.T) {

$ grep -rn 'func TestUpdate_ApprovalsEnter_ReturnsCmdThatPostsNote(' pkg/
pkg/tui/approvals_update_test.go:91:func TestUpdate_ApprovalsEnter_ReturnsCmdThatPostsNote(t *testing.T) {

AI-output-to-terminal audit (R2)

Every path from AI output to the terminal was audited. Verification method: grep for each data flow, trace through to rendering.

Path	Data	Sanitized?	How
watcherBuffer.Append	All watcher buffer text	YES	Calls stripControl directly
watcherBuffer.SetLast	Stream chunks, typewriter output	YES	Calls stripControl directly
buildAskFromVerdict Title/Body	Verdict Summary/Action	YES	stripControl at creation (R2 fix)
Action closures (noteContent, cmdText)	Verdict Action	YES	Use ask.Body (sanitized) not raw verdict.Action (R2 fix)
startTypewriter → advanceTypewriter → SetLast	Verdict summary, synthesis response	YES	Via watcherBuffer.SetLast
agentStreamChunkMsg → SetLast	Agent stream text	YES	Via watcherBuffer.SetLast
Agent session TextDelta → SetLast	Claude text deltas	YES	Via watcherBuffer.SetLast
Agent session ToolUse → Append	Tool name/input	YES	Via watcherBuffer.Append
Agent session Result → SetLast	Final response	YES	Via watcherBuffer.SetLast
Glamour-rendered result → SetLast	Markdown-rendered AI text	YES	Via watcherBuffer.SetLast
approvalsStrip.RenderExpanded	ask.Title	YES	Title sanitized at creation
Error paths (errMsg, setStatusMsg)	API errors, not AI output	N/A	Server error messages, not model output
User-typed prompts in status	User input	N/A	Not AI output
sanitizeEnvValue in commands.go	Incident data for env vars	YES	Calls stripControl directly

Test plan

• go test ./... -count=1 — all packages pass
• go test -race ./pkg/agent/... ./pkg/tui/... ./pkg/ai/... — clean
• gofmt -s -l cmd pkg — clean
• go vet ./... — clean
• golangci-lint cache clean && golangci-lint run — clean
• deadcode ./... — no new entries
• Plan doc at docs/plans/414-413b-hygiene.md

Revert checks (R1, R2, R3)

• R1: Restoring original precedence for i < len(s) && s[i] < 0x40 || ... causes TestStripControl/truncated_CSI_params to panic with index out of range.
• R2: Removing stripControl from buildAskFromVerdict causes TestBuildAskFromVerdict_SanitizesControlSequences to fail (raw escapes in Title/Body).
• R3: Stubbing extractToolRunnerFactory to return nil causes TestExtractToolRunnerFactory_NonNilService_ReturnsUsableFactory to fail.

🤖 Generated with Claude Code

[clcollins]

PR #419 — 413b Hygiene: Adversarial Review

Methodology

Mutations executed (compiled, ran against test suite), not just code reading. Race detector applied to affected packages. Specific areas attacked per the audit plan.

---

MUST-FIX

1. stripControl panics on truncated CSI sequence — CONFIRMED by mutation

File: pkg/tui/watcher.go:472

Bug: Operator precedence error in the CSI-skip loop:

for i < len(s) && s[i] < 0x40 || (s[i] > 0x7E && s[i] < 0x80) {

Go parses this as (i < len(s) && s[i] < 0x40) || (s[i] > 0x7E && s[i] < 0x80). When i >= len(s), the left conjunct short-circuits correctly, but the right disjunct s[i] > 0x7E evaluates out of bounds, causing a panic.

Failure scenario: Attacker embeds a truncated CSI sequence (\x1b[999 with no final byte 0x40–0x7E) in incident data (title, alert name). When the watcher processes this via watcherBuffer.Append → stripControl, the TUI crashes. This is an availability DoS against the SRE tool during an incident.

Mutation executed: Added test TestStripControl_CSINoFinalByte calling stripControl("\x1b[999"). Result: panic: runtime error: index out of range [5] with length 5 at watcher.go:472.

Fix: Add parentheses: for i < len(s) && (s[i] < 0x40 || (s[i] > 0x7E && s[i] < 0x80)) {

---

2. B4 bypass: Ask Title/Body in approvals strip not sanitized — CONFIRMED by reading

File: pkg/tui/approvals.go:135, pkg/tui/model.go:960-962

Bug: buildAskFromVerdict sets ask.Title = verdict.Summary and ask.Body = verdict.Action without calling stripControl. The RenderExpanded method at approvals.go:135 renders ask.Title directly via fmt.Sprintf. The B4 choke point (sanitization at watcherBuffer.Append/SetLast) does NOT cover this path — verdicts flow into Ask structs and are rendered without ever touching the watcher buffer.

Failure scenario: Attacker injects ANSI escapes into incident data that the AI includes in its verdict Summary (which becomes Ask.Title). When the user opens the approvals panel (A key), the terminal renders attacker-controlled escape sequences. An OSC-52 payload could write to the clipboard; CSI cursor-repositioning could repaint the strip to display a different action than what Accept will actually execute — the exact attack B4 was supposed to prevent.

Not mutation-tested (no existing test renders RenderExpanded with ANSI content and asserts clean output). The gap is structural: approvals.go contains zero calls to stripControl, and the data flow from investigationMsg.verdict → buildAskFromVerdict → Ask → RenderExpanded never passes through the sanitized buffer.

Fix: Apply stripControl to verdict.Summary and verdict.Action in buildAskFromVerdict before assigning to ask.Title/ask.Body. Or sanitize in approvalsStrip.Add.

---

3. PR body cites 3 nonexistent test names — CONFIRMED

The traceability table under B1 references:

• TestBuildAskFromVerdict_SnapshotsIncidentID → does not exist
• TestBuildAskFromVerdict_SnapshotsIncidentTitle → does not exist
• TestBuildAskFromVerdict_NilIncidentSafe → does not exist

Actual test names: TestBuildAskFromVerdict_DraftNote_TargetsOriginalIncident, TestBuildAskFromVerdict_Escalation_TargetsOriginalIncident, TestBuildAskFromVerdict_NilSelectedIncident_NoAction. Tests were renamed after the PR body was written. The PR body should be updated to match actual test names — this is the exact failure mode called out by the audit ("previous PR shipped a table citing a nonexistent test").

---

NICE-TO-HAVE

4. postAINoteCmd is dead production code — CONFIRMED

File: pkg/tui/model.go:1016-1031

Only called from approvals_update_test.go:109. Production code now uses postAINoteToIncidentCmd. The old function still reads m.selectedIncident at execution time (the B1 bug), so its presence is confusing and its use in TestUpdate_ApprovalsEnter_ReturnsCmdThatPostsNote is misleading — that test would not catch the B1 regression because it never changes selectedIncident after constructing the Ask.

Recommendation: Delete postAINoteCmd. Update TestUpdate_ApprovalsEnter_ReturnsCmdThatPostsNote to use buildAskFromVerdict or postAINoteToIncidentCmd with an explicit incident ID, or better yet, test the B1 scenario (change selection, accept, verify target).

5. extractToolRunnerFactory nil → downstream path untested at integration level

File: pkg/tui/investigation.go:70

Unit tests verify extractToolRunnerFactory returns nil for non-Anthropic providers, but no test exercises the consequence — the "tool runner or registry not configured" error path in watcherInvestigateCmd (line 70) is never reached by any test. The watcher integration tests set m.toolRunnerFactory = factory directly, bypassing the extraction function.

Risk: Low. The nil check is simple and correct by inspection. But it's exactly the "unwired code" pattern this project keeps hitting.

---

CLEAN

Area	Status	Evidence
B1 snapshot (all kinds)	✅ Mutation-tested	Stubbed ask.IncidentID assignment → TestBuildAskFromVerdict_DraftNote_TargetsOriginalIncident and TestBuildAskFromVerdict_Escalation_TargetsOriginalIncident both fail. Stubbed IncidentTitle → draft note test fails. All four AskKind branches (DraftNote, SuggestedCommand, EscalationSuggestion, default) tested.
B3 determinism	✅ 100/100	TestReadAgentSessionCmd_PrefersEventsOverDone ran 100× with zero failures. Test correctly reproduces the race (both channels ready simultaneously).
B7 ValidateUserFlags	✅ Each flag tested	All 13 denied flags have individual sub-tests that assert error on presence. Removing any flag from deniedFlags breaks exactly one sub-test.
B2/B8 spawn deadlock	✅ Race-clean	go test -race ./pkg/agent/... -count=20 passed. ctx.Done() and timer added to spawn select. Pipe cleanup on retry-as-resume path correct.
B5 stale stream	✅ Clean	watcherStreamDoneMsg now carries ch for identity check (line 654). Chunk handler already had this.
B9 Bedrock region	✅ 3 tests	Config, AWS_REGION, and AWS_DEFAULT_REGION paths all tested in factory_test.go.
Race detector	✅	go test -race passed for pkg/agent (20 runs) and pkg/tui (5 runs).
Golden snapshots	✅ Not affected	TestGolden_* all pass. No snapshot files changed (correct — approval strip is data-dependent, not in goldens).
PermissionAsk	✅ Left with comment	Present at pkg/agent/agent.go:17, handled at pkg/tui/claude.go:297 with // TODO(phase-2) comment.
watcherDedup.seen	✅ Not touched	git diff origin/main shows no changes to dedup/seen code.
Customer data in logs	✅ Fixed	index.go changed from "content", string(lastLine) to "len", len(lastLine). stream.go:121 logs at Debug level with provider name and error only.
Deadcode	✅ No new dead production code	SetTestChannels is test-only (expected). postAINoteCmd is dead (see finding #4).