-
Notifications
You must be signed in to change notification settings - Fork 166
Adding MCP call (via HTTP) #1116
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
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.
Thank you! I have a few questions if you don't mind.
@amin-nikanjam Could you please sign-off your commits? |
Done! |
@amin-nikanjam DCO is still not passing. Please follow the steps enumerated in the following link to ensure it checks: https://github.com/serverlessworkflow/specification/pull/1116/checks?check_run_id=49885563716 |
@amin-nikanjam Why are we not supporting stdio transport, too? I feel it is but trivial to add support for it, isn't it? |
Right, we are not supporting the stdio now. While in MCP spec it is clarified that "Clients SHOULD support stdio whenever possible", we can add it. |
@amin-nikanjam apart from adding |
b57bac1
to
fbade4a
Compare
@amin-nikanjam can you please squash and sign your commit? |
sure, I will wrap it up in a few days. |
@ricardozanini @cdavernas So, I checked the MCP spec and several implementations, in the spec it's clarified that for STDIO, "the client launches the MCP server as a subprocess", so the client has to have the "server_path" to run it. Moreover, after initialization, the client should send "JSON-RPC messages to stdin and receive messages from the server using stdout". What is the best way to include it in the specification? like a "Shell Process"? |
@amin-nikanjam I think there’s some confusion here. The client launching the server as a stdio subprocess is simply an implementation detail—it doesn’t require a dedicated task in the spec. The MCP call itself must support both transport modes; it’s up to implementers how they handle the subprocess. In practice, supporting both is straightforward, and there’s no need to over-specify things at the level of “shell process” in the spec. |
Got it. I made the necessary changes and pushed the commit. |
@amin-nikanjam I think you rebased your fork with the latest commits from main. You can either rebase it again and work through your local git logs to pick the correct commits or open a new PR with a fresh branch. |
Please specify parts of this PR update:
Discussion or Issue link:
What this PR does:
Additional information: