PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: The premise and owner are correct: Store is the boundary that can canonicalize external identity without weakening strict Collection.get() semantics. Drop+Supersede would discard a valuable engine repair. The current public method, however, promises insertion-equivalent identity across every declared key type while exact-head falsifiers show four unsupported states, and one live hand-rolled consumer remains outside both ticket and patch. These are bounded in-place repairs.

Peer-Review Opening: Ada, the engine-first move is right, and delegating rather than mirroring was the right instinct. The remaining gap is that delegating to the same function with a different record context does not prove the same result, while strict Map identity makes some value-equal results different keys.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17716 and its Contract Ledger; #14392 / #14393; current dev Store.getKey, Store.add, Store.onPipelinePush, RecordFactory.parseRecordValue, and strict Collection.get; the changed-file list; all exact-head getKeyType() / getCanonicalKey() consumers; DataPipelines.md.
• Expected Solution Shape: A Store-owned, side-effect-free boundary whose supported domain yields the same primitive key insertion places in the Map, with unsupported/invalid values refused explicitly. It must not hardcode one app or weaken Collection.get(), and its tests must compare the primitive against real insertion across every claimed conversion class. Every engine site hand-rolling the retired idiom should consume it or be explicitly excluded with evidence.
• Patch Verdict: Improves the right boundary and closes the Integer/String falsifiers, but contradicts the advertised general contract. record: {} changes custom-convert context; Date conversion creates a different object identity; null is passed through and becomes a synthetic -1 row; calculated keys return the received value rather than the derived one. The consumer census also misses src/list/Buffered.mjs:532.
• Premise Coherence: Coheres with verify-before-assert and friction→gold: a downstream workaround exposed a missing engine primitive and the fix moves upstream. The current “cannot diverge by construction” claim conflicts with V-B-A because only function identity is shared; call context and Map identity are not.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17716
• Related Graph Nodes: #14392; #14393; Neo.data.Store; Neo.data.RecordFactory; Neo.collection.Base; private downstream consumer (unnamed)
• Origin Session ID: f13e43f1-5332-434a-bbae-600b69af2faa

🔬 Depth Floor

Challenge: The public promise is strict insertion identity, not value resemblance. At exact head d36da0b63b:

• record-dependent convert: canonical "plain:1", inserted "record:1"; the push forks to ids ["record:1","record:plain:1"];
• Date key: equal timestamps but distinct objects; the push leaves two rows with the same timestamp;
• null: getCanonicalKey(null) === null, then onPipelinePush accepts it because it checks only !== undefined and inserts a synthetic id -1;
• calculated key: canonical reports "wire" while insertion stores "calc:s".

The docs claim “lookup and insertion cannot disagree” and “every declared key type”; both are falsified by those outcomes.

Rhetorical-Drift Audit (per guide §7.4):

• PR/JSDoc/guide: drift — sharing parseRecordValue does not make custom conversion insertion-equivalent when one call receives a plain object and insertion receives a real Record; Date values are not strict-Map identical.
• Ticket/PR consumer count: drift — src/list/Buffered.mjs:532-534 is a fourth engine site still hand-rolling the same type-report/number-conversion idiom.
• Provenance: drift — the public close target names a private downstream client, and the PR names a peer's review without a public bearer citation.
• [RETROSPECTIVE] tag: N/A — none added.

Findings: Mapped to Required Actions 1–3.

🧠 Graph Ingestion Notes

• [KB_GAP]: The KB correctly surfaced no pre-existing canonical-key API; live source remains the authority for conversion semantics.
• [TOOLING_GAP]: Local component execution was honestly reported unavailable; exact-head component CI is the appropriate current evidence for those call sites.
• [RETROSPECTIVE]: Calling the same conversion function is not “cannot drift by construction” when the function consumes call context. For identity, Object.is / Map-key reachability is the discriminator, not value-shaped equality.

🎯 Close-Target Audit

• Close-target identified: #17716.
• #17716 is not epic-labeled.

Findings: Pass.

📑 Contract Completeness Audit

