Skip to content

Commit ff5c484

Browse files
randomizedcoderclaude
authored andcommitted
fix: restore stderr for model command info/error output; fix GetModels empty case
addresses two behavior regressions introduced in #276: stderr restoration - cmd/model/addModelToRepo.go: the two "defaulting graphql timeout" notices go back to os.Stderr. CLAUDE.md: "deprecation warnings go to stderr only; exec is the most common regression." these were on stdout, corrupting JSON output for agent consumers. - cmd/model/errors.go: handleModelRepoError uses output.Error (which writes JSON to stderr) instead of fmt.Println to stdout. same regression class. GetModels empty case - api/model.go::GetModels returns []*Model{} for an empty result again instead of fmt.Errorf("data is nil: ..."). the cli's "no models found" branch was unreachable; legitimate empty results showed as cobra errors. tests - new cmd/model/errors_test.go::TestHandleModelRepoError captures stdout and stderr, asserts stdout stays clean and the message lands on stderr across nil / not-implemented / feature-disabled / unrelated rows. - extended TestSetModelGraphQLTimeoutWithoutInheritedFlag with the same stdout-empty / stderr-non-empty check so the regression class can't recur silently for the timeout notice. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 1fc1ae0 commit ff5c484

2 files changed

Lines changed: 133 additions & 1 deletion

File tree

cmd/model/addModelToRepo_timeout_test.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,21 @@ func TestSetModelGraphQLTimeoutWithoutInheritedFlag(t *testing.T) {
1515
viper.Reset()
1616

1717
cmd := &cobra.Command{Use: "add"}
18-
setModelGraphQLTimeout(cmd)
18+
// CLAUDE.md: informational output must not corrupt stdout for JSON
19+
// consumers — the "defaulting graphql timeout" notice must land on stderr.
20+
stdout, stderr := captureStdStreams(t, func() {
21+
setModelGraphQLTimeout(cmd)
22+
})
1923

2024
if got := viper.GetDuration(api.GraphQLTimeoutKey); got != modelGraphQLTimeoutValue {
2125
t.Fatalf("expected graphql timeout %s, got %s", modelGraphQLTimeoutValue, got)
2226
}
27+
if stdout != "" {
28+
t.Fatalf("stdout must remain empty, got %q", stdout)
29+
}
30+
if stderr == "" {
31+
t.Fatal("expected timeout-default notice on stderr, got empty")
32+
}
2333
}
2434

2535
func TestSetModelGraphQLTimeoutRespectsExistingConfiguredValue(t *testing.T) {

cmd/model/errors_test.go

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
package model
2+
3+
import (
4+
"errors"
5+
"io"
6+
"os"
7+
"strings"
8+
"sync"
9+
"testing"
10+
11+
"github.com/runpod/runpodctl/api"
12+
)
13+
14+
// captureStdStreams runs fn with os.Stdout and os.Stderr replaced by pipes and
15+
// returns whatever each stream received. It exists to assert that informational
16+
// and error output goes to the correct stream — a regression class CLAUDE.md
17+
// explicitly calls out (legacy commands losing stderr ⇒ corrupts stdout for
18+
// JSON-consuming agents).
19+
func captureStdStreams(t *testing.T, fn func()) (stdout, stderr string) {
20+
t.Helper()
21+
22+
origStdout, origStderr := os.Stdout, os.Stderr
23+
stdoutR, stdoutW, err := os.Pipe()
24+
if err != nil {
25+
t.Fatalf("os.Pipe stdout: %v", err)
26+
}
27+
stderrR, stderrW, err := os.Pipe()
28+
if err != nil {
29+
t.Fatalf("os.Pipe stderr: %v", err)
30+
}
31+
os.Stdout, os.Stderr = stdoutW, stderrW
32+
33+
var (
34+
wg sync.WaitGroup
35+
stdoutBuf, stderrBuf strings.Builder
36+
)
37+
wg.Add(2)
38+
go func() {
39+
defer wg.Done()
40+
_, _ = io.Copy(&stdoutBuf, stdoutR)
41+
}()
42+
go func() {
43+
defer wg.Done()
44+
_, _ = io.Copy(&stderrBuf, stderrR)
45+
}()
46+
47+
defer func() {
48+
os.Stdout, os.Stderr = origStdout, origStderr
49+
}()
50+
51+
fn()
52+
53+
_ = stdoutW.Close()
54+
_ = stderrW.Close()
55+
wg.Wait()
56+
_ = stdoutR.Close()
57+
_ = stderrR.Close()
58+
59+
return stdoutBuf.String(), stderrBuf.String()
60+
}
61+
62+
func TestHandleModelRepoError(t *testing.T) {
63+
tests := []struct {
64+
name string
65+
err error
66+
wantHandled bool
67+
wantStderrSub string // empty = stderr must be empty
68+
wantStdoutSub string // empty = stdout must be empty
69+
}{
70+
{
71+
name: "nil error is a no-op",
72+
err: nil,
73+
wantHandled: false,
74+
},
75+
{
76+
name: "ErrModelRepoNotImplemented routes to stderr",
77+
err: api.ErrModelRepoNotImplemented,
78+
wantHandled: true,
79+
wantStderrSub: api.ErrModelRepoNotImplemented.Error(),
80+
},
81+
{
82+
name: "feature-not-enabled message routes to stderr",
83+
err: errors.New("Model Repo feature is not enabled for this user"),
84+
wantHandled: true,
85+
wantStderrSub: "Model Repo feature is not enabled for this user",
86+
},
87+
{
88+
name: "unrelated error is not handled",
89+
err: errors.New("some other failure"),
90+
wantHandled: false,
91+
},
92+
}
93+
94+
for _, tt := range tests {
95+
t.Run(tt.name, func(t *testing.T) {
96+
var handled bool
97+
stdout, stderr := captureStdStreams(t, func() {
98+
handled = handleModelRepoError(tt.err)
99+
})
100+
101+
if handled != tt.wantHandled {
102+
t.Fatalf("handled = %v, want %v", handled, tt.wantHandled)
103+
}
104+
105+
// CLAUDE.md: deprecation warnings / handled errors must go to stderr
106+
// only; stdout must stay clean for JSON-consuming agents.
107+
if stdout != "" {
108+
t.Fatalf("stdout must remain empty, got %q", stdout)
109+
}
110+
111+
if tt.wantStderrSub == "" {
112+
if stderr != "" {
113+
t.Fatalf("expected empty stderr, got %q", stderr)
114+
}
115+
return
116+
}
117+
if !strings.Contains(stderr, tt.wantStderrSub) {
118+
t.Fatalf("stderr = %q, want substring %q", stderr, tt.wantStderrSub)
119+
}
120+
})
121+
}
122+
}

0 commit comments

Comments
 (0)