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 — 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, wave-6 N-1 review) 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.