scadaproj/code-reviews/LocalDb/findings.md

# Code Review — LocalDb

| Field | Value |
|-------|-------|
| Library | `ZB.MOM.WW.LocalDb/` |
| Packages | `ZB.MOM.WW.LocalDb`, `ZB.MOM.WW.LocalDb.Replication`, `ZB.MOM.WW.LocalDb.Contracts` |
| Component spec | `docs/plans/2026-07-17-localdb-design.md` (design doc; there is no `components/localdb/` directory, so no SPEC.md exists) |
| Shared contract | None (no `components/localdb/shared-contract/`); the wire contract is `src/ZB.MOM.WW.LocalDb.Contracts/Protos/localdb_sync.proto` |
| Status | Reviewed |
| Last reviewed | 2026-11-09 |
| Reviewer | Claude |
| Commit reviewed | `91fb2b4` |
| Open findings | 0 |

## Summary

The sync engine is carefully built, and its watermark and session machinery holds up under a close
read. The parts checked and found sound:

- Seq assignment matches commit order, because SQLite has a single writer and the oplog is AUTOINCREMENT.
- Watermarks are MAX-guarded, the applier runs one `BEGIN IMMEDIATE` transaction per batch with a
  savepoint per entry, and the applying guard is a row in the same transaction.
- Prune gaps fault the live session, and a rebuilt peer is detected (both directions).
- Snapshot debt is cleared only on the peer's ack, the inbox is bounded and the writer has two lanes.
- Takeover is newest-wins, and the DI extensions are idempotent.

The DI wiring is covered by tests that build the container: `DependencyInjectionTests`,
`GrpcAdapterTests.AddZbLocalDbReplication_*` and `MaintenanceTests.AddZbLocalDbReplication_RegistersMaintenance_BothRoles`
build a provider and resolve the hosted services. No TryAdd-ordering or factory-cycle defect was found.

The findings fall into three themes:

1. **Shapes the apply path cannot converge on.** The applier writes with `INSERT OR REPLACE` and
   treats every `SqliteException` as per-entry poison. That holds for the narrow schemas the two
   consumers use today, but registration accepts several shapes that diverge or lose rows silently:
   - FK-linked tables during a snapshot (LocalDb-002);
   - secondary `UNIQUE` constraints (LocalDb-003);
   - rowid-alias keys assigned on both nodes (LocalDb-005);
   - column names containing `'` (LocalDb-006).
2. **A fail-closed option the snapshot path skips.** `FailClosedOnDrift` is never checked on the
   snapshot path (LocalDb-004).
3. **The endpoint is unauthenticated, and the option docs suggest otherwise.** The library maps an
   unauthenticated sync endpoint, and `ApiKey`'s doc says the key is "expected from the peer"
   (LocalDb-001). Both current consumers (ScadaBridge, OtOpcUa `LocalDbSyncAuthInterceptor`) close
   the hole with their own interceptors.

Every finding except LocalDb-001 and LocalDb-007 was reproduced with a throwaway test against a
scratch copy of the library (not committed). The suite at `91fb2b4` passes: 216/216 offline (223
with the 7 probes added). The known open items #16 (tombstone resurrection past `TombstoneRetention`)
and #26 (Microsoft.Data.Sqlite pool bump) are excluded.

## Checklist coverage

