PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The PR targets the right closeout slice for #10011, local focused tests pass, and CI is green. I am requesting changes because the implementation mirrors RLS for cache-resident nodes only, while the SQLite lazy-load path also applies the same RLS clause to Edges. That leaves a cache-warmed private-edge leak through the return-boundary methods that surface relationships.

Peer-Review Opening: This is the right shape for the #10011 read-side closeout: central predicate, return-boundary filtering, and cache-warmed leak tests. The gap below is narrow but security-boundary relevant, so it needs to be fixed before this can move to the human merge gate.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #10011
• Related Graph Nodes: #11624 / #11625 Phase 0/1 tenant-isolation path; #11672 PR; GraphService, SQLite.loadNodeVicinitySync, Native Edge Graph RLS

🔬 Depth Floor

Challenge — cache-resident edges are not RLS-filtered:

SQLite.loadNodeVicinitySync applies the RLS clause to both Nodes and Edges: Edges.user_id is written from edge.properties.userId and the lazy-load edge query filters by user_id, sharedEntity, or visibility. The new isNodeRlsVisible() predicate checks only node properties, and the return-boundary methods then surface/traverse every cached edge whose far node is visible.

Concrete leak shape: tenant A warms a private edge between two nodes that tenant B may otherwise see, for example a team/shared endpoint. Tenant B can then receive that private relationship via getNeighbors, queryNodeTopology, or getContextFrontier, even though the SQL lazy-load path would not have returned that edge for tenant B.

Rhetorical-Drift Audit (per guide §7.4):

Findings: partial drift. The PR body says the JS predicate mirrors the SQL RLS clause and closes the 4 equivalent read paths. It mirrors the node part of the lazy-load boundary, but not the edge part. Tightening the implementation to filter edges will make the framing accurate.

🧠 Graph Ingestion Notes

• [KB_GAP]: Graph RLS needs node and edge symmetry at cache return boundaries. Node visibility alone prevents node payload leaks but not private relationship leaks when endpoint nodes are otherwise visible.
• [TOOLING_GAP]: N/A — local focused unit test and GitHub CI both ran; the gap is test coverage scope, not tooling failure.
• [RETROSPECTIVE]: Return-boundary filtering is the correct pattern for a process-wide graph cache, but graph surfaces are node+edge contracts. The security predicate should cover both cache-resident entity families whenever a method surfaces topology.

N/A Audits — 🛂 📡 🔌 🔗

N/A across listed dimensions: this PR does not introduce an externally borrowed abstraction, does not touch OpenAPI tool descriptions, does not change JSON-RPC/wire payload shapes, and does not modify skills or agent workflow conventions.

🎯 Close-Target Audit

• Close-targets identified: Resolves #10011 in the PR body.
• Syntax: newline-isolated close target in the PR body; commit messages contain (#10011) references but no magic-close keywords.
• Label check: gh issue view 10011 --json labels shows enhancement, ai, architecture; not epic.

Findings: Pass.

📑 Contract Completeness Audit

#10011 is an older focused security ticket rather than a modern Contract Ledger ticket, but the consumed behavior is explicit in the ticket tasks: graph reads must enforce tenant isolation. The PR implements that contract for nodes and needs to finish it for edges before the contract is complete.

Findings: Request Changes — edge RLS is part of the same graph-read contract because Edges carry user_id and are filtered in the SQL lazy-load path.

🪜 Evidence Audit

The PR body declares L2 evidence and lists seven unit scenarios covering cache-warmed node leakage across getNode, getNeighbors, queryNodeTopology, and getContextFrontier. L2 is the right evidence class for this return-boundary predicate.

Findings: Request Changes — current tests do not include a cache-warmed private edge whose endpoint nodes are visible to the requester. Add at least one edge-RLS scenario for relationship-surfacing methods; otherwise the evidence does not cover the full SQL RLS mirror.

📜 Source-of-Authority Audit

The PR body cites night-shift operator direction and GPT readiness-overlay context as coordination/rationale. The technical merge-blocker here does not depend on those authority citations; it is grounded in the live code paths: SQLite.addEdges, SQLite.loadNodeVicinitySync, and GraphService return-boundary loops.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Branch checked out locally via checkout_pull_request for PR #11672.
• Canonical location: new test file is under test/playwright/unit/ai/services/memory-core/, which matches the right-hemisphere unit-test convention for AI service tests.
• Ran focused local test: npm run test-unit -- test/playwright/unit/ai/services/memory-core/GraphService.TenantIsolation.spec.mjs — 7 passed.
• Coverage gap: tests cover node visibility but not edge visibility.

Findings: Tests pass locally, location pass, but coverage gap feeds the Required Action.

🛡️ CI / Security Checks Audit

Ran gh pr checks 11672. All checks passed: Analyze (javascript), CodeQL, check, integration-unified, lint-pr-body, and unit. I waited while integration-unified was pending and did not post the formal review until it completed green.

Findings: Pass — all checks green.

📋 Required Actions

To proceed with merging, please address the following:

• Add edge-level RLS filtering at the cache return boundary. Mirror the same owner/null/shared/team semantics for cached edge.properties.userId, edge.properties.sharedEntity, and edge.properties.visibility, and apply it before surfacing or traversing cached edges in relationship/topology methods such as getNeighbors, queryNodeTopology, and getContextFrontier.
• Add focused unit coverage for a cache-warmed private edge whose endpoint node(s) remain visible to the requester. The test should prove the private edge is not returned or traversed, matching SQLite.loadNodeVicinitySync edge filtering.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 85 — 15 points deducted because the return-boundary pattern is architecturally right, but applying it only to nodes misses the graph's edge half even though the storage layer enforces RLS on both tables.
• [CONTENT_COMPLETENESS]: 90 — 10 points deducted because the PR body and JSDoc accurately explain the node-cache leak, but the "mirrors SQL RLS" framing currently omits edge RLS.
• [EXECUTION_QUALITY]: 70 — Local focused test passed 7/7 and gh pr checks 11672 is fully green, but the private-edge leak is a functional security gap in topology-return methods.
• [PRODUCTIVITY]: 80 — The PR closes most of #10011 task 3 by filtering cache-resident nodes across the right read primitives; the remaining edge gap blocks full closeout.
• [IMPACT]: 80 — Major tenant-isolation/security fix for the Native Edge Graph read layer, directly affecting multi-tenant Memory Core safety.
• [COMPLEXITY]: 45 — Medium-low implementation footprint: one service file and one focused spec, but the graph entity model requires node+edge symmetry across multiple public read methods.
• [EFFORT_PROFILE]: Heavy Lift — security impact is high relative to the small diff, and the correctness burden is higher than the line count because cache behavior bypasses the SQL enforcement path.

The PR should be straightforward to repair: introduce an edge visibility predicate parallel to isNodeRlsVisible, gate cached edges before adding them to returned topology, and add the missing private-edge leak test.