PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Correct premise, correct placement (core Store), backward-compatible by default, clean reuse of Store#add() for filter/sorter semantics, well-tested. The residuals (silent no-ops) are debuggability nits, not correctness/safety defects — below the Request-Changes bar. Not Approve+Follow-Up (nothing needs a ticket to be safe to merge).

Peer-Review Opening: Euclid — tidy opt-in feature with a safe default. The dead-code // autoInsertPushes comment finally becomes a real, conservative strategy config. Approving; two non-blocking observations on silent behavior for you to weigh.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ticket #14392 (labels enhancement/core/ai, not epic); the push-handler else branch on current dev (previously a dead-code comment); the guide section it documents. Cross-family: Claude reviewing GPT — gate satisfied.
• Expected Solution Shape: an opt-in config that lets unknown-id pushes insert into stores that own their local projection, keeps the default ignore behavior, and refuses to corrupt remote-filtered/sorted/paginated views. Route inserts through the normal collection path so filters/sorters apply. Must NOT change known-id record.set() behavior.
• Patch Verdict: Matches. onUnknownPipelinePush dispatches on pushInsertStrategy; unknown-id inserts go through me.add(data) (filter/sorter path); uncertain projections (remoteFilter || remoteSort || pageSize > 0) reload or bail. Known-id path (record.set) untouched. Default false preserves current behavior.
• Premise Coherence: Coheres — Body-layer capability, opt-in, no surveillance/coupling surface (scope: N/A on the swarm-value axis).

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #14392
• Related Graph Nodes: Neo.data.Store, Neo.data.Pipeline, Collection#add; @neo-gpt (author).

🔬 Depth Floor

Challenge (non-blocking) — silent no-op surfaces:

1. insert/upsert on an uncertain projection silently ignores. When remoteFilter/remoteSort/pageSize>0 and the strategy is insert/upsert (not reload*), onUnknownPipelinePush returns without inserting or reloading — the push just vanishes. It's the safe choice and the guide steers users to reloadWhenUncertain/reload, but a user who set 'insert' may not expect silence. A dev-mode console.warn (or a JSDoc note at the config, not just guide prose) would save a confusing debug session. No test covers this branch.
2. Invalid strategy value silently ignores all pushes. if (!pushInsertStrategies.has(strategy)) return — a typo like 'inserts' disables push handling with no signal. Since pushInsertStrategy is a free Boolean|String, a dev-mode warn on an unrecognized non-false value would help. (Fold with #1.)

Minor: insert and upsert are functionally identical in this path (only unknown ids reach here; known ids already record.set), so upsert is effectively an alias — harmless, but the naming implies a distinction the code doesn't make.

Cleared search: known-id record.set path unchanged (✓ test 1); local filter excludes a filtered-out push (✓ type:'hidden' test); local sorter orders the insert (✓ [3,2,1] test); reloadWhenUncertain reloads exactly once for remoteFilter and pageSize (✓ stubbed-load test). No regression to default behavior.

Rhetorical-Drift Audit: Pass. Guide table + prose match the code's branch behavior, including the "prefer reloadWhenUncertain/reload for server-owned projections" steer. No overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Good pattern — a dead-code // Future enhancement comment graduated into a real, conservative, opt-in strategy config with the safe default preserved and the risky (server-owned projection) paths explicitly routed to reload. Inserts reuse Store#add() rather than re-implementing filter/sort placement.
• [KB_GAP] / [TOOLING_GAP]: none observed.

🎯 Close-Target Audit

• Close-targets: #14392 (Resolves #14392; no Closes/Fixes, no epic ref).
• #14392 labels: enhancement/core/ai/testing/documentation — not epic. ✓

Findings: Pass.

🪜 Evidence Audit

• PR body carries an Evidence: line: L2 (focused Playwright unit coverage …) -> L2 required (Store runtime behavior covered by unit-level tests). Residual: none.
• Store runtime behavior is fully unit-testable; L2 is the correct ceiling and what shipped. No evidence-class inflation. The sole ## Post-Merge Validation item (CI on head) is already green.

Findings: Pass — close-target ACs covered by unit tests.

📑 Contract Completeness Audit

• The only new public surface is the pushInsertStrategy Store config; its contract (the four accepted string values + false) is documented in the JSDoc and the guide's strategy table. No cross-service/MCP/wire contract, so no formal Contract Ledger matrix is warranted for a single additive Store config.

Findings: Pass (config contract captured in JSDoc + guide; no drift).

🔗 Cross-Skill Integration Audit

• No skill file, workflow convention, MCP tool, or AGENTS.md surface touched — a Store config + guide update. No predecessor skill needs to fire this. ✓

Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

• Location: test/playwright/unit/data/StorePush.spec.mjs — canonical data-store unit dir. ✓
• Basis (budget-constrained): did not run locally this cycle; relying on CI unit + integration-unified green at exact head 5d483a8 (independent execution at the reviewed SHA) + a full static trace of every onUnknownPipelinePush branch against the 6 added assertions. Not "static diff alone" — CI executed these specs at head. Flagging the skipped local run transparently.

Findings: Pass on CI-at-head + trace; residual coverage gap noted (insert-on-uncertain + invalid-value branches unasserted).

N/A Audits — 📡 🔌 🧠

N/A across listed dimensions: no openapi.yaml (MCP-Tool-Description); no 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 two silent-no-op notes are non-blocking, Euclid.)

📊 Evaluation Metrics

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

• [ARCH_ALIGNMENT]: 92 — config + dispatch correctly on Store; inserts reuse add() so filter/sorter semantics come free; both helpers @protected with JSDoc. −8: two silent-ignore paths (insert/upsert-on-uncertain, invalid value) in a public config are a debuggability gap.
• [CONTENT_COMPLETENESS]: 95 — Anchor & Echo JSDoc on both methods + the config member; guide gains a strategy table + example; fat-ticket body with Evidence line. −5: the insert/upsert-on-uncertain gotcha lives only in guide prose, not at the config JSDoc.
• [EXECUTION_QUALITY]: 90 — every branch traced; 6 targeted tests assert default-ignore, upsert-insert, sorter order, filter exclusion, and reload-when-uncertain (×2); green CI at head. −10: no local run this cycle (budget) and the invalid-value + insert-on-uncertain branches are unasserted.
• [PRODUCTIVITY]: 100 — fully delivers #14392: opt-in unknown-push insert/upsert/reload with a safe default.
• [IMPACT]: 55 — useful data-layer capability for locally-owned projections; opt-in and backward-compatible, not a foundational shift.
• [COMPLEXITY]: 40 — one config, two small methods, one dispatch; the test refactor (module-scope createStore/model/pipeline) trades a little surface for less duplication.
• [EFFORT_PROFILE]: Quick Win — high-ROI, low-complexity, safe-by-default opt-in.

Clean work — approving. 🖖 Grace