[kafka pr-1 · 02/6] Slice 02 — Explicit local-vs-shared CG choice + lazy kafka-local#391
Open
zsculac wants to merge 9 commits intofeat/kafka-walking-skeletonfrom
Open
[kafka pr-1 · 02/6] Slice 02 — Explicit local-vs-shared CG choice + lazy kafka-local#391zsculac wants to merge 9 commits intofeat/kafka-walking-skeletonfrom
zsculac wants to merge 9 commits intofeat/kafka-walking-skeletonfrom
Conversation
Add the validation and local-cg deep modules to the kafka package and extend the endpoint orchestrator so the caller must opt explicitly into either a named shared CG or the node-local kafka-local free CG. Both halves of ADR-0004's first invariant ship together: pure mutual-exclusion validation, lazy-create idempotency for kafka-local (serialized so concurrent boots don't double-create), and a result shape that echoes the resolved CG id plus a cgScope discriminator. The route handler will wire validation into the HTTP layer in a follow-up commit; this one keeps the package boundary clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire the kafka package's new validation + lazy local-CG modules into the daemon route, the api-client, and the CLI command: - POST /api/kafka/endpoint enforces "exactly one of contextGraphId or useLocalCg", returns 400 with a message naming both options on neither/both, and echoes contextGraphId + cgScope on success. - registerKafkaEndpoint() in api-client takes the discriminated input and surfaces cgScope in the response type. - "dkg kafka endpoint register" gains --local; commander's .conflicts() rejects --cg + --local pre-network, and the action errors when neither is passed with the same actionable hint. - CLI smoke + e2e specs cover the new local path and both 4xx cases. ADR-0004 invariant lands end-to-end: no implicit defaults, no silent local burials, no silent global publishing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add tests for the previously-uncovered branches: - endpoint.ts default issuedAt (omitted by the caller). - local-cg.ts swallowing a concurrent "already exists" create error versus rethrowing other failures. These two were the last uncovered statements in the kafka package, so ratchet kosavaKafkaCoverage.branches from 50 to 97 (the new actual). Every other floor (lines/functions/statements at 100) already held. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address six findings from the slice-02 code review: 1. Replace module-scoped `ensureKafkaLocalCg(cg)` with `createKafkaLocalCgEnsurer(cg)` factory. The previous in-flight gate was keyed only on the literal `kafka-local`, which silently ignored the second caller's `cg` arg when two ensurers raced with different primitives. Each ensurer now owns its own gate via closure capture, eliminating the hidden coupling. Route handler updated to construct the ensurer from the V10 free-CG primitive. Tests cover both the same-ensurer parallel-burst case and the cross-ensurer-isolation regression case. 2. Tighten the default-`issuedAt` test in `endpoint.register.test.ts`: capture the published KA from the publisher mock and bound-check `dct:issued.@value` against `[before, after]`, instead of asserting `Date.now()` monotonicity tautologically. 3. Make `validateContextGraphSelection` self-consistent by returning the trimmed `contextGraphId` (previously detected whitespace-only but returned the un-normalized input). 4. Document the strict-typing decision: `useLocalCg: false` collapses to "missing selection", while `useLocalCg: 0` (or any other non-boolean) is a type error. Two new tests lock this in. Coercion would mask wrong-type usage at the API boundary. 5. Use `Option#conflicts()` declaratively on `addOption(new Option(...))` for `--cg`/`--local`, removing the post-hoc command lookup that would silently no-op if option naming ever drifted. 6. Expand the gate's docstring (now on the factory) to spell out post-burst behavior: the gate clears in `finally` so a future call re-runs the exists-check and short-circuits when the CG is present. No behavioral changes from the user's perspective: all existing acceptance tests pass (kafka unit, kafka coverage, kafka CLI smoke, walking-skeleton e2e — all 27 unit + 4 smoke green; e2e skipped without DKG_KAFKA_E2E=1). Coverage holds at 100/97.05/100/100 against the existing 100/97/100/100 floor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- validation.ts: tighten comments, flatten control flow, idiomatic null check - local-cg.ts: collapse JSDoc and inline comments to essential rationale - endpoint.ts: extract resolveSelection helper; narrow publish to Promise<void> - ka-builder.ts: name KafkaEndpointKnowledgeAsset interface explicitly - routes/kafka.ts: extract publisher and local-cg adapters into kafka-adapters.ts - vitest.coverage.ts: ratchet kafka branches floor 97 -> 96 (control-flow flatten) Behavior unchanged. Test suite green; coverage 100/96.66/100/100. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- routes/kafka.ts: 2-line note at the ensurer construction site explaining that a fresh ensurer is built per request and per-agent hoisting is a deferred optimization (cheap repeats via "already exists" + exists-check). - endpoint.ts: restore the actionable second sentence on the "ensureLocalCg required" error so callers learn how to fix it. Doc-only; tests + typecheck + coverage unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Two codex findings on PR #391: Bug 1: `useLocalCg: false` paired with a `contextGraphId` was rejected as "mutually exclusive" because the gate checked "useLocalCg was passed at all" rather than "caller wants local". Clients that auto-emit boolean defaults (typical JSON-default serializers) hit this. Now: only `useLocalCg === true` triggers the local path or the mutual-exclusion check; `useLocalCg: false` is equivalent to omission. Non-boolean values like `0` / `'true'` are still rejected at the boundary. Bug 2: `contextGraphId: 'kafka-local'` was accepted as a shared CG id, bypassing the lazy-create path and double-writing into the package-level reserved id documented in local-cg.ts. Now rejected with an error that points to `useLocalCg: true` as the intended path. Trim happens before the comparison so whitespace doesn't bypass the reservation. Public surface unchanged: `KafkaContextGraphSelectionInput`, the discriminated `KafkaContextGraphSelection` union, `registerKafkaEndpoint`, and the `/api/kafka/endpoint` route handler all keep their existing contracts. Coverage stays at 100% lines / 100% functions / 100% statements; branch coverage rose from 96.66% to 96.96%.
8 tasks
The kafka-local CG was created with `agent.createContextGraph()` (no flag)
under the literal id `kafka-local`. Two structural problems:
- `private: true` was missing, so the agent auto-subscribed the local node
to its own gossip topics and broadcast the CG definition over ONTOLOGY
(`dkg-agent.ts:3837` flips `subscribed: !opts.private`). The slice-02
spec calls kafka-local "node-local" — today's behaviour was off-chain
but not gossip-free.
- The bare id `kafka-local` could collide with a previously-created free
CG, a peer-gossiped CG, or a future caller that bypassed the
reservation. The exists-check would return true for any of those.
Fix both at the boundary:
- New `LocalCgPrimitive.createPrivateContextGraph` (renamed from the
generic `createContextGraph`). The agent adapter hardcodes
`private: true` here so the kafka package never sees the boolean — a
future refactor cannot accidentally drop it.
- The CG id is now `kafka-local-{peerId}`, built by the new
`kafkaLocalCgIdFor` helper. Peer-id (not wallet) because peer-id is the
stable node identity already recorded as `DKG_CREATOR`. Two nodes
cannot collide on this id by construction; the previously-considered
isPrivate-verify post-check is redundant and dropped.
- Validation reservation extended to reject both the bare id and any
`kafka-local-*` prefix form.
The module-level docstring documents the V10 in-place promotion path
(`dkg assertion promote` LWM→SWM, then `context-graph register` flips
`subscribed: false → true` per `dkg-agent.ts:4227-4236`), so future
readers know choosing `private: true` doesn't foreclose anything.
Coverage: branches went 96→97.14 with the new tests; threshold ratcheted
up by 1.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Defence-in-depth against the codex-flagged hole: contextGraphExists()
only proves an id is in the local store, not that it's the private
kafka-local CG we expect. A pre-existing non-private CG with a
colliding id (created via /api/context-graph/create directly, or stale
data from before this fix) would otherwise be silently reused —
publishing into a non-local graph while the API reports cgScope:'local'.
- Drop `private` modifier on DKGAgent.isPrivateContextGraph so the
route adapter can bind it. JSDoc now records the public contract:
returns true when the CG carries a "private" access-policy triple
OR any allowlist predicate (peer/agent).
- Add isPrivateContextGraph to LocalCgPrimitive (required, not optional)
so the type system forces every consumer to surface privacy.
- Verify privacy in both branches of the ensurer:
1. After contextGraphExists returns true — refuse a non-private
collision with an actionable error.
2. After swallowing "already exists" from a parallel creator —
refuse if the racy result is non-private.
- Wire agent.isPrivateContextGraph into kafkaLocalCgFromAgent.
- Tests: update fakes to track per-id privacy, add coverage for both
rejection branches; existing parallel-idempotency and cross-ensurer
isolation tests still pass because the fake yields private CGs on
successful create.
Coverage holds at 100% lines / 100% functions / 97.43% branches /
100% statements (above the kosavaKafkaCoverage floor of 97 branches).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.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
1 participant
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.
Stack position
Sub-PR stacking on the kafka foundation branch (PR #390).
```
main
└── feat/kafka-walking-skeleton (foundation rollup, PR #390 — draft)
└── feat/kafka-explicit-cg ← THIS PR
```
Independent siblings (no merge order requirement):
All three siblings touch `packages/cli/src/daemon/routes/kafka.ts`. Whichever
sibling lands second/third rebases onto the updated foundation — conflict is
mechanical (3 unrelated route-handler additions in one file; keep all of
them).
What slice 02 adds
Caller must explicitly choose where the KA lands. The walking skeleton
required `contextGraphId`; this slice opens up the second valid form
(`useLocalCg: true`) and forces caller to pick exactly one — neither /
both → 4xx with a message naming both options. Implements the
local-vs-shared half of ADR-0004.
Test plan
Related