fix: replace panic-prone .unwrap() calls with proper error handling i… #264
Conversation
Summary of ChangesHello @jmanhype, 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 significantly enhances the application's resilience by replacing several 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 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 improves robustness by replacing several .unwrap() calls with proper error handling, preventing potential panics. The change to handle MIME type parsing errors is well-implemented. However, the error handling for JSON patch deserialization introduces a logic flaw: on failure, it falls back to sending an unfiltered patch, which defeats the purpose of the filtering and could leak data intended to be hidden. I've left a comment with a suggestion to fix this.
forge-app/src/router.rs
Outdated
| Err(e) => { | ||
| tracing::error!("Failed to deserialize filtered JSON patch: {}", e); | ||
| return Some(Ok(LogMsg::JsonPatch(patch))); | ||
| } |
There was a problem hiding this comment.
Replacing .unwrap() is a great improvement for robustness. However, the new error handling for JSON patch deserialization introduces a logic flaw. In the Err arm, you fall back to returning the original, unfiltered patch. This defeats the purpose of the filtering logic in this block and will leak agent task data to the client, which is supposed to be hidden.
While the PR description mentions this as a fallback, it's a correctness issue. It would be safer to drop the message entirely if the filtered patch cannot be correctly created. You can achieve this by returning None from the filter_map closure.
Err(e) => {
tracing::error!("Failed to deserialize filtered JSON patch: {}", e);
// Returning the original unfiltered patch would leak agent tasks to the client.
// It's safer to drop this update if filtering fails.
return None;
}
…n router - Replace JSON deserialization unwrap with match expression and fallback to original patch on error - Replace MIME parsing unwrap with unwrap_or_else and fallback to application/octet-stream - Add tracing logs for error cases to aid debugging This prevents production crashes on edge cases while maintaining graceful degradation. Co-authored-by: Automagik Genie 🧞 <genie@namastex.ai>
jmanhype
force-pushed
the
claude/fix-unwrap-error-handling-01EMmuRUvbYXAZX78VbZWBBQ
branch
from
6b63787 to
933caf3
Compare
|
@codex review this pr |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review this pr |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
3 participants
…n router
This prevents production crashes on edge cases while maintaining graceful degradation.