feat(agent): session persistence, restart-resume, fire-and-return

[clcollins]

Summary

• Session persistence: JSONL index at ~/.config/srepd/sessions/index.jsonl (XDG-aware) records established sessions so restart-resume works across srepd restarts. Sessions are recorded only after system/init arrives — crash-before-init leaves no stale entry.
• Fake claude harness: Standalone Go binary (pkg/agent/testdata/fakeclaude/main.go) reproducing verified claude 2.1.220 session-ID semantics for integration testing.
• Integration tests: All assert on spawn-flag decision (--resume vs --session-id), never UUID equality. Covers restart-resume, absent index, per-incident isolation, crash-before-init, duplicate session-id, LRU eviction, send timeout, double-render prevention, crash mid-stream, and index robustness.
• L2 fix: startAgentSession now uses context.WithTimeout(30s) instead of context.Background() — a hung child returns an error instead of blocking forever.
• B1 fix: :agent <query> dispatches and returns to the queue immediately (fire-and-return), matching :watcher <query> precedent. Bare :agent still opens chat mode.
• Flag flip: agent_session_enabled default changed from false to true.
• Revert-check gate wired: TestRevertCheck_StubIndexWrite proves H1a/H1b tests have teeth.
• Dead code removed: Session.ID(), Session.IncidentID() (zero callers) deleted. ForTestStubIndexWrite and IndexEntryCount moved into _test.go.
• Docs updated: docs/ai-agents.md table row corrected (true, not false), privacy section updated with srepd's on-disk index details.
• N1: Documented ClaudeArgs last-token heuristic, why it exists, and the known limitation where a trailing path token can hide earlier flags.
• N2: record() now skips appending when the incident already has an established entry, bounding index growth.
• N3: Suppressed duplicate Init marker from subprocess — handleAgentSessionEvent now tracks agentSessionInitSeen.
• N4: Send releases s.mu before blocking channel selects and listens on the lifecycle context, preventing Close() from blocking. writeLoop exits via context cancellation instead of channel close to avoid data races.
• Timing-fidelity fix: The session-ID-in-use recovery (100ms timer) never fired in production — the real CLI takes 352–411ms to reject. Replaced with event-driven stdout peek: race first stdout byte against process exit. No delay on happy path, works regardless of CLI timing.

Test plan

• go test ./pkg/agent/... — all tests pass
• go test ./pkg/tui/... — all tests pass
• CGO_ENABLED=1 go test -race ./pkg/... — no races
• golangci-lint run — 0 issues
• gofmt -s -l cmd pkg — no formatting issues
• go vet ./... — clean
• deadcode ./... — no dead code in pkg/agent
• make test-fixtures — all fixture data sanitized
• make generate-quickstart — quickstart regenerated
• Plan doc at docs/plans/412-session-persistence.md

Acceptance criteria traceability

#	Criterion	Test function	File
H1a	Populated index → --resume	TestIntegration_RestartResume	pkg/agent/integration_test.go
H1b	Absent index → --session-id	TestIntegration_AbsentIndex_SessionID	pkg/agent/integration_test.go
H2	Per-incident isolation	TestIntegration_PerIncidentIsolation	pkg/agent/integration_test.go
L1	Establishment recorded only after init	TestIntegration_CrashBeforeInit	pkg/agent/integration_test.go
L2	Send honours a timeout	TestIntegration_SendHonorsTimeout	pkg/agent/integration_test.go
V1	Harness reproduces verified CLI semantics	TestIntegration_DuplicateSessionID	pkg/agent/integration_test.go
V2	Session-ID-in-use recovery (timing-faithful)	TestIntegration_SessionIDInUse_RetryResume	pkg/agent/integration_test.go
V3	Happy path not delayed by detection	TestIntegration_HappyPathNotDelayed	pkg/agent/integration_test.go
V4	No infinite retry	TestIntegration_SessionIDInUse_NoInfiniteRetry	pkg/agent/integration_test.go
V5	Unrelated failure — no retry	TestIntegration_OtherFailure_NoRetry	pkg/agent/integration_test.go
F1	Flag flips to true, revert-check-gated	TestRevertCheck_StubIndexWrite + TestResolveAgentSessionEnabled_DefaultTrue	pkg/agent/integration_test.go, pkg/tui/model_test.go
B1	:agent <query> fire-and-return	TestChatMode_AgentWithQueryFiresAndReturns + TestChatMode_BareAgentEntersChatMode	pkg/tui/model_test.go
N2	Index skips duplicate appends on re-establishment	TestIndex_NoDuplicateOnReEstablish	pkg/agent/integration_test.go
N3	Single Init marker per session start	TestHandleAgentSessionEvent_InitOnce	pkg/tui/claude_test.go
N4	Send does not block Close via held mutex	TestSession_SendDoesNotBlockClose	pkg/agent/session_test.go

Revert checks

Timing-fidelity revert check (V2 — the headline of this fix)

Temporarily restored the 100ms timer in spawn(), ran the timing-faithful
tests. Both FAIL because the 400ms rejection delay defeats the timer:

$ go test ./pkg/agent/... -run "TestIntegration_SessionIDInUse_RetryResume|TestIntegration_DuplicateSessionID" -count=1 -v
=== RUN   TestIntegration_DuplicateSessionID
Error: Session ID bbd65706-034f-59a7-85d8-825a7a0ce508 is already in use.
    integration_test.go:374: unexpected Error event: exit status 1
--- FAIL: TestIntegration_DuplicateSessionID (0.40s)
=== RUN   TestIntegration_SessionIDInUse_RetryResume
Error: Session ID bbd65706-034f-59a7-85d8-825a7a0ce508 is already in use.
    integration_test.go:884: unexpected Error event: exit status 1
--- FAIL: TestIntegration_SessionIDInUse_RetryResume (0.40s)
FAIL

Restored the event-driven fix, both tests pass again.

Index-write revert check (F1, H1a, H1b)

TestRevertCheck_StubIndexWrite calls ForTestStubIndexWrite() to disable
index file writes, then runs the restart-resume scenario. With writes stubbed,
the second manager has no on-disk index and falls back to --session-id
instead of --resume.

$ go test ./pkg/agent/... -run TestRevertCheck_StubIndexWrite -v
=== RUN   TestRevertCheck_StubIndexWrite
--- PASS: TestRevertCheck_StubIndexWrite (0.50s)
PASS

Send-blocks-Close revert check (N4)

