General review waves 1-2: sweep_expired savepoint scope fix (M-1), coverage adds (arg_opt_i64, StopToken Debug), review report (docs/reviews/)

alkstore-sqlite/src/substrate/ops.rs

@@ -424,6 +424,31 @@ mod real_arg_tests {
         assert_eq!(v, 3);
     }
 
+    #[test]
+    fn optional_arg_helper_keeps_null_and_converts_whole_reals() {
+        let conn = db();
+        conn.create_scalar_function(
+            "alkstore_opt_arg_probe",
+            1,
+            FunctionFlags::SQLITE_UTF8,
+            |ctx| arg_opt_i64(ctx, 0),
+        )
+        .unwrap();
+
+        let v: Option<i64> = conn
+            .query_row("SELECT alkstore_opt_arg_probe(?1)", [3.0_f64], |r| r.get(0))
+            .unwrap();
+        assert_eq!(v, Some(3));
+        let n: Option<i64> = conn
+            .query_row(
+                "SELECT alkstore_opt_arg_probe(?1)",
+                [rusqlite::types::Value::Null],
+                |r| r.get(0),
+            )
+            .unwrap();
+        assert_eq!(n, None);
+    }
+
     #[test]
     fn fractional_reals_are_still_rejected() {
         let conn = db();

alkstore-sqlite/src/substrate/queue_ops.rs

@@ -239,10 +239,11 @@ pub(crate) fn sweep_expired(
     expired_error: &str,
 ) -> rusqlite::Result<i64> {
     let moved = in_savepoint(conn, "alkstore_sweep_expired", || {
-        sweep_expired_live(conn, queue, expired_error)
+        let live_moved = sweep_expired_live(conn, queue, expired_error)?;
+        let retained = sweep_dead_retention(conn, queue)?;
+        Ok(live_moved + retained)
     })?;
-    let retained = sweep_dead_retention(conn, queue)?;
-    Ok(moved + retained)
+    Ok(moved)
 }
 
 pub(crate) fn queue_next_claim_at(conn: &Connection, queue: &str) -> rusqlite::Result<i64> {

alkstore/src/value_type_tests.rs

@@ -153,6 +153,14 @@ fn stop_token_independent_tokens_do_not_flip_each_other() {
     assert!(!b.is_cancelled());
 }
 
+#[test]
+fn stop_token_debug_shows_cancelled_state() {
+    let token = StopToken::new();
+    assert!(format!("{token:?}").contains("cancelled: false"));
+    token.cancel();
+    assert!(format!("{token:?}").contains("cancelled: true"));
+}
+
 #[test]
 fn wake_carries_only_the_channel() {
     let w = Wake {

docs/reviews/2026-10-08-waves-1-2-general-review.md

@@ -0,0 +1,280 @@
+# General review — waves 1 and 2
+
+- **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.