fix: stderr for model output; GetModels empty case (on top of #276)#286
Open
daveseddon-runpod wants to merge 3 commits into
Open
fix: stderr for model output; GetModels empty case (on top of #276)#286daveseddon-runpod wants to merge 3 commits into
daveseddon-runpod wants to merge 3 commits into
Conversation
review-driven follow-ups to PR runpod#276 (CE-1579): - table-driven tests for the helpers added in runpod#276: TestValueOrDash, TestFormatTimestamp (precision tiers + boundary cases), TestCreateCmd_FlagValidation, expanded TestBuildTemplateEndpointGQLInput, TestModelVersionHash edge cases. - benchmark + correctness check for cmd/serverless/create.go::randomString with two alternative implementations; data confirms keeping the current form (call site is one-shot per CLI invocation). - lint fixes in runpod#276-touched files: 6× 200 -> http.StatusOK in api/model.go, sprintfQuotedString in addModelToRepo.go, two PR-introduced err shadows in create.go and addModelToRepo.go, removed an unjustified //nolint:gosec. - math/rand -> math/rand/v2 for randomString with a documented nolint explaining why crypto/rand is not warranted (display-only template-name uniqueness suffix, not a token). - range over int in randomString. - go.mod bump to 1.26.4, drop explicit toolchain directive. - dep bumps: google/uuid 1.4->1.6, x/crypto 0.50->0.53, x/mod 0.34->0.37, and transitive x/net x/sys x/term x/text. - ci.yml + release.yml setup-go bumped to 1.26.4. build + go vet + full test suite all pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…s empty case addresses two behavior regressions introduced in runpod#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>
|
Promptless prepared a documentation update related to this change. Triggered by runpodctl PR #286 The new Review: Document --model-reference flag for serverless create |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up fix-commit on top of #276 addressing two behavior-level regressions called out in review. Single commit, easy to cherry-pick — no feature changes, no scope creep.
Recommendation: same workflow as the previous follow-up (commit
2b7cf85, which was cherry-picked onto the #276 branch) — please cherry-pick the single commit from this PR onto thece1579branch as well, then close this PR. That keeps the #276 history linear and avoids competing/duplicate PR surface.Fixes
Restore stderr for informational/error output — CLAUDE.md says "deprecation warnings go to stderr only (exec is the most common regression)." PR #276 inadvertently moved these to stdout, which corrupts JSON output for agent consumers parsing stdout.
cmd/model/addModelToRepo.go— the two "defaulting graphql timeout to N for model creation operations" notices go back toos.Stderr.cmd/model/errors.go::handleModelRepoError— usesoutput.Error(JSON to stderr) again instead offmt.Printlnto stdout.Fix
GetModelsempty-result regression —api/model.go::GetModelsreturns[]*Model{}again for an empty result, instead offmt.Errorf("data is nil: ..."). Previously the CLI's"no models found"branch was unreachable; legitimate empty results surfaced as cobra errors.Tests
cmd/model/errors_test.go::TestHandleModelRepoError(new, 4 rows): captures stdout + stderr via pipes, asserts stdout stays empty and the appropriate message lands on stderr across nil / not-implemented / feature-disabled / unrelated cases.cmd/model/addModelToRepo_timeout_test.go::TestSetModelGraphQLTimeoutWithoutInheritedFlag: extended with the same stdout-empty / stderr-non-empty check so the regression class can't recur silently for the timeout notice.Test plan
Notes
Deliberately leaves the larger concerns from the review for separate PRs / brosenpod's call: the `--output JSON default` regression in model list/remove/add, the dropped `DefaultGraphQLURL` fallback + renamed env var, and the GQL fallback in `serverless create` silently dropping `--flash-boot`/`--env`/etc. Happy to follow up on any of those individually.
🤖 Generated with Claude Code