fix(http): route error and empty-result messages to stderr#485
fix(http): route error and empty-result messages to stderr#485abhiram304 wants to merge 1 commit intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: d3b32d3 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 refines the application's output behavior by ensuring that human-readable messages and API error details are directed to Highlights
Changelog
Activity
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly routes human-readable messages and HTTP error bodies to stderr, which is a great improvement for consumers of the CLI that pipe stdout to other tools like jq. The changes in gmail/triage.rs and modelarmor.rs are logical and well-implemented. I've added a couple of comments regarding the newly added tests, with suggestions to make them more robust and less prone to giving a false sense of security.
Two violations were breaking pipe-friendly use of the CLI: 1. gmail/triage.rs printed "No messages found…" via println! on both the null-messages and empty-array paths, corrupting stdout for `gws gmail +triage | jq` pipelines. Changed to eprintln!. 2. modelarmor.rs printed the raw API response body unconditionally (before the status check), so on HTTP error the error body leaked to stdout instead of surfacing in the error return. Moved the println! inside the success branch and included the body in the Err message for better diagnostics.
fa34e93 to
d3b32d3
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly routes error and informational messages to stderr instead of stdout, which is a great improvement for pipeline-based workflows. The changes in gmail/triage.rs and modelarmor.rs are well-targeted to fix this issue. I've added a comment with a suggestion to improve testing robustness, pointing out that a new test in modelarmor.rs doesn't fully test the intended error path, which is crucial for ensuring correct error message routing.
Summary
gmail/triage.rswas printing"No messages found matching query: {query}"viaprintln!on both the null-messages and empty-array paths — this corrupted stdout forgws gmail +triage | jqstyle pipelines. Changed toeprintln!.modelarmor.rs(model_armor_post) was printing the raw API response body unconditionally before the status check, so on HTTP 4xx/5xx the error body leaked to stdout. Movedprintln!("{text}")inside the success branch and included{text}in theErrmessage so callers get actionable diagnostics without stdout pollution.Test plan
gws gmail +triage --query label:inbox 2>/dev/null | jq .— stdout is valid JSON only, empty-result message appears on stderrcargo test— all tests pass (triage empty-result message test, modelarmor error path test)🤖 Generated with Claude Code