alkstore/docs/reviews/001-waves-1-2-general-review.md

# General review — waves 1 and 2

> **Post-review note (2026-10-08):** the four decision-needing
> findings below are resolved as
> [ADR-023](../architecture/decisions/023-fourth-review-round.md) —
> N-2 (`encode_payload` typed, `Codec`), M-2 (numeric-argument domains
> pinned contract-side, extents clamp empty — decided broader than
> this report's engine-layer recommendation: the same dialect hazard
> exists on the stream reads, and domains must be engine-uniform),
> N-4 (URI flag dropped, a registered fork delta), N-6 (1 ms default
> stands, cadence carried onto `SqliteOpts`). The task-carried items
> (lint removal, wave-5 suite adds, release-readiness N-1 review —
> wave 7 since ADR-024's renumbering) stand as
> ordered in §7.

- **Reviewer**: opencode (glm-5.3-flash)
- **Scope**: general review of everything landed by waves 1–2 (11 tasks, commits
  `34e0b97..af5b59e`) — security posture, code smells, contract adherence,
  and coverage (cargo-llvm-cov). Distinct from the two wave review gates, which
  were type/shape-scoped.
- **Input**: full read of `alkstore/src` (all 23 files), the substrate subtree
  (`schema.rs`, `watcher.rs`, `ops.rs`, `queue_ops.rs`, `PROVENANCE.md`), the
  contract suite, the trait doc text vs `core-contract.md` and the ADRs, and
  spot-checks against the upstream checkout `/workspace/honker` @ `f4e53c6`.
- **Gates at review time**: build/test/clippy/fmt all green. Coverage 93.1%
  lines / 94.5% functions workspace-wide before this review's inline fixes.

## Verdict

**Solid.** The contract-blind boundary (ADR-012 §2) holds under line-by-line
inspection; the re-derived queue ops are consistently built (uniform validity
predicate everywhere, savepoint-guarded dead-letter moves, no stranded-rows
testing under induced failures); the provenance register is unusually rigorous;
core's doc text matches `core-contract.md` nearly verbatim. Two findings are
real (M-1, M-2); the rest are minor or noted-for-later. M-1/M-2 are substrate-
layer, reachable only through wave-3's engine wiring — no consumer surface
exists yet (engine crates are stubs), so none of this blocks wave 3 from
starting; it just needs to fix or consciously absorb these first.

---

## 1. Security review

Nothing alarming. Checked specifically for:

- **SQL injection** — every SQL statement is a static string; all user-supplied
  values (names, queue names, specs, payloads) ride `rusqlite::params![]`
  binds. The one `format!`-built DDL path (`SAVEPOINT {name}` /
  `RELEASE SAVEPOINT {name}` in `in_savepoint`, ops.rs:71,79) carries only
  compile-time constant savepoint names ("alkstore_*"), never user data —
  safe by construction. `column_present`/`add_column_if_absent` interpolate
  table/column names built from the same constant list.
- **Path/URI injection** — `open_conn` enables `SQLITE_OPEN_URI` (inherited
  from upstream); the path reaches `Connection::open_with_flags` from the
  engine layer's constructor, so a wave-3 engine must not pass consumer-
  influenced strings there without review. Noting it now so the wiring task
  doesn't inherit it silently (N-4).
- **Payload handling** — payloads cross as `serde_json::Value`, serialize
  once (`encode_payload`), and decode via `payload_as` with `Error::Codec`
  mapping; no re-parsing of stored strings beyond serde_json. `encode_payload`
  maps serialization failure to empty bytes (see N-2) — the engine layer
  should validate before insert when wave 3 lands.
- **`unsafe`** — exactly one block per substrate module pair:
  `ctx.get_connection()` in the scalar-function wrappers (the only rusqlite-
  sanctioned way to reach the connection from a UDF) and its use is immediate
  and scoped. `WatcherDeathGuard`/watcher threads contain no unsafe.
- **DoS-shaped surface** — `scheduler_tick` bounds catch-up fires
  (`SCHEDULER_MAX_CATCHUP_FIRES = 64`); `claim_batch` honors its limit only
  via SQLite's `LIMIT ?2` semantics (see M-2); `NOTIFY_ATTACH_MAX_ROWS` caps
  notification growth at attach (D-16); `arg_i64`/`real_to_i64` reject
  fractional/out-of-range REALs cleanly rather than truncating (tested).
- **Secrets** — none; nothing logged beyond watcher diagnostics.

## 2. Findings

### M-1 · `sweep_expired`'s retention half runs outside the savepoint guard — the no-stranded-rows property has a hole (FIXED INLINE)

**Where**: `queue_ops.rs` — `sweep_expired` (was lines 236–246).

**What**: the savepoint frame covered `sweep_expired_live` (expiring live
rows → dead) only; `sweep_dead_retention` (DELETE of aged-out dead rows) ran
after the frame. The module's own doc text
("savepoint-guarded dead-letter moves (the #133 defect class, not inherited)")
and the register's D-12 entry claim both halves are the guarded, atomic shape.
Before the fix, a DB failure mid-retention-DELETE (e.g. `SQLITE_FULL`) after a
successful live→dead sweep did NOT strand rows (both halves commit
independently), but it made `sweep_expired` a two-atomicity operation whose
error left the sweep's *effects* partially applied with `Err` returned —
exactly the state the savepoint guard exists to preclude, and a property the
module claims (and only half-tests: `sweep_rolls_back_no_stranded_rows_*`
induce the failure in the live half).

**Fixed by**: moving `sweep_dead_retention` inside the `in_savepoint` frame
(`Ok(live_moved + retained)` out of the closure). Semantics unchanged when
nothing fails — retention deletion is idempotent, the moved+retained count is
identical — and the error path now actually rolls back what the property says
it does. All 84 substrate tests (retention, no-stranded-rows, decode-failure
rollbacks) pass untouched.

**Follow-up**: the coverage hole is now the only remaining gap — no test
induces retention-DELETE failure. The natural test (trigger raising ABORT on
`__alkstore_dead` DELETE) is sketched in §4 below; left for the wave-5
contract suite or a wave-3 substrate touch-up rather than added now, since
M-1's fix itself is behavior-preserving and fully covered by existing tests.

### M-2 · `claim_batch` ignores a non-positive `n` (upstream-inherited shape, needs an engine-layer guard)

**Where**: `queue_ops.rs` — `claim_batch`, `LIMIT ?2` in the claim CTE.

**What**: in SQLite, `LIMIT -1` = no limit and `LIMIT 0` = empty. So
`claim_batch(conn, q, w, -1, …)` claims the **entire queue**, and
`claim_batch(conn, q, w, 0, …)` is a silent no-op return. Contract text
(core-contract.md queue section, and the `Queue::claim_batch` doc in
`alkstore/src/queue.rs`) pins "Claim up to `n` rows" — no non-positive rule.
The substrate honors its contract-blind discipline by passing `n` through,
which is *correct* for the fork; but wave 3's engine layer will inherit this
behavior unless the trait-impl entry point clamps/rejects `n` first.

**Recommended**: wave 3's engine wiring validates `n >= 1` (or clamps to 0 =
no-work value) at the trait-impl boundary, mirroring how `lock_renew` guards
`ttl_s <= 0` substrate-side. The engine-layer resolution point is the right
place per the ADR-012 boundary — this note exists so the decision is made
there deliberately, not inherited from SQLite's dialect. Adding a test to the
contract suite row (`claim_batch(n=0)` behavior) is cheap and pins whatever
the engine layer decides.

