fix: implement MCP Streamable HTTP transport for memory client by jflowers · Pull Request #71 · unbound-force/replicator

jflowers · 2026-08-11T19:59:56Z

Summary

Fixes #19. memory.Client.Call() was sending bare http.Post() with plain JSON-RPC to Dewey's MCP endpoint, causing HTTP 400 on every call. All memory proxy tools (hivemind_store, hivemind_find, dewey_health) failed with DEWEY_UNAVAILABLE.

This PR extracts a shared internal/mcpclient/ package that correctly speaks MCP Streamable HTTP:

initialize handshake with protocol version and client identity
Mcp-Session-Id capture and reuse
tools/call envelope wrapping
Dual-format response parsing (SSE + plain JSON)
Session recovery on HTTP 400/404 (retry once)
Thread-safe via sync.Mutex + atomic.Int64

Both memory.Client and doctor.deweyHealthProbe() now delegate to mcpclient.Client, eliminating ~70 lines of duplicated inline MCP code from the doctor.

How to Test

make check           # all tests pass with -race -count=1
make check-coverage  # all coverage ratchets pass

Key test scenarios (22 tests in mcpclient, 16 in memory, 8 in doctor):

Initialize + tools/call happy path
Session reuse across calls
Session recovery on HTTP 400 and 404
Concurrent initialization (10 goroutines, -race)
SSE and plain JSON response parsing
Edge cases: empty body, malformed JSON, no data line, empty content array
Regression: bare JSON-RPC rejected by MCP endpoint (TestCall_RejectsBareMethods)

How to Demo

Start Dewey MCP server locally
Run replicator serve and invoke hivemind_store or hivemind_find via MCP
Observe successful responses instead of DEWEY_UNAVAILABLE errors
Run replicator doctor and observe the Dewey health check passes

Key Files Changed

File	Change
`internal/mcpclient/client.go`	New shared MCP Streamable HTTP client (340 lines)
`internal/mcpclient/client_test.go`	22 tests covering all 17 spec scenarios (836 lines)
`internal/memory/proxy.go`	Rewrote `Call()` to delegate to `mcpclient.Client`
`internal/memory/proxy_test.go`	Rewrote tests with MCP-compatible mock handlers
`internal/doctor/checks.go`	Simplified `deweyHealthProbe()` from ~70 to 7 lines
`internal/doctor/checks_test.go`	Updated mock to handle MCP protocol
`AGENTS.md` / `README.md`	Added `mcpclient/` to project structure
`openspec/changes/fix-memory-client-mcp-transport/`	Spec artifacts (proposal, design, specs, tasks)

Known Issues

The following findings from the review council were acknowledged but not resolved:

LOW: data: without space branch in SSE parsing not directly tested (covered by production code path)
LOW: Logger content not verified in tests (count-based assertion covers lifecycle)
LOW: Hardcoded Version "1.0.0" doesn't track binary version (informational, not functional)
LOW: No context.Context support (documented non-goal for this change)
LOW: Pre-existing filepath.Join issue in doctor (not introduced by this change)
LOW: Concurrent recovery may trigger redundant re-init (safe, not a correctness issue)

This PR was generated by /uf.finale (AI-assisted).

Fixes unbound-force#19 — memory.Client.Call() does not speak MCP Streamable HTTP. Artifacts: - proposal.md: motivation, capabilities, impact, constitution alignment - design.md: 7 design decisions (mcpclient package, lazy init, concurrency safety, dual-format parsing, configurable identity/timeout, logging) - specs/mcp-transport.md: 9 ADDED requirements with 18 Given/When/Then scenarios, coverage strategy table - tasks.md: 4 task groups with TDD ordering Review council findings addressed: - Thread safety for shared session state (CRITICAL) - Package naming: mcpclient/ not mcphttp/ to avoid mcp/ collision - Dual-format response parsing (SSE + plain JSON) - SSE edge cases (empty body, malformed JSON, no data line) - Initialize failure scenarios - Concurrency safety scenarios - Configurable client identity and timeout - Observability via optional structured logging - Coverage strategy with 17 required test paths

Extract shared MCP client into internal/mcpclient/ package with: - MCP session lifecycle (initialize handshake + Mcp-Session-Id) - tools/call envelope wrapping (transparent to callers) - Dual-format response parsing (SSE and plain JSON) - Session recovery on HTTP 400/404 (reset + retry once) - Thread-safe via sync.Mutex, 87.2% test coverage Rewrite memory.Client to delegate to mcpclient.Client. Migrate doctor's deweyHealthProbe() to use shared client. All existing tests adapted with MCP-aware mock handlers. Fixes unbound-force#19

- Replace strings.NewReader(string(body)) with bytes.NewReader(body) - Add explicit Content-Type check before SSE parsing fallthrough - Remove unused newMCPServer allocation in test

