alkstore/tasks/review-wave-2.md

id, name, status, depends_on, scope, risk, impact, level, tags

id	name	status	depends_on	scope	risk	impact	level	tags
review-wave-2	Review gate — wave 2 (substrate fork)	completed	fork-provenance-and-floor	narrow	low	component	review	wave-2 review

Description

Review the substrate fork before wave 3 builds the engine on it. Primary lens: diff reviewability against the lineage (ADR-012 §3's fidelity posture — the fork's economics rest on future cherry-picks staying cheap) and the defect class staying out (the fork's reason for existing).

Check:

• Fidelity: the kept half's module structure and internal names match the lineage; renames confined to the table family and hygiene deltas; the diff vs. /workspace/honker @ f4e53c6 contains nothing unregistered.
• Port deltas: W-1/W-2/dead-man's-switch applied as decided; no other behavior changes smuggled in; W-4 (RW watcher connection) deliberately kept.
• Drops: cron, experimental watchers + their optional deps, rate-limit/result tables, superseded queue functions — absent, and their absence registered.
• Re-derivation vs. ADR-010: stamps land per §3a; the claim statement's ordering and visibility semantics match §1–§2; the validity predicate is uniform across ack/heartbeat/retry/fail; savepoint hardening real (test-read it, don't trust the test name); sweep covers both states; the curve/boundary arithmetic is not in the substrate (contract-blind boundary — ADR-012 §2).
• Provenance: register complete and accurate per ADR-018; license notice correct.
• Family standards in the substrate: sync only, no panics, no error swallows, no comments (doc comments fine).
• Gates: full engine-crate test suite, clippy -D warnings, fmt clean.

Acceptance Criteria

• Lineage diff reviewed; every divergence registered or fixed
• Re-derivation matches ADR-010's pinned semantics (spot-checked against the ADR text, not just the tests)
• Provenance register accurate
• All gates green
• Findings recorded; wave 3 decomposition may proceed

References

• docs/architecture/decisions/012-forked-substrate-design.md §3
• docs/architecture/decisions/010-queue-semantics-depth.md
• docs/architecture/decisions/018-provenance-register-and-cherry-picks.md
• docs/plans/implementation.md (Review gates)

Notes

Review performed over a fresh full-coverage lineage diff (subagent, honker-core @ f4e53c6 via git show vs the substrate tree) plus an ADR-010/020/019 spot-check of the re-derivation against the ADR text (not the tests).

Fidelity — clean. Every kept-half divergence is register-addressable: the table-family rename, W-1 (backoff tick counter verified at MAX_RECONNECT_TICKS; initial-open path lineage-identical), W-2 (fallible spawn; baseline-wait carried verbatim), dead-man's switch (panic → log+return; WatcherDeathGuard byte-faithful), W-4 keep-RW (open flags identical to lineage), the pragma_table_info race re-key, the D-16 attach-prune cap, cron_expr → spec, schema/migration extensions (column-order invariant pinned), and hygiene deltas. All kept internal names preserved. Drops verified absent (no cron symbols, no kernel/shm watcher, no rate-limit/result surfaces, no scheduler pause/resume/list/update). queue_ops.rs smuggle-scan: no .ok() swallows, no lineage-identical defect-carrying code.

Re-derivation vs ADR-010 — one real defect found and fixed this session (D-27): scheduler_tick passed the schedule row's relative expires_s directly into the re-derived enqueue's absolute expires_at parameter — the lineage resolved expires_s → now + s inside its enqueue, and ADR-020 §2 pins the relative→absolute resolution at the enqueue instant. A schedule with expires_s: Some(30) would have fired jobs with expires_at = 30 (expired since 1970): swept straight to dead, silent work loss. No prior test covered expiry through the tick. Fixed at the fire site in the tick's transaction (one clock read, now + s); regression test tick_resolves_the_fires_expires_sticker_to_an_absolute_row_value pins the resolved value and the fired job's claimability. This was the review's only code-behavior fix.

Minor, fixed inline (D-28): the cross-mechanism pressure test's lock-churn branch used a literal 'p{producer}' (no interpolation — all producers contended on one lock name) and swallowed its result via .unwrap_or(0); now per-producer names, periodic release, errors propagate. Test-quality only.

Minutiae, no action needed (recorded here, not as register rows): column_present's prepare-error → "absent" fallback produces a different error message than the lineage on a missing table (bootstrap still errors either way); bootstrap/poll_data_version doc-comment trims (D-15-adjacent); the concurrent-open race test's tuned magnitudes. None change behavior.

Register: D-27 (re-derivation fix) and D-28 (test hygiene) appended after the review's fixes; header updated. Cherry-picks stay empty.

Gates: cargo build ok; workspace cargo test — 23 core + 3 contract-suite + 84 sqlite (83 inherited + 1 new regression) — green in one run; cargo clippy --all-targets -- -D warnings clean; cargo fmt --check clean. The flake watch-record of fork-provenance-and-floor (one unreproducible transient) did not recur in this session's runs.

Boundary arithmetic check (ADR-012 §2): the equal-jitter curve's arithmetic is absent from the substrate — retry takes a caller- resolved literal delay; no backoff/curve symbols exist. The schedule boundary math present (parse_every_interval, next_every_boundary, skip_forward) is the ADR-010 §6 / fork-rederive-queue-ops-Notes sanctioned re-derivation surface (@every-only numeric advance per ADR-009 §2; cron machinery and local-TZ arithmetic absent).

Wave 3 watch-item (no action this wave): the W-2 Result<_, String> error is untyped — the engine-layer mapping into the Database variant is wave-3 work per fork-port-connection-watcher's Notes; nothing here blocks decomposition.

Summary

Wave-2 review gate complete. A fresh full-coverage lineage diff (honker-core @ f4e53c6) found every kept-half divergence register-addressable — no unregistered divergence. The ADR-spot-check of the re-derivation found one real contract-semantics defect (scheduler fire's relative expires_s passed through as an absolute expires_at; silent expiry-at-1970 work loss under expires_s-bearing schedules) — fixed inline with a regression test, registered D-27; one test-quality wart fixed and registered D-28. Gates: build, workspace test (110 green), clippy -D warnings, fmt — all clean. Wave 3 decomposition may proceed.