### N-1 · Unbounded growth paths in the engine layer, deferred by design (record, not action)

Three table families grow without internal hygiene caps: `__alkstore_dead`
with `dead_letter_retention_s = NULL` (the default — the doc text is honest
that retention is opt-in), `__alkstore_notifications` rows pruned only at
attach (D-16's 10k cap triggers only next attach — an idle-but-listening
process never prunes), and `__alkstore_stream` (growth is the contract's
durable-log posture; `trim_to` is consumer-invoked per ADR-015). None of
these is wrong for v1; all three are documented engine-internal. **Recorded
so wave 6's release-readiness review consciously accepts them**; the notify
one in particular may want an "attach is not the only prune site" note in the
sqlite engine's docs (cadence recipes) when wave 3 wires the engine.

### N-2 · `encode_payload`'s empty-Vec failure arm is unreachable-but-wrong-looking

`serde_json::to_vec` on a `Value` cannot fail in practice (all its variants
represent valid JSON); the `unwrap_or_else(|_| Vec::new())` arm maps failure
to empty bytes, which would silently store a decode-to-`Null` payload. Two
better shapes: `Result<Vec<u8>, Error::Codec>` (a signature change — core is
pre-release, cheap now) or debug_assert-unreachable. ADR-020 §4 pins "engines
serialize with serde_json and store exactly those bytes" without pinning the
helper's signature, so neither shape violates the ADR. Core-side, minor, cheap
to fix before wave 3 starts consuming `encode_payload` (the sqlite engine is
where the payload bridge first calls it); flagged as a **wave-3 pre-work
candidate** rather than changed inline — signature choice is a call the engine
implementers should weigh in on since both engines consume this helper.