| # | Category | Examined | Notes |
|---|----------|----------|-------|
| 1 | Correctness & logic bugs | ☑ | Watermarks, ack and prune, horizon gap, rebuilt-peer detection, LWW tie-break and per-entry savepoints are correct. Defects in how the apply path handles some schema shapes: LocalDb-002, LocalDb-003, LocalDb-005, LocalDb-006 |
| 2 | Public API surface & compatibility | ☑ | Proto evolution is additive, with capability negotiation (`supports_partial_snapshot`) and `lib_schema_version` left at 1. On-disk schema upgrades v1→v3 are idempotent. No issues found |
| 3 | Concurrency & thread safety | ☑ | One writer task over two lanes, `_master` guarded by a lock, Interlocked/Volatile on the status fields, takeover is cancel-then-await. HLC `Next`/`Observe` are locked. No issues found |
| 4 | Error handling & resilience | ☑ | Fault teardown flushes the control lane, keepalive pings, backoff with a fast path for schema mismatch, maintenance never dies. The poison catch is broad enough to turn FK-ordering failures into permanent dead-letters: LocalDb-002 |
| 5 | Security & secret handling | ☑ | Table names are regex-gated, all values are parameterized, and `Synchronous` is allow-listed. The sync endpoint is unauthenticated by default and `ApiKey` is documented misleadingly: LocalDb-001. Column-name quoting is incomplete (local schema only, not peer-controlled): LocalDb-006 |
| 6 | Performance & resource management | ☑ | Channel, CTS, call and reader-task disposal checked on every exit path. Statements are prepared once per batch, depth is O(1), batches are byte-budgeted, the inbox is bounded. No issues found |
| 7 | Spec & shared-contract adherence | ☑ | Design §Auth says "ZB.MOM.WW.Auth.ApiKeys keyId/Bearer on the channel", but the lib only sends a header (LocalDb-001). Design §Error handling ("Clock skew … optionally fail-closed") is not applied to snapshots (LocalDb-004) |
| 8 | Packaging, dependencies & project layout | ☑ | Three packages, central package management, TreatWarningsAsErrors, InternalsVisibleTo limited to Replication and Tests. Version 0.3.5 is unpublished (#26). No issues found |
| 9 | Testing coverage | ☑ | Broad, and DI is covered by tests that build the container. No test has a schema with a secondary `UNIQUE`, a non-cascading FK under snapshot, a quoted column name, or a drifted snapshot row. Those are the gaps behind LocalDb-002 through LocalDb-006 |
| 10 | Documentation & XML docs | ☑ | XML docs are thorough. The README is stale in places (LocalDb-007) and the `ApiKey` XML doc is wrong (LocalDb-001) |

## Findings

### LocalDb-001 — Sync endpoint is unauthenticated by default, and `ApiKey` is documented as "expected from the peer"

| | |
|--|--|
| Severity | Medium |
| Priority | P2 |
| Category | Security & secret handling |
| Status | Resolved |
| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/LocalDbSyncService.cs:85`, `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/ReplicationOptions.cs:11`, `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/DependencyInjection/ReplicationServiceCollectionExtensions.cs:77` |

**Description**

`ReplicationOptions.ApiKey` is documented as "API key presented to / expected from the peer; null
disables key auth". Only the first half is true:

- `SyncBackgroundService` attaches `Authorization: Bearer <key>` to the outbound call.
- `LocalDbSyncService.Sync` never reads request headers.
- `MapZbLocalDbSync` maps the service with no authorization metadata.
- The library ships no interceptor or validator.

So setting `ApiKey` on both nodes, the natural reading of the option, leaves the passive endpoint
open. The README's Ops notes say this, but the XML doc does not. The design doc (§Auth: "`ZB.MOM.WW.Auth.ApiKeys` keyId/Bearer on
the channel") promises enforcement on the channel. Both current consumers wrote their own
fail-closed interceptor (ScadaBridge, and OtOpcUa's `LocalDbSyncAuthInterceptor`). This is the
duplication the shared library exists to remove, and every future consumer has to rediscover it.

The handshake makes an open endpoint easy to exploit. `SyncSession.HandshakeAsync` sends the
server's own `Handshake`, including every registered table name and its schema digest
(`SyncSession.cs:180`), before validating the caller's. A caller can therefore echo the digests
back and pass the fail-closed schema check without knowing the schema.

**Failure scenario**

A third consumer (or a new OtOpcUa/ScadaBridge host profile) calls `AddZbLocalDbReplication` +
`MapZbLocalDbSync`. It sets `LocalDb:Replication:ApiKey` on both nodes and does not add an
interceptor. Any client that can reach the gRPC port can then:

1. Open `Sync`, read the server's Handshake, and reply with the same digests and
   `last_applied_remote_seq = 0`.
2. Receive a full snapshot of every replicated table, which is complete data exfiltration.
3. Push `DeltaBatch` entries with a high HLC that overwrite or tombstone any replicated row. They
   win LWW and are then replicated onward by nothing, because the 2-node topology does not
   re-forward.
4. Repeatedly displace the legitimate peer's stream, because the guard is newest-wins
   (`LocalDbSyncService.cs:152`).

A different `node_id` in step 1 also makes the server reset its inbound watermark
(`SyncSession.cs:199`).

**Recommendation**

Move the interceptor both consumers already wrote into the library. When `ApiKey` is set, the
service should verify `Bearer` with a constant-time compare (`CryptographicOperations.FixedTimeEquals`)
and refuse with `Unauthenticated` otherwise. Make the endpoint fail closed when no key is
configured, unless the host opts out explicitly, for example with `AllowUnauthenticated = true`. Fix
the `ApiKey` XML doc either way. Both consumers could then delete their copies.

**Resolution**

Resolved 2026-11-09 (scadaproj#36, LocalDb `0.3.6`, unpublished). Re-verified: at `91fb2b4` nothing in the library reads
request headers. `LocalDbSyncService.Sync` now calls `IsAuthorized` before the takeover and before the handshake. When
`ApiKey` is set, the stream must carry `Authorization: Bearer <key>`, compared with `CryptographicOperations.FixedTimeEquals`
over UTF-8; anything else is refused with `Unauthenticated`. The check sits before the takeover, so an unauthenticated
caller can neither read the Handshake (table names and digests) nor displace the real peer.

As the task requires, a blank `ApiKey` keeps the old behaviour (accept), and the service logs a startup warning. That
rules out the review's fail-closed-by-default suggestion: a key-less pair must keep syncing. Wire compatibility holds,
because the initiator has sent this header whenever the key is set since `0.1.0`. The ScadaBridge and OtOpcUa
interceptors check the same key and the same header, so they are unaffected. The `ApiKey` XML doc, the README config
table and the README Ops notes are corrected.

Tests (`GrpcAdapterTests`): two-node convergence with the key on both sides. Missing, wrong, scheme-less and
over-long tokens are each refused before the handshake. A correct token gets the Handshake. An unauthenticated
stream does not displace a live peer: there is no redial, and replication continues. A key on the initiator only
still syncs, and a blank, empty or whitespace key admits every caller. With the check disabled, the 5 refusal and
non-displacement cases fail.

### LocalDb-002 — Snapshot streams tables by name and rows by `pk_json` text, so FK children land before parents and are dead-lettered for good

| | |
|--|--|
| Severity | Medium |
| Priority | P2 |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/Internal/SnapshotStreamer.cs:79` |

**Description**

`SnapshotStreamer.SendAsync` streams tables in ordinal name order
(`.OrderBy(t => t.Name, StringComparer.Ordinal)`, line 79–81). Within a table it pages
`ORDER BY rv.pk_json` (line 196), which is text order: `{"id":10}` sorts before `{"id":2}`.

The receiver applies through `LwwApplier` on a connection with `PRAGMA foreign_keys=ON`. An
immediate FK violation is a `SqliteException`, and `LwwApplier.cs:116` treats it as per-entry
poison: dead-letter, keep going. `SnapshotApplier.OnCompleteAsync` then moves the watermark to
`as_of_seq`, and the sender clears its snapshot debt on the `SnapshotAck`. Nothing ever re-sends the
dead-lettered rows.

Registration explicitly allows these schemas. `EnsureNoInboundActionOnDelete` rejects only
CASCADE / SET NULL / SET DEFAULT, and README §Limitations recommends `NO ACTION`/`RESTRICT`. So a
replicated parent/child pair with a default FK is a supported configuration that the delta path
handles (seq order matches commit order) but the snapshot path does not.

**Failure scenario**

Reproduced in a scratch test:

1. Register `z_parent(id PK)` and `a_child(id PK, parent_id REFERENCES z_parent(id))` on both nodes.
2. Insert `(1)` into `z_parent` and `(10, 1)` into `a_child` on A.
3. Run a full snapshot from A to an empty B, the rebuilt-peer or cap-prune path.

Result on B: `parents=1 children=0`, one dead-letter `a_child: SQLite Error 19: 'FOREIGN KEY
constraint failed'`, watermark advanced to 2. A self-referencing table (`node(id, parent_id
REFERENCES node(id))` with rows 2 and 10→2) loses row 10 the same way. The child rows are gone from
B permanently, and the pair reports itself converged.

No current consumer has an FK on a replicated table (per the 0.3.3 notes), so this is latent. The
first consumer to add one loses data on every snapshot resync.

**Recommendation**

Choose one:

- Apply snapshot batches with `PRAGMA defer_foreign_keys = ON` inside the apply transaction, which
  checks FKs at commit. This only helps if parent and child land in the same transaction, which
  they do not today across batches. More robustly, apply the whole snapshot with FK enforcement off
  (`PRAGMA foreign_keys=OFF` on the snapshot apply connection, set outside a transaction) and run
  `PRAGMA foreign_key_check` at `SnapshotComplete` to log or dead-letter any true orphans.
- Order the snapshot topologically by FK dependency, parents first. Even then a self-referencing
  table still needs deferral.

Also add a snapshot test with a non-cascading FK, and consider not dead-lettering
`SQLITE_CONSTRAINT_FOREIGNKEY` silently on the snapshot path.

**Resolution**

Resolved 2026-11-09 (scadaproj#37, LocalDb `0.3.6`, unpublished). Re-verified with a 2-node test: a wiped peer
received `a_child` before `z_parent` and `node {"id":10}` before `node {"id":2}`. Both children were dead-lettered
(`SQLite Error 19: 'FOREIGN KEY constraint failed'`) and the watermark advanced past them. Fixed on both sides, and
the wire is unchanged:
- **Sender.** `SnapshotStreamer.ParentsFirstAsync` orders the tables in scope topologically by
  `pragma_foreign_key_list` (Kahn's algorithm, ordinal name tie-break). Self-references are ignored, and a cycle
  falls back to name order.
- **Receiver.** `LwwApplier.ApplyBatchAsync` gained `deferForeignKeyViolations`, used only by `SnapshotApplier`. An
  entry that fails with SQLITE_CONSTRAINT_FOREIGNKEY (787) is rolled back to its savepoint and returned as deferred,
  not dead-lettered. The applier retries deferred rows after every batch that applied something, until a pass makes
  no progress. At `SnapshotComplete` it makes one final pass with deferral off, which dead-letters the true orphans
  (with a warning). The delta path is unchanged, because commit order already puts parents first.

Tests (`SnapshotForeignKeyTests`, all 4 fail pre-fix):
- 2-node wiped-peer snapshot with `MaxBatchSize = 1`, cross-table and self-referencing;
- streamer table order;
- a receiver fed child-first in the old sender order;
- a true orphan dead-lettered only at complete.

README §Limitations documents the mixed-version caveat: a pre-0.3.6 receiver still loses child-first self-references.

### LocalDb-003 — `INSERT OR REPLACE` apply silently deletes a different row on a secondary `UNIQUE` conflict, so the pair diverges

| | |
|--|--|
| Severity | Medium |
| Priority | P2 |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/Internal/LwwApplier.cs:340` |

**Description**

`BuildUpsert` emits `INSERT OR REPLACE INTO <table> (...)`. When the incoming row collides with a
local row on a non-PK `UNIQUE` constraint, REPLACE deletes that other row, which has a different
primary key. The deletion:

- does not fire the capture triggers (they are guarded by the applying flag, and `recursive_triggers` is off anyway);
- updates no `__localdb_row_version` row;
- is never shipped to the peer.

LWW only ever compares versions of the key it was handed, so nothing detects the change.
`RegisterReplicated` (`SqliteLocalDb.cs:170`) checks PKs, BLOBs and cascading FKs, but not `UNIQUE`
indexes.

**Failure scenario**

Latent today: the ScadaBridge site tables registered for replication declare no secondary `UNIQUE`
constraint (checked 2026-11-09). Reproduced in a scratch test:

1. Create `t(id INTEGER PRIMARY KEY, code TEXT UNIQUE)` and register it on both nodes.
2. While disconnected, insert `(1,'X')` on A and `(2,'X')` on B, for example the same tag or
   name created on each node during a partition.
3. Reconnect and exchange deltas.

A applies `(2,'X')`, and REPLACE drops row 1. B applies `(1,'X')`, and REPLACE drops row 2. Final
state: A = `{2}`, B = `{1}`. Each row is applied (`applied=1, dl=0`) and A's ledger still says
`{"id":1}` is live. The pair is permanently diverged. Any later snapshot only logs "ledger/table
divergence" and skips the row (`SnapshotStreamer.cs:114`).

**Recommendation**

Reject, or at least warn on, registration of tables that have non-PK `UNIQUE` indexes or
constraints (`pragma_index_list` where `origin IN ('u')` and the index is not the PK). Alternatively,
apply with an explicit `INSERT … ON CONFLICT(<pk cols>) DO UPDATE SET …`. A unique conflict on
another column then aborts and becomes a visible dead-letter instead of a silent delete. Document
that secondary unique keys must be globally unique across both nodes.

**Resolution**

Resolved 2026-11-09 (scadaproj#38, LocalDb `0.3.6`, unpublished). Re-verified with a 2-node test that runs the
review's partition scenario: A ended with only row 2 and B with only row 1, and there were no dead-letters. The
applier now upserts on the primary key only, via `LwwApplier.BuildUpsert`:
`INSERT … ON CONFLICT(<pk cols>) DO UPDATE SET <non-pk> = excluded.<non-pk>`, or `DO NOTHING` when every column is
part of the PK. A clash on any other unique key aborts the statement. The existing per-entry savepoint rolls that
entry back and dead-letters it, so each node keeps its own row and its ledger stays consistent with its table. The
divergence is now visible in `__localdb_dead_letter`. It is still not auto-resolved, which the docs state.

The review's other option was to refuse registration of tables with a secondary `UNIQUE`. It was not taken, because
it could break an existing consumer's boot.

The change also fixes a defect found while testing: a remote update of a parent that has `ON DELETE RESTRICT`
children used to be dead-lettered, because REPLACE deletes before it re-inserts. It now applies.

The `RegisterReplicated` and README docs now state that secondary unique keys must be unique across both nodes. The
cascading-FK registration rationale now cites tombstone deletes rather than REPLACE.

Tests (`ApplyUpsertTests`): the 2-node secondary-UNIQUE conflict and the RESTRICT-parent update both fail pre-fix.
The all-columns-PK table is a guard. The commit also hardens
`SnapshotResyncTests.WipedPeer_SnapshotLiftsItsWatermark_…`. That test cancelled after the rows landed but before
`SnapshotComplete` had moved the watermark, and it failed intermittently in one of 4 full-suite runs. It now also
waits for the watermark.

### LocalDb-004 — `FailClosedOnDrift` is bypassed on the snapshot path, so a future-stamped snapshot row moves the local HLC forward permanently

| | |
|--|--|
| Severity | Medium |
| Priority | P2 |
| Category | Spec & shared-contract adherence |
| Status | Resolved |
| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/Internal/SnapshotStreamer.cs:249` |

**Description**

The drift guard (`MaxHlcDriftAhead` + `FailClosedOnDrift`) lives only in
`SyncSession.ApplyDeltaEntriesAsync` (`SyncSession.cs:599-628`). `SnapshotApplier.OnBatchAsync`
passes every snapshot row straight to `LwwApplier.ApplyBatchAsync`. It takes no
`ReplicationOptions`, so it cannot check drift. The applier then calls
`db.Clock.Observe(maxHlc)` (`LwwApplier.cs:155`), and `OnCompleteAsync` persists `last_seen_hlc`.
That value feeds clock recovery on every later open (`SqliteLocalDb.cs:69-75`).

This contradicts README §Semantics ("With `FailClosedOnDrift`, each over-drift entry is
dead-lettered") and design §Error handling ("configurable max-drift guard (warn, optionally
fail-closed)").

**Failure scenario**

Reproduced in a scratch test with `FailClosedOnDrift = true`. A peer whose clock was a year ahead
when it wrote a row serves a snapshot after a cap prune or rebuild. The receiving node writes 0
dead-letters, applies the row, and its HLC jumps about 365 days ahead. Every later local write on
that node is stamped a year in the future, and the stamp survives restarts through `last_seen_hlc`.
From then on those writes beat every genuine write on the peer under LWW until wall-clock time
catches up. The operator enabled `FailClosedOnDrift` specifically to prevent this.

**Recommendation**

Give `SnapshotApplier` the options and the clock, and run the same per-row drift check as the delta
path: dead-letter over-drift rows when `FailClosedOnDrift`, otherwise warn once. Better, move the
check into `LwwApplier` so both paths share it. Add a snapshot-drift test.

**Resolution**

Resolved 2026-11-09 (scadaproj#39, LocalDb `0.3.6`, unpublished). Re-verified with a 2-node test: A served a
snapshot to a wiped B. One of A's rows had a ledger HLC 365 days ahead. With `FailClosedOnDrift = true`, B applied
that row and wrote no dead-letter.

The fix: `SnapshotApplier` now takes `ReplicationOptions` and a `utcNow` seam, and `SyncSessionFactory` passes the
options. `OnBatchAsync` runs the same per-row check as `SyncSession.ApplyDeltaEntriesAsync`, against
`MaxHlcDriftAhead` and with one warning per snapshot. Under `FailClosedOnDrift`, it dead-letters an over-drift row
through `HlcDriftGuard.DeadLetterAsync`, a new helper that the delta path now uses too. That row's HLC is kept out of
`_maxHlc`, so `OnCompleteAsync` never persists it as `last_seen_hlc`. In warn-only mode, rows are applied, as on the
delta path.

The check stays in the session and snapshot layers rather than moving into `LwwApplier`. That keeps the delta path's
existing metrics and ack accounting unchanged.

Tests (`SnapshotDriftTests`): the test is a theory built through `SyncSessionFactory`. In fail-closed mode it checks
that the row is absent and that a drift dead-letter was written. It also checks that the receiver's `Clock.Current`
and `last_seen_hlc` stay below the future stamp; this case fails pre-fix. In warn mode it checks that the row is
applied and that no dead-letter is written. README §Semantics is updated.

### LocalDb-005 — Auto-assigned rowid / AUTOINCREMENT keys collide across nodes and LWW silently drops one row

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Documentation & XML docs |
| Status | Resolved |
| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb/Internal/SqliteLocalDb.cs:197` |

**Description**

Registration accepts any explicit PK, including an `INTEGER PRIMARY KEY [AUTOINCREMENT]` rowid
alias that each node assigns on its own. LWW keys on `pk_json`, so two distinct rows inserted with
`NULL` ids on the two nodes are treated as two versions of the same row, and one is discarded. This
is inherent to LWW with locally-assigned keys. However, neither the README §Limitations nor the
`RegisterReplicated` docs say that replicated keys must be globally unique (GUIDs or node-scoped).
The README quickstart even uses `id INTEGER PRIMARY KEY`.

**Failure scenario**

Reproduced in a scratch test:

1. Create `ev(id INTEGER PRIMARY KEY AUTOINCREMENT, msg TEXT)` and register it on both nodes.
2. Insert `('from A')` on A and `('from B')` on B, for example when both nodes of a pair log events
   during a split.
3. Exchange deltas.

Both nodes end with `1:from A` only. B's row is lost on both nodes, with no dead-letter or metric.

**Recommendation**

Document in README §Limitations and the `RegisterReplicated` XML docs that replicated PKs must be
globally unique, and that a rowid alias assigned on both nodes loses rows. Optionally log a warning
at registration when the sole PK column is an `INTEGER PRIMARY KEY` rowid alias.

**Resolution**

Resolved 2026-11-09 (scadaproj#46, LocalDb `0.3.6`, unpublished). Re-verified with a throwaway 2-node probe:
`ev(id INTEGER PRIMARY KEY AUTOINCREMENT, msg TEXT)`, `'from A'` inserted on A and `'from B'` on B, both nodes ended
with `1:from B` only and no dead-letter. Fix, taking the review's safer option: `RegisterReplicated` (both overloads)
now **refuses** a table whose primary key is `AUTOINCREMENT` with a `LocalDbRegistrationException` that names the
column and the remedy, before any trigger is installed (`SqliteLocalDb.EnsureNoAutoincrementKey`). Detection reads
SQLite's column metadata (`IsAutoIncrement` from `sqlite3_table_column_metadata`), not the `CREATE` text. A refusal
rather than a warning, because `AUTOINCREMENT` only exists to have SQLite assign the key, which is never safe on a
replicated table, and `SqliteLocalDb` has no logger to warn through.

A plain `INTEGER PRIMARY KEY` is still accepted, because an application may assign it from a node-scoped range;
refusing it would break schemas that are correct today. Its hazard when left `NULL` is documented instead: the
`RegisterReplicated` XML remarks and README §Limitations now say replicated primary keys must be unique across both
nodes and recommend writer-minted TEXT GUIDs, and the quickstart notes it.

Compatibility: checked read-only on 2026-11-09. ScadaBridge's 12 replicated tables (`SiteLocalDbSetup.ReplicatedTables`)
and OtOpcUa's 5 (`LocalDbSetup.ReplicatedTables`: the two deployment-cache tables, the alarm S&F events table, and
the alarm condition state and comments tables) all use TEXT or composite keys, and none is `AUTOINCREMENT`. Both apps had already replaced their historical
`INTEGER PRIMARY KEY AUTOINCREMENT` keys with GUIDs for this reason, so neither boot is affected.

Tests (`RegistrationTests`): `Register_AutoincrementKey_Throws` and `..._WithBaselineToo` fail pre-fix. Two guards pass
both ways: a plain `INTEGER PRIMARY KEY` still registers, and a column named `autoincrement` with a `DEFAULT
'AUTOINCREMENT'` is not mistaken for the keyword.

### LocalDb-006 — Column names containing `'` are captured but break the apply and snapshot SQL

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Correctness & logic bugs |
| Status | Resolved |
| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/Internal/LwwApplier.cs:349` |

**Description**

`TriggerSqlGenerator` escapes both quote styles (`Ident` doubles `"`, `Literal` doubles `'`), so
capture works for any column name. The replication side builds JSON paths with
`JsonPath(column) => "'$.\"" + column.Replace("\"", "\\\"") + "\"'"` (`LwwApplier.cs:349`,
`SnapshotStreamer.cs:205`). That code escapes `"` but not `'`, so a column name containing an
apostrophe ends the SQL string literal early. Only table names are regex-gated at registration
(`SqliteLocalDb.cs:172`); column names come from `PRAGMA table_info` unchecked. The input is the
local schema rather than the peer, so this is a robustness issue, not an injection vector.

**Failure scenario**

Reproduced in a scratch test. For a table with a column `"it's"`, capture on A succeeds. Applying on
B fails `Prepare` with `SQLite Error 1: near "s": syntax error`. That is a `SqliteException` inside
the per-entry try, so every row of the table is dead-lettered and acked (`applied=0 dl=1`). If the
column is part of the PK, the snapshot page query also throws and faults the session on every
reconnect.

**Recommendation**

Escape `'` in `JsonPath`, for example by building the path text and then passing it through the same
`Literal`-style doubling. Alternatively, validate column names at registration with the same
identifier regex used for table names, so registration fails loudly. Add a quoted-column round-trip
test.

**Resolution**

Resolved 2026-11-09 (scadaproj#46, LocalDb `0.3.6`, unpublished). Re-verified with a 2-node test: on a table
`q("o'key" TEXT PRIMARY KEY, "it's" TEXT, "say ""hi""" TEXT)` both nodes dead-lettered every peer row with
`SQLite Error 1: 'near "key": syntax error'`, and a wiped peer's snapshot faulted the sender's page query. The
apply and snapshot paths now build their JSON paths through one shared helper, `JsonPathSql.For`: the column's `"`
is backslash-escaped for the JSON path grammar, then every `'` in the path is doubled for the SQL string literal.
`LwwApplier` and `SnapshotStreamer` no longer carry their own copies. Column names are not newly restricted at
registration, so no existing schema is refused; the wire format is unchanged (the `row_json`/`pk_json` keys come from
capture, which already escaped both quote styles).

Tests (`QuotedColumnNameTests`): a 2-node delta round trip (insert both ways, then an update and a delete keyed on
the quoted PK) and a wiped-peer snapshot through a quoted PK column. Both fail pre-fix and pass after. The table also carries a
`"` column, which the old escape already handled.

### LocalDb-007 — README is stale on version, adoption, test count, `Synchronous` values and baseline scope

| | |
|--|--|
| Severity | Low |
| Priority | P3 |
| Category | Documentation & XML docs |
| Status | Resolved |
| Location | `ZB.MOM.WW.LocalDb/README.md:13` |

**Description**

Several README statements no longer match the code or the library's own `CLAUDE.md`:

- line 13: "Packages (all `0.3.4`)". `Directory.Build.props` is `0.3.5`.
- line 21: "adopted by … OtOpcUa (Phase 1)". OtOpcUa has adopted Phases 1+2.
- line 228: `Synchronous` lists `OFF` / `NORMAL` / `FULL`. The validator also accepts `EXTRA`
  (`LocalDbOptionsValidator.cs:14`).
- line 373: "see Limitations: baselining flags a full, all-tables snapshot". This contradicts the
  0.3.0 per-table behaviour described in the same README at lines 264–266 and 315.
- line 390: "199 tests". There are 216 at this commit.

**Failure scenario**

An operator re-enabling a table follows line 373 and trims every high-volume table, expecting an
all-tables snapshot. That is unnecessary work, and it obscures the real rule: only the baselined
table, an old peer, or a cap prune streams in full. A reader checking which version is current gets
0.3.4 from the README and 0.3.5 from the build.

**Recommendation**

Refresh those five lines when 0.3.5 is published.

**Resolution**

Resolved 2026-11-09 (scadaproj#46, LocalDb `0.3.6`, unpublished). Re-verified: all five statements were still
stale at `c207573`, and the version had since moved to `0.3.6`. README changes:
- **Packages heading.** It now reads `0.3.6`, and notes that the feed carries up to `0.3.4` and that `0.3.5` and
  `0.3.6` are unpublished.
- **Status.** OtOpcUa is listed as adopting Phases 1+2, both apps are shown pinned to `0.3.4`, and there are new
  `0.3.6` and `0.3.5` entries.
- **`Synchronous`.** It lists `OFF` / `NORMAL` / `FULL` / `EXTRA`, matching `LocalDbOptionsValidator.AllowedSynchronous`.
- **Ops notes.** The "baselining flags a full, all-tables snapshot" cross-reference now says that baselining streams
  that one table in full, and that only a cap prune or a pre-`0.3.0` peer streams everything.
- **Test count.** It is 249, the count after the #46/#50 fixes.

Documentation only; no test applies.