feat(stack): add stdio MCP bridge#160
Conversation
WalkthroughAdds a new ChangesMCP Stack Command with API-Scoped Token Authentication
Sequence DiagramsequenceDiagram
participant Client as Client<br/>(stdio)
participant StdioServer as Stdio Server
participant RemoteClient as Remote Client<br/>(HTTP)
participant RemoteAPI as Remote MCP<br/>Endpoint
Client->>StdioServer: JSON-RPC request/notification<br/>(framed)
StdioServer->>StdioServer: Parse frame,<br/>unmarshal JSON-RPC
alt notification
StdioServer->>RemoteClient: Forward notification
else request (ping)
StdioServer->>StdioServer: Handle locally
else request (other)
StdioServer->>RemoteClient: Forward request
end
RemoteClient->>RemoteAPI: POST /api/mcp<br/>with session/protocol headers
RemoteAPI->>RemoteClient: JSON or SSE response
RemoteClient->>RemoteClient: Decode response<br/>(JSON-RPC or SSE)
RemoteClient->>StdioServer: Return result/error
StdioServer->>StdioServer: Compose JSON-RPC<br/>response
StdioServer->>Client: JSON-RPC response<br/>(newline-terminated)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/stack/mcp/root.go`:
- Around line 185-193: The handler for "notifications/cancelled" currently only
logs locally; modify stdioServer.handleNotification so that when msg.Method ==
"notifications/cancelled" it forwards the same msg to the remote MCP server by
calling s.remote.Notify(context.Background(), msg) (just like the
"notifications/initialized" branch), check and log any error from that call
(e.g., using s.err or similar) while still preserving the local cancellation
log; use the existing rpcMessage and s.remote.Notify symbols to locate and
implement the change.
- Around line 364-380: The code currently parses Content-Length via
strconv.Atoi(headerOrJSON) and then allocates payload := make([]byte, length)
without checking for negative values; strconv.Atoi("-1") succeeds and causes a
panic on make. Update the parsing branch (identify headerOrJSON, strconv.Atoi
call, and payload := make([]byte, length)) to validate length >= 0 after
conversion and return a framing/error (e.g., fmt.Errorf("invalid Content-Length:
%d", length)) if length is negative (or otherwise out of expected bounds) before
attempting to allocate or read the payload.
- Around line 196-211: The default case in stdioServer.handleRequest currently
forwards the request to s.remote.Request but masks any upstream failures as
rpcError Code -32601 ("method not found"); change it to propagate the real error
instead: when s.remote.Request(ctx, msg) returns a non-nil err, return
&rpcError{Code: -32000, Message: err.Error()} (or otherwise include err.Error())
so auth/transport/5xx failures are preserved; only return -32601 when there is a
clear indication that the remote responded with an actual "method not found"
condition. Reference: stdioServer.handleRequest, s.remote.Request, rpcError.
In `@pkg/clients.go`:
- Around line 644-645: The cache logic incorrectly ignores apiResource, so
callers requesting a resource-bound token can read/write an unscoped cache
entry; update the cache guard where ReadCachedStackAPIToken (and symmetric
WriteCachedStackAPIToken) are used to require both len(t.apiScopes) == 0 AND
t.apiResource == "" (i.e., only use the shared unscoped cache when there are no
apiScopes AND no apiResource), and apply the same change to the other occurrence
around the Read/Write calls referenced (the block near the second occurrence at
lines 679-680).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ee036afa-b578-41e5-b8a8-21a2d8986cfa
📒 Files selected for processing (5)
cmd/stack/mcp/root.gocmd/stack/mcp/root_test.gocmd/stack/root.gopkg/authentication.gopkg/clients.go
gfyrag
left a comment
gfyrag
left a comment
There was a problem hiding this comment.
Review / Revue
🇬🇧 English
Overall: Solid PR — clean architecture, good test coverage, backward-compatible auth changes. A few items to address before merge.
Auth compatibility with formancehq/auth
I checked the auth server code. There are two important findings:
-
The
scoperequest parameter is ignored in the jwt-bearer grant flow. Inpkg/oidc/grant_type_bearer.go,ValidateJWTProfileScopes()returns scopes as-is from the JWT assertion — thescopeform parameter sent by fctl is not used. The e2e test (test/e2e/jwt_bearer_test.go) confirms this: requested scopes are ignored, only assertion scopes end up in the token. Soledger:read,payments:read,reconciliation:readwon't actually appear in the resulting token unless the auth server is updated. -
The
resourceparameter (RFC 8707) is not implemented. The auth server has no code handling it. Audience is always set toApplicationID(client ID), not to the resource URL. Theresourceform value sent by fctl is silently dropped.
This is consistent with the PR description ("auth-side change needed"), but worth calling out explicitly: both scope and resource plumbing are dead code until the auth server supports them.
Code issues
-
readMCPMessagehas no Content-Length upper bound (root.go:~330): AContent-Length: 9999999999would allocate ~10GB. Add a max size check (e.g. 10MB) beforemake([]byte, length). -
handleRequestswitch is redundant (root.go:~175-185): Thedefaultcase does exactly the same thing as the"initialize", "tools/list", "tools/call"case. Either simplify to just forward everything, or add a// TODOcomment explaining future differentiation. -
decodeSSEResponseonly handles a single SSE event (root.go:~290): Alldata:lines are joined and decoded as one response. If the remote sends multiple SSE events (streaming MCP), this will break. Fine for now if the remote always sends a single event, but should be documented. -
No
ctx.Done()check inServeloop (root.go:~137): The loop blocks onreadMCPMessage(reader)without listening to context cancellation. Graceful shutdown won't work until stdin closes.
Token source API
The 3-constructor chain (NewStackTokenSource → WithAPIScopes → WithAPIScopesAndResource) works but is getting heavy. Consider a functional options or config struct pattern if more parameters are added.
Cache bypass for custom scopes/resources is correct — good call not polluting the shared cache.
🇫🇷 Français
Globalement : PR solide — architecture propre, bonne couverture de tests, changements auth backward-compatible. Quelques points à traiter avant le merge.
Compatibilité auth avec formancehq/auth
J'ai vérifié le code du serveur auth. Deux constats importants :
-
Le paramètre
scopede la requête est ignoré dans le flow jwt-bearer. Danspkg/oidc/grant_type_bearer.go,ValidateJWTProfileScopes()retourne les scopes tels quels depuis le JWT d'assertion — le paramètrescopedu formulaire envoyé par fctl n'est pas utilisé. Le test e2e (test/e2e/jwt_bearer_test.go) le confirme : les scopes demandés sont ignorés, seuls ceux de l'assertion se retrouvent dans le token. Doncledger:read,payments:read,reconciliation:readne seront pas dans le token résultant sans mise à jour du serveur auth. -
Le paramètre
resource(RFC 8707) n'est pas implémenté. Le serveur auth n'a aucun code pour le gérer. L'audience est toujours leApplicationID(client ID), pas l'URL de la ressource. La valeurresourceenvoyée par fctl est silencieusement ignorée.
C'est cohérent avec la description de la PR ("auth-side change needed"), mais ça mérite d'être explicité : le plumbing scope et resource est du code mort tant que le serveur auth ne les supporte pas.
Problèmes dans le code
-
readMCPMessagen'a pas de limite haute sur Content-Length (root.go:~330) : UnContent-Length: 9999999999allouerait ~10GB. Ajouter une vérification de taille max (ex: 10MB) avantmake([]byte, length). -
Le switch dans
handleRequestest redondant (root.go:~175-185) : Ledefaultfait exactement la même chose que le case"initialize", "tools/list", "tools/call". Soit simplifier en forwardant tout, soit ajouter un commentaire// TODOexpliquant la différenciation future. -
decodeSSEResponsene gère qu'un seul événement SSE (root.go:~290) : Toutes les lignesdata:sont jointes et décodées comme une seule réponse. Si le remote envoie plusieurs événements SSE (streaming MCP), ça cassera. OK pour l'instant si le remote envoie toujours un seul événement, mais à documenter. -
Pas de vérification
ctx.Done()dans la boucleServe(root.go:~137) : La boucle bloque surreadMCPMessage(reader)sans écouter l'annulation du contexte. Le shutdown gracieux ne fonctionnera pas tant que stdin n'est pas fermé.
API TokenSource
La chaîne de 3 constructeurs (NewStackTokenSource → WithAPIScopes → WithAPIScopesAndResource) fonctionne mais devient lourde. Envisager un pattern functional options ou config struct si d'autres paramètres s'ajoutent.
Le bypass du cache pour les scopes/resources custom est correct — bonne décision de ne pas polluer le cache partagé.
|
Addressed the new review points in a5f38f1:
Validation: |
|
Actionable comments posted: 0 |
|
Simplified in 6f9f4fb after the scope/resource discussion:
Validation: |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/stack/mcp/root.go (1)
66-78:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse relying-party HTTP transport for the MCP OAuth2 client
runServecreateshttpClient := oauth2.NewClient(cmd.Context(), tokenSource)butcmd.Context()isn’t augmented withoauth2.HTTPClientfromrelyingParty.HttpClient()anywhere in the MCP stack path.- All MCP
/api/mcprequests use this returned client (remoteMCPClientcallsc.httpClient.Do(req)), so custom TLS/proxy/transport settings fromrelyingParty.HttpClient()can be skipped even though token refresh already usesrelyingParty.HttpClient()insideNewStackTokenSource.Suggested fix
tokenSource := fctl.NewStackTokenSource( *stackToken, stackAccess, relyingParty, func(newToken fctl.AccessToken) error { return fctl.WriteStackToken(cmd, profileName, stackID, newToken) }, cmd, profileName, organizationID, stackID, ) - httpClient := oauth2.NewClient(cmd.Context(), tokenSource) + oauthCtx := context.WithValue(cmd.Context(), oauth2.HTTPClient, relyingParty.HttpClient()) + httpClient := oauth2.NewClient(oauthCtx, tokenSource)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/stack/mcp/root.go` around lines 66 - 78, runServe currently calls oauth2.NewClient(cmd.Context(), tokenSource) which ignores the custom transport from relyingParty.HttpClient(); instead, create a context that injects the relyingParty HTTP client via oauth2.HTTPClient (e.g. ctx := context.WithValue(cmd.Context(), oauth2.HTTPClient, relyingParty.HttpClient())) and pass that context to oauth2.NewClient so the returned httpClient used by remoteMCPClient uses the relyingParty transport; update the creation of httpClient (and any uses of tokenSource if needed) to use this new ctx while keeping NewStackTokenSource and token refresh behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmd/stack/mcp/root.go`:
- Around line 66-78: runServe currently calls oauth2.NewClient(cmd.Context(),
tokenSource) which ignores the custom transport from relyingParty.HttpClient();
instead, create a context that injects the relyingParty HTTP client via
oauth2.HTTPClient (e.g. ctx := context.WithValue(cmd.Context(),
oauth2.HTTPClient, relyingParty.HttpClient())) and pass that context to
oauth2.NewClient so the returned httpClient used by remoteMCPClient uses the
relyingParty transport; update the creation of httpClient (and any uses of
tokenSource if needed) to use this new ctx while keeping NewStackTokenSource and
token refresh behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5ddac6b-9f64-4c74-b40e-3fbdfbbdd626
📒 Files selected for processing (3)
cmd/stack/mcp/root.gocmd/stack/mcp/root_test.gopkg/authentication.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/authentication.go
- cmd/stack/mcp/root_test.go
2 participants
Summary
fctl stack mcp serve --transport stdio/api/mcpendpointValidation
go test ./cmd/stack/mcp ./cmd/stack ./pkggo test ./...Notes
scope/resourceplumbing is intentionally not part of this PR because the currentformancehq/authjwt-bearer grant flow does not apply requested scopes beyond membership/assertion scopes and does not consume RFC 8707resourceyet.