Skip to content

Conversation

@jayhack
Copy link
Contributor

@jayhack jayhack commented Aug 9, 2025

Adds the codegen mcp command 2.0. This is currently in beta.

Requires we upgrade the API backend

@codecov
Copy link

codecov bot commented Aug 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

@codegen-sh
Copy link
Contributor

codegen-sh bot commented Aug 9, 2025

High-level review (diff too large to load inline)

I attempted to load the PR diff via the API but it exceeds the service limit (>20k lines), so I can’t leave line-by-line comments. Below is a thorough, actionable review focused on the likely surface area of a major CLI feature (“codegen mcp” 2.0). If you can point me to the key files or split this into smaller PRs (transport, client/core, CLI UX, docs/tests), I’ll follow up with targeted inline comments.

Summary of requested focus areas

  • Please confirm whether this is a rewrite or an incremental refactor. If breaking, include a short migration guide and deprecation strategy.
  • Consider splitting into logical PRs: (1) protocol & transports; (2) CLI UX & subcommands; (3) state/connection lifecycle; (4) docs; (5) tests.
  • Provide an architecture/flow overview for MCP 2.0 (one page + diagrams): transports supported, handshake, capabilities negotiation, reconnection, cancellation, and telemetry.

Code quality & best practices

  • Types & validation:
    • Enforce strict TypeScript (noImplicitAny, strictNullChecks) for all new code; avoid any and unknown without narrowing.
    • Validate all external inputs (URIs, ports, JSON-RPC payloads) using a schema validator (zod/valibot). Reject early with helpful errors.
  • Error handling & UX:
    • Standardize errors via a typed error hierarchy (e.g., McpConnectionError, McpProtocolError, McpAuthError). Map to consistent exit codes.
    • Provide actionable CLI messages plus a --json flag for machine-readable output (don’t scrape stderr).
    • Ensure unhandled promise rejections are trapped; use top-level error boundary and process.on handlers with graceful shutdown.
  • Async & resource cleanup:
    • Adopt AbortController everywhere for timeouts/cancellation; propagate signals across layers.
    • Ensure transports and child processes are disposed in finally blocks; remove listeners to avoid leaks.
  • Logging & observability:
    • Centralize logging with levels (--log-level, --verbose) and stable JSON logs (--log-format=json) for CI.
    • Redact tokens/credentials. Add request IDs/correlation IDs for multi-connection scenarios.
  • CLI consistency:
    • Subcommands should follow verb-noun patterns and consistent flags: codegen mcp connect|start|list|probe.
    • Offer --timeout, --retries, --retry-backoff, and --concurrency flags with sensible defaults.
    • Ensure help text is descriptive, shows examples, and includes exit codes and error classes.
  • Cross-platform:
    • Avoid shelling out via sh -c with untrusted input; prefer spawn without a shell and pass args as an array.
    • Normalize paths (Windows) and consider named pipes vs stdio/TCP differences. Use cross-spawn where needed.

Architecture & maintainability

  • Clear separations:
    • CLI layer (arg parsing, presentation) vs core MCP client (protocol, state machine) vs transport adapters (stdio, TCP, WebSocket).
    • Introduce an IMcpTransport interface (send, onMessage, close, onClose, onError). Keep JSON-RPC framing independent from transport.
  • Protocol state machine:
    • Explicit states (Init, Handshake, Ready, Draining, Closed, Reconnecting). Model transitions and timers.
    • Capabilities negotiation: validate server capabilities against client requirements with precise error messages.
    • Heartbeat/ping/pong and liveness detection with jittered timeouts.
  • Scalability & performance:
    • Avoid buffering full streams; parse messages incrementally. Guard against unbounded queues; apply backpressure or drop policy.
    • Batch or coalesce outbound messages when appropriate. Reuse connections where safe.
    • Consider concurrency limits to protect the process; expose metrics if running long-lived sessions.
  • Extensibility:
    • Keep transports pluggable. Document how to add a new transport and test matrix.
    • Feature-gate experimental protocol features; surface via --experimental flags.

Security review

  • Input hardening:
    • Sanitize/validate all user-provided addresses, file paths, and command strings. Reject file:// paths that escape allowed roots.
    • Never pass user input into shells; if unavoidable, escape thoroughly and document risk.
  • Network & TLS:
    • Enforce TLS for remote connections, validate certificates, and allow pinned CAs. Provide --insecure only for local/dev with explicit warnings.
  • Secrets handling:
    • Don’t log secrets. Support environment-based creds and OS keychain where relevant. Mask in crash reports.
  • Sandbox & file I/O:
    • Respect XDG/OS-specific config/data dirs; avoid world-readable permissions on Linux/Unix. Use fs.mkdtemp for ephemeral dirs.
  • Protocol safety:
    • Length-limit frames; reject oversized or recursive payloads. Validate JSON-RPC id/type/method strictly.

Testing

  • Unit tests:
    • Protocol: handshake, capability negotiation, request/response, error propagation, cancellation, timeouts, backoff.
    • Transports: stdio/TCP/WS happy-path and error-path (partial frames, abrupt close, slowloris, invalid JSON, TLS error).
    • CLI parsing: flags, defaults, invalid combinations; snapshot tests for --help.
  • Integration tests:
    • Spin up a dummy MCP server to verify end-to-end flows; include reconnection and multi-server scenarios.
    • Cross-platform CI: Linux + Windows at minimum.
  • Negative/edge cases:
    • Large payloads, slow servers, duplicate IDs, unsolicited notifications, backpressure behavior, SIGINT/SIGTERM handling.
  • Coverage:
    • Aim for high coverage on core client and transports; ensure new modules are included in coverage thresholds to prevent regressions.

Documentation

  • Update README/CLI docs with:
    • What is MCP 2.0, supported transports, and minimum server requirements.
    • Usage examples and recommended timeouts for typical setups.
    • Exit codes, error classes, and troubleshooting guide.
    • Security considerations, especially around remote connections and credentials.
  • Migration notes:
    • Breaking changes list, deprecated flags, and their replacements. Provide compatibility matrix (client x server).
  • Developer docs:
    • Architecture diagram, module boundaries, and how to add a new transport.

Requested follow-ups for this PR

  1. Provide a brief architecture doc (can be in docs/mcp/ or as PR description) and identify the 5–10 key files for focused review.
  2. Confirm TypeScript strictness is enabled and lint rules applied across all new files. If not, please add.
  3. Add/confirm tests for: handshake timeout, reconnection after server restart, invalid JSON frame, and cancellation via SIGINT.
  4. Add/confirm a --json output mode and redaction of sensitive values in logs.
  5. Verify that no user input is passed into shell commands; if used, replace with spawn w/ args.
  6. If this PR is hard to split, at least isolate commits by subsystem (transport, client, CLI, docs, tests) to ease review.

Happy to drill down further—please point me to specific files or reduce the diff scope and I’ll leave precise inline comments next.

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