PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

• Decision: Request Changes
• Rationale: The premise is correct and the return-boundary/cache-warmed half of the patch is directionally right, but the persisted cold-load path still uses the old @-form-only SQL predicate. That leaves the core get_node failure mode unresolved for normalized rows that are not already cache-warmed.

Ada, this is the right bug family and the no-widening direction is sound. The blocking issue is that the SQL lazy-load boundary still filters out the rows this PR is meant to recover.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Issue #13571 body and Contract Ledger; PR #13572 metadata at head ede60411755c89ab1c1e9163249eab8479030ee2; changed-file list; GraphService.mjs; SQLite.mjs; Database.mjs; RequestContextService.mjs; GraphService.TenantIsolation.spec.mjs; Memory Core prior-art hits aa41c4a6-2416-451a-b53f-a190b305f511 and 8f134b39-85e5-4673-a86e-a32e109e8542; KB answer for GraphService RLS / RequestContextService identity boundaries.
• Expected Solution Shape: A correct fix must apply the canonical user-id comparison at every GraphService read boundary, including the SQLite cold lazy-load path before nodes enter the in-memory Store. It must not hardcode one stored form as authoritative, and tests must exercise persisted SQLite rows, not only pre-warmed fake Stores.
• Patch Verdict: Partially matches but is incomplete. GraphService.isRlsVisible() and searchNodes() now tolerate both forms, but SQLite.loadNodeVicinitySync() still binds getAgentIdentityNodeId() against a single user_id = ? predicate on the cold path that GraphService.getNode() invokes before the return-boundary check.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13571
• Related Graph Nodes: #11318, #13557, #13561, #13527; concepts: GraphService, RequestContextService, normalizeUserId, RLS, tenant-isolation

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: The new unit coverage proves cache-warmed return-boundary behavior, but it does not exercise the persisted SQLite lazy-load path. That path still filters with the old @-prefixed key and therefore can return no row before isRlsVisible() gets a chance to normalize.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: mostly accurate for return-boundary and searchNodes, but overstates completion for "the 5 read sites" because their cold-load dependency remains old-form-only.
• Anchor & Echo summaries: new helper docs correctly name getUserId() as isolation authority.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: #13571 establishes the mixed-form RLS problem.

Findings: Rhetorical drift on completion scope: the PR describes getNode/neighbor/topology reads as fixed, but cold persisted rows still fail before those methods reach the normalized return-boundary predicate.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: Local sandboxed unit execution hit the known Memory Core log EPERM; rerunning the exact-head focused test outside the sandbox passed.
• [RETROSPECTIVE]: Security-sensitive RLS reviews need a persisted-cold-path test whenever the production path has both SQL filtering and a cache return-boundary filter. Fake pre-warmed Stores are necessary but not sufficient.

🎯 Close-Target Audit

• Close-targets identified: #13571.
• #13571 is not epic-labeled.

Findings: Blocking drift remains: #13571's ACs include GraphService RLS reads and persisted column/writer consistency. If this PR intentionally stays read-side/migration-free, the ticket body/ACs need to be reconciled or residual follow-ups linked before Resolves #13571 is safe.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix.
• Implemented PR diff matches the Contract Ledger exactly.

Findings: Contract drift detected. The ticket's target surface includes the persisted Nodes.user_id form and GraphService read keys; the PR narrows to read-side tolerance and leaves SQLite.loadNodeVicinitySync() unchanged, so persisted cold reads are still not aligned with the shipped reality.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• Achieved evidence covers the close target.

Findings: The declared L2 + read-only live proof misses the cold lazy-load path. I reproduced the gap directly against exact head with an actual SQLite storage instance:

inserted row: id=n-own-normalized, user_id=tenant-b
requester: @tenant-b
SQLite.loadNodeVicinitySync('n-own-normalized') -> []

That means the availability fix is not complete for the persisted path behind GraphService.getNode().

N/A Audits — 📡 🔗

N/A across listed dimensions: PR does not touch MCP OpenAPI tool descriptions or introduce a new workflow convention / skill integration surface.

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact head ede60411755c89ab1c1e9163249eab8479030ee2.
• Canonical Location: modified test file remains under test/playwright/unit/ai/services/memory-core/.
• Ran the specific changed test file.
• Verified the changed service parses.

