2026-05-22
Broker MCP Gateway — Slice 6 (conflict-aware merge in mark_publish)
Branch: feat-tools-broker-mcp-gateway-merge. Slice 4b+5 (PR #149) merged onto main as fa9f86d.
Lit up the on_conflict="merge" candidate flow in mark_publish. The broker now has full semantic parity with the local demarkus-mcp's publish surface (modulo the documented ephemeral graph-store gap from Slice 4b). After Slice 6: only Slice 7 (chart + RBAC + docs) and Slice 8 (/knowledge-join plugin slash command) remain before the broker MCP gateway plan is done.
Decisions made this session that weren't in the plan
brokerMergeAdaptercapturesctxin the struct. Normally a code smell, butmerge.Client's interface (FetchVersion / FetchCurrent / Publish) doesn't take ctx — the package was designed againstfetch.Clientwhich also doesn't take ctx. The adapter's lifetime is bounded by onehandleMarkPublishcall, so capturing the handler's ctx in the struct is the load-bearing alternative to losing ctx.Done propagation across the 3-step orchestration. Same shape the local demarkus-mcp'smergeClientAdapteruses against itsmarkClientinterface.- Default
on_conflictflipped to"merge"to match local demarkus-mcp. Slice 3's default was "fail" because the merge code didn't exist yet; Slice 6 flips it. The shift means agents callingmark_publishwithout specifyingon_conflictnow get the safety-net merge-candidate flow on conflict instead of a raw conflict envelope. Aligns with the local server's "convenience that hides necessary complexity creates bugs" thought from/thoughts.md— merge as default is the safer footgun-free choice. optionalIntMeta(vs the localoptionalInt) to avoid name collision. The broker package may grow a future sharedoptionalInthelper used by other handlers; naming this meta-specific avoids reserving the simpler name unnecessarily. Same behavior as the local server's helper.formatMergeOutcomeis duplicated, not hoisted. Same proxy-fidelity story asformatToolResultfrom Slice 2: 25 LOC, stable, byte-for-byte parity with the local server'sformatOutcome. If they ever drift, the existing proxy-fidelity reference test inmcp_tools_read_test.gocatches theformatToolResultside; the merge-specific format isn't covered by that test (yet — could be extended), but the function is simple enough that drift would be visible immediately.
Real bugs surfaced during testing
- Append + auto-resolve happy-path test had
"server-version": "1.0"in fixture data. Worked under Slice 3 because the "fail" branch forwarded the metadata verbatim without parsing. Slice 6's merge path runsoptionalIntMetaover server-version, whichstrconv.Atoi-rejects floats. Fixed the fixture to"1"(matches actual server output). The other append-side tests using "1.0" still work because mark_append's success path doesn't parse server-version — only mark_publish via merge. TestHandleMarkPublishConflictPassesThroughVerbatimhad to be renamed + opt intoon_conflict="fail". Previously asserted the default behavior; now the verbatim-conflict semantics only apply to the explicit "fail" branch since "merge" is default. Renamed toTestHandleMarkPublishFailConflictForwardsVerbatimto make the intent obvious. The test still exists and pins the still-supported opt-out behavior.TestHandleMarkPublishOnConflictMergeRejectedUntilSlice6deleted outright — the property it pinned (merge rejection) no longer applies. Replaced byTestHandleMarkPublishMergeCleanOutcomeOK+TestHandleMarkPublishDefaultOnConflictIsMerge+ 5 other new tests covering the merge surface end to end.- First version of the candidate-without-markers test had the wrong base/ours/theirs triple. Both sides appended a different line after a shared base line — Diff3 flagged that as overlapping because both ended the file with a new value. Rewrote the test with truly disjoint edits (ours edits line 1, theirs edits line 4 of a 4-line document) so Diff3 produces the clean structural merge. Worth pinning in the test comment: line-level disjoint at the ENDS of the file looks the same as overlapping edits to Diff3.
Scope outcome vs plan estimate
- Production code: +163 LOC in
mcp_tools_write.go(brokerMergeAdapter + 3 methods + helpers + merge branch + formatMergeOutcome). Plan estimate ~150 LOC. ✅ - Test code: +375 LOC in
mcp_tools_write_test.go(3 deleted/replaced, 7 new). Plan estimate ~250 LOC. Over by ~50% but covers real distinct semantics — every test pins one explicit invariant (clean OK, no-markers, markers, auth-retry inheritance via shared tokens, default-flip, whitespace-normalization, base-version-zero). Worth it. go test -race ./...green across all 4 modules.pre-commit.sh(fmt + vet + golangci-lint × 4) green.
Surface status after Slice 6
All 13 broker MCP tools have full semantic parity with the local demarkus-mcp:
- 6 verbs (fetch/list/versions/publish/append/archive) with proxy fidelity + auth-race retry.
- 2 federation reads (discover/resolve).
- 5 graph-store tools (backlinks/graph/index/graph_export/graph_publish) backed by an ephemeral per-pod store.
The only documented behavioral gap is the graph-store ephemerality (re-crawl after broker restart). Everything else — including the merge-candidate envelope — is byte-for-byte indistinguishable between brokered and direct-QUIC access.
Next session — Slice 7 starting point
Slice 7 is the chart, RBAC, docs work. Per the plan:
deploy/helm/demarkus-broker/values.yaml—server.mcp.addr,server.mcp.tls.existingSecretRef,server.mcp.sessionMaxIdle,server.mcp.worldTokenTTL,server.mcp.worldPool.worlds[].internalAddressfor non-default Service DNS overrides.- Chart templates: deployment.yaml (new containerPort + TLS mount), service.yaml (gateway port), ingress.yaml (route mcp host to new port), networkpolicy.yaml (allow ingress on mcp port).
deploy/helm/demarkus-broker/README.md— "MCP gateway" section. TLS setup, plugin flow, OAuth flow, rate-limit behavior, ephemeral graph-store note.tools/demarkus-broker/MCP-API.md— operator/developer-facing spec for the 13-tool MCP surface.- Upgrade note in chart README: pre-gateway deployments pick up the listener on
:8081after the chart bump.
This is the slice that unlocks the kind-harness sanity testing for the MCP gateway — the chart changes are the unlock. Once Slice 7 ships, the deploy/kind harness can grow a --with-mcp-smoke stage that drives /mcp via curl + JSON-RPC against a real demarkus-server world. That's the end-to-end "Done When" criterion from the plan.
Slice 8 (/knowledge-join plugin slash command) is small (~50 LOC + tests) and lands last.
Broker MCP Gateway — Slice 7 (chart + RBAC + docs)
Branch: feat-deploy-broker-mcp-gateway-chart. Slice 6 (PR #150) merged onto main as 80a4008.
Slice 7 catches the chart up to the gateway code that landed across Slices 1-6. No production-code changes — the broker binary already had MCPConfig (Addr, TLS, SessionMaxIdle, MaxSessions, FirstMint*) and WorldConfig.InternalAddress from earlier slices. This slice plumbs all of that through values.yaml, the rendered config Secret, the Deployment / Service / NetworkPolicy / Ingress templates, plus a new MCP gateway section in the chart README and a new tools/demarkus-broker/MCP-API.md operator/dev reference.
Decisions made this session that weren't in the plan
-
Q1 — Ingress topology: separate hostname. Plan said "route MCP host/path to the new port" without nailing whether to share the existing host with path-routing or split into two hosts. Chose split:
ingress.mcp.hostis a parallel knob toingress.host, and the chart emits a single Ingress resource with two rules + (optionally) two TLS blocks. Reason: the management API and the MCP gateway each have their OWN.well-known/*surfaces — OIDC discovery on the management side (openid-configuration,jwks.json) and OAuth metadata on the MCP side (oauth-protected-resource,oauth-authorization-server). Same-host path-routing would create a fragile precedence ladder the first time either side grew a new.well-known/*endpoint. Two hostnames = zero collision risk + independent cert rotation. The OCI 8414 metadata aliasing the OIDC Discovery handler (per Slice 1's plan note) is a same-listener choice; the ingress topology is orthogonal. -
Q2 — Chart version bump: cosmetic only. Chart.yaml bumped 0.1.0 → 0.2.0 (version + appVersion in lockstep). Discovered the release pipeline at
.github/workflows/release.yml:520overrides both fields at package time with${{ needs.semver-tools.outputs.new_version }}— the in-tree value is documentation, not load-bearing. Latest publishedtools/v*tag istools/v0.1.15, so the kind harnessBROKER_CHART_VERSIONgot the matching bump (0.1.3 → 0.1.15). The 0.2.0 in Chart.yaml signals "Slice 7 ships substantive chart support" but the next published version will be whatever conventional-commits gives — probably 0.1.16, not 0.2.0. Worth flagging when reviewing: ignore the in-tree number. -
Q3 — Broker-side TLS supported but Ingress-terminated is recommended.
server.mcp.tls.existingSecretRef.nameis the chart's ONE TLS mode — pointing at a pre-existingkubernetes.io/tlsSecret. Chart mounts read-only at/etc/demarkus-broker/tls/mcp/and renderscertFile/keyFilepaths into the broker's config. No in-line PEM mode (cleartext private keys in helm release history are never acceptable). README documents Ingress-terminated as the default + recommended path; broker-terminated is for mTLS broker↔Ingress topologies or Ingress-bypass deployments. -
Q4 —
--with-mcp-smokeships in Slice 7 but scoped down. Originally proposed the full id_token + initialize + tools/call flow. Scoped down to three lightweight checks: RFC 9728 metadata fetch, RFC 8414 metadata fetch, POST /mcp without auth → 401 + WWW-Authenticate. Reason: the full id_token dance requires device-flow + refresh-grant orchestration in shell, which is itself ~100 LOC and is the natural test surface for Slice 8's/knowledge-joinslash command. The three checks prove what Slice 7 actually needs to prove — the chart's MCP listener binds, OAuth metadata renders, auth gate fires. Anything more is testing the binary, not the chart. -
worldTokenTTLknob from the plan is NOT in values.yaml because the broker binary doesn't actually have aWorldTokenTTLfield — the only mention is a doc-comment on MCPConfig. Slice 2's implementation went with "natural expiry fromIssuer.MintFiltered" (plan OQ#5 lean answer) and never plumbed the override. Adding a chart knob the broker silently ignores is exactly the "Question opt-in knobs from plans" footgun from auto-memory. Replaced with a values.yaml comment pointing operators atworlds[].defaultToken.expiresAfterinstead. Plan vs code drift, captured here. -
mcpPorthelper extracts the port fromserver.mcp.addrso Deployment / Service / NetworkPolicy / config-render all use one source of truth. The alternative — surfacing bothaddr(string) andport(number) as separate values — would invite drift. The helper fails template render with a clear message if addr lacks a parseable port, better than renderingcontainerPort: 0and crashing on bind. -
Smoke checks deferred to Slice 8 are listed in code comments so the next person doesn't replicate scope expansion. The kind smoke proves chart-wiring; full E2E proves binary semantics — different proofs, different slices.
Plan vs implementation drift discovered
WorldTokenTTLis a plan field that never landed (above).- The plan also listed
server.mcp.worldPoolfor Slice 7's values surface. There is noMCPConfig.WorldPoolsubstruct in the binary — the worldPool's lifecycle is internal toServer.MCPGateway(), not configurable. Dropped from values.yaml on the same "no knob for a missing field" principle.
Bugs found during testing
- Initial cert-manager Certificate test asserted
containsDocumentagainst the same template that already renders two----separated docs. helm-unittest'scontainsDocumentis per-document-index, not file-wide. Two assertions targeting documentIndex 0 and 1 of the same render kept failing in confusing ways. Split into separateit:cases: one assertshasDocuments: count 2when both hosts are cert-manager, another asserts the full Certificate shape against an MCP-only render (single document, default index). Cleaner test intent, no documentIndex juggling. - The
set: server.mcp.addr: ""test case was unreachable — helm-unittest treats empty-string set values as "no override," so the values.yaml default:8081always won. The fail-fast guard for blank addr is still real (verified manually viahelm template --set server.mcp.addr=), just not unit-testable via helm-unittest. Removed the assert; the manual verification + the in-helperfailmessage are sufficient defense. server.mcp.addrvsserver.portcollision check usedeqon incompatible types. First version compared a string (:8081) to an int (8080) —eqalways false, guard never fired. Fixed by stringifying server.port toprintf ":%d"before comparison.- Shell-script smoke embedded an apostrophe inside a single-quoted
sh -c '...'. "the resource server's identity" / "the broker's OIDC handler" — bash parser tracked it as unmatched-quote EOF. Reworded both comments to drop apostrophes. Lesson: when writing shell-inside-shell, write smoke text without contractions or use heredoc-quoted (<<'EOF') rather thansh -c '...'. - Pre-existing
deployment_test.yamlassertion pinned:0.1.0image tag. Chart.yaml bump to 0.2.0 cascaded — updated the test to match. Existing chart pins like this are an argument for using.Chart.AppVersionin tests via match-anything-version regex instead of pinning a literal, but that's a sweep for later.
Scope outcome vs plan estimate
| Surface | Plan | Actual |
|---|---|---|
| Chart code (values + templates + helpers) | ~50 LOC | ~190 LOC |
| Helm-unittest cases | ~80 LOC | ~245 LOC (24 new cases) |
| README + MCP-API.md | ~300 LOC | ~410 LOC |
--with-mcp-smoke (deploy/kind/up.sh) |
optional | ~85 LOC |
Over the original ~430-LOC budget but well-explained: the cross-template helper + ingress-topology shape + parallel cert-manager Certificate + 3-check smoke each pulled in real lines. The 24 new unit-test cases pin one invariant each (default render, override flow-through, TLS volume mount, MCP port admission, two-host ingress, parallel Certificate, fail-render guards).
Test/lint posture: helm unittest deploy/helm/demarkus-broker → 9 suites, 95 tests, all green. bash pre-commit.sh → fmt + vet + golangci-lint × 4 modules, all green.
Status after this slice
- 7 of 8 slices shipped. Plan's only remaining slice is Slice 8 (
/knowledge-joinplugin slash command + plugin docs). Slice 8 is the user-facing onboarding closer and is also the natural place for the full id_token / initialize / tools/call E2E that Slice 7's smoke deliberately deferred. - The kind harness now has a
--with-mcp-smokeflag that proves the chart's MCP listener wiring against a locally-built broker image. Three checks (metadata × 2, auth challenge × 1) — fast feedback loop for future chart changes. - Chart README + MCP-API.md give operators and plugin developers a single place to read the gateway's contract. The ephemeral graph-store gap is documented prominently (operators should expect re-crawl after restart).
Next session — Slice 8 starting point
feat-plugin-claude-code-knowledge-joinbranch (or similar).plugins/claude-code/commands/knowledge-join.md— prompt-shaped slash command that takes a broker URL.- Validate via
HEAD <url>/.well-known/oauth-protected-resource(the metadata endpoint Slice 7 just stood up). - Derive slug from hostname, run
claude mcp add --transport http {slug} {url}/mcp. - Bump
plugins/claude-code/scripts/lib.shSERVER/CLIENT/TOOLS_VERSION pins perfeedback_plugin_version_pins.md(Slice 7 was chart-only so no plugin bump was warranted; Slice 8 ships plugin-visible behavior). - The full E2E test (id_token → initialize → tools/call) belongs in Slice 8 — the slash command exercises the path naturally.
Slice 7 merged + CodeRabbit round
Slice 7 merged as 277f83f (PR #151). One CodeRabbit review round before merge with six actionable comments — all valid, all addressed in a follow-up patch:
mcpPorthelper silently coerced bad strings to 0 via Sprig's best-effortintcast. The helper now regex-validates the port suffix is purely digits and range-checks 1..65535 before casting. Web-search reference: Sprig'sintiscast.ToInt(notcast.ToIntE); failed conversions return 0 with no error. Worth pinning as a general rule: never trust Sprig'sintfor chart-render validation — always pre-validate the string.ingress.mcp.host == ingress.hostwas allowed and would produce two Ingress rules for the same host backing different ports (controller-dependent precedence, undefined across implementations). Added a render-timefailwith a message pointing at the README's split-hostname rationale.- Port-collision guard was string-compare:
eq ":8081" ":8080". Missed equivalent forms like0.0.0.0:8080vs management:8080. Switched to numeric compare via the (now-strict)mcpPorthelper. - TLS validation comment in values.yaml was misleading — claimed the chart fails template-render if the existingSecretRef points at the wrong shape. Chart only references the Secret by NAME at render time; wrong shape (Opaque instead of kubernetes.io/tls, missing tls.crt/tls.key keys) surfaces as kubelet
CreateContainerConfigErrorat pod startup. Reworded. - MCP-API.md fence missing a language identifier (added
text). - MCP-API.md "per-pod-lifetime" → "pod-scoped" with explicit triggers (Helm rollout, OOMKill, node drain/eviction, kubelet restart). Clearer for operators reading the contract cold.
Four new helm-unittest cases pin the new guards. Helm-unittest went from 95 → 99 green; pre-commit green across all 4 modules. Replies posted to each thread before merge.
Worth pinning
- Sprig's
intis a footgun for chart validation. It's a best-effort cast (cast.ToInt), not the error-returning variant (cast.ToIntE).{{ "abc" | int }}returns 0 with no error. Any time a chart helper pipes a user-controlled string throughint, pre-validate withregexMatch "^[0-9]+$"(or similar) and range-check before the cast. Without that, typos in addr-style fields rendercontainerPort: 0and crash the pod with no breadcrumb in the chart-render output. - String-compare on
host:portis wrong for collision detection.:8080,0.0.0.0:8080,127.0.0.1:8080,[::]:8080all bind the same port but compare as different strings. When the goal is "do these two addr fields collide on bind()," extract the port number and compare numerically.
Surface status after Slice 7
The whole broker MCP gateway plan is one slice away from complete:
- Chart wiring: shipped (Slice 7, PR #151,
277f83f). - Broker binary: shipped (Slices 1-6, all merged).
- Kind harness
--with-mcp-smoke: shipped, three chart-wiring proof checks. - Bucket-store-backed persistent graph: parked for the post-broker design window (
/thoughts.md§ "On Bucket Stores"). - Slice 8 (
/knowledge-joinplugin slash command): remaining work. ~50 LOC + tests per plan estimate. Natural surface for the full id_token / initialize / tools/call E2E that Slice 7's smoke deferred.
Plan v6 published with Implementation Status caught up across Slices 2-7.