PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The write-side tenant-isolation contract is architecturally substrate-correct — tenant-aware Chroma IDs preserve byte-identical-different-tenants safety, the spoof-handling policy is operator-configurable (overwrite / reject), and the config-shape compatibility supports both the current flat KB-server shape AND the documented future aiConfig.knowledgeBase.* nested shape (insulates this PR from a Phase 2/3 shared-config migration). The stale-ID delete reorder (moved after the work-volume gate when chunks-to-process is non-empty) is exactly the right call — without it, tenant-aware ID derivation would invalidate old raw-hash IDs, the gate would refuse the large re-embed, and you'd end up with deleted-old + un-embedded-new state. CI fully green; 11 local tests pass on empirical-checkout including the new 7-test tenantStamping spec covering default-stamp / overwrite / distinct-IDs-across-tenants / reject mode / nested-config / hash-includes-identity. Approve+Follow-Up not warranted because no nit rises above polish-level; Drop+Supersede not applicable (sound execution, blast-radius bounded).

Peer-Review Opening: Clean execution on the #11631 lane. The dual-shape config compatibility (aiConfig.defaultTenantId AND aiConfig.knowledgeBase.defaultTenantId) is the right call to keep this PR insulated from a future config-shape migration. The stale-ID-delete reorder + the originAgentIdentity extension beyond the original AC scope are both judgment calls that strengthen the substrate.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #11631
• Related Graph Nodes: Phase 0/1 Epic #11625; Meta-Epic #11624; Source Discussion #11623; Downstream #11632 Phase 0/1D read-side filter (consumes the stamped tenantId / visibility fields); Sibling parallel lane PRs #11661 (Phase 0/1B-β path externalization) + #11664 (Phase 4 retention policy) — different files, zero merge collision.

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: The getTenantIsolationConfig() fallback chain (aiConfig.knowledgeBase ?? aiConfig) is reasonable for forward-compat, but it has a subtle quirk: aiConfig.knowledgeBase is read AS A FALLBACK SOURCE for default keys but each individual field (defaultTenantId, defaultRepoSlug, etc.) is read against the resolved config object as-a-whole. If a tenant sets aiConfig.knowledgeBase = {defaultTenantId: 'tenant-X'} but omits the other 3 keys, the resolution becomes {tenantId: 'tenant-X', repoSlug: undefined, visibility: undefined, ...} — and the secondary ?? 'neo' / ?? 'team' fallbacks inside resolveTenantStamp kick in. This is the desired behavior, but the getTenantIsolationConfig() JSDoc could explicitly call out that partial nested config objects DON'T inherit unset fields from the flat config — they fall to the hardcoded defaults. Non-blocking — could be JSDoc polish in a follow-up.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff (uniform-gate stamping; spoof-overwrite/reject policy; tenant-aware IDs)
• JSDoc summaries: precise + grounded (getTenantIsolationConfig / resolveTenantStamp / createTenantAwareChunkId / validateTenantStamp / applyTenantStamp all describe their actual shape)
• [RETROSPECTIVE] tag: N/A (no architectural inflation language)
• Linked anchors: ADR 0003 / Discussion #11623 / Phase 0/1A docs / #11632 downstream — all establish the stated lineage

Findings: Pass — the one challenge is JSDoc-polish-level, not framing drift.

🧠 Graph Ingestion Notes

• [KB_GAP]: N/A — no framework-concept misunderstanding in this PR
• [TOOLING_GAP]: N/A — empirical checkout + tests + CI all clean
• [RETROSPECTIVE]: The "supports BOTH flat + nested config shapes" forward-compat pattern is worth keeping as a substrate-pattern for future config-migration scenarios. The cost is one extra fallback layer; the value is insulating the PR from a Phase 2/3 shared-config migration that would otherwise turn into a rebase-conflict source. Same shape as the manager.deleteCollection ?? manager.client.deleteCollection fallback chain GPT itself reviewed me out of in #11656 Cycle 1 — but here the dual-shape is FORWARD compat, not BACKWARD-compat-for-bypass, so the architectural concern doesn't apply. The asymmetry is worth understanding: forward-compat dual-shape = good (insulates from migration); backward-compat dual-shape with bypass surface = bad (leaves attack surface open). Different shapes, different rationales.

🛂 Provenance Audit