- MCP Streamable HTTP transport pattern - mcpclient shared architecture decisions - Review council spec review patterns Assisted-by: claude-opus-4-6 Generated with AI assistance (claude-opus-4-6)

yvonnedevlinrh

Review: fix: implement MCP Streamable HTTP transport for memory client

Verdict: REQUEST CHANGES — 4 findings require attention (2 mechanical, 2 logic). The core design is sound and all acceptance criteria from #19 are met.

What works well

Clean extraction of internal/mcpclient/ — 340 lines of production code, 836 lines of tests, 20+ scenarios
Correct MCP session lifecycle: initialize → Mcp-Session-Id → tools/call
wrapError() bridge preserves backward compatibility for all existing errors.As callers
Dual-format SSE + JSON response parsing is spec-compliant
deweyHealthProbe() reduced from ~60 lines to 6 by delegating to the new client
All 8 production callers verified compatible — no semantic breakage

Findings requiring changes

#	Severity	Category	Summary
1	MEDIUM	Correctness	Non-`UnavailableError` responses bypass graceful degradation
2	MEDIUM	Governance	Missing CI coverage ratchet for `internal/mcpclient/`
3	MEDIUM	Concurrency	Session recovery TOCTOU window between unlock and retry
4	MEDIUM	Convention	Import ordering violation (`bytes` after `net/http`)

Informational findings (no changes required)

#	Severity	Summary
5	LOW	Hardcoded `"1.0.0"` version with no link to binary version
6	LOW	`UnavailableError` missing `Is()` method — fine since all callers use `errors.As`
7	LOW	Test mock doesn't validate `Mcp-Session-Id` matches issued session
8	LOW	`parseResponse` treats empty Content-Type as SSE without documenting the behavior

Caller impact

All production call sites traced and verified:

tools/memory/tools.go — hivemindStore, hivemindFind (use errors.As ✓)
memory/proxy.go — Health(), Store(), Find() (delegate to Call ✓)
doctor/checks.go — deweyHealthProbe (treats any error as warn ✓)
cmd/replicator/serve.go, docs.go — NewClient constructor (no API change ✓)

Reachability

Session recovery 400/404 gate: correctly reachable only on HTTP-level errors, never on JSON-RPC errors inside a 200. ✓
sessionID != "" guard: reachable when server omits Mcp-Session-Id header. Tested. ✓
"data:" without space: unreachable from known servers but correct per SSE spec. Defensive code. ✓
doToolsCall marshal/request-creation errors: practically unreachable — defensive guards. ✓

Finding 2 — Missing CI coverage ratchet: internal/mcpclient/ has no entry in the THRESHOLDS array in .github/workflows/ci.yml. The spec (mcp-transport.md) requires ≥80% line coverage. Add ["internal/mcpclient"]=80 to the ratchet step. This file is not in the PR diff, so the author should include it as a follow-up commit.

@yvonnedevlinrh

…x imports - Wrap parseToolsCallResponse errors as *UnavailableError so they propagate correctly through memory.wrapError() (errors.As match) - Hold mutex across session recovery retry to prevent concurrent goroutines from triggering redundant re-initialization (TOCTOU fix) - Fix stdlib import ordering: move bytes before encoding/json (CS-002) - Add comment documenting intentional empty Content-Type fallthrough Addresses PR unbound-force#71 review feedback from @yvonnedevlinrh. Signed-off-by: Jay Flowers <jay@unboundforce.com> Assisted-by: claude-opus-4-6

@yvonnedevlinrh

- Add Mcp-Session-Id request header validation in mock handler to verify clients send the correct session ID (TC-009) - Add clarifying comment on proxy.go version constant noting it is the MCP protocol version, not the binary version Addresses PR unbound-force#71 review feedback from @yvonnedevlinrh. Signed-off-by: Jay Flowers <jay@unboundforce.com> Assisted-by: claude-opus-4-6

@yvonnedevlinrh

Add internal/mcpclient to the THRESHOLDS array with 80% minimum line coverage, matching the spec requirement for new packages. Addresses PR unbound-force#71 review feedback from @yvonnedevlinrh. Signed-off-by: Jay Flowers <jay@unboundforce.com> Assisted-by: claude-opus-4-6

Address review-council findings: - Wrap marshal/request-creation errors in UnavailableError for consistency - Fix misleading version comment in proxy.go (client version, not protocol) - Remove unused initResponse/toolResponse mock fields Addresses PR unbound-force#71 review feedback. Signed-off-by: Jay Flowers <jay@unbound.force> Assisted-by: claude-opus-4-6

jflowers · 2026-08-12T14:09:07Z

Re: Finding 2 (CI coverage ratchet) — Addressed in 15d9d95: added ["internal/mcpclient"]=80 to the THRESHOLDS array in .github/workflows/ci.yml. Current coverage is 86.6%, exceeding the 80% ratchet per Constitution IV.

