Implement MCP ping/notifications and add startup logging #5687
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Add ping method support to MCPServer class - Add notifications/* method handling (returns null for no response) - Update MCPHTTPTransport to handle null responses (204 No Content) - Add comprehensive logging to MCP server creation and tool registration - Add tests for ping and notifications methods - All tests pass (15 HTTP transport tests, 12 MCP server tests) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Tested ping method returns empty result object - Tested notifications/initialized returns 204 No Content - Verified detailed logging shows all startup steps - Tested both stateful and stateless modes - All functionality working correctly Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Notifications identified by absence of 'id' field (JSON-RPC 2.0 spec) - Also handle MCP convention: methods starting with "notifications/" - Both conditions now properly return null (204 No Content via HTTP) - Updated tests to verify both notification types - All tests pass Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Use 'in' operator to check for id field presence (not undefined check) - id: null is a valid request that expects a response per JSON-RPC 2.0 - Only requests without id field (undefined) are notifications - Added tests for id: null as valid request - All tests pass (71 total, 68 passed, 3 pre-existing timeouts) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Only check for absence of id field per JSON-RPC 2.0 spec - MCP notifications starting with "notifications/" also have no id field - Simplifies logic and makes it more maintainable - All tests still pass (71 total, 68 passed) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Clarified comment about notification detection - Recompiled all workflows to use updated mcp_server.cjs logic - All tests pass Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Add logging to diagnose server startup issues
Implement MCP ping/notifications and add startup logging
Dec 6, 2025
Contributor
|
@copilot remove step that installs @modelcontextprotocol/sdk... |
The SDK dependency was replaced with a standalone HTTP transport implementation in mcp_http_transport.cjs. The installation step is no longer needed and has been removed from the MCP server setup. - Removed npm install command for @modelcontextprotocol/sdk - Removed error handling and success messages for SDK installation - Recompiled all workflows to remove the installation step Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
pelikhan
reviewed
Dec 6, 2025
pkg/workflow/mcp_servers.go
Outdated
| yaml.WriteString(fmt.Sprintf(" echo \"Successfully installed @modelcontextprotocol/sdk@%s\"\n", constants.DefaultMCPSDKVersion)) | ||
| yaml.WriteString(" \n") | ||
|
|
||
| yaml.WriteString(" # Verify required files exist\n") |
Contributor
There was a problem hiding this comment.
@copilot move shell code to file and use helper to write to yaml
Contributor
Author
There was a problem hiding this comment.
Refactored in commit dc2742c. Created pkg/workflow/sh/start_safe_inputs_server.sh with the shell script and used WriteShellScriptToYAML helper to write it to YAML. This reduced 65+ lines of repetitive yaml.WriteString calls to a single helper function call.
Extracted shell script code from mcp_servers.go into a dedicated file and used WriteShellScriptToYAML helper for cleaner code generation. Changes: - Created pkg/workflow/sh/start_safe_inputs_server.sh with server startup logic - Embedded the shell script in sh.go - Replaced 65+ lines of yaml.WriteString calls with WriteShellScriptToYAML helper - Recompiled all workflows with the refactored code This improves maintainability by separating shell script logic from Go code and using the existing helper pattern for script embedding. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MCP Server Implementation - Complete ✅
Successfully implemented ping and notifications support for the MCP server with proper JSON-RPC 2.0 compliance.
Changes Made
pingmethod - Returns empty result object as per MCP specidfield are notifications (no response)id: nullare valid requests that expect a responseTesting Results
Example Log Output
References
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.