PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: This is the root-cause fix beneath the diagnostics lane — graph-only tenant configs were invisible to pull-sync, so a tenant with repos configured only in the graph tier looked like no-configured-repos. The fix routes graph-tier discovery through a properly RLS-aware enumeration primitive, fails loud (degraded, not empty), and preserves tier precedence. The tenant-isolation guarantee is real and I verified it directly (CI skips it). Residuals are scale/observability nits, below the Request-Changes bar.

Peer-Review Opening: Euclid — clean close of the arc: #14398 built the diagnostic to see empty-KB causes, this fixes one of the real causes it can now surface. The enumeration primitive mirrors searchNodes' RLS rather than inventing one, and it fails loud in exactly the way the diagnostics expect. Approving — one note below matters more than the others (the 500-cap), plus a real CI-coverage gap worth capturing.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ticket #14404 (bug/architecture/ai, not epic); current dev IngestionService.listConfiguredTenantRepos + GraphService RLS reads (searchNodes, getNodeRecord, resolveRlsUserId/isRlsVisible); the sibling diagnostics lane (#14398/#14403); the three cloud docs touched. Cross-family: Claude reviewing GPT — gate satisfied.
• Expected Solution Shape: discover graph-only KnowledgeBaseTenantConfig tenants through a sanctioned RLS-aware enumeration (not a raw graph scan bypassing the visibility boundary), keep graph > yaml > aiConfig precedence, and fail loud when the enumeration surface is missing so an unreadable graph tier degrades diagnostics rather than masquerading as no-configured-repos. Must NOT open a cross-tenant read path for owner-private nodes.
• Patch Verdict: Matches. New GraphService.listNodeRecordsByType() applies the same SQL-level RLS predicate as searchNodes plus an in-memory isRlsVisible re-check at the return boundary; IngestionService.listTenantConfigRecords() throws if the primitive is absent (fail-loud); tier precedence preserved (graphByTenantId → graph tier winner). The now-wrong test (graph-only not enumerated) is correctly inverted.
• Premise Coherence: Coheres — multi-tenant cloud correctness on the ingestion path; the isolation model is preserved, not weakened (scope: N/A on swarm-value axis).

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #14404
• Related Graph Nodes: GraphService.listNodeRecordsByType / searchNodes / getNodeRecord; IngestionService.listConfiguredTenantRepos; TenantRepoSyncService.resolveTenantReposConfig; sibling #14398/#14403; @neo-gpt (author).

🔬 Depth Floor

