fix: tool-version-check falls back to package.json for MCP servers that block on --version#3973
Conversation
…at block on --version MCP servers like macos-automator-mcp and claude-code-mcp start a blocking server process when invoked with --version instead of printing a version string. This caused timeout errors, 'could not check latest' status, and server startup warnings leaking to the console. Add get_npm_pkg_version() fallback that reads the version from the npm global package.json when --version times out or produces unparseable output. Suppress stderr on --version calls to prevent MCP server startup logs from leaking.
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 resolves an issue where certain MCP servers, such as macos-automator-mcp and claude-code-mcp, would incorrectly initiate a blocking server process when queried for their version using the --version flag. This behavior led to timeouts, 'unknown' version statuses, and unwanted server startup logs appearing in the console. The solution introduces a robust fallback mechanism that, upon detecting a timeout or unparseable output from the --version command, attempts to retrieve the tool's version directly from its global npm package.json file. Additionally, stderr output during version checks is now suppressed, ensuring a cleaner and more accurate version reporting experience for these tools. 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
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds an npm global package.json fallback to the tool-version-check script: if a tool's --version command times out or yields no parseable version, the script can read the version from npm's global package metadata when a package name is provided. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 a fallback mechanism to retrieve tool versions from package.json for MCP servers that hang when the --version flag is used. This is a solid improvement to handle problematic tools gracefully. The implementation is logical and correctly addresses the issue described. I've included a few suggestions to enhance the script's robustness and maintainability by using jq for JSON parsing, optimizing file reads, and simplifying conditional logic.
| grep -oE '"version"\s*:\s*"[0-9]+\.[0-9]+\.[0-9]+"' "$pkg_json" | | ||
| grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1 |
There was a problem hiding this comment.
Using a chain of grep commands to parse a JSON file can be brittle. Since jq is a more robust and idiomatic tool for handling JSON, it should be preferred. This change will make the script more resilient to potential formatting variations in package.json files.
| grep -oE '"version"\s*:\s*"[0-9]+\.[0-9]+\.[0-9]+"' "$pkg_json" | | |
| grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1 | |
| jq -r '.version // empty' "$pkg_json" | head -1 |
There was a problem hiding this comment.
Declining this suggestion. While jq is more robust for arbitrary JSON, this script runs on diverse systems where jq may not be installed. The grep chain is sufficient for npm's well-structured package.json format — the "version" field is always a simple string. Adding a jq dependency for a single extraction is over-engineering for this use case.
| version=$(head -1 "$_ver_log" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1 || echo "") | ||
| if [[ -z "$version" ]]; then | ||
| version=$(head -1 "$_ver_log" | grep -oE '[0-9]+\.[0-9]+' | head -1 || echo "unknown") | ||
| version=$(head -1 "$_ver_log" | grep -oE '[0-9]+\.[0-9]+' | head -1 || echo "") | ||
| fi |
There was a problem hiding this comment.
This block reads and parses the log file $_ver_log twice. To improve performance and readability, you can read the file's content into a variable once and then perform the grep operations on that variable.
| version=$(head -1 "$_ver_log" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1 || echo "") | |
| if [[ -z "$version" ]]; then | |
| version=$(head -1 "$_ver_log" | grep -oE '[0-9]+\.[0-9]+' | head -1 || echo "unknown") | |
| version=$(head -1 "$_ver_log" | grep -oE '[0-9]+\.[0-9]+' | head -1 || echo "") | |
| fi | |
| local ver_output | |
| ver_output=$(head -1 "$_ver_log") | |
| version=$(echo "$ver_output" | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1) | |
| if [[ -z "$version" ]]; then | |
| version=$(echo "$ver_output" | grep -oE '[0-9]+\.[0-9]+' | head -1) | |
| fi |
There was a problem hiding this comment.
Applied in a8a3921. Reads the log file once into ver_output, then greps from the variable. Also moved rm -f before the grep operations so the temp file is cleaned up earlier.
| echo "$version" | ||
|
|
||
| # If --version produced no parseable version, try npm package.json fallback | ||
| if [[ -z "$version" || "$version" == "unknown" ]] && [[ -n "$pkg" ]]; then |
There was a problem hiding this comment.
The check "$version" == "unknown" is redundant here. Based on the logic in the preceding lines (166-169), the $version variable will either contain a version string or be empty; it will not be the string "unknown". You can simplify this condition for better clarity.
| if [[ -z "$version" || "$version" == "unknown" ]] && [[ -n "$pkg" ]]; then | |
| if [[ -z "$version" ]] && [[ -n "$pkg" ]]; then |
There was a problem hiding this comment.
Applied in a8a3921. Correct analysis — $version can only be empty or a version string at this point, never the literal "unknown". Simplified to [[ -z "$version" ]]. CodeRabbit's nitpick about adding an empty npm_root guard was also applied in the same commit.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.agents/scripts/tool-version-check.sh (2)
171-182: Minor cleanup opportunity in condition.The condition
"$version" == "unknown"on line 173 will never match because$versionis populated from grep output (which would be empty, not "unknown"). The check is harmless but could be simplified:♻️ Optional simplification
- if [[ -z "$version" || "$version" == "unknown" ]] && [[ -n "$pkg" ]]; then + if [[ -z "$version" ]] && [[ -n "$pkg" ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/tool-version-check.sh around lines 171 - 182, The conditional that checks for an "unknown" literal is redundant because version comes from grep output and will be empty when unparseable; update the fallback condition in the tool-version-check logic to only test for an empty/undefined version (e.g., use [[ -z "$version" ]] && [[ -n "$pkg" ]]) so the pkg fallback via get_npm_pkg_version is reached correctly—look for the block referencing the variables version and pkg and the call to get_npm_pkg_version to make this change.
113-126: Well-designed fallback function for npm package version extraction.The implementation correctly handles scoped packages (e.g.,
@steipete/macos-automator-mcp) and has appropriate error handling with early returns. The-fcheck prevents file-not-found errors gracefully.Minor observation: If
npm root -gsucceeds but returns an empty string (rare edge case), the path would become//pkg/package.json. The-fcheck catches this safely, but an explicit empty-check could make intent clearer:🛡️ Optional defensive enhancement
get_npm_pkg_version() { local pkg="$1" local npm_root npm_root="$(npm root -g 2>/dev/null)" || return 1 + [[ -n "$npm_root" ]] || return 1 local pkg_json="${npm_root}/${pkg}/package.json"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/scripts/tool-version-check.sh around lines 113 - 126, The get_npm_pkg_version function should explicitly guard against npm_root being empty: after running npm_root="$(npm root -g ...)" check that npm_root is non-empty before constructing pkg_json to avoid producing paths like //pkg/package.json; update the function (referencing get_npm_pkg_version, the npm_root variable and pkg_json) to return failure if npm_root is empty (or trim/normalize it) before proceeding to the -f check and grep logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.agents/scripts/tool-version-check.sh:
- Around line 171-182: The conditional that checks for an "unknown" literal is
redundant because version comes from grep output and will be empty when
unparseable; update the fallback condition in the tool-version-check logic to
only test for an empty/undefined version (e.g., use [[ -z "$version" ]] && [[ -n
"$pkg" ]]) so the pkg fallback via get_npm_pkg_version is reached correctly—look
for the block referencing the variables version and pkg and the call to
get_npm_pkg_version to make this change.
- Around line 113-126: The get_npm_pkg_version function should explicitly guard
against npm_root being empty: after running npm_root="$(npm root -g ...)" check
that npm_root is non-empty before constructing pkg_json to avoid producing paths
like //pkg/package.json; update the function (referencing get_npm_pkg_version,
the npm_root variable and pkg_json) to return failure if npm_root is empty (or
trim/normalize it) before proceeding to the -f check and grep logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f1ad7953-abdf-497d-a54b-3bdce93c3c92
📒 Files selected for processing (1)
.agents/scripts/tool-version-check.sh
OpenCode is installed via Homebrew (anomalyco/tap/opencode) but was listed under NPM_TOOLS, causing false 'outdated' warnings when the brew tap lagged behind npm releases. Move to BREW_TOOLS so the latest version is checked against the brew tap instead.
|
This PR is from an external contributor (@johnwaldo). Auto-merge is disabled for external PRs — a maintainer must review and approve manually. To approve or decline, comment on this PR:
|
marcusquinn
added
external-contributor
|
Dispatching worker to address unresolved review suggestions before merge.
|
…root, remove dead code
Review suggestions addressed (a8a3921)Applied (3 of 4):
Declined (1 of 4):
All changes verified: ShellCheck clean, script tested and producing correct output for MCP servers. |
2 participants
Summary
macos-automator-mcp,claude-code-mcp) start a blocking server process when invoked with--versioninstead of printing a version string, causing timeout errors, "could not check latest" status, and server startup warnings leaking to the consoleget_npm_pkg_version()fallback that reads the version from the npm globalpackage.jsonwhen--versiontimes out or produces unparseable output--versioncalls to prevent MCP server startup logs from leaking to console outputBefore
After
Summary by CodeRabbit