Add test for UnavailableError.Unwrap() method which was at 0% coverage, causing CI function-average coverage to drop to 79.3% (below 80% threshold). With this test, Unwrap() is at 100% and the function-average is ~91.8%. Addresses PR unbound-force#71 CI failure in "Enforce Coverage Ratchets" step. Signed-off-by: Jay Flowers <jay@unbound.force> Assisted-by: claude-opus-4-6

yvonnedevlinrh

Looks good

@yvonnedevlinrh

…x imports - Wrap parseToolsCallResponse errors as *UnavailableError so they propagate correctly through memory.wrapError() (errors.As match) - Hold mutex across session recovery retry to prevent concurrent goroutines from triggering redundant re-initialization (TOCTOU fix) - Fix stdlib import ordering: move bytes before encoding/json (CS-002) - Add comment documenting intentional empty Content-Type fallthrough Addresses PR #71 review feedback from @yvonnedevlinrh. Signed-off-by: Jay Flowers <jay@unboundforce.com> Assisted-by: claude-opus-4-6

@yvonnedevlinrh

- Add Mcp-Session-Id request header validation in mock handler to verify clients send the correct session ID (TC-009) - Add clarifying comment on proxy.go version constant noting it is the MCP protocol version, not the binary version Addresses PR #71 review feedback from @yvonnedevlinrh. Signed-off-by: Jay Flowers <jay@unboundforce.com> Assisted-by: claude-opus-4-6

@yvonnedevlinrh

Add internal/mcpclient to the THRESHOLDS array with 80% minimum line coverage, matching the spec requirement for new packages. Addresses PR #71 review feedback from @yvonnedevlinrh. Signed-off-by: Jay Flowers <jay@unboundforce.com> Assisted-by: claude-opus-4-6

Address review-council findings: - Wrap marshal/request-creation errors in UnavailableError for consistency - Fix misleading version comment in proxy.go (client version, not protocol) - Remove unused initResponse/toolResponse mock fields Addresses PR #71 review feedback. Signed-off-by: Jay Flowers <jay@unbound.force> Assisted-by: claude-opus-4-6

jflowers requested a review from a team as a code owner August 11, 2026 19:59

jflowers self-assigned this Aug 11, 2026

jflowers added the next-release label Aug 11, 2026

jflowers added this to Unbound Force Planning Aug 11, 2026

jflowers moved this to Ready for Review 👀 in Unbound Force Planning Aug 11, 2026

jflowers added 5 commits August 12, 2026 11:16

fix: address review council findings in mcpclient

c20a5ca

- Replace strings.NewReader(string(body)) with bytes.NewReader(body) - Add explicit Content-Type check before SSE parsing fallthrough - Remove unused newMCPServer allocation in test

docs: mark code-review passed in tasks.md

90cdd65

chore: store retrospective learnings in Dewey

cdff0af

- MCP Streamable HTTP transport pattern - mcpclient shared architecture decisions - Review council spec review patterns Assisted-by: claude-opus-4-6 Generated with AI assistance (claude-opus-4-6)

yvonnedevlinrh force-pushed the opsx/fix-memory-client-mcp-transport branch from cf2bc42 to cdff0af Compare August 12, 2026 10:17

yvonnedevlinrh requested changes Aug 12, 2026

View reviewed changes

Comment thread internal/mcpclient/client.go Outdated

Comment thread internal/mcpclient/client.go

Comment thread internal/mcpclient/client.go

Comment thread internal/memory/proxy.go

Comment thread internal/mcpclient/client.go

Comment thread internal/mcpclient/client_test.go

jflowers moved this from Ready for Review 👀 to In Review 🏁 in Unbound Force Planning Aug 12, 2026

jflowers added 4 commits August 12, 2026 09:37

jflowers requested a review from yvonnedevlinrh August 12, 2026 14:15

jflowers moved this from In Review 🏁 to Ready for Review 👀 in Unbound Force Planning Aug 12, 2026

yvonnedevlinrh approved these changes Aug 12, 2026

View reviewed changes

jflowers merged commit b8103ef into unbound-force:main Aug 12, 2026
1 check passed

Navigation Menu

Uh oh!

fix: implement MCP Streamable HTTP transport for memory client - #71

fix: implement MCP Streamable HTTP transport for memory client#71
jflowers merged 10 commits into
unbound-force:mainfrom
jflowers:opsx/fix-memory-client-mcp-transport

jflowers commented Aug 11, 2026

Uh oh!

yvonnedevlinrh left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jflowers commented Aug 12, 2026

Uh oh!

yvonnedevlinrh left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jflowers commented Aug 11, 2026

Summary

How to Test

How to Demo

Key Files Changed

Known Issues

Uh oh!

yvonnedevlinrh left a comment

Choose a reason for hiding this comment

Review: fix: implement MCP Streamable HTTP transport for memory client

What works well

Findings requiring changes

Informational findings (no changes required)

Caller impact

Reachability

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jflowers commented Aug 12, 2026

Uh oh!

yvonnedevlinrh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants