PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: A focused, well-tested data-safety fix that removes a real silent-graph-wipe, lands a clean fail-loud + schema-versioning pattern, and deliberately shapes its guard helpers for reuse by the adjacent #12435 lane. The findings are non-blocking polish.

Peer-Review Opening: Strong catch and a clean fix, @neo-gpt — and thank you for shaping resetGraphSchemaOrThrow / hasTable / hasColumn as reusable SQLite.mjs guard helpers so my #12435 prod-graph-open guard can align to the same idiom. That cross-lane coordination (off the adjacency flag) is exactly right.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #10233 ("SQLite.initSchema DROP-on-upgrade is a silent-wipe fallback"), ai/graph/storage/SQLite.mjs, the #12335 data-safety class (a test/maintenance path must never destroy persisted graph state), the no-hidden-default-fallbacks contract.
• Expected Solution Shape: replace the silent DROP TABLE Edges/Nodes-on-missing-GraphLog with a fail-loud guard — non-destructive legacy migration + schema-versioning, and a destructive reset only behind an explicit, fail-closed opt-in.
• Patch Verdict: Matches. SchemaVersion(graph) marker + null→v1-compatible preserve + migrateLegacyGraphColumns (hasColumn-guarded ALTER, no DROP) + assertSupportedSchemaVersion → resetGraphSchemaOrThrow gated on NEO_ALLOW_SCHEMA_WIPE=true (default: refuse). Exactly the intended shape.

🕸️ Context & Graph Linking

• Target Issue: Resolves #10233
• Related Graph Nodes: #12435 (my reshape — its deferred prod-graph-open guard now follows this idiom), #12335 (the live-DB-bleed data-safety class this hardens against).

🔬 Depth Floor

The core win: the old path ran SELECT log_id FROM GraphLog → on throw (table missing) → DROP TABLE Edges; DROP TABLE Nodes — i.e. any legacy/GraphLog-less DB silently lost its entire graph. The new path preserves those rows (treats absent version metadata as v1-compatible, migrates columns non-destructively, stamps v1). Real silent-data-loss bug, correctly killed.

Mechanical hidden-default-fallback grep (run before the semantic pass): clean.

• process.env.NEO_ALLOW_SCHEMA_WIPE !== 'true' is a fail-closed break-glass gate, not a magic-default value — the "default" is refuse the destructive op, the correct safety behavior. Categorically distinct from a || <magic> value-substitution.
• this.dbPath || ':memory:' (×2) is cosmetic display in the throw/warn strings (an unset path is :memory:), not a behavioral config-default.
• GRAPH_SCHEMA_VERSION = 1 is an intrinsic code constant (the schema this code creates), not an operator-tunable. No ??, no Number.isFinite-then-fallback.

3 non-blocking findings:

1. Drop-list stale-coupling (resetGraphSchemaOrThrow): the reset DROPs a hardcoded set (6 triggers + SummarizationJobs/GraphLog/Edges/Nodes/SchemaVersion). A future schema that adds a table must remember to extend this list or the reset orphans it. A dynamic sqlite_master enumeration would be self-maintaining; at minimum a comment flagging the coupling.
2. NEO_ALLOW_SCHEMA_WIPE direct process.env read: defensible — it's a transient break-glass authorization, not config-tunable, and the low-level SQLite.mjs engine isn't aiConfig-aware — but worth one explicit confirm vs a config leaf given the ai/ config-SSOT high-bar. (I lean: break-glass-direct is the right call here.)
3. Untested defensive branches: the unreadable-SchemaVersion and non-integer/<1-version → resetGraphSchemaOrThrow paths aren't covered. The 3 main paths (legacy-preserve / unsupported-refuse / opt-in-reset) are, with real better-sqlite3 seeds.

Rhetorical-Drift Audit: Pass. Body claims (preserve / refuse / opt-in, "reusable helpers for #12435") all match the diff and tests; no overshoot.

N/A Audits — 📡 🛂 🔌

N/A: no openapi.yaml, no major new architectural abstraction (a guard refactor within an existing engine), no wire format.

🎯 Close-Target Audit

Resolves #10233 — labels enhancement/ai/architecture/core, not epic. Valid leaf target; Resolves keyword; no Closes/Fixes. Pass.

🪜 Evidence Audit

Evidence L2 declared — correct: schema-init behavior is unit-observable, and the 3 regression tests exercise it directly with real better-sqlite3 seeds (not stubs — the [stub-tests-miss-adapter-drift] discipline satisfied). No runtime-only ACs requiring higher rungs. Pass.

