WarpTickPatchV1 delta patches + commit hash v2 #150
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a replayable delta patch type (WarpTickPatchV1), integrates deterministic patch generation into the commit flow, exposes patch metadata in Snapshots (patch_digest), and migrates commit hashing to a v2 header that commits to patch_digest + parents + state_root + policy_id. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Engine as Engine
participant Store as GraphStore
participant Patch as WarpTickPatchV1
participant Snapshot as Snapshot
App->>Engine: commit_with_receipt(tx_id)
rect rgba(240,248,255,0.9)
Note over Engine,Store: 1) Reserve & capture pre-state
Engine->>Store: store_before = snapshot()
Engine->>Engine: reserve_for_receipt(tx) -> ReserveOutcome
end
rect rgba(255,245,230,0.9)
Note over Engine,Store: 2) Apply rewrites → post-state
Engine->>Store: apply_rewrites(reserved.rewrites)
Store-->>Engine: store_after
end
rect rgba(240,255,240,0.9)
Note over Engine,Patch: 3) Generate delta patch
Engine->>Patch: diff_store(store_before, store_after)
Patch->>Patch: canonicalize(in_slots,out_slots,ops)
Patch->>Patch: compute_patch_digest_v1()
Patch-->>Engine: WarpTickPatchV1{digest}
end
rect rgba(255,240,245,0.9)
Note over Engine,Snapshot: 4) Snapshot v2 compute
Engine->>Engine: compute_rule_pack_id()
Engine->>Snapshot: build(snapshot with patch_digest)
Engine->>Engine: compute_commit_hash_v2(state_root, parents, patch_digest, policy_id)
end
Engine-->>App: (Snapshot, TickReceipt, WarpTickPatchV1)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ 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: 6
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
crates/warp-core/src/engine_impl.rscrates/warp-core/src/lib.rscrates/warp-core/src/snapshot.rscrates/warp-core/src/tick_patch.rscrates/warp-core/tests/tick_receipt_tests.rsdocs/decision-log.mddocs/docs-index.mddocs/execution-plan.mddocs/spec-merkle-commit.mddocs/spec-warp-tick-patch.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every public API across crates (warp-core, warp-ffi, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage. Treat missing docs as a failing test.
Runcargo clippy --all-targets -- -D missing_docsbefore every PR; CI will expect a zero-warning, fully documented surface.
Every source file must start with exactly:// SPDX-License-Identifier: Apache-2.0and// © James Ross Ω FLYING•ROBOTS <https://github.com/flyingrobots>
Files:
crates/warp-core/tests/tick_receipt_tests.rscrates/warp-core/src/lib.rscrates/warp-core/src/snapshot.rscrates/warp-core/src/tick_patch.rscrates/warp-core/src/engine_impl.rs
🧠 Learnings (5)
📚 Learning: 2025-12-28T23:14:28.083Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.083Z
Learning: Start each session by updating *Today's Intent* in `docs/execution-plan.md` and capture milestones, blockers, and decisions in the Decision Log.
Applied to files:
docs/execution-plan.md
📚 Learning: 2025-12-28T23:14:28.083Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.083Z
Learning: When a PR touches non-doc code, update `docs/execution-plan.md` and `docs/decision-log.md` in the same PR.
Applied to files:
docs/execution-plan.md
📚 Learning: 2025-12-28T23:14:28.083Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.083Z
Learning: Applies to **/*.rs : Every public API across crates (warp-core, warp-ffi, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage. Treat missing docs as a failing test.
Applied to files:
crates/warp-core/src/lib.rs
📚 Learning: 2025-12-28T23:14:28.083Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.083Z
Learning: Every meaningful design choice should land in `docs/` (specs, diagrams, memorials) or other durable repo artifacts (e.g. `docs/decision-log.md`).
Applied to files:
docs/decision-log.md
📚 Learning: 2025-12-28T23:14:28.083Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.083Z
Learning: Think in timelines—branching, merging, entropy budgets—and map feature work to Chronos/Kairos/Aion axes where appropriate.
Applied to files:
docs/decision-log.md
🧬 Code graph analysis (3)
crates/warp-core/tests/tick_receipt_tests.rs (3)
crates/warp-core/src/engine_impl.rs (1)
snapshot(327-368)crates/warp-core/src/receipt.rs (2)
tx(42-44)digest(62-64)crates/warp-core/src/tick_patch.rs (1)
digest(248-250)
crates/warp-core/src/snapshot.rs (1)
crates/warp-core/src/tick_patch.rs (2)
policy_id(209-211)new(175-205)
crates/warp-core/src/engine_impl.rs (2)
crates/warp-core/src/snapshot.rs (2)
compute_commit_hash_v2(192-211)compute_state_root(154-156)crates/warp-core/src/tick_patch.rs (8)
diff_store(408-470)policy_id(209-211)rule_pack_id(218-220)in_slots(230-232)new(175-205)out_slots(236-238)ops(242-244)digest(248-250)
🔇 Additional comments (26)
crates/warp-core/src/lib.rs (1)
93-94: LGTM!The public re-exports follow the established pattern in this file, and the doc comment succinctly identifies these as Paper III boundary artifacts. The detailed rustdoc should live on the individual types in
tick_patch.rs.docs/docs-index.md (1)
24-24: LGTM!Index entry correctly placed and accurately describes the new spec document.
docs/decision-log.md (1)
9-9: LGTM!The decision log entry comprehensively captures the Paper III boundary semantics shift: delta patches over recipes, unversioned slots with worldline-derived versioning, and the v2 commit hash committing only to
patch_digest. This provides durable rationale for future maintainers. Based on learnings, every meaningful design choice should land indocs/decision-log.md.crates/warp-core/tests/tick_receipt_tests.rs (2)
92-92: LGTM!The destructuring correctly accommodates the new three-tuple return from
commit_with_receipt. The patch value is properly captured for the subsequent digest assertion.
109-113: LGTM!Excellent coverage: this assertion locks the invariant that
Snapshot.patch_digestmust equalWarpTickPatchV1.digest(). The assertion message clearly documents the v2 commit hash semantics.docs/spec-warp-tick-patch.md (1)
1-166: Solid spec overall.The document clearly defines the tick patch boundary artifact, canonical ordering, and encoding rules. The slicing semantics section (lines 152-166) succinctly captures the Paper III slice extraction algorithm with the critical conservatism requirement. This provides a durable reference for the deterministic replay contract.
docs/execution-plan.md (1)
38-44: LGTM!Today's Intent section is comprehensive: goal, scope, exit criteria, and evidence are all documented. This provides a clear audit trail for the delta tick patches work. Based on learnings, this satisfies the requirement to update
docs/execution-plan.mdwhen a PR touches non-doc code.crates/warp-core/src/snapshot.rs (3)
62-67: LGTM!The
patch_digestfield is properly documented with rustdoc explaining its role in commit hash v2 and the diagnostic nature of other digests. This satisfies the coding guideline requiring rustdoc on all public API fields.
158-186: LGTM on the v1 retention.Renaming to
compute_commit_hash_v1and marking#[allow(dead_code)]is the correct approach for preserving migration tooling capabilities. The comment on lines 159-161 explains the rationale. Future archival data re-computation can use this function.
188-211: LGTM!The v2 commit hash implementation correctly encodes:
- Version tag
2u16(line 200)- Length-prefixed parents (lines 202-205)
state_root+patch_digest+policy_id(lines 207-209)This matches the spec in
docs/spec-merkle-commit.mdsection 2 exactly. The function is appropriately scoped aspub(crate).crates/warp-core/src/tick_patch.rs (7)
1-14: LGTM!License header and module-level documentation are correctly formatted and comprehensive. The Paper III context and V1 semantics are well-documented.
22-38: LGTM!Public API is properly documented. The
const fn code()discriminant mapping is clean, and starting codes at 1 provides safe disambiguation from uninitialized state.
82-143: LGTM!Canonical ordering (deletes before upserts) is semantically correct for deterministic patch application. Documentation on the public enum and all variants meets the rustdoc requirement.
168-205: LGTM!Constructor correctly canonicalizes inputs (sort + dedup) before computing digest. The
#[must_use]annotation and comprehensive rustdoc meet API documentation requirements.
293-317: LGTM!Error type is properly documented with clear variant descriptions.
DisplayandErrorimplementations are correct.
319-338: LGTM!Digest computation uses versioned format (1u16 tag), little-endian length prefixes, and deterministic field ordering. This provides forward compatibility and stable hashing.
407-470: LGTM!
diff_storecorrectly handles node creation/deletion/modification and edge migration (from-node changes). The finalsort_by_keyensures canonical op ordering regardless of iteration order from the underlying maps.crates/warp-core/src/engine_impl.rs (9)
10-16: LGTM!Import changes correctly align with the new v2 commit hash and tick patch integration.
195-210: LGTM!The return type change to include
WarpTickPatchV1is a breaking change but correctly documented in the method's rustdoc. The Paper III reference provides context for the semantic shift.
230-251: LGTM!Using
BTreeSetfor slot collection ensures deterministic ordering. The footprint-to-slot mapping correctly captures conservative read/write sets.
266-290: Full store clone for pre-state capture is expensive but correct for v0.Line 267's
self.store.clone()is O(store_size) in memory and time. For correctness-first v0, this is acceptable. Future optimizations could use COW semantics or incremental diffing.
301-322: LGTM!The v2 commit hash correctly commits to
(parents, state_root, patch_digest, policy_id), matching the spec. Snapshot includespatch_digestfor external verification against the returned patch.
390-403: LGTM!
compute_rule_pack_idproduces a stable, version-tagged hash of registered rule IDs. Sorting and deduping ensures determinism regardless of registration order.
413-441: LGTM!Footprint-to-slot mapping correctly implements conservative slot derivation: write slots are included in both
in_slotsandout_slots(per Paper III over-approximation semantics), while read-only slots appear only inin_slots.
486-552: LGTM!Comprehensive test verifying:
- Patch replay produces identical state root
- Snapshot's
patch_digestmatches the patch's computed digest- Commit hash v2 formula is correctly applied
- Conservative slot tracking includes the scoped entity
This provides strong coverage for the replayability invariant.
345-356: Creating a fullWarpTickPatchV1just for an empty digest is slightly wasteful.Consider pre-computing the empty patch digest as a constant (similar to
DIGEST_LEN0_U64on line 344) to avoid constructing and hashing an empty patch on everysnapshot()call.🔎 Suggested optimization
// In constants.rs or similar: pub static EMPTY_PATCH_DIGEST: Lazy<Hash> = Lazy::new(|| { WarpTickPatchV1::new(0, [0u8; 32], TickCommitStatus::Committed, vec![], vec![], vec![]).digest() });Then in
snapshot():- let patch_digest = WarpTickPatchV1::new( - policy_id, - rule_pack_id, - TickCommitStatus::Committed, - Vec::new(), - Vec::new(), - Vec::new(), - ) - .digest(); + // Empty patch has deterministic digest for any policy_id/rule_pack_id. + let patch_digest = *crate::constants::EMPTY_PATCH_DIGEST;Note: Only valid if the empty patch digest is truly independent of
policy_idandrule_pack_id. If not, the current approach is correct.Wait—the digest includes
policy_idandrule_pack_id, so it's NOT constant across different engine configurations. The current implementation is correct. Disregard the optimization suggestion; the construction is necessary.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
docs/spec-warp-tick-patch.md (1)
41-44: Field naming inconsistency persists.Line 44 defines
DeleteEdge { from: NodeId, id: EdgeId }but line 136 encodes it asedge_id: 32. This drift between data model and encoding will confuse implementers. Pick one name (edge_idpreferred for consistency withUpsertEdge).crates/warp-core/src/tick_patch.rs (2)
67-81: LGTM!
Ordimplementation is correct. Theunreachable!()at line 78 was added per the past review, providing a clear invariant assertion rather than a silent fallback.
269-281: O(total_edges) scan acknowledged—carry on.The
remove_edge_by_idcall is O(total_edges), and the inline NOTE correctly flags this as a v0 correctness trade-off. No action needed here; this matches the past review discussion.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
crates/warp-core/src/constants.rscrates/warp-core/src/engine_impl.rscrates/warp-core/src/lib.rscrates/warp-core/src/tick_patch.rscrates/warp-core/tests/tick_receipt_tests.rsdocs/decision-log.mddocs/execution-plan.mddocs/spec-merkle-commit.mddocs/spec-warp-tick-patch.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every public API across crates (warp-core, warp-ffi, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage. Treat missing docs as a failing test.
Runcargo clippy --all-targets -- -D missing_docsbefore every PR; CI will expect a zero-warning, fully documented surface.
Every source file must start with exactly:// SPDX-License-Identifier: Apache-2.0and// © James Ross Ω FLYING•ROBOTS <https://github.com/flyingrobots>
Files:
crates/warp-core/src/constants.rscrates/warp-core/src/lib.rscrates/warp-core/tests/tick_receipt_tests.rscrates/warp-core/src/engine_impl.rscrates/warp-core/src/tick_patch.rs
🧠 Learnings (4)
📚 Learning: 2025-12-28T23:14:28.083Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.083Z
Learning: Start each session by updating *Today's Intent* in `docs/execution-plan.md` and capture milestones, blockers, and decisions in the Decision Log.
Applied to files:
docs/execution-plan.md
📚 Learning: 2025-12-28T23:14:28.083Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.083Z
Learning: When a PR touches non-doc code, update `docs/execution-plan.md` and `docs/decision-log.md` in the same PR.
Applied to files:
docs/execution-plan.mddocs/decision-log.md
📚 Learning: 2025-12-28T23:14:28.083Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.083Z
Learning: Applies to **/*.rs : Every public API across crates (warp-core, warp-ffi, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage. Treat missing docs as a failing test.
Applied to files:
crates/warp-core/src/lib.rscrates/warp-core/src/engine_impl.rs
📚 Learning: 2025-12-28T23:14:28.083Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.083Z
Learning: Every meaningful design choice should land in `docs/` (specs, diagrams, memorials) or other durable repo artifacts (e.g. `docs/decision-log.md`).
Applied to files:
docs/decision-log.md
🧬 Code graph analysis (2)
crates/warp-core/tests/tick_receipt_tests.rs (3)
crates/warp-core/src/engine_impl.rs (1)
scope_hash(522-527)crates/warp-core/src/receipt.rs (1)
digest(102-104)crates/warp-core/src/tick_patch.rs (1)
digest(249-251)
crates/warp-core/src/engine_impl.rs (3)
crates/warp-core/src/snapshot.rs (2)
compute_commit_hash_v2(192-211)compute_state_root(154-156)crates/warp-core/src/tick_patch.rs (8)
diff_store(413-475)policy_id(210-212)rule_pack_id(219-221)ops(243-245)new(176-206)in_slots(231-233)out_slots(237-239)digest(249-251)crates/warp-core/src/telemetry.rs (1)
reserved(73-75)
🪛 markdownlint-cli2 (0.18.1)
docs/execution-plan.md
45-45: Table column count
Expected: 5; Actual: 4; Too few cells, row will be missing data
(MD056, table-column-count)
46-46: Table column count
Expected: 5; Actual: 4; Too few cells, row will be missing data
(MD056, table-column-count)
🔇 Additional comments (21)
crates/warp-core/src/constants.rs (1)
8-18: LGTM! Exemplary documentation.The rustdoc thoroughly explains intent, encoding semantics (ASCII
"NOP0"as little-endianu32), and links to the tracking issue for future parameterization. This is exactly how public constants should be documented.crates/warp-core/src/lib.rs (1)
55-56: LGTM! Public surface properly documented and organized.New module and re-exports are annotated with rustdoc comments explaining their Paper III context. The
scope_hashfunction and tick-patch boundary artifacts (SlotId,TickCommitStatus,TickPatchError,WarpOp,WarpTickPatchV1) are now part of the stable public API.Also applies to: 60-60, 63-64, 93-94
docs/execution-plan.md (1)
45-50: LGTM! Execution plan updated with delta-patch milestone.The entry properly documents the completed delta tick patch work with clear Goal, Scope, Exit criteria, and Evidence sections. The static analysis warnings (MD056) are false positives—these lines are blockquotes, not table rows.
docs/spec-warp-tick-patch.md (1)
1-10: LGTM! Solid spec foundation for Paper III boundary artifacts.The document cleanly defines the prescriptive vs. descriptive artifact distinction, locks down unversioned slot semantics with worldline interpretation for value versioning, and commits only the replayable delta into
commit_idv2.crates/warp-core/tests/tick_receipt_tests.rs (2)
161-161: LGTM! Tests properly validate the v2 commit hash contract.The three-tuple destructuring and the new assertion
snapshot.patch_digest == patch.digest()directly validate that the canonical patch digest is correctly propagated into the snapshot and committed into commit hash v2.Also applies to: 187-191
7-9: Good:scope_hashnow consumed from public API.The test correctly imports
scope_hashfrom the publicwarp_coresurface rather than duplicating the implementation locally. This validates the export and keeps test behavior aligned with the engine.docs/decision-log.md (1)
9-10: LGTM! Decision log captures the architectural pivot.The entry precisely documents the Paper III alignment: delta patches over recipes, unversioned slots with worldline interpretation, and commit hash v2 scoping. The rationale (cross-language archival, executor decoupling) and consequence (slicing becomes trivial) columns are particularly well-articulated.
docs/spec-merkle-commit.md (2)
34-40: LGTM! V2 header structure is precise and actionable.The spec correctly defines the v2 commit hash inputs:
version: u16 = 2, parents,state_root,patch_digest, andpolicy_id. The cross-reference todocs/spec-warp-tick-patch.mdfor canonical patch encoding keeps the spec DRY.Also applies to: 44-49
53-56: LGTM! Diagnostic digest scope is explicit.Clearly marking
plan_digest,decision_digest, andrewrites_digestas not committed intocommit_idv2 prevents future confusion. This aligns with the Paper III design where only the replayable delta participates in consensus.crates/warp-core/src/engine_impl.rs (7)
239-240: Verify: Full store clone for delta computation.Cloning
self.storecaptures the entire graph before applying rewrites. This is O(n) space and acceptable for correctness-first v0, but should be profiled if tick sizes grow large.
222-224: Good:policy_idnow uses the documented constant with a tracking TODO.The prior review requested a tracking comment for the hardcoded
policy_id. This is now addressed by usingPOLICY_ID_NO_POLICY_V0and the inlineTODO(#151)links to the parameterization issue.
514-527: LGTM!scope_hashis now a public API with full rustdoc.The function documents its stable definition (
blake3(rule_id || scope_node_id)) and explains its role in deterministic scheduler ordering. This satisfies the coding guideline requiring rustdoc on all public APIs.
529-557: Verify: Conservative slot derivation semantics.The implementation includes written slots in both
in_slotsandout_slots. This is intentionally conservative (over-approximation allowed per spec) but worth validating:
n_write→ bothin_slotsandout_slots✓e_write→ bothin_slotsandout_slots✓b_out→ bothin_slotsandout_slots✓Confirm this aligns with the spec's conservatism requirement: "Under-approximate
in_slotsis forbidden."
602-668: LGTM! Patch replay test validates the full v2 contract.The test:
- Captures pre-state before commit
- Replays the patch against pre-state
- Asserts
state_rootmatches post-state- Verifies
snapshot.patch_digest == patch.digest()- Verifies commit hash v2 composition
- Checks slot propagation (
in_slots/out_slotscontain the scoped node)This is comprehensive coverage for the Paper III boundary artifact semantics.
73-78: Private structReserveOutcomeis appropriately undocumented.Internal-only types don't require rustdoc per the guidelines (which target public APIs). The struct cleanly bundles the reservation phase outputs.
289-356: Good extraction: Reservation and application phases now have clear boundaries.
reserve_for_receipthandles the O(n²) blocker attribution and slot collection;apply_reserved_rewriteshandles the mutation. This separation makes the commit flow easier to reason about and test independently.Also applies to: 358-373
crates/warp-core/src/tick_patch.rs (5)
1-20: LGTM!License header, copyright, and module-level documentation are all present and correctly formatted per the coding guidelines. Imports are minimal and precise.
22-38: LGTM!Enum is concise, well-documented, and the
code()encoding reserves zero for potential future use as a sentinel. No issues.
119-144: LGTM!Canonical ordering (deletes before upserts, edges before nodes within category) is correct for idempotent replay. The zero-filled placeholder for single-key operations maintains consistent key width without affecting sort semantics.
457-470: Edge migration logic is correct.When an edge's
fromchanges, the code emitsDeleteEdgefrom the old bucket followed byUpsertEdgeto the new bucket. When only non-fromfields change, onlyUpsertEdgeis emitted (in-place overwrite). This correctly handles edge bucket migration.
83-110: The suggested PartialEq derive onWarpOpis not feasible and contradicts the actual codebase constraints.
WarpOpcannot derivePartialEqbecause bothNodeRecordandEdgeRecordcontainOption<Bytes>fields, and thebytescrate'sBytestype deliberately does not implementPartialEq(it is reference-counted). Consequently, neither record type can derivePartialEq, making the enum unable to derive it either. The suggested diff would not compile. The caveat in the original review ("If the inner types lackPartialEq, this is a non-issue") applies here but was not verified before making the suggestion. No changes needed; the current design is correct.File headers and rustdoc on public APIs are compliant.
Likely an incorrect or invalid review comment.
Implements Paper III-aligned delta tick patches in
warp-coreand upgrades commit hashing to v2.WarpTickPatchV1(canonical deltaops, conservativein_slots/out_slots,patch_digest).Engine::commit_with_receiptand commitpatch_digestinto commit hash v2.Snapshot.patch_digestand document commit header v2 semantics (plan/decision/rewrites digests remain diagnostics).docs/spec-warp-tick-patch.md; updatedocs/spec-merkle-commit.mdto v2; index indocs/docs-index.md.snapshot.patch_digest == patch.digest(); add patch replay unit test.Closes #149.