feat: claude runner persists partial output on termination

[bborbe]

What

Claude runner (agent/claude) captures streamed assistant text during a review and returns it as ClaudeResult.Partial when a run terminates before completing (deadline, non-zero exit, missing result).

Why

Fixes the large-PR review failure (moss PR #1): the LLM investigation burns the job deadline, the partial review output is discarded, and the task escalates with a blank needs_input. This is the library half — the github-pr-review-agent soft-budget + salvage spec (pr-reviewer-soft-time-budget-and-salvage) consumes Partial to persist ## Salvage sections.

Changes

• claude/claude-result.go: add Partial string json:"partial,omitempty"
• claude/claude-runner.go: accumulate streamed assistant text; return it on kill/deadline instead of zeroing; bounded ≥16KiB most-recent-kept
• claude/claude-runner_test.go: kill-path + context-cancel + envelope-negative + bounded-capture specs
• CHANGELOG.md: Unreleased entry

Companion: github-pr-review-agent feature/pr-reviewer-salvage (dep bump will follow this release).

Dark-factory spec: specs/in-progress/049-claude-runner-persists-partial-output.md

[ben-s-pull-request-reviewer]

Now I have all the information needed. Let me compile the comprehensive review report.

---

PR Review: feat: claude runner persists partial output on termination

Toolchain Status

• ast-grep: NOT available in PATH — cannot run mechanical funnel
• selector-mode-guide.md: NOT found — selector adjudication path blocked
• Mechanicals: Pre-computed results from prior funnel run injected via context; used as-is
• Suite file: claude/claude_suite_test.go exists — test discovery OK

---

Must Fix (Critical)

• claude/claude-runner.go:52-84 — Run() calls package-level functions directly (errors.Wrap, cmd.StdoutPipe(), cmd.Start(), cmd.Wait(), strings.Join, errors.Wrapf, errors.New) rather than through injected interfaces. This makes the method untestable and hides its dependency graph. Per go-composition/no-package-function-calls-in-business-logic, each capability should be wrapped in a small interface and injected. (rule: go-composition/no-package-function-calls-in-business-logic)

• claude/claude-runner.go:118-119 — exec.CommandContext(ctx, "claude", args...) is a boundary call with no corresponding audit log line. Add a log statement summarizing method + args + working directory before or after the call. (rule: go-logging/external-call-logs-response)

• claude/claude-runner.go:24 — Untyped string constant tailJoiner = " | " in a const block is an enum value without a paired typed newtype and Available* collection. Declare type tailJoiner string + var AvailableTailJoiners so the closed set is type-checked. (rule: go-enum-type/typed-constants-with-collection)

---

Should Fix (Important)

• claude/claude-runner.go:174 — The inner for _, c := range event.Message.Content loop (line 174–182) has no ctx.Done() check. Add a select { case <-ctx.Done(): return resultText, usage, string(partial), tail; default: } at the top of the loop body. (rule: go-context/cancel-check-in-loop)

• claude/claude-runner.go:276 — Same issue: for _, c := range event.Message.Content loop (line 276–284) lacks ctx.Done() check. (rule: go-context/cancel-check-in-loop)

• claude/claude-runner.go:308 — for _, k := range []string{...} in buildSubprocessEnv (Layer 1 allowlist pass-through) has no ctx.Done() check. While the loop is fast, buildSubprocessEnv itself is called during command setup before the subprocess starts — cancellation should stop env building early. (rule: go-context/cancel-check-in-loop)

• claude/claude-runner.go:331 — for k, v := range r.config.Env (Layer 3 consumer overrides) lacks ctx.Done() check. (rule: go-context/cancel-check-in-loop)

• claude/claude-runner.go:337 — for k, v := range env (conversion to []string) lacks ctx.Done() check. (rule: go-context/cancel-check-in-loop)

• claude/claude-runner.go:234-285 — The main for scanner.Scan() loop in scanOutput correctly checks ctx.Done() at line 236, but the inner for _, c := range event.Message.Content at line 277 and the nested loops for tail/partial building (lines 245, 275) do not propagate the cancellation check. The early return on ctx.Done() at line 238 is correct but only fires between scanner.Scan() iterations — not mid-iteration when processing content.

• claude/claude-runner.go:116, 125, 129, 136 — Multiple glog.V(N).Infof calls in buildCommand and scanOutput use unstructured glog (Go 1.21+ projects should migrate to log/slog). This is an existing-project exemption per the rule, but a future migration is recommended. (rule: go-cli/slog-not-glog-in-new-projects, exempt but noted)

---

Nice to Have (Optional)

• claude/claude-runner.go:161-167 — appendPartial performs a full slice copy on every overflow event (when len(partial) > partialMaxBytes). For sustained high-volume streams that frequently exceed the cap, this is O(n) copy per event. A ring-buffer approach using an array of fixed capacity + head index would be O(1) per append. Low priority — the 16 KiB cap limits damage, and this is a hot-path micro-optimization. (rule: go-performance/avoid-slice-copy-on-overflow, implied)

• claude/claude-runner_test.go:644-658 — The "streaming more assistant text than the cap" test fixture (302 shell-script lines) is fragile: if line lengths or count change, the boundary assertion len(result.Partial) >= 16384 may no longer hold. Consider adding a comment explicitly calculating the expected total bytes to document the 16384-byte threshold.

---

Traceability

Mechanical funnel findings applied as-is from pre-computed JSON (ast-grep runner was not re-executed in this session):

Owner	MUST	SHOULD
go-architecture-assistant	26	0
go-context-assistant	1	5
go-quality-assistant	3	2
go-security-specialist	4	0
go-test-quality-assistant	5	0

Note: The go-architecture-assistant findings on exec.Command, os.LookupEnv, os.Getenv, errors.Wrap, glog, bytes.Buffer, envparse — these are the expected platform-level dependencies for a subprocess runner and are architectural in nature. They are inherited from the existing design (the PR adds partial capture to an already-structured runner). The new Partial field itself introduces no new architectural violations beyond what already existed.

Selector mode skipped: selector-mode-guide.md not found in plugin path. Selector adjudication could not run. This review is based solely on mechanical findings + manual code review.

precommit skipped (selector mode) — CI covers lint+test.

---

Plan Concerns — Addressed or Raised

Concern	Status
correctness: scanOutput returns partial on ctx.Done() but Run() discards it when resultText is empty	Addressed — Run() lines 81–85 return ClaudeResult{Partial: partial} alongside the error, correctly surfacing partial on the cancellation path
correctness: appendPartial unbounded capacity	Not an issue — append grows the slice, then partial[len(partial)-partialMaxBytes:] takes the tail, so no bytes are dropped on reallocation
correctness: malformed JSON scanner drops lines	Not an issue — two-pass unmarshal with continue on error is intentional graceful degradation per spec
performance: O(n) slice copy on overflow	Noted — appendPartial copies on overflow; ring-buffer would be O(1) but is low priority given 16 KiB cap
security: tool_use content intentionally excluded from partial	Correct by design — capturePartial only captures type == "text" content blocks; tool_use payloads are excluded per spec 049 and the inline comment at line 169–172
tests: 16384-byte boundary test fragile	Noted — test comment documents the requirement; could be hardened with explicit byte-count comment

---

Verdict

{
"verdict": "request-changes",
"summary": "The partial output persistence feature is correctly implemented and the core logic (bounded capture, cancellation path, result-survival on schema drift) is sound. However, there are 4 MUST-tier findings — direct package calls in Run() breaking testability, a missing boundary log for the subprocess exec, an untyped string constant violating the enum pattern, and ctx.Done() gaps in inner loops — that need to be addressed before merge.",
"comments": [
{
"file": "claude/claude-runner.go",
"line": 52,
"severity": "critical",
"message": "Run() calls errors.Wrap, cmd.StdoutPipe(), cmd.Start(), cmd.Wait(), strings.Join, errors.Wrapf, errors.New directly — hidden dependencies. Wrap each in an interface and inject."
},
{
"file": "claude/claude-runner.go",
"line": 118,
"severity": "critical",
"message": "exec.CommandContext boundary call has no audit log. Add log: method + args + working directory before or after."
},
{
"file": "claude/claude-runner.go",
"line": 24,
"severity": "critical",
"message": "Untyped string constant tailJoiner lacks a paired typed newtype and AvailableTailJoiners collection. Violates go-enum-type-pattern."
},
{
"file": "claude/claude-runner.go",
"line": 174,
"severity": "major",
"message": "Inner for _, c := range event.Message.Content loop lacks ctx.Done() check."
},
{
"file": "claude/claude-runner.go",
"line": 276,
"severity": "major",
"message": "Inner for _, c := range event.Message.Content loop (inside scanOutput) lacks ctx.Done() check."
},
{
"file": "claude/claude-runner.go",
"line": 308,
"severity": "major",
"message": "for _, k := range []string{...} in buildSubprocessEnv lacks ctx.Done() check."
},
{
"file": "claude/claude-runner.go",
"line": 331,
"severity": "major",
"message": "for k, v := range r.config.Env lacks ctx.Done() check."
},
{
"file": "claude/claude-runner.go",
"line": 337,
"severity": "major",
"message": "for k, v := range env lacks ctx.Done() check."
},
{
"file": "claude/claude-runner.go",
"line": 161,
"severity": "nit",
"message": "appendPartial copies the full slice on every overflow. A ring-buffer (fixed array + head index) would be O(1) vs O(n). Low priority given 16 KiB cap."
}
],
"concerns_addressed": [
"correctness: scanOutput returns partial on ctx.Done() but Run() discards it when resultText is empty — addressed: Run() correctly returns ClaudeResult{Partial: partial} at lines 81-85",
"correctness: appendPartial uses slice of unbounded capacity — not an issue: append grows then truncates tail, no bytes dropped on reallocation",
"correctness: malformed JSON causes scanner to drop lines — not an issue: two-pass unmarshal with continue on error is intentional graceful degradation per spec",
"performance: O(n) slice copy on overflow — acknowledged: appendPartial copies on overflow; ring-buffer would be O(1) but low priority",
"security: tool_use content excluded from partial — correct by design: capturePartial only captures type==text blocks per spec 049",
"tests: 16384-byte boundary test fragile — acknowledged: test comment documents requirement; could add explicit byte count"
]
}

[ben-s-pull-request-reviewer]

I have all the information needed. The diff is empty because PR #48 (feature/pr-reviewer-salvage) was already merged to master — I am reviewing the resulting commit on master.

---

PR Review: feat: claude runner persists partial output on termination

Verdict: approve

Summary: The implementation correctly surfaces a bounded (16 KiB) most-recent-kept partial of streamed assistant text on all termination paths (SIGKILL, context cancellation, non-zero exit, missing result event). The Partial field is additive with omitempty, the ClaudeRunner interface is untouched, and all existing tests pass. The three concerns from the plan are all resolved correctly.

---

Must Fix (Critical)

None.

---

Should Fix (Important)

None.

---

Nice to Have (Optional)

• claude/claude-runner_test.go:688 — The "successful exit with streamed assistant text" spec asserts that Partial contains the streamed text on a clean exit 0, but does not assert that Partial is non-nil. Since Partial is string (not *string), it is always non-nil (zero-value ""), so this is cosmetic only. Consider adding Expect(result.Partial).NotTo(BeEmpty()) to make the intent explicit and serve as a regression guard if the field type ever changes.

---

Traceability

Concern	Resolution
claude/claude-runner.go: bounded ≥16 KiB most-recent-kept — verify ring-buffer or truncation logic correctly preserves latest output	appendPartial (line 166) appends then takes partial[len(partial)-partialMaxBytes:] on overflow — correct most-recent-kept semantics; comment at line 162 justifies the O(n) copy at this cap size
claude/claude-runner.go: streamed text accumulation on kill/deadline — ensure no race between streaming and termination	scanOutput returns string(partial) on ctx.Done() (line 243) — the zeroing branch was removed; the non-zero-exit path (line 76) also returns partial
claude/claude-result.go: Partial field omitempty — verify zero-value (empty string) is semantically correct when run completes normally	"" is the correct zero-value for a bounded capture field; omitempty correctly elides it from JSON when empty; on success the partial is populated with any streamed text (test line 692 confirms)

---

Notes

• appendPartial (line 166–172): The slice-copy truncation is correct. append may allocate a new backing array when partial is at capacity; on overflow the partial[len(partial)-partialMaxBytes:] slice is a new slice of exactly 16 KiB — the most recent bytes are kept, the oldest dropped. The O(n) copy on overflow is bounded at 16 KiB and trivially fast. The comment correctly defends the choice against a ring-buffer alternative.
• ctx.Done() path (line 241–244): Previously this branch returned empty values, discarding all captured output — that was the core bug. The fix is correct.
• CHANGELOG: ## Unreleased (line 11) is the top section (above v0.81.3), and line 13 correctly describes the feature with "partial output can be salvaged instead of lost".
• Scope containment: Only claude/, CHANGELOG.md, and specs/ files were touched — no other package.
• make generate: The //counterfeiter:generate on the interface line produces a mock that is identical before and after since the interface is unchanged.

{
"verdict": "approve",
"summary": "Feature correctly implemented: bounded (16 KiB, most-recent-kept) partial capture surfaced on all termination paths, additive to ClaudeResult with omitempty, interface unchanged, existing tests pass, scope contained to claude/ package.",
"comments": [
{
"file": "claude/claude-runner_test.go",
"line": 688,
"severity": "nit",
"message": "Success-path spec asserts Partial contains streamed text but does not assert it is non-empty. Consider adding Expect(result.Partial).NotTo(BeEmpty()) as a regression guard."
}
],
"concerns_addressed": [
"correctness: bounded 16 KiB most-recent-kept — appendPartial correctly takes last partialMaxBytes on overflow",
"correctness: streamed text accumulation on kill/deadline — ctx.Done() path returns partial instead of zeroing it",
"correctness: Partial omitempty — empty string is semantically correct zero-value; omitempty correctly elides it from JSON"
]
}