fix: release MCP session state so the server stops leaking heap - #85
Merged
Merged
Conversation
The streamable HTTP server registers a session on `initialize` and, on mcp-go v0.43.2, never releases it: `handleDelete` clears the per-session stores but never calls `UnregisterSession`, so every session a client ever opened is retained. A POST-only client leaks roughly 8 KB per initialize, which in production shows up as linear heap growth until the container is OOMKilled. Bump mcp-go to v1.1.0, which unregisters the session on DELETE, and enable its idle sweeper so sessions abandoned without a DELETE are reclaimed too. The sweeper is opt-in even on v1.1.0, so the bump alone is not enough. The new `--session-idle-ttl` flag defaults to 10m and 0 disables it. Shutdown now stops the sweeper and closes sessions that are still registered. Measured over repeated bursts of 500 initialize requests, 15s apart: mcp-go v0.43.2 3.44 -> 7.74 -> 12.96 -> 17.96 -> 21.65 MB live heap this change 3.93 -> 9.55 -> 9.63 -> 9.55 -> 9.55 MB live heap Closes #84 Signed-off-by: younsl <cysl@kakao.com>
`make test` covered ./pkg/... and ./internal/... only, so the tests in cmd/ never ran in CI. That includes the session lifecycle regression tests this branch adds, which is the code path that leaked. Signed-off-by: younsl <cysl@kakao.com>
# Conflicts: # go.mod # go.sum
dimetron
approved these changes
Sep 22, 2026
dimetron
left a comment
dimetron
left a comment
Collaborator
There was a problem hiding this comment.
Verified independently of the PR description.
- Fetched this head and built it:
go build ./...clean. - Ran the new regression tests:
TestStreamableHTTPServerReleasesSessionOnDeleteandTestStreamableHTTPServerSweepsIdleSessionboth PASS; the sweeper test logsSweeping expired session session=mcp-session-..., confirming it actually fires. - Confirmed the root cause against the v0.43.2 tag source:
handlePoststores the session in bothactiveSessionsandserver.sessions, and only the GET/SSE path (streamable_http.go:525-526) unregisters it.handleDeleteat that tag clears only the per-session tool/resource/log-level/request-ID stores, so a POST-only client retains every session forever. - Confirmed
cleanupSessionStatearrives in v0.44.1 and thatWithSessionIdleTTLdefaults to zero even on v1.1.0, so the dependency bump alone is genuinely insufficient. The sweeper option is the part that bounds memory for abandoned sessions. - Fixed the stale branch: merged
main(which had moved on to include the #86 CVE dependency bump) and resolved thego.mod/go.sumconflict.buger/jsonparseris dropped bygo mod tidyafter the mcp-go v1.1.0 bump;go mod whyconfirms it is no longer in the module graph. - CI: build, go-unit-tests, go-e2e-tests, helm-unit-tests, DCO all pass.
Two notes for a follow-up, neither blocking:
- Adding
./cmd/...tomake testpullscmdcoverage (14.2%) into that invocation.ci.yamlhas no coverage threshold, so nothing breaks today, but if a coverage gate is ever added this will interact with it. - The 10m idle TTL is a chosen default, not a derived value. It is safe for the leak (any finite TTL bounds memory) but is worth revisiting once session lifetimes in production are known.
Thanks for the thorough write-up and the repro numbers — the DELETE-frees-nothing control row is what made this diagnosable.
Contributor
Author
|
Thanks for the review and conflict fix! Could this be merged when you have a chance? |
EItanya
approved these changes
Sep 23, 2026
dimetron
added a commit
that referenced
this pull request
Sep 23, 2026
…n leak Port four verified fixes from open PRs. None applied as a patch - all were written against the mark3labs API this branch removed - so each was re-checked against the current code and re-implemented on the typed go-sdk path. k8s_execute_command split its command into argv tokens (#67) The handler passed the whole command as a single argv entry after "--", so the container runtime looked for an executable whose name contained the spaces. Reproduced against a live cluster: kubectl exec pod -- "uname -a" exec: "uname -a": executable file not found in $PATH kubectl exec pod -- uname -a Linux pod ... aarch64 GNU/Linux A single-word command worked, which is why this looked like a routing failure rather than an argv bug. The command is now tokenised, and an args array is appended verbatim so an argument containing spaces can be passed as one token. Fixes a second latent bug in the same handler: Container was declared in the input struct and never used, so -c was silently ignored for multi-container pods. k8s_get_pod_logs gained a previous flag (#79) Adds --previous to read logs from the terminated container instance, ordered before --tail to match kubectl conventions. k8s_get_resources description now states its scope and image behaviour (#70) The description was "Get Kubernetes resources using kubectl". Two things an agent cannot infer from that: omitting both all_namespaces and namespace queries only the tool's own namespace, not the cluster; and the wide output has an IMAGES column for workloads but not for pods, so a pod listing is never a source of image versions. Both produced wrong answers for the reporter. (The all_namespaces string/boolean parsing half of #70 was already fixed by the migration, which typed the field as bool.) kubescape_list_vulnerability_manifests no longer reports a false count (#76) len(manifest.Spec.Payload.Matches) was 0 for every manifest in every cluster: the aggregated API strips spec.payload.matches on LIST and serves it only on GET. The tool reported "vulnerability_count": 0 for images with hundreds of CVEs, and an agent reading that data correctly concluded the cluster was clean. The field is removed rather than corrected - an absent field cannot be mistaken for a measured zero - and the description now says the list is an index and points at kubescape_list_vulnerabilities for real counts. Could not be verified against a live operator here, but the SDK corroborates the cause: upstream ships a dedicated VulnerabilityManifestSummaries clientset whose spec carries SeveritySummary counts and no payload, i.e. summary-for-listing is the intended design and we were listing the full document instead. Streamable HTTP sessions are now reclaimed (#85) cmd/main.go passed nil options, leaving SessionTimeout at its zero value, which disables the idle sweeper. A session never DELETEd - client crash, reset, or POST-only - pinned its goroutine and state for the life of the process. Measured on this branch: 1000 initialize requests without DELETE left 1000 goroutines resident, forever. SessionTimeout is now 10 minutes; with a 2s timeout a probe reclaimed 400 abandoned sessions from 136 goroutines back to 3. Verified: go build ./..., go vet ./... and gofmt clean; go test ./pkg/... ./internal/... ./cmd/... passes 19/19 packages; golangci-lint 0 issues; the e2e suite compiles. New tests cover argv splitting, args verbatim, container selection, --previous present/absent, and the omitted vulnerability_count. Signed-off-by: Dmytro Rashko <dimetron@me.com>
dimetron
added a commit
that referenced
this pull request
Sep 23, 2026
Upstream main still runs mark3labs/mcp-go; this branch migrates to the official go-sdk. The three incoming commits therefore needed judgement, not just a take-theirs/take-ours: * 1bdfcd6 (heap leak, #85) fixes a session leak that exists on mark3labs v0.43.2 by bumping to v1.1.0 and enabling its opt-in sweeper. This branch removed mark3labs entirely in 6dc138b/761a438 and ported the same fix to the go-sdk path. Its only surviving contribution is intent, already implemented, so its cmd/main.go edits are dropped rather than merged - they reference server.StreamableHTTPServer and server.NewMCPServer, which do not exist here. * e5cea22 (prometheus/grafana env vars, #63) applied cleanly to helm/kagent-tools/templates/deployment.yaml. * bd6e49d (quickstart typo, #17) applied cleanly. Conflict resolutions: go.mod/go.sum ours. Upstream is the older line throughout - kubescape k8s-interface 0.0.203 vs 0.0.221, k8s.io 0.35.3 vs 0.37.0, jsonschema-go 0.4.2 vs 0.4.3 - and the SDK differs by design. Re-tidied after. Makefile ours. Upstream pins istio 1.30.1, kubectl 1.36.2, helm 4.2.2, cilium 0.19.4 and golangci-lint v1.63.4; this branch is newer on all five. Keeps the .golangci.yml v2 config that only golangci-lint v2 understands. README.md ours for the --session-idle-ttl default, because this branch defines sessionIdleTTLDefault = 30m; upstream says 10m for its own variable. Upstreams mcp_inspect removal is not taken - the tool is still registered in pkg/utils/common.go:220. cmd/main.go ours (go-sdk API), see 1bdfcd6 above. cmd/streamable_http_test.go ours (add/add, go-sdk version). Verified on the merged tree: go build ./... clean, go vet ./... clean, 19/19 unit packages pass, golangci-lint 0 issues. Signed-off-by: Dmytro Rashko <dimetron@me.com>
Merged
EItanya
pushed a commit
that referenced
this pull request
Sep 23, 2026
* Migrate to GO SDK
Signed-off-by: Dmytro Rashko <dmitriy.rashko@amdocs.com>
* fix(security): resolve govulncheck CVEs and relax MCP SDK input schema
Bump dependencies to clear all 8 govulncheck source-mode findings:
- golang.org/x/net 0.50.0 -> 0.55.0 (GO-2026-5026, GO-2026-4918, GO-2026-4559)
- github.com/cilium/cilium 1.19.0 -> 1.19.3 (GO-2026-5400, GO-2026-4856)
- go.opentelemetry.io/otel 1.40.0 -> 1.43.0, otlploghttp 0.16.0 -> 0.19.0 (GO-2026-4985)
- github.com/go-jose/go-jose/v4 4.1.3 -> 4.1.4 (GO-2026-4945)
- github.com/anchore/syft 1.32.0 -> 1.42.3 (GO-2026-4809)
The only remaining govulncheck reports are 5 uncalled github.com/docker/docker
advisories that have no fixed version available upstream.
Also carry the in-flight go-sdk migration work: relax the inferred input
schema (drop the auto-Required list and allow additional properties) so tools
keep the pre-migration calling contract, add the toolResultText e2e helper for
readable failures, and document the typed MCP tool I/O conventions.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* feat(utils): add mcp_inspect tool and bump MCP SDK to v1.7.0
Add a typed mcp_inspect tool that echoes its input and returns all HTTP
headers received with the MCP request, for debugging client requests.
Headers are canonicalized and sorted for stable output.
Also bump github.com/modelcontextprotocol/go-sdk v1.6.1 -> v1.7.0 and the
go directive to 1.27.0 in support of the migration work.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* chore(lint): bump golangci-lint to v2.13.2 and add config for go 1.27
The pinned v1.63.4 predates the go 1.27 directive and cannot load the
module. Add a version: "2" config and pin the v2 module path so
`make lint` (part of `make test`) runs on the migrated tree.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* refactor(errors): type ToolError.Context as map[string]string
Step 3 of the go-sdk migration: replace the dynamic
map[string]interface{} context with a concrete map[string]string so no
untyped map remains in the error path.
Callers that passed non-string values are converted at the call site:
- prometheus: status_code (int) -> decimal string
- helm: helm_args ([]string) -> space-joined string
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* refactor(kubescape): replace untyped response maps with structs
Step 12 of the go-sdk migration: the read-only scan/report tools built
their JSON responses from map[string]interface{} literals. Introduce
concrete output structs for all seven handlers and decode them typed in
the tests, so no untyped map remains in the kubescape response path.
Conditional fields (seccomp_profile, fix_state/fix_versions) keep their
omit-when-absent behaviour via omitempty and a pointer.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* style: gofmt errors struct and type the last test-only maps
Step 15 cleanup: gofmt the ToolError field alignment, and decode the
logger trace_id assertion into an anonymous struct. The mcp_test payload
stays a map because jsonschema.Resolved.Validate rejects structs for
object schemas (google/jsonschema-go#23) — noted inline.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* refactor: return typed outputs from all MCP handlers
Complete the go-sdk migration by removing the last untyped tool I/O. Every
handler previously returned Out=any, so the SDK could not infer an output
schema, populate StructuredContent, or validate the result. All 145 handlers
now return a concrete Out type.
Raw CLI text goes through the shared mcp.TextOutput wrapper ({output: "..."})
via new mcp.TextResult / mcp.TextError / mcp.TextOf helpers. Structured
responses return their own DTO. Text stays in Content as before, so existing
clients are unaffected; StructuredContent is now populated too.
Three SDK behaviours drove the design, each confirmed against a live
in-memory server:
- json.RawMessage is inferred as a byte slice, not arbitrary JSON, and
validation then rejects real objects/arrays. pkg/prometheus now re-indents
dynamic API JSON with json.Indent instead of decoding into interface{}.
- Zero values are validated on error paths too, so a map field without
omitempty makes every error return fail with "validating tool output".
pkg/kubescape gained omitempty on its map fields; CheckStatus.Details is
typed as []PodCheckEntry instead of interface{}. Slices infer as nullable
and need no change.
- Types with custom JSON marshallers can fail schema inference, which panics
AddTool at registration. v1beta1.WorkloadConfigurationScan is one such type,
so handleGetConfigurationScan returns mcp.TextOutput. The new
cmd/tools_output_schema_test.go registers every provider and fails if any
Out type cannot produce a valid schema.
test/e2e/helpers_test.go now returns *mcp.CallToolResult and []*mcp.Tool
instead of interface{}, and the input-schema regression test exercises a real
client->server call with typed partial arguments rather than validating a
map[string]any fixture by hand.
The only remaining "any" tokens are generic type parameters ([T any]), which
bind concrete types at every call site.
AGENTS.md documents the typed-output contract, the TextResult/TextError/
TextOf helpers, and the three output-schema pitfalls above.
Verified: gofmt clean, go build ./... and go vet ./... pass, make lint reports
0 issues, go test ./pkg/... ./internal/... ./cmd/... passes 19/19 packages,
every pkg/ package is above the 80% coverage gate, and an in-repo grep finds
no interface{}, no Out=any, and no untyped maps.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* ci: pin Go toolchain from go.mod instead of a stale version spec
Both Go jobs pinned `go-version: '^1.26.1'`, which resolves to 1.26.8, but
go.mod requires go >= 1.27.0. actions/setup-go@v6 also sets GOTOOLCHAIN=local
(an intentional change in setup-go 1d76b95), so the toolchain is never
auto-downloaded and the job dies during `make test` -> `make build`:
go: go.mod requires go >= 1.27.0 (running go 1.26.8; GOTOOLCHAIN=local)
This failed before a single test ran, in both go-unit-tests and go-e2e-tests.
The `build` job was unaffected because the Dockerfile uses
chainguard/go:latest, which already ships 1.27.
Use `go-version-file: 'go.mod'` so CI tracks the module's own Go directive and
cannot drift when the directive is bumped again. The Docker build and the lint
config (pinned for the go 1.27 directive) were already consistent with 1.27;
only the two setup-go pins were stale.
Also correct the Go version listed in the AGENTS.md repository tree (1.25.6 ->
1.27.0), which is the same drift that caused this.
Verified under the exact CI condition (GOTOOLCHAIN=local, go.mod at 1.27.0):
go build ./..., go vet ./... and go test ./pkg/... ./internal/... ./cmd/...
all pass, and make lint reports 0 issues. Reproduced the original failure as a
control by running go 1.26.8 with GOTOOLCHAIN=local against the 1.27.0
directive.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* test(e2e): type e2e assertions and fix setup races
The e2e suite reported success without exercising the migrated behaviour: three
specs skipped silently, a helper swallowed tool errors, and setup raced
kube-proxy. Fix the harness and assert the typed-output contract end to end.
Assertions now decode structuredContent into the shared mcp.TextOutput DTO via
decodeTextOutput, so a handler regressing to Out=any fails the suite. Applied to
the k8s, helm, istio, cilium and argo specs; a new istio_version spec runs
against the installed control plane.
Bugs fixed:
- ciliumStatus was the only MCP helper that did not check result.IsError, so its
spec passed even when the tool returned an error. It now returns an error.
- The helm spec never ran: helmListReleases sent all_namespaces as the string
"true" while the tool declares a boolean, so every call failed input
validation and the spec skipped itself. It now sends a bool.
- InstallKAgentTools waited on pod status.phase only. phase=Running precedes the
readiness probe (initialDelaySeconds=15), so GetMCPClient could connect to a
server that had not begun serving. It now waits for the Ready condition and
probes the NodePort before the suite starts.
- CreateNamespace/DeleteNamespace ignored in-flight namespace deletion, so
consecutive runs failed with "unable to create new content ... because it is
being terminated". Both now wait for the namespace to settle. Note
--ignore-not-found exits 0 for a missing namespace, so the waits key on empty
output rather than on an error.
- GetMCPClient retries the initialize handshake, because the NodePort resets
connections until kube-proxy programs the new endpoint.
- The helm install context (120s) was shorter than helm's own timeout, killing
helm with "signal: killed"; the context now outlives it, and --timeout is 3m
to clear the readiness delay.
Adds an opt-in Cilium lifecycle spec (E2E_CILIUM_LIFECYCLE=true) that installs
Cilium through cilium_install_cilium, verifies status, then uninstalls through
cilium_uninstall_cilium. It is opt-in because it replaces the cluster CNI, which
Kind does not use by default and CI cannot tolerate. Installing a CNI resets pod
networking on the node and drops the pod's own long-lived MCP session, so the
spec reconnects afterwards. The uninstall runs inside the It rather than
DeferCleanup: the ordered container's AfterAll deletes the namespace first,
leaving no server to call. A host-side DeferCleanup removes a leaked DaemonSet
so a failure cannot poison later runs.
Also set tools.metrics.port=8085 in test-values-e2e.yaml. Without it the
deployment template emits containerPort 8084 twice and Helm 4 rejects the
manifest with "duplicate entries for key [containerPort=8084]", which broke
every deploy.
Verified locally on a Kind cluster with the repo's NodePort mappings:
- default suite: 24 passed, 0 failed, 2 skipped (the two Cilium specs)
- with E2E_CILIUM_LIFECYCLE=true: 25 passed, 0 failed, 1 skipped, Cilium
installed and then uninstalled, nothing leaked
- 3 consecutive default runs passed, where the same sequence previously failed
roughly half the time
- gofmt and go vet clean; unit tests unaffected (19/19 packages pass)
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* test(e2e): sweep every read-only tool for typed-output conformance
The suite previously touched about 8 of the 126 advertised tools. Add a
data-driven sweep that drives all 80 tools the server registers in read-only
mode and asserts two invariants for each:
- the call completes without a protocol/transport error, and
- the result honours the typed-output contract: a success carries
structuredContent (so a handler regressing to Out=any fails the suite), and
a failure reports IsError with a readable message rather than breaking
transport.
The read-only set is taken from the providers themselves: the sweep registers
argo, cilium, helm, istio, k8s, kubescape, prometheus and utils with
readOnly=true and uses the resulting tool list as the safety rail. No name
pattern guessing, and a write-capable tool cannot be reached from the sweep's
invocation list. This matters because an earlier attempt at a regex
classification wrongly flagged helm_repo_update, which the server registers
read-only (it only refreshes the local chart cache).
Tools needing identifiers that exist only alongside a live dependency (Cilium
endpoint/service/recorder IDs, an existing Helm release, a vulnerability
manifest name) are enumerated with an explicit skip reason rather than called
with invented arguments, which would only re-test input validation. Tools whose
backing dependency is absent on a Kind cluster (Cilium on kindnet, no Prometheus
server, no Kubescape operator) legitimately answer with a tool error; the sweep
records those instead of failing, so the run reports
ok=24 toolerr=32 skipped=24 for 80 covered tools.
The sweep runs inside the ordered k8s container, before AfterAll deletes the
namespace, so the deployed server is still reachable.
Also fixes a bug introduced earlier with the namespace race fix: CreateNamespace
waited for the namespace to be absent, but a healthy namespace left over from a
previous run must be reused, not waited out. It now waits for the namespace to be
usable (gone, or present and Active). DeleteNamespace no longer blocks, since
deletion may linger on CRD finalizers and would turn a slow finalizer into an
AfterAll failure; CreateNamespace is the only place the wait matters.
Verified on a local Kind cluster with the repo's NodePort mappings: the suite
passes 26 specs (0 failed) and twice consecutively; gofmt and go vet are clean
and unit tests still pass 19/19 packages.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* chore(deps): bump Go dependencies and bundled CLI versions
Go dependencies, direct:
- github.com/modelcontextprotocol/go-sdk v1.7.0 -> v1.8.0 (the release that
prompted this). It adds no new protocol revision; the work is transport
hardening plus ServerOptions.SupportedProtocolVersions and SetCacheable. The
output-schema machinery the typed-output migration depends on (toolForErr,
setSchema, ToolHandlerFor, AddTool) is byte-for-byte unchanged, so no handler
changes were needed.
- k8s.io/{api,apimachinery,client-go,apiextensions-apiserver} v0.35.3 -> v0.37.0
- go.opentelemetry.io/otel* v1.43.0 -> v1.46.0
- github.com/prometheus/client_golang v1.23.2 -> v1.24.1, client_model v0.6.2 -> v0.6.3
- github.com/kubescape/k8s-interface v0.0.203 -> v0.0.221
- github.com/onsi/ginkgo/v2 v2.27.2 -> v2.33.0, gomega v1.38.2 -> v1.43.1
- github.com/stretchr/testify v1.11.1 -> v1.12.1
- go directive 1.27.0 -> 1.27.1
github.com/kubescape/storage v0.0.239 -> v0.0.300, not the latest v0.2.0. That
module is imported directly for its v1beta1 API types and generated clientset,
and upstream removed both the types and the client methods this code uses
(v1beta1.ExecCalls, OpenCalls, HTTPEndpoint, NetworkConnections,
CommunicationType, NetworkPort, SingleSeccompProfile, and the
ApplicationProfiles/WorkloadConfigurationScans/NetworkNeighborhoods client
methods). v0.0.300 is the newest version that still exposes them; going further
requires rewriting the kubescape DTOs, which is out of scope here.
Bundled CLI versions, checked against each project's latest release:
- istioctl 1.30.1 -> 1.31.0
- argo 1.9.0 -> 1.10.0
- kubectl 1.36.2 -> 1.37.0
- helm 4.2.2 -> 4.3.0
- cilium 0.19.4 -> 0.20.0
make check-releases now reports all six checks green.
Verified: go build ./... and go vet ./... pass; go test ./pkg/... ./internal/...
./cmd/... passes 19/19 packages; make lint reports 0 issues; gofmt clean. The
image was rebuilt with make docker-build and each binary was executed inside it
to confirm the versions actually installed, rather than trusting the pin:
kubectl v1.37.0, helm v4.3.0, istioctl 1.31.0, cilium-cli v0.20.0,
kubectl-argo-rollouts v1.10.0, and the server reporting go1.27.1.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* docs: add migration spec set and CLAUDE.md agent guide
Add the specification documents produced for the mark3labs/mcp-go -> official
go-sdk migration, plus a Claude Code entry point.
specs/ holds the migration materials: requirements, design, plan, research
comparison and the skill notes. specs/tools/001-migrate-mcp-go-to-official-sdk/
is the current copy and carries the run summary; the older
specs/migrate-mcp-go-to-official-sdk/ copy shares five byte-identical files.
CLAUDE.md mirrors the repository guide for Claude Code.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* docs: correct CLAUDE.md to point at AGENTS.md and drop stale API
CLAUDE.md documented the pre-migration mark3labs API throughout: the tool
registration example used mcp.NewTool with WithString/WithDescription, handlers
used request.RequireString()/RequireBool(), and results used
mcp.NewToolResultText(). None of that exists in the codebase after the go-sdk
migration, so the examples would not compile and an agent following them would
write handlers that break. It also had no mention of the typed-output contract
that the migration introduced.
It referenced things that are not in the repository either: pkg/logger (logging
lives in internal/logger), make coverage-report (no such target), coverage.md,
and test/integration/.
Replace the duplicated and outdated content with a short file that directs
readers to AGENTS.md as the single source of truth and adds only what AGENTS.md
does not cover: the run-local flags, the registration/typed-output points that
are easy to get wrong, and the logging package location.
Also correct a claim both files made: that CI enforces the 80% coverage
threshold. The go-unit-tests job runs "go test -v -cover", which reports
coverage but has no gate, so a threshold breach cannot fail the build. Both
files now describe 80% as the standard to check manually, and note that
internal/commands and internal/cmd sit below it.
Every claim in the rewritten file was checked against the repository: the CLI
flags against "--help", each linked file for existence, per-package coverage
with "go test -cover", the DCO check against the open pull request, and the
claim that no provider imports the SDK directly with grep (all 127
registrations go through internal/mcp).
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* fix(security): redact credentials in mcp_inspect and close two review findings
Findings from a codex review of this branch.
1. mcp_inspect disclosed credentials (high).
The HTTP transport hands handlers the raw inbound header set, so req.Header
contains Authorization, Cookie and anything else the client sent. The tool
returned every value to any caller able to invoke it, exposing the caller's own
bearer token and session secrets.
Header values are now withheld when the canonical name contains a
credential-bearing fragment (authorization, cookie, token, secret, api-key,
credential, password, authenticat, session, bearer, jwt, signature). An earlier
exact-match list was insufficient and a second review pass caught it: it missed
Private-Token, X-Amz-Security-Token, X-Access-Token and similar real-world
names, so the policy is deny-by-substring. Over-redacting a debugging value
costs nothing; leaking a credential does not. Header names are still returned
and the received value count is preserved, so the tool remains useful for
debugging which headers arrived.
2. Tool-level failures left the span status unset (medium).
A handler signalling failure via IsError=true with a nil Go error incremented
the Prometheus failure counter but left the OTel span status unset, because both
RecordError and SetStatus sat inside `if err != nil`. Traces therefore
disagreed with metrics and the span was neither Ok nor Error. The middleware now
marks such spans Error using the tool's message, and sets is_error on the span.
3. kubescape_get_vulnerability_details emitted its payload twice (medium).
For a non-object (array) Out the SDK appends the serialised value as an extra
TextContent block, so returning the matches as Out *and* marshalling them into
Content produced the vulnerability list twice for clients reading Content. The
handler now returns `nil, matches, nil` and lets the SDK serialise.
Tests cover each fix and were checked for vacuity: reverting the span fix makes
TestToolMiddleware_MarksSpanForToolLevelError fail with "span status: expected
Error, got Unset", and TestIsSensitiveHeader pins both directions of the
redaction policy (18 credential names withheld, 8 innocuous headers kept
visible). Verified end to end by deploying the image and sending Authorization,
Private-Token, X-Amz-Security-Token and X-Access-Token: all returned [REDACTED]
while X-Request-Id stayed visible.
Verified: go build ./..., go vet ./... and gofmt clean; go test ./pkg/...
./internal/... ./cmd/... passes 19/19 packages; make lint reports 0 issues; the
e2e suite passes 26 specs with the read-only sweep still covering 80/80 tools.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* chore: drop migration spec set and the tool-name golden test
Removes material that should not ship in the pull request.
specs/ holds the working documents from the SDK migration (requirements, design,
plan, research comparison, skill notes). Nothing in the repository depends on
them - no CI job, Makefile target, or doc link references the path - so they were
pure review noise in the diff.
cmd/testdata/tool_names_v0.2.1.txt and cmd/tools_regression_test.go are removed
as a pair. They arrived in the same commit (ef66d5a) and are hard-coupled: the
test opens the golden file with require.NoError, so deleting the data alone fails
the build with "open testdata/tool_names_v0.2.1.txt: no such file or directory".
Removing the file alone was therefore not an option.
The dropped test asserted that tool names shipped in v0.2.1 are still registered,
which is a real check, but the golden baseline is 132 lines of v0.2.1-era data
that the reviewer flagged as not belonging in this PR. Tool registration is still
covered by TestEveryToolHasValidOutputSchema (every advertised tool must infer a
valid output schema) and by the e2e sweep, which asserts the server exposes all
80 read-only tools. The e2e comment that referenced the removed test name is
updated accordingly.
Verified: go build ./..., go vet ./... and gofmt clean; go test ./pkg/...
./internal/... ./cmd/... passes 19/19 packages; golangci-lint reports 0 issues;
the e2e suite still compiles under -tags=test. 3727 lines removed.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* fix(mcp): reap abandoned streamable HTTP sessions after the SDK migration
The migration to the official MCP SDK replaced the mcp-go transport with
sdkmcp.NewStreamableHTTPHandler and passed nil for its options. In the
official SDK, SessionTimeout is the only reaper and a zero value means
"never close", so every session registered by an initialize POST was
retained for the lifetime of the process. A POST-only client - the normal
request/response mode for this server - never sends DELETE, so heap grew
with the number of sessions ever created.
Measured before this change, using the same wiring as run(): 200
initialize requests left 200 registered sessions and they were never
reclaimed.
Wire up SessionTimeout and expose it as --session-idle-ttl (default 30m,
0 disables the reaper). The timeout is a safety net rather than a session
lifetime: each request from a client resets its timer, so only sessions
that have seen no traffic are closed.
The old mcp-go server.WithHeartbeatInterval(30s) has no direct equivalent
and is deliberately not replaced. The official transport flushes a single
": ok" SSE comment so a proxy forwards response headers promptly, but it
has no periodic comment heartbeat, and ServerOptions.KeepAlive is not a
substitute: it sends JSON-RPC pings and closes the session when they fail.
A POST-only client cannot answer those pings, so enabling KeepAlive would
evict live sessions (observed as HTTP 404 on a later request) instead of
keeping them alive. The reasoning is recorded on newStreamableHTTPHandler.
Also corrects the HTTPMiddleware doc comment, which still described the
mark3labs-era behaviour. The function is still used, but the context
values it sets are no longer read by the migrated handlers, which take
headers from req.Extra.Header via mcp.Header.
Tests (cmd/streamable_http_test.go): idle sessions are reclaimed, an
active client is not evicted, and DELETE releases a session immediately.
TestStreamableHTTPReclaimsIdleSessions was confirmed to fail against the
previous nil-options wiring and pass with the fix.
Verified: go build ./..., go vet, gofmt clean; golangci-lint 0 issues;
go test -tags=test -cover ./pkg/... ./internal/... ./cmd/... passes
16/16 packages.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* fix(k8s,kubescape): exec argv, log --previous, log scope docs, session leak
Port four verified fixes from open PRs. None applied as a patch - all were
written against the mark3labs API this branch removed - so each was re-checked
against the current code and re-implemented on the typed go-sdk path.
k8s_execute_command split its command into argv tokens (#67)
The handler passed the whole command as a single argv entry after "--", so the
container runtime looked for an executable whose name contained the spaces.
Reproduced against a live cluster:
kubectl exec pod -- "uname -a"
exec: "uname -a": executable file not found in $PATH
kubectl exec pod -- uname -a
Linux pod ... aarch64 GNU/Linux
A single-word command worked, which is why this looked like a routing failure
rather than an argv bug. The command is now tokenised, and an args array is
appended verbatim so an argument containing spaces can be passed as one token.
Fixes a second latent bug in the same handler: Container was declared in the
input struct and never used, so -c was silently ignored for multi-container pods.
k8s_get_pod_logs gained a previous flag (#79)
Adds --previous to read logs from the terminated container instance, ordered
before --tail to match kubectl conventions.
k8s_get_resources description now states its scope and image behaviour (#70)
The description was "Get Kubernetes resources using kubectl". Two things an
agent cannot infer from that: omitting both all_namespaces and namespace queries
only the tool's own namespace, not the cluster; and the wide output has an
IMAGES column for workloads but not for pods, so a pod listing is never a source
of image versions. Both produced wrong answers for the reporter. (The
all_namespaces string/boolean parsing half of #70 was already fixed by the
migration, which typed the field as bool.)
kubescape_list_vulnerability_manifests no longer reports a false count (#76)
len(manifest.Spec.Payload.Matches) was 0 for every manifest in every cluster:
the aggregated API strips spec.payload.matches on LIST and serves it only on
GET. The tool reported "vulnerability_count": 0 for images with hundreds of
CVEs, and an agent reading that data correctly concluded the cluster was clean.
The field is removed rather than corrected - an absent field cannot be mistaken
for a measured zero - and the description now says the list is an index and
points at kubescape_list_vulnerabilities for real counts.
Could not be verified against a live operator here, but the SDK corroborates the
cause: upstream ships a dedicated VulnerabilityManifestSummaries clientset whose
spec carries SeveritySummary counts and no payload, i.e. summary-for-listing is
the intended design and we were listing the full document instead.
Streamable HTTP sessions are now reclaimed (#85)
cmd/main.go passed nil options, leaving SessionTimeout at its zero value, which
disables the idle sweeper. A session never DELETEd - client crash, reset, or
POST-only - pinned its goroutine and state for the life of the process.
Measured on this branch: 1000 initialize requests without DELETE left 1000
goroutines resident, forever. SessionTimeout is now 10 minutes; with a 2s
timeout a probe reclaimed 400 abandoned sessions from 136 goroutines back to 3.
Verified: go build ./..., go vet ./... and gofmt clean; go test ./pkg/...
./internal/... ./cmd/... passes 19/19 packages; golangci-lint 0 issues; the e2e
suite compiles. New tests cover argv splitting, args verbatim, container
selection, --previous present/absent, and the omitted vulnerability_count.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* refactor(mcp): drop the SDK type aliases, use go-sdk types directly
Reviewer feedback on #66: "Do we really need to re-export these? They're
already public in the source directory." They are not needed, and the
re-exports were pure indirection - eight `type X = sdk.X` aliases plus a
`var NewServer = sdk.NewServer` that added a second name for every SDK type
a provider touches.
Provider packages now import the SDK directly as `sdkmcp`, matching the alias
already used in cmd/main.go, and keep importing internal/mcp only for what
actually carries repository behaviour:
AddTool instrumented registration (metrics + input-schema
relaxation, without which k8s_get_resources rejects
omitted optional fields)
TextOutput/TextResult/
TextError/TextOf the typed-output convention
NewToolResultText/
NewToolResultError result constructors
Header bearer-token passthrough
The removal is safe for external callers because `mcp.Server` was a type
alias, so `RegisterTools(s *mcp.Server, ...)` is the identical type as
*sdkmcp.Server - no signature changes.
The package doc now states that SDK types are used directly, so the aliases
do not creep back in.
800 call sites across 21 files. Ten files (all _test.go) only ever used the
aliases and so lose the internal/mcp import entirely.
Verified: go build ./... clean; go vet ./... clean; unit tests pass 19/19
packages; golangci-lint 0 issues. The e2e suite fails in [BeforeAll] on a
Helm deploy error into the Kind cluster - environmental, before any spec runs
(17 passed, 10 skipped) - not caused by this change.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* docs: make AGENTS.md the single guide, drop CLAUDE.md
CLAUDE.md existed only to point at AGENTS.md and add a few paragraphs. Two
guides drift, and this one already had: it named the file after one specific
agent tool, and its "Registration goes through internal/mcp, never call
sdk.AddTool" rule now reads as a contradiction of the alias removal, where
providers import the SDK directly for types while still registering through
mcp.AddTool.
AGENTS.md is the single source of truth. The one section CLAUDE.md held that
AGENTS.md lacked - how to run the server locally, and its flags - is now a
"Run Locally" subsection under Build & Test Commands, and DEVELOPMENT.md
no longer points at the deleted file.
Deliberately not ported: architecture prose duplicated in AGENTS.md Project
Overview, the logging and commit notes duplicated verbatim, and the
coverage-gate caveat AGENTS.md already carries in more detail under Testing.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
* chore(deps): bump istioctl to 1.31.1
make check-releases flagged the pin as one patch behind:
TOOLS_ISTIO_VERSION=1.31.0 != 1.31.1
The other four CLI pins (kubectl 1.37.0, helm 4.3.0, cilium-cli 0.20.0,
argo-rollouts 1.10.0) and GO_VERSION 1.27.1 are already current. The image is
rebuilt from this pin in the Dockerfile, so no vendored binary is stale.
Signed-off-by: Dmytro Rashko <dimetron@me.com>
---------
Signed-off-by: Dmytro Rashko <dmitriy.rashko@amdocs.com>
Signed-off-by: Dmytro Rashko <dimetron@me.com>
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
3 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.
What this changes
The streamable HTTP server registers an MCP session on
initializeand never released it, so heap grew with the number of sessions ever created until the pod was OOMKilled.github.com/mark3labs/mcp-gofromv0.43.2tov1.1.0, which unregisters the session inhandleDelete(mark3labs/mcp-go#724, released inv0.44.1).server.WithSessionIdleTTL, so sessions abandoned without aDELETEare reclaimed as well. It is opt-in even onv1.1.0(the default is zero, which disables it), so the dependency bump alone does not fix POST-only clients.--session-idle-ttl, default10m,0disables the sweeper.cmdpackage tests inmake test. They were excluded, so nothing incmd/was covered by CI, including the regression tests added here.Why the dependency bump alone is not enough
On
v0.43.2the session is stored inactiveSessionsand inserver.sessionsoninitialize, buthandleDeleteonly clears the per-session tool, resource, log-level and request-ID stores.UnregisterSessionis reached on theGETSSE path only, so a POST-only client, which is the normal request/response mode, retains every session it ever opened.v1.1.0fixes theDELETEpath, but a client that crashes, is restarted, or has its connection dropped by a load balancer never sends one. The sweeper is what bounds memory in that case.Verification
Eight bursts of 500
initializerequests, 15s apart, against both binaries. Live objects arego_memstats_heap_objectstaken as the minimum of 25 samples, which approximates the post-GC live set; the GC counter confirms collections ran between samples (6 at start, 15 and 34 at the end of the two runs).v0.43.2live objectsv0.43.2heapOn
v0.43.2the live set climbs with cumulative sessions and never returns. With this change it oscillates around its starting value and comes back to it: 21.7k objects and 4.09 MB after 4,000 sessions, against 20.2k and 3.90 MB before the first request.Tests added in
cmd/streamable_http_test.goassert the behavior directly rather than through heap numbers, usingserver.Hooksto observe session registration:DELETEis releasedDELETEis released by the sweeperAlso ran locally:
make build,make test-only(all packages pass),make helm-test(23 tests pass),gofmt -l .,go vet,go fix.make lintfails on this machine, but it fails identically onmainwith the same linter binary:golangci-lint v1.63.4reportsundefined: Byand similar typecheck errors for the ginkgo dot-imports intest/e2eunder a newer local Go toolchain. E2E was not run; it needs a Kind cluster.Operator impact
Default behavior changes: session state idle for more than 10 minutes is now reclaimed. A session reclaimed while its client is still alive is re-established on the next
initialize. Deployments that want a different value can set it throughtools.argsin the Helm chart, and--session-idle-ttl=0restores the previous, unbounded behavior.pprofon the metrics port, item 3 of the proposed fix in the issue, is left out to keep this change focused. Happy to open a separate PR for it.Closes #84