PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The three other shapes are all wrong here. Request Changes would be nitpick-theater — the diff does exactly what its ticket says, its regression is stronger than the minimum, and my one finding is pre-existing code this PR never touches. Approve+Follow-Up is the worst normal outcome and would be actively harmful in this chain: #15825 → #16273 → #16275 for three lines of one function in a single afternoon is precisely the fragmentation we push back on, so I have put the sibling fix in front of the author as a fold-in suggestion rather than minting a third ticket. Drop+Supersede does not apply — no §9.0 structural trigger fires: the premise is valid (I raised it), the ticket is same-day fresh, the author did not bypass anything, and there is no better existing substrate. Approve, with the merge gate resting on the pending Tests run rather than on this review.

Peer-Review Opening: Thanks for turning my #16272 side-note into a proper regression rather than a drive-by patch — the identity assertion you added is the part that makes this durable. One pre-existing finding three lines below your fix, non-blocking, plus a correction to a citation I got wrong in the review that spawned this ticket.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16273 body; changed-file list; ai/services/memory-core/GraphService.mjs linkNodes() on current dev (lines 537–620) before treating the diff as evidence; src/data/RecordFactory.mjs set/reset/setSilent surface; src/core/Base.mjs:1070; ai/graph/Database.mjs:40-70 for the edges_ config contract; ai/graph/storage/SQLite.mjs:395-450 for sibling isRecord precedent; my own #16272 review, which is the source of this ticket.
• Expected Solution Shape: One-line setter-arity correction at the hydrated branch, plus a regression that hydrates a real edge, relinks it, and compares the cached Record against the durable SQLite row. It must not repair coherence by replacing the cache entry — that would pass a naive test while breaking every holder of the Record reference. Test isolation: real SQLite, no mocked Record.
• Patch Verdict: Matches, and improves on the expected shape in one respect I would not have specified — expect(GraphService.db.edges.get(initialRow.id)).toBe(cachedEdge) pins object identity, closing exactly the replace-instead-of-update escape hatch above. Confirmed against the diff at 05b7ee94674f7ebf290596a2af49040f62ca4b42.
• Premise Coherence: coheres: verify-before-assert. The ticket exists because a reviewer read the call shape at source instead of trusting that a two-arg setter meant what it looked like; the PR closes it by re-deriving the arity rather than pattern-matching the sibling branch. It also coheres with friction→gold — a review finding became a ticket became a falsifiable regression, without the finder implementing it.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16273
• Related Graph Nodes: #15825 (originating defect), PR #16272 (predecessor, where I reported this sibling), Neo.data.RecordFactory, Neo.ai.services.memory-core.GraphService, edge-cache-coherence
• Origin Session ID: 56105163-6e66-44b6-8c6f-9e81bc1be08c

🔬 Depth Floor

Challenge: The else branch three lines below your fix writes a different weight than the durable row, on every relink — details and evidence in Required Actions. Summarised here because it is the thing to watch: this PR establishes RAM/SQLite coherence for the hydrated shape while the non-hydrated shape in the same if remains incoherent, so the function is now half-guaranteed. I am not blocking on it and I did not establish that the branch is reached at runtime.

Second, smaller: I also owe a correction on my own #16272 review — I cited src/core/Base.mjs:1070 set(values={}) as the mechanism. For a hydrated edge the applicable setter is src/data/RecordFactory.mjs:310 set(fields), which forwards to notifyChange({fields, model, record: this}); the Record shadows core.Base here. Same arity, so the conclusion held and your fix is right, but the pointer was to the wrong layer. Recording it so it is not inherited by the next reader of that review.

Rhetorical-Drift Audit:

• PR description: framing matches what the diff substantiates (no overshoot)
• Anchor & Echo summaries: no new architectural prose added by this diff
• [RETROSPECTIVE] tag: none claimed
• Linked anchors: #16273 and #15825 do establish the claimed lineage — verified both

Findings: Pass. "keeping the active RAM object identical to SQLite after a relink" is precisely what the spec proves, scoped to the hydrated branch; the body does not claim the function as a whole is coherent, which would have been an overshoot given the finding below.

🧠 Graph Ingestion Notes

• [KB_GAP]: Neo.data.Record's bulk setter is set(fields) — one object — and it shadows core.Base.set(values={}). Both silently discard a two-arg call: no throw, no log, no dirty flag. This subsystem has now produced the same defect twice (#15825, #16273). The durable lesson is the silence, not the arity.
• [TOOLING_GAP]: A two-arg call on a one-arg bulk setter is statically detectable and produced two separate production defects. If JSDoc Type Lint or an ESLint rule can flag .set(<string>, …) on a Record/core.Base receiver, that is a mechanical guard worth more than the two fixes combined.
• [RETROSPECTIVE]: The review→ticket→PR chain worked here without the finder implementing the fix. I reported the sibling in #16272 with no consequence claimed because I had not established one; Emmy filed it, proved the consequence with a storage-backed regression, and fixed it. That separation is worth keeping — an unproven finding is still worth reporting as long as it is labelled as unproven.

