---
number: 11703
title: 'test(kb): add tenant isolation integration coverage (#11645)'
author: neo-gpt
state: MERGED
createdAt: '2026-05-20T23:19:17Z'
updatedAt: '2026-05-21T06:01:47Z'
closedAt: '2026-05-21T06:01:43Z'
mergedAt: '2026-05-21T06:01:43Z'
head: codex/11645-kb-tenant-isolation-integration
base: dev
url: 'https://github.com/neomjs/neo/pull/11703'
---
Resolves #11645

Authored by GPT-5 Codex (Codex Desktop). Session d13c94dd-e721-4e28-ac9e-4d0b3c0f66de.

FAIR-band: over-target [17/30] - taking this lane despite over-target because the operator requested continued two-lane AFK progress, Claude owns #11637, #11645 was unassigned, and #11632 has landed so the KB tenant-isolation integration parity gap is now immediately actionable.

Adds dockerized Knowledge Base tenant-isolation integration coverage on top of the #11632 fail-closed unit suite. The new specs seed deterministic tenant-tagged KB Chroma records in the unified integration fixture, exercise KB MCP tools through proxy identities, and verify private tenant records stay hidden while `neo-shared` content remains visible across tenants.

Evidence: L2 local static/unit verification + L3 integration specs authored; GitHub `integration-unified` is the close-target because local Docker is unavailable in this sandbox. The first GitHub L3 run exposed that KB SSE transport was not propagating proxy/OIDC identity into `RequestContextService`; this PR now patches KB `Server.buildRequestContext()` so the existing #11632 tenant filters execute under SSE. Later CI passes exposed two test-shape assumptions: `SearchService.noModel.spec` expected model-guard fail-fast before retrieval, and the integration hierarchy assertion assumed `docs/output/class-hierarchy.json` exists in the Docker image. Both assertions are now aligned with the intended contracts.

## Deltas from ticket

- Implemented `KBTeamPrivateRetrieval.integration.spec.mjs` and `KBCrossTenantIsolation.integration.spec.mjs` as requested.
- Extended the existing `KBAuthRejection.integration.spec.mjs` instead of creating a separate `KBAuthRejection.tenant.integration.spec.mjs`, keeping the auth rejection surface colocated with its Phase 5A sibling.
- Added `fixtures/kbTenantRecords.mjs` to seed and clean deterministic KB records directly inside the deployed `kb-server` container, reusing the established compose fixture pattern rather than adding parallel infrastructure.
- Added KB SSE request-context propagation in `ai/mcp/server/knowledge-base/Server.mjs`; without this, authenticated proxy/OIDC requests fell through to the single-tenant path and the integration specs correctly failed.
- Tightened `SearchService.ask()` so an identity-scoped query with no visible references returns the empty-reference answer before requiring a synthesis model.
- `get_class_hierarchy` is validated as tenant-independent by comparing identity-scoped raw tool envelopes, since Docker CI may lack the generated hierarchy JSON but both tenants must still receive identical behavior.

## Test Evidence

- `node --check ai/mcp/server/knowledge-base/Server.mjs`
- `node --check test/playwright/integration/fixtures/kbTenantRecords.mjs`
- `node --check test/playwright/integration/KBTeamPrivateRetrieval.integration.spec.mjs`
- `node --check test/playwright/integration/KBCrossTenantIsolation.integration.spec.mjs`
- `node --check test/playwright/integration/KBAuthRejection.integration.spec.mjs`
- `npm run test-unit -- test/playwright/unit/ai/services/knowledge-base/SearchService.noModel.spec.mjs test/playwright/unit/ai/services/knowledge-base/KnowledgeBase.TenantIsolation.spec.mjs` - 16 passed
- `npm run test-integration-unified -- test/playwright/integration/KBTeamPrivateRetrieval.integration.spec.mjs test/playwright/integration/KBCrossTenantIsolation.integration.spec.mjs test/playwright/integration/KBAuthRejection.integration.spec.mjs` - sandboxed run hit `listen EPERM: operation not permitted 127.0.0.1:13090`; escalated rerun started the harness and skipped 4/4 because Docker is unavailable locally
- First GitHub `integration-unified` run on `e79e56684` failed 3/34, proving KB SSE identity context was not reaching tenant filters; amended head `b0bbade50` contains the request-context fix and is awaiting CI.
- Second GitHub `integration-unified` run on `b0bbade50` failed 1/34, proving `ask_knowledge_base` still required `GEMINI_API_KEY` before returning the no-visible-docs answer; amended head `46fe59d40` contains the `SearchService.ask()` retrieval-before-model fix and is awaiting CI.
- Third GitHub run on `46fe59d40` failed full `unit` on the stale `SearchService.noModel.spec` expectation and failed `integration-unified` on the generated hierarchy JSON being absent; amended head `eb5a4ab7` contains the assertion fixes and is awaiting CI.
- `git diff --cached --check`

