-
Notifications
You must be signed in to change notification settings - Fork 14
feat(dsl): implement workflow tool mode handling and MCP server node #212
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
base: eng-96/tool-registry
Are you sure you want to change the base?
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7074d5305c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| async execute() { | ||
| // The Docker runner handles execution. | ||
| // At runtime, the workflow engine will resolve the dynamic endpoint. | ||
| return { | ||
| endpoint: 'http://localhost:8080', // Placeholder |
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.
Invoke the docker runner for MCP server node
In the workflow runner, only component.execute is called; Docker execution is not triggered automatically (see run-component.activity.ts, which expects components to call runComponentWithRunner themselves). This execute implementation just returns a placeholder endpoint and never starts the container, so workflows that include this node will not actually launch an MCP server and will point to http://localhost:8080 even if nothing is running. Unless an external server is already up, the node will silently succeed but downstream tool calls will fail.
Useful? React with 👍 / 👎.
| let encryptedCredentials: string | undefined; | ||
| if (authToken) { | ||
| const encryptionMaterial = await this.encryption.encrypt(authToken); | ||
| encryptedCredentials = JSON.stringify(encryptionMaterial); |
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.
Store auth token as JSON or handle raw string
registerRemoteMcp encrypts a raw authToken string, but getToolCredentials later does JSON.parse on the decrypted payload. For non-JSON tokens, this throws and credentials are dropped, so remote MCP auth tokens become unretrievable. Either JSON.stringify the token before encryption or make getToolCredentials accept plain strings for remote-mcp tools.
Useful? React with 👍 / 👎.
f79081a to
190a3d9
Compare
Signed-off-by: betterclever <paliwal.pranjal83@gmail.com>
- Add port mapping support to DockerRunner - Refactor mcp-server component to use dynamic runner config and exposed ports - Fix trace event mapping in E2E tests - Add container cleanup to mcp-tool-mode E2E test - Ensure local MCP registration uses actual containerId and endpoint - Fix workflow definition validation by adding node positions in test Signed-off-by: betterclever <paliwal.pranjal83@gmail.com>
190a3d9 to
ca18b5e
Compare
Summary
WorkflowNodeMetadatato supportmode('normal' | 'tool') andtoolConfig.core.mcp.servercomponent (Docker-based) to allow workflows to start and manage external tool providers.Stacked on: #208 (ENG-96)