Skip to content

chore: add ping utility, modify default command in dockerfile, update defaults for http_host #56

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

Merged
merged 4 commits into from
Jun 26, 2025

Conversation

eddyv
Copy link
Member

@eddyv eddyv commented Jun 26, 2025

This pull request introduces several changes to improve the HTTP and SSE transport implementations, align the ping endpoint with JSON-RPC 2.0 standards, and enhance configuration flexibility. The most significant updates include modifying the ping endpoint schemas and handlers, updating the default HTTP host configuration, and dynamically setting server URLs based on environment variables.

Transport Updates

  • Ping Endpoint Schema and Handler Updates: Refactored the pingHandler to align with JSON-RPC 2.0 standards by introducing pingRequestSchema and updating pingResponseSchema. Removed the GET /ping endpoint and standardized the POST /ping endpoint for both HTTP and SSE transports. [1] [2] [3]

Configuration Enhancements

  • Dynamic HTTP Host Configuration: Changed the default HTTP_HOST from "localhost" to "0.0.0.0" to allow binding to all interfaces. Updated the Swagger server URL to dynamically use HTTP_HOST and HTTP_PORT from environment variables. [1] [2]

Miscellaneous Improvements

  • Dockerfile CMD Addition: Added a default CMD to specify --transport http for production environments.

@Copilot Copilot AI review requested due to automatic review settings June 26, 2025 16:54
@eddyv eddyv requested a review from a team as a code owner June 26, 2025 16:54
@eddyv eddyv enabled auto-merge (squash) June 26, 2025 16:55
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the ping endpoints to align with JSON-RPC 2.0, updates configuration defaults for HTTP hosting, and enhances container startup behavior.

  • Standardize /ping as a POST-only JSON-RPC 2.0 endpoint for both HTTP and SSE transports
  • Change default HTTP_HOST to 0.0.0.0 and make Swagger server URL dynamic
  • Add a default Docker CMD for --transport http and bump dependency versions

Reviewed Changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/mcp/transports/sse.ts Remove GET /ping, update POST /ping to JSON-RPC 2.0 schema
src/mcp/transports/http.ts Same changes for HTTP transport
src/mcp/transports/ping.ts Add request schema, update response schema, simplify handler signature
src/mcp/transports/server.ts Import env, update Swagger server URL to use env.HTTP_HOST & HTTP_PORT
src/env-schema.ts Change default HTTP_HOST to 0.0.0.0
README.md Update documented default for HTTP_HOST
Dockerfile Add default CMD ["--transport", "http"]
package.json Bump several dev and prod dependency versions
Comments suppressed due to low confidence (5)

src/mcp/transports/ping.ts:7

  • JSON-RPC 2.0 allows the id to be a string, number, or null. Consider updating the schema to type: ["string", "number"] (or including null) to ensure full compliance.
    id: { type: "string" },

src/mcp/transports/ping.ts:16

  • Per JSON-RPC 2.0 spec, the jsonrpc property must equal "2.0". Add an enum: ["2.0"] constraint to enforce this constant value.
    jsonrpc: { type: "string" },

src/mcp/transports/sse.ts:138

  • Removing the GET /ping endpoint is a breaking change for any existing health checks. Update documentation or provide a deprecation path so clients aren’t unexpectedly impacted.
          if (!reply.sent) {

src/mcp/transports/ping.ts:23

  • Consider adding unit tests for the new JSON-RPC 2.0 pingHandler to verify correct handling of id propagation and response schema under different inputs.
export function pingHandler() {

Copy link
Contributor

@stephenheg stephenheg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eddyv eddyv merged commit 185fc7f into main Jun 26, 2025
2 checks passed
@eddyv eddyv deleted the chore/docker-updates branch June 26, 2025 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants