--- id: review-wave-1 name: Review gate — wave 1 (core contract surface) status: completed depends_on: [core-trait-surface, contract-suite-scaffold] scope: narrow risk: low impact: project level: review tags: [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` 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 - [x] Every divergence from pinned ADR text found and fixed (or an ADR amendment proposed if the text itself is wrong) - [x] All gates green - [x] 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 `expect`s 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.