fix: replace unwrap() with proper error handling in MCP server

[Bortlesboat]

Summary

• Replace panic-prone unwrap() calls with safe alternatives in mcp_server.rs
• Handle broken stdout pipe by breaking the server loop instead of silently continuing to process undeliverable requests
• Add unit test coverage for build_mcp_cli, walk_resources, and handle_request

Continues the work from #109 which addressed some MCP server unwraps.

Changes

Location	Before	After
get_one("services")	.unwrap()	.unwrap_or("")
req.get("id")	.unwrap() after is_none() check	if let Some(id) pattern
tools_cache.as_ref()	.unwrap()	match expression
parts.last()	.unwrap()	.ok_or_else() with validation error
stdout write	let _ = (silent)	break loop on pipe error

Test plan

• Added 10 unit tests covering CLI parsing, resource walking, and request handling
• CI: cargo test, cargo clippy, cargo fmt

[changeset-bot]

🦋 Changeset detected

Latest commit: 4721bed

The changes in this PR will be included in the next version bump.

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

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! 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 stability and reliability of the MCP server by replacing panic-prone unwrap() calls with more robust error handling patterns. It also introduces a mechanism to gracefully shut down the server when the stdout pipe is broken, preventing the server from continuing to process requests that cannot be delivered. Furthermore, new unit tests have been added to ensure the correct behavior of CLI parsing, resource walking, and request handling, improving overall code quality and maintainability.

Highlights

• Improved Error Handling: Replaced panic-prone unwrap() calls with safer alternatives like unwrap_or(""), if let Some, match expressions, and ok_or_else() across various parts of the MCP server.
• Robust Stdout Pipe Management: Implemented a mechanism to detect broken stdout pipes during write operations and gracefully shut down the server loop, preventing silent failures and unhandled requests.
• Expanded Unit Test Coverage: Added new unit tests for build_mcp_cli, walk_resources, and handle_request functions, enhancing the test suite's robustness.

Changelog

• .changeset/fix-mcp-server-unwrap.md
  • Added a new changeset file detailing the error handling improvements and new unit tests for the MCP server.
• src/mcp_server.rs
  • Replaced .unwrap() with .map(|s| s.as_str()).unwrap_or("") for matches.get_one("services") to provide a default empty string.
  • Refactored request ID handling from an is_none() check followed by unwrap() to a safer if let Some(id) pattern.
  • Modified stdout write operations to check for errors and break the server loop if the pipe is broken, logging a message to eprintln!.
  • Replaced .unwrap() on tools_cache.as_ref() with a match expression to handle None cases by returning an empty JSON array.
  • Replaced .unwrap() on parts.last() with .ok_or_else() to return a GwsError::Validation if the method name is missing.
  • Added a new #[cfg(test)] mod tests block containing unit tests for build_mcp_cli, walk_resources, and handle_request methods.

Activity

• No specific activity (comments, reviews, or progress updates) has been recorded for this pull request beyond its initial creation.

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request does a good job of improving error handling in the MCP server by replacing several unwrap() calls with safer alternatives. The changes to handle broken stdout pipes and the addition of unit tests are also valuable improvements. I've found a couple of places where the new error handling logic is on an unreachable path, which could be confusing. My review includes suggestions to make these cases clearer and more explicit.

I am having trouble creating individual review comments. Click here to see my feedback.

src/mcp_server.rs (393-395)

This ok_or_else call is on an unreachable path. The split method on a &str always returns a non-empty iterator, so parts.last() will never be None. The original unwrap() was safe. Since the goal of this PR is to remove unwrap(), using expect() with an explanation is a good alternative to document why this operation is safe.

    let method_name = parts.last().expect("split() on a &str always yields at least one element");

src/mcp_server.rs (200-206)

The None branch of this match is unreachable because the if block on line 197 guarantees that tools_cache is Some. While the current fallback to an empty array is safe, it can be misleading for future maintainers. Using unreachable! makes the code's invariant explicit and will cause a panic if this assumption is ever violated, which is helpful for debugging.

            let tools = match tools_cache.as_ref() {
                Some(t) => json!(t),
                None => unreachable!("tools_cache is guaranteed to be populated by the check above"),
            };
            Ok(json!({
                "tools": tools
            }))

[codecov]

Codecov Report

❌ Patch coverage is 83.07692% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.92%. Comparing base (473dd30) to head (68e2365).
⚠️ Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
src/mcp_server.rs	83.07%	22 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #189      +/-   ##
==========================================
+ Coverage   56.21%   56.92%   +0.70%     
==========================================
  Files          38       38              
  Lines       13853    13973     +120     
==========================================
+ Hits         7788     7954     +166     
+ Misses       6065     6019      -46     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[googleworkspace-bot]

/gemini review

[gemini-code-assist]

Code Review

This pull request significantly improves the robustness of the MCP server by replacing several unwrap() calls with proper error handling and adding checks for broken stdout pipes. The changes are well-implemented and the addition of unit tests is a great enhancement. I have one suggestion to further improve error handling by replacing an expect() call with ok_or_else to align with the PR's goal of avoiding panics, as mentioned in the description.

[gemini-code-assist]

While parts.last() is not expected to be None here due to the parts.len() < 3 check, using expect() still introduces a potential panic. The pull request description mentions replacing this with .ok_or_else(), which would be a more robust approach that aligns better with the goal of removing panics. Returning a GwsError::Validation would be safer if the invariant ever changes.

Suggested change

	let method_name = parts
	.last()
	.expect("split() on a &str always yields at least one element");
	let method_name = parts
	.last()
	.ok_or_else(|| GwsError::Validation("Invalid tool name format".to_string()))?;