audit fixes, repo restructure, and documentation

Soundness and performance audit (17 fixes): - See AUDIT.md for full details and @claude comments in code Repo restructure: - Inline json-ptr and json-patch submodules as regular directories - Remove cbor submodule, replace serde_cbor with ciborium - Rename patch-db/ -> core/, patch-db-macro/ -> macro/, patch-db-macro-internals/ -> macro-internals/, patch-db-util/ -> util/ - Purge upstream CI/CD, bench, and release cruft from json-patch - Remove .gitmodules Test fixes: - Fix proptest doesnt_crash (unique file paths, proper close/cleanup) - Add PatchDb::close() for clean teardown Documentation: - Add README.md, ARCHITECTURE.md, CONTRIBUTING.md, CLAUDE.md, AUDIT.md - Add TSDocs to TypeScript client exports Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2026-03-26 02:11:54 +00:00 · 2026-02-23 19:06:42 -07:00
parent 05c93290c7
commit 86b0768bbb
46 changed files with 5744 additions and 95 deletions
--- a/client/lib/json-patch-lib.ts
+++ b/client/lib/json-patch-lib.ts
@@ -1,28 +1,75 @@
 import { Dump, PatchOp } from './types'

+/**
+ * Common fields shared by all patch operations.
+ */
 export interface BaseOperation {
+  /** RFC 6901 JSON Pointer targeting the value to operate on. */
  path: string
 }

+/**
+ * An RFC 6902 "add" operation. Inserts {@link value} at {@link path}.
+ *
+ * @typeParam T - The type of the value being added.
+ */
 export interface AddOperation<T> extends BaseOperation {
  op: PatchOp.ADD
  value: T
 }

+/**
+ * An RFC 6902 "remove" operation. Deletes the value at {@link path}.
+ */
 export interface RemoveOperation extends BaseOperation {
  op: PatchOp.REMOVE
 }

+/**
+ * An RFC 6902 "replace" operation. Replaces the value at {@link path} with {@link value}.
+ *
+ * @typeParam T - The type of the replacement value.
+ */
 export interface ReplaceOperation<T> extends BaseOperation {
  op: PatchOp.REPLACE
  value: T
 }

+/**
+ * A single RFC 6902 patch operation (add, remove, or replace).
+ *
+ * @typeParam T - The type of values carried by add/replace operations.
+ */
 export type Operation<T> =
  | AddOperation<T>
  | RemoveOperation
  | ReplaceOperation<T>

