3B1: backend-neutral curation-store domain model - #137
Conversation
…le (3B1) The backend-neutral core for human curation decisions: a versioned wire format, validation, per-chunk projection, and reconciliation against a corpus. Pure — no I/O, no id generation; a backend owns those. A decision is an append-only event keyed by a stable ChunkId, latest by (occurred_at, event_id); cluster context is an audit snapshot, never identity. Nineteen tests pin the contract, including the named "malformed store is not an empty store" — a decode failure must never read as "no decisions". Also: explicit-version refusal, duplicate/empty-id and bad-timestamp rejection, unpinned- chunk refusal (no filename fallback), order-independent corpus/chunk fingerprints that ignore curator-mutable fields, latest-by-timestamp-then-id projection, and reconciliation where a changed chunk orphans but a regrouped cluster does not. The functions are stubs (empty/error), so fourteen fail; the types and wire shape compile. SwancoreTag gains Ord for canonical tag ordering. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM
…rmat (3B1) Fill the 3B1 domain model's remaining behavior so all 19 tests pass: - project(): latest decision per chunk by (occurred_at, event_id), order- independent, cluster context excluded from projected identity. - reconcile(): an event stays active when its chunk is present with an unchanged pinned fingerprint — even if the shared corpus fingerprint moved or the cluster regrouped; orphaned (with reason) when the chunk is gone or its material changed. No fuzzy remap. corpus_match reports the shared fingerprint comparison independently. serde_json moves from dev- to a normal core dependency: the versioned encode/decode now lives in lib, not just tests. It is pure and std-only, so the backend-neutral, zero-I/O contract holds (no fs, no wasm-hostile deps). Tag canonicalization is deterministic by the SwancoreTag enum's declaration order (not alphabetical); the test asserts that order and the dedup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a public, versioned curation store with typed events, deterministic fingerprints and JSON encoding, validation, latest-decision projection, and corpus reconciliation with orphan classification. ChangesCuration store
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/src/curation_store.rs`:
- Around line 238-273: Update the store decoding validation around
CurationStoreV1 to add a store-level empty-field error variant and reject an
empty store.corpus_fingerprint before iterating over store.events. Keep the
existing event-level validation and errors unchanged, and report the
corpus_fingerprint field through the new store-level error.
- Around line 285-310: Update valid_timestamp and the event
ordering/canonicalization comparison to parse UTC timestamps into validated
datetime values, rejecting impossible calendar or clock fields and normalizing
optional fractional seconds before comparison. Ensure fractional timestamps sort
by their actual instants rather than raw strings, and add regression tests
covering fractional-second ordering and invalid dates/times.
- Around line 211-276: 添加针对 encode_store 和 decode_store 的有界模糊测试，覆盖任意畸形 JSON 解码不
panic、有效 CurationStoreV1 的编码解码往返，以及 canonical 编码的幂等性（重复编码结果一致）。将其纳入现有 CI
测试流程，保持测试规模有界并复用现有构造器或随机数据生成工具。
- Around line 214-219: Update encode_store to set canonical.version to the
version-1 value after cloning and before canonicalize/serialization, ensuring
caller-provided versions cannot produce invalid wire data while preserving the
existing encoding flow.
- Around line 176-186: Validate chunk.source.sha256 in the fingerprinting path
before constructing the canonical string: accept only non-empty, exactly
64-character lowercase hexadecimal values and treat malformed pins like missing
pins via unpinned. Update the test fixture’s “hashof_” value to a valid
lowercase SHA-256 hex string, keeping valid pin fingerprinting unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c43ba649-fb4c-4a69-9e45-328f028fb544
📒 Files selected for processing (4)
core/Cargo.tomlcore/src/corpus.rscore/src/curation_store.rscore/src/lib.rs
| let sha = chunk.source.sha256.as_deref().ok_or_else(unpinned)?; | ||
| let track = chunk.source.track_index.ok_or_else(unpinned)?; | ||
| let bars = match chunk.source.bar_range { | ||
| Some((first, last)) => format!("{first}-{last}"), | ||
| None => "none".to_owned(), | ||
| }; | ||
| let canonical = format!( | ||
| "chunk-v9\nid={}\nsha256={sha}\ntrack={track}\nbars={bars}", | ||
| chunk.id.0 | ||
| ); | ||
| Ok(ChunkFingerprint(source_sha256(canonical.as_bytes()))) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject malformed SHA-256 pins before fingerprinting.
Some("") and other non-64-character lowercase-hex values currently count as pinned. Such chunks will not orphan when their underlying source changes, defeating the material-identity guarantee documented by SourceRef.
Proposed validation
let sha = chunk.source.sha256.as_deref().ok_or_else(unpinned)?;
+if sha.len() != 64
+ || !sha
+ .bytes()
+ .all(|byte| matches!(byte, b'0'..=b'9' | b'a'..=b'f'))
+{
+ return Err(unpinned());
+}Update the test fixture’s "hashof_" value to valid lowercase SHA-256 hex.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let sha = chunk.source.sha256.as_deref().ok_or_else(unpinned)?; | |
| let track = chunk.source.track_index.ok_or_else(unpinned)?; | |
| let bars = match chunk.source.bar_range { | |
| Some((first, last)) => format!("{first}-{last}"), | |
| None => "none".to_owned(), | |
| }; | |
| let canonical = format!( | |
| "chunk-v9\nid={}\nsha256={sha}\ntrack={track}\nbars={bars}", | |
| chunk.id.0 | |
| ); | |
| Ok(ChunkFingerprint(source_sha256(canonical.as_bytes()))) | |
| let sha = chunk.source.sha256.as_deref().ok_or_else(unpinned)?; | |
| if sha.len() != 64 | |
| || !sha | |
| .bytes() | |
| .all(|byte| matches!(byte, b'0'..=b'9' | b'a'..=b'f')) | |
| { | |
| return Err(unpinned()); | |
| } | |
| let track = chunk.source.track_index.ok_or_else(unpinned)?; | |
| let bars = match chunk.source.bar_range { | |
| Some((first, last)) => format!("{first}-{last}"), | |
| None => "none".to_owned(), | |
| }; | |
| let canonical = format!( | |
| "chunk-v9\nid={}\nsha256={sha}\ntrack={track}\nbars={bars}", | |
| chunk.id.0 | |
| ); | |
| Ok(ChunkFingerprint(source_sha256(canonical.as_bytes()))) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/src/curation_store.rs` around lines 176 - 186, Validate
chunk.source.sha256 in the fingerprinting path before constructing the canonical
string: accept only non-empty, exactly 64-character lowercase hexadecimal values
and treat malformed pins like missing pins via unpinned. Update the test
fixture’s “hashof_” value to a valid lowercase SHA-256 hex string, keeping valid
pin fingerprinting unchanged.
| pub fn encode_store(store: &CurationStoreV1) -> Vec<u8> { | ||
| let mut canonical = store.clone(); | ||
| canonicalize(&mut canonical); | ||
| // Our types are plain structs/enums of strings, numbers, and vecs, so | ||
| // serialization is infallible; the default is unreachable. | ||
| serde_json::to_vec(&canonical).unwrap_or_default() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ensure encode_store always emits version 1.
Because CurationStoreV1.version is public, callers can pass another value and receive bytes that decode_store immediately rejects. Enforce the type’s wire invariant during encoding.
Proposed fix
let mut canonical = store.clone();
+canonical.version = CURATION_STORE_VERSION;
canonicalize(&mut canonical);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn encode_store(store: &CurationStoreV1) -> Vec<u8> { | |
| let mut canonical = store.clone(); | |
| canonicalize(&mut canonical); | |
| // Our types are plain structs/enums of strings, numbers, and vecs, so | |
| // serialization is infallible; the default is unreachable. | |
| serde_json::to_vec(&canonical).unwrap_or_default() | |
| pub fn encode_store(store: &CurationStoreV1) -> Vec<u8> { | |
| let mut canonical = store.clone(); | |
| canonical.version = CURATION_STORE_VERSION; | |
| canonicalize(&mut canonical); | |
| // Our types are plain structs/enums of strings, numbers, and vecs, so | |
| // serialization is infallible; the default is unreachable. | |
| serde_json::to_vec(&canonical).unwrap_or_default() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/src/curation_store.rs` around lines 214 - 219, Update encode_store to
set canonical.version to the version-1 value after cloning and before
canonicalize/serialization, ensuring caller-provided versions cannot produce
invalid wire data while preserving the existing encoding flow.
|
Blocking review for HEAD 3B1 is on the right architectural boundary, and keeping
Also validate/decide the semantics of CI is green on this HEAD, so these are domain-review findings rather than test failures. #137 remains HOLD; 3B2 and 3C stay frozen. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 266f750c76
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut latest: BTreeMap<&ChunkId, &CurationEvent> = BTreeMap::new(); | ||
| for event in events { | ||
| let wins = latest.get(&event.chunk_id).is_none_or(|current| { | ||
| (&event.occurred_at, &event.event_id) > (¤t.occurred_at, ¤t.event_id) |
There was a problem hiding this comment.
When stores mix accepted timestamp precisions, e.g. a native event 2026-07-18T10:00:00Z and an OPFS/JS event 2026-07-18T10:00:00.001Z, this raw string comparison treats the whole-second value as later because Z sorts after ., so project can surface an older decision for the chunk. Since valid_timestamp accepts both forms, normalize timestamps to one sortable representation or compare parsed instants before applying the (occurred_at, event_id) tie-break.
Useful? React with 👍 / 👎.
| datetime.len() == mask.len() | ||
| && datetime.chars().zip(mask.chars()).all(|(c, m)| match m { | ||
| 'd' => c.is_ascii_digit(), | ||
| lit => c == lit, | ||
| }) |
There was a problem hiding this comment.
When decoding untrusted store bytes, values like 2026-99-99T99:99:99Z pass this check because it only verifies digit positions. The module documents occurred_at as a valid UTC timestamp and returns InvalidTimestamp, so malformed dates can enter canonical stores and then drive projection/reconciliation ordering; add range/date validation before accepting the field.
Useful? React with 👍 / 👎.
…ange pin (3B1) Corrective for the PR #137 review. Three blockers, each with failing tests: 1. Timestamps ordered lexicographically but the validator accepted variable- width fractional seconds and any digit garbage (2026-99-88T77:66:55Z), so string order did not equal wall-clock order. New tests: fractional seconds (.500/.1/.100) rejected; every out-of-range field rejected; impossible civil days (Feb 30, Feb 29 in a common year) rejected while a real leap day survives; projection picks the chronologically latest event. 2. encode_store could serialize an invalid store (duplicate ids, empty fields, bad timestamp, wrong version, empty envelope fingerprint) and even swallow a serialize error into empty bytes. New tests: encode refuses each rule and never yields bytes for an invalid store. 3. bar_range: None silently hashed as "none". New test: a chunk without a bar_range is a typed UnpinnedChunk, not a whole-track identity — all 9909 corpus chunks carry one, so a missing range is an incomplete pin. Scaffolding to make the target API compile: a shared StoreValidationError used by both directions; StoreDecodeError::Invalid and a new StoreEncodeError; encode_store now returns Result. Implementation is still the old behavior, so the six new tests fail. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM
…range pin (3B1) Resolve the three PR #137 blockers; all 25 curation_store tests pass. 1. Timestamps. valid_timestamp now accepts only the canonical YYYY-MM-DDTHH:MM:SSZ (second precision, no fractional part) with every field in its real civil-calendar range (leap-year-aware day count). Fixed width and zero-padded means byte-lexicographic order equals chronological order, so project()'s String comparison is a correct clock. Fractional seconds are rejected, not normalized, because their variable width is exactly what broke the ordering. 2. encode_store validates the canonicalized store with the same rules as decode before serializing and maps a serialize failure to StoreEncodeError::Serialize — unwrap_or_default() is gone, so a write can never persist invalid state nor swallow an error into an empty "no decisions" file. 3. chunk_fingerprint requires bar_range like sha256 and track_index; a missing range is UnpinnedChunk, not a "none" whole-track identity. days_in_month is a const fn using is_multiple_of for the Gregorian leap rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NkqJUU6d1sW1RAfvyHrqVM
Corrective for the three blockers (RED
|
1 participant
What
The pure domain model for human curation decisions — step 3B1 of the
dedup-aware curation bootstrap. Types, a versioned wire format, validation, a
per-chunk projection, and reconciliation against a corpus. Zero I/O: no
std::fs, no temp files, no rename/replace, no OPFS, no UUID minting. A backend(3B2 native, 3C OPFS) owns bytes and id generation; this module owns the format
and its rules.
New module
core/src/curation_store.rs.Model
A decision is an append-only event, keyed by a stable
ChunkId— never aposition, cluster, or representative. The latest decision for a chunk is the
event with the greatest
(occurred_at, event_id). Cluster context rides alongas an audit snapshot on the event; it is not part of the decision's
identity, so correcting dedup provenance (a representative moving) never moves
or drops a human decision.
chunk_fingerprint—ChunkId+sha256+track_index+bar_range,hashed. A chunk missing
sha256/track_indexis a typedFingerprintError::UnpinnedChunk— a filename is not an identity.corpus_fingerprint— schema version + the sorted set of chunkfingerprints, hashed. Independent of chunk order and of every curator-mutable
field (tags, reviewer, timestamps).
encode_store/decode_store— one canonical byte form (tags sorted +deduped, events ordered by
(occurred_at, event_id)). Decode reads theexplicit
versionfirst (unknown → typedUnsupportedVersion, never aguess), then validates empty opaque fields, timestamps, and duplicate ids.
Malformed bytes are a typed error, never an empty store.
project— latest decision per chunk, order-independent; cluster contextexcluded from projected identity.
reconcile— an event stays active when its chunk is present with anunchanged pinned fingerprint, even if the shared corpus fingerprint moved or
the cluster regrouped; orphaned (with reason: missing vs. changed
material) otherwise. No fuzzy remap.
corpus_matchreports the shared-fingerprint comparison independently.
Tests
19 tests (RED→GREEN in separate commits), covering the full acceptance
contract: unknown-version refusal, malformed≠empty, duplicate-id, empty-field,
invalid-timestamp, unpinned-chunk, order-independence of both fingerprints,
curator-mutable-field invariance, timestamp+event_id tie-break, projection
order-independence, corpus-expansion-keeps-active, deleted/changed orphaning,
cluster-context-does-not-orphan, deterministic tag canonicalization, and a
decode→encode single-canonical-form round-trip.
Notes
serde_jsonmoves from a dev- to a normalgriff-coredependency: theversioned encode/decode now lives in lib, not only tests. Pure and std-only,
so the backend-neutral / wasm-safe contract holds.
SwancoreTagenum's declaration order(deterministic), not alphabetical.
ChunkMeta.reviewer, cockpit, or CLI. 3B2(native FS atomic adapter) and 3C (OPFS + cockpit flow) remain frozen.
Validation
cargo test -p griff-core --lib— 269/269 pass (19 new).cargo clippy --workspace --all-targets— clean (full pedantic/nursery/restriction wall).
cargo fmt --check— clean.griff-cli::missing_file_golden(asserts anEnglish OS string on a Russian-locale box) is unrelated to this change.
🤖 Generated with Claude Code
Summary by CodeRabbit