fix(cli): update mcp sdk - #196
Conversation
github-actions
Bot
added
scope(ci)
scope(cli)
dependencies
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe CLI migrates to ChangesMCP server migration
Sitemap filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant startMcpServer
participant serveStdio
participant ServerFactory
participant ServerContext
participant MCPRequest
MCPClient->>startMcpServer: start MCP server
startMcpServer->>serveStdio: provide server factory and onerror
serveStdio->>ServerFactory: create MCP server
MCPClient->>ServerContext: invoke registered tool
ServerContext->>MCPRequest: read progressToken
ServerContext->>MCPRequest: notify progress
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
projects/site/src/_11ty/plugins/sitemap-xml.jsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. projects/site/src/_11ty/plugins/sitemap-xml.test.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/cli/src/mcp/index.ts (1)
41-54: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilently swallowed
notify()failures make progress-reporting issues undebuggable.
.catch(() => undefined)discards any error fromctx.mcpReq.notify(...)with no trace. If the transport write fails, tool authors/operators get zero signal.🔧 Proposed fix to log discarded notify errors
ctx.mcpReq .notify({ method: 'notifications/progress', params: { progressToken, progress: progressCount, message } }) - .catch(() => undefined); + .catch(error => console.error('Failed to send progress notification:', error));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/cli/src/mcp/index.ts` around lines 41 - 54, Update attachProgress so failures from ctx.mcpReq.notify are logged instead of silently discarded. Preserve the existing progress notification behavior and catch handling, but report the rejected error with sufficient context for diagnosing failed progress updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@projects/cli/src/mcp/index.ts`:
- Around line 41-54: Update attachProgress so failures from ctx.mcpReq.notify
are logged instead of silently discarded. Preserve the existing progress
notification behavior and catch handling, but report the rejected error with
sufficient context for diagnosing failed progress updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7cfd584b-d9fe-4c1d-88fd-bdb45fa3fd2a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
NOTICE.mdprojects/cli/DEVELOPMENT.mdprojects/cli/NOTICE.mdprojects/cli/README.mdprojects/cli/package.jsonprojects/cli/src/mcp/index.test.tsprojects/cli/src/mcp/index.ts
- Updated the package dependency in package.json and pnpm-lock.yaml. - Modified the NOTICE.md and README.md files to reflect the new server package. - Adjusted the MCP server implementation in index.ts to utilize the new server API. - Updated tests to ensure compatibility with the new server structure. Signed-off-by: Cory Rylan <crylan@nvidia.com>
4cfc643 to
7560ebe
Compare
b65df51 to
ea864cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/site/src/_11ty/plugins/sitemap-xml.test.ts`:
- Line 42: Update the sitemap test around renderSitemap() to include a
/DESIGN.md fixture in its input before asserting that the generated sitemap
excludes the corresponding URL. Keep the existing non-containment assertion,
ensuring it now verifies the implementation filters an actually provided
DESIGN.md entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c14e6064-57ee-4fad-8ad1-dc616ddcdf1a
📒 Files selected for processing (2)
projects/site/src/_11ty/plugins/sitemap-xml.jsprojects/site/src/_11ty/plugins/sitemap-xml.test.ts
| ); | ||
| expect(sitemap).toContain('<loc>https://nvidia.github.io/elements/docs/about/support/</loc>'); | ||
| expect(sitemap).toContain('<loc>https://nvidia.github.io/elements/DESIGN.md</loc>'); | ||
| expect(sitemap).not.toContain('<loc>https://nvidia.github.io/elements/DESIGN.md</loc>'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the DESIGN.md exclusion assertion meaningful.
renderSitemap() never receives /DESIGN.md in this test, so this assertion passes regardless of whether the implementation includes that URL. Add a /DESIGN.md fixture, or move this into a dedicated test.
Proposed test fix
{
content: '<code>{"dateModified":"2026-07-25T00:00:00.000Z"}</code>',
url: '/docs/about/support/'
+ },
+ {
+ content: '',
+ url: '/DESIGN.md'
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(sitemap).not.toContain('<loc>https://nvidia.github.io/elements/DESIGN.md</loc>'); | |
| { | |
| content: '<code>{"dateModified":"2026-07-25T00:00:00.000Z"}</code>', | |
| url: '/docs/about/support/' | |
| }, | |
| { | |
| content: '', | |
| url: '/DESIGN.md' | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/site/src/_11ty/plugins/sitemap-xml.test.ts` at line 42, Update the
sitemap test around renderSitemap() to include a /DESIGN.md fixture in its input
before asserting that the generated sitemap excludes the corresponding URL. Keep
the existing non-containment assertion, ensuring it now verifies the
implementation filters an actually provided DESIGN.md entry.
ea864cc to
7102625
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/site/src/docs/mcp/index.md`:
- Line 84: Change the code fence immediately preceding the Codex configuration
snippet in the MCP documentation from shell syntax highlighting to toml, leaving
the TOML content unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0a6d3053-ea9f-4caf-b299-9afaa1464421
📒 Files selected for processing (3)
projects/site/src/_11ty/plugins/sitemap-xml.jsprojects/site/src/_11ty/plugins/sitemap-xml.test.tsprojects/site/src/docs/mcp/index.md
Signed-off-by: Cory Rylan <crylan@nvidia.com>
7102625 to
f1ba540
Compare
|
🎉 This issue has been resolved in version 2.1.8 🎉 |
Summary by CodeRabbit
DESIGN.md.