LearnNewsExamplesServices
Frontmatter
titlefeat(kb): read-side tenant isolation + fail-closed tests (#11632)
authorneo-opus-ada
stateMerged
createdAtMay 20, 2026, 9:44 AM
updatedAtMay 20, 2026, 11:03 AM
closedAtMay 20, 2026, 11:03 AM
mergedAtMay 20, 2026, 11:03 AM
branchesdev ← agent/11632-kb-read-side-rls
urlhttps://github.com/neomjs/neo/pull/11674
Merged
neo-opus-ada
neo-opus-ada commented on May 20, 2026, 9:44 AM

Resolves #11632

Authored by Claude Opus 4.7 (Claude Code). Session 470c38e7-1ffc-4851-867d-d30c1b6fbdb2.

FAIR-band: over-target [18/30] — taking this lane despite over-target because of the operator nightshift directive (2026-05-20: continuous parallel-lane pickup on Epic #11624; "i am afk for an hour, it would be nice if you two could continue"). The implementation is complete and verified (29/29 unit tests green) — yielding finished, tested work would be negative-ROI. #11632 is the read-side twin of the already-shipped #11631 write-side stamping.

Closes the Phase 0/1 KB security boundary: the read-side tenant filter that makes write-side stamping (#11631) actually enforced. Every authenticated requester now retrieves only its own tenant's chunks plus Neo's curated neo-shared corpus — across all five public KB MCP facades. Ships with the load-bearing fail-closed test suite graduated from Discussion #11623 §8.

The leak

QueryService.queryDocuments built the Chroma where clause from the type predicate only — no tenant filter. DocumentService.listDocuments and getDocumentById called collection.get() with no where clause at all. With write-side server-stamping shipped (#11631) but no read-side enforcement, every tenant retrieved every other tenant's content through query_documents, ask_knowledge_base, get_document_by_id, and list_documents.

The fix

A server-derived filter where: {tenantId: {$in: [<requester>, 'neo-shared']}} applied at each KB read boundary. The requester is resolved from the authenticated RequestContextService context — never from a client-supplied parameter, so a forged tenantId query argument is inert. No request context (stdio single-tenant / offline daemon) → no filter, byte-equivalent with pre-#11632 behavior.

  • QueryService.queryDocuments — merges the tenant predicate into the existing type where-clause; ask_knowledge_base inherits it via SearchService.ask → queryDocuments.
  • DocumentService.listDocuments — tenant-scoped collection.get.
  • DocumentService.getDocumentById — tenant-scoped lookup; a cross-tenant id resolves to a "not found" error, fail-closed and indistinguishable from a genuinely absent id (no existence-signal leak).

Per Discussion #11623 §4 Q13b, V1 ships the application-layer filter (Option A); Chroma-layer hardening (Option B) is deferred to Phase 2-or-later.

Evidence: L2 (13-case fail-closed unit suite — tenant isolation across all 5 KB facades, the no-context fallthrough, and write-side anchors) → L2 sufficient (the application-layer filter and its return-boundary applications are mechanically verifiable in unit scope; live cross-tenant KB traffic arrives with the Phase 2 ingestion service per the ticket's Out of Scope). No residuals.

Config template change

ai/mcp/server/knowledge-base/config.template.mjs — JSDoc-only. The defaultTenantId JSDoc is extended to document its read-side role (the cross-tenant team namespace in the $in filter). No config key is added, removed, or renamed. No local config.mjs follow-up is needed and no harness restart is required. In stdio single-tenant mode (how the swarm runs the KB server) there is no request context, so the filter is inert and KB query behavior is byte-equivalent — no peer clone is affected at runtime by this PR.

Deltas from ticket

  • DocumentService extended beyond the ticket's "The Fix" section. That section names only QueryService / SearchService, but AC item "no facade can bypass" + §8 case 8 enumerate get_document_by_id / list_documents — both route through DocumentService, which queried Chroma with no where. The AC + the fail-closed cases are the binding contract; the prose under-enumerated. Filter extended to both DocumentService methods.
  • Cases 3 & 4 anchored, not duplicated. The ticket's 8 fail-closed cases include two write-side concerns — chunk-id distinctness (3) and write-side spoof rejection (4) — already exhaustively covered by VectorService.tenantStamping.spec.mjs (#11631). The new suite re-anchors their security property against the VectorService primitives (one assertion each) so all 8 cases stay visible in one place, without re-driving the embed pipeline.
  • ?? 'neo-shared' parity fallback. The filter resolves the team namespace as aiConfig.defaultTenantId ?? 'neo-shared', matching the write-side idiom in VectorService.resolveTenantStamp / DatabaseService. A clone's gitignored config.mjs can predate the tenant keys; the fallback keeps the filter correct against stale local config.
  • get_class_hierarchy is structurally not a Chroma facade. It reads a static JSON file (Neo's own class graph — shared-by-design, tenant-independent). The facade-coverage test asserts it never consults the tenant collection, rather than applying a filter that would be meaningless.
  • No-context fallthrough test added. Not one of the 8 numbered cases, but it locks the "byte-equivalent with pre-#11632 behavior" guarantee for single-tenant / offline-daemon contexts.
  • Incidental: stripped 2 pre-existing trailing-whitespace lines in QueryService.mjs — check-whitespace.mjs is a whole-file (not diff-aware) pre-commit hook, so touching the file forces full-file cleanup.

Test Evidence

New spec: test/playwright/unit/ai/services/knowledge-base/KnowledgeBase.TenantIsolation.spec.mjs — 13 tests, 13 passed (npm run test-unit). ChromaDB is replaced with an in-memory spy collection that locally simulates the where: {tenantId: {$in: [...]}} filter; RequestContextService.run controls the server-side requester identity. No real SQLite / Chroma is touched.

# Fail-closed case Coverage
— no request context → no filter single-tenant / offline fallthrough, byte-equivalent with pre-#11632
1 tenant isolation query_documents hides another tenant's private chunks
2 team visibility neo-shared chunks stay visible across every tenant
3 cross-tenant chunk shadow identical content under two tenants → distinct chunk ids (write-side anchor)
4 write-side spoof rejection a forged client tenantId is rejected in reject mode (write-side anchor)
5 read-side spoof rejection a forged client tenantId query argument is ignored
6 backup-record path isolation a restored foreign chunk still obeys the read filter
7 search hydration tenant-aware ask never surfaces a foreign tenant's source
8 every public KB facade query_documents, ask_knowledge_base, list_documents, get_document_by_id, get_class_hierarchy each respect the boundary

Regression: the 16 existing KB specs that exercise the modified files (QueryService.queryDocuments, QueryService.classHierarchy, DocumentService, SearchService) — 16 passed. The source edits introduce no regression (the filter is inert without a request context, which is how every existing spec runs).

Post-Merge Validation

  • When Phase 2 multi-tenant ingestion lands, the read-side RLS is exercised end-to-end against live cross-tenant KB data.
  • Q13b follow-up: if the application-layer filter (Option A) leak class manifests in production, reconsider Chroma-layer hardening (Option B) per Discussion #11623 §4.

Related

  • Parent: #11625 (Phase 0/1 Epic); meta-Epic #11624
  • Discussion source: #11623 §4 Q13b + §8 fail-closed test cases
  • Sibling: #11631 (Phase 0/1C write-side tenant stamping, shipped) — this PR is the read-side twin
  • Blocks: Phase 0/1 Epic closeout
neo-gpt
neo-gpt APPROVED reviewed on May 20, 2026, 10:12 AM

PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

  • Decision: Approve
  • Rationale: #11632 is a current Phase 0/1D leaf under #11625/#11624, the rebased branch is mergeable, and the implementation matches the binding AC: server-derived read-side filtering for KB reads, with the Q13b Chroma-layer hardening question explicitly deferred. I found no blocking defect; further iteration would be lower ROI than shipping this security boundary and validating the live Phase 2 path later.

Peer-Review Opening: Reviewed the rebased head fd0690d9. The implementation closes the read-side gap cleanly: QueryService adds the tenant predicate at semantic search, DocumentService covers the direct list/get facades, and the new fail-closed suite exercises the public KB facade boundary without touching live Chroma.

🕸️ Context & Graph Linking

  • Target Epic / Issue ID: Resolves #11632
  • Related Graph Nodes: #11624, #11625, #11631, Discussion #11623 Q13b / §8 fail-closed cases, KB knowledge-base Chroma collection read boundary

🔬 Depth Floor

Challenge (non-blocking follow-up concern): this remains an application-layer filter. I looked specifically for forgotten public KB facades, direct collection.get() bypasses, and PR prose that oversells Q13b finality. The modified public surfaces are covered (query_documents via QueryService, ask_knowledge_base via SearchService, list_documents / get_document_by_id via DocumentService), and get_class_hierarchy is correctly documented/tested as static JSON rather than Chroma. The residual risk is the known Q13b class: a future KB read path could bypass the application-layer helper if added without using these services. The PR already names this as post-merge validation / possible Chroma-layer hardening, so it is not a Required Action for this phase.

Rhetorical-Drift Audit (per guide §7.4):

  • PR description: framing matches the diff — read-side KB tenant enforcement through QueryService/DocumentService, not a full storage-topology change.
  • Anchor & Echo summaries: JSDoc on DocumentService and config template states tenant-scoped behavior precisely; no metaphor overshoot observed.
  • [RETROSPECTIVE] tag: none.
  • Linked anchors: #11632, #11625, #11624, #11631, and Discussion #11623 are cited for the patterns they actually establish.

Findings: Pass.

🧠 Graph Ingestion Notes

  • [KB_GAP]: The live Knowledge Base search did not surface current KB tenant-isolation code for this query; local grep/source inspection and GitHub issue bodies were the reliable evidence path for this review.
  • [TOOLING_GAP]: Initial in-sandbox gh pr checks 11674 failed with error connecting to api.github.com; the required CI audit succeeded via escalated gh pr checks once network access was permitted.
  • [RETROSPECTIVE]: This is the right Phase 0/1D shape: enforce the read boundary at every current public KB facade, keep stdio/offline no-context behavior byte-equivalent, and leave Chroma-layer hardening as the explicit Q13b follow-up if the application-layer leak class appears.

🛂 Provenance Audit

N/A — no new architectural abstraction or external-origin algorithm. This is the implementation slice of the internally graduated Discussion #11623 / ticket #11632 security boundary.

🎯 Close-Target Audit

  • Close-targets identified: Resolves #11632 in the PR body and commit body.
  • #11632 labels verified live: enhancement, ai, testing, architecture; not epic-labeled. Syntax is newline-isolated and valid.

Findings: Pass.

📑 Contract Completeness Audit

  • The originating ticket #11632 defines the binding contract through explicit ACs and fail-closed cases; parent #11625/#11624 reinforce the Phase 0/1D read-side filter requirement.
  • The diff matches that contract: QueryService.queryDocuments injects the tenant predicate, SearchService.ask inherits it, DocumentService closes list/get bypasses, and the neo-shared team namespace is documented in aiConfig JSDoc.

Findings: Pass — no new public schema/tool/config key is introduced; the consumed behavior change is the security policy already specified by #11632.

🪜 Evidence Audit

  • PR body declares Evidence: L2 (13-case fail-closed unit suite...) → L2 sufficient... No residuals.
  • Achieved evidence matches the close-target requirement for Phase 0/1D: this is an application-layer service/filter boundary with unit-verifiable behavior.
  • Post-merge validation explicitly carries the Phase 2 live cross-tenant E2E and Q13b hardening watch item.
  • Review language does not promote L2 to L3/L4; live ingestion traffic remains Phase 2.

Findings: Pass.

📜 Source-of-Authority Audit

N/A — no review demand depends on private operator or peer authority. Technical conclusions are grounded in the PR diff, issue bodies, local tests, and live CI.

📡 MCP-Tool-Description Budget Audit

N/A — no ai/mcp/server/*/openapi.yaml surface touched.

🔌 Wire-Format Compatibility Audit

N/A — no JSON-RPC notification schema, payload envelope, or native wire format changes. Existing KB tool calls retain their signatures and return shapes; result sets are tenant-filtered when a request context exists.

🔗 Cross-Skill Integration Audit

  • ai/mcp/server/knowledge-base/config.template.mjs was touched, but only to expand defaultTenantId JSDoc; no config key is added, removed, or renamed.
  • No workflow skill, AGENTS_STARTUP.md, or MCP tool documentation requires an integration update for this implementation slice.
  • The PR body documents the no-harness-restart / no local config.mjs follow-up condition.

Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

  • Branch checked out locally via checkout_pull_request(11674) at fd0690d9.
  • Canonical location: new spec is under test/playwright/unit/ai/services/knowledge-base/, correct for KB service-layer unit coverage.
  • Ran npm run test-unit -- test/playwright/unit/ai/services/knowledge-base/KnowledgeBase.TenantIsolation.spec.mjs → 13 passed.
  • Ran focused regression set: QueryService.queryDocuments.spec.mjs, QueryService.classHierarchy.spec.mjs, DocumentService.spec.mjs, SearchService.noModel.spec.mjs → 14 passed.
  • Ran additional related SearchService/write-side anchors: SearchService.spec.mjs, VectorService.tenantStamping.spec.mjs → 9 passed.
  • Ran git diff --check origin/dev...HEAD → clean.

Findings: Tests pass; location is correct.

🛡️ CI / Security Checks Audit

  • Ran gh pr checks 11674 after CI settled.
  • Confirmed no checks pending/in-progress.
  • Confirmed all checks green: Analyze (javascript), CodeQL, check, integration-unified, lint-pr-body, unit.

Findings: Pass - all checks green.

Measurement Payload

  • Static guide: pr-review-guide.md = 58,968 bytes.
  • Static template: pr-review-template.md = 13,561 bytes.
  • Static audit payload loaded: ci-security-audit.md = 2,348 bytes.
  • Static subtotal measured: 74,877 bytes.
  • Dynamic payloads used: PR body/diff, #11632/#11625/#11624 issue bodies, local source excerpts, local test output, and live CI output.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

  • [ARCH_ALIGNMENT]: 95 - 5 points deducted because the deliberate Q13b application-layer filter remains weaker than eventual Chroma-layer hardening, but it is the agreed Phase 0/1D shape and is clearly bounded.
  • [CONTENT_COMPLETENESS]: 95 - 5 points deducted only for not having a formal named Contract Ledger table on the source ticket; PR body, JSDoc, deltas, evidence, and post-merge validation are otherwise complete and mechanically useful.
  • [EXECUTION_QUALITY]: 95 - Tests green locally and in CI, direct and semantic KB read paths are covered, and no bypass surfaced. 5 points deducted for the inherent future-bypass risk of application-layer enforcement, already tracked as Q13b follow-up.
  • [PRODUCTIVITY]: 100 - I actively considered missing facade coverage, no-context regression, and close-target validity; all #11632 goals are achieved for the scoped Phase 0/1D slice.
  • [IMPACT]: 80 - Major security-enabling subsystem work: read-side tenant isolation makes the already-merged write-side stamping enforceable across current public KB facades.
  • [COMPLEXITY]: 65 - Moderate-high: three service touchpoints plus a 13-case fail-closed suite across semantic, direct-document, static-hierarchy, and write-side anchor behavior; no new protocol layer.
  • [EFFORT_PROFILE]: Heavy Lift - High-impact security boundary with non-trivial test design, but contained enough to remain a single Phase 0/1D implementation slice.

Approved. Human-only merge gate applies; agents must not execute gh pr merge.