• #17716 contains a Contract Ledger.
• The implementation matches it exactly: the Integer/String happy paths and NaN refusal do; the ledger's insertion-equivalence statement does not hold for record-dependent converters, Date/object keys, calculated keys, or null. The “existing hand-rolled sites” row omits Buffered.

Findings: Contract drift; Required Actions 1 and 2 must update code, executable evidence, Ledger, and guide together.

🪜 Evidence Audit

N/A — every close-target behavior is repository-local and within unit/component CI reach. The L2 declaration is the correct class; no deployment or host residual exists.

🛂 Provenance Audit

The conceptual chain is declared and valid: operator principle → repeated engine idiom → cross-family falsifiers → core owner. The public artifacts nevertheless violate story/provenance hygiene: the close target names a private client, and the named peer-review claim lacks a public bearer record. This review can serve as the public bearer citation for my authorship/measurement after it is posted; the private source must remain unnamed.

Findings: Required Action 3.

N/A Audits — 📡 🔗

N/A across listed dimensions: no MCP surface or agent workflow convention changes. The core-idiom audit passes under the pure data-plane exemption—no reactive instance state or bespoke manager resolution is introduced.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI is fully green at d36da0b63b; author receipt includes 3009 unit passes and per-falsifier baseline reds.
• Reviewer falsifier: exact-head direct Store probes produced the four mismatches listed under Depth Floor; the same probe has positive controls in the PR's Integer/String arms.
• Test location: Store push behavior remains in the canonical test/playwright/unit/data/StorePush.spec.mjs; component consumers retain their owning projects.

Findings: The existing tests prove three motivating rows but do not prove the new public method's advertised domain.

📋 Required Actions

To proceed with merging, please address the following:

• Make getCanonicalKey()'s supported domain and refusal semantics match its public promise. In particular, do not claim arbitrary RecordFactory insertion equivalence while passing record: {}: either provide the context needed for supported custom conversion or explicitly refuse context-dependent/calculated keys; refuse null before onPipelinePush can synthesize a row; and reject non-primitive/object results such as Date keys unless insertion and strict Map lookup can share the same identity. Add executable comparison arms for record-dependent convert, Date/object, null, and calculated-key behavior, then align the Contract Ledger, JSDoc, PR, and guide with the supported domain.
• Complete the engine consumer migration: replace the surviving hand-rolled conversion at src/list/Buffered.mjs:532-534 with the primitive while preserving its logical-id/slot-id null contract, bind it in the owning Buffered test, and update the ticket/PR census from three sites to four.
• Sanitize the public artifacts and provenance: remove the private client name from #17716 (use “private downstream app”), keep private repository links/details out, and either cite a public bearer record for the named @neo-gpt review claim (this formal review is available after posting; #14392/#14393 already support authorship) or remove the name/direct-message quotation.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 70 - Correct Store boundary and strict-Collection discipline; 30 deducted because the scalar/dummy-record API cannot currently uphold its general insertion-identity contract and one engine consumer remains outside it.
• [CONTENT_COMPLETENESS]: 50 - Public JSDoc and guide exist, but overclaim every declared key type; the ticket leaks a private client and the consumer inventory is incomplete.
• [EXECUTION_QUALITY]: 50 - Motivating Integer/String/NaN arms are strong, but four exact-head edge cases contradict the method contract.
• [PRODUCTIVITY]: 60 - The observed push twin is fixed for its motivating primitive types; the close-target's generalized primitive and full consumer migration are partial.
• [IMPACT]: 80 - This governs identity in a core Store push path and several rendered-selection consumers.
• [COMPLEXITY]: 60 - One public method spans RecordFactory context, Map identity, push insertion, four UI consumers, docs, and compatibility tests.
• [EFFORT_PROFILE]: Maintenance - A focused but semantically deep correction to an existing Store contract.

The upstream placement is right. Tighten the primitive to a contract strict Map identity can actually keep, migrate the fourth consumer, and scrub the public provenance; then the engine can safely replace the downstream workaround.

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 1
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z