Skip to content

Commit 6e5c089

Browse files
peyton-altclaude
andcommitted
feat(agent): finish the TextGenError migration across every path and consumer
Closes out the remaining gaps rather than deferring them, so this PR lands complete. The unification previously stopped at the explain + non-streaming GenerateText boundary; everything else fell through to the generic "Failed to generate summary" branch with no classification and no remediation. **Streaming path now classifies (the one with real user impact).** TextGeneratorAdapter prefers streaming, so GenerateTextStreaming is the path `explain --generate` actually takes for Claude — and nine of its ten failure returns were plain fmt.Errorf. A stale key gave: claude stream failed: Invalid API key · Please run /login: exit status 2 and now gives: Claude authentication failed message: Invalid API key · Please run /login try: run `claude login` and retry All ten returns route through a shared streamFailure() that applies the same stderr classification as generate.go (HTTP status, then auth phrase) and attaches evidence. cmd.Start failures classify as CLIMissing via IsExecNotFoundErr, so a missing binary reads "not installed or not on PATH" instead of a raw exec error. **External agents return *TextGenError.** They are selectable summary providers, so without this the type's own doc claim ("every summary provider") was false and their failures missed errors.As entirely. **Pi is wired and covered.** PiAgent gained a CommandRunner field, so pi joins TestGenerateText_Matrix — it previously had zero coverage of any failure kind. Separately, pi/models.go no longer routes --list-models through HandleTextGenResult: that builds the summary error surface, so a model-listing failure could render as "Pi failed to generate the summary". **Registry gaps now fail a test instead of degrading silently.** TestSummaryProviderTablesAreComplete enumerates SummaryCapableAgents() (newly exported) and asserts displayNameFor, syntheticFallback and summaryProviderBinaries cover all six. Forgetting one previously shipped a raw registry key in the label ("copilot-cli authentication failed"), a missing try row, or an agent silently absent from the picker — none of which failed anything. Also dropped //nolint:exhaustive from kindPrefix so a sixth Kind fails lint where it has to be handled. **Classifier completed.** ClassifyStderrHTTPStatus now maps every 4xx (matching classifyEnvelopeFields' >=400 && <500 arm — 413/422 previously fell to Unknown on one path and Config on the other), treats 402 as rate-limit rather than config since credit exhaustion is a "wait or top up" problem, and matches the parenthesized form ("Unauthorized (401)", "rate limit exceeded (429)"). Precedence changed from positional to specificity: Auth > RateLimit > Config. Completing the 4xx mapping made "first recognized wins" degenerate to "first wins", which let a leading 413 mask a later 401 — my own regression test caught it. Auth and RateLimit carry specific remediations; Config is the catch-all, so the actionable kind should win wherever it appears. **Privacy.** manual_commit_condensation.go logged err.Error() verbatim, and Message can be provider stdout — the model's prose summary of the user's transcript — since stdout is the fallback for stdout-primary CLIs. Now logs kind/provider/api_status/exit_code/message_len instead. CLAUDE.md forbids logging user content. **Hygiene:** removed the unreachable double-parse branch in GenerateText (the only failure return not wrapped in withEvidence), and dropped a vacuous errors.Is(err, context.Canceled) assertion in the streaming test that used context.Background() and could never fire. Verified: 8572 unit tests, 448 integration tests, lint 0 issues on a cleaned cache. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent a255e26 commit 6e5c089

14 files changed

Lines changed: 341 additions & 53 deletions

cmd/entire/cli/agent/claudecode/generate.go

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -271,14 +271,20 @@ func (c *ClaudeCodeAgent) GenerateText(ctx context.Context, prompt string, model
271271
Message: "claude CLI returned empty output",
272272
})
273273
}
274+
// classifyClaudeEnvelope already parsed these same bytes successfully — with
275+
// runErr == nil it returns non-nil on any parse failure, so reaching here
276+
// means the parse succeeded. The previous defensive branch here was
277+
// unreachable and was the only failure return in this function not wrapped
278+
// in withEvidence; parseGenerateTextResponse is pure, so the second call
279+
// cannot disagree with the first.
274280
result, _, parseErr := parseGenerateTextResponse(res.Stdout)
275281
if parseErr != nil {
276-
return "", &agent.TextGenError{
282+
return "", withEvidence(&agent.TextGenError{
277283
Kind: agent.TextGenErrorUnknown,
278284
Provider: agent.AgentNameClaudeCode,
279-
Message: fmt.Sprintf("unexpected parse failure on success path: %v", parseErr),
285+
Message: agent.TruncateStderr(fmt.Sprintf("failed to parse claude CLI response: %v", parseErr)),
280286
Cause: parseErr,
281-
}
287+
})
282288
}
283289
return result, nil
284290
}