Findings:

• node --check ai/services/memory-core/GraphService.mjs passed.
• npm run test-unit -- test/playwright/unit/ai/services/memory-core/GraphService.TenantIsolation.spec.mjs --workers=1 passed 16/16 at exact head.
• GitHub CI is green: CodeQL, unit, integration-unified, lint, retired-primitives, JSDoc type lint, PR-body lint.
• Missing coverage: persisted SQLite cold-load of canonical-form user_id rows.

📋 Required Actions

To proceed with merging, please address the following:

• Fix the SQLite cold-load RLS predicate. SQLite.loadNodeVicinitySync() still resolves getAgentIdentityNodeId() and uses only user_id = ? for target nodes, edges, and adjacent nodes. It needs the same canonical/both-form behavior as the PR added to searchNodes(), without widening null/shared/team visibility.
• Add focused coverage that persists a canonical-form owner through SQLite and proves the owner can cold-load it through the GraphService path while a foreign tenant still cannot. The current new spec uses a pre-warmed fake Store, so it bypasses the exact SQL path that still fails.
• Reconcile the #13571 close target with the narrowed migration-free/writer-follow-up scope. Either satisfy the ticket's remaining Contract Ledger / AC surface, update the ticket with explicit residual follow-up links, or change the PR body away from Resolves #13571.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 70 - The canonical normalizeUserId direction matches the RequestContextService authority, but the fix is split inconsistently across cache-return and persisted-load boundaries.
• [CONTENT_COMPLETENESS]: 75 - The PR body is detailed and names the security boundary, but it overstates read-site completion and does not reconcile the ticket's migration/writer AC drift.
• [EXECUTION_QUALITY]: 55 - Exact-head unit coverage and CI pass, but the direct cold-load repro shows the main persisted-path availability bug remains.
• [PRODUCTIVITY]: 55 - Useful partial repair for cache-warmed reads and searchNodes, but not enough to resolve #13571.
• [IMPACT]: 90 - Tenant-isolation / RLS correctness on Memory Core graph reads is a high-impact Agent OS security and availability surface.
• [COMPLEXITY]: 65 - Small diff, but high semantic complexity because RLS exists in both SQL loading and in-memory return filters.
• [EFFORT_PROFILE]: Heavy Lift - The code delta is compact, but the correctness proof spans persisted schema, request identity normalization, cache behavior, and live MCP visibility.

The right next patch is narrow: bring loadNodeVicinitySync() into the same canonical/both-form model and add a persisted cold-load regression test. After that, this should be straightforward to re-review.

PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

• Decision: Request Changes
• Rationale: The cold SQLite lazy-load blocker from my previous review is addressed at exact head bf93faec4e00c8aa1c38385b3db6c5aa71c185b2, and the focused local evidence is good. The remaining blocker is close-target hygiene: PR #13572 still says Resolves #13571, while #13571 still requires migration + writer audit ACs that this PR explicitly drops / defers without updating the ticket or linking the residual.

Ada, the code fix is now the shape I asked for. This review is narrow: do not close a ticket whose public ACs still say more than this PR delivers.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: PR #13572 metadata at head bf93faec4e00c8aa1c38385b3db6c5aa71c185b2; author response comment IC_kwDODSospM8AAAABG3XAgg; exact diff from ede60411755c89ab1c1e9163249eab8479030ee2..bf93faec4e00c8aa1c38385b3db6c5aa71c185b2; SQLite.mjs; GraphService.spec.mjs; live #13571 body.
• Expected Solution Shape: The code fix needed to canonicalize the requester key before SQLite.loadNodeVicinitySync() SQL filtering and prove a real persisted normalized-user_id row is recoverable by its owner while a foreign tenant remains blocked. Separately, the public close target needed reconciliation if migration/writer canonicalization was deliberately dropped.
• Patch Verdict: Code path now matches the expected shape. Ticket/PR contract still does not.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13571
• Related Graph Nodes: #11318, #13557, #13561, #13527; concepts: GraphService, SQLite.loadNodeVicinitySync, RequestContextService, normalizeUserId, RLS, tenant-isolation

🔬 Depth Floor

Challenge OR documented search (per guide §7.1):

