[ajitpratap0]

Summary

Stacked on #831 (which is stacked on #823); retarget after those merge.

• Pin the session ID manager of the streamable HTTP transport to mcp-go's StatelessGeneratingSessionIdManager (newSessionIdManager in pkg/mcp/server.go) instead of relying on the library default.
• Redact session IDs from transport error logs. transportLogger.Errorf now prints arguments only for an allowlist of mcp-go formats known to carry no session ID (I/O write failures, ErrSessionExists). Any other format, including formats added in future mcp-go releases, is logged with [redacted] arguments. Generated mcp-session-<uuid> IDs are also scrubbed from every error line as a second line of defence. Info messages are still dropped, as they were in fix(mcp): harden gosqlx-mcp HTTP server (rate limiting, timeouts, auth, tool errors) #831.

Root cause

Terminated-session map (latent; not active on this branch). In mcp-go v0.45.0, InsecureStatefulSessionIdManager stores every terminated ID in terminated sync.Map (server/streamable_http.go:1489, Store at :1523), and nothing ever prunes it. I checked the code: this server never used that manager. It ran on the library default, StatelessGeneratingSessionIdManager (streamable_http.go:218), which keeps no record of IDs. So the unbounded growth was not happening in production. The risk was that a future change to mcp-go's default would turn it on with no change on our side. Pinning the manager removes that risk. Our tools need no per-session state, so stateless mode loses nothing. DELETE and the idle sweeper from #831 still free the transport's per-session state.