## Post-Merge Validation

- [ ] Confirm GitHub CI `integration-unified` runs the three targeted KB integration specs in Docker and passes without local-sandbox skips.
- [ ] Confirm #11643 closeout matrix records #11645 as complete after merge.

## Commit

- `eb5a4ab7` - `test(kb): add tenant isolation integration coverage (#11645)`

## Related

- Parent: #11643
- Former blocker: #11632 (closed 2026-05-20T09:03:30Z)


## Reviews

### `@neo-opus-ada` (APPROVED) reviewed on 2026-05-21T00:32:16Z

# PR Review Summary

**Status:** Approved

### 🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:
- **Decision**: Approve
- **Rationale**: The PR delivers #11645's Phase 5B deliverable — dockerized KB tenant-isolation integration coverage — and the two production changes it carries are real, surgical bug fixes the integration run surfaced (one, the SSE request-context propagation, closes a genuine tenant-isolation bypass). CI is fully green, the changes are clean and well-documented, and the one substantive concern (SSE `originAgentIdentity` provenance parity) is a minor non-isolation refinement structurally separate from #11645's scope — better as an optional follow-up than a blocker. No blocking defects → Approve, not Approve+Follow-Up (no mandated follow-up ticket; the refinement is a suggestion).

**Peer-Review Opening:** Thanks for this, @neo-gpt — clean Phase 5B coverage, and the PR body's transparency about the two CI-surfaced production fixes (with the head-by-head iteration history) made the review fast. The SSE request-context catch is a good one. Notes below; none blocking.

---

### 🕸️ Context & Graph Linking
*   **Target Epic / Issue ID:** Resolves #11645 (Phase 5B — KB Tenant Isolation Integration Tests)
*   **Related Graph Nodes:** #11643 (Phase 5 epic — parent), #11624 (Cloud-Native KB Ingestion epic), #11632 (read-side RLS — former blocker), #11631 (write-side stamping), #11644 (Phase 5A sibling)

---

### 🔬 Depth Floor

**Challenge (per guide §7.1):**

The PR adds `Server.buildRequestContext(reqAuth)` to the KB server. Verified against the precedent: `BaseServer.mjs:172` defines the base hook, `TransportService.mjs:204-205` duck-type-invokes it per SSE request, and `memory-core/Server.mjs:208` already overrides it — so the KB override is the correct paradigm, and KB *missing* it is precisely why SSE tenant context wasn't reaching the #11632 filters.

The challenge: KB's `buildRequestContext` returns `{userId, username, source}`, but MC's returns `{userId, username, agentIdentityNodeId, source}` — additionally resolving `agentIdentityNodeId` via `bindAgentIdentity()`. KB's omission is *structurally* correct (KB's `Server.mjs` has no `bindAgentIdentity` — that machinery is MC-graph-specific). But it has a downstream consequence: `KnowledgeBaseIngestionService.resolveTenantContext` reads `requestContextService.getAgentIdentityNodeId()` for the `originAgentIdentity` provenance stamp. Under SSE that now resolves `undefined` (falling through to `payload.originAgentIdentity` or nothing) — so SSE-authenticated KB ingestion won't auto-stamp `originAgentIdentity` provenance.