### N-3 · Validation asymmetry: `validate_name`'s whitespace-both-kinds rule is duplicated as prose in `Doc` but the whitespace set is narrow

`validate_name` rejects names whose `trim().is_empty()` — tab, space, newline
(and their mixes). Notably this does NOT reject zero-width, `\r`-only, or
other unicode-blank-only names (e.g. "\u{200B}", "\r"). Harmless (they round-
trip stringly everywhere and never collide with the reserved prefix; the
engines' SQL never interprets them), and `name.trim()` uses
char::is_whitespace only — so "\r" alone (is_whitespace = true) IS rejected.
Zero-width space "\u{200B}" is not whitespace by Rust's definition, passes,
and is arguably a legal (if ugly) name on every engine. **Not a bug**; noting
so future review rounds don't re-derive the question. No action.

### N-4 · `SQLITE_OPEN_URI` is inherited from upstream and reaches the wave-3 engine constructor

`open_conn` passes `OpenFlags::SQLITE_OPEN_URI` (honker uses it for `file:`),
and the watcher's `open_watcher_conn` does not. URI mode means the *path*
string is URI-interpretable (`?` params like `?nolock=1` mutate locking). No
consumer path reaches it yet (engine crates are stubs); wave 3's `open`
signature should either (a) drop the flag (breaking nothing today), or (b)
document that the path argument is URI-interpreted. Cheap to decide now;
surfaces here because it's the substrate's inherited attack surface, not the
engine's.

### N-5 · `with_tx`'s panic-disposition is documented but half-tested

The doc text (store.rs:123) pins "`Err`, drop, or panic across the closure ⇒
rollback (ADR-021 §4's drop = rollback)". `trait_probe_tests.rs` exercises
Ok→commit and Err→rollback; no test drives a panicking closure through
`with_tx`. The panic path matters because the closure's future is pinned
inside `with_tx`'s own future — the rollback depends on the *engine's*
TxHandle Drop impl being panic-safe, which is wave-3 territory (the sqlite
engine's handle-on-lease Drop). Adding a panics-closure probe to core is
cheap; flagged for the wave-5 contract suite rather than inline (panics-in-
closure semantics can only be fully pinned once a real engine's handle exists
— the Probe store would only test the probe).

### N-6 · Watcher poll cadence inherited at 1ms

`DEFAULT_WATCHER_POLL_INTERVAL = 1ms` with a `PRAGMA data_version` query per
tick — the lineage's default, kept verbatim per D-20. This is ~1000 pragma
queries/sec/instance on an idle store. Honker ships the same number, and the
config surface (`with_poll_interval`) allows raising it; but the wave-3
engine's `open` should decide whether 1ms is the shipping default or a dev
default (SQLite's `data_version` read is cheap-ish, but it's a wake-signal
cadence a busy system could tune). Not a defect; a tuning decision for wave
3's config surface.

---

## 3. Code smells / smells-adjacent

**None blocking.** Noted, not acted on:

- `mod.rs` sets crate-wide `#![allow(dead_code)]` `#![allow(unused_imports)]`
  (documented, wave-2 posture — the substrate isn't wired yet). **This lint
  suppression must be removed when wave 3 wires the engine layer** — it will
  otherwise mask genuinely dead substrate code that should be ported-cut.
  Filed here so wave 3 doesn't miss the removal.
- `row_to_dead_job` clones `row_to_live_job`'s decode then overwrites four
  fields — slightly clever, but the DEAD_SELECT's shape (constant list,
  matching `LIVE_COLUMNS` order + 2 extra) makes it readable; leave as-is.
- `queue_next_claim_at`'s `claim_expires_at + 1` in the UNION second arm is
  correct (a processing row's wake is the first second past its deadline) but
  under-documented in SQL; the doc text above it (line 1219's `>= unixepoch()`
  note) exists — fine.
- `queue_ops.rs` line-formatting churn between `enqueue(...)` callsites is
  rustfmt-normal; no smells.

## 4. Coverage analysis (cargo-llvm-cov)

Workspace 93.10% lines / 94.52% functions before this review's inline fixes;
~85% after on previously-missed substrate regions (see deltas below).

Per-file notes (only the interesting ones):

| File | Cover | Notes |
|---|---|---|
| `stop_token.rs` | 63.6% | **Fixed inline**: `Debug` impl lines 51–55 were the only miss; new `stop_token_debug_shows_cancelled_state` test covers them. Now 100%. |
| `ops.rs` | 87.9% | **Partly fixed inline**: `arg_opt_i64` (28–33) was fully dead — new `optional_arg_helper_keeps_null_and_converts_whole_reals` test covers it. Remaining misses are error-path bodies (real_to_i64's message arms except the fractional one, savepoint-undo arms, `UnwindUndo` drop log path, stream/lock fn wrappers' err paths) — all error arms or wrapper glue; the machinery itself is covered. Two notable uncovered lines: 103–108 (`UnwindUndo::drop` eprintln on post-panic rollback failure) is genuinely unreachable-in-test territory; 171/177-184/190-197 are the scalar-attachment sites' success bodies, covered only through the pressure suite's stream/lock paths (which do run). |
| `queue_ops.rs` | 96.0% | Very strong. Missed lines are error-path tails (105 = claim CTE's RETURNING tail under a claim-race arm; 122/137/158/188/214/316/324/335/397 = `.execute`-error tails; 442/472/493 = dead_letter_exhausted's error path tail; 525/562 = parse/spec error arms partially covered; 1215 = asserted-dead line in an assert-driven test). None is a semantic hole; the no-stranded-rows, validity-predicate, and catch-up-cap suites are extensive. |
| `schema.rs` | 89.6% | Misses are `add_column_if_absent`'s migration arms (244–305) — the pre-existing-schema migration test exercises the happy ALTER path but each absent-column arm's *err* branch is uncovered; `Write`/`Readers` close paths partially covered through the lock tests. Wave 3 (real engine `open` integration) will naturally widen this. |
| `watcher.rs` | 93.6% | Missing: 65–71 (HighRes/LowRes `file_id` variants — Windows-only, fine to leave), 99–110 (`is_transient_lock_error` matches — driven only indirectly), 187–202 (reconnect-loop success body — the W-1 test drives the *failure* path; a reconnect *success* test would require a file appearing mid-run), 280/331/386/428/437 (drop guards' success bodies), 473/586-591 (death-signal test's arms). The reconnect-success case (187–202) is the one worth a cheap test if wave 3 touches the watcher at all. |
| `payload.rs` | 92.3% | One missed line = `encode_payload`'s failure arm (see N-2 — if the helper's signature becomes fallible, coverage and correctness both improve). |
| contract-suite `properties.rs` | 94.9% | Exemplar row only so far; wave 5 fills rows (by design). |

**Suggested wave-5 contract-suite adds** (from the coverage + finding analysis):
sweep-retention-failure rollback pin (M-1's follow-up), `claim_batch` non-
positive-`n` decision pin (M-2's follow-up), `with_tx` panic-disposition probe
(N-5's follow-up), and the reconnect-success watcher path if the engine keeps
the current watcher shape.

## 5. Contract-adherence spot-checks

Checked (and passing) specifically:

- `#[non_exhaustive]` placement matches ADR-017 §3 exactly: `Error`,
  `Job`, `JobState`, `Schedule`, `StreamEvent`, `Wake` — on; `EnqueueOpts`,
  `QueueOpts`, `ScheduleOpts` — off (opts are consumer-constructed).
- Core deps: `serde`, `serde_json`, `thiserror` only — no driver deps
  (ADR-001/ADR-020 §4).
- Core has no `unwrap`/`expect`/`panic!` outside `#[cfg(test)]` modules and
  one documented-in-ADR test-relaxation (contract-suite rows).
- Trait surface matches `core-contract.md`'s pinned list method-for-method
  (`Store`, `TxHandle`, `Queue`, `StreamHandle`, `Outbox`, `Lock`,
  `JobHandle`, `EventReceiver`, `WakeReceiver`, `StopToken`, `WithTxClosure`).
- `with_tx`'s provided-body shape matches ADR-021 §4's pinned signature and
  Ok⇒commit / Err⇒rollback dispositions.
- Reserved namespace: `RESERVED_PREFIX`/`RESERVED_LISTENER_RECONNECTED`
  exact-match core-contract.md; substrate-derived outbox queues will ride
  `__alkstore_outbox:{name}` (engine-side, wave 3).
- Substrate is contract-blind: no `alkstore` types, no core imports under
  `alkstore-sqlite/src/substrate/` (verified grep: zero `alkstore::` in the
  subtree).
- Provenance register: fork point, D-01..D-28 all present, categorized,
  lineage-tagged; D-27/D-28 (wave-2 gate fixes) present. `LICENSE` bundle
  (MIT OR Apache-2.0) in-tree under `src/substrate/`.
- Substrate code carries no panics, no unwrap/expect outside tests.

## 6. Inline fixes made by this review

1. `stop_token.rs` Debug coverage — test added (`value_type_tests.rs`).
2. `arg_opt_i64` dead-code coverage — test added (`ops.rs` real_arg_tests).
3. **M-1 fix**: `sweep_expired` retention DELETE moved inside the
   `in_savepoint` frame (behavior-preserving; the whole operation now
   actually satisfies the no-stranded-rows property under failure).

All gates re-run green after the fixes: build, workspace tests
(24 + 85 + 3), clippy `-D warnings`, fmt --check.

## 7. Recommended ordering

- **Before wave 3 starts**: decide N-2's `encode_payload` signature (tiny,
  core-side, both engines consume it) and N-4's URI-open posture (engine
  constructor). Both are 10-minute decisions, both get cheaper if made now.
- **Wave 3 wiring tasks**: carry M-2's `n` guard decision, remove the two
  `allow` lints in `substrate/mod.rs`, and adjudicate N-6's poll-cadence
  default.
- **Wave 5 contract suite**: the §4 test adds.
- **Wave 6 release review**: consciously accept N-1's growth postures or
  add engine-side hygiene.