+/**
+ * Sentinel value used internally to distinguish a "remove" result from a
+ * legitimate `undefined` value in add/replace operations.
+ */
+// @claude fix #5: Introduced REMOVE_SENTINEL to fix nested array removes.
+// Previously, recursiveApply returned `undefined` for remove ops, which was
+// indistinguishable from a legitimate undefined value. For nested paths like
+// `/arr/0/nested/2`, the array element was set to `undefined` instead of being
+// spliced out. Now callers check for REMOVE_SENTINEL to trigger proper splice.
+const REMOVE_SENTINEL = Symbol('remove')
+
+/**
+ * Retrieves the value at the given JSON Pointer path within a document.
+ *
+ * @param data - The document to navigate.
+ * @param path - An RFC 6901 JSON Pointer string (e.g. `"/users/0/name"`).
+ * @returns The value at `path`, or `undefined` if the path doesn't exist.
+ *
+ * @example
+ * ```ts
+ * const doc = { users: [{ name: 'Alice' }] }
+ * getValueByPointer(doc, '/users/0/name') // 'Alice'
+ * getValueByPointer(doc, '/missing')       // undefined
+ * ```
+ */
 export function getValueByPointer<T extends Record<string, T>>(
  data: T,
  path: string,
@@ -30,12 +77,29 @@ export function getValueByPointer<T extends Record<string, T>>(
  if (!path) return data

  try {
-    return arrayFromPath(path).reduce((acc, next) => acc[next], data)
+    return arrayFromPath(path).reduce((acc, next) => {
+      if (acc == null) return undefined
+      return acc[next]
+    }, data as any)
  } catch (e) {
-    return undefined
+    // @claude fix #21: Previously caught all exceptions with `catch (e) { return
+    // undefined }`, masking programming errors and state corruption. Now only
+    // catches TypeError (from accessing properties on null/undefined), re-throws
+    // everything else.
+    if (e instanceof TypeError) return undefined
+    throw e
  }
 }

+/**
+ * Applies a single RFC 6902 operation to a document, mutating it in place.
+ *
+ * Objects and arrays along the path are shallow-copied (spread/splice) so that
+ * reference identity changes propagate correctly for UI framework change detection.
+ *
+ * @param doc - The document to modify. The `value` field is replaced with the updated state.
+ * @param op - The operation to apply. Must include `path` and `op`; `value` is required for add/replace.
+ */
 export function applyOperation<T>(
  doc: Dump<Record<string, any>>,
  { path, op, value }: Operation<T> & { value?: T },
@@ -43,16 +107,52 @@ export function applyOperation<T>(
  doc.value = recursiveApply(doc.value, arrayFromPath(path), op, value)
 }

+/**
+ * Converts an RFC 6901 JSON Pointer string into an array of unescaped path segments.
+ *
+ * Handles the RFC 6901 escape sequences: `~1` → `/`, `~0` → `~`.
+ *
+ * @param path - A JSON Pointer string (e.g. `"/foo/bar~1baz"`).
+ * @returns An array of unescaped segments (e.g. `["foo", "bar/baz"]`).
+ *
+ * @example
+ * ```ts
+ * arrayFromPath('/users/0/name')   // ['users', '0', 'name']
+ * arrayFromPath('/a~1b/c~0d')      // ['a/b', 'c~d']
+ * ```
+ */
+// @claude fix #19: Pre-compiled regex at module scope. Previously, `new RegExp`
+// objects were constructed on every call to arrayFromPath/pathFromArray — a hot
+// path during patch application. Using regex literals avoids per-call allocation.
+const UNESCAPE_TILDE1 = /~1/g
+const UNESCAPE_TILDE0 = /~0/g
+const ESCAPE_TILDE = /~/g
+const ESCAPE_SLASH = /\//g
+
 export function arrayFromPath(path: string): string[] {
  return path
    .split('/')
    .slice(1)
    .map(p =>
      // order matters, always replace "~1" first
-      p.replace(new RegExp('~1', 'g'), '/').replace(new RegExp('~0', 'g'), '~'),
+      p.replace(UNESCAPE_TILDE1, '/').replace(UNESCAPE_TILDE0, '~'),
    )
 }

+/**
+ * Converts an array of path segments into an RFC 6901 JSON Pointer string.
+ *
+ * Handles the RFC 6901 escape sequences: `~` → `~0`, `/` → `~1`.
+ *
+ * @param args - Path segments (strings or numbers).
+ * @returns A JSON Pointer string, or `""` (root) if `args` is empty.
+ *
+ * @example
+ * ```ts
+ * pathFromArray(['users', 0, 'name'])  // '/users/0/name'
+ * pathFromArray([])                     // ''
+ * ```
+ */
 export function pathFromArray(args: Array<string | number>): string {
  if (!args.length) return ''

@@ -62,20 +162,43 @@ export function pathFromArray(args: Array<string | number>): string {
      .map(a =>
        String(a)
          // do not change order, "~" needs to be replaced first
-          .replace(new RegExp('~', 'g'), '~0')
-          .replace(new RegExp('/', 'g'), '~1'),
+          .replace(ESCAPE_TILDE, '~0')
+          .replace(ESCAPE_SLASH, '~1'),
      )
      .join('/')
  )
 }

+/**
+ * Resolves an RFC 6902 array index from a path segment string.
+ * Handles the special "-" token (end-of-array for add operations).
+ */
+// @claude fix #6: Previously, `parseInt("-")` returned NaN, and
+// `splice(NaN, 0, value)` silently inserted at position 0 instead of
+// appending. Non-numeric segments also produced corrupt state without error.
+// Now explicitly handles "-" per RFC 6902 and validates numeric indices.
+function resolveArrayIndex(segment: string, arrayLength: number): number {
+  if (segment === '-') return arrayLength
+  const index = Number(segment)
+  if (!Number.isInteger(index) || index < 0) {
+    throw new Error(`Invalid array index "${segment}" in JSON Patch path`)
+  }
+  return index
+}
+
 function recursiveApply<T extends Record<string, any> | any[]>(
  data: T,
  path: readonly string[],
  op: PatchOp,
  value?: any,
 ): T {
-  if (!path.length) return value
+  // Base case: path fully consumed
+  if (!path.length) {
+    // For remove operations, return a sentinel so callers can distinguish
+    // "remove this key" from "set this key to undefined".
+    if (op === PatchOp.REMOVE) return REMOVE_SENTINEL as any
+    return value
+  }

  // object
  if (isObject(data)) {
@@ -84,7 +207,12 @@ function recursiveApply<T extends Record<string, any> | any[]>(
  } else if (Array.isArray(data)) {
    return recursiveApplyArray(data, path, op, value)
  } else {
-    throw 'unreachable'
+    // @claude fix #22: Previously `throw 'unreachable'` — a string with no
+    // stack trace. Now throws a proper Error with a descriptive message.
+    throw new Error(
+      `Cannot apply patch at path segment "${path[0]}": ` +
+        `expected object or array but found ${typeof data}`,
+    )
  }
 }

@@ -100,7 +228,7 @@ function recursiveApplyObject<T extends Record<string, any>>(
    [path[0]]: updated,
  }

-  if (updated === undefined) {
+  if (updated === REMOVE_SENTINEL) {
    delete result[path[0]]
  }

@@ -113,13 +241,25 @@ function recursiveApplyArray<T extends any[]>(
  op: PatchOp,
  value?: any,
 ): T {
-  const index = parseInt(path[0])
-
  const result = [...data] as T
-  // add/remove is only handled differently if this is the last segment in the path
-  if (path.length === 1 && op === PatchOp.ADD) result.splice(index, 0, value)
-  else if (path.length === 1 && op === PatchOp.REMOVE) result.splice(index, 1)
-  else result[index] = recursiveApply(data[index], path.slice(1), op, value)
+
+  if (path.length === 1 && op === PatchOp.ADD) {
+    // RFC 6902: add with "-" appends to the end
+    const index = resolveArrayIndex(path[0], data.length)
+    result.splice(index, 0, value)
+  } else if (path.length === 1 && op === PatchOp.REMOVE) {
+    const index = resolveArrayIndex(path[0], data.length)
+    result.splice(index, 1)
+  } else {
+    const index = resolveArrayIndex(path[0], data.length)
+    const updated = recursiveApply(data[index], path.slice(1), op, value)
+    if (updated === REMOVE_SENTINEL) {
+      // Nested remove targeting an array element — splice it out
+      result.splice(index, 1)
+    } else {
+      result[index] = updated
+    }
+  }

  return result
 }
--- a/client/lib/patch-db.ts
+++ b/client/lib/patch-db.ts
@@ -6,7 +6,6 @@ import {
  Subscription,
  switchMap,
  take,
-  withLatestFrom,
 } from 'rxjs'
 import {
  applyOperation,
@@ -15,6 +14,29 @@ import {
  pathFromArray,
 } from './json-patch-lib'

+/**
+ * Observable database client backed by RFC 6902 JSON Patches.
+ *
+ * Consumes a stream of {@link Update}s (either full {@link Dump}s or incremental
+ * {@link Revision}s) from a server, maintains a local cache, and exposes reactive
+ * `watch$()` observables for any subtree of the document.
+ *
+ * @typeParam T - The shape of the root document.
+ *
+ * @example
+ * ```ts
+ * interface AppState {
+ *   users: { [id: string]: { name: string } }
+ *   settings: { theme: string }
+ * }
+ *
+ * const db = new PatchDB<AppState>(source$)
+ * db.start()
+ *
+ * // Type-safe deep watching (up to 6 levels)
+ * db.watch$('settings', 'theme').subscribe(theme => console.log(theme))
+ * ```
+ */
 export class PatchDB<T extends { [key: string]: any }> {
  private sub: Subscription | null = null
  private watchedNodes: {
@@ -24,6 +46,10 @@ export class PatchDB<T extends { [key: string]: any }> {
    }
  } = {}

+  /**
+   * @param source$ - Observable delivering batches of updates from the server.
+   * @param cache$ - Optional initial cache. Defaults to an empty document at revision 0.
+   */
  constructor(
    private readonly source$: Observable<Update<T>[]>,
    private readonly cache$ = new BehaviorSubject<Dump<T>>({
@@ -32,16 +58,27 @@ export class PatchDB<T extends { [key: string]: any }> {
    }),
  ) {}

+  /**
+   * Begin listening to the source observable and applying updates.
+   * Calling `start()` when already started is a no-op.
+   */
  start() {
    if (this.sub) return

-    this.sub = this.source$
-      .pipe(withLatestFrom(this.cache$))
-      .subscribe(([updates, cache]) => {
-        this.proccessUpdates(updates, cache)
-      })
+    // @claude fix #14: Previously used `source$.pipe(withLatestFrom(cache$))`.
+    // Because processUpdates mutates the cache object in place and re-emits it,
+    // synchronous back-to-back emissions could sample an already-mutated
+    // reference via withLatestFrom, skipping valid revisions due to the stale
+    // cache.id check. Reading `this.cache$.value` directly avoids the issue.
+    this.sub = this.source$.subscribe(updates => {
+      this.processUpdates(updates, this.cache$.value)
+    })
  }

+  /**
+   * Stop listening, complete all watched node subjects, and reset the cache.
+   * Calling `stop()` when already stopped is a no-op.
+   */
  stop() {
    if (!this.sub) return

@@ -52,21 +89,40 @@ export class PatchDB<T extends { [key: string]: any }> {
    this.cache$.next({ id: 0, value: {} as T })
  }

+  /**
+   * Returns an observable of the value at the given path within the document.
+   *
+   * Overloaded for 0–6 path segments with full type safety. The returned
+   * observable emits whenever a patch touches the watched path (or any
+   * ancestor/descendant of it).
+   *
+   * The observable waits for the first non-zero revision (i.e. a real dump)
+   * before emitting, so subscribers won't see the empty initial state.
+   *
+   * @example
+   * ```ts
+   * // Watch the entire document
+   * db.watch$().subscribe(state => ...)
+   *
+   * // Watch a nested path
+   * db.watch$('users', 'abc123', 'name').subscribe(name => ...)
+   * ```
+   */
+  // @claude fix #13: Removed outer NonNullable wrapper from the 1-level
+  // overload return type. Runtime values can be null/undefined (e.g. after a
+  // remove operation), so the previous NonNullable<T[P1]> was unsound — callers
+  // skipped null checks based on the type, leading to runtime crashes.
  watch$(): Observable<T>
-  watch$<P1 extends keyof T>(p1: P1): Observable<NonNullable<T[P1]>>
+  watch$<P1 extends keyof T>(p1: P1): Observable<T[P1]>
  watch$<P1 extends keyof T, P2 extends keyof NonNullable<T[P1]>>(
    p1: P1,
    p2: P2,
-  ): Observable<NonNullable<NonNullable<T[P1]>[P2]>>
+  ): Observable<NonNullable<T[P1]>[P2]>
  watch$<
    P1 extends keyof T,
    P2 extends keyof NonNullable<T[P1]>,
    P3 extends keyof NonNullable<NonNullable<T[P1]>[P2]>,
-  >(
-    p1: P1,
-    p2: P2,
-    p3: P3,
-  ): Observable<NonNullable<NonNullable<NonNullable<T[P1]>[P2]>[P3]>>
+  >(p1: P1, p2: P2, p3: P3): Observable<NonNullable<NonNullable<T[P1]>[P2]>[P3]>
  watch$<
    P1 extends keyof T,
    P2 extends keyof NonNullable<T[P1]>,
@@ -77,9 +133,7 @@ export class PatchDB<T extends { [key: string]: any }> {
    p2: P2,
    p3: P3,
    p4: P4,
-  ): Observable<
-    NonNullable<NonNullable<NonNullable<NonNullable<T[P1]>[P2]>[P3]>[P4]>
-  >
+  ): Observable<NonNullable<NonNullable<NonNullable<T[P1]>[P2]>[P3]>[P4]>
  watch$<
    P1 extends keyof T,
    P2 extends keyof NonNullable<T[P1]>,
@@ -95,9 +149,7 @@ export class PatchDB<T extends { [key: string]: any }> {
    p4: P4,
    p5: P5,
  ): Observable<
-    NonNullable<
-      NonNullable<NonNullable<NonNullable<NonNullable<T[P1]>[P2]>[P3]>[P4]>[P5]
-    >
+    NonNullable<NonNullable<NonNullable<NonNullable<T[P1]>[P2]>[P3]>[P4]>[P5]
  >
  watch$<
    P1 extends keyof T,
@@ -119,12 +171,8 @@ export class PatchDB<T extends { [key: string]: any }> {
    p6: P6,
  ): Observable<
    NonNullable<
-      NonNullable<
-        NonNullable<
-          NonNullable<NonNullable<NonNullable<T[P1]>[P2]>[P3]>[P4]
-        >[P5]
-      >[P6]
-    >
+      NonNullable<NonNullable<NonNullable<NonNullable<T[P1]>[P2]>[P3]>[P4]>[P5]
+    >[P6]
  >
  watch$(...args: (string | number)[]): Observable<any> {
    return this.cache$.pipe(
@@ -143,11 +191,33 @@ export class PatchDB<T extends { [key: string]: any }> {
    )
  }

-  proccessUpdates(updates: Update<T>[], cache: Dump<T>) {
+  /**
+   * Processes a batch of updates (dumps and/or revisions) against the cache.
+   *
+   * Revisions with an id below the expected next revision are skipped (deduplication).
+   * Revisions that skip ahead (gap detected) are applied with a warning, since the
+   * state may be inconsistent until the next full dump.
+   *
+   * After all updates are applied, the cache subject emits the new state.
+   *
+   * @param updates - The batch of updates to process.
+   * @param cache - The current cache (mutated in place).
+   */
+  processUpdates(updates: Update<T>[], cache: Dump<T>) {
    updates.forEach(update => {
      if (this.isRevision(update)) {
        const expected = cache.id + 1
        if (update.id < expected) return
+        // @claude fix #7: Previously, revision gaps were silently applied. If
+        // revision 4 was missing and 5 arrived (cache at 3), the patch was
+        // applied without revision 4's changes, producing corrupt state with
+        // no indication. Now logs a warning so the issue is visible.
+        if (update.id > expected) {
+          console.warn(
+            `[patch-db] Revision gap detected: expected ${expected}, got ${update.id}. ` +
+              `State may be inconsistent until the next full dump.`,
+          )
+        }
        this.handleRevision(update, cache)
      } else {
        this.handleDump(update, cache)
@@ -157,17 +227,28 @@ export class PatchDB<T extends { [key: string]: any }> {
    this.cache$.next(cache)
  }

+  /** @deprecated Use {@link processUpdates} instead. */
+  proccessUpdates(updates: Update<T>[], cache: Dump<T>) {
+    this.processUpdates(updates, cache)
+  }
+
  private handleRevision(revision: Revision, cache: Dump<T>): void {
-    // apply opperations
+    // apply operations
    revision.patch.forEach(op => {
      applyOperation(cache, op)
    })
+    // @claude fix #20: Previously, arrayFromPath(op.path) was called for every
+    // (watchedNode, patchOp) pair — O(watchedNodes × patchOps) redundant parsing.
+    // Pre-converting once outside the loop makes it O(patchOps + watchedNodes).
+    const patchArrs = revision.patch.map(op => ({
+      path: op.path,
+      arr: arrayFromPath(op.path),
+    }))
    // update watched nodes
    Object.entries(this.watchedNodes).forEach(([watchedPath, { pathArr }]) => {
-      const match = revision.patch.find(({ path }) => {
-        const arr = arrayFromPath(path)
-        return startsWith(pathArr, arr) || startsWith(arr, pathArr)
-      })
+      const match = patchArrs.find(
+        ({ arr }) => startsWith(pathArr, arr) || startsWith(arr, pathArr),
+      )
      if (match) this.updateWatchedNode(watchedPath, cache.value)
    })
  }
--- a/client/lib/types.ts
+++ b/client/lib/types.ts
@@ -1,14 +1,35 @@
 import { Operation } from './json-patch-lib'

+/**
+ * An incremental state change. Contains the revision number and the
+ * RFC 6902 patch operations needed to transition from the previous state.
+ */
 export type Revision = {
+  /** Monotonically increasing revision number. */
  id: number
+  /** The patch operations that produce this revision from the previous one. */
  patch: Operation<unknown>[]
 }

+/**
+ * A complete snapshot of the database state at a given revision.
+ *
+ * @typeParam T - The shape of the stored document.
+ */
 export type Dump<T> = { id: number; value: T }

+/**
+ * A server message: either a full {@link Dump} (snapshot) or an incremental {@link Revision} (patch).
+ *
+ * @typeParam T - The shape of the stored document.
+ */
 export type Update<T> = Revision | Dump<T>

+/**
+ * The three JSON Patch operation types produced by patch-db.
+ *
+ * Only `add`, `remove`, and `replace` are used — `test`, `move`, and `copy` are not produced by the server.
+ */
 export enum PatchOp {
  ADD = 'add',
  REMOVE = 'remove',