Reintroduced defer s.mu.Unlock() (lock held across blocking select) and
confirmed TestSession_SendDoesNotBlockClose FAILS:

$ go test ./pkg/agent/... -run TestSession_SendDoesNotBlockClose -v -timeout 10s
=== RUN   TestSession_SendDoesNotBlockClose
    session_test.go:1014: Close() blocked — Send holds s.mu across blocking select
--- FAIL: TestSession_SendDoesNotBlockClose (2.10s)
FAIL

Restored the fix, test passes again.

Dead code gate

$ go run golang.org/x/tools/cmd/deadcode@latest ./... 2>&1 | grep pkg/agent
(no output — clean)

TestSession_CloseNoSpuriousError mock update

The TestSession_CloseNoSpuriousError mock's wait() was updated to block
until context cancellation, matching real exec.CommandContext behaviour.
The original mock returned immediately, which was invisible to the timer-based
code but incorrectly triggered the event-driven "process exited before stdout"
path. The test's intent is unchanged — it still verifies that Close() does
not produce a spurious Error event.

Deviations

B1 reverses plan 411. Plan 411 said :agent <query> "opens chat mode and
sends." This PR makes it fire-and-return instead — the query is dispatched and
control returns to the incident queue immediately. This is a deliberate,
maintainer-requested reversal based on live usage: a one-shot question should
not hijack the view. The implementation follows the :watcher <query> precedent
in the same file.

Testing this live

# 1. Build
make build

# 2. Launch in dev mode (ALWAYS --dev — the real binary makes live PagerDuty/OCM calls)
./dist/srepd_linux_amd64_v1/srepd --dev

# 3. Select an incident with arrow keys and press Enter

# 4. Fire-and-return test: press `:` and type `agent hello`, then Enter
#    EXPECT: input blurs, focus returns to the incident list immediately
#    EXPECT: status bar shows agent activity (spinning indicator)
#    EXPECT: response appears in the watcher pane when ready

# 5. Chat mode test: press `:` and type `agent` (bare, no query), then Enter
#    EXPECT: chat mode opens with a focused input pane
#    Type a message and press Enter
#    EXPECT: response renders with markdown formatting (bold headings, indented lists)
#    Press Esc → returns to incident list

# 6. Two-turn / multi-turn test (the headline behaviour this PR delivers):
#    Press `:` and type `agent what is 6 times 7?`, then Enter
#    Wait for the response in the watcher pane
#    Press `:` and type `agent what was my previous question?`, then Enter
#    EXPECT: the agent's response references "6 times 7" — proving session
#    context persists across queries for the same incident

# 7. Press Esc → returns to the incident queue

What will NOT work in --dev mode: PagerDuty and OCM API calls return
mock data, so incident enrichment (cluster info, service logs) is simulated.
The agent subprocess (claude) requires a real Claude Code install and
Anthropic API key.

Sources

