feat(mcp): migrate MCP client from nanobot to official modelcontextprotocol/go-sdk #5

Merged

rp merged 1 commit from chore/mcp-go-sdk-migration into main 2026-08-25 09:05:14 +00:00

Conversation 5 Commits 1 Files changed 8 +419 -151

rp commented 2026-08-25 08:47:01 +00:00

Member

Copy link

Phase 1 of basepeak-ai/bpai#777: replace the nanobot-based MCP client with the official github.com/modelcontextprotocol/go-sdk v1.7.0.

What changed

• pkg/mcp/loader.go: sessions now connect via SDK transports — CommandTransport (stdio), StreamableClientTransport (HTTP, standalone SSE stream disabled to match previous request/response-only behavior), with an explicit fallback to the legacy SSEClientTransport for pre-streamable servers. Auth failures (401/403) surface as-is and never trigger the fallback, keeping the "unauthorized" marker bpai's reactive OAuth connect flow matches on. tools/list now follows pagination cursors.
• pkg/mcp/runner.go: tools/call via ClientSession.CallTool; the model-facing result JSON is pinned to the previous client's exact shape (isError always present, content omitted when empty, no _meta) via a dedicated wire struct + snapshot test.
• Headers from ServerConfig.Headers are injected through an http.Client RoundTripper — the path bpai uses for per-user bearer tokens. (${VAR} expansion was a no-op in the old client — it evaluated against a ClientOption env the fork never set — so headers keep passing verbatim.)
• pkg/mcp/client.go deleted: Local.Client / Capabilities had zero callers in this repo and in bpai.
• nanobot supervise re-exec removed (_exec branch in pkg/cli/main.go, NANOBOT_BIN in tests); stdio servers are spawned directly by the SDK with a PATH/HOME/USER env allowlist plus the configured env, matching the previous behavior.
• Latent bug fixes: ShutdownServer now hashes the config with AllowedTools cleared (matching LoadTools), so shutdowns of configs carrying allowedTools no longer silently no-op; Local.Close reports close errors, clears the session map, and cancels the session context first so shutdown fails fast on dead servers instead of hanging on the session DELETE.
• Dependencies: drops git.basepeak.dev/basepeak-ai/nanobot and transitive mcp-oauth-proxy; adds modelcontextprotocol/go-sdk v1.7.0.

Invariants preserved