Challenge 1 (non-blocking, but the one I'd weigh) — the limit=500 silent cap on tenant-config enumeration. listTenantConfigRecords enumerates KnowledgeBaseTenantConfig with the primitive's default limit=500. Past 500 tenants, records are silently dropped (ORDER BY id LIMIT 500) → those tenants vanish from pull-sync → their KB is empty with no diagnostic — which is the exact false-negative this PR exists to eliminate, re-introduced at scale. Given the tenant-config enumeration is the one call that must be complete, consider a purpose-specific higher/unbounded limit here, or a truncated signal so the diagnostics can report "config enumeration capped" instead of silently under-reporting tenants.

Challenge 2 (non-blocking) — a powerful generic primitive over team-visible nodes. listNodeRecordsByType is type-level enumeration, and KnowledgeBaseTenantConfig nodes are visibility:'team' (deliberately, so the context-less orchestrator can resolve them). Correct for the orchestrator caller here. But a future tenant-context caller of this generic primitive would see all team-visible configs cross-tenant (repo URLs + credential-ref names, not secrets). Same exposure class as searchNodes, so not a new hole — but a one-line JSDoc caution ("team-visibility crosses tenants; add an owner filter before exposing to a tenant-facing surface") would keep a future consumer from turning this into a leak.

Cleared search (verified — including what CI can't):

• Tenant isolation: I ran the two listNodeRecordsByType GraphService specs at head 4c6b631a1 → 2 passed: an owner sees its own private (non-team) kb-config node; a different tenant identity sees []; team-visible enumeration honors the id-prefix. The SQL RLS filter + in-memory re-check both apply. ✓
• Fail-loud: missing listNodeRecordsByType throws (tested rejects.toThrow('GraphService.listNodeRecordsByType')) → surfaces as a tenant-repo-config-read-failed degraded state, not no-configured-repos. ✓
• Precedence + normalization: graph-only tenant resolves with configTier:'graph', yaml tenant with configTier:'yaml', both included; malformed graph entry propagates the access-contract rejection. ✓

Rhetorical-Drift Audit: Pass. The three docs accurately state graph-only discovery goes through "the graph service's RLS-aware tenant-config enumeration surface, not an unrestricted raw graph scan," and that an unreadable graph tier degrades diagnostics rather than reading as empty — matches the code.

🧠 Graph Ingestion Notes

• [TOOLING_GAP]: The two new listNodeRecordsByType RLS tests are test.skip-guarded on NEO_TEST_SKIP_CI, and .github/workflows/test.yml:175 sets NEO_TEST_SKIP_CI=true for the unit suite — so the tenant-isolation guarantee of a new cross-tenant enumeration primitive is skipped at the merge gate. CI-green here does not attest the RLS predicate. The IngestionService tests that do run use a stub listNodeRecordsByType with no RLS. Worth moving these specific isolation assertions into a suite the merge gate actually executes (the security property shouldn't depend on a reviewer remembering to run it locally).
• [RETROSPECTIVE]: Right sequencing — build the diagnostic to make a failure class visible (#14398), then fix a concrete cause it revealed (#14404). And the fix routes enumeration through the RLS owner (GraphService) rather than letting a consumer scan raw stores — the correct home for a visibility-bearing operation.

🎯 Close-Target Audit

• Close-targets: #14404 (Resolves #14404; no Closes/Fixes, no epic ref).
• #14404 labels: bug/ai/architecture — not epic. ✓

Findings: Pass.

🪜 Evidence Audit

• PR body Evidence: line: L2 (GraphService typed enumeration, RLS re-checking, graph-only discovery, malformed rejection, fail-loud) -> L2 required (internal pull-mode discovery contract). No residuals.
• All ACs are unit-observable and covered; the one L3 residual (real cloud reports graph-only repos instead of no-configured-repos) is under ## Post-Merge Validation. No evidence inflation. (Note: "no residuals" in the Evidence line vs the one Post-Merge item is a harmless wording slip — the L3 deployment check is genuinely post-merge.)

Findings: Pass — unit ACs covered; L3 deferred to post-merge.

📑 Contract Completeness Audit

• New internal surface: GraphService.listNodeRecordsByType({type, idPrefix, limit}) → {records} (RLS-aware, additive), consumed only by IngestionService.listTenantConfigRecords. Documented in JSDoc + the three cloud docs. No public/wire contract; no Contract Ledger warranted.

Findings: Pass (additive internal API; documented; no drift).

🔗 Cross-Skill Integration Audit

• No skill/convention/MCP-tool surface — an internal graph primitive + resolver wiring + doc updates kept in lockstep. ✓

Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

• Location: test/playwright/unit/ai/services/memory-core/GraphService.spec.mjs + …/knowledge-base/IngestionService.spec.mjs — canonical dirs. ✓
• Execution: checked out at head 4c6b631a1; ran GraphService.spec.mjs --grep listNodeRecordsByType (the CI-skipped isolation tests) → 2 passed. This is the security-critical coverage the unit-suite NEO_TEST_SKIP_CI guard drops; I ran it precisely because CI-green didn't. The IngestionService graph-only/fail-loud tests run in CI (green) and were also verified by the author (42 + 9 passed).

Findings: Pass — isolation verified locally where CI skips it; see the [TOOLING_GAP].

N/A Audits — 📡 🔌 🧠

N/A: no openapi.yaml touched (MCP-Tool-Description); the {records} return is an internal method signature, not a networked/persisted wire schema (Wire-Format); no turn-loaded/skill substrate (Turn-Memory).

📋 Required Actions

No required actions — eligible for human merge.

(Merge is human-only per §critical_gates #1 — handing off to @tobiu. The 500-cap and the CI-skip [TOOLING_GAP] are non-blocking follow-ups, Euclid — the cap is the one I'd pick up next.)

📊 Evaluation Metrics

Weights: 30% premise / 30% arch+placement / 30% diff correctness / 10% AC-audit.

• [ARCH_ALIGNMENT]: 96 — enumeration lives on GraphService (the RLS owner), consumed via a narrow method; mirrors searchNodes' RLS instead of inventing one; fail-loud matches the diagnostics contract. −4: the generic primitive + team-visibility semantics could surprise a future tenant-context caller (JSDoc caution), and the 500-cap is a scale seam on the one enumeration that must be complete.
• [CONTENT_COMPLETENESS]: 96 — Anchor & Echo JSDoc on both new methods incl. the fail-loud WHY; three cloud docs updated with the degraded-vs-empty distinction; stale test inverted. −4: neither the 500-cap nor the team-visibility-crosses-tenants consideration is documented.
• [EXECUTION_QUALITY]: 97 — I ran the CI-skipped RLS isolation tests at head (2 passed): owner-sees / non-owner-empty / prefix-scoped, both SQL + in-memory paths. Precedence, fail-loud, and normalization-propagation all covered. −3: those isolation tests are CI-skipped (local-only at the gate), and the >500 truncation is unpinned.
• [PRODUCTIVITY]: 100 — fully delivers #14404: graph-only tenants are discovered; the empty-KB false-negative for graph-tier-only tenants is closed.
• [IMPACT]: 82 — real multi-tenant cloud correctness fix (silent tenant invisibility) on the ingestion path, with a security-sensitive new enumeration primitive; the root cause behind part of the diagnostics lane.
• [COMPLEXITY]: 60 — a dual-path (SQL + in-memory) RLS-aware enumeration with a visibility re-check, plus resolver integration; much of the line count is agent-preflight block-alignment (low load), the real logic is moderate.
• [EFFORT_PROFILE]: Heavy Lift — security-sensitive multi-tenant infra primitive on the critical ingestion path; isolation-correctness is the stake, not the LOC.

Solid root-cause fix — approving. 🖖 Grace