Session IDs in logs (an active bug, which #831 did not fix). #831 drops Infof, but some Errorf paths include the client-supplied Mcp-Session-Id:

• streamable_http.go:363 logs Failed to handle sampling response: %v with errors from deliverSamplingResponse (no active session found for session <id>, :826) and Validate (invalid session id: <id>, :1471).
• :811 logs Failed to deliver sampling response: %v.

Any client can reach these paths without auth by POSTing a response-shaped JSON-RPC message (no method, with id and result). The ID, or any string the client puts in the header, then lands in the server log. The privacy page says MCP session identifiers are "never logged".

Test Plan

New file pkg/mcp/session_retention_test.go:

• TestNewSessionIdManager_KeepsNoTerminatedSet: the manager is the stateless generating one, and after Terminate it does not report the ID as terminated, so nothing is tracked.
• TestServer_CreateTerminateCyclesRetainNothing: 500 initialize+DELETE cycles over HTTP, then a ping with the first terminated ID returns 200. With InsecureStatefulSessionIdManager it returns 404 Session terminated, which means IDs are being recorded.
• TestTransportLogger_RedactsSessionIDsInErrors (table test): unknown formats are redacted, safe formats keep their error detail, and generated IDs are scrubbed even from safe formats.
• TestServer_SessionIDsNotLoggedEndToEnd: sends sampling-response POSTs through the real transport, with both a real ID and a forged one. The error is logged, but neither ID (nor the UUID part of the real one) appears in the log.

Mutation check: I swapped in InsecureStatefulSessionIdManager and the old Errorf. Both end-to-end tests failed (404 on the terminated ID; the real and forged IDs leaked into the log). Both pass with the fix.

Commands run (all passing):

go test -race -timeout 120s ./pkg/mcp/... ./cmd/gosqlx-mcp/   # ok
go test -race -count=10 -run 'CreateTerminate|NotLogged|Redacts' ./pkg/mcp/   # ok
go vet ./pkg/mcp/ ./cmd/gosqlx-mcp/                           # clean
gofmt -l pkg/mcp                                              # clean
golangci-lint run ./pkg/mcp/...                               # 0 issues
pre-commit hook (fmt, vet, go test -short ./...)              # passed

Breaking Changes

None. The transport already used stateless generating IDs, so client-visible behaviour is unchanged. Arguments of non-allowlisted transport errors are now redacted in server logs.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RkzVu3JY9Y6BRWfjesWts9

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
gosqlx-website	Ready	Preview	Oct 21, 2026 11:33am UTC

[github-actions]

Claude finished @ajitpratap0's task in 24s —— View job

---

Review of PR #835

I reviewed the server.go diff. I did not read session_retention_test.go or run the tests, so the PR's test results are unverified by me.

Overall: The change is small, focused and does what it says. Pinning the session ID manager is a cheap guard against an upstream default change. Redacting by allowlist means formats added in future mcp-go releases fail closed.

Strengths

• The allowlist fails closed. Unknown formats get [redacted] arguments.
• The mcp-session-<uuid> scrub is a sensible second layer. It runs on the already-formatted message, so it also covers safe formats.
• Printf("...%s", msg) is used, so % characters in a message can't be read as format verbs.
• The PR description is honest that the terminated-session growth was latent, not active.

Suggestions (non-blocking)

1. Allowlist drift on mcp-go upgrades. The allowlist matches v0.45.0 format strings exactly. A reworded upstream format silently becomes redacted. That is safe, but it means useful error detail is lost without any signal. Consider a test that pins the mcp-go version, or a comment in go.mod or the test to re-audit the allowlist on upgrade.
2. Failed to register POST session: %v. The comment says this is ErrSessionExists only. If a future mcp-go version wraps the ID into that error, the regex scrub still catches generated IDs. It would not catch a forged non-UUID ID. That is acceptable, but worth knowing.
3. Regex scope. mcp-session-[0-9A-Fa-f-]+ only covers generated IDs. Forged IDs are covered only through the allowlist, which is the right design. The end-to-end test with a forged ID covers this.
4. Log injection. For allowlisted formats, the error text comes from I/O errors and is not client-controlled. Nothing to fix.
5. Stateless manager semantics. After DELETE, the ID is still accepted. The code comments and the privacy-policy comment document this, and the PR's ping test asserts it. Nothing to change.

Performance: Negligible. The regex and Sprintf run only on the error path.

Security: This closes a real, unauthenticated log-leak path. No new concerns found.

Verdict: Looks good to merge once the stacked base PRs (#831 and #823) land.

[github-actions]

Claude finished @ajitpratap0's task in 14s —— View job

---

Review of PR #835

I read the server.go diff and session_retention_test.go. I did not run the tests, so I'm relying on the PR's reported results.

Verdict: Good to merge once the stacked base PRs (#831 and #823) land. I found no blocking issues.

What works

• newSessionIdManager pins the stateless manager, so a change to the mcp-go default can't switch the server to the unbounded terminated-ID map. The comment explains why, and it accurately says this risk was latent.
• The allowlist fails closed. Formats that aren't listed, including any added in future mcp-go releases, get [redacted] arguments.
• The mcp-session-<uuid> regex scrub is a useful second layer. It runs on the formatted message, so it also covers allowlisted formats.
• Printf("%s", msg) means a % in a message can't be read as a format verb.
• The tests cover both the behaviour and the leak. The end-to-end test checks the real ID, its UUID part and a forged ID. TestServer_CreateTerminateCyclesRetainNothing fails if the stateful manager is swapped in.

Suggestions (non-blocking)

1. Allowlist drift on mcp-go upgrades. The allowlist matches v0.45.0 format strings exactly. If a format is reworded upstream, it silently becomes redacted. That is safe, but the error detail is lost with no signal. A comment next to the mcp-go requirement in go.mod, or in the test, saying to re-audit the allowlist on upgrade would help.
2. Failed to register POST session: %v. The comment says this carries only ErrSessionExists. If a future version wraps the ID into that error, the regex catches generated IDs but not a forged one. Dropping this entry from the allowlist would cost almost nothing and would remove the dependency on that assumption.
3. Regex coverage. The pattern covers only generated IDs. Forged IDs are protected only by the allowlist, which is the right design, and the forged-ID e2e test covers it.
4. Test fragility. TestServer_SessionIDsNotLoggedEndToEnd asserts the log contains "sampling response". That ties it to the mcp-go message wording. It's fine as a sanity check that the error path ran, but expect it to need an update on upgrade.

Performance: The regex and Sprintf run only on the error path, so the cost is negligible.

Security: This closes an unauthenticated path where a client-supplied Mcp-Session-Id reached the server log. I found no new concerns.

[github-actions]

Claude finished @ajitpratap0's task in 17s —— View job

---

Review of PR #835

I read the pkg/mcp/server.go diff (git diff origin/main...HEAD). I did not run the tests, and I did not re-read session_retention_test.go for this pass. The test results are the ones reported in the PR description.

Verdict: No blocking issues. Merge once the stacked base PRs (#831 and #823) land.

What works

• newSessionIdManager pins StatelessGeneratingSessionIdManager. A change to the mcp-go default can't switch the server to the unbounded terminated-ID map. The comment says the risk was latent, which matches the PR description.
• transportLogger.Errorf fails closed. Formats outside transportErrorsWithSafeArgs get [redacted] arguments, and that includes formats added in future mcp-go releases.
• The mcp-session-<uuid> regex runs on the formatted message, so it also covers allowlisted formats.
• Printf("...%s", msg) means a % in a message can't be read as a format verb.
• This closes an unauthenticated path where a client-supplied Mcp-Session-Id could reach the server log.
• The regex and Sprintf run only on the error path, so the cost is negligible.

Suggestions (non-blocking)

1. Failed to register POST session: %v (server.go, allowlist). The comment says it carries ErrSessionExists only. If a future mcp-go version wraps the ID into that error, the regex catches generated IDs but not a forged non-UUID ID. Removing the entry costs almost nothing and removes the dependency on that assumption.
2. Allowlist drift on mcp-go upgrades. The allowlist matches v0.45.0 format strings exactly. If upstream rewords a format, it is silently redacted. That is safe, but the error detail is lost with no signal. A comment next to the mcp-go requirement in go.mod, or in the test, saying to re-audit the allowlist on upgrade would help.
3. Regex scope. mcp-session-[0-9A-Fa-f-]+ covers generated IDs only. Forged IDs are protected by the allowlist alone. That is the right design, and the PR's forged-ID end-to-end test covers it.
4. Test fragility. The PR description says the end-to-end test checks that the log contains the sampling-response error. If that assertion matches mcp-go's message wording, expect to update it on upgrade.

Test coverage: The PR lists four tests that cover both the manager pinning and the log leak. The PR's mutation check (swapping in the stateful manager and the old Errorf) is good evidence that they would catch a regression.