# 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

- **`brokerMergeAdapter` captures `ctx` in the struct.** Normally a code smell, but `merge.Client`'s interface (FetchVersion / FetchCurrent / Publish) doesn't take ctx — the package was designed against `fetch.Client` which also doesn't take ctx. The adapter's lifetime is bounded by one `handleMarkPublish` call, 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's `mergeClientAdapter` uses against its `markClient` interface.
- **Default `on_conflict` flipped 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 calling `mark_publish` without specifying `on_conflict` now 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 local `optionalInt`) to avoid name collision.** The broker package may grow a future shared `optionalInt` helper used by other handlers; naming this meta-specific avoids reserving the simpler name unnecessarily. Same behavior as the local server's helper.
- **`formatMergeOutcome` is duplicated, not hoisted.** Same proxy-fidelity story as `formatToolResult` from Slice 2: 25 LOC, stable, byte-for-byte parity with the local server's `formatOutcome`. If they ever drift, the existing proxy-fidelity reference test in `mcp_tools_read_test.go` catches the `formatToolResult` side; 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 runs `optionalIntMeta` over server-version, which `strconv.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.
- **`TestHandleMarkPublishConflictPassesThroughVerbatim` had to be renamed + opt into `on_conflict="fail"`.** Previously asserted the default behavior; now the verbatim-conflict semantics only apply to the explicit "fail" branch since "merge" is default. Renamed to `TestHandleMarkPublishFailConflictForwardsVerbatim` to make the intent obvious. The test still exists and pins the still-supported opt-out behavior.
- **`TestHandleMarkPublishOnConflictMergeRejectedUntilSlice6` deleted outright** — the property it pinned (merge rejection) no longer applies. Replaced by `TestHandleMarkPublishMergeCleanOutcomeOK` + `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[].internalAddress` for 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 `:8081` after 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.host` is a parallel knob to `ingress.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:520` overrides both fields at package time with `${{ needs.semver-tools.outputs.new_version }}` — the in-tree value is documentation, not load-bearing. Latest published `tools/v*` tag is `tools/v0.1.15`, so the kind harness `BROKER_CHART_VERSION` got 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.name` is the chart's ONE TLS mode — pointing at a pre-existing `kubernetes.io/tls` Secret. Chart mounts read-only at `/etc/demarkus-broker/tls/mcp/` and renders `certFile/keyFile` paths 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-smoke` ships 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-join` slash 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.

- **`worldTokenTTL` knob from the plan is NOT in values.yaml** because the broker binary doesn't actually have a `WorldTokenTTL` field — the only mention is a doc-comment on MCPConfig. Slice 2's implementation went with "natural expiry from `Issuer.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 at `worlds[].defaultToken.expiresAfter` instead. Plan vs code drift, captured here.

- **`mcpPort` helper extracts the port from `server.mcp.addr`** so Deployment / Service / NetworkPolicy / config-render all use one source of truth. The alternative — surfacing both `addr` (string) and `port` (number) as separate values — would invite drift. The helper fails template render with a clear message if addr lacks a parseable port, better than rendering `containerPort: 0` and 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

- `WorldTokenTTL` is a plan field that never landed (above).
- The plan also listed `server.mcp.worldPool` for Slice 7's values surface. There is no `MCPConfig.WorldPool` substruct in the binary — the worldPool's lifecycle is internal to `Server.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 `containsDocument` against the same template that already renders two `---`-separated docs.** helm-unittest's `containsDocument` is per-document-index, not file-wide. Two assertions targeting documentIndex 0 and 1 of the same render kept failing in confusing ways. Split into separate `it:` cases: one asserts `hasDocuments: count 2` when 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 `:8081` always won. The fail-fast guard for blank addr is still real (verified manually via `helm template --set server.mcp.addr=`), just not unit-testable via helm-unittest. Removed the assert; the manual verification + the in-helper `fail` message are sufficient defense.
- **`server.mcp.addr` vs `server.port` collision check used `eq` on incompatible types.** First version compared a string (`:8081`) to an int (8080) — `eq` always false, guard never fired. Fixed by stringifying server.port to `printf ":%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 than `sh -c '...'`.
- **Pre-existing `deployment_test.yaml` assertion pinned `:0.1.0` image 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.AppVersion` in 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-join` plugin 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-smoke` flag 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-join` branch (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.sh` SERVER/CLIENT/TOOLS_VERSION pins per `feedback_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:

1. **`mcpPort` helper silently coerced bad strings to 0** via Sprig's best-effort `int` cast. The helper now regex-validates the port suffix is purely digits and range-checks 1..65535 before casting. Web-search reference: Sprig's `int` is `cast.ToInt` (not `cast.ToIntE`); failed conversions return 0 with no error. Worth pinning as a general rule: never trust Sprig's `int` for chart-render validation — always pre-validate the string.
2. **`ingress.mcp.host == ingress.host` was allowed** and would produce two Ingress rules for the same host backing different ports (controller-dependent precedence, undefined across implementations). Added a render-time `fail` with a message pointing at the README's split-hostname rationale.
3. **Port-collision guard was string-compare**: `eq ":8081" ":8080"`. Missed equivalent forms like `0.0.0.0:8080` vs management `:8080`. Switched to numeric compare via the (now-strict) `mcpPort` helper.
4. **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 `CreateContainerConfigError` at pod startup. Reworded.
5. MCP-API.md fence missing a language identifier (added `text`).
6. 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 `int` is 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 through `int`, pre-validate with `regexMatch "^[0-9]+$"` (or similar) and range-check before the cast. Without that, typos in addr-style fields render `containerPort: 0` and crash the pod with no breadcrumb in the chart-render output.
- **String-compare on `host:port` is wrong for collision detection.** `:8080`, `0.0.0.0:8080`, `127.0.0.1:8080`, `[::]:8080` all 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-join` plugin 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.