• Plan 410a (verification amendment)
• Plan 412 (this PR's plan)
• Verified claude 2.1.220 session-ID behaviour (captured 2026-07-28)
• PR agent: persistent per-incident Claude Code sessions #414 review thread (L1, L2 defects)

🤖 Generated with Claude Code

[clcollins]

Security Review — PR #416: Session Persistence (pkg/agent/)

Adversarial security review of the persistent Claude Code CLI subprocess sessions added in this PR. Reviewed by reading the diff and source; no code was modified.

---

Finding 1 — Legacy spawn paths bypass flag denylist (MEDIUM)

File: pkg/tui/claude.go:108 (agentQuery) and pkg/tui/claude.go:502 (streamAgentCmd)

The new session path in pkg/agent/session.go:42-55 correctly validates agent_cli_command tokens against deniedFlags via validateUserFlags. However, the two legacy paths — agentQuery (blocking, line 108) and streamAgentCmd (streaming, line 502) — parse agent_cli_command with strings.Fields and pass the result directly to exec.CommandContext with no validateUserFlags call.

Exploit scenario: Set agent_cli_command: "claude --dangerously-skip-permissions --print" in config. When session mode is disabled (agent_session_enabled: false) or the session path is unreachable, the legacy path spawns Claude with --dangerously-skip-permissions, giving the agent full host access without operator approval.

Verified: Read both functions end-to-end; neither calls validateUserFlags or any equivalent. The session path at session.go:200 does call it.

Suggested fix: Apply validateUserFlags to the legacy agentQuery and streamAgentCmd functions, or refactor to share a single validated command-building function.

---

Finding 2 — Write goroutine leak on Send() context cancellation (MEDIUM)

File: pkg/agent/session.go:178-192

Send() spawns a goroutine to write to stdin (line 179). If the context is cancelled while the write is blocked (pipe full because child stopped reading), Send() returns at line 190-191, but the goroutine remains blocked on s.stdin.Write(data) until Close() is eventually called and breaks the pipe.

Exploit scenario: A hung child process stops reading stdin. The user (or the TUI) retries sending messages. Each timed-out Send() leaks one goroutine pinned on a blocked pipe write. Between the timeout and the eventual Close(), goroutines accumulate proportionally to retry attempts.

Verified: The goroutine at line 179-182 has no cancellation path — io.Writer does not accept a context. Close() at line 296+ does close s.stdin, which would unblock the writes, but there is no guarantee Close() is called promptly after a timeout.

Suggested fix: After the context-cancelled branch fires, close s.stdin (or the entire session) to unblock the leaked goroutine. Alternatively, set the session into a "draining" state that prevents further Send() calls and triggers Close().

---

Finding 3 — Index file: two Write calls not atomic (LOW)

File: pkg/agent/index.go:132-133

_, _ = f.Write(data)
_, _ = f.Write([]byte("\n"))

The data and newline are two separate Write calls. Although O_APPEND makes each individual write atomic on Linux (for sizes under PIPE_BUF), a crash or SIGKILL between the two calls produces a partial line without a trailing newline. The next append concatenates with this partial line, corrupting two entries.

Verified: The load function (index.go:60-82) tolerates corrupt lines by logging a warning and skipping them, so this is self-healing. However, combining both writes into one call is trivial and eliminates the window entirely.

Suggested fix: f.Write(append(data, '\n')) — single atomic append.

---

Finding 4 — Full parent environment inherited by child process (LOW)

File: pkg/tui/claude.go:118, claude.go:445, claude.go:523

All three spawn paths call os.Environ() and append incident metadata. The parent's full environment — including PAGERDUTY_TOKEN, ANTHROPIC_API_KEY, and any other credentials — is inherited by the child Claude CLI process and visible via /proc/<pid>/environ.

Mitigating factors: The child is a trusted binary (Claude CLI), and this is consistent with the existing codebase pattern (commands.go:815). No secrets appear in argv (visible in ps). Incident data passed via env vars is sanitized by sanitizeEnvValue.

Suggested fix: Consider filtering os.Environ() to exclude known sensitive vars (e.g., *TOKEN*, *SECRET*, *API_KEY*) before passing to the child, or document this as an accepted design decision. The Claude CLI likely needs ANTHROPIC_API_KEY, so at minimum document which vars are intentionally inherited.

---

Finding 5 — Full prompt logged at debug level (LOW)

File: pkg/tui/claude.go:120

log.Debug("tui.agentQuery()", "command", agentCLICommand, "prompt", prompt)

The full prompt (including system prompt and incident context) is written to the debug log at ~/.config/srepd/debug.log. This could include incident details, cluster names, and alert content. The info-level log at line 177 correctly truncates to 80 chars; the debug path does not.

Suggested fix: Truncate the prompt in the debug log the same way the info-level log does, or omit it.

---

Clean areas (verified, no issues)

• No shell involvement. All three spawn sites use exec.CommandContext with argv slices. No sh -c / bash -c / string concatenation into shell commands. Verified at session.go:67, claude.go:33, claude.go:517.

• --bare never in spawn args. BuildSpawnArgs (agent.go:215-238) never constructs --bare. The denylist at session.go:23-37 blocks it via agent_cli_command. Test TestBuildSpawnArgs_NeverBare covers 5 combinations; TestSpawn_RejectsDeniedFlags covers the injection path including --bare and --bare=true variants.

• AllowedTools / PermissionMode injection impossible. Both are passed as single argv elements (agent.go:230-236), not split. A value like "Bash --dangerously-skip-permissions" is passed as one argument to --allowedTools — the CLI treats it as a (likely invalid) tool name, not separate flags. Both flags are also in deniedFlags, preventing injection via agent_cli_command.

• SessionIDFor is safe. UUIDv5 from SHA-1 (agent.go:43-45) produces a well-formed UUID regardless of input. Path separators, NUL bytes, newlines, and very long strings all hash to a valid UUID. No path traversal possible.

• Index file permissions correct. Directory created with 0700 (index.go:103), file with 0600 (index.go:122). No world-readable state.

• JSON injection in index impossible. json.Marshal properly escapes special characters (index.go:117). Malicious incident IDs cannot inject fields.

• NDJSON line length bounded. Both session (session.go:253-254) and legacy (claude.go:561-562) paths set scanner.Buffer(make([]byte, 256*1024), 256*1024). A 300KB-line test (session_test.go:523-559) verifies the error surfaces correctly.

• LRU enforcement works. GetOrCreate (session.go:383-389) uses a for loop to evict down to maxLive before creating. Mutex-serialized. Cannot open unbounded subprocesses.

• Process reaping correct. WaitDelay = 2*time.Second set at session.go:71. Deferred wait() in readLoop reaps the child. Close() cancels context (SIGKILL) and closes stdin. Pipe cleanup on spawn failure covers all three error paths (session.go:67-88).

• Test fixtures clean. All UUIDs use fake markers (00000000-fake-0000-0000-000000000000). No real domains (openshiftapps.com, devshift.net, etc.), no tokens, no customer data. make test-fixtures glob (Makefile:247) covers *.ndjson files. fakeclaude/main.go contains only synthetic data.

• Error strings do not leak secrets. All fmt.Errorf calls in new code emit structural descriptions only. The body-stripping pattern from ai/classify.go is preserved at claude.go:303. Stderr from child is logged on error but contains only CLI diagnostics.

• TUI Update loop not blocked. All subprocess I/O runs in tea.Cmd goroutines, not in Update.

---

Summary

#	Finding	Severity	File:Line
1	Legacy spawn paths bypass validateUserFlags denylist	MEDIUM	claude.go:108, claude.go:502
2	Write goroutine leaked on Send() context cancellation	MEDIUM	session.go:178-192
3	Index write: data + newline in two calls (crash window)	LOW	index.go:132-133
4	Full parent env (incl. tokens) inherited by child	LOW	claude.go:118,445,523
5	Full prompt logged at debug level	LOW	claude.go:120

No critical-severity findings. The two medium findings are actionable — especially #1, which creates a config-driven path to --dangerously-skip-permissions in the legacy spawn functions.

[clcollins]

PR #416 Intent/Acceptance Review — Plan 412 (Session Persistence)

Reviewer mandate: verify every criterion by tracing the call path from user action to effect. "The function exists and has a test" is not evidence.

---

Verdict Table

#	Criterion	Verdict	Evidence
H1a	Populated index -> --resume	MET	GetOrCreate (session.go:394) checks m.index.has(incidentID) -> sets s.resumed = true -> BuildSpawnArgs(resume=true) (agent.go:224-225) -> --resume. Index loaded from disk by newSessionIndex -> idx.load() (index.go:42-83) using os.Open + json.Unmarshal. TestIntegration_RestartResume asserts argv[1] has --resume after a fresh manager loads the same sessionDir.
H1b	Absent index -> --session-id	MET	m.index.has() returns false -> s.resumed stays false -> BuildSpawnArgs(resume=false) (agent.go:227) -> --session-id. TestIntegration_AbsentIndex_SessionID confirms.
H2	Per-incident isolation	MET	Sessions keyed by incidentID in SessionManager.sessions map (session.go:323). Each Session has its own events chan, stdin pipe, done chan. No shared buffer or global. TestIntegration_PerIncidentIsolation asserts distinct --session-id values for two incidents.
L1	Index entry only after system/init	MET	onEstablished callback set in GetOrCreate (session.go:400-402), fired only when ev.Kind == Init in readLoop (session.go:264-267). TestIntegration_CrashBeforeInit uses crash_before_init script (fake exits before init), verifies no index file, then fresh manager uses --session-id.
L2	startAgentSession uses context.WithTimeout	MET	claude.go:456: ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second). TestIntegration_SendHonorsTimeout verifies a cancelled context returns error.
V1	Harness reproduces duplicate --session-id semantics	MET	fakeclaude/main.go:70-78: checks state dir for existing ID, writes stderr, exits 1, NO result line. TestIntegration_DuplicateSessionID pre-writes state file, asserts Error event surfaces without hang.
F1	agent_session_enabled defaults true, gated on revert check	PARTIAL	Default is true in DefaultOptionalKeys (config.go:42) and resolveAgentSessionEnabled() (model.go:318-323). BUT: the revert check is missing. ForTestStubIndexWrite() exists (index.go:149) but is never called from any test. Plan 412 explicitly requires: "Gate the flip on the revert check passing for both headline tests (1 and 2)." No test stubs the index write and verifies the headline test would fail without it. The gate is absent.
B1	:agent bare -> chat; :agent <query> -> fire-and-return	MET	msgHandlers.go:831-838: bare -> enterChatMode(); with query -> claudePromptMsg dispatched, no enterChatModeState(). Input blurred and table focused at lines 828-829 (before the isAgentCommand check). TestChatMode_AgentWithQueryFiresAndReturns asserts chatMode == false and cmd != nil.

