Skip to content

feat: enable Streamable HTTP MCP servers & lazy loading of tools #2550

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

seratch
Copy link
Member

@seratch seratch commented Aug 21, 2025

This pull request adds the following features:

  1. Add Streamable HTTP MCP server support
  2. Lazy loading of MCP server tools without blocking the shell initialization (resolves Optional / Lazy Loading of MCP Servers #2335)

If we want only 2. (lazy loading), I can send a separate pull request for it.

@seratch seratch added the enhancement New feature or request label Aug 21, 2025
@seratch seratch force-pushed the streamable-http=mcp-lazy-loading branch from 4e8252e to 12f023d Compare August 21, 2025 07:27
@seratch seratch added the codex-rust-review Perform a detailed review of Rust changes. label Aug 21, 2025
@github-actions github-actions bot added codex-rust-review-in-progress and removed codex-rust-review Perform a detailed review of Rust changes. labels Aug 21, 2025
Copy link

Summary
Adds Streamable HTTP MCP server support and lazily loads MCP tools to avoid blocking shell startup. Updates config schema/docs, core wiring, mcp-client transport, and TUI status rendering.

Notes

  • PR body is clear; consider briefly stating the motivation (startup latency + remote MCP support) and a short before/after UX note, plus a screenshot of the /mcp view.

Review
Thoughtful design and clean integration; a few nits and polish items to consider:

  • codex-rs/mcp-client/src/mcp_client.rs
    • Prefer anyhow’s formatting over pre-formatting: replace anyhow!(format!(...)) with anyhow!(\"... {} ...\", x, y).
    • In send_request_http, content-type checks look good; consider handling charset (e.g., starts_with(\"application/json\") is fine) and returning the raw status on non-JSON/SSE responses for better diagnostics.
    • Minor fmt nit: format!(\"{} {}\", command, args.join(\" \")) can be format!(\"{command} {}\", args.join(\" \")).
  • codex-rs/core/src/mcp_connection_manager.rs
    • Nice lazy init with OnceCell; good timeouts. Consider bubbling per-server initialize errors into the errors map (similar to spawn failures) when refresh_tools() is called, so a /mcp refresh can surface them explicitly.
    • qualify_tools: truncation + SHA1 suffix is solid. Since we allow only ASCII in names, slicing is safe; add a short comment noting the ASCII precondition (already implied by validation).
  • codex-rs/core/src/codex.rs
    • Great that tool loading is kicked off post SessionConfigured. Consider emitting a brief “loading MCP tools…” event when any MCP servers are configured, so UIs can indicate initial loading state deterministically.
  • codex-rs/core/src/config_types.rs
    • Streamable HTTP variant looks good. Consider documenting default header expectations (e.g., auth header) next to the type, matching the docs.
  • codex-rs/tui/src/history_cell.rs
    • Rendering looks good; for style consistency, prefer ratatui’s Stylize helpers over Span::styled(…, Style::default()…) where possible (see styles.md).
    • New /mcp output paths are user-facing; please update/accept snapshot tests accordingly.
  • codex-rs/mcp-client/Cargo.toml
    • Keep [dependencies] alphabetized per repo conventions.

Testing and snapshots

  • Please run and accept TUI snapshots if expected: cargo test -p codex-tui, cargo insta pending-snapshots -p codex-tui, then cargo insta accept -p codex-tui if all good.
  • Rust fmt/lints: from codex-rs/, run just fmt and just fix -p codex-mcp-client (and codex-core if needed).

View workflow run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optional / Lazy Loading of MCP Servers
1 participant