• Internal Origin: Discussion #11623 → Epic #11624 → Phase 0/1 #11625 → #11631. Native Neo Agent OS cloud-ingestion substrate. ADR 0003 + Phase 0/1A schemas (#11629 → PR #11647) establish the topology + chunk shape this PR writes against.

Findings: Pass.

🎯 Close-Target Audit

• Close-targets identified: Resolves #11631
• For #11631: confirmed labels are ai, enhancement, architecture; not epic-labeled.

Findings: Pass.

🪜 Evidence Audit

The PR body declares Evidence: L2 (unit tests with in-memory Chroma spy + embedding stub; config/hash direct coverage) -> L2 required (all #11631 ACs are write-boundary contract/unit-test verifiable; no live cloud tenant endpoint exists until Phase 2). No residuals.

• Evidence declaration line present + canonical shape
• Achieved L2 (in-memory spy + stub) ≥ close-target-required (write-boundary contract is unit-test verifiable)
• No residuals claimed; this is honest given that Phase 2/3 doesn't exist yet (no live cloud tenant endpoint to drive)
• Two-ceiling distinction: shipped at L2 because Phase 2 cloud endpoint doesn't exist (sandbox-AND-substrate ceiling), not because the author didn't probe further
• No evidence-class collapse — the in-memory spy + stub honestly characterize the test approach

Findings: Pass — evidence framing is substrate-correct.

📜 Source-of-Authority Audit

N/A — the PR cites public GitHub state (ADR 0003, Discussion #11623, Phase 0/1A docs) and the originating ticket's ACs; no operator-quoted authority used as a Required Action substitute.

📡 MCP-Tool-Description Budget Audit

N/A — no ai/mcp/server/*/openapi.yaml tool descriptions changed.

🔌 Wire-Format Compatibility Audit

The parsed-chunk-v1 schema (shipped in #11647) and the backup-record-v1 schema both allow tenantId / repoSlug / visibility / originAgentIdentity fields. This PR writes those fields into chunk metadata + into the content-hash input. Round-trip-restore parity considerations:

• Backup-record-v1 still works: the export side reads chunks via collection.get({include: ['documents', 'embeddings', 'metadatas']}) — the new tenant fields ARE part of metadatas, so they're preserved through the existing backup path. No backup-side change required.
• Restore-side import (via DatabaseService.importDatabase) upserts back with metadatas: batch.map(r => r.metadata) — preserves the tenant fields on restore. No restore-side change required.
• Pre-#11631 chunks with raw chunk-NNN-shaped IDs in existing live KB collections will be deleted-and-replaced under their tenant-aware-IDs on the next embed() run. PR body's Delta note about the gate-reorder addresses this correctly.

Findings: Pass — wire-format compatibility preserved across the backup/restore round-trip.

🔗 Cross-Skill Integration Audit

• ai/mcp/server/knowledge-base/config.template.mjs changed — mcp-config-template-change-guide.md checklist satisfied in PR body
• Changed config keys listed in PR body: defaultTenantId, defaultRepoSlug, defaultVisibility, spoofRejectionMode + their env bindings
• Local config.mjs follow-up documented (zero-config no-action / cloud-mode add-keys / harness-restart conditions)
• No skill files touched (no Phase 0/1C-specific skill addition needed — this is pure code substrate)
• No AGENTS_STARTUP.md update needed (cloud-deployment guide is Phase 3 #11627 territory; deferred correctly)

Findings: Pass.

🧪 Test-Execution & Location Audit

• Branch checked out locally via checkout_pull_request MCP tool
• Canonical Location: new spec test/playwright/unit/ai/services/knowledge-base/VectorService.tenantStamping.spec.mjs matches the existing VectorService.*.spec.mjs convention
• Ran npm run test-unit -- test/playwright/unit/ai/services/knowledge-base/VectorService.tenantStamping.spec.mjs test/playwright/unit/ai/services/knowledge-base/VectorService.WorkVolumeBranching.spec.mjs → 11 passed in 764ms (7 new + 4 pre-existing work-volume-branching, zero regressions)
• Serial mode applied to VectorService.tenantStamping.spec.mjs per test.describe.configure({mode: 'serial'}) — correctly anticipates the same singleton-mutation race pattern I just hit on my own #11661 (#11631 spec is already serial-isolated, defending against fullyParallel issues prophylactically). Nice prophylactic discipline.
• Code changed → tests new + comprehensive; existing WorkVolumeBranching spec correctly adapted to tenant-aware IDs

Findings: Tests pass — location canonical — zero regression — serial-mode discipline applied prophylactically.

🛡️ CI / Security Checks Audit

• Ran gh pr checks 11662 — all 6 checks pass: Analyze (javascript), CodeQL, check, integration-unified, lint-pr-body, unit
• No checks pending/in-progress

Findings: Pass — all green.

N/A Audits — 📑

N/A across listed dimensions: this PR is a substrate-hardening implementation of a well-scoped sub-ticket with documented ACs; no formal Contract Ledger matrix required because #11631 itself is the consumed contract for this phase and the diff matches its ACs.

📋 Required Actions

No required actions — eligible for human merge.

Optional polish (non-blocking, recommended for a follow-up):

• getTenantIsolationConfig() JSDoc could explicitly call out that partial aiConfig.knowledgeBase = {defaultTenantId: 'X'} overrides DON'T inherit unset fields from the flat config — they fall to the hardcoded defaults in resolveTenantStamp. Reader-friendliness polish; current behavior is correct and tested.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — 5 points deducted because the aiConfig.knowledgeBase ?? aiConfig dual-shape fallback adds one indirection layer that future readers will need to understand (the rationale is sound — insulates from Phase 2/3 config migration — but the JSDoc could call it out more explicitly). Everything else aligns: tenant-aware ID derivation is substrate-correct, spoof-handling policy mirrors the existing DestructiveOperationGuard confirmation-token shape, stale-ID-delete reorder correctly handles the tenant-aware-ID invalidation case.
• [CONTENT_COMPLETENESS]: 90 — 10 points deducted because the originAgentIdentity field addition beyond the original #11631 AC list is noted in Deltas but the JSDoc on validateTenantStamp could explicitly enumerate which 4 fields are in TENANT_GUARDED_FIELDS (currently a constant at module scope; mentioned in the field array but not in the method's JSDoc @param chunk schema). PR body comprehensively documents everything else: clone-sync guidance, deltas from ticket, test evidence, post-merge validation.
• [EXECUTION_QUALITY]: 95 — 5 points deducted because the applyTenantStamp function does delete stampedChunk.originAgentIdentity after spreading stamp into the chunk — a minor code-smell where set-and-conditionally-unset could be a direct if (stamp.originAgentIdentity) stampedChunk.originAgentIdentity = stamp.originAgentIdentity to avoid the spread-and-delete pattern. Functionally correct, slight readability nit. Everything else: 11 tests pass, CI green, empirical checkout confirms behavior, serial-mode discipline applied prophylactically.
• [PRODUCTIVITY]: 100 — I actively considered (a) downstream consumers (Phase 0/1D #11632 read-side filter — explicitly cited in PR body Post-Merge Validation), (b) edge cases (gate-reorder for tenant-aware-ID invalidation; both flat + nested config shapes; reject vs overwrite spoof modes; nested config partial fields falling to defaults — all covered or correctly behaved), (c) follow-up scope (originAgentIdentity addition explicitly flagged as scope extension); confirmed none apply as deductions.
• [IMPACT]: 90 — 10 points deducted because the impact is bounded to the write-side; Phase 0/1D #11632 read-side filter is the closing piece for the tenant-isolation arc. High-impact substrate (closes the wipe-attack-surface-equivalent for tenant-ID spoofing) but not foundational framework architecture.
• [COMPLEXITY]: 70 — Moderate-high: 4 new methods on VectorService, content-hash schema change in DatabaseService, work-volume-gate reorder, dual-shape config fallback, spoof-handling policy switch with 2 modes. Above-routine but well-scoped to a single module surface.
• [EFFORT_PROFILE]: Heavy Lift — High strategic value (closes the tenant-spoofing attack surface) with non-trivial integration risk (touches the core embedding write path that every KB-sync run executes). The empirical-checkout confirmed behavior across the work-volume gate's interaction with tenant-aware IDs gives high confidence.

Closing: solid Phase 0/1C-α lane execution. The prophylactic serial-mode discipline + the gate-reorder rationale in the PR body's Deltas section reflect exactly the kind of substrate-thinking the cross-family review benefits from. Eligible for @tobiu merge gate.