Carried-forward Plan 411 checks

#	Criterion	Verdict	Evidence
411-1	Two consecutive :agent messages share one conversation	MET	handleClaudePrompt -> startAgentSession -> mgr.GetOrCreate(incidentID, env). On second call for same incident, GetOrCreate returns existing *Session (session.go:362-369), same subprocess.
411-2	Restart resumes prior conversation	MET	NewSessionManager -> newSessionIndex(cfg.SessionDir) -> idx.load() reads disk -> GetOrCreate -> index.has() returns true -> s.resumed = true -> --resume. Disk write: idx.record() calls os.MkdirAll(dir, 0700) (index.go:103) + `os.OpenFile(idx.path, O_CREATE
411-3	Per-incident isolation	MET	See H2 above.
411-4	Tool-use lines render; final markdown-rendered	MET	handleAgentSessionEvent (claude.go:221-292): ToolUse -> "⚙ <tool> <input>" line appended to watcher buffer (claude.go:254-256). Result -> glamour render via m.markdownRenderer.Render(ev.Text) (claude.go:272). ParseStreamEvent handles tool_use blocks in assistant messages (agent.go:148-152) — the headline bug fix from plan 411.
411-5	Non-claude agent_cli_command unchanged	MET	handleClaudePrompt gates session path on isClaudeCLI(agentCmd) (claude.go:192). Non-claude falls to legacy streaming (claude.go:202) or blocking fallback (claude.go:211). isClaudeCLI checks filepath.Base(field) == "claude" (claude.go:349).
411-6	Config keys exist, consumed, documented	MET	All four keys in DefaultOptionalKeys + OptionalKeys (config.go:42-43,58-61). Consumed: resolveAgentSessionEnabled(), resolveAgentMaxSessions(), resolveAgentAllowedTools(), viper.GetString("agent_permission_mode"). Documented in README table and docs/ai-agents.md.

Deadcode check

deadcode ./... (whole-project, all main packages): zero findings in pkg/agent. The ./cmd/... variant flags everything as unreachable due to Bubble Tea interface dispatch, which is expected. Production wiring confirmed: InitialModel (model.go:470) and InitialModelWithConfig (model.go:606) both call agent.NewSessionManager; handleClaudePrompt (claude.go:192-198) calls startAgentSession -> mgr.GetOrCreate. CloseAgentSessions (tui.go:2698) calls CloseAll.

The UUID-equality trap

All 10 integration tests assert on the --resume vs --session-id argv flag decision, never on UUID equality. Verified by reading every test: they use hasFlag(argv[n].Args, "--resume") and hasFlag(argv[n].Args, "--session-id"). No test compares session IDs for equality across managers. No false-green defect found on this axis.

---

Gaps (prioritized)

1. MISSING: Revert check (F1 incomplete) — HIGH

ForTestStubIndexWrite() is defined (index.go:149) but never called. Plan 412 requires: "Gate the flip on the revert check passing for both headline tests (1 and 2)." The expected pattern:

func TestIntegration_RevertCheck_RestartResume(t *testing.T) {
    // Same setup as TestIntegration_RestartResume, but:
    mgr1.ForTestStubIndexWrite()
    // ... run same flow ...
    // Assert second spawn does NOT use --resume (proving the index is load-bearing)
}

Without this, the agent_session_enabled: true default is ungated. The whole point of 410a was that dead persistence code passed CI; the revert check is the mechanism that prevents a regression. This is the exact failure class that caused the original rejection.

2. STALE DOCS: docs/ai-agents.md configuration table — MEDIUM

Line 157: | agent_session_enabled | false | ... Default false — session persistence not yet implemented; flips to true when per-incident resume lands. |

Line 165 example: agent_session_enabled: false

These directly contradict the code, which defaults to true (config.go:42, model.go:318-323). The description is copy-pasted from the PR (i) salvage and was never updated for the flag flip. A user reading the docs would think sessions are off by default.

3. MISSING: PR Deviations section — MEDIUM

Plan 412 spec requires: "Note the deviation from plan 411 in this plan's post-mortem and in the PR's ## Deviations section." B1 reverses plan 411's decision that :agent <query> opens chat mode. The PR description has no ## Deviations section. The plan doc docs/plans/412-session-persistence.md has no post-mortem or deviations record either.

4. MISSING: Plan doc post-mortem and UNVERIFIED resolutions — LOW

docs/plans/412-session-persistence.md is 99 lines with no post-mortem section. The plan spec records that --resume <unknown> behaviour is UNVERIFIED against the real CLI. This finding should be recorded in the plan doc per repo convention. The fakeclaude binary's comment at line 9 acknowledges it's unverified, but the plan doc should too.

5. B1 test coverage gap — LOW

TestChatMode_AgentWithQueryFiresAndReturns asserts chatMode == false and cmd != nil but does not assert m.input.Focused() == false or table focused. The code does blur/focus correctly (msgHandlers.go:828-829), but the test doesn't verify it. Minor — the assertions on chatMode and cmd are the load-bearing ones.

---

PR Description Claims vs Reality

Claim	Verified?
"All assert on spawn-flag decision, never UUID equality"	TRUE — confirmed by reading all 10 integration tests
"10 integration tests"	TRUE — counted 10 test functions in integration_test.go
"deadcode ./... — no dead code in pkg/agent"	TRUE — confirmed, zero findings from full-project deadcode
"L2 fix: context.WithTimeout(30s) instead of context.Background()"	TRUE — claude.go:456
"B1 fix: fire-and-return"	TRUE — msgHandlers.go:831-838, enterChatModeState() removed from query path
"Flag flip: default changed from false to true"	TRUE in code — FALSE in docs/ai-agents.md config table

Scope Discipline

No pkg/delta/, pkg/ai/tools/, pkg/ai/policy/, pkg/mcpserver/ in the diff. Clean. Only plan 411/412 scope.

Privacy Documentation

README (line 88) documents the session metadata storage path. docs/ai-agents.md (lines 216-223) has a substantive privacy note covering both srepd's index and Claude Code's own session storage. Adequate.

---

Summary

The PR genuinely delivers session persistence — the index is written to disk via os.MkdirAll/os.OpenFile, read back on manager construction, and the --resume vs --session-id decision is correctly driven by the on-disk state. This is NOT the false-green pattern from the prior rejection. The fake claude harness is well-designed and the integration tests are structurally sound.

One structural gap remains: the revert check (ForTestStubIndexWrite) that plan 412 and 410a require as the gate for the agent_session_enabled: true default is defined but never exercised. This is ironic given the PR's history — the revert check is the specific mechanism designed to prevent the exact failure class that caused the original rejection. The docs inconsistency in ai-agents.md (still says default false) compounds this: neither the test gate nor the documentation reflects the flag flip.

Recommendation: Add the revert check tests and fix the stale docs before merge. Everything else is solid.

[clcollins]

PR #416 Code Review — Session Persistence, Restart-Resume, Fire-and-Return

Must-Fix

1. CRITICAL: Context cancellation kills the child process after first Send

pkg/tui/claude.go:456-457 → pkg/agent/session.go:214

startAgentSession creates a 30-second timeout context and passes it to s.Send():

ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()
if err := s.Send(ctx, fullPrompt); err != nil { ... }
return agentSessionEventMsg{...}

On the first Send, spawn(ctx) derives the subprocess's lifetime context from this caller context:

spawnCtx, cancel := context.WithCancel(ctx)   // session.go:214 — child of the 30s ctx
s.ctx = spawnCtx
stdin, stdout, wait, err := s.exec.Start(spawnCtx, binPath, args, s.env)

When the tea.Cmd closure returns (immediately after Send writes to stdin), defer cancel() fires, cancelling the 30-second context. Go context propagation cancels spawnCtx. exec.CommandContext then kills the child process.

Failure scenario: Every session-based agent interaction kills the Claude process within milliseconds of the first message being sent. The readLoop sees s.ctx.Done(), exits, the events channel closes, the user sees an immediate done/error event. Multi-turn conversation is impossible.

Why tests miss this: All integration tests call s.Send(context.Background(), ...) directly, never going through startAgentSession. The production-only code path is untested.

Fix: spawn should derive spawnCtx from context.Background() (not from the caller's Send context), since the process lifetime must be independent of any single Send call:

spawnCtx, cancel := context.WithCancel(context.Background())

The Send context should only gate the stdin write (which it already does separately in lines 178-192).

---

2. Dead exported functions — test-only API surface (CONVENTIONS.md violation)

pkg/agent/index.go:138 — SessionManager.IndexEntryCount()
pkg/agent/index.go:149 — SessionManager.ForTestStubIndexWrite()

Both are exported but have zero production callers — only called from tests. This is the same dead-code pattern flagged in the PR's predecessors. Convention: test-only helpers should be in _test.go files (same package) or use unexported names.

Additionally:

• Session.ID() (session.go:134) — exported, zero callers anywhere (not even tests)
• Session.IncidentID() (session.go:139) — exported, zero callers anywhere
• SessionIDFor() (agent.go:43) — exported, only called internally + tests

Fix: Move IndexEntryCount and ForTestStubIndexWrite to a *_test.go file (exported test helpers in _test.go within the same package work fine). Remove ID() and IncidentID() if they have no planned consumer.

---

Nice-to-Have

3. Swallowed write errors in record() — silent data loss

pkg/agent/index.go:132-133

_, _ = f.Write(data)
_, _ = f.Write([]byte("\n"))

Both write errors are discarded. If the write fails (disk full), the in-memory map was already updated (line 96) but nothing persisted. On restart, the index misses this session → next spawn uses --session-id instead of --resume → duplicate session error.

The two separate Write calls also create a crash-atomicity window: a crash between them produces a line without a trailing newline. load() handles this via corrupt-line tolerance, but the concatenation of two entries into one line corrupts both.

Fix: Log the write errors. Combine into a single write: f.Write(append(data, '\n')).

4. Missing scanner.Err() check in load()

pkg/agent/index.go:75 (after the for scanner.Scan() loop)

If the scanner stops due to a read error (not EOF), the error is silently lost. The index is partially loaded, and the system proceeds with incomplete data.

Fix: Add if err := scanner.Err(); err != nil { charlog.Warn("agent.index.load", "error", err) } after the loop.

5. record() holds idx.mu during file I/O

pkg/agent/index.go:93-134

The lock is held through MkdirAll, OpenFile, Write, and Close. has() (called by GetOrCreate on the TUI thread) will block until the I/O completes. In practice, appending one line to a local file is sub-millisecond, but on a degraded filesystem this blocks the entire SessionManager (because GetOrCreate holds m.mu → calls has() → blocks on idx.mu).

Fix (if warranted): Update the in-memory map under the lock, release the lock, then do file I/O outside it.

6. ForTestStubIndexWrite writes idx.path without holding idx.mu

pkg/agent/index.go:149-153

Writes idx.path unsynchronized while record() reads it under idx.mu. Race detector would flag this if a test called it while a session's readLoop is processing an Init event.

Fix: Acquire idx.mu before writing idx.path.

---

Clean Areas

• go.mod hygiene: No changes to go.mod. No replace directives. google/uuid was already direct on main. Clean.
• No new lint suppressions: Zero //nolint directives in new code. golangci-lint passes with 0 issues.
• Build: make build succeeds.
• Race detector: pkg/agent passes with -race. (cmd failure is pre-existing on main; unrelated.)
• Type assertions: No bare .(T) assertions in the diff — comma-ok pattern used throughout.
• context.Context first parameter: Followed in all new functions.
• Commit hygiene: Genuine TDD history — failing tests committed first (61534f5), implementation second (725a2f7), remaining integration third (aa9868a).
• Plan document: Present at docs/plans/412-session-persistence.md with Context, Solution, Files Modified sections.
• Fire-and-return behavior (B1): Correct. enterChatModeState() properly removed from the :agent <query> path. The query dispatches via handleClaudePrompt which expands the watcher pane. Test updated to assert chatMode == false.
• Chat viewport ownership: Clean separation. m.chatViewport is independent from m.watcherViewport. No state bleed.
• Key dispatch ordering: Chat mode dispatched before chord/global keybindings — correct and documented.
• renderChatPane receiver: Value receiver, read-only — no mutations discarded.
• enterChatModeState() not dead: Still called by enterChatMode() and tests.
• Concurrency: Lock hierarchy (m.mu → s.mu / idx.mu) is consistent. onEstablished has a valid happens-before chain via the go statement in spawn(). done channel closed via sync.Once. All shared fields properly synchronized.
• Goroutine/pipe leaks: WaitDelay set at session.go:71. readLoop terminates on Close() via context cancellation. Stdin/stdout closed in Close() with nil-guards.
• Error wrapping: New errors use %w consistently.

[clcollins]

PR #416 Test Quality & Coverage Review

Reviewer goal: Find every remaining test that would still pass if its feature were deleted.

Context: This PR previously shipped tests (#414) that passed while the feature did not exist. The core sin was testing in-memory encode/decode with no production caller. This review repeats that audit empirically.

---

1. Mandatory Gates (Plan 410a)

Gate 2b — Traceability Matrix: MISSING ❌

The PR body contains a "Test plan" checklist of commands (go test, golangci-lint, etc.) but no acceptance-criterion → test-function → file mapping table. Plan 412 lists test labels (H1a, H1b, H2, L1, V1, etc.) but the PR description does not include the required table. The plan doc is not a substitute for the PR body requirement.

Gate 2c — Revert Check Statement: MISSING ❌

The PR body does not mention performing a revert check. ForTestStubIndexWrite() in index.go:147 exists as a hook for the revert check, but it is never called by any test (0% coverage, zero grep hits in *_test.go). It is dead code.

Gate 2d — Deadcode Gate: PASS ✅

go run golang.org/x/tools/cmd/deadcode@latest ./... reports no dead code in pkg/agent/. ForTestStubIndexWrite is a public method on SessionManager, so the tool cannot flag it (it's reachable from external callers), but it is effectively dead since no test or production code calls it.

Gate 2e — Flag Defaults to False: FAIL ❌

agent_session_enabled defaults to true, not false:

• pkg/config/config.go:41 — "agent_session_enabled": "true"
• pkg/tui/model.go:320 — resolveAgentSessionEnabled() returns true when not set

The plan doc (412) explicitly states "flag flipped from false to true." The 410a requirement says this salvage PR must default to false because session persistence is deferred. This is a rejection criterion.

---

2. Mutation Matrix

#	Mutation	Caught?	By which test(s)
M1	record() → no-op (index persistence disabled)	YES	TestIntegration_RestartResume (file existence + --resume flag)
M2	BuildSpawnArgs always uses --session-id (never --resume)	YES (4/5)	TestBuildSpawnArgs_Resume, TestIntegration_RestartResume, TestIntegration_LRUEvictionResume, TestSessionManager_CrashedSessionReplaced. Only TestSessionManager_LRUEviction survives (it tests eviction, not resume).
M3	BuildSpawnArgs always uses --resume (never --session-id)	YES (3/3)	TestBuildSpawnArgs_NewSession, TestIntegration_AbsentIndex_SessionID, TestIntegration_CrashBeforeInit
M4	Record index on spawn instead of after system/init	YES	TestIntegration_CrashBeforeInit (index non-empty after crash)
M5	SessionIDFor returns same UUID for all incidents	YES (2/2)	TestSessionIDFor_DistinctPerIncident, TestIntegration_PerIncidentIsolation
M6	Remove 30s timeout in startAgentSession (restore context.Background())	NO ❌	No test in pkg/tui/ covers startAgentSession (7.7% coverage). The L2 fix is untested at the TUI integration level.
M7	Fake claude accepts duplicate --session-id (no exit 1)	YES	TestIntegration_DuplicateSessionID (timeout catches missing error event)
M8	Re-add enterChatModeState() to :agent <query> path	YES	TestChatMode_AgentWithQueryFiresAndReturns in model_test.go:1831 (asserts chatMode == false)

Summary: 7 of 8 mutations killed. M6 survives — the L2 timeout fix has no test.

---

3. Coverage

Per-package

Package	Coverage
pkg/agent	86.2%
pkg/tui	78.7%
Total (all packages)	80.6% (threshold: 55%)

Notable uncovered branches in pkg/agent

Function	Coverage	Gap
ForTestStubIndexWrite	0%	Dead code — never called
record()	64%	Error paths for MkdirAll and OpenFile failures
Session.ID()	0%	Trivial getter, not a concern
Session.IncidentID()	0%	Trivial getter, not a concern
summarizeToolInput	60%	Fallback path (no description/command/file_path)

Notable uncovered branches in pkg/tui (session-related)

Function	Coverage	Gap
startAgentSession	7.7%	The L2 fix (30s timeout) is here — essentially untested
readAgentSessionCmd	12.5%	Session event reader loop barely covered
handleAgentSessionEvent	47.6%	Event handling partially covered
resolveSessionDir	0%	XDG_CONFIG_HOME resolution untested
resolveAgentMaxSessions	0%	Max sessions config resolution untested

---

4. Test Quality Audit

UUID-Equality Trap: CLEAR ✅

All persistence tests assert on the --resume vs --session-id flag decision from the fake's recorded argv. No persistence test asserts UUID equality. The TestSessionIDFor_Deterministic test asserts UUID equality but tests the SessionIDFor pure function, not persistence — this is correct.

Missing Tests

Gap	Severity	What to add
No concurrent GetOrCreate/Close test	Medium	SessionManager uses a mutex-guarded map and LRU order slice. No test hammers GetOrCreate and Close from multiple goroutines simultaneously. TestSessionManager_EvictionNoRace runs sequentially. Add a test that spawns 10 goroutines calling GetOrCreate with random incident IDs while another goroutine calls CloseAll.
startAgentSession timeout untested (M6)	High	The L2 fix (30s timeout) lives at claude.go:456 but startAgentSession has 7.7% coverage. A regression removing the timeout would go undetected.
ForTestStubIndexWrite dead code	Low	Either delete it or add the revert-check test that uses it. Currently it's a public API with zero callers.
resolveSessionDir untested	Medium	XDG_CONFIG_HOME resolution is at 0% coverage. Add table-driven test with t.Setenv.
No test for resolveAgentMaxSessions	Low	0% coverage on config resolution for max sessions.

Tests Asserting on Pure Helpers Without Production Callers

This was the #414 failure mode. I checked every function in pkg/agent/ for production callers:

Function	Has production caller?	Status
ParseStreamEvent	Yes (session.go:readLoop)	OK
EncodeUserTurn	Yes (session.go:Send)	OK
BuildSpawnArgs	Yes (session.go:spawn)	OK
SessionIDFor	Yes (session.go:NewSession)	OK
newSessionIndex / load / has / record	Yes (session.go:NewSessionManager, GetOrCreate)	OK
ForTestStubIndexWrite	NO	Dead code ❌
IndexEntryCount	Only from tests (integration_test.go:634)	Test-only accessor, acceptable but should be documented

ForTestStubIndexWrite is the only function reachable ONLY from tests (and even then, it's not called). IndexEntryCount is used in TestIntegration_IndexRobustness — it's a test accessor, which is acceptable.

Fixture Integrity

• raw-capture.ndjson: Sanitized (all UUIDs contain "fake", no real domains). Real capture structure.
• partial-capture.ndjson: Same sanitization. Contains stream_event deltas for double-render testing.
• fakeclaude/main.go: Not a fixture but a test harness. Documents which behaviors are verified vs unverified against real CLI.
• make test-fixtures: PASSES — globs both .json and .ndjson under testdata/.

Golden Snapshots

TestGolden_ChatMode passes. Box-drawing borders (╭╰│) close correctly. The 🤖 emoji renders properly in ASCII profile mode.

Race Detection

go test -race ./pkg/agent/... PASSES. However, as noted above, no test exercises concurrent GetOrCreate/Close, so the race detector has limited opportunity to find issues in the session manager's mutex-guarded state.

Flakiness Risk

• TestIntegration_RestartResume uses time.Sleep(500 * time.Millisecond) (line 156) to wait for the second process to start. This is a timing dependency that could flake under load.
• TestSession_CLICommandArgsPreserved uses time.Sleep(100 * time.Millisecond) (line 257). Same concern.
• No test spawns a real claude binary — all use the fake harness or mock executors. Good.

---

5. Suite Status

Check	Result
go test ./pkg/agent/...	PASS (all tests)
go test ./pkg/tui/...	PASS (all tests)
go test -race ./pkg/agent/...	PASS
gofmt -s -l	PASS (no formatting issues)
go vet ./...	PASS
make test-fixtures	PASS
TestGolden (all modes)	PASS
Coverage threshold (55%)	PASS (80.6%)

Note: go test ./cmd/... fails due to pre-existing config validation issues unrelated to this PR.

---

6. Verdict

Blockers (rejection criteria from 410a)

1. Gate 2e: agent_session_enabled defaults true, must be false. The plan says session persistence is being shipped but the 410a requirement says this salvage PR must default to false. The PR body says "Flag flip" and the plan doc confirms it. This contradicts 410a §2e.

2. Gate 2b: No traceability matrix in PR body. The plan doc lists test labels but the PR description lacks the required acceptance-criterion → test → file table.

3. Gate 2c: No revert check performed or stated. ForTestStubIndexWrite exists but is never called. The PR body does not mention performing the revert check.

High-Priority Findings

4. M6: L2 timeout fix untested. startAgentSession at claude.go:456 has 7.7% coverage. Removing the 30s timeout is undetectable by the test suite.

5. ForTestStubIndexWrite is dead code (0% coverage, 0 callers). Either delete it or implement the revert-check test that uses it.

Medium-Priority Findings

6. No concurrent GetOrCreate/Close stress test. The session manager is mutex-guarded but concurrent access is never tested.

7. time.Sleep in integration tests (RestartResume:156, CLICommandArgsPreserved:257) creates flakiness risk. Consider polling on a condition instead.

8. resolveSessionDir and resolveAgentMaxSessions at 0% coverage. These config resolvers are untested.

[clcollins]

Adversarial re-review of PR #416 — post-fix commits aa9868a..5b9b720

Reviewer: automated adversarial audit
Scope: 13 commits since last review, full feature coherence check

---

TL;DR

The five flagged fixes (F1–F5) all hold under adversarial testing. Three mutation tests confirmed that deleting each fix's implementation causes the corresponding tests to fail. No must-fix defects found. Two nice-to-have items below.

---

F1 — Critical bug (subprocess lifetime)

Verdict: FIXED. Verified by running.

spawn() now derives spawnCtx from s.lifecycleCtx (manager-scoped, cancelled by CloseAll), not the caller's Send context. session.go:231

Verified:

• (a) Mutation test: changed context.WithCancel(s.lifecycleCtx) → context.WithCancel(ctx) and ran TestSession_CallerCancelDoesNotKillProcess — failed immediately with "session must still be alive after caller context cancel." Restored; test passes.
• (b) Reaping: TestSessionManager_CloseAllReapsChildren confirms that CloseAll() still kills children (two sessions both reach Done() within 3s).
• (c) L2 timeout: TestIntegration_SendHonorsTimeout and TestSession_SendHonorsContext both pass — Send with a cancelled/timed-out context returns promptly. The 30s timeout in startAgentSession (claude.go:459) bounds the write wait.
• (d) The contextAwareExecutor (session_test.go:897) was purpose-built for this fix — it respects context cancellation, unlike the older mocks that discarded it. The regression test genuinely exercises the bug's failure mode.

F2 — --bare denylist bypass

Verdict: FIXED. Verified by running.

ValidateUserFlags(ClaudeArgs(fields)) is called once in handleClaudePrompt (claude.go:172) before the three-way dispatch, and redundantly in spawn() (session.go:222).

Bypass attack results (18 vectors tested):

• All direct --bare, --bare=true, --bare=1, --session-id=abc, -c, -r forms: BLOCKED
• -- separator then --bare: BLOCKED (overly strict, but correct for security)
• Wrapper commands (toolbox run claude --bare, flatpak-spawn --host claude --bare): BLOCKED — ClaudeArgs scans backwards for last claude basename
• Non-claude binary (/usr/bin/claude-wrapper --bare): BLOCKED — basename is not "claude" so ClaudeArgs falls back to fields[1:]
• Double claude token (claude --bare claude --print): PASS-THROUGH — correct by design, --bare is a wrapper arg
• Unicode lookalike (cyrillic "а"): binary wouldn't match claude or isClaudeCLI, so it takes the blocking fallback path. No bypass.
• Mutation test: set ValidateUserFlags to always return nil — both TestSpawn_RejectsDeniedFlags (22 subtests) and TestHandleClaudePrompt_RejectsDeniedFlagsAllPaths (21 subtests across all 3 paths) failed. Tests have teeth.

F3 — Write goroutine leak → writeLoop

Verdict: FIXED. Verified by reading + race detector.

Per-Send goroutines replaced with a single writeLoop per session (session.go:253-257). Concurrency analysis:

• Send on closed channel: Impossible. Send holds s.mu and checks s.closed before touching s.writeCh. Close holds s.mu when closing s.writeCh and sets s.closed = true. Mutual exclusion prevents racing.
• Close racing writeLoop: Safe. writeLoop captures stdin by value (parameter, not field access — session.go:253), so Close setting s.stdin = nil doesn't affect the writeLoop's reference. close(s.writeCh) causes range s.writeCh to exit cleanly.
• Deadlock if consumer stops draining: Not possible. writeLoop writes to the child's stdin, not to the events channel. If stdin blocks (pipe full), writeLoop blocks, but Send's timeout context unblocks the select on s.writeCh. CloseAll cancels lifecycleCtx, which kills the child via exec.CommandContext, eventually closing the pipe and unblocking writeLoop.
• Goroutine leak on eviction: Close calls close(s.writeCh), which unblocks writeLoop. s.cancel() kills the child, which unblocks readLoop. Both exit cleanly.
• Double-close: Close nil-guards s.writeCh before closing and sets it to nil after. Idempotent under mutex.
• -race detector: passes cleanly.
• TestSession_WriteGoroutineDoesNotLeak: confirms goroutine count stays flat across 5 timed-out sends.

F4 — Index hygiene

Verdict: FIXED. Verified by reading.

• Atomic write: f.Write(append(data, '\n')) — single call, well under PIPE_BUF (4096), O_APPEND ensures atomicity. index.go:142
• scanner.Err() checked: index.go:76. Confirmed by TestIndex_ScannerErrChecked.
• Narrowed lock — no TOCTOU: record() updates the in-memory map under idx.mu, releases lock, then does file I/O. Two concurrent record() calls both update the map atomically (last writer wins in memory) and both append to the file (last entry wins on reload at index.go:73). Consistent.
• has() cannot observe torn state: has() reads the in-memory map under idx.mu. The map is always updated atomically before the lock is released by record(). index.go:88-93
• Mutation test: making record() a no-op causes TestIntegration_RestartResume to fail. The persistence path has teeth.

F5 — Revert-check gate

Verdict: FIXED. Verified by running.

ForTestStubIndexWrite is in index_test_helpers_test.go (test-only, not compiled into production). TestRevertCheck_StubIndexWrite (integration_test.go:710) stubs index writes, then asserts that the second manager uses --session-id (not --resume). This is the inverse of TestIntegration_RestartResume, proving that test has teeth.

---

410a gates

• deadcode: go run golang.org/x/tools/cmd/deadcode@latest ./... — no reachable-only-from-tests code in pkg/agent.
• Test names: all test names cited in the PR body confirmed present via grep -rn 'func Test' --include='*_test.go'.
• agent_session_enabled defaulting to true: CORRECT per 410a rule 3f. Not re-reported.
• go test ./pkg/agent/... ./pkg/tui/...: all pass.
• go test -race ./pkg/agent/... ./pkg/tui/...: all pass.
• golangci-lint run: 0 issues.

---

Systemic test weakness — mock executors that discard context

This was the root cause of the original critical bug surviving: mockStreamExecutor.Start(_ context.Context, ...), argCapturingExecutor.Start(_ context.Context, ...), and customStdinExecutor.Start(_ context.Context, ...) all discard context. These mocks still exist and are still used by many tests.

The contextAwareExecutor was correctly added for the F1 regression test, but the older mocks remain. Any future test that uses mockStreamExecutor or argCapturingExecutor will be structurally blind to context-lifetime bugs.

Not a must-fix (the critical regression test uses the right executor), but worth being aware of for future test development.

---

Mutation test results summary

Fix	Mutation	Test that caught it	Result
F1 (lifecycleCtx)	Revert to context.WithCancel(ctx)	TestSession_CallerCancelDoesNotKillProcess	FAILED as expected
F2 (ValidateUserFlags)	Return nil always	TestSpawn_RejectsDeniedFlags (22 subtests), TestHandleClaudePrompt_RejectsDeniedFlagsAllPaths (21 subtests)	FAILED as expected
F4 (index.record)	Make record() a no-op	TestIntegration_RestartResume	FAILED as expected

All three mutations were detected. No persistence test uses UUID equality — they all assert on --resume vs --session-id flag decisions.

---

Nice-to-have findings

N1: Double Init event in TUI (cosmetic)

startAgentSession (claude.go:464) returns a synthetic agent.Event{Kind: agent.Init} immediately on successful Send. The real Init event from the subprocess arrives later via readAgentSessionCmd. handleAgentSessionEvent processes Init by appending an empty marker line to the watcher buffer (claude.go:246). Result: two marker lines per session start — a cosmetic double-blank.

file: pkg/tui/claude.go:464
Suggested fix: either don't return a synthetic Init (return a different event type like a "send succeeded" msg), or deduplicate Init events in handleAgentSessionEvent.

N2: Send holds mutex across blocking channel operations

Send (session.go:171) holds s.mu for its entire duration, including the select on s.writeCh (line 197-201) and errCh (line 203-211). If writeLoop blocks on stdin.Write (child stdin pipe full), Send blocks on s.writeCh <- req while holding s.mu, preventing Close() from acquiring the lock.

This is not a deadlock because:

• The caller (startAgentSession) uses a 30-second timeout context
• CloseAll cancels the manager context before calling Close, which kills the child via exec.CommandContext, eventually unblocking the pipe

But calling Send(context.Background(), ...) on a session whose child has stopped reading stdin would block Close() indefinitely. Since the only current caller uses a timeout, this is theoretical.

file: session.go:171-212
Suggested fix: release s.mu before the channel selects (capture writeCh by value under the lock, then release). Or document the context.Background() hazard.

---

Must-fix findings

None.

---

Conclusion

All five fixes hold. The test suite has teeth — three mutation tests independently confirmed that deleting each implementation causes the corresponding tests to fail. No test uses UUID equality for persistence assertions. The denylist blocks all 18 bypass vectors attempted. Race detector and linter are clean. The PR is ready to merge.