From 8b03960e39ff330a5f7e5540e6b75ab4374a21b4 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Wed, 7 Oct 2026 16:15:48 +0000 Subject: [PATCH] =?UTF-8?q?Wave-1=20review=20gate:=20validation=20coverage?= =?UTF-8?q?=20fixes=20(unschedule,=20handle-level=20consumer-local=20entry?= =?UTF-8?q?=20points),=20ADR-008=20=C2=A74=20annotations=20(whitespace-exc?= =?UTF-8?q?lusion,=20class=20scope),=20task=20check-line=20staleness=20fix?= =?UTF-8?q?=20(ADR-008/009/021,=20core-contract)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- alkstore-contract-suite/src/properties.rs | 63 +++++++++++++- .../tests/suite_harness.rs | 40 +++++---- alkstore/src/outbox.rs | 3 +- alkstore/src/queue.rs | 3 +- alkstore/src/store.rs | 5 +- alkstore/src/stream_handle.rs | 9 +- alkstore/src/tx.rs | 6 +- docs/architecture/core-contract.md | 5 +- .../decisions/008-contract-v1-pinning.md | 16 +++- tasks/review-wave-1.md | 84 +++++++++++++++++-- 10 files changed, 196 insertions(+), 38 deletions(-) diff --git a/alkstore-contract-suite/src/properties.rs b/alkstore-contract-suite/src/properties.rs index 46db9cd..d82bb86 100644 --- a/alkstore-contract-suite/src/properties.rs +++ b/alkstore-contract-suite/src/properties.rs @@ -9,10 +9,20 @@ use serde_json::json; -use alkstore::{BoxedFuture, Error, RESERVED_PREFIX, Result, validate_shared_name}; +use alkstore::{ + BoxedFuture, Delivery, Error, JobHandle, RESERVED_PREFIX, Result, validate_shared_name, +}; use crate::factory::StoreFactory; +struct NoDelivery; + +impl Delivery for NoDelivery { + fn deliver<'a>(&'a mut self, _job: Box) -> BoxedFuture<'a, Result<()>> { + Box::pin(std::future::ready(Ok(()))) + } +} + fn expect_invalid(label: &str, out: alkstore::Result) { match out { Err(Error::InvalidName { .. }) => {} @@ -49,9 +59,13 @@ fn expect_ok(label: &str, out: alkstore::Result) { /// [`Error::InvalidName`], and the reserved-prefix name /// (`__alkstore_...`) with [`Error::ReservedName`], on the /// shared-namespace kinds — channels, streams, queues, outboxes, -/// locks, schedule names, and `schedule()`'s queue argument. +/// locks, schedule names (including `unschedule`), and `schedule()`'s +/// queue argument. /// Consumer-local identifiers take the non-empty rule only: `owner` -/// reserved-prefixed is legal, `owner` empty is not. The tx path is +/// reserved-prefixed is legal, `owner` empty is not — likewise +/// `worker_id` on the claim/pull entry points (`claim_one`, +/// `claim_batch`, `run_once`) and the consumer arguments on the +/// stream handle's offset/read forms. The tx path is /// exercised through `begin_tx` and `with_tx` (the provided wrapper): /// validation must not be bypassable by the commit-atomic path. /// @@ -96,6 +110,11 @@ pub async fn name_validation_rejects_empty_and_reserved(factory: &dyn StoreFacto .schedule("tick", "@every 1s", "", json!(null), Default::default()) .await, ); + expect_invalid("unschedule(empty name)", store.unschedule("").await); + expect_reserved( + "unschedule(reserved name)", + store.unschedule(RESERVED_PREFIX).await, + ); // Auto-commit paths — reserved-prefix names. expect_reserved( @@ -155,6 +174,40 @@ pub async fn name_validation_rejects_empty_and_reserved(factory: &dyn StoreFacto store.try_lock("lock", RESERVED_PREFIX, 60).await, ); + // Handle-level name-bearing entry points: the claim/pull ops' + // `worker_id` and the stream handle's consumer arguments are + // consumer-local identifiers (non-empty only, ADR-008 §4's + // third-round annotation) — validated at the handle's entry + // points identically to the store-level ones. + let queue = store + .queue("work", alkstore::QueueOpts::default()) + .await + .expect("queue handle"); + expect_invalid("claim_one(empty worker_id)", queue.claim_one("").await); + expect_invalid( + "claim_batch(empty worker_id)", + queue.claim_batch("", 1).await, + ); + let stream = store.stream("work").await.expect("stream handle"); + expect_invalid( + "save_offset(empty consumer)", + stream.save_offset("", 0).await, + ); + expect_invalid("get_offset(empty consumer)", stream.get_offset("").await); + expect_invalid( + "read_from_consumer(empty consumer)", + stream.read_from_consumer("", 10).await, + ); + expect_invalid( + "get_offset(whitespace-only consumer)", + stream.get_offset(" \t ").await, + ); + let mut outbox = store.outbox("work").await.expect("outbox handle"); + expect_invalid( + "run_once(empty worker_id)", + outbox.run_once("", &mut NoDelivery).await, + ); + // Tx paths — the twins validate identically (begin_tx route). let mut tx = store.begin_tx().await.expect("begin_tx opens"); expect_invalid( @@ -247,6 +300,10 @@ pub async fn name_validation_rejects_empty_and_reserved(factory: &dyn StoreFacto "read_from_consumer_tx(empty consumer)", tx.read_from_consumer_tx("work", "", 10).await, ); + expect_invalid( + "get_offset_tx(empty consumer)", + tx.get_offset_tx("work", "").await, + ); expect_invalid( "outbox_enqueue_tx(empty outbox)", tx.outbox_enqueue_tx("", Default::default(), json!(null)) diff --git a/alkstore-contract-suite/tests/suite_harness.rs b/alkstore-contract-suite/tests/suite_harness.rs index d356661..db8dd32 100644 --- a/alkstore-contract-suite/tests/suite_harness.rs +++ b/alkstore-contract-suite/tests/suite_harness.rs @@ -79,9 +79,11 @@ impl TxHandle for MockTx { fn get_offset_tx<'a>( &'a mut self, stream: &str, - _consumer: &str, + consumer: &str, ) -> BoxedFuture<'a, Result> { - let r = alkstore::validate_shared_name(stream).map(|_| 0); + let r = alkstore::validate_shared_name(stream) + .and_then(|_| alkstore::validate_local_name(consumer)) + .map(|_| 0); Box::pin(std::future::ready(r)) } fn read_since_tx<'a>( @@ -133,16 +135,18 @@ impl Queue for MockQueue { } fn claim_one<'a>( &'a self, - _worker_id: &str, + worker_id: &str, ) -> BoxedFuture<'a, Result>>> { - Box::pin(std::future::ready(Ok(None))) + let r = alkstore::validate_local_name(worker_id).map(|_| None); + Box::pin(std::future::ready(r)) } fn claim_batch<'a>( &'a self, - _worker_id: &str, + worker_id: &str, _n: i64, ) -> BoxedFuture<'a, Result>>> { - Box::pin(std::future::ready(Ok(Vec::new()))) + let r = alkstore::validate_local_name(worker_id).map(|_| Vec::new()); + Box::pin(std::future::ready(r)) } fn ack_batch<'a>(&'a self, _ids: &[i64]) -> BoxedFuture<'a, Result> { Box::pin(std::future::ready(Ok(0))) @@ -183,16 +187,18 @@ impl StreamHandle for MockStream { } fn read_from_consumer<'a>( &'a self, - _consumer: &str, + consumer: &str, _limit: i64, ) -> BoxedFuture<'a, Result>> { - Box::pin(std::future::ready(Ok(Vec::new()))) + let r = alkstore::validate_local_name(consumer).map(|_| Vec::new()); + Box::pin(std::future::ready(r)) } - fn save_offset<'a>(&'a self, _consumer: &str, _offset: i64) -> BoxedFuture<'a, Result<()>> { - Box::pin(std::future::ready(Ok(()))) + fn save_offset<'a>(&'a self, consumer: &str, _offset: i64) -> BoxedFuture<'a, Result<()>> { + Box::pin(std::future::ready(alkstore::validate_local_name(consumer))) } - fn get_offset<'a>(&'a self, _consumer: &str) -> BoxedFuture<'a, Result> { - Box::pin(std::future::ready(Ok(0))) + fn get_offset<'a>(&'a self, consumer: &str) -> BoxedFuture<'a, Result> { + let r = alkstore::validate_local_name(consumer).map(|_| 0); + Box::pin(std::future::ready(r)) } fn trim_to<'a>(&'a self, _horizon: i64) -> BoxedFuture<'a, Result> { Box::pin(std::future::ready(Ok(0))) @@ -287,8 +293,9 @@ impl Store for MockStore { }); Box::pin(std::future::ready(r)) } - fn unschedule<'a>(&'a self, _name: &str) -> BoxedFuture<'a, Result> { - Box::pin(std::future::ready(Ok(false))) + fn unschedule<'a>(&'a self, name: &str) -> BoxedFuture<'a, Result> { + let r = alkstore::validate_shared_name(name).map(|_| false); + Box::pin(std::future::ready(r)) } fn run_schedules<'a>(&'a self, _stop: StopToken) -> BoxedFuture<'a, Result<()>> { Box::pin(std::future::ready(Ok(()))) @@ -310,10 +317,11 @@ impl Outbox for MockOutbox { } fn run_once<'a>( &'a mut self, - _worker_id: &str, + worker_id: &str, _delivery: &'a mut dyn Delivery, ) -> BoxedFuture<'a, Result> { - Box::pin(std::future::ready(Ok(false))) + let r = alkstore::validate_local_name(worker_id).map(|_| false); + Box::pin(std::future::ready(r)) } } diff --git a/alkstore/src/outbox.rs b/alkstore/src/outbox.rs index 8c111fa..ff97c7c 100644 --- a/alkstore/src/outbox.rs +++ b/alkstore/src/outbox.rs @@ -47,7 +47,8 @@ pub trait Outbox: Send + Sync { /// Pull op: claim one job, run `delivery`, ack on `Ok`, /// `retry(err, None)` (the queue curve) on `Err`. The consumer /// calls it in its own loop. `true` = claimed and processed; - /// `false` = no work (a value, not an error). + /// `false` = no work (a value, not an error). `worker_id` is + /// consumer-local, non-empty only (`InvalidName`; ADR-019 §2). fn run_once<'a>( &'a mut self, worker_id: &str, diff --git a/alkstore/src/queue.rs b/alkstore/src/queue.rs index 4151062..d83a0c1 100644 --- a/alkstore/src/queue.rs +++ b/alkstore/src/queue.rs @@ -40,7 +40,8 @@ pub trait Queue: Send + Sync { ) -> BoxedFuture<'a, Result>>>; /// Claim up to `n` rows, same ordering and exclusivity as - /// [`Queue::claim_one`]. + /// [`Queue::claim_one`]. `worker_id` is consumer-local, non-empty + /// only (`InvalidName`). fn claim_batch<'a>( &'a self, worker_id: &str, diff --git a/alkstore/src/store.rs b/alkstore/src/store.rs index e36f8cc..8f9d915 100644 --- a/alkstore/src/store.rs +++ b/alkstore/src/store.rs @@ -96,7 +96,10 @@ pub trait Store: Send + Sync { ) -> BoxedFuture<'a, Result>; /// Remove a schedule registration. `true` = a row was removed; - /// `false` = no such schedule (a value, not an error). + /// `false` = no such schedule (a value, not an error). `name` is a + /// name-bearing entry point (ADR-009 §1) — validated like the + /// schedule name: non-empty (`InvalidName`), reserved-prefix + /// rejected (`ReservedName`). fn unschedule<'a>(&'a self, name: &str) -> BoxedFuture<'a, Result>; /// The opt-in scheduler runner: acquire the leadership lock (the diff --git a/alkstore/src/stream_handle.rs b/alkstore/src/stream_handle.rs index caf00bb..e13538f 100644 --- a/alkstore/src/stream_handle.rs +++ b/alkstore/src/stream_handle.rs @@ -45,7 +45,8 @@ pub trait StreamHandle: Send + Sync { ) -> BoxedFuture<'a, Result>>; /// Cursor read from a consumer's stored checkpoint (absent - /// consumer = 0). + /// consumer = 0). `consumer` is a consumer-local identifier + /// (non-empty only — `InvalidName`). fn read_from_consumer<'a>( &'a self, consumer: &str, @@ -55,11 +56,13 @@ pub trait StreamHandle: Send + Sync { /// Save a consumer's checkpoint — always explicit (no /// auto-checkpoint cadence). Monotone: a save below the stored /// checkpoint is a silent no-op (ADR-019 §6); the `Err` arm - /// carries `Database` only (ADR-021 §5). + /// carries `Database` only (ADR-021 §5). `consumer` is a + /// consumer-local identifier (non-empty only — `InvalidName`). fn save_offset<'a>(&'a self, consumer: &str, offset: i64) -> BoxedFuture<'a, Result<()>>; /// Inspect a consumer's stored checkpoint; absent consumer = 0 - /// (the pre-save state, ADR-019 §1). + /// (the pre-save state, ADR-019 §1). `consumer` is a consumer-local + /// identifier (non-empty only — `InvalidName`). fn get_offset<'a>(&'a self, consumer: &str) -> BoxedFuture<'a, Result>; /// Bounded growth, consumer-invoked: delete events with diff --git a/alkstore/src/tx.rs b/alkstore/src/tx.rs index e0a0fa7..9bf198d 100644 --- a/alkstore/src/tx.rs +++ b/alkstore/src/tx.rs @@ -97,7 +97,8 @@ pub trait TxHandle: Send { /// Read a consumer's stored checkpoint inside the caller's /// transaction; absent consumer = 0 (ADR-019 §1). Pure read. - /// Validates `stream`. + /// Validates `stream`; `consumer` is consumer-local (non-empty + /// only). fn get_offset_tx<'a>( &'a mut self, stream: &str, @@ -115,7 +116,8 @@ pub trait TxHandle: Send { ) -> BoxedFuture<'a, Result>>; /// Read stream events from a consumer's stored checkpoint, inside - /// the caller's transaction. Pure read. Validates `stream`. + /// the caller's transaction. Pure read. Validates `stream`; + /// `consumer` is consumer-local (non-empty only). fn read_from_consumer_tx<'a>( &'a mut self, stream: &str, diff --git a/docs/architecture/core-contract.md b/docs/architecture/core-contract.md index 371464a..08f6b99 100644 --- a/docs/architecture/core-contract.md +++ b/docs/architecture/core-contract.md @@ -544,7 +544,10 @@ by [ADR-009](decisions/009-scheduler-collapse.md) §1): - Reserved-prefix names are rejected at every entry point — the name-bearing methods *and their `*_tx` counterparts* — with the typed `ReservedName` error; empty names with `InvalidName` - ([ADR-008](decisions/008-contract-v1-pinning.md) §4). Stream-consumer + ([ADR-008](decisions/008-contract-v1-pinning.md) §4 — enforced as + non-empty-after-whitespace-exclusion per that ADR's wave-1 review + annotation: whitespace-only names are `InvalidName` on both name + classes; names are not trimmed). Stream-consumer names are consumer-local identifiers, not a reserved-namespace kind. - Engine-derived names in consumer namespaces carry the reserved prefix (the outbox's backing queue is derived under the prefix — diff --git a/docs/architecture/decisions/008-contract-v1-pinning.md b/docs/architecture/decisions/008-contract-v1-pinning.md index 1881373..9833731 100644 --- a/docs/architecture/decisions/008-contract-v1-pinning.md +++ b/docs/architecture/decisions/008-contract-v1-pinning.md @@ -247,9 +247,19 @@ struct Wake { channel: String } only** — an empty stream-consumer name or empty `try_lock` owner is `InvalidName`, mirroring `worker_id`'s rule ([ADR-019](019-mechanism-handle-surfaces.md) §2) — with no - reserved-prefix rejection (they are not a shared namespace, so a - leading `__alkstore_` is the caller's own local name, no engine - namespace to collide with).)* + reserved-prefix rejection (they are not a shared namespace, so a + leading `__alkstore_` is the caller's own local name, no engine + namespace to collide with). *(Annotated 2026-10-07, wave-1 review: + the stream-consumer-name class covers every method's consumer-name + argument — `save_offset(_tx)`, `subscribe`, `get_offset(_tx)`, + `read_from_consumer(_tx)`, the receiver forms — the class is by + identifier role, not per-method enumeration. The non-empty rule is + enforced as **non-empty after whitespace exclusion** — a + whitespace-only name (`" "`, `"\t\n"`) is `InvalidName` on both + name classes, never a meaningful name on any engine; names are not + trimmed — the caller's name is stored as given, engine quoting is + the engine's obligation. Pre-release class-1 annotations per + [ADR-017](017-contract-versioning.md) §2.)* - Per-engine internal names: - SQLite: honker's machinery owns two categories of internal names, and the contract treats them differently. Its `_honker_*` *table* diff --git a/tasks/review-wave-1.md b/tasks/review-wave-1.md index d7cae5c..a183fa8 100644 --- a/tasks/review-wave-1.md +++ b/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 \ No newline at end of file +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. \ No newline at end of file