-
Notifications
You must be signed in to change notification settings - Fork 300
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mcp #478
base: main
Are you sure you want to change the base?
Mcp #478
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a first pass review!
package.json
Outdated
@@ -55,7 +55,8 @@ | |||
}, | |||
"dependencies": { | |||
"@ironclad/rivet-core": "workspace:^", | |||
"@ironclad/rivet-node": "workspace:^" | |||
"@ironclad/rivet-node": "workspace:^", | |||
"@modelcontextprotocol/sdk": "^1.5.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add this to the individual packages, not the root package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a better node body? Maybe it's just the example, but a blank body doesn't seem very helpful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
packages/app/vite.config.ts
Outdated
alias: { | ||
'@ironclad/rivet-core': resolve('../core/src/index.ts'), | ||
'@ironclad/trivet': resolve('../trivet/src/index.ts'), | ||
// 'node:child_process': resolve('./src/mocks/node-polyfills.ts'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we don't need these any more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes this is removed
@@ -0,0 +1,116 @@ | |||
// Mock implementation for node:child_process | |||
export const spawn = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we don't need this file any more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it came back
packages/app/vite.config.ts
Outdated
@@ -34,6 +52,10 @@ export default defineConfig({ | |||
plugins: [visualizer()], | |||
}, | |||
}, | |||
// define: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
label: 'Configuration', | ||
dataKey: 'config', | ||
useInputToggleDataKey: 'useEndpointInput', | ||
helperMessage: 'Configuration for the MCP server' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please run prettier on this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prettier is done
@@ -0,0 +1,235 @@ | |||
# MCP Node for Rivet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move these docs into the docs project? And structure it like the existing per-node documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved
ignores = [], | ||
} = options; | ||
|
||
const resolvedPath = await resolveBaseDir(baseDir, path); | ||
console.log('Reading directory from path:', resolvedPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to remove this and other console logs in the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed console.log
): Promise<{ output: string; metadata: Record<string, unknown> }> { | ||
let configFile: object; | ||
try { | ||
configFile = this.data.useEndpointInput |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be behind a different data flag, not useEndpointInput
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes changed...
throw new MCPError(MCPErrorType.INVALID_RESPONSE, 'Invalid format for MCP configuration'); | ||
} | ||
|
||
const serverId = this.data.useEndpointInput |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use getInputOrData
for this pattern, getInputOrData(this.data, inputs, 'serverId')
This looks great! Quick question: Will this work in node integration using runGraphInFile? |
@@ -0,0 +1,116 @@ | |||
// Mock implementation for node:child_process | |||
export const spawn = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it came back
alias: { | ||
'@ironclad/rivet-core': resolve('../core/src/index.ts'), | ||
'@ironclad/trivet': resolve('../trivet/src/index.ts'), | ||
'node:child_process': resolve('./src/mocks/node-polyfills.ts'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why were these added back in?
@@ -48,6 +48,8 @@ | |||
"@google-cloud/vertexai": "^0.1.3", | |||
"@google/generative-ai": "^0.21.0", | |||
"@huggingface/inference": "^2.6.4", | |||
"@modelcontextprotocol/sdk": "^1.5.0", | |||
"@tauri-apps/api": "^1.5.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Core cannot depend on tauri
Add Model Context Protocol (MCP) Integration
Description
This PR adds support for the Model Context Protocol (MCP), enabling seamless integration with external AI capabilities, tools, and resources through a standardized protocol. The implementation includes both HTTP and STDIO communication modes, making it flexible for different deployment scenarios.
Key Features
MCPNode
for handling MCP communicationsImplementation Details
MCPNode
class with full TypeScript type safetymcp-config.json
Technical Considerations
Testing
The implementation includes:
Documentation
Breaking Changes
None. This is a purely additive change that maintains compatibility with existing functionality.
Future Considerations