feat: stream-driven progress for explain --generate#964
feat: stream-driven progress for explain --generate#964peyton-alt wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR upgrades entire explain --generate to use Claude CLI streaming output for incremental progress reporting, and introduces a TTY-gated deadline strategy (no deadline on interactive TTY by default; idle-based safety deadline in non-TTY).
Changes:
- Add a streaming text-generation capability (
StreamingTextGenerator) and implement it for the Claude Code agent using--output-format stream-jsonwith an NDJSON stream parser. - Thread streaming progress events through summarization and render them in
explainviasummaryProgressWriter(TTY in-place updates vs. line-per-event for non-TTY/accessible mode). - Replace the prior fixed wall-clock timeout behavior with a resolver-driven deadline policy plus an idle watchdog for non-TTY default behavior.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/entire/cli/explain.go | Adds progress rendering, deadline resolution, idle watchdog, and transcript-size preflight message for explain --generate. |
| cmd/entire/cli/explain_test.go | Updates and adds tests for progress writer, idle watchdog, and deadline policy behavior. |
| cmd/entire/cli/summarize/summarize.go | Extends GenerateFromTranscript with an optional progress callback and threads it into Claude generation. |
| cmd/entire/cli/summarize/summarize_test.go | Updates call sites for new signature and adds tests for progress callback threading. |
| cmd/entire/cli/summarize/claude.go | Prefers streaming generation when available and forwards progress callback into the agent. |
| cmd/entire/cli/summarize/claude_test.go | Adds test coverage for streaming preference/fallback in ClaudeGenerator. |
| cmd/entire/cli/strategy/manual_commit_condensation.go | Updates summarization call to match new GenerateFromTranscript signature. |
| cmd/entire/cli/agent/agent.go | Introduces progress phase/types and the StreamingTextGenerator interface. |
| cmd/entire/cli/agent/capabilities.go | Adds capability detection helper AsStreamingTextGenerator and declared-cap field. |
| cmd/entire/cli/agent/capabilities_test.go | Adds tests validating AsStreamingTextGenerator behavior. |
| cmd/entire/cli/agent/claudecode/generate.go | Switches Claude Code generation to streaming NDJSON parsing and emits progress callbacks. |
| cmd/entire/cli/agent/claudecode/response.go | Adds NDJSON stream event types and streamClaudeResponse parser. |
| cmd/entire/cli/agent/claudecode/response_test.go | Replaces legacy response parsing tests with stream parser tests using fixtures. |
| cmd/entire/cli/agent/claudecode/claude_test.go | Adds streaming-generation tests and delegation test for GenerateText. |
| cmd/entire/cli/agent/claudecode/testdata/stream_success.jsonl | Adds real-world success fixture for stream-json NDJSON. |
| cmd/entire/cli/agent/claudecode/testdata/stream_error_404.jsonl | Adds error fixture (is_error:true, 404) for stream-json NDJSON. |
| case <-ticker.C: | ||
| ts := lastActivity.Load() | ||
| if ts > 0 && time.Since(time.Unix(0, ts)) >= idle { | ||
| fired.Store(true) | ||
| cancel() | ||
| return | ||
| } | ||
| // ts == 0 means no activity yet; keep waiting. |
There was a problem hiding this comment.
startIdleWatchdog treats lastActivity == 0 as “no activity yet; keep waiting”, which means the watchdog will never fire if the subprocess produces no stream events at all (the default non-TTY safety deadline becomes ineffective). Consider initializing lastActivity to time.Now() before starting the watchdog, or treating ts==0 as “idle since start” and canceling after idle elapses.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 33a095c. Configure here.
| Type string `json:"type"` // "message_start" | "content_block_delta" | "message_delta" | ... | ||
| Delta *StreamDelta `json:"delta,omitempty"` | ||
| Message *StreamMessage `json:"message,omitempty"` | ||
| TTFTms int `json:"ttft_ms,omitempty"` |
There was a problem hiding this comment.
TTFTms field mapped to wrong JSON nesting level
Medium Severity
The TTFTms field is defined on StreamInnerEvent (the nested event JSON object), but the real Claude CLI output places ttft_ms at the outer NDJSON line level alongside session_id and uuid. The captured fixture testdata/stream_success.jsonl confirms this: ttft_ms sits at the top level, not inside event. Since StreamEvent has no TTFTms field, the value is silently dropped during deserialization, so ev.Event.TTFTms is always 0 in production. The synthetic test fixture in claude_test.go incorrectly puts ttft_ms inside event, masking the bug.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 33a095c. Configure here.
| cancel() | ||
| return | ||
| } | ||
| // ts == 0 means no activity yet; keep waiting. |
There was a problem hiding this comment.
Idle watchdog never fires without initial stream activity
Medium Severity
The idle watchdog only fires when ts > 0, but lastActivity starts at its zero value (0). If the Claude CLI subprocess hangs before producing any stream events, lastActivity is never updated, so the watchdog never cancels the context. In the non-interactive (CI) code path — where the idle watchdog is the sole timeout mechanism — this causes the process to hang indefinitely. The old code had a hard 30-second wall-clock default that would have caught this.
Reviewed by Cursor Bugbot for commit 33a095c. Configure here.
| timeoutDuration = time.Until(deadline) | ||
| func generateCheckpointAISummary(ctx context.Context, errW io.Writer, scopedTranscript []byte, filesTouched []string, agentType types.AgentType) (*checkpoint.Summary, error) { | ||
| // Create progress writer so the user sees phase updates on stderr. | ||
| progressWriter := newSummaryProgressWriter(errW) |
There was a problem hiding this comment.
Nil errW causes panic in progress writer
Medium Severity
newSummaryProgressWriter(errW) is called unconditionally, but the caller generateCheckpointSummary has an existing if errW != nil guard (line 488), indicating errW can be nil. When errW is nil and progress events arrive, handle calls fmt.Fprintln(s.w, line) on a nil io.Writer, which panics. The old code always guarded writes to errW with a nil check; the new progress writer path does not.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 33a095c. Configure here.
33a095c to
5e80233
Compare
cd538a4 to
92449c4
Compare
3a3b257 to
c6c0c5a
Compare
Switches `entire explain --generate` from silent wait to real-time stream-driven progress. Independent of typed Claude error classification (#963). ### User-visible changes **Interactive (TTY):** live progress phases with in-place updates Generating checkpoint summary... (transcript: 47.0 KB) → Sending request to Anthropic... → Anthropic responded (TTFT 1.9s, 35.9k cached input tokens) -- generating... → Writing summary... (~1.2k tokens) ✓ Summary generated (3.1s, 1.9k output tokens) ✓ Summary generated and saved **Non-TTY (CI):** same lines, one per event (no in-place updates) **ACCESSIBLE=1:** one line per event, no ANSI escape sequences ### Architecture - `agent.StreamingTextGenerator` optional capability interface with `AsStreamingTextGenerator` helper. Plain-error contract — typed error classification will be wired separately (see #963). - Claude Code implements it via `GenerateTextStreaming` in a new file (`generate_streaming.go`), leaving the existing `GenerateText` path untouched. Falls back internally to `--output-format json` when the CLI rejects streaming flags. - `summaryProgressWriter` renders `GenerationProgress` phases using existing `statusStyles` for consistent styling with `entire status`. - TTY-gated deadline: no deadline interactive (Ctrl+C is the timeout); idle-based 5min watchdog non-TTY with a 30min wall-clock cap. - `ProgressFn` threads through `summarize.GenerateFromTranscript` and `ClaudeGenerator`; preserved as nil for the condensation path. ### Scope note This PR intentionally does not map Claude auth/rate-limit/config errors to actionable messages — that's #963's typed-error work. When #963 lands on main, a small follow-up will wire typed errors into the streaming path. Users hitting stream errors in the meantime see a generic "claude CLI reported error: <text>" (HTTP status included when present). ### Tests - Stream parser: success + error fixtures, malformed-line resilience, reader errors - GenerateTextStreaming: phase emission, envelope error surfacing, legacy fallback, context cancellation - Progress writer: all phases, Generating dedup, lastActivity tracking - Idle watchdog: stale-fire, bump-prevents-fire, zero-idle noop - Deadline resolver: interactive no-deadline vs non-interactive idle Entire-Checkpoint: 56ee3789dcea
c6c0c5a to
9c500a7
Compare


Summary
Switches
entire explain --generatefrom silent wait + wall-clock timeout to real-time stream-driven progress and a TTY-gated deadline strategy.Previously stacked on #963 (typed error classification). Rebased off main — now fully independent. See Scope note below.
What users see now
Interactive (TTY):
Non-TTY (CI): same lines, one per event (no in-place
\rupdates). 5-minute idle-based safety deadline + 30-minute hard wall-clock cap.ACCESSIBLE=1: one line per event, no ANSI escape sequences.
Architecture
claude --print --output-format stream-json --verboseconsumed incrementally by an NDJSON parser (streamClaudeResponse). Falls back internally to--output-format jsonif the CLI rejects streaming flags.agent.StreamingTextGeneratoroptional capability interface withAsStreamingTextGeneratorhelper. Claude Code implements it viaGenerateTextStreamingin a newgenerate_streaming.gofile — existingGenerateTextpath untouched.summaryProgressWriterrendersGenerationProgressphases using existingstatusStyles(lipgloss) for consistent styling withentire status. In-place\rupdates on TTY; one line per event in CI/accessible mode.ProgressFnthreading:summarize.GenerateFromTranscriptgains aProgressFnparameter, forwarded throughClaudeGeneratortoGenerateTextStreaming. Condensation path passesnil(no progress).Scope note (re: #963)
This PR was originally stacked on #963 (typed
ClaudeErrorclassification) so that stream failures mapped to actionable messages (auth →claude login, rate-limit, etc.). Rebasing off main means the streaming path currently returns plainerrorvalues — users hitting stream failures see a genericclaude CLI reported error: <text>(with HTTP status when present) instead of kind-specific advice.When #963 lands on main, a small follow-up will wire typed errors into the streaming path (~1 file, map
*ClaudeErrorkinds → user-facing messages ingenerate_streaming.goandexplain.go). Happy-path UX is unaffected; this only changes how rare failures are phrased.Test plan
mise run check(fmt + lint + unit + integration + E2E canary) passesentire explain -c <id> --generateon TTY — verify progress phases renderentire explain -c <id> --generate | cat— verify non-TTY mode (one line per event)ACCESSIBLE=1 entire explain -c <id> --generate— verify no ANSI escapes🤖 Generated with Claude Code