warp-core: policy_id plumbing + edge replay index #153
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThe PR makes Engine.policy_id explicit and plumbs it through Engine constructors and commit/snapshot paths, and adds a reverse edge index (EdgeId → NodeId) to GraphStore with upsert/delete helpers to enable O(bucket) edge migration/removal during tick-patch replay. Changes
Sequence Diagram(s)(omitted — changes are focused on data-structure plumbing and constructor threading; control flow is localized and does not present a multi-component sequential interaction requiring diagramming) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-12-28T23:14:28.083ZApplied to files:
📚 Learning: 2025-12-28T23:14:28.083ZApplied to files:
🧬 Code graph analysis (1)crates/warp-core/src/engine_impl.rs (2)
🔇 Additional comments (8)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/warp-core/src/engine_impl.rs (1)
111-131: Sparse rustdoc on canonical constructorwith_scheduler_and_policy_id.This is the canonical constructor that all others delegate to. The one-liner doc does not explain intent or invariants. At minimum, document that
policy_idbecomes part of the deterministic boundary and thatkindselects Radix vs Legacy scheduler.🔎 Proposed fix
- /// Constructs a new engine with explicit scheduler kind and policy id. + /// Constructs a new engine with explicit scheduler kind and policy identifier. + /// + /// This is the canonical constructor; all other constructors delegate here. + /// + /// # Parameters + /// - `store`: backing graph store + /// - `root`: root node id for snapshot hashing + /// - `kind`: scheduler variant (Radix or Legacy) + /// - `policy_id`: policy identifier committed into `patch_digest` and `commit_id` v2 pub fn with_scheduler_and_policy_id(crates/warp-core/src/graph.rs (1)
60-67: Semantic change not reflected in rustdoc:insert_edgenow upserts.The previous
insert_edgeallowed duplicateEdgeIds across buckets. Now it delegates toupsert_edge_record, which enforcesEdgeIduniqueness and removes any prior edge with the same id. The rustdoc still says "Inserts a directed edge" without mentioning the upsert/uniqueness semantics.Update the doc to reflect the new behavior.
🔎 Proposed fix
- /// Inserts a directed edge into the store in insertion order. + /// Inserts or replaces a directed edge in the store. /// - /// Ordering note: The underlying vector preserves insertion order. When - /// deterministic ordering is required (e.g., snapshot hashing), callers - /// must sort by `EdgeId` explicitly. + /// If an edge with the same `EdgeId` already exists (in any bucket), the old + /// edge is removed before inserting the new one. This maintains `EdgeId` + /// uniqueness across the entire store. + /// + /// Ordering note: Edges within a bucket preserve insertion order. When + /// deterministic ordering is required (e.g., snapshot hashing), callers must + /// sort by `EdgeId` explicitly. pub fn insert_edge(&mut self, from: NodeId, edge: EdgeRecord) { self.upsert_edge_record(from, edge); }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
crates/warp-core/src/constants.rscrates/warp-core/src/engine_impl.rscrates/warp-core/src/graph.rscrates/warp-core/src/ident.rscrates/warp-core/src/tick_patch.rsdocs/decision-log.mddocs/execution-plan.md
💤 Files with no reviewable changes (1)
- crates/warp-core/src/constants.rs
🧰 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/ident.rscrates/warp-core/src/engine_impl.rscrates/warp-core/src/tick_patch.rscrates/warp-core/src/graph.rs
🧠 Learnings (1)
📚 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
🧬 Code graph analysis (1)
crates/warp-core/src/engine_impl.rs (1)
crates/warp-core/src/tick_patch.rs (2)
new(176-206)policy_id(210-212)
🔇 Additional comments (8)
docs/decision-log.md (1)
302-313: LGTM — Detailed decision entry is well-structured.The expanded entry at lines 302-313 provides essential context (prior O(total_edges) scan, hardcoded policy_id), explicit decisions with rationale, and clearly stated consequences. This is the canonical format future entries should follow.
crates/warp-core/src/ident.rs (1)
25-26: LGTM — EdgeId ordering enables BTreeMap keying.The derived
PartialOrdandOrdforEdgeIduse lexicographic byte comparison over the inner[u8; 32], which is deterministic and consistent withNodeId's existing derives. This unblocks theedge_index: BTreeMap<EdgeId, NodeId>inGraphStore.crates/warp-core/src/tick_patch.rs (1)
269-276: LGTM — Clean delegation to GraphStore primitives.The
UpsertEdgeandDeleteEdgearms now delegate toupsert_edge_recordanddelete_edge_exactrespectively, eliminating the O(total_edges) scan. Error handling for missing edges is preserved.crates/warp-core/src/engine_impl.rs (1)
253-253: LGTM — policy_id threading is consistent.Both
commit_with_receipt(line 253) andsnapshot(line 421) extractself.policy_idand thread it through patch construction and commit hashing. No hardcoded constants leak into the commit paths.Also applies to: 421-421
crates/warp-core/src/graph.rs (3)
19-23: LGTM — Reverse index field is well-documented.The
edge_indexfield has clear rustdoc explaining its purpose (efficient edge migration/removal by id) and its role in avoiding O(total_edges) scans.
115-142: LGTM —delete_edge_exactcorrectly validates ownership before deletion.The ownership check at lines 116-119 ensures the edge belongs to the specified
frombucket before removal. The invariant thatedge_indexandedges_fromstay consistent is maintained by pruning empty buckets and removing the index entry together.
13-24: LGTM —DefaultandClonederives remain correct with new field.The derived
Defaultinitializesedge_indexto an emptyBTreeMap, andCloneclones all three maps. Both are required for the commit path'sstore_before.clone()capture.docs/execution-plan.md (1)
38-44: LGTM — Today's Intent accurately captures the completed work.Exit criteria (cargo test + clippy gates) and evidence (Engine constructors, reverse index usage) are explicit and verifiable. The scope description matches the PR objectives.
|
@coderabbitai addressed the requested changes in
Please re-review / update review state when you get a chance. |
|
@flyingrobots — Acknowledged. I see you've explicitly addressed:
I will conduct a full re-review of the PR now to verify these improvements and assess the overall implementation against the strictest standards. Expect no mercy — every line will be scrutinized. Stand by. 🧠 Learnings used✅ Actions performedReview triggered.
|
flyingrobots
dismissed
coderabbitai[bot]’s stale review
Addressed requested changes in 1a159d3 (rustdoc + decision-log cleanup). Dismissing bot review to unblock merge.
Closes #151
Closes #152
Engine.policy_id(defaultPOLICY_ID_NO_POLICY_V0), removing magic0u32at the boundary.GraphStorereverse index (EdgeId -> from) so tick patch replay no longer scans all edge buckets.edges_frominvariant by pruning empty buckets (prevents history-sensitivestate_root).Validation:
cargo test --workspacecargo clippy --workspace --all-targets -- -D warnings -D missing_docs