From 2a145cdb3abdd516196e812533fc1644b75a645a Mon Sep 17 00:00:00 2001 From: zknpr Date: Wed, 3 Jun 2026 11:00:03 +0200 Subject: [PATCH 01/12] docs(undo): spec for operation-aware json_patch undo via read-modify-write (#425 part 1) Design doc for the undo-fidelity half of #425, superseding the closed PR #426 inverse-merge-patch attempt. Read-modify-write (read current cell, restore only the forward patch's touched keys, write the full object back; else value-replace) is non-lossy across the edge cases #426 closed unresolved (nested explicit nulls; non-object current cell) and collapses the edge-case treadmill that caused #426's review churn. Part 2 (futureStack hot-exit persistence) is a separate spec/PR. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../2026-06-03-json-patch-undo-rmw-design.md | 227 ++++++++++++++++++ 1 file changed, 227 insertions(+) create mode 100644 docs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.md diff --git a/docs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.md b/docs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.md new file mode 100644 index 0000000..abcab30 --- /dev/null +++ b/docs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.md @@ -0,0 +1,227 @@ +# Operation-aware json_patch cell undo (read-modify-write) + +- **Issue:** [#425](https://github.com/zknpr/SQLite-Explorer/issues/425) — "Hot-exit does not persist redo/undone state (futureStack) — undo of a saved edit is lost on reopen" +- **Scope of this spec:** the **undo-fidelity** half of #425 only (the issue's second sub-problem). PR **1 of 2**. +- **Supersedes:** closed PR [#426](https://github.com/zknpr/SQLite-Explorer/pull/426) (`fix/json-patch-undo`), which attempted the same fix with an inverse-merge-patch algorithm. +- **Release:** **none in this PR.** This PR ships code + tests only. The 1.5.0 → **1.5.1** bump, + the CHANGELOG entries (for *both* parts), and the `v1.5.1` tag are done in **Part 2** — the PR that + fully fixes #425. Rationale: #425 is a single issue with two sub-problems; the 1.5.1 release should + ship the complete fix, not a half of it. +- **Out of scope (→ PR 2):** futureStack hot-exit persistence (issue's original sub-problem); the + version bump, CHANGELOG, and release tag. + +## 1. Problem + +Undo of a JSON cell edit replaces the whole cell with the recorded prior document: + +```ts +// src/core/engine/wasm/WasmDatabaseEngine.ts (undoCellUpdate) +await this.updateCell(targetTable, targetRowId, targetColumn, priorValue ?? null); +// native: nativeWorker.ts undoModification — same full-value replacement +``` + +If a **different key of the same cell** changed between the tracked edit and the undo, that +concurrent change is clobbered. Real trigger: the cell is open in a VFS editor tab +(`virtualFileSystem.ts`) while it is also edited inline in the grid; undoing the inline edit +reverts the tab's independent change. Single-pane / strictly-sequential undo is already correct; +only interleaved same-cell edits are affected. Severity: P2 (data fidelity, not corruption). + +The forward path already records everything an operation-aware undo needs (no history-format +change required): + +```ts +// src/hostBridge.ts — single cell (updateCell) +const patch = this.tryGeneratePatch(value, originalValue); // RFC 7396 merge patch, or undefined +newValue: patch ?? value, // for json_patch edits this is the FORWARD PATCH string +operation: patch ? 'json_patch' : 'set', +priorValue: originalValue, // the FULL prior cell value (string | null) +// batch (updateCellBatch) records the same three fields per affectedCells[] entry +``` + +## 2. Why #426's inverse-patch approach was abandoned + +#426 built an **inverse merge patch** (`invertMergePatch` / `tryCreateInverseMergePatch`) and applied +it with SQLite's `json_patch(COALESCE(col,'{}'), ?)`. That representation is structurally lossy +against RFC 7396, so each review round plugged one leak and exposed another. The PR was **closed +with two unresolved P2 correctness holes** still open from its second review round: + +1. **Nested explicit nulls** (`json-utils.ts`): the guard only rejected a top-level `priorVal === null`. + A restored *object* containing nested explicit nulls is emitted into a merge patch, where + `null` means *delete* — so those keys vanish on undo. +2. **Non-object current cell** (`WasmDatabaseEngine.ts`): if the cell became SQL `NULL` or a JSON + scalar/array between edit and undo, `json_patch()` does **not** throw — it silently merges with + wrong semantics — so the `try/catch` fallback never fires. + +Both are inherent to expressing the undo as a merge patch (`null` = delete) and delegating the merge +to `json_patch()` (permissive on non-object inputs). They are not fixable without abandoning the +representation. + +## 3. Approach: read-modify-write, entirely in JS + +Instead of emitting a patch and letting SQLite merge it, **read the current cell value into JS, +compute the exact restored object, and write the whole object back.** This sidesteps both failure +modes above: explicit nulls are ordinary JS values (no `null`=delete ambiguity), and "is the current +cell an object?" is a deterministic JS check (no reliance on `json_patch()` throwing). + +### 3.1 Pure decision helper (`src/core/json-utils.ts`) + +A single pure function replaces `invertMergePatch` / `tryCreateInverseMergePatch`: + +```ts +type JsonUndoPlan = + | { kind: 'restore'; value: string } // write JSON.stringify(restoredObject) via SET col = ? + | { kind: 'replace' }; // value-replace: SET col = priorValue (recorded) + +// currentRaw = value read from the DB *now*; forwardPatchRaw = recorded newValue; +// priorRaw = recorded priorValue. All are raw cell values (string | null | …). +function computeJsonPatchUndo( + currentRaw: CellValue, + forwardPatchRaw: CellValue, + priorRaw: CellValue +): JsonUndoPlan; +``` + +**Eligibility for surgical restore** — all three must hold, else `{ kind: 'replace' }`: + +- `current` parses to a JSON **object**, and +- `prior` parses to a JSON **object**, and +- `forwardPatch` parses to a JSON **object**. + +(A non-object `forwardPatch` means the forward edit replaced the whole document — scenario 7. A +non-object `current` means we cannot surgically restore into it — scenario 6. A non-object/NULL +`prior` means value-replace restores it exactly — scenario 8. See §5 limitation.) + +**Restore walk** (reference; the §4 acceptance table is the binding contract). Clone `current`; walk +**only the forward patch's key structure**, drawing restoration values from `prior`: + +``` +restoreInto(currentObj, patchObj, priorObj, depth): + if depth > MAX_DEPTH: throw + result = { ...currentObj } + for key in Object.keys(patchObj): + pv = patchObj[key]; priorHas = hasOwn(priorObj, key); priorVal = priorObj[key] + if isObject(pv): # forward patched a nested object + if priorHas and isObject(priorVal): + result[key] = restoreInto(asObject(result[key]) ?? {}, pv, priorVal, depth+1) + else if priorHas: # prior was scalar/array/null → restore wholesale + result[key] = priorVal # exact, incl. explicit null + else: # forward ADDED this object key + child = restoreInto(asObject(result[key]) ?? {}, pv, {}, depth+1) + if isEmptyObject(child): delete result[key] # collapse a wholly-added object + else: result[key] = child # keep concurrent nested siblings + else: # forward set key to scalar/array/null + if priorHas: result[key] = priorVal # restore exact prior, incl. explicit null + else: delete result[key] # forward added it → remove + return result +``` + +`isObject(x)` ≡ non-null, `typeof === 'object'`, **not** an array (RFC 7396: arrays are atomic). +Reuse #426's `isObject` arrays-are-atomic correction. + +### 3.2 Engine wiring (`WasmDatabaseEngine.undoCellUpdate`, `nativeWorker.undoModification`) + +Per cell with `operation === 'json_patch'`: + +1. **Read** the current value of `(table, rowId, column)` via the engine's existing query path. +2. `plan = computeJsonPatchUndo(current, cell.newValue, cell.priorValue)`. +3. `plan.kind === 'restore'` → write `plan.value` with a plain `SET col = ?` (the existing + `updateCell(table, row, col, value)` value path — **not** the `patch` argument). + `plan.kind === 'replace'` → `updateCell(table, row, col, cell.priorValue ?? null)`. + +Cells with `operation !== 'json_patch'` keep today's value-replacement undo unchanged. + +**Atomicity (batch).** When any cell in a batch undo is `json_patch`, the per-cell read+write +sequence for the whole batch runs inside **one `SAVEPOINT`** (release on success, rollback-to on +error) so a mid-batch failure does not leave a partially-undone row. (This was #426's open atomicity +P2.) Reuse the existing `safeRollback` pattern; do not let a rollback error mask the original. + +**No per-engine duplication.** The decision logic lives once in `computeJsonPatchUndo`; each engine +only reads-then-writes. (This removes #426's duplicated `createUndoCellUpdate` across the WASM engine +and `nativeWorker`.) Keep the paired-contract comment noting web and native must interpret +`cell_update` fields identically. + +## 4. Acceptance criteria (binding contract) + +Each row is a unit-level scenario. Cell `data` holds a JSON document; the *tracked* forward edit is +listed, then any *concurrent* edit to a different key, then the expected state after undoing the +tracked edit. Rows marked *(#426)* are where #426 regressed or fell back. + +| # | Prior cell | Tracked forward patch | Concurrent edit (different key) | Current at undo | Expected after undo | +|---|-----------|-----------------------|---------------------------------|-----------------|---------------------| +| 1 | `{status:"draft",owner:"ada"}` | `{status:"published"}` | add `reviewer:"grace"` | `{status:"published",owner:"ada",reviewer:"grace"}` | `{status:"draft",owner:"ada",reviewer:"grace"}` | +| 2 | `{}` | `{meta:{reviewed:true}}` | none | `{meta:{reviewed:true}}` | `{}` — wholly-added object **removed** *(#426 left `{meta:{}}`)* | +| 3 | `{}` | `{meta:{reviewed:true}}` | add `meta.note:"keep"` | `{meta:{reviewed:true,note:"keep"}}` | `{meta:{note:"keep"}}` — only added leaf removed | +| 4 | `{a:null,b:1}` | `{a:2}` | none | `{a:2,b:1}` | `{a:null,b:1}` — explicit null **restored** *(#426 fell back to clobber)* | +| 5 | `{meta:{a:null,keep:1}}` | `{meta:{a:2}}` | none | `{meta:{a:2,keep:1}}` | `{meta:{a:null,keep:1}}` — nested explicit null restored *(#426 round-2 P2, unfixed)* | +| 6 | `{status:"draft",owner:"ada"}` | `{status:"published"}` | cell overwritten with non-object (`"plain text"` / SQL NULL / `5` / `[1,2]`) | e.g. `"plain text"` | value-replace → `{status:"draft",owner:"ada"}` *(#426 round-2 P2: `json_patch` didn't throw → silent wrong)* | +| 7 | `{a:1}` | `5` (scalar whole-doc replace) | add `extra` | n/a | value-replace → `{a:1}` | +| 8 | SQL `NULL` | `{added:true}` | none | `{added:true}` | value-replace → SQL `NULL` | +| 9 | batch: two rows each json_patch, each with a concurrent sibling on the other key | per-row `{count:…}` | per-row `concurrent:…` | per row | each row: count restored, concurrent kept; **atomic** (all-or-nothing) | + +Cross-cutting invariants: + +- **Round-trip:** for any object prior/patch, `undo(forward(prior))` with no concurrent edit equals `prior` exactly (key order aside), including explicit nulls. +- **Sibling survival:** keys absent from the forward patch's key structure are never read, written, or deleted by undo. +- **Non-`json_patch` edits:** behavior byte-for-byte unchanged from 1.5.0. + +## 5. Accepted limitation (documented, not a TODO) + +When the cell's recorded `prior` is **not** a JSON object (SQL `NULL`, scalar, or array), undo +value-replaces to that prior (scenarios 6–8). If a *concurrent* edit had added a sibling key to such +a cell after the tracked json_patch edit, that sibling is clobbered. This is an extreme corner (cell +was non-object → json-patch-edited → a *different* key concurrently added → undo) and matches the +prior 1.5.0 behavior; surfacing it is out of scope. Document it in a code comment at the eligibility +guard so a future reader knows it is deliberate. + +## 6. Reuse vs rewrite (relative to #426 / `fix/json-patch-undo`) + +**Reuse (adapt):** +- The ~14 edge-case test *scenarios* in `tests/unit/json_utils.test.ts`, `tests/unit/sqlite_db.test.ts`, + `tests/unit/nativeWorker.test.ts`. Assertions for scenarios 1/3/4/5 change from "value-replaced" to + "sibling/null preserved"; SQL-shape assertions change from `json_patch(COALESCE(col,'{}'), ?)` to a + plain `SET col = ?` carrying the full restored object. +- The `isObject` arrays-are-atomic fix. +- The `operation` / `newValue` plumbing and native→`operationsFacade` routing. + +**Add:** +- Scenario 2 (empty-object collapse) and scenario 5 (nested explicit null) tests — the cases #426 lacked/failed. + +**Rewrite:** +- The core algorithm: delete `invertMergePatch` / `tryCreateInverseMergePatch` (and the + `UnfaithfulInverseMergePatchError` machinery); replace with `computeJsonPatchUndo` + `restoreInto`. +- The engine undo paths switch from "emit inverse patch via `json_patch` SQL" to "read current → compute → write full object / value-replace". + +## 7. Testing + +- **Unit (pure):** `computeJsonPatchUndo` / `restoreInto` against every §4 scenario and the + round-trip + sibling-survival invariants. No DB needed. +- **Engine (WASM):** `tests/unit/sqlite_db.test.ts` — real `WasmDatabaseEngine` over an in-memory DB; + drive forward edit(s) + concurrent edit(s), call `undoModification`, assert the stored cell. +- **Engine (native):** `tests/unit/nativeWorker.test.ts` — the recording-mock harness; assert the + emitted SQL is the plain `SET col = ?` with the expected restored object (or value-replace), and + that batch json_patch undo is wrapped in a single savepoint. +- **Revert-proof:** reverting the source files must fail exactly the new/changed tests; restoring + makes them pass. +- **Build gate:** `npx tsc --noEmit` clean; `node scripts/build.mjs` OK; full `npm test` green. +- **Manual smoke:** open a JSON cell in a VFS tab, edit a different key inline in the grid, undo the + inline edit, confirm the tab's key survives. + +## 8. Files touched + +| File | Change | +|------|--------| +| `src/core/json-utils.ts` | Replace inverse-patch helpers with `computeJsonPatchUndo` + `restoreInto`; keep `isObject` arrays-atomic | +| `src/core/engine/wasm/WasmDatabaseEngine.ts` | `undoCellUpdate`: read-current → compute → write; batch savepoint; drop duplicated helper | +| `src/nativeWorker.ts` | `undoModification` (cell_update): same RMW via `operationsFacade`; batch savepoint | +| `tests/unit/json_utils.test.ts` | Replace inverse-patch tests with `computeJsonPatchUndo` scenarios (incl. #2, #5) | +| `tests/unit/sqlite_db.test.ts` | WASM undo scenarios (assertions updated to sibling/null-preserving) | +| `tests/unit/nativeWorker.test.ts` | Native undo SQL-shape + batch-atomicity scenarios | + +**Not touched in this PR:** `package.json` / `CHANGELOG.md`. The 1.5.1 bump and changelog (covering +both sub-problems) are deferred to Part 2 — the PR that closes #425. See the Release note above. + +## 9. Delegation note + +Implementation is delegated to Codex (per repo policy). The §4 acceptance table is the binding +contract; §3.1's `restoreInto` is reference guidance, not prescriptive. Verify the diff + run the +full suite + revert-proof check before opening the PR. From 816e911eb1b2abea08fede8dfb7599d8a48a3004 Mon Sep 17 00:00:00 2001 From: zknpr Date: Wed, 3 Jun 2026 11:09:43 +0200 Subject: [PATCH 02/12] =?UTF-8?q?docs(undo):=20implementation=20plan=20for?= =?UTF-8?q?=20json=5Fpatch=20undo=20(RMW)=20=E2=80=94=20#425=20part=201?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task-by-task TDD plan: pure computeJsonPatchUndo helper, WASM + native engine read-modify-write wiring, full verification. No version/changelog (1.5.1 ships with part 2). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../plans/2026-06-03-json-patch-undo-rmw.md | 697 ++++++++++++++++++ 1 file changed, 697 insertions(+) create mode 100644 docs/superpowers/plans/2026-06-03-json-patch-undo-rmw.md diff --git a/docs/superpowers/plans/2026-06-03-json-patch-undo-rmw.md b/docs/superpowers/plans/2026-06-03-json-patch-undo-rmw.md new file mode 100644 index 0000000..90fb5ea --- /dev/null +++ b/docs/superpowers/plans/2026-06-03-json-patch-undo-rmw.md @@ -0,0 +1,697 @@ +# json_patch Cell Undo (read-modify-write) Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. **Repo override:** per the project's Codex delegation policy, the implementer role is Codex; this plan is the scoped brief. Verify diff + tests before opening the PR. + +**Goal:** Make undo of a JSON cell edit restore only the keys the edit touched, preserving concurrent changes to other keys of the same cell, with no data loss across the edge cases that sank PR #426. + +**Architecture:** A pure decision helper (`computeJsonPatchUndo`) reads the cell's *current* value, the recorded forward merge-patch (`newValue`), and the recorded full prior (`priorValue`), and returns either a full restored object to write (`SET col = ?`) or a `replace` signal (value-replace to `priorValue`). Both engines (WASM `WasmDatabaseEngine`, native `nativeWorker`) read-then-write through it. All JSON reasoning happens in JS — no `json_patch()` SQL on the undo path — which eliminates RFC 7396 `null`=delete ambiguity and `json_patch()`'s silent-wrong behavior on non-object inputs. + +**Tech Stack:** TypeScript, `node:test` + `node:assert`, `tsx` test runner, esbuild. SQLite via sql.js (WASM) and txiki-js (native). + +**Spec:** `docs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.md` (§4 acceptance table is the binding contract). + +**Base branch:** `fix/json-patch-undo-rmw` (off `main`). **No** `package.json`/`CHANGELOG.md` change in this PR — the 1.5.1 bump ships with Part 2 (issue #425's other half). + +**Test commands:** +- Single file: `npx tsx --tsconfig tsconfig.test.json --test tests/unit/.test.ts` +- Full suite: `npm test` +- Type-check: `npx tsc --noEmit` +- Build: `node scripts/build.mjs` + +--- + +## File Structure + +| File | Responsibility | Change | +|------|----------------|--------| +| `src/core/json-utils.ts` | Pure RFC-7396 merge-patch + the new undo decision helper | Add `JsonUndoPlan`, `computeJsonPatchUndo`, `restoreInto`, `parseJsonObject`; tighten `isObject` to exclude arrays | +| `src/core/engine/wasm/WasmDatabaseEngine.ts` | WASM cell undo wiring | Rewrite `undoCellUpdate` to read→compute→write (single + batch-with-savepoint) | +| `src/nativeWorker.ts` | Native cell undo wiring | Rewrite `undoModification` `cell_update` branch to read→compute→write (single `run` + batch `execBatch`) | +| `tests/unit/json_utils.test.ts` | Pure helper coverage (exhaustive) | Add `computeJsonPatchUndo` scenarios + invariants | +| `tests/unit/sqlite_db.test.ts` | WASM end-to-end undo coverage | Add real-engine undo scenarios (wiring, fallback, batch atomicity) | +| `tests/unit/nativeWorker.test.ts` | Native undo SQL-shape coverage | Add read-then-write SQL-shape + batch-atomicity scenarios | + +**Test design (avoid redundancy):** the pure helper gets exhaustive scenario coverage in `json_utils.test.ts`. The engine tests do **not** re-test every algorithm branch — they verify the *wiring* (current value is read; restore writes a full-object `SET`; non-object-current and non-`json_patch` route to value-replace; batch undo is atomic) with a representative subset. + +--- + +## Task 1: Pure undo decision helper (`json-utils.ts`) + +**Files:** +- Modify: `src/core/json-utils.ts` +- Test: `tests/unit/json_utils.test.ts` + +- [ ] **Step 1: Write the failing tests** + +Append to `tests/unit/json_utils.test.ts` (the file already imports `generateMergePatch, applyMergePatch` — extend the import to include `computeJsonPatchUndo`): + +```ts +// add computeJsonPatchUndo to the existing top-of-file import from '../../src/core/json-utils' +import { generateMergePatch, applyMergePatch, computeJsonPatchUndo } from '../../src/core/json-utils'; + +describe('computeJsonPatchUndo (RMW undo decision)', () => { + const j = (v: unknown) => JSON.stringify(v); + const expectRestore = (plan: ReturnType, expected: unknown) => { + assert.strictEqual(plan.kind, 'restore'); + assert.deepStrictEqual(JSON.parse((plan as { kind: 'restore'; value: string }).value), expected); + }; + const expectReplace = (plan: ReturnType) => + assert.strictEqual(plan.kind, 'replace'); + + it('s1: restores the edited key and preserves a concurrent sibling key', () => { + expectRestore( + computeJsonPatchUndo( + j({ status: 'published', owner: 'ada', reviewer: 'grace' }), // current (forward + concurrent) + j({ status: 'published' }), // forward patch (newValue) + j({ status: 'draft', owner: 'ada' }) // prior + ), + { status: 'draft', owner: 'ada', reviewer: 'grace' } + ); + }); + + it('s2: removes a wholly-added object key (empty-object collapse)', () => { + expectRestore( + computeJsonPatchUndo(j({ meta: { reviewed: true } }), j({ meta: { reviewed: true } }), j({})), + {} + ); + }); + + it('s3: removes only the added nested leaf, keeping a concurrent nested sibling', () => { + expectRestore( + computeJsonPatchUndo(j({ meta: { reviewed: true, note: 'keep' } }), j({ meta: { reviewed: true } }), j({})), + { meta: { note: 'keep' } } + ); + }); + + it('s4: restores an explicit null at the edited key (preserving a sibling)', () => { + expectRestore( + computeJsonPatchUndo(j({ a: 2, b: 1 }), j({ a: 2 }), j({ a: null, b: 1 })), + { a: null, b: 1 } + ); + }); + + it('s5: restores a nested explicit null in a restored subtree', () => { + expectRestore( + computeJsonPatchUndo(j({ meta: { a: 2, keep: 1 } }), j({ meta: { a: 2 } }), j({ meta: { a: null, keep: 1 } })), + { meta: { a: null, keep: 1 } } + ); + }); + + it('s6: value-replaces when the current cell is not a JSON object', () => { + for (const current of ['plain text', null, '5', '[1,2]']) { + expectReplace(computeJsonPatchUndo(current, j({ status: 'published' }), j({ status: 'draft' }))); + } + }); + + it('s7: value-replaces when the forward patch was a scalar/array whole-doc replacement', () => { + expectReplace(computeJsonPatchUndo(j({ a: 1 }), '5', j({ a: 1 }))); + expectReplace(computeJsonPatchUndo(j({ a: 1 }), '[1,2]', j({ a: 1 }))); + }); + + it('s8: value-replaces when the prior cell was SQL NULL / non-object', () => { + expectReplace(computeJsonPatchUndo(j({ added: true }), j({ added: true }), null)); + expectReplace(computeJsonPatchUndo(j({ added: true }), j({ added: true }), '5')); + }); + + it('invariant: round-trips an object edit (no concurrent change) back to prior', () => { + const prior = { status: 'draft', meta: { reviewed: false, owner: 'ada' } }; + const forward = generateMergePatch(prior, { status: 'published', meta: { reviewed: true, owner: 'ada' } }); + const current = applyMergePatch(prior, forward); + expectRestore(computeJsonPatchUndo(j(current), j(forward), j(prior)), prior); + }); + + it('invariant: never touches keys outside the forward patch structure', () => { + expectRestore( + computeJsonPatchUndo( + j({ a: 'forward-changed', untouched: { deep: 1 }, sibling: 2 }), + j({ a: 'forward-changed' }), + j({ a: 'orig' }) + ), + { a: 'orig', untouched: { deep: 1 }, sibling: 2 } + ); + }); +}); +``` + +- [ ] **Step 2: Run the tests, verify they fail** + +Run: `npx tsx --tsconfig tsconfig.test.json --test tests/unit/json_utils.test.ts` +Expected: FAIL — `computeJsonPatchUndo` is not exported (compile/import error). + +- [ ] **Step 3: Implement the helper** + +In `src/core/json-utils.ts`: (a) tighten `isObject` to exclude arrays; (b) add the types + helper below (place after `applyMergePatch`). Reuse the existing `MAX_DEPTH` constant. + +```ts +// Tighten the existing isObject (RFC 7396: arrays are atomic values, not merge targets): +function isObject(val: unknown): val is Record { + return val !== null && typeof val === 'object' && !Array.isArray(val); +} + +export type JsonUndoPlan = + | { kind: 'restore'; value: string } // write JSON.stringify(restored) via SET col = ? + | { kind: 'replace' }; // value-replace: SET col = recorded priorValue + +/** Parse a raw cell value to a plain JSON object, or undefined if it is not one. */ +function parseJsonObject(raw: unknown): Record | undefined { + if (typeof raw !== 'string') return undefined; + try { + const parsed = JSON.parse(raw); + return isObject(parsed) ? parsed : undefined; + } catch { + return undefined; + } +} + +/** + * Decide how to undo a forward json_patch cell edit by read-modify-write. + * + * Surgical restore requires current, forwardPatch, and prior to all be JSON + * objects; otherwise we value-replace to the recorded prior. (Accepted + * limitation: when prior is SQL NULL / scalar / array we value-replace, which + * can clobber a concurrent sibling added to such a cell — an extreme corner that + * matches pre-RMW behavior.) + * + * @param currentRaw the cell value read from the DB *now* + * @param forwardPatchRaw the recorded forward merge patch (ModificationEntry.newValue) + * @param priorRaw the recorded full prior cell value (ModificationEntry.priorValue) + */ +export function computeJsonPatchUndo( + currentRaw: unknown, + forwardPatchRaw: unknown, + priorRaw: unknown +): JsonUndoPlan { + const current = parseJsonObject(currentRaw); + const forwardPatch = parseJsonObject(forwardPatchRaw); + const prior = parseJsonObject(priorRaw); + if (!current || !forwardPatch || !prior) { + return { kind: 'replace' }; + } + return { kind: 'restore', value: JSON.stringify(restoreInto(current, forwardPatch, prior, 0)) }; +} + +/** + * Walk only the forward patch's key structure, restoring each touched key from + * `prior` into a clone of `current`; keys absent from the patch are untouched. + */ +function restoreInto( + currentObj: Record, + patchObj: Record, + priorObj: Record, + depth: number +): Record { + if (depth > MAX_DEPTH) { + throw new Error('JSON undo restore depth limit exceeded'); + } + const result: Record = { ...currentObj }; + for (const key of Object.keys(patchObj)) { + const pv = patchObj[key]; + const priorHas = Object.prototype.hasOwnProperty.call(priorObj, key); + const priorVal = priorHas ? priorObj[key] : undefined; + + if (isObject(pv)) { + if (priorHas && isObject(priorVal)) { + // Forward patched a nested object that was also an object in prior — recurse. + const base = isObject(result[key]) ? (result[key] as Record) : {}; + result[key] = restoreInto(base, pv, priorVal, depth + 1); + } else if (priorHas) { + // Prior was a scalar / array / explicit null — restore it wholesale. + result[key] = priorVal; + } else { + // Forward ADDED this object key — strip the leaves it added, keep concurrent siblings. + const base = isObject(result[key]) ? (result[key] as Record) : {}; + const child = restoreInto(base, pv, {}, depth + 1); + if (Object.keys(child).length === 0) { + delete result[key]; // wholly-added object, nothing concurrent survived -> drop it + } else { + result[key] = child; + } + } + } else if (priorHas) { + // Forward set a scalar/array/null at an existing key — restore exact prior (incl. explicit null). + result[key] = priorVal; + } else { + // Forward added this scalar key — remove it. + delete result[key]; + } + } + return result; +} +``` + +- [ ] **Step 4: Run the tests (new + existing), verify all pass** + +Run: `npx tsx --tsconfig tsconfig.test.json --test tests/unit/json_utils.test.ts` +Expected: PASS for the new `computeJsonPatchUndo` suite **and** the pre-existing `generateMergePatch`/`applyMergePatch` suites (the `isObject` array change must not regress them — arrays were already meant to be atomic). + +- [ ] **Step 5: Commit** + +```bash +git add src/core/json-utils.ts tests/unit/json_utils.test.ts +git commit -m "feat(undo): pure read-modify-write json_patch undo decision helper + +computeJsonPatchUndo restores only the forward patch's touched keys from the +recorded prior, preserving concurrent sibling edits; value-replaces when current, +prior, or the forward patch is not a JSON object. isObject now excludes arrays +(RFC 7396 atomic). Pure + exhaustively unit-tested; engine wiring follows. + +Co-Authored-By: Claude Opus 4.8 (1M context) " +``` + +--- + +## Task 2: WASM engine undo wiring (`WasmDatabaseEngine.undoCellUpdate`) + +**Files:** +- Modify: `src/core/engine/wasm/WasmDatabaseEngine.ts:284-298` (the `undoCellUpdate` method) and the import of `json-utils` (add `computeJsonPatchUndo`) +- Test: `tests/unit/sqlite_db.test.ts` + +- [ ] **Step 1: Write the failing tests** + +Append to `tests/unit/sqlite_db.test.ts` inside the `describe('WasmDatabaseEngine', …)` block (harness already provides `engine` and a `users(id,name,age,data TEXT)` table; each test self-cleans with `DELETE FROM users`). Forward json_patch edits are driven with `updateCell(table,row,col,null,patchString)`. + +```ts +it('undo single json_patch edit preserves a concurrent sibling key (s1)', async () => { + await engine.executeQuery('DELETE FROM users'); + const prior = JSON.stringify({ status: 'draft', owner: 'ada' }); + const forward = JSON.stringify({ status: 'published' }); + await engine.insertRow('users', { id: 1, name: 'A', age: 30, data: prior }); + await engine.updateCell('users', 1, 'data', null, forward); // tracked edit + await engine.updateCell('users', 1, 'data', null, JSON.stringify({ reviewer: 'grace' })); // concurrent edit + + await engine.undoModification({ + modificationType: 'cell_update', description: 'u', targetTable: 'users', + targetRowId: 1, targetColumn: 'data', priorValue: prior, newValue: forward, operation: 'json_patch' + }); + + const r = await engine.executeQuery('SELECT data FROM users WHERE id = 1'); + assert.deepStrictEqual(JSON.parse(r[0].rows[0][0] as string), { status: 'draft', owner: 'ada', reviewer: 'grace' }); +}); + +it('undo of a wholly-added object key removes it entirely (s2)', async () => { + await engine.executeQuery('DELETE FROM users'); + const forward = JSON.stringify({ meta: { reviewed: true } }); + await engine.insertRow('users', { id: 1, name: 'A', age: 30, data: '{}' }); + await engine.updateCell('users', 1, 'data', null, forward); + + await engine.undoModification({ + modificationType: 'cell_update', description: 'u', targetTable: 'users', + targetRowId: 1, targetColumn: 'data', priorValue: '{}', newValue: forward, operation: 'json_patch' + }); + + const r = await engine.executeQuery('SELECT data FROM users WHERE id = 1'); + assert.deepStrictEqual(JSON.parse(r[0].rows[0][0] as string), {}); +}); + +it('undo restores an explicit null at the edited key (s4)', async () => { + await engine.executeQuery('DELETE FROM users'); + const prior = JSON.stringify({ a: null, b: 1 }); + const forward = JSON.stringify({ a: 2 }); + await engine.insertRow('users', { id: 1, name: 'A', age: 30, data: prior }); + await engine.updateCell('users', 1, 'data', null, forward); + + await engine.undoModification({ + modificationType: 'cell_update', description: 'u', targetTable: 'users', + targetRowId: 1, targetColumn: 'data', priorValue: prior, newValue: forward, operation: 'json_patch' + }); + + const r = await engine.executeQuery('SELECT data FROM users WHERE id = 1'); + const parsed = JSON.parse(r[0].rows[0][0] as string); + assert.deepStrictEqual(parsed, { a: null, b: 1 }); + assert.ok(Object.prototype.hasOwnProperty.call(parsed, 'a')); // explicit null kept, not deleted +}); + +it('undo value-replaces when the cell became non-JSON since the edit (s6)', async () => { + await engine.executeQuery('DELETE FROM users'); + const prior = JSON.stringify({ status: 'draft', owner: 'ada' }); + const forward = JSON.stringify({ status: 'published' }); + await engine.insertRow('users', { id: 1, name: 'A', age: 30, data: prior }); + await engine.updateCell('users', 1, 'data', null, forward); + await engine.updateCell('users', 1, 'data', 'plain text'); // current is now non-object + + await engine.undoModification({ + modificationType: 'cell_update', description: 'u', targetTable: 'users', + targetRowId: 1, targetColumn: 'data', priorValue: prior, newValue: forward, operation: 'json_patch' + }); + + const r = await engine.executeQuery('SELECT data FROM users WHERE id = 1'); + assert.deepStrictEqual(JSON.parse(r[0].rows[0][0] as string), { status: 'draft', owner: 'ada' }); +}); + +it('batch undo restores each json_patch cell, keeps concurrent siblings, and is atomic (s9)', async () => { + await engine.executeQuery('DELETE FROM users'); + const p1 = JSON.stringify({ count: 1, stable: 'one' }); + const p2 = JSON.stringify({ count: 10, stable: 'two' }); + await engine.insertRow('users', { id: 1, name: 'A', age: 30, data: p1 }); + await engine.insertRow('users', { id: 2, name: 'B', age: 31, data: p2 }); + await engine.updateCellBatch('users', [ + { rowId: 1, column: 'data', value: JSON.stringify({ count: 2 }), operation: 'json_patch' }, + { rowId: 2, column: 'data', value: JSON.stringify({ count: 11 }), operation: 'json_patch' } + ]); + await engine.updateCell('users', 1, 'data', null, JSON.stringify({ concurrent: 'one' })); + await engine.updateCell('users', 2, 'data', null, JSON.stringify({ concurrent: 'two' })); + + await engine.undoModification({ + modificationType: 'cell_update', description: 'u', targetTable: 'users', + affectedCells: [ + { rowId: 1, columnName: 'data', priorValue: p1, newValue: JSON.stringify({ count: 2 }), operation: 'json_patch' }, + { rowId: 2, columnName: 'data', priorValue: p2, newValue: JSON.stringify({ count: 11 }), operation: 'json_patch' } + ] + }); + + const r = await engine.executeQuery('SELECT id, data FROM users ORDER BY id'); + assert.deepStrictEqual(JSON.parse(r[0].rows[0][1] as string), { count: 1, stable: 'one', concurrent: 'one' }); + assert.deepStrictEqual(JSON.parse(r[0].rows[1][1] as string), { count: 10, stable: 'two', concurrent: 'two' }); +}); +``` + +- [ ] **Step 2: Run the tests, verify they fail** + +Run: `npx tsx --tsconfig tsconfig.test.json --test tests/unit/sqlite_db.test.ts` +Expected: FAIL — current `undoCellUpdate` does blind value-replacement, so s1 yields `{status:'draft',owner:'ada'}` (concurrent `reviewer` lost), s2 yields `{meta:{reviewed:true}}` reverted to `{}` only by luck of value-replace… (s1/s4 assertions fail). + +- [ ] **Step 3: Rewrite `undoCellUpdate`** + +Add `computeJsonPatchUndo` to the `json-utils` import at the top of `WasmDatabaseEngine.ts`. Replace the method body (lines 284-298) with: + +```ts +private async undoCellUpdate(targetTable: string, mod: ModificationEntry): Promise { + const { affectedCells, targetRowId, targetColumn, priorValue, newValue, operation } = mod; + + if (affectedCells) { + // If any cell is a json_patch undo we must read-modify-write per cell; wrap the + // whole batch in a SAVEPOINT so a mid-batch failure rolls the row set back atomically. + if (affectedCells.some(c => c.operation === 'json_patch')) { + const savepoint = `sp_undo_${Date.now()}`; + await this.executeQuery(`SAVEPOINT ${savepoint}`); + try { + for (const c of affectedCells) { + await this.undoOneCell(targetTable, c.rowId, c.columnName, c.priorValue, c.newValue, c.operation); + } + await this.executeQuery(`RELEASE SAVEPOINT ${savepoint}`); + } catch (err) { + await this.safeRollbackSavepoint(savepoint, 'undoCellUpdate'); + throw err; + } + } else { + // Pure value-replacement batch keeps the existing single-statement-per-cell path. + await this.updateCellBatch(targetTable, affectedCells.map(c => ({ + rowId: c.rowId, column: c.columnName, value: c.priorValue ?? null + }))); + } + } else if (targetRowId !== undefined && targetColumn) { + await this.undoOneCell(targetTable, targetRowId, targetColumn, priorValue, newValue, operation); + } +} + +/** Undo one cell: read-modify-write for json_patch edits, value-replacement otherwise. */ +private async undoOneCell( + targetTable: string, + rowId: RecordId, + column: string, + priorValue: CellValue | undefined, + newValue: CellValue | undefined, + operation: ModificationEntry['operation'] +): Promise { + if (operation === 'json_patch') { + const current = await this.readCellValue(targetTable, rowId, column); + const plan = computeJsonPatchUndo(current, newValue, priorValue); + if (plan.kind === 'restore') { + await this.updateCell(targetTable, rowId, column, plan.value); // plain SET col = ? + return; + } + } + await this.updateCell(targetTable, rowId, column, priorValue ?? null); +} + +/** Read a single cell's current value (mirrors the SELECT used by the json_patch fallback). */ +private async readCellValue(table: string, rowId: RecordId, column: string): Promise { + const escapedCol = escapeIdentifier(column); + const escapedTbl = escapeIdentifier(table); + const res = await this.executeQuery( + `SELECT ${escapedCol} FROM ${escapedTbl} WHERE rowid = ?`, [validateRowId(rowId)] + ); + return (res[0]?.rows[0]?.[0] ?? null) as CellValue; +} +``` + +(Confirm `safeRollbackSavepoint` exists — it is used at `WasmDatabaseEngine.ts:245` by `applyModifications`. Reuse it.) + +- [ ] **Step 4: Run the tests, verify they pass** + +Run: `npx tsx --tsconfig tsconfig.test.json --test tests/unit/sqlite_db.test.ts` +Expected: PASS for all new scenarios and all pre-existing `WasmDatabaseEngine` tests. + +- [ ] **Step 5: Commit** + +```bash +git add src/core/engine/wasm/WasmDatabaseEngine.ts tests/unit/sqlite_db.test.ts +git commit -m "feat(undo): operation-aware json_patch cell undo in the WASM engine + +undoCellUpdate now reads the current cell and routes json_patch undos through +computeJsonPatchUndo (full-object SET), preserving concurrent sibling edits; +non-json_patch and non-object-current cells value-replace. Batch undos that touch +any json_patch cell are wrapped in a SAVEPOINT for atomicity. + +Co-Authored-By: Claude Opus 4.8 (1M context) " +``` + +--- + +## Task 3: Native engine undo wiring (`nativeWorker.undoModification`) + +**Files:** +- Modify: `src/nativeWorker.ts` — the `cell_update` branch of `undoModification` (around lines 543-560) and add `computeJsonPatchUndo` to the `json-utils` import +- Test: `tests/unit/nativeWorker.test.ts` + +**Native read/write primitives:** read current via `operationsFacade.executeQuery('SELECT FROM WHERE rowid = ?', [rowIdNum])` → `result[0].rows[0][0]` (backed by `worker.call('query', …)`). Write a restored object or value-replace via `operationsFacade.updateCell(table, rowId, column, value)` (value path → `worker.call('run', ['UPDATE … SET col = ? WHERE rowid = ?', [value, rowIdNum]])`). Batch writes go through one `worker.call('execBatch', [items])` for atomicity. + +- [ ] **Step 1: Write the failing tests** + +The harness in `tests/unit/nativeWorker.test.ts` already supports a per-call responder: `createRecordingConnection(respondToCall?)` where `respondToCall(call) => { result } | { error }`. For RMW, answer `query` (the read) with the current cell, and assert the subsequent `run`/`execBatch` write. Add inside the existing `describe('createNativeDatabaseConnection', …)`: + +```ts +it('undoes a single json_patch cell by reading current then writing the restored object', async () => { + // Respond to the RMW SELECT with the post-forward+concurrent current value; default-respond the write. + const current = { status: 'published', owner: 'ada', reviewer: 'grace' }; + const connection = await createRecordingConnection((call) => { + if (call.method === 'query') { + return { result: { columns: ['payload'], values: [[JSON.stringify(current)]] } }; + } + return { result: { changes: 1, lastInsertRowId: 1 } }; + }); + try { + connection.calls.length = 0; + await connection.databaseOps.undoModification({ + modificationType: 'cell_update', description: 'u', targetTable: 'docs', + targetRowId: 7, targetColumn: 'payload', + priorValue: JSON.stringify({ status: 'draft', owner: 'ada' }), + newValue: JSON.stringify({ status: 'published' }), operation: 'json_patch' + }); + + const queryCall = connection.calls.find(c => c.method === 'query'); + const runCall = connection.calls.find(c => c.method === 'run'); + assert.ok(queryCall, 'expected a SELECT read'); + assert.ok(runCall, 'expected a SET write'); + const [sql, params] = runCall!.args as [string, unknown[]]; + assert.strictEqual(sql, `UPDATE "docs" SET "payload" = ? WHERE rowid = ?`); + assert.deepStrictEqual(JSON.parse(params[0] as string), { status: 'draft', owner: 'ada', reviewer: 'grace' }); + assert.strictEqual(params[1], 7); + } finally { + connection.dispose(); + } +}); + +it('value-replaces a single json_patch undo when the current cell is non-object', async () => { + const connection = await createRecordingConnection((call) => { + if (call.method === 'query') return { result: { columns: ['payload'], values: [['plain text']] } }; + return { result: { changes: 1, lastInsertRowId: 1 } }; + }); + try { + const prior = JSON.stringify({ status: 'draft' }); + connection.calls.length = 0; + await connection.databaseOps.undoModification({ + modificationType: 'cell_update', description: 'u', targetTable: 'docs', + targetRowId: 7, targetColumn: 'payload', + priorValue: prior, newValue: JSON.stringify({ status: 'published' }), operation: 'json_patch' + }); + const runCall = connection.calls.find(c => c.method === 'run')!; + const [sql, params] = runCall.args as [string, unknown[]]; + assert.strictEqual(sql, `UPDATE "docs" SET "payload" = ? WHERE rowid = ?`); + assert.deepStrictEqual(params, [prior, 7]); // value-replace to recorded prior + } finally { + connection.dispose(); + } +}); + +it('undoes a batch of json_patch cells atomically via execBatch of restored-object writes', async () => { + const currents: Record = { + 3: { count: 2, stable: 'one', concurrent: 'a' }, + 4: { count: 11, stable: 'two', concurrent: 'b' } + }; + const connection = await createRecordingConnection((call) => { + if (call.method === 'query') { + const [, params] = call.args as [string, unknown[]]; + return { result: { columns: ['payload'], values: [[JSON.stringify(currents[Number(params[0])])]] } }; + } + return { result: { changes: 1, lastInsertRowId: 1 } }; + }); + try { + connection.calls.length = 0; + await connection.databaseOps.undoModification({ + modificationType: 'cell_update', description: 'u', targetTable: 'docs', + affectedCells: [ + { rowId: 3, columnName: 'payload', priorValue: JSON.stringify({ count: 1, stable: 'one' }), newValue: JSON.stringify({ count: 2 }), operation: 'json_patch' }, + { rowId: 4, columnName: 'payload', priorValue: JSON.stringify({ count: 10, stable: 'two' }), newValue: JSON.stringify({ count: 11 }), operation: 'json_patch' } + ] + }); + + const batchCall = connection.calls.find(c => c.method === 'execBatch'); + assert.ok(batchCall, 'batch json_patch undo must write through one execBatch (atomic)'); + const items = (batchCall!.args as [Array<{ sql: string; params: unknown[] }>])[0]; + assert.strictEqual(items.length, 2); + assert.ok(items.every(i => i.sql === `UPDATE "docs" SET "payload" = ? WHERE rowid = ?`)); + assert.deepStrictEqual(JSON.parse(items[0].params[0] as string), { count: 1, stable: 'one', concurrent: 'a' }); + assert.deepStrictEqual(JSON.parse(items[1].params[0] as string), { count: 10, stable: 'two', concurrent: 'b' }); + } finally { + connection.dispose(); + } +}); +``` + +> **Codex note:** confirm the recording mock's `query` response shape (`{ columns, values }`) matches what `NativeWorkerProcess`/`worker.call('query', …)` expects in this harness; adjust the mock response wrapper if the harness double-wraps. The assertions on the emitted write SQL/params are the contract and must hold. + +- [ ] **Step 2: Run the tests, verify they fail** + +Run: `npx tsx --tsconfig tsconfig.test.json --test tests/unit/nativeWorker.test.ts` +Expected: FAIL — current native undo emits a blind value-replacement `run`/`execBatch` (no `query` read; batch loses concurrent keys). + +- [ ] **Step 3: Rewrite the `cell_update` branch of `undoModification`** + +Add `computeJsonPatchUndo` to the `json-utils` import. Replace the `case 'cell_update':` block (lines ~543-560) with read-modify-write. Define a local helper that resolves each cell to a concrete write `{ value }`: + +```ts +case 'cell_update': { + const { newValue, operation } = mod; + + // Resolve one cell to the value to write back (full restored object, or prior for value-replace). + const resolveUndoValue = async ( + rowId: RecordId, column: string, + cellPrior: CellValue | undefined, cellNew: CellValue | undefined, + cellOp: ModificationEntry['operation'] + ): Promise => { + if (cellOp === 'json_patch') { + const read = await operationsFacade.executeQuery( + `SELECT ${escapeIdentifier(column)} FROM ${escapeIdentifier(targetTable)} WHERE rowid = ?`, + [Number(rowId)] + ); + const current = (read[0]?.rows[0]?.[0] ?? null) as CellValue; + const plan = computeJsonPatchUndo(current, cellNew, cellPrior); + if (plan.kind === 'restore') return plan.value; + } + return cellPrior ?? null; + }; + + if (affectedCells) { + // Read+compute every cell first, then write the whole set in one atomic execBatch. + const items = []; + for (const c of affectedCells) { + const value = await resolveUndoValue(c.rowId, c.columnName, c.priorValue, c.newValue, c.operation); + items.push({ + sql: `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(c.columnName)} = ? WHERE rowid = ?`, + params: [value, Number(c.rowId)] + }); + } + if (items.length > 0) { + await worker.call('execBatch', [items]); + } + } else if (targetRowId !== undefined && targetColumn) { + const value = await resolveUndoValue(targetRowId, targetColumn, priorValue, newValue, operation); + await worker.call('run', [ + `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(targetColumn)} = ? WHERE rowid = ?`, + [value, Number(targetRowId)] + ]); + } + break; +} +``` + +(`priorValue`, `affectedCells`, `targetRowId`, `targetColumn` are already destructured at the top of `undoModification`; add `newValue` and `operation` to that destructure. Keep the paired-contract comment noting web and native must interpret `cell_update` fields identically.) + +- [ ] **Step 4: Run the tests, verify they pass** + +Run: `npx tsx --tsconfig tsconfig.test.json --test tests/unit/nativeWorker.test.ts` +Expected: PASS for the new scenarios and all pre-existing native tests. + +- [ ] **Step 5: Commit** + +```bash +git add src/nativeWorker.ts tests/unit/nativeWorker.test.ts +git commit -m "feat(undo): operation-aware json_patch cell undo in the native engine + +undoModification cell_update now reads current then writes the restored object +(or value-replaces) via computeJsonPatchUndo — matching the WASM engine. Batch +json_patch undos write through one execBatch for atomicity. + +Co-Authored-By: Claude Opus 4.8 (1M context) " +``` + +--- + +## Task 4: Full verification (build, types, suite, revert-proof) + +**Files:** none (verification only). + +- [ ] **Step 1: Type-check** + +Run: `npx tsc --noEmit` +Expected: no errors. + +- [ ] **Step 2: Build both targets** + +Run: `node scripts/build.mjs` +Expected: completes; `out/extension.js`, `out/extension-browser.js`, `out/worker.js`, `out/worker-browser.js` produced. + +- [ ] **Step 3: Full unit suite** + +Run: `npm test` +Expected: all tests pass (prior count + the new json_utils/sqlite_db/nativeWorker scenarios). + +- [ ] **Step 4: Revert-proof check** + +Temporarily revert the three source files (keep the tests) and confirm the new tests fail, then restore: + +```bash +git stash push -- src/core/json-utils.ts src/core/engine/wasm/WasmDatabaseEngine.ts src/nativeWorker.ts +npx tsx --tsconfig tsconfig.test.json --test tests/unit/json_utils.test.ts tests/unit/sqlite_db.test.ts tests/unit/nativeWorker.test.ts # expect failures +git stash pop +npm test # expect green again +``` + +(Note: reverting `json-utils.ts` will also remove the export and cause compile-level failures in the engine files — that is acceptable evidence the tests bind to the new code. If the stash makes the type-check too noisy to run the tests, revert only the engine files for the WASM/native suites and only `json-utils.ts`'s helper body for the pure suite.) + +- [ ] **Step 5: Manual smoke (optional but recommended)** + +Build, F5 Extension Development Host, open a DB with a JSON column. Open a JSON cell in a VS Code tab (VFS), edit a *different* key inline in the grid, undo the inline edit, confirm the tab's key survives. + +- [ ] **Step 6: No commit** + +Task 4 produces no code changes. Do **not** bump `package.json` or edit `CHANGELOG.md` — the 1.5.1 release ships with Part 2. + +--- + +## Self-Review + +**1. Spec coverage:** §4 scenarios 1–9 → Task 1 covers the pure-decision form of 1–8; Task 2 (WASM) covers 1,2,4,6,9 end-to-end; Task 3 (native) covers 1,6,9 at the SQL-shape level. §3.2 atomicity → Task 2 SAVEPOINT, Task 3 single `execBatch`. §3.1 eligibility/`isObject` → Task 1. §5 limitation → comment in `computeJsonPatchUndo`. §6 reuse/rewrite → Task 1 additive on `main`; `isObject` fix in Task 1; plumbing already on `main`. §7 testing → Tasks 1–4. No uncovered spec requirement. + +**2. Placeholder scan:** no TBD/TODO; the one "Codex note" in Task 3 is a concrete confirmation instruction (mock response shape) with the binding assertions stated, not a deferred decision. + +**3. Type consistency:** `computeJsonPatchUndo(currentRaw, forwardPatchRaw, priorRaw): JsonUndoPlan` used identically in Tasks 1/2/3. `JsonUndoPlan = {kind:'restore';value:string} | {kind:'replace'}`. `restoreInto`/`parseJsonObject` private to `json-utils.ts`. Engine writes use `updateCell(table,row,col,value)` (WASM) and `worker.call('run'/'execBatch', …)` (native) — matching the confirmed signatures. + +--- + +## Notes for the implementer + +- `validateRowId` and `escapeIdentifier` are already imported in both engine files — reuse them. +- Do not introduce `json_patch()` SQL anywhere on the undo path; RMW writes full objects or value-replaces. +- Preserve existing behavior for non-`json_patch` cell undos (byte-for-byte) and for all non-cell modification types. From 759acc660f41b2f2649e9d0cf2dd4fcddb7e3a09 Mon Sep 17 00:00:00 2001 From: zknpr Date: Wed, 3 Jun 2026 11:22:08 +0200 Subject: [PATCH 03/12] feat(undo): pure read-modify-write json_patch undo decision helper computeJsonPatchUndo restores only the forward patch's touched keys from the recorded prior, preserving concurrent sibling edits; value-replaces when current, prior, or the forward patch is not a JSON object. isObject now excludes arrays (RFC 7396 atomic). Pure + exhaustively unit-tested; engine wiring follows. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/json-utils.ts | 86 +++++++++++++++++++++++++++++++- tests/unit/json_utils.test.ts | 92 ++++++++++++++++++++++++++++++++++- 2 files changed, 176 insertions(+), 2 deletions(-) diff --git a/src/core/json-utils.ts b/src/core/json-utils.ts index b386c34..8993a3e 100644 --- a/src/core/json-utils.ts +++ b/src/core/json-utils.ts @@ -108,6 +108,90 @@ export function applyMergePatch(target: unknown, patch: unknown, depth = 0): unk return targetObj; } +export type JsonUndoPlan = + | { kind: 'restore'; value: string } + | { kind: 'replace' }; + +/** Parse a raw cell value to a plain JSON object, or undefined if it is not one. */ +function parseJsonObject(raw: unknown): Record | undefined { + if (typeof raw !== 'string') return undefined; + try { + const parsed = JSON.parse(raw); + return isObject(parsed) ? parsed : undefined; + } catch { + return undefined; + } +} + +/** + * Decide how to undo a forward json_patch cell edit by read-modify-write. + * + * Surgical restore requires current, forwardPatch, and prior to all be JSON + * objects; otherwise we value-replace to the recorded prior. When prior is SQL + * NULL, scalar, or array, value-replace deliberately preserves the previous + * behavior for that extreme non-object corner. + * + * @param currentRaw the cell value read from the DB now + * @param forwardPatchRaw the recorded forward merge patch + * @param priorRaw the recorded full prior cell value + */ +export function computeJsonPatchUndo( + currentRaw: unknown, + forwardPatchRaw: unknown, + priorRaw: unknown +): JsonUndoPlan { + const current = parseJsonObject(currentRaw); + const forwardPatch = parseJsonObject(forwardPatchRaw); + const prior = parseJsonObject(priorRaw); + if (!current || !forwardPatch || !prior) { + return { kind: 'replace' }; + } + return { kind: 'restore', value: JSON.stringify(restoreInto(current, forwardPatch, prior, 0)) }; +} + +/** + * Walk only the forward patch's key structure, restoring each touched key from + * prior into a clone of current; keys absent from the patch are untouched. + */ +function restoreInto( + currentObj: Record, + patchObj: Record, + priorObj: Record, + depth: number +): Record { + if (depth > MAX_DEPTH) { + throw new Error('JSON undo restore depth limit exceeded'); + } + const result: Record = { ...currentObj }; + for (const key of Object.keys(patchObj)) { + const pv = patchObj[key]; + const priorHas = Object.prototype.hasOwnProperty.call(priorObj, key); + const priorVal = priorHas ? priorObj[key] : undefined; + + if (isObject(pv)) { + if (priorHas && isObject(priorVal)) { + const base = isObject(result[key]) ? (result[key] as Record) : {}; + result[key] = restoreInto(base, pv, priorVal, depth + 1); + } else if (priorHas) { + result[key] = priorVal; + } else { + const base = isObject(result[key]) ? (result[key] as Record) : {}; + const child = restoreInto(base, pv, {}, depth + 1); + if (Object.keys(child).length === 0) { + delete result[key]; + } else { + result[key] = child; + } + } + } else if (priorHas) { + result[key] = priorVal; + } else { + delete result[key]; + } + } + return result; +} + function isObject(val: unknown): val is Record { - return val !== null && typeof val === 'object'; + return val !== null && typeof val === 'object' && !Array.isArray(val); } diff --git a/tests/unit/json_utils.test.ts b/tests/unit/json_utils.test.ts index 78d2d59..1644df0 100644 --- a/tests/unit/json_utils.test.ts +++ b/tests/unit/json_utils.test.ts @@ -1,7 +1,7 @@ import { describe, it } from 'node:test'; import assert from 'node:assert'; -import { generateMergePatch, applyMergePatch } from '../../src/core/json-utils'; +import { generateMergePatch, applyMergePatch, computeJsonPatchUndo } from '../../src/core/json-utils'; describe('JSON Merge Patch (RFC 7396)', () => { describe('generateMergePatch', () => { @@ -104,3 +104,93 @@ describe('JSON Merge Patch (RFC 7396)', () => { }); }); }); + +describe('computeJsonPatchUndo (RMW undo decision)', () => { + const j = (v: unknown) => JSON.stringify(v); + const expectRestore = (plan: ReturnType, expected: unknown) => { + assert.strictEqual(plan.kind, 'restore'); + assert.deepStrictEqual(JSON.parse((plan as { kind: 'restore'; value: string }).value), expected); + }; + const expectReplace = (plan: ReturnType) => + assert.strictEqual(plan.kind, 'replace'); + + it('s1: restores the edited key and preserves a concurrent sibling key', () => { + expectRestore( + computeJsonPatchUndo( + j({ status: 'published', owner: 'ada', reviewer: 'grace' }), // current (forward + concurrent) + j({ status: 'published' }), // forward patch (newValue) + j({ status: 'draft', owner: 'ada' }) // prior + ), + { status: 'draft', owner: 'ada', reviewer: 'grace' } + ); + }); + + it('s2: removes a wholly-added object key (empty-object collapse)', () => { + expectRestore( + computeJsonPatchUndo(j({ meta: { reviewed: true } }), j({ meta: { reviewed: true } }), j({})), + {} + ); + }); + + it('s3: removes only the added nested leaf, keeping a concurrent nested sibling', () => { + expectRestore( + computeJsonPatchUndo(j({ meta: { reviewed: true, note: 'keep' } }), j({ meta: { reviewed: true } }), j({})), + { meta: { note: 'keep' } } + ); + }); + + it('s4: restores an explicit null at the edited key while preserving a concurrently added sibling', () => { + // c exists in current but not prior — added concurrently after the tracked edit. + // Blind value-replacement to prior would drop c; RMW restores a=null and keeps c. + expectRestore( + computeJsonPatchUndo(j({ a: 2, b: 1, c: 3 }), j({ a: 2 }), j({ a: null, b: 1 })), + { a: null, b: 1, c: 3 } + ); + }); + + it('s5: restores a nested explicit null while preserving a concurrently added nested sibling', () => { + // meta.c was added concurrently; RMW restores meta.a=null and keeps meta.c. + expectRestore( + computeJsonPatchUndo( + j({ meta: { a: 2, keep: 1, c: 3 } }), + j({ meta: { a: 2 } }), + j({ meta: { a: null, keep: 1 } }) + ), + { meta: { a: null, keep: 1, c: 3 } } + ); + }); + + it('s6: value-replaces when the current cell is not a JSON object', () => { + for (const current of ['plain text', null, '5', '[1,2]']) { + expectReplace(computeJsonPatchUndo(current, j({ status: 'published' }), j({ status: 'draft' }))); + } + }); + + it('s7: value-replaces when the forward patch was a scalar/array whole-doc replacement', () => { + expectReplace(computeJsonPatchUndo(j({ a: 1 }), '5', j({ a: 1 }))); + expectReplace(computeJsonPatchUndo(j({ a: 1 }), '[1,2]', j({ a: 1 }))); + }); + + it('s8: value-replaces when the prior cell was SQL NULL / non-object', () => { + expectReplace(computeJsonPatchUndo(j({ added: true }), j({ added: true }), null)); + expectReplace(computeJsonPatchUndo(j({ added: true }), j({ added: true }), '5')); + }); + + it('invariant: round-trips an object edit (no concurrent change) back to prior', () => { + const prior = { status: 'draft', meta: { reviewed: false, owner: 'ada' } }; + const forward = generateMergePatch(prior, { status: 'published', meta: { reviewed: true, owner: 'ada' } }); + const current = applyMergePatch(prior, forward); + expectRestore(computeJsonPatchUndo(j(current), j(forward), j(prior)), prior); + }); + + it('invariant: never touches keys outside the forward patch structure', () => { + expectRestore( + computeJsonPatchUndo( + j({ a: 'forward-changed', untouched: { deep: 1 }, sibling: 2 }), + j({ a: 'forward-changed' }), + j({ a: 'orig' }) + ), + { a: 'orig', untouched: { deep: 1 }, sibling: 2 } + ); + }); +}); From 58e402356b4f0415f6c224575b8566a3088ae0b6 Mon Sep 17 00:00:00 2001 From: zknpr Date: Wed, 3 Jun 2026 11:39:11 +0200 Subject: [PATCH 04/12] feat(undo): operation-aware json_patch cell undo in the WASM engine undoCellUpdate now reads the current cell and routes json_patch undos through computeJsonPatchUndo (full-object SET), preserving concurrent sibling edits; non-json_patch and non-object-current cells value-replace. Batch undos that touch any json_patch cell are wrapped in a SAVEPOINT for atomicity. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/engine/wasm/WasmDatabaseEngine.ts | 80 +++++++++-- tests/unit/sqlite_db.test.ts | 153 +++++++++++++++++++++ 2 files changed, 222 insertions(+), 11 deletions(-) diff --git a/src/core/engine/wasm/WasmDatabaseEngine.ts b/src/core/engine/wasm/WasmDatabaseEngine.ts index a485a8e..ab837ce 100644 --- a/src/core/engine/wasm/WasmDatabaseEngine.ts +++ b/src/core/engine/wasm/WasmDatabaseEngine.ts @@ -24,7 +24,7 @@ import type { } from '../../types'; import { escapeIdentifier, validateSqlType, validateRowId, validateRowIds } from '../../sql-utils'; import { buildSelectQuery, buildCountQuery } from '../../query-builder'; -import { applyMergePatch } from '../../json-utils'; +import { applyMergePatch, computeJsonPatchUndo } from '../../json-utils'; import { getNodeFs } from '../../platform/fs'; // ============================================================================ @@ -282,19 +282,77 @@ export class WasmDatabaseEngine implements DatabaseOperations { } private async undoCellUpdate(targetTable: string, mod: ModificationEntry): Promise { - const { affectedCells, targetRowId, targetColumn, priorValue } = mod; + const { affectedCells, targetRowId, targetColumn, priorValue, newValue, operation } = mod; if (affectedCells) { - // Batch undo - const updates = affectedCells.map(cell => ({ - rowId: cell.rowId, - column: cell.columnName, - value: cell.priorValue ?? null - })); - await this.updateCellBatch(targetTable, updates); + const hasJsonPatchUndo = affectedCells.some(cell => cell.operation === 'json_patch'); + if (hasJsonPatchUndo) { + // JSON-patch undo reads current cells before writing restored values, so + // the whole history entry is protected by one savepoint for atomicity. + const savepointName = this.createSavepointName('sp_undo_cell_update'); + await this.executeQuery(`SAVEPOINT ${savepointName}`); + try { + for (const cell of affectedCells) { + await this.undoOneCell( + targetTable, + cell.rowId, + cell.columnName, + cell.priorValue, + cell.newValue, + cell.operation + ); + } + await this.executeQuery(`RELEASE ${savepointName}`); + } catch (err) { + await this.safeRollbackSavepoint(savepointName, 'undoCellUpdate'); + throw err; + } + } else { + // Pure value-replacement batches keep the existing batch update path. + const updates = affectedCells.map(cell => ({ + rowId: cell.rowId, + column: cell.columnName, + value: cell.priorValue ?? null + })); + await this.updateCellBatch(targetTable, updates); + } } else if (targetRowId !== undefined && targetColumn) { - // Single cell undo - await this.updateCell(targetTable, targetRowId, targetColumn, priorValue ?? null); + await this.undoOneCell(targetTable, targetRowId, targetColumn, priorValue, newValue, operation); + } + } + + /** + * Undo one cell update. JSON-patch entries use read-modify-write so only the + * originally edited keys are restored; all other cell updates write priorValue. + */ + private async undoOneCell( + targetTable: string, + rowId: RecordId, + column: string, + priorValue: CellValue | undefined, + newValue: CellValue | undefined, + operation: ModificationEntry['operation'] + ): Promise { + if (operation === 'json_patch') { + const currentValue = await this.readCellValue(targetTable, rowId, column); + const plan = computeJsonPatchUndo(currentValue, newValue, priorValue); + if (plan.kind === 'restore') { + await this.updateCell(targetTable, rowId, column, plan.value); + return; + } } + + await this.updateCell(targetTable, rowId, column, priorValue ?? null); + } + + /** + * Read the current value of one cell using validated rowid and escaped identifiers. + */ + private async readCellValue(table: string, rowId: RecordId, column: string): Promise { + const result = await this.executeQuery( + `SELECT ${escapeIdentifier(column)} FROM ${escapeIdentifier(table)} WHERE rowid = ?`, + [validateRowId(rowId)] + ); + return (result[0]?.rows[0]?.[0] ?? null) as CellValue; } private async undoRowInsert(targetTable: string, mod: ModificationEntry): Promise { diff --git a/tests/unit/sqlite_db.test.ts b/tests/unit/sqlite_db.test.ts index 944b9f3..2f504c7 100644 --- a/tests/unit/sqlite_db.test.ts +++ b/tests/unit/sqlite_db.test.ts @@ -243,4 +243,157 @@ describe('WasmDatabaseEngine', () => { engine.insertRow = originalInsertRow; } }); + + it('undo single json_patch edit preserves a concurrent sibling key (s1)', async () => { + // The tracked edit changes only status; the later reviewer key must survive undo. + await engine.executeQuery('DELETE FROM users'); + const prior = JSON.stringify({ status: 'draft', owner: 'ada' }); + const forward = JSON.stringify({ status: 'published' }); + await engine.insertRow('users', { id: 1, name: 'A', age: 30, data: prior }); + await engine.updateCell('users', 1, 'data', null, forward); + await engine.updateCell('users', 1, 'data', null, JSON.stringify({ reviewer: 'grace' })); + + await engine.undoModification({ + modificationType: 'cell_update', + description: 'undo status', + targetTable: 'users', + targetRowId: 1, + targetColumn: 'data', + priorValue: prior, + newValue: forward, + operation: 'json_patch' + }); + + const result = await engine.executeQuery('SELECT data FROM users WHERE id = 1'); + assert.deepStrictEqual(JSON.parse(result[0].rows[0][0] as string), { + status: 'draft', + owner: 'ada', + reviewer: 'grace' + }); + }); + + it('undo of a wholly-added object key removes it entirely (s2)', async () => { + // A forward patch that introduced an object with no surviving concurrent child should remove it. + await engine.executeQuery('DELETE FROM users'); + const forward = JSON.stringify({ meta: { reviewed: true } }); + await engine.insertRow('users', { id: 1, name: 'A', age: 30, data: '{}' }); + await engine.updateCell('users', 1, 'data', null, forward); + + await engine.undoModification({ + modificationType: 'cell_update', + description: 'undo meta', + targetTable: 'users', + targetRowId: 1, + targetColumn: 'data', + priorValue: '{}', + newValue: forward, + operation: 'json_patch' + }); + + const result = await engine.executeQuery('SELECT data FROM users WHERE id = 1'); + assert.deepStrictEqual(JSON.parse(result[0].rows[0][0] as string), {}); + }); + + it('undo restores an explicit null at the edited key while preserving a concurrent sibling (s4)', async () => { + // Explicit null is a stored value here, not a merge-patch delete marker. c is added + // concurrently after the tracked edit; blind value-replacement to prior would drop it. + await engine.executeQuery('DELETE FROM users'); + const prior = JSON.stringify({ a: null, b: 1 }); + const forward = JSON.stringify({ a: 2 }); + await engine.insertRow('users', { id: 1, name: 'A', age: 30, data: prior }); + await engine.updateCell('users', 1, 'data', null, forward); + await engine.updateCell('users', 1, 'data', null, JSON.stringify({ c: 3 })); + + await engine.undoModification({ + modificationType: 'cell_update', + description: 'undo explicit null', + targetTable: 'users', + targetRowId: 1, + targetColumn: 'data', + priorValue: prior, + newValue: forward, + operation: 'json_patch' + }); + + const result = await engine.executeQuery('SELECT data FROM users WHERE id = 1'); + const parsed = JSON.parse(result[0].rows[0][0] as string); + assert.deepStrictEqual(parsed, { a: null, b: 1, c: 3 }); + assert.ok(Object.prototype.hasOwnProperty.call(parsed, 'a')); + }); + + it('undo value-replaces when the cell became non-JSON since the edit (s6)', async () => { + // Surgical restore is unsafe when the current cell is not an object, so undo writes the recorded prior. + await engine.executeQuery('DELETE FROM users'); + const prior = JSON.stringify({ status: 'draft', owner: 'ada' }); + const forward = JSON.stringify({ status: 'published' }); + await engine.insertRow('users', { id: 1, name: 'A', age: 30, data: prior }); + await engine.updateCell('users', 1, 'data', null, forward); + await engine.updateCell('users', 1, 'data', 'plain text'); + + await engine.undoModification({ + modificationType: 'cell_update', + description: 'undo non-object current', + targetTable: 'users', + targetRowId: 1, + targetColumn: 'data', + priorValue: prior, + newValue: forward, + operation: 'json_patch' + }); + + const result = await engine.executeQuery('SELECT data FROM users WHERE id = 1'); + assert.deepStrictEqual(JSON.parse(result[0].rows[0][0] as string), { + status: 'draft', + owner: 'ada' + }); + }); + + it('batch undo restores each json_patch cell, keeps concurrent siblings, and is atomic (s9)', async () => { + // Two read-modify-write undos in one history entry must restore only the count keys. + await engine.executeQuery('DELETE FROM users'); + const p1 = JSON.stringify({ count: 1, stable: 'one' }); + const p2 = JSON.stringify({ count: 10, stable: 'two' }); + await engine.insertRow('users', { id: 1, name: 'A', age: 30, data: p1 }); + await engine.insertRow('users', { id: 2, name: 'B', age: 31, data: p2 }); + await engine.updateCellBatch('users', [ + { rowId: 1, column: 'data', value: JSON.stringify({ count: 2 }), operation: 'json_patch' }, + { rowId: 2, column: 'data', value: JSON.stringify({ count: 11 }), operation: 'json_patch' } + ]); + await engine.updateCell('users', 1, 'data', null, JSON.stringify({ concurrent: 'one' })); + await engine.updateCell('users', 2, 'data', null, JSON.stringify({ concurrent: 'two' })); + + await engine.undoModification({ + modificationType: 'cell_update', + description: 'undo batch', + targetTable: 'users', + affectedCells: [ + { + rowId: 1, + columnName: 'data', + priorValue: p1, + newValue: JSON.stringify({ count: 2 }), + operation: 'json_patch' + }, + { + rowId: 2, + columnName: 'data', + priorValue: p2, + newValue: JSON.stringify({ count: 11 }), + operation: 'json_patch' + } + ] + }); + + const result = await engine.executeQuery('SELECT id, data FROM users ORDER BY id'); + assert.deepStrictEqual(JSON.parse(result[0].rows[0][1] as string), { + count: 1, + stable: 'one', + concurrent: 'one' + }); + assert.deepStrictEqual(JSON.parse(result[0].rows[1][1] as string), { + count: 10, + stable: 'two', + concurrent: 'two' + }); + }); }); From 80d821f7120e48f5a6e3d349de0687d91538782b Mon Sep 17 00:00:00 2001 From: zknpr Date: Wed, 3 Jun 2026 11:39:12 +0200 Subject: [PATCH 05/12] feat(undo): operation-aware json_patch cell undo in the native engine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit undoModification cell_update now reads current then writes the restored object (or value-replaces) via computeJsonPatchUndo — matching the WASM engine. Batch json_patch undos write through one execBatch for atomicity. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/nativeWorker.ts | 86 +++++++++++++++--- tests/unit/nativeWorker.test.ts | 156 +++++++++++++++++++++++++++++++- 2 files changed, 222 insertions(+), 20 deletions(-) diff --git a/src/nativeWorker.ts b/src/nativeWorker.ts index cedaa20..630704f 100644 --- a/src/nativeWorker.ts +++ b/src/nativeWorker.ts @@ -38,6 +38,7 @@ import type { } from './core/types'; import { escapeIdentifier, validateSqlType, validateRowId, validateRowIds } from './core/sql-utils'; import { buildSelectQuery, buildCountQuery } from './core/query-builder'; +import { computeJsonPatchUndo } from './core/json-utils'; // ============================================================================ // Utility Functions @@ -535,30 +536,85 @@ export async function createNativeDatabaseConnection( * Undo a modification by executing the inverse SQL. */ undoModification: async (mod: ModificationEntry) => { - const { modificationType, targetTable, targetRowId, targetColumn, priorValue, affectedCells, deletedRows, columnDef, deletedColumns } = mod; + const { modificationType, targetTable, targetRowId, targetColumn, priorValue, newValue, operation, affectedCells, deletedRows, columnDef, deletedColumns } = mod; if (!targetTable) return; switch (modificationType) { - case 'cell_update': + case 'cell_update': { + // Keep this undo interpretation paired with + // WasmDatabaseEngine.undoCellUpdate so ModificationEntry + // fields keep one interpretation across desktop and web undo. + const resolveUndoValue = async ( + rowId: RecordId, + column: string, + cellPrior: CellValue | undefined, + cellNew: CellValue | undefined, + cellOperation: ModificationEntry['operation'] + ): Promise => { + if (cellOperation === 'json_patch') { + const rowIdNum = validateRowId(rowId); + const read = await operationsFacade.executeQuery( + `SELECT ${escapeIdentifier(column)} FROM ${escapeIdentifier(targetTable)} WHERE rowid = ?`, + [rowIdNum] + ); + const currentValue = (read[0]?.rows[0]?.[0] ?? null) as CellValue; + const plan = computeJsonPatchUndo(currentValue, cellNew, cellPrior); + if (plan.kind === 'restore') { + return plan.value; + } + } + return cellPrior ?? null; + }; + if (affectedCells) { - // Batch undo - const updates = affectedCells.map(c => ({ - rowId: c.rowId, - column: c.columnName, - value: c.priorValue - } as CellUpdate)); - await worker.call('execBatch', [ - updates.map(u => ({ - sql: `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(u.column)} = ? WHERE rowid = ?`, - params: [u.value, Number(u.rowId)] - })) - ]); + const hasJsonPatchUndo = affectedCells.some(cell => cell.operation === 'json_patch'); + if (hasJsonPatchUndo) { + // Read and compute all values first, then send one execBatch + // so native SQLite applies the write set atomically. + const batchItems: { sql: string; params: CellValue[] }[] = []; + for (const cell of affectedCells) { + const rowIdNum = validateRowId(cell.rowId); + const value = await resolveUndoValue( + cell.rowId, + cell.columnName, + cell.priorValue, + cell.newValue, + cell.operation + ); + batchItems.push({ + sql: `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(cell.columnName)} = ? WHERE rowid = ?`, + params: [value, rowIdNum] + }); + } + if (batchItems.length > 0) { + await worker.call('execBatch', [batchItems]); + } + } else { + // Pure value-replacement batches keep the existing SQL shape. + const updates = affectedCells.map(c => ({ + rowId: c.rowId, + column: c.columnName, + value: c.priorValue + } as CellUpdate)); + await worker.call('execBatch', [ + updates.map(u => ({ + sql: `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(u.column)} = ? WHERE rowid = ?`, + params: [u.value, Number(u.rowId)] + })) + ]); + } } else if (targetRowId !== undefined && targetColumn) { const rowIdNum = Number(targetRowId); const sql = `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(targetColumn)} = ? WHERE rowid = ?`; - await worker.call('run', [sql, [priorValue, rowIdNum]]); + if (operation === 'json_patch') { + const value = await resolveUndoValue(targetRowId, targetColumn, priorValue, newValue, operation); + await worker.call('run', [sql, [value, validateRowId(targetRowId)]]); + } else { + await worker.call('run', [sql, [priorValue, rowIdNum]]); + } } break; + } case 'row_insert': if (targetRowId !== undefined) { diff --git a/tests/unit/nativeWorker.test.ts b/tests/unit/nativeWorker.test.ts index 13d3c03..ef8fb28 100644 --- a/tests/unit/nativeWorker.test.ts +++ b/tests/unit/nativeWorker.test.ts @@ -15,6 +15,8 @@ interface RecordedNativeCall { args: unknown[]; } +type RecordedNativeResponder = (call: RecordedNativeCall) => { result?: unknown; error?: string }; + function encodeNativeMessage(message: unknown): Buffer { // Native worker messages are length-prefixed V8 payloads, matching NativeWorkerProcess.writeMessage. const payload = v8.serialize(message); @@ -23,7 +25,7 @@ function encodeNativeMessage(message: unknown): Buffer { return Buffer.concat([header, payload]); } -function createRecordingNativeProcess(recordedCalls: RecordedNativeCall[]) { +function createRecordingNativeProcess(recordedCalls: RecordedNativeCall[], respondToCall?: RecordedNativeResponder) { const mockProcess = new EventEmitter() as any; mockProcess.stdout = new EventEmitter(); mockProcess.stderr = new EventEmitter(); @@ -49,9 +51,10 @@ function createRecordingNativeProcess(recordedCalls: RecordedNativeCall[]) { const call = v8.deserialize(payload) as RecordedNativeCall; recordedCalls.push(call); - // Every operation under test only needs a successful native response. + // Tests can provide per-method native responses; otherwise use a generic write success. queueMicrotask(() => { - emitMessage({ id: call.id, result: { changes: 1, lastInsertRowId: 1 } }); + const response = respondToCall?.(call) ?? { result: { changes: 1, lastInsertRowId: 1 } }; + emitMessage({ id: call.id, ...response }); }); } }; @@ -197,13 +200,13 @@ describe('createNativeDatabaseConnection', () => { mock.restoreAll(); }); - async function createRecordingConnection(): Promise<{ + async function createRecordingConnection(respondToCall?: RecordedNativeResponder): Promise<{ databaseOps: DatabaseOperations; calls: RecordedNativeCall[]; dispose: () => void; }> { const calls: RecordedNativeCall[] = []; - mock.method(child_process, 'spawn', () => createRecordingNativeProcess(calls)); + mock.method(child_process, 'spawn', () => createRecordingNativeProcess(calls, respondToCall)); const { createNativeDatabaseConnection } = require('../../src/nativeWorker'); const bundle = await createNativeDatabaseConnection({ fsPath: tempDir } as any); @@ -284,6 +287,149 @@ describe('createNativeDatabaseConnection', () => { bundle.workerMethods[Symbol.dispose](); }); + it('undoes a single json_patch cell by reading current then writing the restored object', async () => { + // The SELECT response is the current document after the forward edit plus a concurrent key. + const current = { status: 'published', owner: 'ada', reviewer: 'grace' }; + const connection = await createRecordingConnection((call) => { + if (call.method === 'query') { + return { result: { columns: ['payload'], values: [[JSON.stringify(current)]] } }; + } + return { result: { changes: 1, lastInsertRowId: 1 } }; + }); + + try { + connection.calls.length = 0; + await connection.databaseOps.undoModification({ + modificationType: 'cell_update', + description: 'undo payload', + targetTable: 'docs', + targetRowId: 7, + targetColumn: 'payload', + priorValue: JSON.stringify({ status: 'draft', owner: 'ada' }), + newValue: JSON.stringify({ status: 'published' }), + operation: 'json_patch' + }); + + const queryCall = connection.calls.find(call => call.method === 'query'); + const runCall = connection.calls.find(call => call.method === 'run'); + assert.ok(queryCall, 'expected a SELECT read'); + assert.ok(runCall, 'expected a SET write'); + + const [readSql, readParams] = queryCall.args as [string, unknown[]]; + assert.strictEqual(readSql, `SELECT "payload" FROM "docs" WHERE rowid = ?`); + assert.deepStrictEqual(readParams, [7]); + + const [sql, params] = runCall.args as [string, unknown[]]; + assert.strictEqual(sql, `UPDATE "docs" SET "payload" = ? WHERE rowid = ?`); + assert.deepStrictEqual(JSON.parse(params[0] as string), { + status: 'draft', + owner: 'ada', + reviewer: 'grace' + }); + assert.strictEqual(params[1], 7); + } finally { + connection.dispose(); + } + }); + + it('value-replaces a single json_patch undo when the current cell is non-object', async () => { + // A non-object current value makes surgical restore unsafe, but the undo still performs the read. + const connection = await createRecordingConnection((call) => { + if (call.method === 'query') { + return { result: { columns: ['payload'], values: [['plain text']] } }; + } + return { result: { changes: 1, lastInsertRowId: 1 } }; + }); + + try { + const prior = JSON.stringify({ status: 'draft' }); + connection.calls.length = 0; + await connection.databaseOps.undoModification({ + modificationType: 'cell_update', + description: 'undo payload', + targetTable: 'docs', + targetRowId: 7, + targetColumn: 'payload', + priorValue: prior, + newValue: JSON.stringify({ status: 'published' }), + operation: 'json_patch' + }); + + const queryCall = connection.calls.find(call => call.method === 'query'); + const runCall = connection.calls.find(call => call.method === 'run'); + assert.ok(queryCall, 'expected a SELECT read'); + assert.ok(runCall, 'expected a SET write'); + const [sql, params] = runCall.args as [string, unknown[]]; + assert.strictEqual(sql, `UPDATE "docs" SET "payload" = ? WHERE rowid = ?`); + assert.deepStrictEqual(params, [prior, 7]); + } finally { + connection.dispose(); + } + }); + + it('undoes a batch of json_patch cells atomically via execBatch of restored-object writes', async () => { + // Reads happen before the one execBatch write so the native worker applies the batch atomically. + const currents: Record = { + 3: { count: 2, stable: 'one', concurrent: 'a' }, + 4: { count: 11, stable: 'two', concurrent: 'b' } + }; + const connection = await createRecordingConnection((call) => { + if (call.method === 'query') { + const [, params] = call.args as [string, unknown[]]; + return { result: { columns: ['payload'], values: [[JSON.stringify(currents[Number(params[0])])]] } }; + } + return { result: { changes: 1, lastInsertRowId: 1 } }; + }); + + try { + connection.calls.length = 0; + await connection.databaseOps.undoModification({ + modificationType: 'cell_update', + description: 'undo batch payloads', + targetTable: 'docs', + affectedCells: [ + { + rowId: 3, + columnName: 'payload', + priorValue: JSON.stringify({ count: 1, stable: 'one' }), + newValue: JSON.stringify({ count: 2 }), + operation: 'json_patch' + }, + { + rowId: 4, + columnName: 'payload', + priorValue: JSON.stringify({ count: 10, stable: 'two' }), + newValue: JSON.stringify({ count: 11 }), + operation: 'json_patch' + } + ] + }); + + const queryCalls = connection.calls.filter(call => call.method === 'query'); + const batchCall = connection.calls.find(call => call.method === 'execBatch'); + assert.strictEqual(queryCalls.length, 2); + assert.ok(batchCall, 'batch json_patch undo must write through one execBatch'); + + const items = (batchCall.args as [Array<{ sql: string; params: unknown[] }>])[0]; + assert.strictEqual(items.length, 2); + assert.ok(items.every(item => item.sql === `UPDATE "docs" SET "payload" = ? WHERE rowid = ?`)); + assert.deepStrictEqual(JSON.parse(items[0].params[0] as string), { + count: 1, + stable: 'one', + concurrent: 'a' + }); + assert.deepStrictEqual(JSON.parse(items[1].params[0] as string), { + count: 10, + stable: 'two', + concurrent: 'b' + }); + assert.strictEqual(items[0].params[1], 3); + assert.strictEqual(items[1].params[1], 4); + } finally { + connection.dispose(); + } + }); + it('replays single json_patch cell redo through the patch-aware updateCell primitive', async () => { const connection = await createRecordingConnection(); From a389f842b288496a186a0aea8d2b3507bb0c1e1a Mon Sep 17 00:00:00 2001 From: zknpr Date: Wed, 3 Jun 2026 12:24:32 +0200 Subject: [PATCH 06/12] fix(undo): proto-safety, nested-object fallback, and number-precision guard (review) Address PR #427 review of restoreInto/computeJsonPatchUndo: - CR3 (CodeRabbit, Major): plain object literals + spread let a JSON key like __proto__/constructor mutate the temp object's prototype. Build containers with Object.create(null) + hasOwnProperty guards so such keys are treated as data. - CX1 (Codex, P2): when a concurrent edit replaced a patched nested object with a scalar, restore built a partial object and dropped untouched prior siblings. Fall back to the full prior subtree when the current nested value is not an object. - CX3 (Codex, P2): RMW reparsed the whole cell, corrupting JSON integers > 2^53 even in untouched siblings (regression vs verbatim value-replace). hasPrecisionRiskyInteger detects them and value-replaces (byte-exact). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/json-utils.ts | 34 +++++++++++++++--- tests/unit/json_utils.test.ts | 65 +++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 5 deletions(-) diff --git a/src/core/json-utils.ts b/src/core/json-utils.ts index 8993a3e..f590405 100644 --- a/src/core/json-utils.ts +++ b/src/core/json-utils.ts @@ -140,6 +140,13 @@ export function computeJsonPatchUndo( forwardPatchRaw: unknown, priorRaw: unknown ): JsonUndoPlan { + // Large integer tokens cannot round-trip through JSON.parse/JSON.stringify + // safely. In that case undo falls back to the recorded prior string so + // untouched sibling numbers remain byte-exact. + if (hasPrecisionRiskyInteger(currentRaw) || hasPrecisionRiskyInteger(priorRaw)) { + return { kind: 'replace' }; + } + const current = parseJsonObject(currentRaw); const forwardPatch = parseJsonObject(forwardPatchRaw); const prior = parseJsonObject(priorRaw); @@ -162,7 +169,9 @@ function restoreInto( if (depth > MAX_DEPTH) { throw new Error('JSON undo restore depth limit exceeded'); } - const result: Record = { ...currentObj }; + // Null-prototype clones treat keys such as "__proto__" and "constructor" + // as ordinary JSON data instead of inherited accessors/properties. + const result: Record = Object.assign(Object.create(null), currentObj); for (const key of Object.keys(patchObj)) { const pv = patchObj[key]; const priorHas = Object.prototype.hasOwnProperty.call(priorObj, key); @@ -170,13 +179,19 @@ function restoreInto( if (isObject(pv)) { if (priorHas && isObject(priorVal)) { - const base = isObject(result[key]) ? (result[key] as Record) : {}; - result[key] = restoreInto(base, pv, priorVal, depth + 1); + if (Object.prototype.hasOwnProperty.call(result, key) && isObject(result[key])) { + const base = result[key] as Record; + result[key] = restoreInto(base, pv, priorVal, depth + 1); + } else { + result[key] = priorVal; + } } else if (priorHas) { result[key] = priorVal; } else { - const base = isObject(result[key]) ? (result[key] as Record) : {}; - const child = restoreInto(base, pv, {}, depth + 1); + const base = (Object.prototype.hasOwnProperty.call(result, key) && isObject(result[key])) + ? (result[key] as Record) + : Object.create(null); + const child = restoreInto(base, pv, Object.create(null), depth + 1); if (Object.keys(child).length === 0) { delete result[key]; } else { @@ -192,6 +207,15 @@ function restoreInto( return result; } +function hasPrecisionRiskyInteger(raw: unknown): boolean { + if (typeof raw !== 'string') return false; + // Neutralize string literals so only structural JSON number tokens are + // inspected for integer runs that JavaScript cannot represent exactly. + const structural = raw.replace(/"(?:\\.|[^"\\])*"/g, '""'); + const digitRuns = structural.match(/\d{16,}/g); + return !!digitRuns && digitRuns.some(run => !Number.isSafeInteger(Number(run))); +} + function isObject(val: unknown): val is Record { return val !== null && typeof val === 'object' && !Array.isArray(val); } diff --git a/tests/unit/json_utils.test.ts b/tests/unit/json_utils.test.ts index 1644df0..14d0e37 100644 --- a/tests/unit/json_utils.test.ts +++ b/tests/unit/json_utils.test.ts @@ -176,6 +176,71 @@ describe('computeJsonPatchUndo (RMW undo decision)', () => { expectReplace(computeJsonPatchUndo(j({ added: true }), j({ added: true }), '5')); }); + it('restores literal __proto__ keys as data without mutating prototypes', () => { + const priorRaw = '{"a":1,"__proto__":{"x":9}}'; + const forwardRaw = '{"__proto__":{"x":10}}'; + const currentRaw = '{"a":1,"b":2}'; + + const beforePrototype = ({} as Record).x; + const plan = computeJsonPatchUndo(currentRaw, forwardRaw, priorRaw); + + assert.strictEqual(plan.kind, 'restore'); + const restoredRaw = (plan as { kind: 'restore'; value: string }).value; + assert.strictEqual(restoredRaw, '{"a":1,"b":2,"__proto__":{"x":9}}'); + const restored = JSON.parse(restoredRaw); + assert.deepStrictEqual(restored.__proto__, { x: 9 }); + assert.strictEqual(({} as Record).x, beforePrototype); + }); + + it('preserves literal __proto__ and constructor keys already present on the current cell', () => { + const priorRaw = '{"a":1,"constructor":{"safe":1,"owner":"ada"}}'; + const forwardRaw = '{"b":2,"constructor":{"safe":2}}'; + const currentRaw = '{"a":1,"b":2,"__proto__":{"x":9},"constructor":{"safe":2,"owner":"ada","keep":3}}'; + + const plan = computeJsonPatchUndo(currentRaw, forwardRaw, priorRaw); + + assert.strictEqual(plan.kind, 'restore'); + const restoredRaw = (plan as { kind: 'restore'; value: string }).value; + assert.strictEqual(restoredRaw.includes('"__proto__":{"x":9}'), true); + expectRestore( + plan, + JSON.parse('{"a":1,"__proto__":{"x":9},"constructor":{"safe":1,"owner":"ada","keep":3}}') + ); + assert.strictEqual(({} as Record).x, undefined); + }); + + it('restores the full prior subtree when the current nested value is no longer an object', () => { + expectRestore( + computeJsonPatchUndo( + j({ meta: 'archived' }), + j({ meta: { reviewed: true } }), + j({ meta: { reviewed: false, owner: 'ada' } }) + ), + { meta: { reviewed: false, owner: 'ada' } } + ); + }); + + it('value-replaces when untouched integer tokens cannot survive JSON parse/stringify exactly', () => { + expectReplace( + computeJsonPatchUndo( + '{"id":9007199254740993,"a":2}', + j({ a: 2 }), + '{"id":9007199254740993,"a":1}' + ) + ); + }); + + it('still performs a surgical restore when object cells have no precision-risky integers', () => { + expectRestore( + computeJsonPatchUndo( + j({ id: 42, a: 2, untouched: 'keep' }), + j({ a: 2 }), + j({ id: 42, a: 1 }) + ), + { id: 42, a: 1, untouched: 'keep' } + ); + }); + it('invariant: round-trips an object edit (no concurrent change) back to prior', () => { const prior = { status: 'draft', meta: { reviewed: false, owner: 'ada' } }; const forward = generateMergePatch(prior, { status: 'published', meta: { reviewed: true, owner: 'ada' } }); From 2e52b4449dbd83b129e467d9ffc24b3dc551b353 Mon Sep 17 00:00:00 2001 From: zknpr Date: Wed, 3 Jun 2026 12:24:32 +0200 Subject: [PATCH 07/12] refactor(undo): WASM batch undo via updateCellBatch (review) Address PR #427 review G1 (Gemini): the manual SAVEPOINT + per-cell updateCell loop duplicated atomicity that updateCellBatch already provides. Compute each cell's undo value, then write via a single updateCellBatch. sqlite_db tests cover the nested-object fallback and precision-risky-integer cases through the real engine. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/engine/wasm/WasmDatabaseEngine.ts | 66 +++++------- tests/unit/sqlite_db.test.ts | 113 +++++++++++++++++++++ 2 files changed, 141 insertions(+), 38 deletions(-) diff --git a/src/core/engine/wasm/WasmDatabaseEngine.ts b/src/core/engine/wasm/WasmDatabaseEngine.ts index ab837ce..5f49a88 100644 --- a/src/core/engine/wasm/WasmDatabaseEngine.ts +++ b/src/core/engine/wasm/WasmDatabaseEngine.ts @@ -284,64 +284,54 @@ export class WasmDatabaseEngine implements DatabaseOperations { private async undoCellUpdate(targetTable: string, mod: ModificationEntry): Promise { const { affectedCells, targetRowId, targetColumn, priorValue, newValue, operation } = mod; if (affectedCells) { - const hasJsonPatchUndo = affectedCells.some(cell => cell.operation === 'json_patch'); - if (hasJsonPatchUndo) { - // JSON-patch undo reads current cells before writing restored values, so - // the whole history entry is protected by one savepoint for atomicity. - const savepointName = this.createSavepointName('sp_undo_cell_update'); - await this.executeQuery(`SAVEPOINT ${savepointName}`); - try { - for (const cell of affectedCells) { - await this.undoOneCell( - targetTable, - cell.rowId, - cell.columnName, - cell.priorValue, - cell.newValue, - cell.operation - ); - } - await this.executeQuery(`RELEASE ${savepointName}`); - } catch (err) { - await this.safeRollbackSavepoint(savepointName, 'undoCellUpdate'); - throw err; - } - } else { - // Pure value-replacement batches keep the existing batch update path. - const updates = affectedCells.map(cell => ({ + const updates: CellUpdate[] = []; + for (const cell of affectedCells) { + updates.push({ rowId: cell.rowId, column: cell.columnName, - value: cell.priorValue ?? null - })); - await this.updateCellBatch(targetTable, updates); + value: await this.computeUndoValue( + targetTable, + cell.rowId, + cell.columnName, + cell.priorValue, + cell.newValue, + cell.operation + ) + }); } + await this.updateCellBatch(targetTable, updates); } else if (targetRowId !== undefined && targetColumn) { - await this.undoOneCell(targetTable, targetRowId, targetColumn, priorValue, newValue, operation); + await this.updateCell( + targetTable, + targetRowId, + targetColumn, + await this.computeUndoValue(targetTable, targetRowId, targetColumn, priorValue, newValue, operation) + ); } } /** - * Undo one cell update. JSON-patch entries use read-modify-write so only the - * originally edited keys are restored; all other cell updates write priorValue. + * Compute the value to write for one undo cell. JSON-patch entries read the + * current cell and surgically restore touched keys when that can be done + * exactly; all other cases write the recorded prior value. */ - private async undoOneCell( - targetTable: string, + private async computeUndoValue( + table: string, rowId: RecordId, column: string, priorValue: CellValue | undefined, newValue: CellValue | undefined, operation: ModificationEntry['operation'] - ): Promise { + ): Promise { if (operation === 'json_patch') { - const currentValue = await this.readCellValue(targetTable, rowId, column); + const currentValue = await this.readCellValue(table, rowId, column); const plan = computeJsonPatchUndo(currentValue, newValue, priorValue); if (plan.kind === 'restore') { - await this.updateCell(targetTable, rowId, column, plan.value); - return; + return plan.value; } } - await this.updateCell(targetTable, rowId, column, priorValue ?? null); + return priorValue ?? null; } /** diff --git a/tests/unit/sqlite_db.test.ts b/tests/unit/sqlite_db.test.ts index 2f504c7..8c71352 100644 --- a/tests/unit/sqlite_db.test.ts +++ b/tests/unit/sqlite_db.test.ts @@ -348,6 +348,56 @@ describe('WasmDatabaseEngine', () => { }); }); + it('undo restores the full prior subtree when the current nested value is not an object', async () => { + // The tracked patch only touched meta.reviewed. If the current meta value + // has since become a scalar, undo cannot preserve nested current state and + // must restore the complete prior meta object, including untouched owner. + await engine.executeQuery('DELETE FROM users'); + const prior = JSON.stringify({ meta: { reviewed: false, owner: 'ada' } }); + const forward = JSON.stringify({ meta: { reviewed: true } }); + await engine.insertRow('users', { id: 1, name: 'A', age: 30, data: prior }); + await engine.updateCell('users', 1, 'data', JSON.stringify({ meta: 'archived' })); + + await engine.undoModification({ + modificationType: 'cell_update', + description: 'undo scalar nested current', + targetTable: 'users', + targetRowId: 1, + targetColumn: 'data', + priorValue: prior, + newValue: forward, + operation: 'json_patch' + }); + + const result = await engine.executeQuery('SELECT data FROM users WHERE id = 1'); + assert.deepStrictEqual(JSON.parse(result[0].rows[0][0] as string), { + meta: { reviewed: false, owner: 'ada' } + }); + }); + + it('undo value-replaces cells with precision-risky integer tokens without rounding siblings', async () => { + // The id token is outside JavaScript's safe integer range. Undo must avoid + // JSON.parse/stringify read-modify-write so the untouched id remains byte-exact. + await engine.executeQuery('DELETE FROM users'); + const prior = '{"id":9007199254740993,"a":1}'; + const current = '{"id":9007199254740993,"a":2}'; + await engine.insertRow('users', { id: 1, name: 'A', age: 30, data: current }); + + await engine.undoModification({ + modificationType: 'cell_update', + description: 'undo precision-risky JSON patch', + targetTable: 'users', + targetRowId: 1, + targetColumn: 'data', + priorValue: prior, + newValue: JSON.stringify({ a: 2 }), + operation: 'json_patch' + }); + + const result = await engine.executeQuery('SELECT data FROM users WHERE id = 1'); + assert.strictEqual(result[0].rows[0][0], prior); + }); + it('batch undo restores each json_patch cell, keeps concurrent siblings, and is atomic (s9)', async () => { // Two read-modify-write undos in one history entry must restore only the count keys. await engine.executeQuery('DELETE FROM users'); @@ -396,4 +446,67 @@ describe('WasmDatabaseEngine', () => { concurrent: 'two' }); }); + + it('batch json_patch undo writes the computed restored values through updateCellBatch', async () => { + // The batch primitive owns SAVEPOINT atomicity. Undo computes restored + // values first, then delegates the write set to updateCellBatch once. + await engine.executeQuery('DELETE FROM users'); + const p1 = JSON.stringify({ count: 1, stable: 'one' }); + const p2 = JSON.stringify({ count: 10, stable: 'two' }); + await engine.insertRow('users', { id: 1, name: 'A', age: 30, data: JSON.stringify({ count: 2, stable: 'one' }) }); + await engine.insertRow('users', { id: 2, name: 'B', age: 31, data: JSON.stringify({ count: 11, stable: 'two' }) }); + + const originalUpdateCell = engine.updateCell.bind(engine); + const originalUpdateCellBatch = engine.updateCellBatch.bind(engine); + const batchCalls: CellUpdate[][] = []; + let singleCellWrites = 0; + + engine.updateCell = async (table, rowId, column, value, patch) => { + singleCellWrites++; + return originalUpdateCell(table, rowId, column, value, patch); + }; + engine.updateCellBatch = async (table, updates) => { + batchCalls.push(updates.map(update => ({ ...update }))); + return originalUpdateCellBatch(table, updates); + }; + + try { + await engine.undoModification({ + modificationType: 'cell_update', + description: 'undo batch through primitive', + targetTable: 'users', + affectedCells: [ + { + rowId: 1, + columnName: 'data', + priorValue: p1, + newValue: JSON.stringify({ count: 2 }), + operation: 'json_patch' + }, + { + rowId: 2, + columnName: 'data', + priorValue: p2, + newValue: JSON.stringify({ count: 11 }), + operation: 'json_patch' + } + ] + }); + } finally { + engine.updateCell = originalUpdateCell; + engine.updateCellBatch = originalUpdateCellBatch; + } + + assert.strictEqual(singleCellWrites, 0); + assert.strictEqual(batchCalls.length, 1); + assert.deepStrictEqual(batchCalls[0].map(update => ({ + rowId: update.rowId, + column: update.column, + value: JSON.parse(update.value as string), + operation: update.operation + })), [ + { rowId: 1, column: 'data', value: { count: 1, stable: 'one' }, operation: undefined }, + { rowId: 2, column: 'data', value: { count: 10, stable: 'two' }, operation: undefined } + ]); + }); }); From 335152383ede1c43fd9b5a2bf945443d914a8376 Mon Sep 17 00:00:00 2001 From: zknpr Date: Wed, 3 Jun 2026 12:24:32 +0200 Subject: [PATCH 08/12] fix(undo): serialize native ops + batch reads for json_patch undo (review) Address PR #427 review: - CX2 (Codex, P2): native json_patch undo read+write were separate worker RPCs, so a concurrent same-cell write could interleave and re-introduce the clobber. Add a per-connection promise-chain operation lock around the public operations; internal facade-to-facade calls use the raw (unlocked) implementation to avoid deadlock. Worker ops are already FIFO, so the lock only keeps each operation's messages contiguous. - G2 (Gemini): drop the hasJsonPatchUndo branch; one unified compute/write path. - CR4 (CodeRabbit): batch the per-cell SELECT reads into a single queryBatch round-trip. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/nativeWorker.ts | 169 +++++++++++++++++++++----------- tests/unit/nativeWorker.test.ts | 128 +++++++++++++++++++++--- 2 files changed, 229 insertions(+), 68 deletions(-) diff --git a/src/nativeWorker.ts b/src/nativeWorker.ts index 630704f..38f7610 100644 --- a/src/nativeWorker.ts +++ b/src/nativeWorker.ts @@ -504,8 +504,10 @@ export async function createNativeDatabaseConnection( throw new Error(`Failed to open database "${displayName}": ${message}. Path: ${filePath}`, { cause: err }); } - // Create operations facade - const operationsFacade: DatabaseOperations = { + // Public mutating operations are wrapped below with a per-connection + // promise-chain lock. The raw implementation is used for internal + // facade-to-facade calls so composite operations do not deadlock. + const rawOperations: DatabaseOperations = { engineKind: Promise.resolve('native'), executeQuery: async (sql: string, params?: CellValue[]): Promise => { @@ -544,20 +546,13 @@ export async function createNativeDatabaseConnection( // Keep this undo interpretation paired with // WasmDatabaseEngine.undoCellUpdate so ModificationEntry // fields keep one interpretation across desktop and web undo. - const resolveUndoValue = async ( - rowId: RecordId, - column: string, + const computeUndoValue = ( + currentValue: CellValue, cellPrior: CellValue | undefined, cellNew: CellValue | undefined, cellOperation: ModificationEntry['operation'] - ): Promise => { + ): CellValue => { if (cellOperation === 'json_patch') { - const rowIdNum = validateRowId(rowId); - const read = await operationsFacade.executeQuery( - `SELECT ${escapeIdentifier(column)} FROM ${escapeIdentifier(targetTable)} WHERE rowid = ?`, - [rowIdNum] - ); - const currentValue = (read[0]?.rows[0]?.[0] ?? null) as CellValue; const plan = computeJsonPatchUndo(currentValue, cellNew, cellPrior); if (plan.kind === 'restore') { return plan.value; @@ -567,51 +562,61 @@ export async function createNativeDatabaseConnection( }; if (affectedCells) { - const hasJsonPatchUndo = affectedCells.some(cell => cell.operation === 'json_patch'); - if (hasJsonPatchUndo) { - // Read and compute all values first, then send one execBatch - // so native SQLite applies the write set atomically. - const batchItems: { sql: string; params: CellValue[] }[] = []; - for (const cell of affectedCells) { - const rowIdNum = validateRowId(cell.rowId); - const value = await resolveUndoValue( - cell.rowId, - cell.columnName, + const undoValues: CellValue[] = affectedCells.map(cell => cell.priorValue ?? null); + const jsonPatchIndexes: number[] = []; + const queries: { sql: string; params: CellValue[] }[] = []; + + for (let index = 0; index < affectedCells.length; index++) { + const cell = affectedCells[index]; + if (cell.operation !== 'json_patch') continue; + const rowIdNum = validateRowId(cell.rowId); + jsonPatchIndexes.push(index); + queries.push({ + sql: `SELECT ${escapeIdentifier(cell.columnName)} FROM ${escapeIdentifier(targetTable)} WHERE rowid = ?`, + params: [rowIdNum] + }); + } + + if (queries.length > 0) { + const read = await worker.call('queryBatch', [queries]); + if (!read || !read.results || read.results.length < queries.length) { + throw new Error('json_patch undo failed: queryBatch returned incomplete results'); + } + + for (let queryIndex = 0; queryIndex < queries.length; queryIndex++) { + const cellIndex = jsonPatchIndexes[queryIndex]; + const cell = affectedCells[cellIndex]; + const currentValue = (read.results[queryIndex]?.values?.[0]?.[0] ?? null) as CellValue; + undoValues[cellIndex] = computeUndoValue( + currentValue, cell.priorValue, cell.newValue, cell.operation ); - batchItems.push({ - sql: `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(cell.columnName)} = ? WHERE rowid = ?`, - params: [value, rowIdNum] - }); - } - if (batchItems.length > 0) { - await worker.call('execBatch', [batchItems]); } - } else { - // Pure value-replacement batches keep the existing SQL shape. - const updates = affectedCells.map(c => ({ - rowId: c.rowId, - column: c.columnName, - value: c.priorValue - } as CellUpdate)); - await worker.call('execBatch', [ - updates.map(u => ({ - sql: `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(u.column)} = ? WHERE rowid = ?`, - params: [u.value, Number(u.rowId)] - })) - ]); + } + + const batchItems = affectedCells.map((cell, index) => ({ + sql: `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(cell.columnName)} = ? WHERE rowid = ?`, + params: [undoValues[index], validateRowId(cell.rowId)] + })); + + if (batchItems.length > 0) { + await worker.call('execBatch', [batchItems]); } } else if (targetRowId !== undefined && targetColumn) { - const rowIdNum = Number(targetRowId); + const rowIdNum = validateRowId(targetRowId); const sql = `UPDATE ${escapeIdentifier(targetTable)} SET ${escapeIdentifier(targetColumn)} = ? WHERE rowid = ?`; + let value = priorValue ?? null; if (operation === 'json_patch') { - const value = await resolveUndoValue(targetRowId, targetColumn, priorValue, newValue, operation); - await worker.call('run', [sql, [value, validateRowId(targetRowId)]]); - } else { - await worker.call('run', [sql, [priorValue, rowIdNum]]); + const read = await rawOperations.executeQuery( + `SELECT ${escapeIdentifier(targetColumn)} FROM ${escapeIdentifier(targetTable)} WHERE rowid = ?`, + [rowIdNum] + ); + const currentValue = (read[0]?.rows[0]?.[0] ?? null) as CellValue; + value = computeUndoValue(currentValue, priorValue, newValue, operation); } + await worker.call('run', [sql, [value, rowIdNum]]); } break; } @@ -624,7 +629,7 @@ export async function createNativeDatabaseConnection( case 'row_delete': if (deletedRows && deletedRows.length > 0) { - await operationsFacade.insertRowBatch(targetTable, deletedRows.map(dr => dr.row)); + await rawOperations.insertRowBatch(targetTable, deletedRows.map(dr => dr.row)); } break; @@ -682,7 +687,7 @@ export async function createNativeDatabaseConnection( switch (modificationType) { case 'cell_update': if (affectedCells) { - await operationsFacade.updateCellBatch(targetTable, affectedCells.map(c => ({ + await rawOperations.updateCellBatch(targetTable, affectedCells.map(c => ({ rowId: c.rowId, column: c.columnName, value: c.newValue ?? null, @@ -692,9 +697,9 @@ export async function createNativeDatabaseConnection( const replayOperation = normalizeReplayCellOperation(operation); if (replayOperation === 'json_patch') { const patch = typeof newValue === 'string' ? newValue : JSON.stringify(newValue ?? null); - await operationsFacade.updateCell(targetTable, targetRowId, targetColumn, null, patch); + await rawOperations.updateCell(targetTable, targetRowId, targetColumn, null, patch); } else { - await operationsFacade.updateCell(targetTable, targetRowId, targetColumn, newValue ?? null); + await rawOperations.updateCell(targetTable, targetRowId, targetColumn, newValue ?? null); } } break; @@ -702,31 +707,31 @@ export async function createNativeDatabaseConnection( case 'row_insert': if (rowData) { const dataToInsert = targetRowId !== undefined ? { ...rowData, rowid: targetRowId } : rowData; - await operationsFacade.insertRow(targetTable, dataToInsert); + await rawOperations.insertRow(targetTable, dataToInsert); } break; case 'row_delete': if (affectedRowIds) { - await operationsFacade.deleteRows(targetTable, affectedRowIds); + await rawOperations.deleteRows(targetTable, affectedRowIds); } break; case 'column_add': if (targetColumn && columnDef) { - await operationsFacade.addColumn(targetTable, targetColumn, columnDef.type, columnDef.defaultValue); + await rawOperations.addColumn(targetTable, targetColumn, columnDef.type, columnDef.defaultValue); } break; case 'column_drop': if (deletedColumns) { - await operationsFacade.deleteColumns(targetTable, deletedColumns.map(c => c.name), droppedIndexes ?? undefined); + await rawOperations.deleteColumns(targetTable, deletedColumns.map(c => c.name), droppedIndexes ?? undefined); } break; case 'table_create': if (tableDef && tableDef.columns) { - await operationsFacade.createTable(targetTable, tableDef.columns); + await rawOperations.createTable(targetTable, tableDef.columns); } break; } @@ -735,7 +740,7 @@ export async function createNativeDatabaseConnection( flushChanges: async () => {}, discardModifications: async (mods: ModificationEntry[]) => { for (let i = mods.length - 1; i >= 0; i--) { - await operationsFacade.undoModification(mods[i]); + await rawOperations.undoModification(mods[i]); } }, @@ -1210,6 +1215,56 @@ export async function createNativeDatabaseConnection( } }; + // Serialize host-facing operations so multi-message sequences such as + // json_patch undo read/compute/write cannot interleave with another edit. + let operationLock: Promise = Promise.resolve(); + const withOperationLock = async (operation: () => Promise): Promise => { + const previous = operationLock; + let release!: () => void; + operationLock = new Promise(resolve => { + release = resolve; + }); + + await previous; + try { + return await operation(); + } finally { + release(); + } + }; + + const operationsFacade: DatabaseOperations = { + engineKind: rawOperations.engineKind, + executeQuery: (sql, params) => withOperationLock(() => rawOperations.executeQuery(sql, params)), + serializeDatabase: name => withOperationLock(() => rawOperations.serializeDatabase(name)), + applyModifications: (mods, signal) => withOperationLock(() => rawOperations.applyModifications(mods, signal)), + undoModification: mod => withOperationLock(() => rawOperations.undoModification(mod)), + redoModification: mod => withOperationLock(() => rawOperations.redoModification(mod)), + flushChanges: signal => withOperationLock(() => rawOperations.flushChanges(signal)), + discardModifications: (mods, signal) => withOperationLock(() => rawOperations.discardModifications(mods, signal)), + updateCell: (table, rowId, column, value, patch) => + withOperationLock(() => rawOperations.updateCell(table, rowId, column, value, patch)), + insertRow: (table, data) => withOperationLock(() => rawOperations.insertRow(table, data)), + insertRowBatch: (table, rows) => withOperationLock(() => rawOperations.insertRowBatch(table, rows)), + deleteRows: (table, rowIds) => withOperationLock(() => rawOperations.deleteRows(table, rowIds)), + deleteColumns: (table, columns, dropDependentIndexes) => + withOperationLock(() => rawOperations.deleteColumns(table, columns, dropDependentIndexes)), + findDependentIndexes: (table, columns) => + withOperationLock(() => rawOperations.findDependentIndexes(table, columns)), + createTable: (table, columns) => withOperationLock(() => rawOperations.createTable(table, columns)), + updateCellBatch: (table, updates) => withOperationLock(() => rawOperations.updateCellBatch(table, updates)), + addColumn: (table, column, type, defaultValue) => + withOperationLock(() => rawOperations.addColumn(table, column, type, defaultValue)), + fetchTableData: (table, options) => withOperationLock(() => rawOperations.fetchTableData(table, options)), + fetchTableCount: (table, options) => withOperationLock(() => rawOperations.fetchTableCount(table, options)), + fetchSchema: () => withOperationLock(() => rawOperations.fetchSchema()), + getTableInfo: table => withOperationLock(() => rawOperations.getTableInfo(table)), + getPragmas: () => withOperationLock(() => rawOperations.getPragmas()), + setPragma: (pragma, value) => withOperationLock(() => rawOperations.setPragma(pragma, value)), + ping: () => withOperationLock(() => rawOperations.ping()), + writeToFile: path => withOperationLock(() => rawOperations.writeToFile(path)) + }; + return { databaseOps: operationsFacade, isReadOnly: forceReadOnly ?? false diff --git a/tests/unit/nativeWorker.test.ts b/tests/unit/nativeWorker.test.ts index ef8fb28..9a0b9ee 100644 --- a/tests/unit/nativeWorker.test.ts +++ b/tests/unit/nativeWorker.test.ts @@ -15,7 +15,18 @@ interface RecordedNativeCall { args: unknown[]; } -type RecordedNativeResponder = (call: RecordedNativeCall) => { result?: unknown; error?: string }; +type RecordedNativeResponse = { result?: unknown; error?: string }; +type RecordedNativeResponder = (call: RecordedNativeCall) => RecordedNativeResponse | Promise; + +function createDeferred() { + let resolve!: (value: T) => void; + let reject!: (reason?: unknown) => void; + const promise = new Promise((resolvePromise, rejectPromise) => { + resolve = resolvePromise; + reject = rejectPromise; + }); + return { promise, resolve, reject }; +} function encodeNativeMessage(message: unknown): Buffer { // Native worker messages are length-prefixed V8 payloads, matching NativeWorkerProcess.writeMessage. @@ -51,10 +62,16 @@ function createRecordingNativeProcess(recordedCalls: RecordedNativeCall[], respo const call = v8.deserialize(payload) as RecordedNativeCall; recordedCalls.push(call); - // Tests can provide per-method native responses; otherwise use a generic write success. - queueMicrotask(() => { - const response = respondToCall?.(call) ?? { result: { changes: 1, lastInsertRowId: 1 } }; - emitMessage({ id: call.id, ...response }); + // Tests can provide per-method native responses, including deferred + // promises for deterministic interleaving checks; otherwise writes + // receive a generic success response. + queueMicrotask(async () => { + try { + const response = await (respondToCall?.(call) ?? { result: { changes: 1, lastInsertRowId: 1 } }); + emitMessage({ id: call.id, ...response }); + } catch (err) { + emitMessage({ id: call.id, error: err instanceof Error ? err.message : String(err) }); + } }); } }; @@ -367,16 +384,24 @@ describe('createNativeDatabaseConnection', () => { } }); - it('undoes a batch of json_patch cells atomically via execBatch of restored-object writes', async () => { - // Reads happen before the one execBatch write so the native worker applies the batch atomically. + it('undoes a batch of json_patch cells with one queryBatch read and one execBatch write', async () => { + // The read side is batched into one worker round-trip, and the write + // side remains one execBatch so restored values are applied atomically. const currents: Record = { 3: { count: 2, stable: 'one', concurrent: 'a' }, 4: { count: 11, stable: 'two', concurrent: 'b' } }; const connection = await createRecordingConnection((call) => { - if (call.method === 'query') { - const [, params] = call.args as [string, unknown[]]; - return { result: { columns: ['payload'], values: [[JSON.stringify(currents[Number(params[0])])]] } }; + if (call.method === 'queryBatch') { + const [queries] = call.args as [Array<{ sql: string; params: unknown[] }>]; + return { + result: { + results: queries.map(query => ({ + columns: ['payload'], + values: [[JSON.stringify(currents[Number(query.params[0])])]] + })) + } + }; } return { result: { changes: 1, lastInsertRowId: 1 } }; }); @@ -406,10 +431,18 @@ describe('createNativeDatabaseConnection', () => { }); const queryCalls = connection.calls.filter(call => call.method === 'query'); + const queryBatchCalls = connection.calls.filter(call => call.method === 'queryBatch'); const batchCall = connection.calls.find(call => call.method === 'execBatch'); - assert.strictEqual(queryCalls.length, 2); + assert.strictEqual(queryCalls.length, 0); + assert.strictEqual(queryBatchCalls.length, 1); assert.ok(batchCall, 'batch json_patch undo must write through one execBatch'); + const [queries] = queryBatchCalls[0].args as [Array<{ sql: string; params: unknown[] }>]; + assert.deepStrictEqual(queries, [ + { sql: `SELECT "payload" FROM "docs" WHERE rowid = ?`, params: [3] }, + { sql: `SELECT "payload" FROM "docs" WHERE rowid = ?`, params: [4] } + ]); + const items = (batchCall.args as [Array<{ sql: string; params: unknown[] }>])[0]; assert.strictEqual(items.length, 2); assert.ok(items.every(item => item.sql === `UPDATE "docs" SET "payload" = ? WHERE rowid = ?`)); @@ -430,6 +463,79 @@ describe('createNativeDatabaseConnection', () => { } }); + it('serializes overlapping undoModification and updateCell worker messages', async () => { + // The undo read is intentionally held open. A public updateCell call + // started during that read must wait until undo has also written, so the + // undo read/write sequence remains contiguous at the worker boundary. + const queryStarted = createDeferred(); + const queryResponse = createDeferred(); + const connection = await createRecordingConnection((call) => { + if (call.method === 'query') { + queryStarted.resolve(); + return queryResponse.promise; + } + return { result: { changes: 1, lastInsertRowId: 1 } }; + }); + + let undoPromise: Promise | undefined; + let updatePromise: Promise | undefined; + try { + connection.calls.length = 0; + undoPromise = connection.databaseOps.undoModification({ + modificationType: 'cell_update', + description: 'undo payload', + targetTable: 'docs', + targetRowId: 7, + targetColumn: 'payload', + priorValue: JSON.stringify({ status: 'draft', owner: 'ada' }), + newValue: JSON.stringify({ status: 'published' }), + operation: 'json_patch' + }); + + await queryStarted.promise; + updatePromise = connection.databaseOps.updateCell('docs', 7, 'payload', '{"status":"manual"}'); + await new Promise(resolve => setImmediate(resolve)); + await new Promise(resolve => setImmediate(resolve)); + + try { + assert.deepStrictEqual( + connection.calls.map(call => call.method), + ['query'], + 'concurrent updateCell must not write while undo is between read and write' + ); + } finally { + queryResponse.resolve({ + result: { + columns: ['payload'], + values: [[JSON.stringify({ status: 'published', owner: 'ada', reviewer: 'grace' })]] + } + }); + await Promise.allSettled([undoPromise, updatePromise]); + } + + assert.deepStrictEqual(connection.calls.map(call => call.method), ['query', 'run', 'run']); + const undoRun = connection.calls[1]; + const updateRun = connection.calls[2]; + assert.deepStrictEqual(JSON.parse((undoRun.args as [string, unknown[]])[1][0] as string), { + status: 'draft', + owner: 'ada', + reviewer: 'grace' + }); + assert.deepStrictEqual((updateRun.args as [string, unknown[]])[1], ['{"status":"manual"}', 7]); + } finally { + queryResponse.resolve({ + result: { + columns: ['payload'], + values: [[JSON.stringify({ status: 'published', owner: 'ada' })]] + } + }); + if (undoPromise || updatePromise) { + await Promise.allSettled([undoPromise, updatePromise].filter(Boolean) as Promise[]); + } + connection.dispose(); + } + }); + it('replays single json_patch cell redo through the patch-aware updateCell primitive', async () => { const connection = await createRecordingConnection(); From 44f2b727c5c189518d26f19a10c596456e988348 Mon Sep 17 00:00:00 2001 From: zknpr Date: Wed, 3 Jun 2026 12:24:32 +0200 Subject: [PATCH 09/12] docs(undo): markdown lint fixes in design spec (review) Address PR #427 review CR1/CR2 (CodeRabbit): prefix #426 so the line is not parsed as an ATX heading (MD018); tag the pseudocode fence as text (MD040). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../specs/2026-06-03-json-patch-undo-rmw-design.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.md b/docs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.md index abcab30..0471a85 100644 --- a/docs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.md +++ b/docs/superpowers/specs/2026-06-03-json-patch-undo-rmw-design.md @@ -40,7 +40,7 @@ priorValue: originalValue, // the FULL prior cell value (string | null) ## 2. Why #426's inverse-patch approach was abandoned -#426 built an **inverse merge patch** (`invertMergePatch` / `tryCreateInverseMergePatch`) and applied +PR #426 built an **inverse merge patch** (`invertMergePatch` / `tryCreateInverseMergePatch`) and applied it with SQLite's `json_patch(COALESCE(col,'{}'), ?)`. That representation is structurally lossy against RFC 7396, so each review round plugged one leak and exposed another. The PR was **closed with two unresolved P2 correctness holes** still open from its second review round: @@ -94,7 +94,7 @@ non-object `current` means we cannot surgically restore into it — scenario 6. **Restore walk** (reference; the §4 acceptance table is the binding contract). Clone `current`; walk **only the forward patch's key structure**, drawing restoration values from `prior`: -``` +```text restoreInto(currentObj, patchObj, priorObj, depth): if depth > MAX_DEPTH: throw result = { ...currentObj } From 71cb53935baa96fed782cb5d34b3e241130d0568 Mon Sep 17 00:00:00 2001 From: zknpr Date: Wed, 3 Jun 2026 13:12:29 +0200 Subject: [PATCH 10/12] fix(undo): value-replace JSON numbers that overflow to non-finite (review) Address PR #427 Codex re-review (json-utils.ts:216): the precision guard only checked 16+ digit integer runs, so an exponent like 1e999 passed; RMW then JSON.parse'd it to Infinity and wrote it back as null, corrupting an untouched sibling. Rename hasPrecisionRiskyInteger -> hasPrecisionRiskyNumber and also reject any token that parses to a non-finite number, so those cells value-replace (byte-exact). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/json-utils.ts | 19 ++++++++++++------- tests/unit/json_utils.test.ts | 10 ++++++++++ tests/unit/sqlite_db.test.ts | 24 ++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/src/core/json-utils.ts b/src/core/json-utils.ts index f590405..257f864 100644 --- a/src/core/json-utils.ts +++ b/src/core/json-utils.ts @@ -140,10 +140,10 @@ export function computeJsonPatchUndo( forwardPatchRaw: unknown, priorRaw: unknown ): JsonUndoPlan { - // Large integer tokens cannot round-trip through JSON.parse/JSON.stringify + // Some JSON number tokens cannot round-trip through JSON.parse/JSON.stringify // safely. In that case undo falls back to the recorded prior string so // untouched sibling numbers remain byte-exact. - if (hasPrecisionRiskyInteger(currentRaw) || hasPrecisionRiskyInteger(priorRaw)) { + if (hasPrecisionRiskyNumber(currentRaw) || hasPrecisionRiskyNumber(priorRaw)) { return { kind: 'replace' }; } @@ -207,13 +207,18 @@ function restoreInto( return result; } -function hasPrecisionRiskyInteger(raw: unknown): boolean { +function hasPrecisionRiskyNumber(raw: unknown): boolean { if (typeof raw !== 'string') return false; - // Neutralize string literals so only structural JSON number tokens are - // inspected for integer runs that JavaScript cannot represent exactly. + // Neutralize string literals so only structural JSON number tokens are inspected. const structural = raw.replace(/"(?:\\.|[^"\\])*"/g, '""'); - const digitRuns = structural.match(/\d{16,}/g); - return !!digitRuns && digitRuns.some(run => !Number.isSafeInteger(Number(run))); + const tokens = structural.match(/-?\d+(?:\.\d+)?(?:[eE][+-]?\d+)?/g); + if (!tokens) return false; + return tokens.some(tok => { + const n = Number(tok); + if (!Number.isFinite(n)) return true; + if (/^-?\d+$/.test(tok)) return !Number.isSafeInteger(n); + return false; + }); } function isObject(val: unknown): val is Record { diff --git a/tests/unit/json_utils.test.ts b/tests/unit/json_utils.test.ts index 14d0e37..32e882c 100644 --- a/tests/unit/json_utils.test.ts +++ b/tests/unit/json_utils.test.ts @@ -230,6 +230,16 @@ describe('computeJsonPatchUndo (RMW undo decision)', () => { ); }); + it('value-replaces when untouched exponent tokens overflow JSON number parsing', () => { + expectReplace( + computeJsonPatchUndo( + '{"huge":1e999,"a":2}', + j({ a: 2 }), + '{"huge":1e999,"a":1}' + ) + ); + }); + it('still performs a surgical restore when object cells have no precision-risky integers', () => { expectRestore( computeJsonPatchUndo( diff --git a/tests/unit/sqlite_db.test.ts b/tests/unit/sqlite_db.test.ts index 8c71352..dce2722 100644 --- a/tests/unit/sqlite_db.test.ts +++ b/tests/unit/sqlite_db.test.ts @@ -398,6 +398,30 @@ describe('WasmDatabaseEngine', () => { assert.strictEqual(result[0].rows[0][0], prior); }); + it('undo value-replaces cells with overflowing exponent tokens without nulling siblings', async () => { + // The huge token parses to Infinity in JavaScript. Undo must avoid + // JSON.parse/stringify read-modify-write so the untouched token does + // not become null and the stored prior string remains byte-exact. + await engine.executeQuery('DELETE FROM users'); + const prior = '{"huge":1e999,"a":1}'; + const current = '{"huge":1e999,"a":2}'; + await engine.insertRow('users', { id: 1, name: 'A', age: 30, data: current }); + + await engine.undoModification({ + modificationType: 'cell_update', + description: 'undo overflowing JSON patch number', + targetTable: 'users', + targetRowId: 1, + targetColumn: 'data', + priorValue: prior, + newValue: JSON.stringify({ a: 2 }), + operation: 'json_patch' + }); + + const result = await engine.executeQuery('SELECT data FROM users WHERE id = 1'); + assert.strictEqual(result[0].rows[0][0], prior); + }); + it('batch undo restores each json_patch cell, keeps concurrent siblings, and is atomic (s9)', async () => { // Two read-modify-write undos in one history entry must restore only the count keys. await engine.executeQuery('DELETE FROM users'); From 96d0f8927f00fce0d0ab6a55c3daa7865b979ce8 Mon Sep 17 00:00:00 2001 From: zknpr Date: Wed, 3 Jun 2026 13:12:29 +0200 Subject: [PATCH 11/12] fix(undo): serialize WASM in-process undo read/write; share one op-lock (review) Address PR #427 Codex re-review (WasmDatabaseEngine.ts:327): the native CX2 fix did not cover the WASM in-process path, where workerFactory exposes undoModification and updateCell as direct endpoint calls, so a concurrent edit could still interleave between a json_patch undo's read and write. Extract the per-connection promise-chain lock into a shared serializeOperations() (Proxy over DatabaseOperations; passes through engineKind/dispose symbols; deadlock-free since internal facade calls bypass it) and apply it to BOTH facades: native (replacing the inline lock) and the WASM in-process facade in workerFactory. The worker-thread WASM path is one-RPC-per-op and needs no lock. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/operation-serializer.ts | 48 ++++++++++ src/nativeWorker.ts | 51 +--------- src/workerFactory.ts | 3 +- tests/unit/operation_serializer.test.ts | 121 ++++++++++++++++++++++++ 4 files changed, 173 insertions(+), 50 deletions(-) create mode 100644 src/core/operation-serializer.ts create mode 100644 tests/unit/operation_serializer.test.ts diff --git a/src/core/operation-serializer.ts b/src/core/operation-serializer.ts new file mode 100644 index 0000000..13798ef --- /dev/null +++ b/src/core/operation-serializer.ts @@ -0,0 +1,48 @@ +import type { DatabaseOperations } from './types'; + +const passthroughSymbols = new Set([ + Symbol.dispose, + Symbol.asyncDispose +]); + +/** + * Native worker ops and WASM in-process ops are already serial at their boundary; + * this lock keeps each public operation's internal multi-step sequence + * (json_patch undo read->compute->write) contiguous against concurrent edits. + */ +export function serializeOperations(ops: DatabaseOperations): DatabaseOperations { + let tail: Promise = Promise.resolve(); + const wrappedMethods = new Map(); + + const runSerialized = async (operation: () => Promise): Promise => { + const previous = tail; + let release!: () => void; + tail = new Promise(resolve => { + release = resolve; + }); + + await previous; + try { + return await operation(); + } finally { + release(); + } + }; + + return new Proxy(ops as DatabaseOperations & Record, { + get(target, property, receiver) { + const value = Reflect.get(target, property, receiver); + if (passthroughSymbols.has(property) || typeof value !== 'function') { + return value; + } + + if (!wrappedMethods.has(property)) { + wrappedMethods.set(property, (...args: unknown[]) => ( + runSerialized(() => value.apply(target, args)) + )); + } + + return wrappedMethods.get(property); + } + }) as DatabaseOperations; +} diff --git a/src/nativeWorker.ts b/src/nativeWorker.ts index 38f7610..bc51470 100644 --- a/src/nativeWorker.ts +++ b/src/nativeWorker.ts @@ -39,6 +39,7 @@ import type { import { escapeIdentifier, validateSqlType, validateRowId, validateRowIds } from './core/sql-utils'; import { buildSelectQuery, buildCountQuery } from './core/query-builder'; import { computeJsonPatchUndo } from './core/json-utils'; +import { serializeOperations } from './core/operation-serializer'; // ============================================================================ // Utility Functions @@ -1215,55 +1216,7 @@ export async function createNativeDatabaseConnection( } }; - // Serialize host-facing operations so multi-message sequences such as - // json_patch undo read/compute/write cannot interleave with another edit. - let operationLock: Promise = Promise.resolve(); - const withOperationLock = async (operation: () => Promise): Promise => { - const previous = operationLock; - let release!: () => void; - operationLock = new Promise(resolve => { - release = resolve; - }); - - await previous; - try { - return await operation(); - } finally { - release(); - } - }; - - const operationsFacade: DatabaseOperations = { - engineKind: rawOperations.engineKind, - executeQuery: (sql, params) => withOperationLock(() => rawOperations.executeQuery(sql, params)), - serializeDatabase: name => withOperationLock(() => rawOperations.serializeDatabase(name)), - applyModifications: (mods, signal) => withOperationLock(() => rawOperations.applyModifications(mods, signal)), - undoModification: mod => withOperationLock(() => rawOperations.undoModification(mod)), - redoModification: mod => withOperationLock(() => rawOperations.redoModification(mod)), - flushChanges: signal => withOperationLock(() => rawOperations.flushChanges(signal)), - discardModifications: (mods, signal) => withOperationLock(() => rawOperations.discardModifications(mods, signal)), - updateCell: (table, rowId, column, value, patch) => - withOperationLock(() => rawOperations.updateCell(table, rowId, column, value, patch)), - insertRow: (table, data) => withOperationLock(() => rawOperations.insertRow(table, data)), - insertRowBatch: (table, rows) => withOperationLock(() => rawOperations.insertRowBatch(table, rows)), - deleteRows: (table, rowIds) => withOperationLock(() => rawOperations.deleteRows(table, rowIds)), - deleteColumns: (table, columns, dropDependentIndexes) => - withOperationLock(() => rawOperations.deleteColumns(table, columns, dropDependentIndexes)), - findDependentIndexes: (table, columns) => - withOperationLock(() => rawOperations.findDependentIndexes(table, columns)), - createTable: (table, columns) => withOperationLock(() => rawOperations.createTable(table, columns)), - updateCellBatch: (table, updates) => withOperationLock(() => rawOperations.updateCellBatch(table, updates)), - addColumn: (table, column, type, defaultValue) => - withOperationLock(() => rawOperations.addColumn(table, column, type, defaultValue)), - fetchTableData: (table, options) => withOperationLock(() => rawOperations.fetchTableData(table, options)), - fetchTableCount: (table, options) => withOperationLock(() => rawOperations.fetchTableCount(table, options)), - fetchSchema: () => withOperationLock(() => rawOperations.fetchSchema()), - getTableInfo: table => withOperationLock(() => rawOperations.getTableInfo(table)), - getPragmas: () => withOperationLock(() => rawOperations.getPragmas()), - setPragma: (pragma, value) => withOperationLock(() => rawOperations.setPragma(pragma, value)), - ping: () => withOperationLock(() => rawOperations.ping()), - writeToFile: path => withOperationLock(() => rawOperations.writeToFile(path)) - }; + const operationsFacade = serializeOperations(rawOperations); return { databaseOps: operationsFacade, diff --git a/src/workerFactory.ts b/src/workerFactory.ts index 73204ed..3666171 100644 --- a/src/workerFactory.ts +++ b/src/workerFactory.ts @@ -15,6 +15,7 @@ import * as vsc from 'vscode'; import path from 'path'; import { connectWorkerPort, Transfer } from './core/rpc'; +import { serializeOperations } from './core/operation-serializer'; import { GlobalOutputChannel } from './main'; import type { CellValue, @@ -291,7 +292,7 @@ async function createInProcessWasmDatabaseConnection( }; return { - databaseOps: operationsFacade, + databaseOps: serializeOperations(operationsFacade), isReadOnly: result.isReadOnly ?? false }; } diff --git a/tests/unit/operation_serializer.test.ts b/tests/unit/operation_serializer.test.ts new file mode 100644 index 0000000..e9b2b01 --- /dev/null +++ b/tests/unit/operation_serializer.test.ts @@ -0,0 +1,121 @@ +import { describe, it } from 'node:test'; +import assert from 'node:assert'; +import { serializeOperations } from '../../src/core/operation-serializer'; +import type { DatabaseOperations, QueryResultSet } from '../../src/core/types'; + +function createDeferred() { + let resolve!: (value: T) => void; + let reject!: (reason?: unknown) => void; + const promise = new Promise((resolvePromise, rejectPromise) => { + resolve = resolvePromise; + reject = rejectPromise; + }); + return { promise, resolve, reject }; +} + +function createOperations(overrides: Partial = {}): DatabaseOperations { + const operations: DatabaseOperations = { + engineKind: Promise.resolve('wasm'), + executeQuery: async (): Promise => [], + serializeDatabase: async () => new Uint8Array(), + applyModifications: async () => {}, + undoModification: async () => {}, + redoModification: async () => {}, + flushChanges: async () => {}, + discardModifications: async () => {}, + updateCell: async () => {}, + insertRow: async () => undefined, + insertRowBatch: async () => {}, + deleteRows: async () => {}, + deleteColumns: async () => {}, + findDependentIndexes: async () => [], + createTable: async () => {}, + updateCellBatch: async () => {}, + addColumn: async () => {}, + fetchTableData: async () => ({ + headers: [], + rows: [] + }), + fetchTableCount: async () => 0, + fetchSchema: async () => ({ + tables: [], + views: [], + indexes: [] + }), + getTableInfo: async () => [], + getPragmas: async () => ({}), + setPragma: async () => {}, + ping: async () => true, + writeToFile: async () => {} + }; + return Object.assign(operations, overrides); +} + +describe('serializeOperations', () => { + it('runs overlapping public operations one at a time in FIFO order', async () => { + const events: string[] = []; + const firstStarted = createDeferred(); + const releaseFirst = createDeferred(); + const raw = createOperations({ + engineKind: Promise.resolve('native'), + executeQuery: async (sql: string): Promise => { + events.push(`start:${sql}`); + if (sql === 'first') { + firstStarted.resolve(); + await releaseFirst.promise; + } + events.push(`end:${sql}`); + return []; + } + }); + const dispose = () => { + events.push('dispose'); + }; + Object.defineProperty(raw, Symbol.dispose, { + configurable: true, + value: dispose + }); + + const wrapped = serializeOperations(raw); + + assert.strictEqual(wrapped.engineKind, raw.engineKind); + assert.strictEqual((wrapped as DatabaseOperations & { [Symbol.dispose]: () => void })[Symbol.dispose], dispose); + + const first = wrapped.executeQuery('first'); + await firstStarted.promise; + const second = wrapped.executeQuery('second'); + await new Promise(resolve => setImmediate(resolve)); + await new Promise(resolve => setImmediate(resolve)); + + assert.deepStrictEqual(events, ['start:first']); + + releaseFirst.resolve(); + await Promise.all([first, second]); + + assert.deepStrictEqual(events, [ + 'start:first', + 'end:first', + 'start:second', + 'end:second' + ]); + }); + + it('releases the operation lock after a public operation throws', async () => { + const events: string[] = []; + const raw = createOperations({ + executeQuery: async (sql: string): Promise => { + events.push(sql); + if (sql === 'first') { + throw new Error('boom'); + } + return []; + } + }); + const wrapped = serializeOperations(raw); + + await assert.rejects(() => wrapped.executeQuery('first'), /boom/); + await wrapped.executeQuery('second'); + + assert.deepStrictEqual(events, ['first', 'second']); + }); +}); From e0e6b3b391203711947973bcdd9102eac912c52e Mon Sep 17 00:00:00 2001 From: zknpr Date: Wed, 3 Jun 2026 13:25:27 +0200 Subject: [PATCH 12/12] fix(undo): value-replace any JSON number that cannot round-trip (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #427 Codex round-3 (json-utils.ts:219): the precision guard accepted high-precision decimals like 0.1234567890123456789012345, which an RMW reparse rounds, corrupting an untouched sibling. Replace the integer-only check with a uniform round-trip test — a token is risky if non-finite or JSON.stringify(Number(tok)) !== tok — covering large integers, overflow, and high-precision decimals in one rule. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/core/json-utils.ts | 9 ++++++--- tests/unit/json_utils.test.ts | 12 ++++++++++++ tests/unit/sqlite_db.test.ts | 23 +++++++++++++++++++++++ 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/src/core/json-utils.ts b/src/core/json-utils.ts index 257f864..aaac9c6 100644 --- a/src/core/json-utils.ts +++ b/src/core/json-utils.ts @@ -215,9 +215,12 @@ function hasPrecisionRiskyNumber(raw: unknown): boolean { if (!tokens) return false; return tokens.some(tok => { const n = Number(tok); - if (!Number.isFinite(n)) return true; - if (/^-?\d+$/.test(tok)) return !Number.isSafeInteger(n); - return false; + // Non-finite overflow (e.g. 1e999 -> Infinity), or any token whose exact + // text a JSON parse/serialize round-trip does not reproduce — large + // integers and high-precision decimals alike — cannot be restored by RMW + // without changing the stored number. Value-replace writes the recorded + // prior string back byte-exact instead. + return !Number.isFinite(n) || JSON.stringify(n) !== tok; }); } diff --git a/tests/unit/json_utils.test.ts b/tests/unit/json_utils.test.ts index 32e882c..3b1e5a1 100644 --- a/tests/unit/json_utils.test.ts +++ b/tests/unit/json_utils.test.ts @@ -240,6 +240,18 @@ describe('computeJsonPatchUndo (RMW undo decision)', () => { ); }); + it('value-replaces when an untouched high-precision decimal cannot round-trip', () => { + // The decimal carries more precision than a JS double; JSON.parse/stringify + // would round it, so undo must value-replace to keep it byte-exact. + expectReplace( + computeJsonPatchUndo( + '{"precise":0.1234567890123456789012345,"a":2}', + j({ a: 2 }), + '{"precise":0.1234567890123456789012345,"a":1}' + ) + ); + }); + it('still performs a surgical restore when object cells have no precision-risky integers', () => { expectRestore( computeJsonPatchUndo( diff --git a/tests/unit/sqlite_db.test.ts b/tests/unit/sqlite_db.test.ts index dce2722..a0b1ea5 100644 --- a/tests/unit/sqlite_db.test.ts +++ b/tests/unit/sqlite_db.test.ts @@ -422,6 +422,29 @@ describe('WasmDatabaseEngine', () => { assert.strictEqual(result[0].rows[0][0], prior); }); + it('undo value-replaces cells with high-precision decimals without rounding siblings', async () => { + // The decimal carries more precision than a JS double. Undo must avoid the + // JSON.parse/stringify restore so the untouched value stays byte-exact. + await engine.executeQuery('DELETE FROM users'); + const prior = '{"precise":0.1234567890123456789012345,"a":1}'; + const current = '{"precise":0.1234567890123456789012345,"a":2}'; + await engine.insertRow('users', { id: 1, name: 'A', age: 30, data: current }); + + await engine.undoModification({ + modificationType: 'cell_update', + description: 'undo high-precision decimal', + targetTable: 'users', + targetRowId: 1, + targetColumn: 'data', + priorValue: prior, + newValue: JSON.stringify({ a: 2 }), + operation: 'json_patch' + }); + + const result = await engine.executeQuery('SELECT data FROM users WHERE id = 1'); + assert.strictEqual(result[0].rows[0][0], prior); + }); + it('batch undo restores each json_patch cell, keeps concurrent siblings, and is atomic (s9)', async () => { // Two read-modify-write undos in one history entry must restore only the count keys. await engine.executeQuery('DELETE FROM users');