cmd/entire/cli/agent/claudecode/generate_streaming.go

Lines changed: 72 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -58,13 +58,24 @@ func (c *ClaudeCodeAgent) GenerateTextStreaming(
5858

5959
stdout, err := cmd.StdoutPipe()
6060
if err != nil {
61-
return "", fmt.Errorf("claude stream stdout pipe: %w", err)
61+
return "", streamFailure("", 0, 0, err, fmt.Sprintf("claude stream stdout pipe: %v", err))
6262
}
6363
var stderr bytes.Buffer
6464
cmd.Stderr = &stderr
6565

6666
if err := cmd.Start(); err != nil {
67-
return "", fmt.Errorf("claude stream start: %w", err)
67+
// A missing/unexecutable binary must classify as CLIMissing so the user
68+
// gets "not installed or not on PATH" rather than a raw exec error.
69+
if agent.IsExecNotFoundErr(err) {
70+
return "", &agent.TextGenerationError{
71+
Err: &agent.TextGenError{
72+
Kind: agent.TextGenErrorCLIMissing,
73+
Provider: agent.AgentNameClaudeCode,
74+
Cause: err,
75+
},
76+
}
77+
}
78+
return "", streamFailure("", 0, 0, err, fmt.Sprintf("claude stream start: %v", err))
6879
}
6980

7081
// Count stdout bytes so the timeout diagnostic can distinguish "provider
@@ -86,7 +97,11 @@ func (c *ClaudeCodeAgent) GenerateTextStreaming(
8697

8798
// Specific envelope error outranks a generic ctx-cancel message.
8899
if final != nil && final.IsError {
89-
return "", envelopeErrorMessage(final)
100+
return "", &agent.TextGenerationError{
101+
Err: envelopeErrorMessage(final),
102+
Stderr: agent.TruncateStderr(stderr.String()),
103+
StdoutBytes: counted.n,
104+
}
90105
}
91106

92107
if final != nil {
@@ -95,14 +110,11 @@ func (c *ClaudeCodeAgent) GenerateTextStreaming(
95110
// result envelope and cmd.Wait). A non-context process failure — the
96111
// CLI exiting non-zero on its own — remains authoritative.
97112
if waitErr != nil && !isContextKill(ctx, waitErr) {
98-
stderrStr := strings.TrimSpace(stderr.String())
99-
if stderrStr != "" {
100-
return "", fmt.Errorf("claude stream failed: %s: %w", stderrStr, waitErr)
101-
}
102-
return "", fmt.Errorf("claude stream failed: %w", waitErr)
113+
return "", streamFailure(stderr.String(), counted.n, exitCodeOf(waitErr), waitErr,
114+
fmt.Sprintf("claude stream failed: %v", waitErr))
103115
}
104116
if final.Result == nil {
105-
return "", errors.New("claude returned empty result")
117+
return "", streamFailure(stderr.String(), counted.n, 0, nil, "claude returned empty result")
106118
}
107119
if progress != nil {
108120
progress(agent.GenerationProgress{
@@ -144,16 +156,61 @@ func (c *ClaudeCodeAgent) GenerateTextStreaming(
144156
slog.String("stderr", strings.TrimSpace(stderrStr)))
145157
return c.GenerateText(ctx, prompt, model)
146158
}
147-
if stderrStr != "" {
148-
return "", fmt.Errorf("claude stream failed: %s: %w", strings.TrimSpace(stderrStr), waitErr)
149-
}
150-
return "", fmt.Errorf("claude stream failed: %w", waitErr)
159+
return "", streamFailure(stderrStr, counted.n, exitCodeOf(waitErr), waitErr,
160+
fmt.Sprintf("claude stream failed: %v", waitErr))
151161
}
152162

153163
if parseErr != nil {
154-
return "", fmt.Errorf("claude stream parse: %w", parseErr)
164+
return "", streamFailure(stderr.String(), counted.n, 0, parseErr,
165+
fmt.Sprintf("claude stream parse: %v", parseErr))
166+
}
167+
return "", streamFailure(stderr.String(), counted.n, 0, nil,
168+
"claude exited without producing a result")
169+
}
170+
171+
// streamFailure classifies a streaming failure the same way the non-streaming
172+
// path does — HTTP status on stderr, then Claude's auth-phrase fallback — and
173+
// attaches the captured evidence.
174+
//
175+
// Before this existed, nine of the ten failure returns in GenerateTextStreaming
176+
// were plain fmt.Errorf, so they reached the user through
177+
// formatCheckpointSummaryError's default branch as a raw Go error string with
178+
// no remediation row. That mattered more than it looked: TextGeneratorAdapter
179+
// prefers streaming, so this is the path `explain --generate` actually takes
180+
// for Claude. A stale key (claude exits 2, "Invalid API key" on stderr, no
181+
// envelope) got "claude stream failed: ... exit status 2" here while the
182+
// non-streaming fallback correctly said "Claude authentication failed".
183+
func streamFailure(stderrBuf string, stdoutBytes int, exitCode int, cause error, fallbackMsg string) error {
184+
stderrStr := strings.TrimSpace(stderrBuf)
185+
msg := stderrStr
186+
if msg == "" {
187+
msg = fallbackMsg
188+
}
189+
kind := agent.ClassifyStderrHTTPStatus(stderrStr)
190+
if kind == agent.TextGenErrorUnknown && containsAuthPhrase(stderrStr) {
191+
kind = agent.TextGenErrorAuth
192+
}
193+
return &agent.TextGenerationError{
194+
Err: &agent.TextGenError{
195+
Kind: kind,
196+
Provider: agent.AgentNameClaudeCode,
197+
Message: agent.TruncateStderr(msg),
198+
ExitCode: exitCode,
199+
Cause: cause,
200+
},
201+
Stderr: agent.TruncateStderr(stderrBuf),
202+
StdoutBytes: stdoutBytes,
203+
}
204+
}
205+
206+
// exitCodeOf returns the process exit code from err, or 0 when err is not an
207+
// *exec.ExitError (a launch failure produces no exit code).
208+
func exitCodeOf(err error) int {
209+
var exitErr *exec.ExitError
210+
if errors.As(err, &exitErr) {
211+
return exitErr.ExitCode()
155212
}
156-
return "", errors.New("claude exited without producing a result")
213+
return 0
157214
}
158215

159216
// envelopeErrorMessage formats an is_error result envelope as a typed

cmd/entire/cli/agent/claudecode/generate_streaming_test.go

Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -182,9 +182,6 @@ func TestGenerateTextStreaming_EnvelopeErrorSurfaced(t *testing.T) {
182182
if err == nil {
183183
t.Fatal("expected error from is_error envelope")
184184
}
185-
if errors.Is(err, context.Canceled) {
186-
t.Errorf("expected envelope error, got Canceled")
187-
}
188185
// Streaming envelope errors must surface as a typed *agent.TextGenError so
189186
// the explain layer's renderTextGenError can route on Kind
190187
// (auth/rate-limit/config) instead of substring-matching err.Error().
@@ -345,3 +342,51 @@ func equalPhases(a, b []agent.ProgressPhase) bool {
345342
}
346343
return true
347344
}
345+
346+
// TestGenerateTextStreaming_ClassifiesStderrFailures pins that the streaming
347+
// path classifies envelope-less failures the same way GenerateText does.
348+
//
349+
// This is the path `explain --generate` actually takes for Claude
350+
// (TextGeneratorAdapter prefers streaming), and until now nine of its ten
351+
// failure returns were plain fmt.Errorf. A stale key produced
352+
// "claude stream failed: Invalid API key ... exit status 2" with no
353+
// remediation, while the non-streaming fallback correctly said
354+
// "Claude authentication failed".
355+
func TestGenerateTextStreaming_ClassifiesStderrFailures(t *testing.T) {
356+
t.Parallel()
357+
tests := []struct {
358+
name string
359+
stderr string
360+
wantKind agent.TextGenErrorKind
361+
}{
362+
{"auth phrase", "Invalid API key · Please run /login", agent.TextGenErrorAuth},
363+
{"http 401", "ERROR: 401 Unauthorized", agent.TextGenErrorAuth},
364+
{"http 429", "ERROR: 429 Too Many Requests", agent.TextGenErrorRateLimit},
365+
{"http 404", "ERROR: 404 Not Found", agent.TextGenErrorConfig},
366+
}
367+
for _, tc := range tests {
368+
t.Run(tc.name, func(t *testing.T) {
369+
t.Parallel()
370+
ag := &ClaudeCodeAgent{CommandRunner: testutil.FakeStreamCmd("", tc.stderr, 2)}
371+
_, err := ag.GenerateTextStreaming(context.Background(), "test", "haiku", nil)
372+
if err == nil {
373+
t.Fatal("expected an error")
374+
}
375+
var tge *agent.TextGenError
376+
if !errors.As(err, &tge) {
377+
t.Fatalf("errors.As(*TextGenError) failed: %T %v", err, err)
378+
}
379+
if tge.Kind != tc.wantKind {
380+
t.Errorf("Kind = %q; want %q", tge.Kind, tc.wantKind)
381+
}
382+
if tge.Provider != agent.AgentNameClaudeCode {
383+
t.Errorf("Provider = %q; want claude-code", tge.Provider)
384+
}
385+
// Evidence must ride along too, same as every other failure site.
386+
var failure *agent.TextGenerationError
387+
if !errors.As(err, &failure) {
388+
t.Errorf("errors.As(*TextGenerationError) failed — evidence lost: %T", err)
389+
}
390+
})
391+
}
392+
}

cmd/entire/cli/agent/external/external.go

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -374,14 +374,40 @@ func (e *Agent) CalculateTokenUsage(transcriptData []byte, fromOffset int) (*age
374374

375375
// --- TextGenerator methods ---
376376

377+
// GenerateText implements agent.TextGenerator for external plugins.
378+
//
379+
// Failures return *agent.TextGenError like every built-in provider. External
380+
// agents are selectable summary providers, so without this the explain layer's
381+
// errors.As would miss them and they would fall through to the generic
382+
// "Failed to generate summary" branch with no classification and no
383+
// remediation row — and agent.TextGenError's own doc claim ("every summary
384+
// provider") would be false.
385+
//
386+
// Provider is the plugin's dynamic name, so it cannot appear in the
387+
// compile-time display/remediation tables; displayNameFor falls back to the
388+
// registry key, which is the right answer for a third-party plugin.
377389
func (e *Agent) GenerateText(ctx context.Context, prompt string, model string) (string, error) {
378390
stdout, err := e.run(ctx, []byte(prompt), "generate-text", "--model", model)
379391
if err != nil {
380-
return "", fmt.Errorf("generate-text: %w", err)
392+
kind := agent.TextGenErrorUnknown
393+
if agent.IsExecNotFoundErr(err) {
394+
kind = agent.TextGenErrorCLIMissing
395+
}
396+
return "", &agent.TextGenError{
397+
Kind: kind,
398+
Provider: e.Name(),
399+
Message: agent.TruncateStderr(err.Error()),
400+
Cause: err,
401+
}
381402
}
382403
var resp GenerateTextResponse
383404
if err := json.Unmarshal(stdout, &resp); err != nil {
384-
return "", fmt.Errorf("generate-text: invalid JSON: %w", err)
405+
return "", &agent.TextGenError{
406+
Kind: agent.TextGenErrorUnknown,
407+
Provider: e.Name(),
408+
Message: agent.TruncateStderr(fmt.Sprintf("invalid JSON from generate-text: %v", err)),
409+
Cause: err,
410+
}
385411
}
386412
return resp.Text, nil
387413
}

cmd/entire/cli/agent/generate_matrix_test.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/entireio/cli/cmd/entire/cli/agent/copilotcli"
1515
"github.com/entireio/cli/cmd/entire/cli/agent/cursor"
1616
"github.com/entireio/cli/cmd/entire/cli/agent/geminicli"
17+
"github.com/entireio/cli/cmd/entire/cli/agent/pi"
1718
"github.com/entireio/cli/cmd/entire/cli/agent/types"
1819
)
1920

@@ -27,9 +28,8 @@ const windowsOSTest = "windows"
2728
// Gemini's provider-specific phrase heuristic is covered separately in
2829
// geminicli/ since it is the only agent with an extraClassify hook.
2930
//
30-
// Pi is absent: PiAgent has no CommandRunner field and hardcodes nil in
31-
// RunIsolatedTextGeneratorCLIRaw, so its subprocess cannot be stubbed. Adding
32-
// that field would let pi join this table.
31+
// All five non-Claude summary providers are covered, pi included since it
32+
// gained a CommandRunner field.
3333
func TestGenerateText_Matrix(t *testing.T) {
3434
t.Parallel()
3535
if runtime.GOOS == windowsOSTest {
@@ -59,6 +59,9 @@ func TestGenerateText_Matrix(t *testing.T) {
5959
{"geminicli", agent.AgentNameGemini, "gemini CLI returned empty output", func(r agent.TextCommandRunner) textGenerator {
6060
return &geminicli.GeminiCLIAgent{CommandRunner: r}
6161
}},
62+
{"pi", agent.AgentNamePi, "pi CLI returned empty output", func(r agent.TextCommandRunner) textGenerator {
63+
return &pi.PiAgent{CommandRunner: r}
64+
}},
6265
}
6366

6467
// assertComposition pins the contract the #964/#1005 reconciliation created:

cmd/entire/cli/agent/pi/generate.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,6 @@ func (a *PiAgent) GenerateText(ctx context.Context, prompt string, model string)
2020
}
2121
args = append(args, prompt)
2222

23-
res, runErr := agent.RunIsolatedTextGeneratorCLIRaw(ctx, nil, "pi", args, "")
23+
res, runErr := agent.RunIsolatedTextGeneratorCLIRaw(ctx, a.CommandRunner, "pi", args, "")
2424
return agent.HandleTextGenResult(res, runErr, agent.AgentNamePi, "pi CLI returned empty output", nil) //nolint:wrapcheck // return unwrapped: the explain layer renders label+message from the typed error, so a wrap prefix would leak into user output. errors.As (*TextGenError) / errors.Is (ctx sentinel) must reach it unflattened.
2525
}

cmd/entire/cli/agent/pi/models.go

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package pi
33
import (
44
"bufio"
55
"context"
6+
"errors"
67
"fmt"
78
"strings"
89

@@ -16,10 +17,24 @@ var _ agent.ModelLister = (*PiAgent)(nil)
1617
// Pi has a real enumeration command spanning every configured provider, so the
1718
// result reflects what this machine/account can actually use.
1819
func (a *PiAgent) ListModels(ctx context.Context) ([]agent.ModelInfo, error) {
19-
res, runErr := agent.RunIsolatedTextGeneratorCLIRaw(ctx, nil, "pi", []string{"--list-models"}, "")
20-
out, err := agent.HandleTextGenResult(res, runErr, agent.AgentNamePi, "pi --list-models returned empty output", nil)
21-
if err != nil {
22-
return nil, fmt.Errorf("pi --list-models: %w", err)
20+
// Deliberately NOT HandleTextGenResult: that helper builds the summary-path
21+
// error surface, so a model-listing failure would render as "Pi failed to
22+
// generate the summary" if it ever reached formatCheckpointSummaryError.
23+
// Listing models is a different operation and gets a plain error.
24+
res, runErr := agent.RunIsolatedTextGeneratorCLIRaw(ctx, a.CommandRunner, "pi", []string{"--list-models"}, "")
25+
if runErr != nil {
26+
detail := strings.TrimSpace(string(res.Stderr))
27+
if detail == "" {
28+
detail = strings.TrimSpace(string(res.Stdout))
29+
}
30+
if detail == "" {
31+
detail = runErr.Error()
32+
}
33+
return nil, fmt.Errorf("pi --list-models: %s", agent.TruncateStderr(detail))
34+
}
35+
out := strings.TrimSpace(string(res.Stdout))
36+
if out == "" {
37+
return nil, errors.New("pi --list-models returned empty output")
2338
}
2439
return parsePiModelList(out), nil
2540
}

cmd/entire/cli/agent/pi/pi.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,12 @@ func init() {
4343
// PiAgent implements agent.Agent for the pi coding agent.
4444
//
4545
//nolint:revive // PiAgent is clearer than Agent in this context
46-
type PiAgent struct{}
46+
type PiAgent struct {
47+
// CommandRunner lets tests stub the pi subprocess. Nil means
48+
// exec.CommandContext. Without this field pi could not join
49+
// TestGenerateText_Matrix and its failure classification was unverified.
50+
CommandRunner agent.TextCommandRunner
51+
}
4752

4853
// NewPiAgent returns a new Pi agent instance.
4954
func NewPiAgent() agent.Agent {

0 commit comments

Comments
 (0)