Skip to content

Commit 7133988

Browse files
peyton-altclaude
andcommitted
fix(pi): keep the cause in the ListModels error chain
Trail #193 finding 019fb4fa-0f6 (low): pi/models.go used %s instead of %w, so runErr dropped out of the error chain entirely and callers could not ask errors.Is(err, exec.ErrNotFound) — i.e. "is pi installed?" — programmatically. Mine, from the same commit that moved ListModels off HandleTextGenResult. Wrapping naively duplicated the text, because detail falls back to runErr.Error() when the subprocess produced no output. When there is no output, runErr is the whole story, so it is now wrapped alone rather than interpolated and then wrapped again. TestListModels_PreservesCauseAndStaysUntyped pins both halves and both directions: the cause is reachable via errors.Is, it appears exactly once, and the error is deliberately NOT an *agent.TextGenError — listing models is not summary generation, and routing it through that surface would let a model-listing failure render as "Pi failed to generate the summary". Also rewrote the streamFailure doc comment in the present tense. Trail finding 019fb472-90c reported the streaming path as still returning bare fmt.Errorf, citing "lines 100/148" — the pre-fix layout. Those returns were migrated in 6e5c089 and the finding's own location (line 174) landed inside the comment describing the OLD behavior. The comment now states what the code does rather than what it used to do, so it cannot be read as a description of current behavior. Verified: zero bare fmt.Errorf/errors.New failure returns remain in generate_streaming.go; 8573 unit tests; lint 0 issues on a cleaned cache. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 6e5c089 commit 7133988

3 files changed

Lines changed: 61 additions & 11 deletions

File tree

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

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -172,14 +172,16 @@ func (c *ClaudeCodeAgent) GenerateTextStreaming(
172172
// path does — HTTP status on stderr, then Claude's auth-phrase fallback — and
173173
// attaches the captured evidence.
174174
//
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".
175+
// EVERY failure return in GenerateTextStreaming goes through this or an
176+
// explicit *TextGenerationError — there are no bare fmt.Errorf failure returns
177+
// left in this file, and TestGenerateTextStreaming_ClassifiesStderrFailures
178+
// pins that for the auth-phrase, 401, 429 and 404 shapes.
179+
//
180+
// Why it matters: TextGeneratorAdapter prefers streaming, so this is the path
181+
// `explain --generate` actually takes for Claude. A stale key (claude exits 2,
182+
// "Invalid API key" on stderr, no envelope) must produce "Claude
183+
// authentication failed" with a remediation row, not a raw Go error string via
184+
// formatCheckpointSummaryError's default branch.
183185
func streamFailure(stderrBuf string, stdoutBytes int, exitCode int, cause error, fallbackMsg string) error {
184186
stderrStr := strings.TrimSpace(stderrBuf)
185187
msg := stderrStr

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,19 @@ func (a *PiAgent) ListModels(ctx context.Context) ([]agent.ModelInfo, error) {
2323
// Listing models is a different operation and gets a plain error.
2424
res, runErr := agent.RunIsolatedTextGeneratorCLIRaw(ctx, a.CommandRunner, "pi", []string{"--list-models"}, "")
2525
if runErr != nil {
26+
// %w on runErr so callers can still reach the underlying error
27+
// (exec.ErrNotFound in particular) via errors.Is/As — dropping the cause
28+
// would make "is pi installed?" unanswerable programmatically. When the
29+
// subprocess produced no output, runErr IS the whole story, so wrap it
30+
// alone rather than interpolating its text and then wrapping it again.
2631
detail := strings.TrimSpace(string(res.Stderr))
2732
if detail == "" {
2833
detail = strings.TrimSpace(string(res.Stdout))
2934
}
3035
if detail == "" {
31-
detail = runErr.Error()
36+
return nil, fmt.Errorf("pi --list-models: %w", runErr)
3237
}
33-
return nil, fmt.Errorf("pi --list-models: %s", agent.TruncateStderr(detail))
38+
return nil, fmt.Errorf("pi --list-models: %s: %w", agent.TruncateStderr(detail), runErr)
3439
}
3540
out := strings.TrimSpace(string(res.Stdout))
3641
if out == "" {

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

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,14 @@
11
package pi
22

3-
import "testing"
3+
import (
4+
"context"
5+
"errors"
6+
"os/exec"
7+
"strings"
8+
"testing"
9+
10+
"github.com/entireio/cli/cmd/entire/cli/agent"
11+
)
412

513
func TestParsePiModelList(t *testing.T) {
614
raw := "provider model context max-out thinking images\n" +
@@ -33,3 +41,38 @@ func TestParsePiModelList_HeaderAndBlanksSkipped(t *testing.T) {
3341
t.Fatalf("expected no models, got %#v", got)
3442
}
3543
}
44+
45+
// TestListModels_PreservesCauseAndStaysUntyped pins two things about the
46+
// ListModels error path that are easy to get wrong in opposite directions.
47+
//
48+
// 1. The cause survives. It is wrapped with %w, not interpolated with %s, so
49+
// callers can still ask errors.Is(err, exec.ErrNotFound) — i.e. "is pi
50+
// installed?" — programmatically.
51+
// 2. It is NOT an *agent.TextGenError. Listing models is not summary
52+
// generation; routing it through the summary classifier would let a
53+
// model-listing failure render as "Pi failed to generate the summary" if it
54+
// ever reached formatCheckpointSummaryError.
55+
func TestListModels_PreservesCauseAndStaysUntyped(t *testing.T) {
56+
t.Parallel()
57+
a := &PiAgent{CommandRunner: func(ctx context.Context, _ string, _ ...string) *exec.Cmd {
58+
return exec.CommandContext(ctx, "definitely-no-such-binary-xyz")
59+
}}
60+
_, err := a.ListModels(context.Background())
61+
if err == nil {
62+
t.Fatal("expected an error")
63+
}
64+
if !errors.Is(err, exec.ErrNotFound) {
65+
t.Errorf("errors.Is(exec.ErrNotFound) = false; the cause was dropped from the chain: %v", err)
66+
}
67+
var tge *agent.TextGenError
68+
if errors.As(err, &tge) {
69+
t.Error("ListModels must not return *agent.TextGenError; that is the summary-path error surface")
70+
}
71+
if !strings.Contains(err.Error(), "pi --list-models") {
72+
t.Errorf("err = %v; want the operation named", err)
73+
}
74+
// The cause must appear once, not twice.
75+
if strings.Count(err.Error(), "executable file not found") != 1 {
76+
t.Errorf("err = %q; cause should appear exactly once", err.Error())
77+
}
78+
}

0 commit comments

Comments
 (0)