fix: improve raw API diagnostics for invalid or empty JSON responses#257
fix: improve raw API diagnostics for invalid or empty JSON responses#257vuth-dogo wants to merge 2 commits intolarksuite:mainfrom
Conversation
Wrap JSON decode failures from DoAPI and HandleResponse in api_error exit details with a hint to use --output when the endpoint may return non-JSON payloads. Co-Authored-By: Paperclip <noreply@paperclip.ing> Made-with: Cursor
github-actions
bot
added
the
size/L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughReplace generic network error handling for API responses with specialized error wrappers that detect JSON decode failures and empty JSON bodies; add a new client error utility and tests asserting API errors include diagnostic messages and hints. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
|
|
Greptile SummaryThis PR improves Key changes:
Confidence Score: 5/5Safe to merge; all three previously raised concerns have been addressed and no new issues found. All prior review concerns (bare io.EOF misclassification, dead errors.Is/errors.As code due to %v wrapping, and fragile test assertion) are resolved in this revision. No remaining P0 or P1 issues were found. The design correctly gates io.EOF/io.ErrUnexpectedEOF checks behind allowEOF, the %w change preserves the error chain as needed, and test coverage is solid for the new paths. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[lark-cli api GET /path] --> B[ac.DoAPI]
B --> C{err != nil?}
C -->|yes| D[WrapDoAPIError]
D --> E{isJSONDecodeError allowEOF=false}
E -->|SyntaxError or invalid character| F[ExitAPI + --output hint]
E -->|bare io.EOF or other| G[ExitNetwork]
C -->|no| H[HandleResponse]
H --> I[ParseJSONResponse uses %w]
I --> J{decode err?}
J -->|yes| K[WrapJSONResponseParseError]
K --> L{body empty?}
L -->|yes| M[ExitAPI empty JSON body + hint]
L -->|no| N{isJSONDecodeError allowEOF=true}
N -->|yes| O[ExitAPI invalid JSON + hint]
N -->|no| P[ExitNetwork]
J -->|no| Q[CheckLarkResponse and output result]
Reviews (2): Last reviewed commit: "fix: tighten API JSON decode error class..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/client/api_errors.go`:
- Around line 54-61: The JSON error classification currently checks for io.EOF
and string substrings but misses io.ErrUnexpectedEOF and the "unexpected EOF"
message and also fails when the original error is wrapped with "%v"; update the
error check block (the code that tests errors.Is(err, io.EOF) || errors.As(err,
&syntaxErr) || errors.As(err, &unmarshalTypeErr)) to also check errors.Is(err,
io.ErrUnexpectedEOF) and to test strings.Contains(msg, "unexpected EOF"), and
then change the error wrapping where the response error is created (the place
that uses fmt.Errorf("%v", err)) to use fmt.Errorf("%w", err) so errors.Is/As
still work when the error is wrapped; reference the existing variables err,
syntaxErr, unmarshalTypeErr when making these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 467a6e6f-7d55-42ad-bb06-6655db8d1c4f
📒 Files selected for processing (5)
cmd/api/api.gocmd/api/api_test.gointernal/client/api_errors.gointernal/client/response.gointernal/client/response_test.go
internal/client/api_errors.go
Outdated
| if errors.Is(err, io.EOF) || errors.As(err, &syntaxErr) || errors.As(err, &unmarshalTypeErr) { | ||
| return true | ||
| } | ||
|
|
||
| msg := err.Error() | ||
| return strings.Contains(msg, "unexpected end of JSON input") || | ||
| strings.Contains(msg, "invalid character") || | ||
| strings.Contains(msg, "cannot unmarshal") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check ParseJSONResponse wrapping style:"
rg -n 'response parse error: %.' internal/client/response.go -n -C2
echo
echo "Check JSON-decode detection branches:"
rg -n 'errors\.Is\(err, io\.(EOF|ErrUnexpectedEOF)\)|unexpected EOF|unexpected end of JSON input|invalid character|cannot unmarshal' internal/client/api_errors.go -n -C2Repository: larksuite/cli
Length of output: 728
🏁 Script executed:
cat go.mod | head -5Repository: larksuite/cli
Length of output: 110
🏁 Script executed:
rg -n 'fmt\.Errorf.*%v' internal/client/ --type goRepository: larksuite/cli
Length of output: 202
🏁 Script executed:
rg -n 'json\.Decoder|dec\.Decode' internal/client/ --type go -A3Repository: larksuite/cli
Length of output: 366
🌐 Web query:
Go json.Decoder truncated JSON error message "unexpected EOF"
💡 Result:
The "unexpected EOF" error from Go's json.Decoder typically indicates truncated or incomplete JSON input, where the parser reaches the end of available data before completing a valid JSON value (e.g., missing closing quote, bracket, or brace). Key causes and solutions: 1. Truncated input stream: Common with HTTP requests/bodies or pipes where data ends prematurely. - Ensure full body read: Use io.ReadAll(r.Body) then json.Unmarshal, or verify Content-Length matches. - Example from HTTP handler: Multiple requests (OPTIONS preflight + POST) exhaust body; check r.Method == "POST". 2. Syntax errors mimicking truncation: Unclosed strings/objects/arrays like {"key": "value trigger SyntaxError with "unexpected EOF" message. - Validate JSON: json.Valid(data) before decoding. - Source code shows decoder returns io.ErrUnexpectedEOF (wrapped as SyntaxError) on premature EOF during token scan. 3. Decoder vs Unmarshal difference: Decoder.Decode historically returned io.ErrUnexpectedEOF for truncated input; Unmarshal used SyntaxError{"unexpected end of JSON input"}. Issue #25956 discusses alignment (ongoing in json/v2). 4. Streaming edge cases: Pipes/files signal EOF after partial writes; recreate decoder or read fully. To debug: dec := json.NewDecoder(r) err := dec.Decode(&v) if err != nil { if serr, ok := err.(*json.SyntaxError); ok { fmt.Printf("Syntax error at offset %d: %v\n", serr.Offset, err) } } Use dec.InputOffset for error position. For strict validation, call dec.DisallowUnknownFields. Prevent via full reads or input validation.
Citations:
- 1: https://pkg.go.dev/encoding/json/
- 2: encoding/json: eof error of NewDecoder().Decode() should be same with Unmarshal() golang/go#25956
- 3: https://pkg.go.dev/encoding/json@go1.25.6
- 4: https://go.dev/src/encoding/json/stream.go
- 5: https://go.dev/src/encoding/json/decode.go
- 6: https://stackoverflow.com/questions/27994327/golang-json-unmarshal-unexpected-end-of-json-input
🏁 Script executed:
rg -n 'fmt\.Errorf' internal/client/ --type go | grep -i 'decode\|json\|parse'Repository: larksuite/cli
Length of output: 202
🏁 Script executed:
python3 << 'EOF'
# Check what the error message for io.ErrUnexpectedEOF is
# by examining its string representation in Go stdlib
import subprocess
result = subprocess.run(['rg', '-n', 'ErrUnexpectedEOF', '.', '--type', 'go'],
capture_output=True, text=True)
if result.stdout:
print("Found ErrUnexpectedEOF references in repo:")
print(result.stdout)
else:
print("No ErrUnexpectedEOF references in repo (expected - it's stdlib)")
# Check if there are tests or examples showing JSON decoder errors
result2 = subprocess.run(['rg', '-n', 'Decode.*error|json.*decode.*error',
'internal/client/', '--type', 'go', '-i'],
capture_output=True, text=True)
if result2.stdout:
print("\nJSON decode error patterns in tests/code:")
print(result2.stdout[:1000])
EOFRepository: larksuite/cli
Length of output: 469
🏁 Script executed:
# Verify the actual error message from io.ErrUnexpectedEOF
python3 << 'EOF'
# In Go, io.ErrUnexpectedEOF has the message "unexpected EOF"
# This is confirmed by the Go stdlib source
# Let's verify response.go line 114 is the only place with this pattern
import subprocess
result = subprocess.run(['rg', '-n', 'Errorf.*parse', 'internal/client/', '--type', 'go'],
capture_output=True, text=True)
print("All parse error wrappings in internal/client:")
print(result.stdout)
EOFRepository: larksuite/cli
Length of output: 249
Add checks for io.ErrUnexpectedEOF and "unexpected EOF" to detect truncated JSON.
Lines 54–61 miss truncated JSON responses that json.Decoder.Decode() emits as io.ErrUnexpectedEOF. Since response.go:114 wraps errors with %v instead of %w, the direct error type is lost and only substring matching remains. The fallback checks also omit "unexpected EOF", allowing truncated payloads to be misclassified as network errors.
Add the missing checks and fix the error wrapping:
Proposed fixes
diff --git a/internal/client/api_errors.go b/internal/client/api_errors.go
@@
- if errors.Is(err, io.EOF) || errors.As(err, &syntaxErr) || errors.As(err, &unmarshalTypeErr) {
+ if errors.Is(err, io.EOF) ||
+ errors.Is(err, io.ErrUnexpectedEOF) ||
+ errors.As(err, &syntaxErr) ||
+ errors.As(err, &unmarshalTypeErr) {
return true
}
@@
- return strings.Contains(msg, "unexpected end of JSON input") ||
+ return strings.Contains(msg, "unexpected EOF") ||
+ strings.Contains(msg, "unexpected end of JSON input") ||
strings.Contains(msg, "invalid character") ||
strings.Contains(msg, "cannot unmarshal")diff --git a/internal/client/response.go b/internal/client/response.go
@@
- if err := dec.Decode(&result); err != nil {
- return nil, fmt.Errorf("response parse error: %v (body: %s)", err, util.TruncateStr(string(resp.RawBody), 500))
+ if err := dec.Decode(&result); err != nil {
+ return nil, fmt.Errorf("response parse error: %w (body: %s)", err, util.TruncateStr(string(resp.RawBody), 500))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if errors.Is(err, io.EOF) || errors.As(err, &syntaxErr) || errors.As(err, &unmarshalTypeErr) { | |
| return true | |
| } | |
| msg := err.Error() | |
| return strings.Contains(msg, "unexpected end of JSON input") || | |
| strings.Contains(msg, "invalid character") || | |
| strings.Contains(msg, "cannot unmarshal") | |
| if errors.Is(err, io.EOF) || | |
| errors.Is(err, io.ErrUnexpectedEOF) || | |
| errors.As(err, &syntaxErr) || | |
| errors.As(err, &unmarshalTypeErr) { | |
| return true | |
| } | |
| msg := err.Error() | |
| return strings.Contains(msg, "unexpected EOF") || | |
| strings.Contains(msg, "unexpected end of JSON input") || | |
| strings.Contains(msg, "invalid character") || | |
| strings.Contains(msg, "cannot unmarshal") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/client/api_errors.go` around lines 54 - 61, The JSON error
classification currently checks for io.EOF and string substrings but misses
io.ErrUnexpectedEOF and the "unexpected EOF" message and also fails when the
original error is wrapped with "%v"; update the error check block (the code that
tests errors.Is(err, io.EOF) || errors.As(err, &syntaxErr) || errors.As(err,
&unmarshalTypeErr)) to also check errors.Is(err, io.ErrUnexpectedEOF) and to
test strings.Contains(msg, "unexpected EOF"), and then change the error wrapping
where the response error is created (the place that uses fmt.Errorf("%v", err))
to use fmt.Errorf("%w", err) so errors.Is/As still work when the error is
wrapped; reference the existing variables err, syntaxErr, unmarshalTypeErr when
making these changes.
- Do not treat bare io.EOF from DoAPI as JSON parse path - Treat ErrUnexpectedEOF / partial decode as JSON diagnostics in response path - Wrap decode errors with %w in ParseJSONResponse for errors.Is - Adjust tests and add api_errors unit coverage Co-Authored-By: Paperclip <noreply@paperclip.ing> Made-with: Cursor
|
Pushed follow-up commit
Validation (local, |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
2 participants
Summary
Improves
lark-cli apiUX when an endpoint returns an empty body or non-JSON content that still fails JSON parsing: failures are reported asapi_errorwith a clear message and a hint to use--outputfor file/binary-style responses (see #215).Changes
internal/client/api_errors.gowithWrapDoAPIErrorandWrapJSONResponseParseErrorto classify JSON decode issues vs generic network errors.cmd/apiDoAPI errors andHandleResponseJSON parse errors through the new helpers.cmd/apiandinternal/client.Test Plan
go test ./cmd/api ./internal/client -run 'TestApiCmd_InvalidJSONResponse_ShowsDiagnostic|TestHandleResponse_EmptyJSONBody_ShowsDiagnostic'go mod tidy(nogo.mod/go.sumchanges)golangci-lint--new-from-rev=origin/main(0 issues)GOTOOLCHAIN=go1.23.8 make unit-test— pass (darwin/arm64); confirms full suite is green when pinned to Go 1.23.8 as in coordinator validation.go1.26.0on the same host still hitsSIGSEGVin the race-enabled unit suite for some packages; CI / supported Go version should be treated as the canonical gate for Go 1.26+ behavior.Related Issues
Summary by CodeRabbit
--outputflag for troubleshooting.