feat(core): add core/mcp and core/session packages#13
Merged
Conversation
Implements phase 4 of the v0.1 bootstrap (#1). Adds the multi-server MCP client wrapper and the JSON Lines session store, both built against [github.com/modelcontextprotocol/go-sdk] v1.6.0. core/mcp/ - Client manages parallel connections to one or more MCP servers via errgroup; one-shot lifecycle (Connect must be called exactly once; failure or Close latches the gate). - ServerConfig validation at construction: rejects empty/ whitespace Name, leading/trailing whitespace in Name, duplicate names, namespace-separator in Name, empty/ whitespace Endpoint, malformed HTTP URLs (url.Parse with scheme + host required), and stdio Endpoint with no command tokens. All errors wrap ErrConfig. - Headers field is now wired into HTTP-based transports via a custom http.RoundTripper that injects them on every request. Defensive copy at construction so post-construction mutation does not propagate. - Tool names namespaced as "server__tool"; SplitName / JoinName parser pair, FuzzSplitName fuzz target. - Aggregated Catalog sorted deterministically (server name, then bare tool name). Defensive copies through Catalog.copy. - Resources / Prompts / Tools use SDK iterator helpers for transparent pagination. Lock-and-release pattern so a slow listing on one server does not starve concurrent ops on others. - Call routes by SplitName, retries with bounded backoff (exponential + full jitter, default 500ms base / 30s cap / 5 attempts). Backoff.Delay clamps negatives to 0, caps shift at 31, guards math.MaxInt64 overflow. - WithDialer lets tests inject in-memory transports per-Client (no package globals) so parallel tests are race-free. - PrefixClassifier groups tools by prefix-before-first- underscore. Names with no underscore, leading underscore, or empty bare name fall into "misc". - contentToToolContent recognizes TextContent, ImageContent, AudioContent, EmbeddedResource, ResourceLink. Unknown SDK content types fall back to a JSON dump in Text with a diagnostic on marshal failure. - Stdio subprocess env inheritance documented at package level with the recommended hardening. core/session/ - Session is an append-only chat history value (intentionally not concurrency-safe; serialize at the caller). - Tokenizer interface with v1 LengthHeuristic (bytes-per-token, default 4); v1.1 may swap in a real tokenizer behind the same interface. - Truncate drops oldest non-system messages until under budget; system prompt is never dropped. Edge cases documented (single oversize message, oversize system prompt, may leave Messages empty). - Save / Load use JSON Lines: a "session.header" envelope followed by N "session.message" envelopes. Forward-compat skipping for unknown envelope types; loud-fail on missing type field. - messageEnvelope and toolCallEnvelope have explicit snake_case JSON tags so the persisted format stays interoperable; upstream provider.ToolCall (no JSON tags) never reaches the wire directly. - FuzzLoad guards the decoder against arbitrary input. Fuzz seeds use a fixed timestamp (not time.Now()) for corpus reproducibility. Build tooling - Makefile fuzz-quick uses an isolated mktemp fuzz cache so the corpus does not grow between local runs and trip the fuzz runner's deadline. FUZZTIME default raised from 5s to 15s. Per-target -timeout=120s headroom. Process Followed the Pre-commit Review Loop (PRL) under the local review-gate hook. Multiple adversarial-review rounds across two diff hashes surfaced 18 + 9 + 1 + 22 ≈ 50 findings total. Critical / real-bug findings addressed in the working tree: - Headers field documented but never wired into transports. - Backoff.Delay panicked on negative attempts. - Backoff.Delay int64+1 overflow with math.MaxInt64 cap. - Close left a stale catalog readable via Catalog(). - Failed Connect did not latch the gate (could re-attempt). - Resources/Prompts held the RLock long enough to starve concurrent ops on unrelated servers. - SDK iterator-based pagination was missing (silent truncation past page 1). - provider.ToolCall persistence used Pascal-case JSON keys. - Empty type field in a session line was silently dropped. - Fuzz seed used time.Now() (corpus not reproducible). - Missing ResourceLink case in contentToToolContent. - convertToolResult panicked on nil result. - Whitespace-only Endpoint passed New (Now also rejects whitespace in Name). - Catalog ordering non-deterministic (Tools order varied). - URL syntax not validated for HTTP transports at New. - fakeServer goroutines could call t.Logf after test end. - Backoff retry test would pass with hardcoded 0 (added growth assertion). Acknowledged design choices documented in code: - Catalog is a one-time snapshot; no live-refresh in v1. - Connect holds the write-lock for full I/O duration; ctx cancellation is the abort path. - Save is non-atomic; caller uses temp+rename. - Session is not concurrency-safe. - Call retries on every non-context error; future versions may classify jsonrpc2 codes. Verified locally - make verify PASS end-to-end - go test -race all packages green - 4 fuzz targets 15s each, 0 panics - 5-arch cross-compile all green Closes #5. Reviewed-by: pre-commit-gate (artifact .claude/.last-review.md)
…osec suppression for exec Two gosec findings on PR #13 came through GitHub Advanced Security even though `make verify` was clean locally. Root cause: the suppressions used `//nolint:gosec` (golangci-lint dialect) but the standalone gosec binary that writes the SARIF Advanced Security ingests does not honor `nolint`. Two findings: 1. G404 — math/rand/v2 used for backoff jitter. 2. G204 — exec.Command with variable argv (the stdio MCP server command line from operator config). For G404 (jitter): the right fix is not "suppress better," it's "use the better function." Switched to `crypto/rand.Int(rand.Reader, big.NewInt(int64(d)+1))`. The per-call cost is negligible against the network I/O backoff is gating, and there is no longer anything to suppress for any scanner. The math/rand and math/big imports are also cleaned up; the elaborate dual-suppression comment block is gone. For G204 (exec): there is no "better function" — exec.Command IS the API for stdio subprocesses, and argv genuinely comes from the maintainer's `ServerConfig` (trusted by design). Replaced the `//nolint:gosec` annotation with gosec's native `// #nosec G204 -- argv is operator-trusted config` syntax, which is honored by both the standalone gosec used in security.yml and by gosec running inside golangci-lint. Backoff.Delay now handles the (effectively impossible) error from rand.Int by returning the clamped delay as a safe fallback rather than panicking — `crypto/rand.Reader` is documented as always available on supported platforms, but defense in depth. Verified locally - make verify PASS end-to-end - go test -race backoff tests still green - TestBackoff_GrowsAcrossAttempts (statistical) still passes - golangci-lint 0 issues - gosec (standalone) 0 issues (verified G404 absent, G204 properly suppressed via #nosec) - semgrep 0 findings Reviewed-by: pre-commit-gate (artifact .claude/.last-review.md)
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
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.
Closes #5. Part of #1 (project bootstrap, v0.1).
This is phase 4 of 7 in the v0.1 bootstrap. It lands the multi-server MCP client wrapper and the JSON Lines session store. With this merged, every collaborator the agent loop will need (events, provider, mcp, session) is in place — phase 5 (loop / router / approval) is the next step.
Summary
core/mcp/— thin wrapper overgithub.com/modelcontextprotocol/go-sdkv1.6.0. Manages parallel connections to one or more MCP servers, aggregates the tool catalog withserver__toolnamespacing, routes tool calls back to the originating server, and retries transient failures with bounded backoff.core/session/— append-only chat history with JSON Lines persistence. Token-aware truncation behind aTokenizerinterface; v1 ships a byte-length heuristic.Makefilefuzz-quicknow uses an isolatedmktempfuzz cache so the corpus does not grow between local runs and trip the fuzz runner's deadline.FUZZTIMEraised from 5s to 15s.Phase mapping
feat/bootstrap-skeletonfeat/bootstrap-cicore/event+core/providerfeat/bootstrap-event-providercore/mcp+core/session(this PR)feat/bootstrap-mcp-sessioncore/loop+core/router+core/approvalfeat/bootstrap-loop-router-approvalcmd/askCLIfeat/bootstrap-askexamples/acme-revenue+ ADR + architecture docfeat/bootstrap-acme-exampleWhat lands
core/mcp/client.go(742 lines)Client: parallel connections to N MCP servers viaerrgroup. One-shot lifecycle —Connectmust be called exactly once. Failure orCloselatches the gate; constructing a newClientis the only way to retry.ServerConfigvalidation at construction: rejects empty/whitespaceName, leading/trailing whitespace inName, duplicate names, namespace separator inName, empty/whitespaceEndpoint, malformed HTTP URLs (url.Parsewith scheme + host required), and stdioEndpointwith no command tokens. Every error wrapsErrConfig.Headersfield is wired: HTTP-based transports get a customhttp.RoundTripperthat injects the configured headers on every outbound request. Defensive copy at construction so caller-side mutation does not propagate.server__toolseparator (__).SplitName/JoinNameparser pair, exhaustively fuzz-tested.Catalog: sorted deterministically (server name, then bare tool name) so twoConnectcalls produce identicalToolsordering even though the underlying sessions map is randomized. Defensive copies throughCatalog.copy.Resources,Prompts, and the catalog fetch use the SDK's iterator helpers (sess.Resources/Prompts/Toolsreturningiter.Seq2) so multi-page servers are fully enumerated. Lock-and-release pattern: the client lock is held only long enough to copy the session pointer, then released for iteration. A slow listing on one server cannot starve concurrent operations on other servers.Call: routes bySplitName; retries on every non-context error with bounded backoff (exponential + full jitter, default 500ms base / 30s cap / 5 attempts). ReturnsErrServerUnavailableon exhaustion. Tool-level errors flow throughToolResult.IsError, not Go errors.Backoff.Delay: clamps negative attempts to 0; caps the shift at 31 to avoid wraparound; clampsmath.MaxInt64soint64(d)+1doesn't overflow intoMinInt64.WithDialeroption lets tests inject in-memory transports per-Client(no package globals) so parallel tests are race-free.contentToToolContent: recognizesTextContent,ImageContent,AudioContent,EmbeddedResource,ResourceLink. Unknown SDK content types fall back to a JSON dump inTextwith a diagnostic on marshal failure.ErrConfig,ErrUnknownServer,ErrServerUnavailable,ErrInvalidName.core/mcp/catalog.go(93 lines)Catalogstruct with flatToolsslice andToolsByServermap.Toolkitgrouping with a pluggableToolkitClassifier.PrefixClassifiergroups by the substring before the first underscore in the bare name (the Plexaradatahub_*/trino_*/s3_*/memory_*pattern). Names with no underscore, leading underscore (_foo), or empty bare name fall intomisc.core/session/session.go(324 lines)Sessionis an append-only chat history value withID,Created,Updated, and[]provider.Message. Intentionally not concurrency-safe (caller serializes).Tokenizerinterface;LengthHeuristicships as the v1 implementation (bytes / 4 by default). Replaceable for v1.1.Truncatedrops oldest non-system messages until undermaxTokens. The system prompt is never dropped, even when it alone exceeds the budget. A single oversize non-system message is dropped along with everything before it; the session may be left empty.Save/Loaduse JSON Lines: asession.headerenvelope followed by Nsession.messageenvelopes. Forward-compat skipping for unknown envelope types; loud-fail (ErrFormat) on missingtypefield.messageEnvelopeandtoolCallEnvelopecarry explicit snake_case JSON tags so the persisted format stays interoperable. Upstreamprovider.ToolCall(no JSON tags) never reaches the wire directly.Tests (1247 lines across 4 files)
core/mcp/client_test.go(730 lines, 17+ tests) — uses an in-memorysdkmcp.Serverwired throughWithDialerfor hermetic testing. Covers:SplitName/JoinNameround-trip, allNewvalidation paths, parallelConnectaggregation,Callrouting, unknown server, invalid name, parallel-connect-failure cleanup, backoff bounds and growth, post-CloseErrUnknownServer, twice-Connectrejection, failed-Connectlatching, deterministic catalog ordering, multi-page pagination (server withPageSize: 1), defensive catalog copy.core/mcp/catalog_test.go—PrefixClassifiercases, custom classifiers, empty catalogs, leading-underscore names.core/mcp/fuzz_test.go—FuzzSplitNameround-trip; useserrors.Is(err, ErrInvalidName)for sentinel checks.core/session/session_test.go(325 lines, 13 tests) —New,Appendupdates timestamp, fullSave/Loadround-trip, error cases, unknown envelope skip, missing-typerejection, snake-case wire format pin, blank-line tolerance,LengthHeuristicmath, allTruncateedge cases.core/session/fuzz_test.go—FuzzLoaddecoder fuzz with reproducible (fixed-timestamp) seeds.Local quality gate
make verify(full gate)go test -race -shuffle=on -count=1 ./...core/eventcoveragecore/mcpcoveragecore/providercoveragecore/sessioncoveragegolangci-lint run ./...govulncheckProcess — Pre-commit Review Loop (PRL)
Followed the local
review-gatehook on this commit. The hook ran an adversarial sub-agent review across two diff hashes (the hash flips when fixes change the diff, restarting the round counter). Across all rounds, ~50 findings were surfaced. Every critical bug was fixed in the working tree before commit. Documented design choices were left in place with explicit code comments. The commit body enumerates every addressed and every deferred finding for traceability.Critical bugs caught and fixed
Headersfield was documented but never wired into HTTP transports — silent auth-token drop. Now flows through a customhttp.RoundTripper.Backoff.Delaypanicked on negative attempts (runtime error: negative shift amount).Backoff.Delaycould overflowint64(d) + 1whenCap = math.MaxInt64, causingrand.Int64Nto panic.Closecleared sessions but left a stale catalog readable viaCatalog().Connectdid not latch the gate; a retry could partially re-initialize.Resources/Promptsheld the read-lock for full iteration → starved concurrent ops on unrelated servers.provider.ToolCallpersistence used Pascal-case JSON keys (no JSON tags upstream) — wire format would not interop with any other reader.typefield was silently dropped onLoad.time.Now()— corpus was non-reproducible.ResourceLinkincontentToToolContent.convertToolResultpanicked on nil result.EndpointpassedNew; whitespace inNamealso now rejected.Connectcalls.EndpointURL syntax not validated atNew(nowurl.Parsewith scheme + host required).fakeServergoroutines could callt.Logfafter test end.Acknowledged design choices, documented in code
Catalogis a one-time snapshot atConnecttime; no live-refresh ontools/list_changednotifications in v1.Connectholds the write-lock for full I/O duration; ctx cancellation is the abort path.Saveis non-atomic; callers do temp+rename.Sessionis not concurrency-safe; caller serializes.Callretries on every non-context error in v1; future versions may classify jsonrpc2 codes (-32601Method not found,-32602Invalid params) as non-retryable.Out of scope (deferred to later phases)
core/loopagent loop andcore/routertoolkit classifier (phase 5)core/approval(phase 5)/tokenize(v0.3)tools/list_changednotification handling (post-v1)Test plan
lintpasses:gofmt,go vet,golangci-lint,golangci-lint config verify,go mod verify,go mod tidy -difftest (ubuntu-latest)andtest (macos-latest)green; coverage upload with thecoreflag (best-effort while Codecov registration is pending — see chore(ci): re-enable codecov fail_ci_if_error after Codecov registration #12)buildmatrix green acrossdarwin/{amd64,arm64},linux/{amd64,arm64},windows/amd64gosecclean;semgrepclean;govulncheckclean;trivy fscleananalyze go(CodeQL) cleandependency-reviewreports the new transitive deps (github.com/modelcontextprotocol/go-sdk,github.com/google/jsonschema-go,golang.org/x/oauth2, etc.); confirm all licenses are within the allowlist (Apache-2.0, BSD-2/3-Clause, MIT, etc.)conventional commitPR title check passes (feat(core): ...)ci passaggregator greenProvider,Client, andSessionsurfaces