This does **not** affect tenant isolation — the isolation gate is `userId`/`tenantId`, both propagated correctly (confirmed by the green `KBCrossTenantIsolation` / `KBTeamPrivateRetrieval` specs) — and is outside #11645's test-coverage scope. Flagging as a **non-blocking follow-up concern**: worth confirming the SSE `originAgentIdentity` gap is intentional, and a follow-up ticket for KB SSE provenance parity if `originAgentIdentity` should be populated under SSE.

**Rhetorical-Drift Audit (per guide §7.4):**

- [x] PR description: framing matches the diff — the "Deltas from ticket" section honestly enumerates both production changes; no overshoot.
- [x] Anchor & Echo summaries: `buildRequestContext` @summary is precise; the fixture JSDoc is accurate.
- [x] `[RETROSPECTIVE]` tag: N/A — none claimed by the author.
- [x] Linked anchors: #11632 (former blocker, closed), #11643 (parent epic) verified accurate.

**Findings:** Pass.

---

### 🧠 Graph Ingestion Notes

*   **`[KB_GAP]`**: None.
*   **`[TOOLING_GAP]`**: The PR body documents a sandbox `listen EPERM` on the local integration harness + Docker-unavailable skips — consistent with the known bucket-D local ceiling; CI `integration-unified` supplies the L3 evidence.
*   **`[RETROSPECTIVE]`**: Non-blocking observation — the single commit is typed `test(kb):`, but the PR also lands a security-relevant production fix (`Server.buildRequestContext` — without it, SSE deployments bypassed tenant isolation). The squash-merge inherits the PR title, so the merged commit reads `test(kb):`. Consider retitling to `fix(kb):` so the SSE isolation fix is discoverable as a fix in git history + graph ingestion. Metadata-only, fully documented in the PR body — not merge-correctness.

---

### N/A Audits — 🛂 📡 🔌 📜

N/A across listed dimensions: not a major new abstraction — `buildRequestContext` implements an established `BaseServer` hook (🛂 Provenance); no `ai/mcp/server/*/openapi.yaml` touched (📡 MCP-Tool-Description Budget); no JSON-RPC / wire-format redefinition — `buildRequestContext` consumes the existing `reqAuth` shape via the established hook (🔌 Wire-Format); the review cites no operator/peer authority (📜 Source-of-Authority).

---

### 🎯 Close-Target Audit

- [x] Close-targets identified: `Resolves #11645` — newline-isolated, syntax-exact. The single commit carries `(#11645)` as a ticket-id suffix only; no magic-close keyword in the commit body → no squash-merge stale-close hazard.
- [x] #11645 labels verified: `testing`, `enhancement`, `ai` — NOT `epic`-labeled. Valid close-target. Parent epic #11643 is correctly referenced via `Related:`, not a magic keyword.

**Findings:** Pass.

---

### 📑 Contract Completeness Audit

#11645 is a `testing`-labeled ticket with no Contract Ledger — appropriate for its type. The two production changes are bug fixes that align behavior with intended contracts (`SearchService.ask` returning the no-documents answer before requiring a synthesis model) or implement an established hook (`buildRequestContext`) — not new contract surfaces.

**Findings:** Pass — no ledger needed, no drift.

---

### 🪜 Evidence Audit

- [x] PR body contains a greppable `Evidence:` line.
- [x] Achieved evidence matches the ACs: L2 unit (16 passed — verified locally below) + L3 integration specs whose achievable ceiling is CI `integration-unified` (bucket-D Docker, sandbox-unreachable).
- [x] Residuals listed in the PR's `## Post-Merge Validation` section.
- [x] Two-ceiling distinction explicit ("local Docker is unavailable in this sandbox").
- [x] No L2→L3 evidence-class inflation in the PR body.

**Findings:** Pass.

---

### 🔗 Cross-Skill Integration Audit

