fix: colorize stderr warnings and errors on TTY terminals#394
fix: colorize stderr warnings and errors on TTY terminals#394seang1121 wants to merge 3 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 9267f3f 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 |
googleworkspace-bot
added
area: http
area: core
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 enhances the user experience by introducing colorized output for warnings and errors displayed on 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
|
![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 introduces colorized output for warnings and errors on TTY terminals, which is a nice enhancement for user experience. The implementation correctly uses std::io::IsTerminal to detect TTYs and adds helper functions for different colors. The changes are applied to several error/warning messages across the codebase. My main feedback is to refactor the new color helper functions to reduce code duplication, which will improve long-term maintainability.
src/error.rs
Outdated
| pub(crate) fn yellow(s: &str) -> String { | ||
| if is_tty() { | ||
| format!("\x1b[33m{s}\x1b[0m") | ||
| } else { | ||
| s.to_string() | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn red(s: &str) -> String { | ||
| if is_tty() { | ||
| format!("\x1b[31m{s}\x1b[0m") | ||
| } else { | ||
| s.to_string() | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn bold(s: &str) -> String { | ||
| if is_tty() { | ||
| format!("\x1b[1m{s}\x1b[0m") | ||
| } else { | ||
| s.to_string() | ||
| } | ||
| } |
There was a problem hiding this comment.
There's some code duplication across the yellow, red, and bold functions. The logic for checking is_tty() and formatting the string is identical in all three. You can refactor this into a single private helper function to improve maintainability. This way, if the logic needs to change in the future, you'll only have to update it in one place.
fn colorize(s: &str, code: &str) -> String {
if is_tty() {
format!("\x1b[{code}m{s}\x1b[0m")
} else {
s.to_string()
}
}
pub(crate) fn yellow(s: &str) -> String {
colorize(s, "33")
}
pub(crate) fn red(s: &str) -> String {
colorize(s, "31")
}
pub(crate) fn bold(s: &str) -> String {
colorize(s, "1")
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces colored output for warnings and errors on TTY terminals, which is a great enhancement for user experience. The implementation correctly checks for TTY before adding ANSI color codes. My review includes one high-severity suggestion in src/executor.rs to prevent potential ANSI injection from error messages, making the implementation safer and more consistent with other changes in this PR.
src/executor.rs
Outdated
| } | ||
| Err(e) => { | ||
| eprintln!("⚠️ Model Armor sanitization failed: {e}"); | ||
| eprintln!("{}", crate::error::yellow(&format!("⚠️ Model Armor sanitization failed: {e}"))); |
There was a problem hiding this comment.
Coloring the entire formatted string, which includes the error e, could lead to ANSI injection if the error's Display implementation contains malicious escape codes. This could corrupt terminal output.
A safer approach, consistent with the change in src/main.rs, is to only color the static part of the warning message.
| eprintln!("{}", crate::error::yellow(&format!("⚠️ Model Armor sanitization failed: {e}"))); | |
| eprintln!("{} {e}", crate::error::yellow("⚠️ Model Armor sanitization failed:")); |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a nice usability improvement by colorizing warnings and errors on TTY terminals. The implementation is clean and avoids new dependencies by using the Rust standard library. I've identified a potential security vulnerability in the new colorize function that should be addressed. Additionally, I have a suggestion to improve the newly added tests for better clarity and maintainability. Overall, this is a great addition.
| fn colorize(s: &str, code: &str) -> String { | ||
| if is_tty() { | ||
| format!("\x1b[{code}m{s}\x1b[0m") | ||
| } else { | ||
| s.to_string() | ||
| } | ||
| } |
There was a problem hiding this comment.
The colorize function directly includes the input string s in the formatted output. If a string containing ANSI escape codes is passed to yellow, red, or bold, it could lead to terminal escape injection. This would allow an attacker to manipulate the terminal's output, or in some terminals, execute commands. While the current usages in this PR are safe as they use static strings, future uses with dynamic or user-controlled data could introduce a vulnerability. To mitigate this, the input string should be sanitized to remove any existing escape codes before being wrapped in new ones.
| fn colorize(s: &str, code: &str) -> String { | |
| if is_tty() { | |
| format!("\x1b[{code}m{s}\x1b[0m") | |
| } else { | |
| s.to_string() | |
| } | |
| } | |
| fn colorize(s: &str, code: &str) -> String { | |
| if is_tty() { | |
| // Basic sanitization to prevent terminal escape injection. | |
| // This removes the ESC character, which is the start of most control sequences. | |
| let sanitized_s = s.replace('\x1b', ""); | |
| format!("\x1b[{code}m{sanitized_s}\x1b[0m") | |
| } else { | |
| s.to_string() | |
| } | |
| } |
| // In test environments stderr is not a TTY, so the helpers return plain strings. | ||
| #[test] | ||
| fn test_yellow_non_tty() { | ||
| // Stderr is not a TTY in CI/test runners; plain string is returned. | ||
| let result = yellow("hello"); | ||
| // Either plain or ANSI-wrapped depending on environment; must contain the text. | ||
| assert!(result.contains("hello")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_red_non_tty() { | ||
| let result = red("error"); | ||
| assert!(result.contains("error")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_bold_non_tty() { | ||
| let result = bold("important"); | ||
| assert!(result.contains("important")); | ||
| } |
There was a problem hiding this comment.
The tests test_yellow_non_tty, test_red_non_tty, and test_bold_non_tty are redundant and their assertions are too weak to be meaningful. The assert!(result.contains(...)) check passes even if the string is colorized, so it doesn't verify the "plain string is returned" behavior mentioned in the comment. The test_color_helpers_no_ansi_when_non_tty test already covers this scenario correctly and more robustly. To improve test suite clarity, correctness, and maintainability, these three redundant tests should be removed.
2 participants
Closes #317
Summary
pub(crate)ANSI color helpers (yellow,red,bold) and anis_tty()check tosrc/error.rsstd::io::IsTerminal); plain text is emitted otherwise (pipes, CI, redirected output)IsTerminaltrait already available on the Rust stdlib (stabilized 1.70)Changed call sites:
src/error.rs💡 API not enabled…(accessNotConfigured)src/main.rswarning: unknown output formatwarning:prefix)src/executor.rs⚠️ Model Armor: prompt injection detectedsrc/executor.rs⚠️ Model Armor sanitization failedTest plan
cargo test— new unit tests inerror.rsverify helpers return plain strings in non-TTY environmentsgws drive files list --format badformatin a real terminal and confirmwarning:appears in yellowgws drive files list --format badformat 2>&1 | cat) and confirm no ANSI escape codes appear