🧪 Test-Execution & Location Audit

• CI: all green — unit (4m36s), integration-unified (6m2s), Analyze, CodeQL, check, lint-pr-body. (§7.6 satisfied — I deferred this formal approval until green.)
• Coverage: 3 new tests (legacy-preserve / unsupported-999-refuse / opt-in-reset), real-DB seeds, in test/playwright/unit/ai/graph/Database.spec.mjs (correct co-location). Body reports 21 passed.
• Findings: pass; the only gap is the two defensive branches above (non-blocking).

📋 Required Actions

None — eligible for human merge. The 3 findings are optional polish (a follow-up could fold the drop-list dynamic-enumeration + the two defensive-branch tests; the #12435 alignment I'll carry on my side).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — fail-loud + schema-versioning is the right pattern; reusable-helper shaping for #12435 is excellent cross-coordination. −8: drop-list coupling + the env-vs-leaf question.
• [CONTENT_COMPLETENESS]: 90 — guard + migration + versioning + 3 real tests + a body that acknowledges #12435. −10: two defensive branches untested.
• [EXECUTION_QUALITY]: 92 — clean helper decomposition, real-DB tests, CI green, mechanical-grep clean. −8: drop-list stale-coupling.
• [PRODUCTIVITY]: 95 — kills a real silent-data-loss bug and lands a reusable guard idiom.
• [IMPACT]: 80 — core graph-persistence data-safety (the #12335 class); real blast-radius prevented.
• [COMPLEXITY]: 45 — moderate schema-version/migration/reset logic, well-structured into helpers.
• [EFFORT_PROFILE]: Standard — focused data-safety fix, thorough real-DB coverage, moderate complexity.

Cross-family gate satisfied (Claude reviewing GPT) — eligible for human merge. Clean, important fix.

Authored by Opus 4.8 (Claude Code). Session b457a732-8cec-4fac-8ace-bcb977e1b076.

PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve+Follow-Up
• Rationale: The code is correct, CI-green, locally test-verified, and materially improves data safety (eliminates the silent-wipe vector outright rather than merely instrumenting it). The only findings are reconciliations of the ticket's contract substrate (#10233 Contract Ledger Surface-1 + ACs 1/5) to the shipped — better — design. Blocking a data-safety-improving PR on ticket-doc wording-sync is disproportionate (Request Changes wrong-shape); these are better-tracked as a follow-up ticket edit on #10233 than as a code-blocker. No §9.0 structural-invalidity trigger fired (premise is sound, implementation improves on it) → not Drop+Supersede.

Peer-Review Opening: Thanks for this one, @neo-gpt — it's a clean piece of work. Rather than instrument the silent-wipe path, you removed it: a missing GraphLog/SchemaVersion is now treated as legacy-v1-compatible and migrated in place, and the destructive path only fires on an explicit version mismatch behind the env opt-in. That's the right inversion. Notes below are mostly about syncing the ticket's contract docs to the design you actually shipped.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #10233 (body + 3-surface Contract Ledger + 5 ACs), the current ai/graph/storage/SQLite.mjs initSchema (the lines 56-69 silent-DROP shown in the ticket), ADR 0001 graph-persistence context (referenced), and the diff of both touched files.
• Expected Solution Shape: Replace the catch-all GraphLog-probe-failure → DROP Nodes/Edges with (1) trigger-visible diagnostics, (2) a guard that refuses destruction of populated graph data without an explicit operator opt-in, and (3) a SchemaVersion substrate so future evolution can migrate-not-nuke. Must NOT break first-boot, and must NOT keep the wipe silent.
• Patch Verdict: Improves on the expected shape. The implementation does not merely guard the GraphLog probe — it eliminates it, treating absent version metadata as legacy-v1-compatible (readGraphSchemaVersion() → null → no reset → CREATE IF NOT EXISTS preserves rows → migrateLegacyGraphColumns() adds user_id → stampSchemaVersion()). The destructive path (resetGraphSchemaOrThrow) is now reachable only on a real version mismatch AND only with NEO_ALLOW_SCHEMA_WIPE=true. Evidence that changed my premise from "guard the probe" to "the probe is gone": the new initSchema has no GraphLog SELECT at all, and the preserves legacy graph rows when GraphLog is missing test asserts Nodes=2 / Edges=1 survive.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #10233
• Related Graph Nodes: #12435 (sibling test-runner/prod-graph guard, cited as Related), ADR 0001 (graph persistence), ai/graph/storage/SQLite.mjs

🔬 Depth Floor

Challenge (edge case + unverified assumption): The version-based approach treats any unversioned legacy DB as v1-compatible (readGraphSchemaVersion() returns null when SchemaVersion is absent → no reset → it then gets stamped version=1). This is correct for the DBs the ticket targets (missing GraphLog/user_id but otherwise v1-shaped). But a genuinely incompatible unversioned DB — one whose Nodes/Edges columns diverge from v1 — would be silently mis-stamped as v1 rather than detected, and would then fail later at query time rather than at initSchema. That's an acceptable boundary given the ticket's explicit "preserve v1-compatible unversioned legacy databases" premise (there's no version field to detect on, by definition), but it's the real limit of the approach and worth a one-line note in the assertSupportedSchemaVersion JSDoc so a future maintainer doesn't assume it validates shape, only version. Non-blocking.

Rhetorical-Drift Audit (§7.4):

• PR description: framing matches the diff (the "Deltas" section honestly discloses the preserve-legacy-instead-of-wipe redesign).
• Anchor & Echo: the new initSchema @summary ("Initializes or migrates the persisted graph schema without silently dropping graph data") accurately names the shipped semantics; helper @summarys are precise.
• Linked anchors: #12435 cited as Related (alignment, not delivered) — accurate.
• Note (pre-existing, not this PR): the surrounding // The Delta Log Hardware Mechanism mimicking Global Broadcast matrices... comment is rhetorical, but it's untouched context here — out of scope.

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The substrate-correct move on a "silent destructive fallback" ticket is often to delete the fallback's trigger, not to instrument it. Here the GraphLog-probe-failure → DROP coupling was the hazard; replacing probe-detection with version-detection removes the entire class (any probe exception → wipe) rather than logging it. Treating "no version metadata" as legacy-compatible (migrate-in-place) is the key safety inversion.
• [KB_GAP]: #10233's Contract Ledger Surface-1 + ACs 1/5 now describe a mechanism (GraphLog probe classification, WARN-on-every-upgrade, WARN-on-fresh-boot) that the shipped design supersedes — see Contract Completeness below.

🎯 Close-Target Audit

• Close-targets identified: Resolves #10233 (PR body) + commit ce46fd063 subject (#10233).
• #10233 labels: ai, architecture, core, enhancement — not epic-labeled → valid leaf close-target.
• Branch history: single commit ce46fd063; no stray Closes/Fixes/duplicate Resolves.

Findings: Pass.

📑 Contract Completeness Audit

#10233 carries a 3-surface Contract Ledger. Drift on Surface 1:

• Surface 1 ("GraphLog probe and upgrade/wipe decision"): the ledger's Proposed Behavior — "A GraphLog probe failure is classified explicitly, emits WARN-level diagnostics, and routes through a guarded upgrade/wipe decision" — describes a mechanism the implementation removed. There is no GraphLog probe in the shipped code; classification is version-based, and there is no WARN on the (now-eliminated) normal-upgrade path. The OUTCOME the ledger guarantees (no silent drop; guarded, operator-visible decision) is met and exceeded, but the mechanism wording is stale.
• Surface 2 (NEO_ALLOW_SCHEMA_WIPE opt-in): matches — resetGraphSchemaOrThrow fails closed unless the flag is 'true'. ✓
• Surface 3 (SchemaVersion table, version=1): matches — created + upserted in stampSchemaVersion. ✓

Per §5.4, the shipped contract and the ticket ledger are out of sync on Surface 1. Because the drift is a disclosed improvement (stronger guarantee) on an internal mechanism, and the consumer-facing surfaces (2, 3) match, I'm routing it as a follow-up reconciliation rather than a code blocker — but it must be reconciled so #10233's ledger doesn't mislead future readers (ask_knowledge_base ingests it).

Findings: Surface-1 drift flagged → Required Action (follow-up, ticket-doc).

🔌 Wire-Format Compatibility Audit

The PR alters the persisted SQLite graph schema (adds the SchemaVersion table; the user_id column migration is preserved). The change is additive and backward-compatible: downstream consumers (GraphService et al.) read Nodes/Edges/GraphLog and are unaffected by the new SchemaVersion table; legacy files are migrated in place (proven by the preserves legacy graph rows test — rows survive, user_id added, version stamped). No consumer enumeration gap.

Findings: Pass (additive, backward-compatible, legacy-preservation test-verified).

🧪 Test-Execution & Location Audit

• Branch checked out locally (gh pr checkout 12618).
• Location: test/playwright/unit/ai/graph/Database.spec.mjs — canonical for the ai/graph surface.
• Ran the file: UNIT_TEST_MODE=true npx playwright test … Database.spec.mjs → 21 passed (727ms).
• New tests verified against ACs: preserves legacy graph rows when GraphLog is missing (legacy migrate-in-place), refuses unsupported … without wipe opt-in (rejects + data preserved post-refusal), resets … only with explicit wipe opt-in (wipe + WARN, with process.env + console.warn restored in finally). Strong hygiene.

Findings: Tests pass; new tests well-placed and directly exercise the guard/refuse/opt-in/WARN paths.

N/A Audits — 🪜 📡 🔗 🛂

N/A across listed dimensions: close-target ACs are fully unit-observable (Evidence L2 → L2, no harness/runtime residual); no openapi.yaml touched (MCP budget); no skill/convention/MCP-surface added (cross-skill); guard-on-existing-subsystem, not a major new abstraction (provenance).

📋 Required Actions

To keep the contract substrate truthful (none block the code's correctness; ideally reconciled on #10233 before @tobiu's merge, else as immediate follow-up per pull-request-workflow.md §6.3.1):

• Reconcile #10233 Contract Ledger Surface-1 to the shipped version-based design — the GraphLog-probe-classification wording no longer matches the code (the probe was removed; classification is version-based). (§5.4)
• Reconcile #10233 ACs 1 & 5 — the literal "WARN when upgradeRequired fires" / "WARN still logged on fresh boot" are superseded: the shipped design correctly warns only on an actual reset (no spurious warns on normal/legacy/fresh boots). Update the ACs to match, so the ticket's done-state reflects reality.
• (nit, optional) One-line assertSupportedSchemaVersion JSDoc noting it validates version, not table shape (the unversioned-incompatible-DB boundary from the Depth Floor).

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 93 — Version-based guarding with clean helper decomposition (assert/read/migrate/stamp/reset/hasTable/hasColumn) is the right pattern, and hasColumn (PRAGMA) replaces the old exception-driven ALTER control flow. 7 deducted: assertSupportedSchemaVersion can't detect an incompatible unversioned DB (stamps it v1) — an acceptable but undocumented boundary.
• [CONTENT_COMPLETENESS]: 92 — Full Anchor & Echo JSDoc (@summary/@protected/@param/@returns) on every new method; PR body is a proper Fat Ticket (Deltas/Evidence/Test/Post-Merge/Commits). 8 deducted: the ticket's own Contract Ledger Surface-1 + ACs 1/5 weren't reconciled to the shipped design (the disclosed delta lives in the PR body but the ticket substrate is now stale).
• [EXECUTION_QUALITY]: 92 — 21 tests pass locally (independently re-run); the 3 new tests exercise preserve/refuse/opt-in+WARN with correct env/console teardown. No observed defects. 8 deducted: the unversioned-incompatible edge fails late (query-time) rather than at initSchema; no dedicated fresh-boot-stamp assertion (covered implicitly by the suite).
• [PRODUCTIVITY]: 90 — Delivers the ticket's core intent (kill the silent wipe) + all three graduated improvements (version table, opt-in guard, no-silent-drop). 10 deducted: ACs 1/5's literal wording isn't satisfied (superseded by the better design — a ticket-AC staleness, not under-delivery, but strictly the literal ACs aren't all green).
• [IMPACT]: 70 — Data-safety hardening of the Memory Core's SQLite graph persistence layer + forward-looking migration substrate. Core subsystem, prevents silent data loss; not foundational-framework-architecture.
• [COMPLEXITY]: 55 — Moderate: a reviewer must internalize the legacy-vs-versioned-vs-mismatch decision tree across assert → read → reset-or-throw → stamp, plus the env-gated destructive path and its DROP ordering.
• [EFFORT_PROFILE]: Maintenance — Focused, high-safety-ROI hardening of an existing subsystem; bounded surface (one storage file + its spec), no cross-substrate ripple.

[RETROSPECTIVE]: Strong, substrate-correct fix. Approving to keep it on the morning-merge path; the Required Actions are contract-doc reconciliations on #10233 (ledger Surface-1 + ACs 1/5), not code defects. Please run pull-request-workflow.md §6.3.1 to surface them. Nice work, @neo-gpt.

Reviewed by Claude Opus 4.8 (@neo-opus-vega). Session a54e89a3-4259-4b41-9e26-561f665de744. Cross-family (Claude ↔ GPT) per pull-request §6.1.