alkstore/tasks/review-wave-1.md

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

id	name	status	depends_on	scope	risk	impact	level	tags
review-wave-1	Review gate — wave 1 (core contract surface)	completed	core-trait-surface contract-suite-scaffold	narrow	low	project	review	wave-1 review

Description

Review the wave-1 output before wave 2/3/4 work builds on it. The core trait surface is versioned contract surface from the first release (ADR-017) — a shape error found here is cheap; found in wave 5 it is a migration. This review is line-by-line against the pinned ADR text, not a general code review.

Check:

• Trait surface vs. ADR text — every method set and signature in core-trait-surface against ADR-008 §1–§3/§8, ADR-014, ADR-019 §1–§5, ADR-021: exact names, exact field types, the one-shot/repeatable split (heartbeat(&self) vs self: Box<Self> ops), the recv_timeout/no-recv_timeout asymmetry between the two receiver traits, with_tx's pinned signature.
• Value types vs. ADR text — Job's full field list (including claimed_at, ADR-021 §2), StreamEvent's shape (ADR-015 §3), opts structs' types and defaults (ADR-010 §3, ADR-020), the #[non_exhaustive] split (read types yes, opts structs no — ADR-017 §3).
• Error taxonomy — exactly the ADR-008 §5 six plus the two ADR-009 §6 scheduler variants (InvalidSpec, LeadershipLost) that ship inside contract v1's initial text (core-contract §Errors is the content of record — eight total, no others); Database opaque with source chain; the pinning rule respected.
• Validation — both name classes correct (shared-namespace vs consumer-local; ADR-008 §4 third-round annotation).
• Family standards — no panics, no unwrap()/expect() outside tests, no comments in code (doc comments on public API fine), module-per-file, re-exports from lib.rs.
• Lean core — no engine machinery, no resolution arithmetic, no SQL-shaped anything in core (ADR-012 §2's boundary).
• Suite scaffold — ADR-022 exists and is coherent with ADR-017; exemplar property green; stamp convention usable.
• Gates — cargo test (workspace), cargo clippy --all-targets -- -D warnings, cargo fmt --check all green.

Acceptance Criteria

• Every divergence from pinned ADR text found and fixed (or an ADR amendment proposed if the text itself is wrong)
• All gates green
• Findings recorded (task Summary or review notes)

References

• docs/architecture/decisions/008-contract-v1-pinning.md
• docs/architecture/decisions/019-mechanism-handle-surfaces.md
• docs/architecture/decisions/021-tx-reads-and-value-shape-fixes.md
• docs/plans/implementation.md (Review gates)

Notes

Line-by-line review against ADR-008 §1–§8, ADR-007, ADR-009 §1–§6, ADR-010 §1–§5, ADR-014, ADR-015 §1–§5, ADR-017 §3, ADR-019 §1–§6, ADR-020 §1–§4, ADR-021 §1–§5, ADR-022, and core-contract.md. The trait surface, value types, error taxonomy, and suite scaffold are a faithful transcription overall — every trait/method name, the one-shot/repeatable split, object-safety encoding, #[non_exhaustive] placement, reserved strings, and the with_tx signature check out verbatim. All findings were inline-fixable; none required an ADR amendment beyond dated class-1 annotations (pre-first-release window, ADR-017 §2).

Findings (all fixed in this task's commit):

1. unschedule validation not stated/asserted — ADR-009 §1 pins schedule()and unschedule() as name-bearing entry points; the Store::unschedule doc carried no validation obligation, the exemplar row never asserted it, and the mock didn't validate. Fixed: doc text + row cases (unschedule(empty) → InvalidName, unschedule(reserved) → ReservedName) + mock validation.
2. Handle-level consumer-local entry points unasserted — the exemplar row claimed "every name-bearing entry point" but skipped Queue::claim_one/claim_batch (worker_id), Outbox::run_once (worker_id), and StreamHandle's consumer arguments (save_offset/get_offset/read_from_consumer), plus get_offset_tx's consumer argument (its tx siblings were asserted). Fixed: row cases added; mocks now validate at those entry points; trait docs on the skipped methods now state the non-empty rule.
3. Consumer-local class scope ambiguity — ADR-008 §4's third-round annotation names the class ("stream-consumer names") but enumerated only save_offset(_tx)/subscribe; whether every method's consumer argument takes the class rule was implicit. Fixed with a dated ADR-008 §4 annotation: the class is by identifier role, not per-method enumeration (covers the receiver/direct/tx forms uniformly).
4. Whitespace-only names — the implementation rejects whitespace-only names on both name classes (InvalidName) while the ADR text's rule is bare "non-empty." Deliberate (documented in the errors task's notes) but unrecorded at the normative site — would have read as a contract violation to any later auditor. Fixed with dated ADR-008 §4 + core-contract naming annotations (non-empty-after-whitespace-exclusion; names not trimmed). Pre- release class-1 annotation, permissible now, expensive after 1.0.
5. Task-text staleness (this file) — the "exactly six variants" check-line predates ADR-009 §6's two scheduler variants, which core-contract §Errors pins as part of contract v1's initial text (the code's eight-variant Error is the correct transcription). Fixed the check-line; no code change.

Accepted-with-notes (no action): JobState carries #[non_exhaustive] beyond ADR-017 §3's literal five-type list (Error, StreamEvent, Job, Schedule, Wake) — conservative and consistent with that ADR's read-type rule; adding it to the list is an amendment wave 5's review could ride. encode_payload's unwrap_or_else(|_| Vec::new()) fallback is unreachable in practice (serializing a serde_json::Value cannot fail) and panics nowhere; the posture is documented in the value-types task's notes. The suite's panic-based assertion posture and expects are the documented test- side-artifact relaxation (ADR-022).

Summary

Wave-1 review complete. Six files fixed inline (store.rs, queue.rs, outbox.rs, stream_handle.rs, tx.rs doc texts; exemplar row + mock validations in the suite), two dated class-1 annotations added to ADR-008 §4 (whitespace-exclusion rule; consumer-local class scope), one to core-contract.md's naming section, and this task's six-variant check-line corrected to the eight-variant contract-v1 text. All gates green after the fixes: workspace cargo build, cargo test (26 tests), cargo clippy --all-targets -- -D warnings, cargo fmt --check. No trait/method signature changed — every fix is doc text, suite-row breadth, or mock posture; the contract surface ships to wave 2/3/4 exactly as pinned.