SANDBOX-1567: Add stateless mode configuration#39
Merged
Conversation
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
WalkthroughAdds a stateless mode flag/env, threads it into server construction and the MCP HTTP handler, rejects stateless with stdio transport, exposes stateless replicas behind an nginx load‑balancer in compose, updates the container entrypoint, and expands e2e tests for stateful vs stateless behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant StartCmd as start_server
participant Argocd as ArgoCD_Client
participant ServerPkg as internal/server
participant MCPHandler as MCP_HTTP_Handler
participant Transport as Transport
CLI->>StartCmd: parse flags (including --stateless)
StartCmd->>Transport: validate transport vs stateless
alt transport == stdio and stateless == true
Transport-->>StartCmd: error -> abort
else
StartCmd->>Argocd: create/obtain client
StartCmd->>ServerPkg: New(logger, Argocd, stateless)
ServerPkg-->>StartCmd: Server instance (capabilities set)
StartCmd->>MCPHandler: configure StreamableHTTPOptions{Stateless: stateless}
StartCmd->>Transport: start listening (http)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
fbm3307
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/start_server.go`:
- Around line 98-110: Remove the unresolved merge markers and merge the two
variants so the HTTP server initialization uses the StreamableHTTPOptions struct
with Stateless set (ensure the call includes
&mcp.StreamableHTTPOptions{Stateless: stateless} where the mcp server is
constructed) and also re-add the metrics handler by registering
mux.Handle("/metrics", promhttp.Handler()) before the healthcheck registration;
update the block around the mcp.New/handler setup (look for
StreamableHTTPOptions, stateless, mux.Handle and promhttp.Handler references) to
contain both the options and the metrics endpoint and remove the conflict marker
lines (<<<<<<<, =======, >>>>>>>).
- Around line 63-67: Resolve the git conflict markers in the logger.Info call by
removing the conflict lines (<<<<<<< HEAD, =======, >>>>>>> master) and keep a
single logger.Info invocation that includes the fields "transport", "argocd-url"
(use that key name), "insecure" (argocdInsecure), "debug" (debug) and also
include "stateless" (stateless); update the call site (logger.Info) so it
compiles with no merge markers and uses "argocd-url" consistently.
🧹 Nitpick comments (1)
cmd/start_server.go (1)
76-78: Appropriate guard for stdio transport.Good validation to prevent incompatible configuration. However, consider moving this check to
PreRunEalongside the transport validation for consistency and earlier failure.♻️ Optional: Move validation to PreRunE
PreRunE: func(_ *cobra.Command, _ []string) error { if transport != "stdio" && transport != "http" { return fmt.Errorf("invalid transport: choose between 'http' and 'stdio'") } + if stateless && transport == "stdio" { + return fmt.Errorf("stateless mode is not supported for stdio transport") + } return nil },Then remove lines 76-78 from
RunE.
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/e2e/server_test.go`:
- Around line 461-476: The trailing, unused nolint directive causes a lint
failure; remove the "//nolint:gosec" comment from the return statement in
newHTTPServerCmd so the call to exec.CommandContext(ctx, "argocd-mcp-server",
args...) no longer carries the obsolete directive; ensure no other nolint
annotations remain in that function (newHTTPServerCmd) or on the
exec.CommandContext call.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/e2e/server_test.go`:
- Around line 539-552: The test currently conditionally checks
initResult.Capabilities.Tools and may skip asserting Tools.ListChanged; update
assertListChanged to require the Tools capability is present (use require.NotNil
on initResult.Capabilities.Tools) before asserting Tools.ListChanged so the test
fails loudly on regressions; keep the existing require.NotNil checks for
InitializeResult and Capabilities and the Prompts branch unchanged, and refer to
the assertListChanged function and InitializeResult/Capabilities/Tools symbols
when making the change.
🧹 Nitpick comments (1)
test/e2e/server_test.go (1)
268-358: Consider avoiding hard-coded ports to reduce E2E flakiness.
Fixed ports can collide on shared CI runners or dev machines. Consider selecting free ports or allowing env overrides so the tests are less brittle.
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
xcoulon
reviewed
Jan 22, 2026
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
xcoulon
reviewed
Jan 27, 2026
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/e2e/server_test.go`:
- Around line 42-49: The composite literal for the "http" test case (and the
similar literal around the later block) is mis-indented causing gofmt failures;
fix by reformatting the composite literals so fields line up with Go formatting
conventions—either run `gofmt -w` on the file or adjust the braces/indentation
for the test case structs that initialize mcp.ClientSession via newClientSession
(look for the test case with name "http" and the later composite at lines
~241-248) so the init func, keys, and values are properly indented and the file
passes gofmt.
xcoulon
reviewed
Jan 27, 2026
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/e2e/server_test.go`:
- Around line 293-297: The loop currently uses a short declaration which
re-declares err and shadows the earlier err; to fix it, declare variables
outside the loop (e.g., var tools *mcp.ListToolsResponse; var err error) or
reuse the existing err by replacing the := with = inside the for loop so the
call to session.ListTools(ctx, &mcp.ListToolsParams{}) assigns to the
already-declared err; update the loop that calls session.ListTools and the
variable declarations so only one err variable exists (referencing
session.ListTools, tools, and err).
🧹 Nitpick comments (1)
test/e2e/server_test.go (1)
380-394: Consider aligning strictness withassertInitializeResponse.
assertInitializeResponseusesrequire.NotNilfor Tools capability (line 418), butassertListChangeduses a conditional check. For consistency and to fail loudly on regressions, consider aligning the validation approach.💡 Optional: Align with assertInitializeResponse
func assertListChanged(t *testing.T, session *mcp.ClientSession, expected bool) { t.Helper() initResult := session.InitializeResult() require.NotNil(t, initResult, "should have initialize result") require.NotNil(t, initResult.Capabilities, "should have capabilities") - if initResult.Capabilities.Tools != nil { - assert.Equal(t, expected, initResult.Capabilities.Tools.ListChanged, - "Tools.ListChanged should be %t", expected) - } + require.NotNil(t, initResult.Capabilities.Tools, "should have tools capability") + assert.Equal(t, expected, initResult.Capabilities.Tools.ListChanged, + "Tools.ListChanged should be %t", expected) if initResult.Capabilities.Prompts != nil {
xcoulon
reviewed
Jan 27, 2026
xcoulon
reviewed
Jan 27, 2026
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
Signed-off-by: Feny Mehta <fbm3307@gmail.com>
xcoulon
approved these changes
Jan 28, 2026
Collaborator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We want to run multiple replicas of the mcp server. and to be able to do that it needs to be stateless.
Adding the configuration for it.
Added e2e tests too for it
Taken inspiration from PR
Assisted by : Cursor
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.