fix(formatter): exit non-zero on JSON serialization failure instead of silent empty output#828
Conversation
…f silent empty output
🦋 Changeset detectedLatest commit: 08643d9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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 addresses an issue where JSON serialization failures were being silently swallowed, resulting in empty output and a successful exit code. By implementing explicit error handling in the formatter, the CLI now provides actionable feedback to the user when serialization fails, improving overall robustness and debugging capabilities. 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
|
googleworkspace-bot
added
the
area: core
![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 during JSON serialization by printing an error message to stderr and exiting with a non-zero status code instead of silently returning an empty string. It also adds a corresponding unit test and a changeset file. The review feedback highlights a security concern regarding potential terminal escape sequence injection, recommending that the error messages printed to stderr be sanitized using crate::output::sanitize_for_terminal.
…jection Wrap the serialization error string with sanitize_for_terminal before printing to stderr, as the error text could theoretically contain user-controlled data with embedded escape sequences.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the JSON formatting functions (format_value and format_value_paginated) to print an error message to stderr and exit with a non-zero code when JSON serialization fails, rather than returning an empty string. The reviewer correctly identified that calling std::process::exit(1) inside these formatting utility functions is an anti-pattern because it bypasses RAII cleanup, makes the code untestable for error paths, and tightly couples formatting logic to the process lifecycle. The reviewer recommends refactoring both functions to return a Result instead, propagating the error to the caller.
Change format_value and format_value_paginated to return Result<String, serde_json::Error> so that serialization failures propagate to callers rather than aborting the process mid-flight. All call sites now use .map_err(|e| GwsError::Other(...))? or (in format_and_print) return Result<(), GwsError>. This fixes the RAII bypass and makes both functions testable for error paths without killing the test runner. Resolves Gemini r2 comment on PR googleworkspace#828.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the CLI to exit with a non-zero code and a clear error message when JSON serialization fails, changing the formatting functions to return a Result and propagating these errors across the codebase. While the error handling is a great improvement, several UTF-8 characters—including table separators, ellipses, emojis, and accented test strings—were corrupted into Mojibake during the changes and must be restored to prevent broken terminal layouts and garbled output.
The refactor-to-return-Result commit double-encoded several Unicode literals via a Windows-1252/Latin-1 round-trip. Restore the original code points: â"€ (mojibake) → ─ (U+2500 box drawing horizontal) â€" (mojibake) → — (U+2014 em dash) … (mojibake) → … (U+2026 horizontal ellipsis) â†' (mojibake) → → (U+2192 right arrow) 😀 (mojibake) → 😀 (U+1F600 emoji) é (mojibake) → é (U+00E9) ï (mojibake) → ï (U+00EF)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request modifies the CLI formatter to return a Result<String, serde_json::Error> instead of a plain String when formatting values. This allows the application to handle JSON serialization errors gracefully and exit with a non-zero status and a clear error message instead of silently printing empty stdout. The error propagation has been integrated across various response handlers, executors, and helper modules (calendar, triage, workflows), and the corresponding unit tests have been updated to unwrap the results. I have no additional feedback to provide as no review comments were submitted.
2 participants
Summary
When
serde_json::to_string_prettyfailed (e.g. on values containing NaN/Infinity),format_valuereturned an empty string viaunwrap_or_default(). The CLI then printed nothing and exited 0, silently swallowing the API response.This change replaces the silent fallback with an explicit stderr error message and a non-zero exit code. The same fix is applied to
format_value_paginatedwhich had the identical issue.Fixes #740
Changes
format_value: replacedunwrap_or_default()with an explicitmatchthat printserror: failed to serialize response to JSON: {e}to stderr and exits with code 1format_value_paginated: same fix for the compact JSON pathtest_format_value_json_non_emptytest to assert valid values always produce non-empty outputChecklist
cargo fmt --allpassedcargo clippy -- -D warningspassedcargo testpassed