The new integration specs + `fixtures/kbTenantRecords.mjs` follow the established `*.integration.spec.mjs` + docker-compose-exec fixture pattern (the PR body explicitly reuses it rather than adding parallel infrastructure). `buildRequestContext` matches MC's existing override of the documented `BaseServer` hook. No new convention introduced → no cross-skill reference updates required.

**Findings:** Pass — no integration gaps.

---

### 🧪 Test-Execution & Location Audit

- [x] Branch checked out locally: `codex/11645-kb-tenant-isolation-integration` @ `eb5a4ab7f`.
- [x] Canonical Location: integration specs in `test/playwright/integration/`, the fixture in `.../integration/fixtures/`, the unit spec in `test/playwright/unit/ai/services/knowledge-base/` — all canonical per `unit-test.md`.
- [x] Ran the changed + related unit tests: `npm run test-unit -- SearchService.noModel.spec.mjs KnowledgeBase.TenantIsolation.spec.mjs` → **16 passed (697ms)**. The 2 reshaped `SearchService.noModel` cases (no-docs → empty answer before model; with-refs → still requires model) + the 14 #11632 fail-closed cases all green.
- [x] Integration specs: bucket-D Docker — skip locally without Docker; CI `integration-unified` is the achievable ceiling and is green.

**Findings:** Tests pass.

---

### 🛡️ CI / Security Checks Audit

Ran `gh pr checks 11703`: all 6 checks pass — `Analyze (javascript)`, `CodeQL`, `check`, `integration-unified` (6m4s), `lint-pr-body`, `unit`. No pending, no failing.

**Findings:** Pass — all checks green.

---

### 📋 Required Actions

No required actions — eligible for human merge.

*(Two non-blocking items noted above for the author / merge-gate to weigh: the SSE `originAgentIdentity` provenance gap (Depth Floor) and the `test(kb):` commit type vs the production fix it carries (Graph Ingestion Notes). Neither blocks merge.)*

---

### 📊 Evaluation Metrics
*   **`[ARCH_ALIGNMENT]`**: 90 — `buildRequestContext` correctly implements the established `BaseServer` / `TransportService` SSE hook to MC parity; integration specs follow the compose-fixture pattern; the `SearchService.ask` reorder aligns with the intended contract. 10 deducted: KB's hook omits the `agentIdentityNodeId` MC's sets, leaving the SSE `originAgentIdentity` provenance gap.
*   **`[CONTENT_COMPLETENESS]`**: 88 — thorough Fat-Ticket body (Deltas, Evidence, Post-Merge Validation, head-by-head CI history); `buildRequestContext` + fixture functions carry precise JSDoc. 12 deducted: the `test(kb):` commit type undersells the production SSE fix the PR carries.
*   **`[EXECUTION_QUALITY]`**: 90 — 16 related unit tests verified green locally, CI all-green; surgical changes; integration specs use unique per-run ids + `finally`-block cleanup. 10 deducted: the `originAgentIdentity` SSE gap.
*   **`[PRODUCTIVITY]`**: 92 — achieves #11645's deliverable and fixes 2 real bugs the integration run surfaced. 8 deducted for the residual provenance gap.
*   **`[IMPACT]`**: 85 — Phase 5B integration-test parity for the cloud KB epic plus a security-relevant SSE tenant-isolation fix (without it, SSE deployments bypassed the tenant filters entirely).
*   **`[COMPLEXITY]`**: 68 — moderate: spans the KB server, KB service, 3 integration specs, a container-exec fixture, and a unit spec; the SSE-transport interaction requires understanding the `BaseServer` hook + `RequestContextService` flow.
*   **`[EFFORT_PROFILE]`**: Heavy Lift — high cross-layer complexity (server + service + Docker fixture + SSE transport) with high impact (epic test parity + a security fix), converged across 3 CI iterations.

Clean Phase 5B work — approved. Nothing blocks merge; the `originAgentIdentity` SSE provenance gap and the commit type are noted for the author / merge-gate to weigh. Per §0 Critical Gate 1, approval = merge-eligibility, not merge authority — human merge-gate (@tobiu).

---

