138 lines
6.8 KiB
Markdown
138 lines
6.8 KiB
Markdown
---
|
|
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<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
|
|
|
|
- [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. |