fix(cli): reject fetchJson on malformed JSON or response stream errors - #28875
fix(cli): reject fetchJson on malformed JSON or response stream errors#28875chelsealong wants to merge 2 commits into
Conversation
Malformed or truncated GitHub API responses returned with HTTP 200 threw an uncaught SyntaxError from inside the response 'end' callback instead of rejecting the fetchJson() promise, and the response stream had no 'error' listener. Wrap JSON.parse in try/catch and listen for stream errors so both cases reject with contextual errors that the caller's existing error handling can report. Fixes google-gemini#28646
|
📊 PR Size: size/M
|
gemini-cli
Bot
added
priority/p2
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the robustness of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves error handling in the fetchJson function by catching response stream errors and handling malformed JSON parsing gracefully, with corresponding unit tests added. The feedback suggests handling the potentially undefined statusCode property on the IncomingMessage object by using a nullish coalescing operator to provide a fallback value in the error messages.
| res.on('error', (err) => { | ||
| reject( | ||
| new Error( | ||
| `Response stream error while fetching ${url} (status ${res.statusCode}): ${err.message}`, | ||
| ), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The statusCode property on IncomingMessage is optional and can be undefined. According to the repository's general rules, when consuming an object where a property is optional in its type definition, callers must handle the undefined case (e.g., by providing a default with ??). Please use a nullish coalescing operator to provide a fallback value.
res.on('error', (err) => {
reject(
new Error(
'Response stream error while fetching ' +
url +
' (status ' +
(res.statusCode ?? 'unknown') +
'\nnetwork error: ' +
err.message,
),
);
});References
- When consuming an object, if a property is optional in its type definition (interface), callers must handle the undefined case (e.g., by providing a default with ??).
| reject( | ||
| new Error( | ||
| `Failed to parse JSON from ${url} (status ${res.statusCode}): ${ | ||
| err instanceof Error ? err.message : String(err) | ||
| }`, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
The statusCode property on IncomingMessage is optional and can be undefined. According to the repository's general rules, when consuming an object where a property is optional in its type definition, callers must handle the undefined case (e.g., by providing a default with ??). Please use a nullish coalescing operator to provide a fallback value.
reject(
new Error(
'Failed to parse JSON from ' +
url +
' (status ' +
(res.statusCode ?? 'unknown') +
'\nnetwork error: ' +
(err instanceof Error ? err.message : String(err)),
),
);References
- When consuming an object, if a property is optional in its type definition (interface), callers must handle the undefined case (e.g., by providing a default with ??).
…hJson error messages
|
Addressed both inline comments: added |
1 participant
Summary
fetchJson()inpackages/cli/src/config/extensions/github_fetch.tscollectsresponse chunks and calls
JSON.parse(data)inside the response'sendeventcallback with no
try/catch, and the response stream has noerrorlistener.A malformed or truncated JSON body returned with HTTP 200 throws a
SyntaxErrorfrom that callback instead of rejecting the returned promise,so extension commands can crash instead of receiving an actionable error.
Fix
JSON.parseintry/catchand reject with a contextual errorincluding the URL and status code.
errorlistener that rejects with a contextualerror instead of leaving the promise pending or throwing unhandled.
This is a minimal, targeted fix scoped to the exact defect described in the
issue; it does not change redirect handling, header handling, or any other
behavior of
fetchJson().Related Issues
Fixes #28646
Tests
Added two regression tests to
github_fetch.test.ts:"Failed to parse JSON from ..." error instead of throwing.
errorevent now rejects with a contextual"Response stream error while fetching ..." error.
Verified both new tests fail against the pre-fix code
(
git checkout HEAD~1 -- packages/cli/src/config/extensions/github_fetch.ts)with:
And passing after restoring the fix:
How to Validate
All pass:
github_fetch.test.ts: 10/10 tests passingpackages/cli/src/config/extensions/: 181/181 tests passing (no regressions)Pre-Merge Checklist
surface as promise rejections with more context, matching the
existing behavior for other
fetchJson()failure modes.AI assistance disclosure
This change was prepared with the assistance of an AI coding agent
(Claude, Anthropic) operating under human supervision.