fix: correct exit codes and JSON error output#57
Conversation
Move error formatting from cli.Execute() to main() so errors are printed once: as structured JSON when -o json, plain text otherwise. Execute() now returns (error, outputFormat) instead of printing directly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Detect Zenodo's 400 responses caused by invalid tokens (LikelyAuthError) and map them to exit code 2 instead of 1 - Add hint for 400 auth errors suggesting token check Fixes the three error handling test cases in issue #47: 1. Invalid token → exit code 2 with helpful hint 2. Invalid record ID → exit code 1 with "not found" hint (already worked) 3. -o json with bad token → structured JSON error on stderr Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ran-codes
changed the title
There was a problem hiding this comment.
Pull request overview
This PR fixes duplicate error output when using -o json by refactoring error formatting to be centralized in main(). Previously, cli.Execute() printed errors directly, and main() would also print structured JSON errors, causing duplication. The solution changes Execute() to return both the error and the resolved output format, allowing main() to handle all error formatting consistently.
Changes:
- Refactored
cli.Execute()to return(error, string)instead of printing errors directly - Centralized error formatting in
main()with format-aware output (plain text or JSON) - Added
LikelyAuthError()method to detect auth errors disguised as 400 responses from Zenodo - Updated exit code logic to return code 2 for likely auth errors
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
internal/cli/root.go |
Modified Execute() to return error and output format instead of printing, removed duplicate error printing |
cmd/zenodo/main.go |
Centralized error formatting with format-aware output, removed isJSONOutput() helper, added exit code handling for likely auth errors |
internal/model/error.go |
Added LikelyAuthError() method to detect 400 responses that indicate auth issues, added hint for 400 status code |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func Execute() (error, string) { | ||
| // Run PersistentPreRunE first via cobra, then return output format. | ||
| err := rootCmd.Execute() | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Error: %s\n", err) | ||
| } | ||
| return err | ||
| return err, appCtx.Output |
There was a problem hiding this comment.
When PersistentPreRunE fails (e.g., config loading error on line 38), appCtx.Output will be an empty string (zero value). This means the outputFmt returned will be empty, causing error formatting to fall into the else branch, which may not be the intended behavior.
Consider resolving the output format from the flags directly in Execute() before calling rootCmd.Execute(), or provide a default value like "table" when appCtx.Output is empty.
| json.NewEncoder(os.Stderr).Encode(errObj) | ||
| } else { | ||
| // Plain text error to stderr. | ||
| fmt.Fprintf(os.Stderr, "Error: %s\n", err) |
There was a problem hiding this comment.
The plain text error output only prints the error message without the hint. In contrast, the JSON error output includes the hint (lines 28-30). This creates an inconsistent user experience where hints are only shown when using JSON output.
Consider also printing the hint for plain text errors. For example, if there's an APIError with a hint, print both the error and the hint to stderr.
| fmt.Fprintf(os.Stderr, "Error: %s\n", err) | |
| fmt.Fprintf(os.Stderr, "Error: %s\n", err) | |
| var apiErr *model.APIError | |
| if errors.As(err, &apiErr) { | |
| if hint := apiErr.Hint(); hint != "" { | |
| fmt.Fprintf(os.Stderr, "Hint: %s\n", hint) | |
| } | |
| } |
| func (e *APIError) LikelyAuthError() bool { | ||
| if e.Status != 400 { | ||
| return false | ||
| } | ||
| // Zenodo returns a "validation error" with empty field messages when the | ||
| // token is invalid on /deposit/depositions endpoints. | ||
| for _, d := range e.Errors { | ||
| if d.Field != "" && d.Message == "" { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The new LikelyAuthError() method lacks test coverage. Given that this codebase has comprehensive test coverage (as seen in internal/api/client_test.go with tests for APIError), consider adding unit tests for this new method.
Tests should cover:
- Returns false for non-400 status codes
- Returns true when a 400 error has Details with non-empty Field and empty Message
- Returns false when a 400 error has other Detail patterns (e.g., both Field and Message present)
| switch e.Status { | ||
| case 400: | ||
| if e.LikelyAuthError() { | ||
| return "This may be caused by an invalid API token. Check your token with: zenodo config set token <TOKEN>" |
There was a problem hiding this comment.
The hint message uses a different format than the 401 and 403 cases. Line 31 says "Check your token with: zenodo config set token" while lines 35 and 37 use "Set it with:" and "Generate a new token at:". For consistency and clarity, consider using a more parallel structure. For example, "This may be caused by an invalid API token. Set it with: zenodo config set token <TOKEN>" to match the style of line 35.
| return "This may be caused by an invalid API token. Check your token with: zenodo config set token <TOKEN>" | |
| return "This may be caused by an invalid API token. Set it with: zenodo config set token <TOKEN>" |
Summary
LikelyAuthError()method onAPIErrorto identify 400s with empty field messages (Zenodo's quirk for bad tokens)ELI5
When you use a bad API token, Zenodo says "validation error" instead of "unauthorized". The CLI now recognizes this pattern and gives you exit code 2 (auth error) with a hint to check your token, instead of a confusing exit code 1 with "size:" gibberish.
Also, when you use
-o json, errors now come out as proper JSON instead of plain text.Test results
records list --token "bad"records get 99999999999records list -o json --token "bad"{"status":400,"hint":"...","code":2}Test plan
go build ./...passesgo test ./...passesCloses the error handling section of #47.
🤖 Generated with Claude Code