Wave-1 review gate: validation coverage fixes (unschedule, handle-level consumer-local entry points), ADR-008 §4 annotations (whitespace-exclusion, class scope), task check-line staleness fix (ADR-008/009/021, core-contract)

tasks/review-wave-1.md

@@ -1,7 +1,7 @@
 ---
 id: review-wave-1
 name: Review gate — wave 1 (core contract surface)
-status: pending
+status: completed
 depends_on: [core-trait-surface, contract-suite-scaffold]
 scope: narrow
 risk: low
@@ -31,7 +31,10 @@ Check:
   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 six variants, no extras; `Database`
+- **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).
@@ -47,10 +50,10 @@ Check:
 
 ## Acceptance Criteria
 
-- [ ] Every divergence from pinned ADR text found and fixed (or an ADR
+- [x] 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)
+- [x] All gates green
+- [x] Findings recorded (task Summary or review notes)
 
 ## References
 
@@ -61,8 +64,75 @@ Check:
 
 ## Notes
 
-> To be filled by implementation agent
+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
 
-> To be filled on completion
+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.