N/A Audits — 📑 🪜 📡 🔗

N/A across listed dimensions: internal RAM-cache coherence fix with no public/consumed surface, no OpenAPI or skill/convention surface, and close-target ACs fully covered by the storage-backed unit regression (PR body declares Evidence: L2 … → L2 required, which matches).

🎯 Close-Target Audit

• Close-targets identified: #16273
• For each #N: confirmed not epic-labeled — #16273 carries bug, ai, testing

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI at 05b7ee9467 is 7/8 green with Tests still in_progress — Body Lint, Retired Primitives, AiConfig Antipattern, AiConfig Test-Mutation, JSDoc Type, CodeQL, Ticket Archaeology all success. Not green yet; see Required Actions. Author per-surface receipt present and appropriate (focused RED/GREEN plus GraphService.spec.mjs 51/51).
• Reviewer falsifier: run, with a named concern — below.
• Test location: pass. test/playwright/unit/ai/services/memory-core/GraphService.spec.mjs mirrors the source path.

Reviewer falsifier — named concern: does the new spec actually distinguish the two behaviours, or does it merely fail on an absent capability? Mine on #16267 were the weaker class and I said so there, so I checked yours rather than assume.

Checked out 05b7ee9467, reverted only line 610 back to ramEdge.set('properties', newProps), re-ran:

npx playwright test -c test/playwright/playwright.config.unit.mjs GraphService --grep "16273"
  1 failed  2 passed
  GraphService.spec.mjs:170  expect(received).toEqual(expected)   - Expected -2 / + Received +2

It fails on the deep-equality assertion with both phase and weight stale, while the identity assertion at :169 still passes. So the discriminating power sits on the RAM-vs-durable comparison and it is a genuine behavioural falsifier, not the absent-capability class. Restored, then GREEN at head: 77 passed.

Instrument correction: my first attempt at this ran 76 passed and I nearly reported it. I had fetched the PR ref without checking it out, so I had tested origin/dev — unfixed line, spec not present. Every result above is from 05b7ee9467 actually on disk, confirmed by grep -c 16273 on the spec and by reading line 610.

Findings: Pass on author evidence, spec placement, and falsifier. Required-CI Tests still pending.

📋 Required Actions

To proceed with merging, please address the following:

• Wait for the Tests check to complete at 05b7ee9467. My local run covers the GraphService surface, but it is not the required check and CI-pending is not CI-green. No action expected from you beyond letting it finish — flagged so the merge gate is not read as satisfied.

Non-blocking, author's call — the else branch at GraphService.mjs:612-613:

ramEdge.properties.weight = newWeight;              // 612
Object.assign(ramEdge.properties, edgeProperties);  // 613

Line 577 builds edgeProperties = {weight, ...properties}, so a weight key is always present — the raw per-call increment argument, not the accumulated newWeight. Object.assign therefore always clobbers line 612, making line 612 dead: its value never survives the next statement. Replaying all three write paths with real values (weight=1, currentWeight=1 ⇒ newWeight=1.1):

Same coherence property, sibling branch — and unlike the bug you fixed, this one is not a silent no-op, it writes a wrong value.

What I have not established: that the branch is reached at runtime. I checked before asserting and the honest answer is that it is not obviously dead — ai/graph/Database.mjs:54 types the config {Object|Neo.data.Store|null}, and the dual shape is handled deliberately across the subsystem, including for edges at ai/graph/storage/SQLite.mjs:440-443. Real defect, unproven reachability, no live consequence claimed.

Suggestion: fold it in here — reorder to Object.assign(...) then ramEdge.properties.weight = newWeight, deleting the dead line. If you would rather hold this PR at exactly its ticket scope, that is defensible and I am not holding the approval for it either way.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 - Fix sits at the exact call site of the defect, uses the Record's own documented bulk setter, and changes no control flow. Placement of both source and spec is correct. Not higher only because the sibling branch leaves the function's coherence guarantee half-applied.
• [CONTENT_COMPLETENESS]: 90 - Close-target ACs fully met and evidenced. The gap is scope-adjacent, not missing work.
• [EXECUTION_QUALITY]: 96 - One-line source change, no collateral. The regression is better than the minimum: real SQLite, real hydration, identity assertion, full toEqual against the durable row rather than toMatchObject. RED independently re-derived.
• [PRODUCTIVITY]: 95 - Ticket filed and PR delivered inside an hour of the finding, with the ticket authored by the fixer rather than the finder.
• [IMPACT]: 80 - Silent cache staleness in the graph layer that every MC reader consults; low blast radius per occurrence, high confusion cost when it surfaces, as #15825 demonstrated.
• [COMPLEXITY]: 20 - One line of source. The difficulty was entirely in seeing it.
• [EFFORT_PROFILE]: Quick Win - a one-line correction with a durable regression, and no debt added.

Approving on substance; the merge gate is the pending Tests run, not this review. The #16272 → #16273 → this PR chain is the shape I would want repeated: a finding reported without an over-claimed consequence, picked up by someone else, and proven before being fixed.

Reviewed by Ada (Claude Opus 5, Claude Code).