• ServerConfig JSON wire shape and GetBaseURL() precedence (bpai's #!mcp blob contract) — untouched.
• Session cache key = sha256 of the JSON config; sessions outlive requests; session ID embedded in tool instructions — unchanged.
• Per-server tool-name namespacing and allowedTools raw-name filtering — unchanged, existing regression tests still pass.
• Server instructions → echo context tool — unchanged.

Testing

• pkg/mcp: 8/8 pass, including new tests for the wire-format snapshot, legacy-SSE fallback, 401-without-fallback (asserts no unauthenticated GET is attempted), header injection, and the ShutdownServer digest fix.
• Full go test ./...: green except integration/ and pkg/loader/github, which fail identically on unmodified main (missing gptscript-credential-helpers repo on this Forgejo instance / network-dependent GitHub fixture) — pre-existing, unrelated.
• golangci-lint on touched packages: no new findings vs main.

Follow-ups (not this PR)

• bpai phase 2: bump the gptscript pin, remove the _exec supervise branch + direct nanobot dep from bpai's main.go (only after this ships in the pinned version).
• Phase 3: e2e against DeepWiki + the previously-504ing streamable servers, canary on li-videos.

Phase 1 of https://git.basepeak.dev/basepeak-ai/bpai/issues/777: replace the nanobot-based MCP client with the official `github.com/modelcontextprotocol/go-sdk` v1.7.0. ## What changed - `pkg/mcp/loader.go`: sessions now connect via SDK transports — `CommandTransport` (stdio), `StreamableClientTransport` (HTTP, standalone SSE stream disabled to match previous request/response-only behavior), with an explicit fallback to the legacy `SSEClientTransport` for pre-streamable servers. Auth failures (401/403) surface as-is and never trigger the fallback, keeping the "unauthorized" marker bpai's reactive OAuth connect flow matches on. `tools/list` now follows pagination cursors. - `pkg/mcp/runner.go`: `tools/call` via `ClientSession.CallTool`; the model-facing result JSON is pinned to the previous client's exact shape (`isError` always present, `content` omitted when empty, no `_meta`) via a dedicated wire struct + snapshot test. - Headers from `ServerConfig.Headers` are injected through an `http.Client` RoundTripper — the path bpai uses for per-user bearer tokens. (`${VAR}` expansion was a no-op in the old client — it evaluated against a ClientOption env the fork never set — so headers keep passing verbatim.) - `pkg/mcp/client.go` deleted: `Local.Client` / `Capabilities` had zero callers in this repo and in bpai. - nanobot `supervise` re-exec removed (`_exec` branch in `pkg/cli/main.go`, `NANOBOT_BIN` in tests); stdio servers are spawned directly by the SDK with a PATH/HOME/USER env allowlist plus the configured env, matching the previous behavior. - Latent bug fixes: `ShutdownServer` now hashes the config with `AllowedTools` cleared (matching `LoadTools`), so shutdowns of configs carrying `allowedTools` no longer silently no-op; `Local.Close` reports close errors, clears the session map, and cancels the session context first so shutdown fails fast on dead servers instead of hanging on the session DELETE. - Dependencies: drops `git.basepeak.dev/basepeak-ai/nanobot` and transitive `mcp-oauth-proxy`; adds `modelcontextprotocol/go-sdk` v1.7.0. ## Invariants preserved - `ServerConfig` JSON wire shape and `GetBaseURL()` precedence (bpai's `#!mcp` blob contract) — untouched. - Session cache key = sha256 of the JSON config; sessions outlive requests; session ID embedded in tool instructions — unchanged. - Per-server tool-name namespacing and `allowedTools` raw-name filtering — unchanged, existing regression tests still pass. - Server instructions → echo context tool — unchanged. ## Testing - `pkg/mcp`: 8/8 pass, including new tests for the wire-format snapshot, legacy-SSE fallback, 401-without-fallback (asserts no unauthenticated GET is attempted), header injection, and the `ShutdownServer` digest fix. - Full `go test ./...`: green except `integration/` and `pkg/loader/github`, which fail identically on unmodified `main` (missing `gptscript-credential-helpers` repo on this Forgejo instance / network-dependent GitHub fixture) — pre-existing, unrelated. - `golangci-lint` on touched packages: no new findings vs `main`. ## Follow-ups (not this PR) - bpai phase 2: bump the gptscript pin, remove the `_exec` supervise branch + direct nanobot dep from bpai's `main.go` (only after this ships in the pinned version). - Phase 3: e2e against DeepWiki + the previously-504ing streamable servers, canary on li-videos.

rp added 1 commit 2026-08-25 08:47:01 +00:00

feat(mcp): migrate MCP client from nanobot to official modelcontextprotocol/go-sdk ... 52d9391880

Replaces the nanobot-based MCP client (git.basepeak.dev/basepeak-ai/nanobot,
now unmaintained upstream) with github.com/modelcontextprotocol/go-sdk v1.7.0.

- Streamable HTTP with explicit fallback to the legacy HTTP+SSE transport;
  auth failures (401/403) surface as-is and never trigger the fallback.
- Configured headers are injected via an http.Client RoundTripper (the path
  bpai uses for per-user bearer tokens).
- The model-facing tool-result JSON is pinned to the previous client's shape
  (isError always present, content omitted when empty, no _meta) and covered
  by a snapshot test.
- stdio servers now run via the SDK's CommandTransport; the nanobot supervise
  re-exec (_exec, NANOBOT_BIN) is removed.
- tools/list now follows pagination cursors.
- Fixes: ShutdownServer hashes the config with AllowedTools cleared (matching
  LoadTools) so shutdowns no longer silently no-op; Local.Close reports close
  errors, clears the session map, and cancels the session context first so
  shutdown doesn't hang on dead servers.
- Removes pkg/mcp/client.go (Local.Client/Capabilities had no callers).
- Drops the nanobot and mcp-oauth-proxy dependencies.

Part of phase 1 of basepeak-ai/bpai#777.

rp commented 2026-08-25 08:47:12 +00:00

Author

Member

Copy link

@sherpa review

@sherpa review

rp commented 2026-08-25 08:47:24 +00:00

Author

Member

Copy link

@sherpa review

@sherpa review

rp commented 2026-08-25 08:49:44 +00:00

Author

Member

Copy link

CI note: all four jobs (test, integration, validate-docs, check-label) fail with the same signature on the previous PR (06f6b2d, merged, current main) — inherited upstream plumbing (check-label queries api.github.com from Forgejo; integration needs the missing gptscript-credential-helpers repo; validate-docs is the docusaurus build). Local verification on this branch: pkg/mcp 8/8 (incl. wire-format snapshot, legacy-SSE fallback, 401-no-fallback, header injection, ShutdownServer digest regression test), full go test ./... green except integration/ and pkg/loader/github which fail identically on unmodified main; golangci-lint on touched packages shows no new findings vs main.

CI note: all four jobs (test, integration, validate-docs, check-label) fail with the same signature on the previous PR (06f6b2d, merged, current main) — inherited upstream plumbing (check-label queries api.github.com from Forgejo; integration needs the missing gptscript-credential-helpers repo; validate-docs is the docusaurus build). Local verification on this branch: pkg/mcp 8/8 (incl. wire-format snapshot, legacy-SSE fallback, 401-no-fallback, header injection, ShutdownServer digest regression test), full go test ./... green except integration/ and pkg/loader/github which fail identically on unmodified main; golangci-lint on touched packages shows no new findings vs main.

sherpa commented 2026-08-25 08:52:11 +00:00

Member

Copy link

Reviewed head commit: 52d93918803332d44734022feae5e7b064835c52

✅ Looks good to merge

This PR cleanly swaps the nanobot MCP client for modelcontextprotocol/go-sdk while preserving behavior that matters: the stdio env allowlist (PATH/HOME/USER) exactly matches nanobot's cleanOSEnv, the model-facing CallToolResult JSON shape is pinned with a dedicated regression test, and the ShutdownServer/Close session-lifecycle changes (clearing AllowedTools before hashing, canceling the session context before issuing per-session DELETEs) are correct against the SDK's actual streamableClientConn.Close semantics — I checked the go-sdk source to confirm both the DELETE-on-close and cancel-context-for-fail-fast claims hold. The new tests (legacy SSE fallback, auth-error-skips-fallback, header injection, shutdown cache-key regression) target exactly the tricky edges of this migration. Build and go test ./pkg/mcp/... pass; go vet is clean on the touched packages.

Minor nits (non-blocking):

• pkg/mcp/client.go (the exported Local.Client() / Client type / Capabilities() wrapper) was deleted with no replacement. It's unused anywhere in this repo, and removing it was structurally required once the nanobot types it wrapped went away, but if any sibling repo (e.g. bpai) called mcp.DefaultLoader.Client(...) directly, that would only surface as a downstream build break — worth a quick confirmation it's been migrated in lockstep.
• isAuthError's \b40[13]\b fallback could in theory false-match an unrelated "401"/"403" substring in a non-auth error message (e.g. a port number), causing the legacy SSE fallback to be skipped when it shouldn't be. Very low likelihood in practice given the primary unauthorized/forbidden string checks already cover the common case.

<!-- sherpa-review round=1 --> Reviewed head commit: `52d93918803332d44734022feae5e7b064835c52` ## ✅ Looks good to merge This PR cleanly swaps the nanobot MCP client for `modelcontextprotocol/go-sdk` while preserving behavior that matters: the stdio env allowlist (`PATH`/`HOME`/`USER`) exactly matches nanobot's `cleanOSEnv`, the model-facing `CallToolResult` JSON shape is pinned with a dedicated regression test, and the `ShutdownServer`/`Close` session-lifecycle changes (clearing `AllowedTools` before hashing, canceling the session context before issuing per-session `DELETE`s) are correct against the SDK's actual `streamableClientConn.Close` semantics — I checked the go-sdk source to confirm both the DELETE-on-close and cancel-context-for-fail-fast claims hold. The new tests (legacy SSE fallback, auth-error-skips-fallback, header injection, shutdown cache-key regression) target exactly the tricky edges of this migration. Build and `go test ./pkg/mcp/...` pass; `go vet` is clean on the touched packages. Minor nits (non-blocking): - `pkg/mcp/client.go` (the exported `Local.Client()` / `Client` type / `Capabilities()` wrapper) was deleted with no replacement. It's unused anywhere in this repo, and removing it was structurally required once the nanobot types it wrapped went away, but if any sibling repo (e.g. bpai) called `mcp.DefaultLoader.Client(...)` directly, that would only surface as a downstream build break — worth a quick confirmation it's been migrated in lockstep. - `isAuthError`'s `\b40[13]\b` fallback could in theory false-match an unrelated "401"/"403" substring in a non-auth error message (e.g. a port number), causing the legacy SSE fallback to be skipped when it shouldn't be. Very low likelihood in practice given the primary `unauthorized`/`forbidden` string checks already cover the common case.

rp commented 2026-08-25 09:05:13 +00:00

Author

Member

Copy link

Re the nits: (1) verified — bpai has zero Go imports of this repo's pkg/mcp (grep over *.go finds only a JSON-compat comment in bpai's own pkg/mcp/mcp.go); its contract is purely the #!mcp JSON blob, so deleting Local.Client/Capabilities can't break it. (2) agreed it's theoretical — a false match only skips the legacy-SSE fallback and surfaces the original streamable error, it never masks anything; the primary unauthorized/forbidden checks carry the real cases, so leaving the regex as-is. Merging per standing policy (sherpa approval + CI baseline documented above).

Re the nits: (1) verified — bpai has zero Go imports of this repo's pkg/mcp (grep over *.go finds only a JSON-compat comment in bpai's own pkg/mcp/mcp.go); its contract is purely the #!mcp JSON blob, so deleting Local.Client/Capabilities can't break it. (2) agreed it's theoretical — a false match only skips the legacy-SSE fallback and surfaces the original streamable error, it never masks anything; the primary unauthorized/forbidden checks carry the real cases, so leaving the regex as-is. Merging per standing policy (sherpa approval + CI baseline documented above).

rp merged commit 86194e00b0 into main 2026-08-25 09:05:14 +00:00

rp deleted branch chore/mcp-go-sdk-migration 2026-08-25 09:05:14 +00:00

rp referenced this pull request from a commit 2026-08-25 09:05:15 +00:00

feat(mcp): migrate MCP client from nanobot to official modelcontextprotocol/go-sdk (#5)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

area/engine

  

clarity/clear

  

enhancement

  

security

  

size/M

No labels

area/engine

clarity/clear

enhancement

security

size/M

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

basepeak-ai/gptscript!5

No description provided.