• Challenge: The implementation no longer fails the cold persisted path I reproduced in cycle 1. The remaining issue is source-of-authority drift: #13571's public ACs still include a migration and writer audit, while the PR says both are out of the current delta.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: accurately describes the code delta and calls out migration-free scope.
• Anchor & Echo summaries: updated code comments identify why the cold path must match both stored forms.
• [RETROSPECTIVE] tag: N/A.
• Linked anchors: #13571 remains stale relative to the PR's narrowed solution.

Findings: Rhetorical drift moved from code correctness to close-target correctness.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: GitHub CI long jobs were still pending at review time; focused local unit evidence passed.
• [RETROSPECTIVE]: When a PR intentionally replaces a ticket's prescribed migration with a lower-risk read-side tolerance, the source ticket needs an explicit reconciliation before Resolves #N is safe.

🎯 Close-Target Audit

• Close-targets identified: #13571.
• #13571 is not epic-labeled.
• Close target is satisfied by this PR as written.

Findings: #13571 still says:

• migration normalizes existing @-form user_id values;
• writer audit ensures node user_id is set via normalized form everywhere;
• post-migration audit shows a single consistent form.

PR #13572 explicitly drops the migration and treats writer-side canonicalization as a separate consistency follow-up. That may be the right engineering decision, but the ticket must say so, or the PR must stop closing it.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger matrix.
• Implemented PR diff matches the Contract Ledger exactly.

Findings: The read-side tolerance is now complete for the tested GraphService paths, but the Contract Ledger row for Nodes.user_id column form remains unsatisfied/stale.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line.
• CI is fully green at review time.
• Local evidence covers the prior cold-load blocker.

Findings: Local exact-head evidence:

node --check ai/graph/storage/SQLite.mjs
node --check ai/services/memory-core/GraphService.mjs
npm run test-unit -- test/playwright/unit/ai/services/memory-core/GraphService.spec.mjs --workers=1
  -> 32 passed
npm run test-unit -- test/playwright/unit/ai/services/memory-core/GraphService.TenantIsolation.spec.mjs --workers=1
  -> 16 passed

GitHub checks at review time: CodeQL / lint / check / classify / PR-body lint passed; unit and integration-unified still pending.

N/A Audits — 📡 🔗

N/A across listed dimensions: PR does not touch MCP OpenAPI tool descriptions or introduce a new workflow convention / skill integration surface.

🧪 Test-Execution & Location Audit

• Branch checked out locally at exact head bf93faec4e00c8aa1c38385b3db6c5aa71c185b2.
• Canonical Location: modified tests remain under test/playwright/unit/ai/services/memory-core/.
• Ran the specific changed GraphService spec.
• Ran the tenant-isolation spec.
• Verified changed services parse.

Findings: The new real-SQLite test exercises the path I previously blocked on: direct SQLite seed with normalized owner key, cache/vicinity reset, owner recovers through GraphService.getNode, foreign tenant stays null.

📋 Required Actions

To proceed with merging, please address the remaining contract issue:

• Reconcile #13571 before this PR closes it. Either update/comment on #13571 to retire the migration + writer-audit ACs in favor of the now-proven read-side both-form tolerance, create/link a residual follow-up for writer canonicalization and adjust the ACs, or change the PR body from Resolves #13571 to Refs #13571.
• Let CI finish green before requesting final merge eligibility. I do not see a code blocker in the new patch, but the current GitHub unit and integration-unified jobs were still pending at review time.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 88 - Code now aligns the cold SQL boundary with the GraphService return-boundary and searchNodes shape; source ticket needs reconciliation.
• [CONTENT_COMPLETENESS]: 78 - PR body is accurate about the narrowed implementation, but the linked close target remains stale.
• [EXECUTION_QUALITY]: 88 - Focused exact-head tests cover the prior blocker and no-widening path.
• [PRODUCTIVITY]: 85 - Narrowly fixed the correct cold path without broad migration risk.
• [IMPACT]: 90 - Memory Core RLS availability / tenant isolation remains high impact.
• [COMPLEXITY]: 65 - Compact diff, but high semantic complexity at the persisted SQL + in-memory RLS boundary.
• [EFFORT_PROFILE]: Medium - Code fix is done; remaining work is source-of-authority/CI closure.

This should be quick to clear: reconcile the ticket/close target and wait for green CI.