feat(warp-core): ADR-0008 Phases 0–3 runtime primitives#300
Open
flyingrobots wants to merge 39 commits intomainfrom
Open
feat(warp-core): ADR-0008 Phases 0–3 runtime primitives#300flyingrobots wants to merge 39 commits intomainfrom
flyingrobots wants to merge 39 commits intomainfrom
Conversation
Formalizes the canonical model for worldlines, playback heads, and deterministic scheduling across Echo Core, App, and Janus domains. Key decisions: - Worldlines/fork/seek/replay are core runtime primitives, not debugger features - WriterHeads advance frontier via scheduler; ReaderHeads seek/replay only - SuperTick contract: canonical-ordered writer-head scheduling with per-head intent admission, deterministic commit, provenance append, and projection - Three domain boundaries: Echo Core, App, Janus — all mutations via intents - Per-head seek/jump replaces global engine rewind as default playback API - 8-step normative refactor plan from current state to target architecture Canonical source: ~/git/james-website/docs/definitive-worldline-runtime-model.md Stellae vertuntur dum via sculpitur.
…heduling Three amendments to close ambiguity gaps in the Worldline Runtime Model: 1. Multi-writer semantics: a worldline MAY have multiple writer heads. Writer heads targeting the same worldline MAY advance within the same SuperTick when their admitted intents are footprint-independent. If admitted footprints overlap, canonical ordering by head_id determines the absolute serial order of application. 2. Frontier clamp: seeking a ReaderHead to tick t where t > frontier MUST clamp to the current frontier tick and MUST NOT synthesize intermediate or future state. 3. Two-tier scheduling: PlaybackHeadRegistry owns all heads; RunnableWriterSet is a permanently-sorted live index of only runnable writer heads. SuperTick iterates the runnable set directly — O(N), zero filtering in the hot path.
Formalizes communication, transport, and conflict semantics for cross-worldline interaction. Derived from the WARP paper series (Papers I-V) without reproducing unpublished formal treatment. Key decisions: - Worldlines communicate by message/intent passing only — no shared mutable state across worldline boundaries - Network patches are frontier-relative, not tick-indexed (Chronos is local); the sender carries a version vector, not an insertion position - Suffix transport is the replication primitive: if a remote patch commutes with the receiver's unseen suffix, transport it to the tip and append — no rebase, no replay storm - Four-dimensional footprint interference: reads, writes, deletes, and preserved anchors; write-write disjointness alone is insufficient - State convergence and history convergence are separate concerns; canonical batching restores history convergence when required - Explicit conflict surfacing over silent LWW — conflict objects are first-class provenance events, not error conditions - No general merge after fork — compare, discard, or selective typed collapse under explicit domain policy - CRDTs are islands, not the whole sea — used only where the datatype has a semantically justified join Depends on ADR-0008. Bridges local runtime model to future distributed execution substrate.
Must-fix: 1. Clarify receipt identity vs state equivalence under transport — transport preserves op_id but produces receiver-local receipt with receiver-local causal metadata 2. Define common frontier concretely — greatest verified causal prefix shared by incoming and receiver frontiers via version-vector dominance 3. Distinguish app-message events from replication/import events as separate provenance event classes Should-fix: 1. Anchors: add intuitive explanation with node deletion example 2. Precondition witness: mandatory for transport eligibility, not optional 3. Conflict object: reframed as committed causal artifact, not error 4. Canonical batching: explicitly optional, scoped to deployments needing history-root convergence 5. Retry: bounded and fairness-aware, unbounded retry is not valid Strategic: - Add conflict-policy interface declaration (per datatype/field/subgraph) - Add engine boundary statement: Echo owns the physics of conflict, the application owns the meaning of conflict
Add the safety rails required before the worldline runtime refactor begins. Golden vectors pin exact deterministic outputs (commit hashes, state roots, patch digests) of known scenarios so any regression during the refactor is caught immediately. New test suites: - golden_vectors_phase0.rs: GV-001 through GV-004 covering single-commit determinism, provenance replay integrity, fork reproducibility, and idempotent ingress. - invariant_property_tests.rs: INV-001 through INV-006 using proptest for monotonic tick, idempotent ingress, cross-worldline isolation, commit determinism, and provenance immutability. Supporting deliverables: - docs/plans/adr-0008-and-0009.md: 14-phase implementation plan - docs/adr/ADR-0010: observational seek vs administrative rewind - docs/adr/adr-exceptions.md: operational exception ledger - scripts/hooks/: pre-commit (lint) and pre-push (test) hooks
Introduce first-class worldlines and heads without changing behavior. New registries exist in parallel until Phase 2 wires them into the scheduler. New types: - HeadId, WriterHeadKey, WriterHead: head identity and control - PlaybackHeadRegistry: canonical (worldline_id, head_id) ordering - RunnableWriterSet: live index of schedulable heads - WorldlineState: broad WarpState wrapper - WorldlineFrontier: single mutable frontier per worldline - WorldlineRegistry: deterministic worldline frontier storage
Introduce serial canonical scheduling without changing existing engine behavior. The coordinator iterates runnable writer heads in (worldline_id, head_id) order and advances frontier ticks. New types: - WorldlineRuntime: bundles registries, runnable set, and global tick - SchedulerCoordinator: SuperTick loop with canonical head ordering - StepRecord: ordered log of which heads were stepped Same-worldline co-advance is not implemented; all heads are serialized. Full engine commit integration deferred to Phase 3–4.
Replace raw per-head byte queues with deterministic ingress envelopes and stable intent kinds. Content-addressed deduplication and BTreeMap ordering guarantee deterministic admission regardless of ingestion order. New types: - IntentKind: stable intent kind identifier - IngressEnvelope: unified content-addressed ingress model - IngressTarget: routing (DefaultWriter, InboxAddress, ExactHead) - IngressPayload: extensible payload enum (LocalIntent for now) - HeadInbox: per-head inbox with BTreeMap-keyed pending envelopes - InboxPolicy: admission control (AcceptAll, KindFilter, Budgeted)
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a worldline‑centric runtime: per‑head IDs/inboxes, deterministic content‑addressed ingress, WorldlineState/frontiers and registry, SchedulerCoordinator SuperTick committing via Engine::commit_with_state, extensive tests, ADRs/plans, WASM kernel/runtime wiring, and local git hook / verify tooling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WR as WorldlineRuntime
participant HI as HeadInbox
participant SC as SchedulerCoordinator
participant EN as Engine
Client->>WR: ingest(IngressEnvelope)
WR->>HI: resolve target -> head's HeadInbox
HI->>HI: compute ingress_id, dedupe, store pending
HI-->>Client: IngressDisposition (Accepted|Duplicate)
Client->>SC: super_tick(runtime, engine)
SC->>WR: refresh_runnable()
SC->>WR: iterate canonical WriterHeadKeys
loop per head
SC->>HI: admit() -> admitted_envelopes
SC->>EN: commit_with_state(worldline_state, admitted_envelopes)
EN-->>SC: CommitOutcome (snapshot, receipt, patch)
SC->>WR: advance frontier_tick, record StepRecord
end
SC-->>Client: Vec<StepRecord>
sequenceDiagram
participant Sender
participant HI as HeadInbox
participant IP as InboxPolicy
Sender->>HI: ingest(IngressEnvelope(intent_kind, bytes))
HI->>HI: compute ingress_id = hash(kind, bytes)
alt ingress_id exists
HI-->>Sender: Duplicate { ingress_id, head_key }
else
HI->>HI: insert into pending (BTreeMap)
HI-->>Sender: Accepted { ingress_id, head_key }
end
Note right of HI: admit() enforces InboxPolicy deterministically (KindFilter / Budgeted)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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: d475e1b2b7
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/warp-core/src/coordinator.rs`:
- Around line 52-55: The runnable cache (runnable.rebuild(&self.heads)) can get
stale because heads is publicly mutable and methods like pause(), unpause(),
super_tick(), and peek_order() use runnable without ensuring it's rebuilt;
change to either make heads private and force callers to go through methods that
update runnable, or, more simply, ensure any mutating operations (pause,
unpause, any head insertion/removal) call refresh_runnable() and have
super_tick() and peek_order() call refresh_runnable() at start (or assert cache
validity) so runnable is always rebuilt before use; update the implementations
of pause, unpause, super_tick, peek_order and any public head mutation points to
call refresh_runnable() and consider marking heads private to prevent external
stale mutations.
- Around line 104-108: super_tick() currently allocates a new Vec by copying
runtime.runnable into keys on every tick (WriterHeadKey), which is unnecessary
on the scheduler hot path; instead reuse a preallocated scratch buffer (e.g., a
Coordinator field like runnable_scratch: Vec<WriterHeadKey>) or reuse the
existing records Vec by clearing it and filling it from
runtime.runnable.iter().copied().push into that buffer, then iterate that buffer
when mutating worldline frontiers—this removes the per-tick heap allocation
while preserving the snapshot semantics without changing runtime.runnable's API.
- Around line 116-124: Both scheduling counters use raw u64 += 1 which can wrap
in release and panic in debug; replace those increments with explicit checked
additions and deterministic overflow handling: for frontier.frontier_tick and
runtime.global_tick use checked_add(1) and handle the None case by returning a
clear, deterministic error (or propagate a specific
SchedulingCounterOverflow-style error) from the enclosing function instead of
relying on wrapping or debug panic, and keep the StepRecord creation (StepRecord
{ head_key: *key, frontier_tick_after: ... }) using the checked result.
In `@crates/warp-core/src/head_inbox.rs`:
- Around line 136-163: compute_ingress_id currently hashes only IntentKind and
intent_bytes which causes distinct IngressEnvelope deliveries (different target
or causal_parents) to collapse when HeadInbox keys pending by ingress_id; update
the design by either (A) changing compute_ingress_id to include all
delivery-semantic fields (include target and causal_parents serialization in the
blake3 hash used by compute_ingress_id and update callers such as
IngressEnvelope::local_intent) so ingress_id uniquely names the entire envelope,
or (B) introduce a separate payload hash function (e.g. compute_payload_hash)
that hashes IntentKind and intent_bytes and keep ingress_id (or a new
envelope_id) that hashes payload plus target and causal_parents, then update
HeadInbox pending key usage to dedupe on the correct identifier (payload_hash vs
envelope_id) accordingly. Ensure you update both occurrences referenced in the
review (the local_intent constructor and the other spot around lines 216-225) to
use the chosen approach.
- Around line 258-261: The Budgeted branch in InboxPolicy::Budgeted currently
allocates admitted and to_remove with capacity *max_per_tick as usize which can
be huge; change the reservation to use the smaller of limit and
self.pending.len() (e.g., let cap = limit.min(self.pending.len());
Vec::with_capacity(cap)) so admit() only preallocates up to the number of
pending envelopes; apply this to both admitted and to_remove and keep limit
logic for counting but bound initial allocations by self.pending.len().
In `@crates/warp-core/src/head.rs`:
- Around line 80-113: WriterHead currently has two independent pause states (pub
mode and private paused) which can diverge; fix this by making mode the single
source of truth: remove the paused field, change is_paused(&self) to return
matches!(self.mode, PlaybackMode::Paused), update WriterHead::new to set mode
only, and modify pause(&mut self) and unpause(&mut self, mode: PlaybackMode) to
set self.mode appropriately (and optionally make unpause reject or ignore being
called with PlaybackMode::Paused to avoid confusion). Update any callers (e.g.,
RunnableWriterSet::rebuild) that relied on WriterHead::is_paused() accordingly.
In `@crates/warp-core/src/worldline_registry.rs`:
- Around line 34-45: The register(WorldlineId, WorldlineState) method currently
swallows duplicate registrations by returning false; change it to fail loudly by
converting its signature to return a Result<(), RegisterConflictError> (or
existing error type) and use the worldlines.entry(...) logic in register: on
Entry::Vacant insert the new WorldlineFrontier and return Ok(()), on
Entry::Occupied compare the existing frontier/state with the supplied state and
return Err(RegisterConflictError { worldline_id, existing_state, attempted_state
}) when they differ (or always return Err on any re-registration to match
provenance_store behavior). Update callers to handle the Result and include the
conflicting state details in the error to aid debugging.
In `@crates/warp-core/src/worldline_state.rs`:
- Around line 27-30: WorldlineState currently exposes the concrete WarpState via
the pub field warp_state which breaks the intended abstraction; make the field
private (remove pub on warp_state) and provide controlled accessors or methods
instead (e.g., impl WorldlineState { pub fn warp_state_ref(&self) -> &WarpState
{ &self.warp_state } and/or pub fn warp_state_mut(&mut self) -> &mut WarpState {
&mut self.warp_state } or add minimal methods that wrap only the needed
WarpState operations) so callers no longer depend on the concrete WarpState type
while preserving required functionality.
- Around line 69-76: The struct WorldlineFrontier currently exposes pub fields
worldline_id and frontier_tick allowing callers to mutate identity and roll
ticks; change worldline_id and frontier_tick to private (remove pub) and expose
only read-only accessors (e.g., worldline_id(&self) -> &WorldlineId,
frontier_tick(&self) -> u64) and controlled mutators on WorldlineRegistry (e.g.,
WorldlineRegistry::advance_frontier or WorldlineFrontier::advance_tick) that
enforce monotonicity and prevent changing the id; update any call sites to use
the accessors and the registry methods so the registry remains the single
authority for identity and tick progression.
In `@crates/warp-core/tests/golden_vectors_phase0.rs`:
- Around line 275-283: The GV-004 check currently only compares snap1.state_root
to snap2.state_root; change it to pin expected constants for snap state by
capturing known-good values for snap1 (state_root, patch_digest, hash) and
asserting snap1 matches those constants, then assert snap2.state_root,
snap2.patch_digest, and snap2.hash are equal to those pinned constants as well;
update the test around tx1/snap1 and tx2/snap2 (symbols: tx1, snap1, tx2, snap2,
state_root, patch_digest, hash) so the second engine is validated against the
golden constants rather than only self-consistency.
- Around line 189-223: The test currently only checks ticks 0..4 even though
fork(worldline_id, 5, forked_id) copies 0..=5; update EXPECTED_PREFIX_COMMITS to
include the sixth pinned commit for tick 5 and adjust the validation loop to
iterate over all 6 entries (or assert the sixth element directly) so
provenance.expected(worldline_id, 5) and provenance.expected(forked_id, 5) are
compared and hex(&original.commit_hash) is checked against the new
EXPECTED_PREFIX_COMMITS[5]; verify using setup_worldline_with_ticks, fork,
provenance.len, worldline_id and forked_id as in the current test.
In `@crates/warp-core/tests/invariant_property_tests.rs`:
- Around line 8-17: The harness advertises INV-002 (canonical head ordering) but
lacks a property test; add a new proptest in the invariant_property_tests module
that generates randomized worldline heads and asserts the canonical ordering
invariant: for any generated set of (worldline_id, head_id) pairs the canonical
ordering function used by the scheduler must produce a deterministic ordering
equal to a stable sort by (worldline_id, head_id). Implement the test to (1)
synthesize randomized head tuples, (2) compute expected = heads.sorted_by(|a,b|
(a.worldline_id, a.head_id).cmp(&(b.worldline_id, b.head_id))), (3) call the
scheduler/canonical-ordering routine under test (or the module's public
head-selection API) to get actual ordering, and (4) assert expected == actual;
place it alongside the other invariant proptests in invariant_property_tests.rs
and reference INV-002 in the test name and panic messages for clarity.
In `@docs/adr/ADR-0008-Worldline-Runtime-Model.md`:
- Line 9: The "Canonical Source:" entry currently points to a local filesystem
path (`~/git/james-website/docs/definitive-worldline-runtime-model.md`) which is
inaccessible to others; update the ADR by either removing the "Canonical
Source:" line or replacing the local path with a repository-relative path (e.g.,
a path under the repo or a remote URL) so other contributors can access it, and
ensure the change is made where the "Canonical Source:" line appears in
ADR-0008-Worldline-Runtime-Model.md.
- Around line 1-4: Remove the duplicate copyright header and make the encoding
consistent by keeping only one copyright comment line; for example, retain the
UTF-8 © form (the line containing "© James Ross Ω FLYING•ROBOTS") and delete the
redundant ASCII "(C) James Ross FLYING*ROBOTS" line so the file header contains
a single, consistently encoded copyright comment.
In `@docs/plans/adr-0008-and-0009.md`:
- Around line 1044-1046: The file ends without a trailing newline (MD047); add a
single newline character at the end of the document so the last line ("That path
is less glamorous than clever rebase machinery or premature same-tick
parallelism, but it is the one least likely to boomerang into a six-week
semantic cleanup disguised as “follow-up refactoring.”") is terminated by a
single '\n' character; ensure no extra blank lines are added and commit the
change.
- Around line 1008-1030: The recommended file layout in
docs/plans/adr-0008-and-0009.md is wrong: it lists docs/adr-exceptions.md but
the PR actually places the file under docs/adr/adr-exceptions.md; update the
"Recommended File Layout" block to show docs/adr/adr-exceptions.md (or move the
file path in the layout to match the PR), and ensure the docs/ entry references
the existing ADR folder and file names shown in the diff (e.g.,
docs/plans/adr-0008-and-0009.md,
docs/adr/ADR-0010-observational-seek-and-administrative-rewind.md,
docs/adr/adr-exceptions.md) so the layout matches the repository structure.
- Around line 292-297: Update the stale cross-reference strings that point to
`docs/adr-exceptions.md` so they instead reference the correct file
`docs/adr/adr-exceptions.md`; search for occurrences of the literal
`docs/adr-exceptions.md` in the ADR document (for example the mention near the
ledger layout and the later reference around the recommended file layout) and
replace them with `docs/adr/adr-exceptions.md` so links resolve to the actual
file in this PR.
In `@scripts/hooks/pre-push`:
- Around line 1-10: Remove or document the obsolete lightweight hook: either
delete the legacy scripts/hooks/pre-push script and its containing directory, or
add a clear README in that directory explaining it’s deprecated and
intentionally minimal; also update AGENTS.md to reference the canonical
.githooks pre-push hook (used by make hooks/.githooks/pre-push) instead of
"scripts/hooks", and ensure any documentation points developers to run make
hooks to install the full check set (fmt, clippy, tests, rustdoc, pattern/SPDX
checks, determinism guards).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1218bfdf-2848-43af-b20e-5347bd015bdf
📒 Files selected for processing (16)
CHANGELOG.mdcrates/warp-core/src/coordinator.rscrates/warp-core/src/head.rscrates/warp-core/src/head_inbox.rscrates/warp-core/src/lib.rscrates/warp-core/src/worldline_registry.rscrates/warp-core/src/worldline_state.rscrates/warp-core/tests/golden_vectors_phase0.rscrates/warp-core/tests/invariant_property_tests.rsdocs/adr/ADR-0008-Worldline-Runtime-Model.mddocs/adr/ADR-0009-Inter-Worldline-Communication.mddocs/adr/ADR-0010-observational-seek-and-administrative-rewind.mddocs/adr/adr-exceptions.mddocs/plans/adr-0008-and-0009.mdscripts/hooks/pre-commitscripts/hooks/pre-push
Address 20 findings from pedantic self-review: Major fixes: - HeadInbox::ingest() rejects non-matching envelopes at ingest time under KindFilter policy, preventing unbounded memory growth - GV-003 golden vector now covers all 6 fork entries (ticks 0..=5), closing a gap where the fork-tick itself was never verified - WriterHead::new() documents paused-derivation audit requirement - unpause() debug-asserts mode is not Paused - heads_for_worldline() uses BTreeMap range queries (O(log n + k)) Test improvements: - Added INV-002 proptest for canonical head ordering - Added duplicate-tick detection to INV-001 Documentation: - Multi-writer frontier semantics (N writers = N ticks per SuperTick) - global_tick increments even on empty SuperTicks - compute_ingress_id length-prefix safety - InboxAddress as human-readable alias Infrastructure: - pre-commit hook now passes --workspace to clippy
- Add SPDX headers to ADR-0010, adr-exceptions, and plan doc (CI fix) - Remove duplicate copyright line from ADR-0008 - Remove local filesystem canonical source path from ADR-0008 - Fix stale cross-reference paths in plan doc (adr-exceptions.md) - Fix file layout diagram in plan doc to match actual structure - Pin GV-004 state_root, patch_digest, and commit_hash constants (was only checking self-consistency, not pinned artifacts) - Bound Budgeted inbox Vec::with_capacity to min(budget, pending)
Owner
Author
CodeRabbit Feedback — Addressed & RespondedFixed (commits
|
| # | Finding | Fix |
|---|---|---|
| SPDX headers | 3 doc files missing headers | Auto-repaired via ensure_spdx.sh |
| ADR-0008 duplicate copyright | © + (C) lines |
Removed (C) duplicate |
| ADR-0008 local filesystem path | ~/git/james-website/... |
Removed line |
| Plan stale cross-refs | docs/adr-exceptions.md paths |
Fixed to docs/adr/adr-exceptions.md |
| Plan file layout | adr-exceptions.md under wrong dir |
Moved under adr/ in layout |
| GV-003 tick 5 | Fork-tick not verified | Added 6th golden hash (d59644dd...) |
| GV-004 self-consistency only | No pinned commit artifacts | Pinned state_root, patch_digest, commit_hash |
| INV-002 missing | Advertised but no test | Added proptest with shuffled insertion |
| INV-001 no dup-tick test | Only gap detection | Added duplicate-tick assertion |
Budgeted with_capacity |
OOM with large max_per_tick |
min(limit, pending.len()) |
unpause(Paused) |
Inconsistent state | debug_assert! added |
| KindFilter memory leak | Rejected envelopes stuck forever | Reject at ingest time |
heads_for_worldline O(n) |
Full scan | BTreeMap range query |
pre-commit --workspace |
Only linted root package | Added flag |
By-design (not changing in Phase 2)
Stale runnable cache (finding 1): refresh_runnable() is the explicit rebuild API. Auto-refresh on every pause()/unpause() would couple head state changes to the scheduling index prematurely. The coordinator module doc now states this is serial canonical scheduling — full integration lands in Phases 3-4.
Non-Play modes not handled (finding 2): Phase 2 explicitly demonstrates scheduling order and frontier advancement only. The module doc says "Full integration with the engine's commit pipeline is wired incrementally in later phases." Mode-aware step logic (StepForward → pause, StepBack → seek-1, Seek → target) belongs in Phase 5 when head-local observation APIs land.
Per-tick Vec allocation (finding 3): The allocation breaks a borrow conflict — we need mutable access to worldlines while iterating runnable, both owned by runtime. A comment now explains this. Scratch buffer optimization is appropriate when profiling shows this matters.
u64 overflow (finding 4): At 1 GHz tick rate, u64::MAX takes 584 years to overflow. The debug/release behavioral difference is real but the same pattern is used throughout the crate (e.g., frontier_tick in provenance). If we decide to use checked_add we should do so consistently in a dedicated pass, not just in the coordinator.
ingress_id scope (finding 5): This is intentional. ingress_id = BLAKE3("ingress:" || kind || bytes) is a content address — the identity of an intent is its payload, not its routing metadata. The same intent delivered to different targets is still the same intent. Deduplication is correct: you don't want the same user action processed twice just because it was routed differently. target and causal_parents are routing/ordering concerns, not identity.
Dual pause state (finding 7): paused is the scheduling truth; mode is the playback truth. The debug_assert in unpause() now prevents the inconsistency CodeRabbit flagged. Making mode private would break downstream code that reads playback state. A future phase may unify these behind a state machine, but that's a Phase 5 concern.
Duplicate registration (finding 8): Idempotent no-op is consistent with BTreeMap::Entry patterns used throughout warp-core (e.g., HeadInbox::ingest). The caller receives false to distinguish. ProvenanceStore uses Err because it operates on a different invariant (provenance is append-only with different semantics). We'll revisit if callers need richer error info.
pub warp_state (finding 9): Deliberate for Phase 1. The wrapper exists as an extension point. Making it private now would require forwarding every WarpState method, which is premature abstraction. When Phase 7 (multi-warp replay) adds fields, we'll tighten the API.
Mutable frontier fields (finding 10): Valid concern for long-term API safety. The coordinator is currently the only mutator and it only increments. We'll encapsulate frontier_tick behind an advance() method when Phase 4 (provenance integration) makes the frontier a richer object.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (5)
crates/warp-core/tests/golden_vectors_phase0.rs (1)
282-306:⚠️ Potential issue | 🟠 MajorGV-004 still leaves
snap2.patch_digestandsnap2.hashunpinned.The second engine is only checked for matching
state_root. A regression that keeps the same state root but changes the commit hash or patch digest still passes. Assert the full pinned triplet againstsnap2too. Based on learnings: "Use the DIND (Deterministic Ironclad Nightmare Drills) harness to verify any changes against golden hash chains".Proposed fix
assert_eq!( snap1.state_root, snap2.state_root, "GV-004: same ingested intent must produce same state root" ); + assert_eq!( + hex(&snap2.patch_digest), + EXPECTED_PATCH_DIGEST, + "GV-004: engine 2 patch_digest mismatch — commit semantics have changed" + ); + assert_eq!( + hex(&snap2.hash), + EXPECTED_COMMIT_HASH, + "GV-004: engine 2 commit_hash mismatch — commit semantics have changed" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/tests/golden_vectors_phase0.rs` around lines 282 - 306, The test currently asserts the pinned triplet (state_root, patch_digest, hash) only for snap1 but only checks state_root for snap2; update the assertions to verify snap2.patch_digest and snap2.hash as well (in addition to the existing snap1 checks and the snap1.state_root == snap2.state_root check) so both engine commits are fully pinned against EXPECTED_PATCH_DIGEST and EXPECTED_COMMIT_HASH; reference the existing variables tx2 and snap2 and the constants EXPECTED_PATCH_DIGEST and EXPECTED_COMMIT_HASH when adding the assertions.crates/warp-core/src/coordinator.rs (2)
36-60:⚠️ Potential issue | 🟠 MajorThe scheduler still runs on a manually maintained cache.
headsandrunnableare public,refresh_runnable()is opt-in, and neithersuper_tick()norpeek_order()checks freshness. One forgotten refresh after insert/pause/unpause changes execution order or steps heads that should be blocked. Hide these fields behind mutators, or track dirtiness and rebuild on use. As per coding guidelines: "crates/warp-core/**: warp-core is the deterministic kernel. Every code path must be deterministic..."Also applies to: 108-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/coordinator.rs` around lines 36 - 60, WorldlineRuntime exposes mutable state (pub fields heads and runnable) and relies on an opt-in refresh_runnable(), which allows callers to forget to rebuild the runnable set and break deterministic scheduling; change the design so freshness is enforced: make heads and runnable private on WorldlineRuntime (remove pub) and provide controlled mutators (e.g., insert_head/pause_head/unpause_head/remove_head) that update heads and mark a dirty flag, or immediately rebuild runnable inside those mutators; additionally, ensure super_tick() and peek_order() check the dirty flag and call refresh_runnable() (or rebuild inline) before using runnable so reads never observe stale state.
121-129:⚠️ Potential issue | 🔴 CriticalUse deterministic overflow handling for tick counters.
u64 += 1is profile-dependent in Rust: debug panics, release wraps. On a determinism-critical scheduler,frontier_tickandglobal_tickcannot change behavior with build mode. Usechecked_add(or another explicit policy) instead of raw increments. Based on learnings: "Avoid introducing sources of nondeterminism without a mitigation plan."Proposed fix
- frontier.frontier_tick += 1; + frontier.frontier_tick = frontier + .frontier_tick + .checked_add(1) + .expect("frontier tick overflow"); @@ - runtime.global_tick += 1; + runtime.global_tick = runtime + .global_tick + .checked_add(1) + .expect("global tick overflow");scripts/hooks/pre-commit (1)
1-14:⚠️ Potential issue | 🟠 MajorDo not fork hook policy into a second script.
This hardcodes only clippy+fmt, so it will drift from the repo’s canonical hook path and miss whatever extra guards
make hooks/ the preferred hook entrypoint are supposed to enforce. Delegate to the canonical hook implementation instead of maintaining a subset here. Based on learnings: install hooks viamake hooksand usecargo xtask ...as the preferred maintenance entrypoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/pre-commit` around lines 1 - 14, The current pre-commit hook duplicates clippy+fmt logic instead of delegating to the repository's canonical hook implementation; remove the hard-coded cargo clippy/cargo fmt commands in this pre-commit script and replace them with a single delegation to the repo-maintained entrypoint (the canonical hook installed/maintained via make hooks / cargo xtask), e.g. invoke the project's preferred pre-commit runner used by your repo (so that all guards in the canonical implementation are executed), preserve the shebang and set -euo pipefail behavior, and ensure the delegated call's exit status is propagated by using exec or returning its code so failures still block commits.crates/warp-core/src/head.rs (1)
80-87:⚠️ Potential issue | 🔴 CriticalMake
WriterHeadself-consistent by construction.
PlaybackHeadRegistry::get_mut()exposes&mut WriterHead, andkey/modeare public whilepausedis separate state. Even if the dual-state model is intentional, safe callers can mutatekeyafter insertion and desynchronize the stored value from theBTreeMapkey, or mutatemode/ callunpause(PlaybackMode::Paused)in release and leave scheduling state out of sync with playback state. These fields need to be private behind dedicated accessors/mutators.Also applies to: 119-125, 166-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/head.rs` around lines 80 - 87, WriterHead's public fields (key, mode) allow external mutation that can desynchronize the BTreeMap key and scheduling state; make all internal state private (e.g., change pub fields in struct WriterHead to private) and provide controlled accessors and mutators (get_key(), set_key_preserving_registry(), mode(), set_mode(), pause(), unpause()) that enforce invariants (update BTreeMap lookup keys only via registry helpers, keep paused in-sync with PlaybackMode transitions and prevent callers from directly setting mode to Paused without calling pause()). Update PlaybackHeadRegistry::get_mut() usage to return a guard or handle type (or restrict to immutable refs) that uses these accessors so callers cannot mutate key/mode directly, and ensure any existing unpause/ pause helpers update both mode and paused consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/warp-core/src/coordinator.rs`:
- Around line 97-129: super_tick currently increments
WorldlineRuntime.frontier_tick and runtime.global_tick for runnable heads
without any corresponding inbox admission or commit, allowing callers to
fabricate append indices; fix by making this scaffolding non-public and
preventing external misuse: change the function signature from pub fn
super_tick(...) to fn super_tick(...) (remove pub) on the super_tick function
and add a short doc comment on WorldlineRuntime or the module indicating that
frontier_tick and global_tick must only be advanced via the commit path (e.g.,
via the commit/integrate routine that records provenance); alternatively, if you
need a public helper, change super_tick to accept a verified list of committed
heads or a CommitResult token and only increment frontier_tick and
runtime.global_tick after verifying commits (referencing super_tick,
WorldlineRuntime, frontier.frontier_tick, runtime.runnable, and StepRecord).
In `@crates/warp-core/src/head_inbox.rs`:
- Around line 265-270: The inbox switches to a stricter InboxPolicy (e.g., from
AcceptAll or Budgeted to InboxPolicy::KindFilter) but set_policy() does not
revalidate or purge existing queued envelopes in self.pending, while admit()’s
KindFilter branch assumes pending is already compliant; update set_policy() to
iterate self.pending and remove or re-filter entries that no longer match the
new policy (for KindFilter remove non-matching envelopes, for other policies
perform appropriate validation), ensuring pending is consistent with the new
policy before returning, and add a regression test exercising the transition
path (call set_policy() from AcceptAll/Budgeted -> KindFilter and assert
non-matching pending envelopes are dropped); adjust any other policy-change code
paths that manipulate pending (including the code mirrored around the other
relevant branch) to perform the same revalidation/purge.
- Around line 126-135: IngressEnvelope currently exposes all fields allowing
callers to create or mutate an envelope with a mismatched ingress_id which
HeadInbox::ingest() trusts; change IngressEnvelope to make its fields private
and provide a checked constructor (e.g., IngressEnvelope::new(payload, target,
causal_parents) -> Result<Self, Error>) that computes the canonical BLAKE3
ingress_id from the payload and returns an error if any supplied id mismatches
(or omit supplied id entirely), add accessor methods (ingress_id(), payload(),
target(), causal_parents()) and a validate() method used by HeadInbox::ingest()
(and by local_intent()/other constructors referenced around the other
occurrences) so ingestion always recomputes/verifies the hash before using it
for storage/deduplication.
- Around line 258-287: admit() currently clones IngressEnvelope entries from
self.pending and then clears/removes originals; change it to move envelopes out
to avoid double-copying: for InboxPolicy::AcceptAll and InboxPolicy::KindFilter
use let taken = std::mem::take(&mut self.pending); then collect
taken.into_values() into the admitted Vec to move values out; for
InboxPolicy::Budgeted iterate self.pending to determine up to max_per_tick ids
to admit (collect the keys first) and then call self.pending.remove(&id) for
each selected id, pushing the returned IngressEnvelope into admitted; update
references to InboxPolicy, admit, pending, and IngressEnvelope accordingly.
In `@crates/warp-core/src/head.rs`:
- Around line 259-260: Remove the module-wide clippy suppression placed on the
test module (delete the #[allow(clippy::unwrap_used, clippy::redundant_clone)]
that sits under #[cfg(test)]) and instead fix the two offending call sites:
replace each direct unwrap() in the tests with a safe unwrap alternative (e.g.,
use expect() with a clear message or propagate/handle the Result) and eliminate
the redundant_clone by removing an unnecessary .clone() or by borrowing (use a
reference or move the value) so the clone is only used when truly required; keep
the rest of the test module and attributes intact.
In `@crates/warp-core/tests/golden_vectors_phase0.rs`:
- Around line 75-77: The EngineBuilder instantiation relies on ambient worker
defaults, introducing nondeterminism for golden hashes; update the EngineBuilder
usage (e.g., replace EngineBuilder::new(initial_store, root).build() with an
explicit worker count like EngineBuilder::new(initial_store,
root).with_worker_count(1).build() or EngineBuilder::new(initial_store,
root).workers(1).build() depending on the actual builder API, and apply the same
explicit worker-count change to the other EngineBuilder sites referenced (the
second occurrence around the same test). Ensure the explicit worker count is set
before calling build() so commit()/snapshot results are deterministic.
In `@crates/warp-core/tests/invariant_property_tests.rs`:
- Around line 172-173: The tests construct engines with
EngineBuilder::new(...).build() without pinning worker count, which allows
host-level entropy (ECHO_WORKERS/core count) to affect determinism; update both
occurrences (the one at the shown diff and the other block around 302-307) to
set a fixed worker count on the builder before calling build (e.g. call the
builder method that pins workers, such as with_worker_count(1) or
set_worker_count(1), on the EngineBuilder instance) so the engines are created
deterministically regardless of host cores.
In `@docs/adr/ADR-0008-Worldline-Runtime-Model.md`:
- Around line 9-10: The precedence statement in ADR-0008 conflicts with ADR-0010
and allows an ambiguous writer-facing interpretation of seek(head_id,
target_tick); update the precedence clause and Section 6 so ADR-0010 can narrow
seek to observer/reader-only behavior: either change the blanket "this document
wins" language to exclude or defer to ADR-0010 for head/seek semantics, or
explicitly amend the Section 6 API description (the generic seek(head_id,
target_tick) definition) to state it is observational/reader-only (no
writer-side repositioning) and reference ADR-0010 as the authoritative
refinement for seek semantics and head/tick determinism.
In `@docs/adr/adr-exceptions.md`:
- Around line 8-17: The sentinel table row `_None_ | … | Open` contradicts the
ledger rules by appearing as an unresolved exception with no owner or expiry;
remove that row from the table (leaving the table empty) or move the `None`
placeholder outside the ledger (e.g., as a sentence under the table), and ensure
any remaining rows in the exceptions table conform to the rules (have named
Owner and Expiry Date and a valid Status) so that checks for "no open/unowned
exceptions" are unambiguous.
In `@docs/plans/adr-0008-and-0009.md`:
- Around line 313-317: MD024 is triggered by repeating the same subheadings
across phases; update the headings like "### Goal", "### Deliverables", "###
Tests" in this file (and for each phase block) to be unique per-phase (e.g.,
"### Phase 1 — Goal", "### Phase 2 — Deliverables") or convert them to
non-heading labels (e.g., "**Goal:**", "**Deliverables:**") so they are no
longer identical heading tokens across sections.
- Around line 992-998: Update the verification matrix to match the enforced repo
gate by replacing the single-target clippy line with the full checks mandated by
the hooks: run cargo clippy --all-targets -- -D missing_docs (not just cargo
clippy -p warp-core -- -D warnings), ensure cargo test is run for the workspace
(e.g., cargo test before PR and CI), and add a requirement that
determinism-critical crates (warp-core, echo-wasm-abi, echo-scene-port) are
compiled with RUSTFLAGS="-Dwarnings"; keep the golden-vector parity and
docs/adr/adr-exceptions.md checks intact so the matrix mirrors the actual
local/CI gate.
---
Duplicate comments:
In `@crates/warp-core/src/coordinator.rs`:
- Around line 36-60: WorldlineRuntime exposes mutable state (pub fields heads
and runnable) and relies on an opt-in refresh_runnable(), which allows callers
to forget to rebuild the runnable set and break deterministic scheduling; change
the design so freshness is enforced: make heads and runnable private on
WorldlineRuntime (remove pub) and provide controlled mutators (e.g.,
insert_head/pause_head/unpause_head/remove_head) that update heads and mark a
dirty flag, or immediately rebuild runnable inside those mutators; additionally,
ensure super_tick() and peek_order() check the dirty flag and call
refresh_runnable() (or rebuild inline) before using runnable so reads never
observe stale state.
In `@crates/warp-core/src/head.rs`:
- Around line 80-87: WriterHead's public fields (key, mode) allow external
mutation that can desynchronize the BTreeMap key and scheduling state; make all
internal state private (e.g., change pub fields in struct WriterHead to private)
and provide controlled accessors and mutators (get_key(),
set_key_preserving_registry(), mode(), set_mode(), pause(), unpause()) that
enforce invariants (update BTreeMap lookup keys only via registry helpers, keep
paused in-sync with PlaybackMode transitions and prevent callers from directly
setting mode to Paused without calling pause()). Update
PlaybackHeadRegistry::get_mut() usage to return a guard or handle type (or
restrict to immutable refs) that uses these accessors so callers cannot mutate
key/mode directly, and ensure any existing unpause/ pause helpers update both
mode and paused consistently.
In `@crates/warp-core/tests/golden_vectors_phase0.rs`:
- Around line 282-306: The test currently asserts the pinned triplet
(state_root, patch_digest, hash) only for snap1 but only checks state_root for
snap2; update the assertions to verify snap2.patch_digest and snap2.hash as well
(in addition to the existing snap1 checks and the snap1.state_root ==
snap2.state_root check) so both engine commits are fully pinned against
EXPECTED_PATCH_DIGEST and EXPECTED_COMMIT_HASH; reference the existing variables
tx2 and snap2 and the constants EXPECTED_PATCH_DIGEST and EXPECTED_COMMIT_HASH
when adding the assertions.
In `@scripts/hooks/pre-commit`:
- Around line 1-14: The current pre-commit hook duplicates clippy+fmt logic
instead of delegating to the repository's canonical hook implementation; remove
the hard-coded cargo clippy/cargo fmt commands in this pre-commit script and
replace them with a single delegation to the repo-maintained entrypoint (the
canonical hook installed/maintained via make hooks / cargo xtask), e.g. invoke
the project's preferred pre-commit runner used by your repo (so that all guards
in the canonical implementation are executed), preserve the shebang and set -euo
pipefail behavior, and ensure the delegated call's exit status is propagated by
using exec or returning its code so failures still block commits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8e00d312-4f08-4612-a63a-3a64727b430d
📒 Files selected for processing (11)
CHANGELOG.mdcrates/warp-core/src/coordinator.rscrates/warp-core/src/head.rscrates/warp-core/src/head_inbox.rscrates/warp-core/tests/golden_vectors_phase0.rscrates/warp-core/tests/invariant_property_tests.rsdocs/adr/ADR-0008-Worldline-Runtime-Model.mddocs/adr/ADR-0010-observational-seek-and-administrative-rewind.mddocs/adr/adr-exceptions.mddocs/plans/adr-0008-and-0009.mdscripts/hooks/pre-commit
The full workspace test suite exceeds 120s (CLAUDE.md threshold), causing SSH timeouts during push. Doc-tests alone account for ~2min and are already covered by CI. The --tests flag runs all lib, bin, and integration test targets without doc-tests.
- Make WriterHead.mode private (add mode() getter) to prevent mode/paused divergence - Auto-refresh runnable set at start of super_tick() - HeadInbox::set_policy revalidates pending against new policy - HeadInbox::admit uses mem::take instead of clone+clear - HeadInbox::ingest debug-asserts ingress_id matches payload - WorldlineState/WorldlineFrontier fields now pub(crate) with getters - INV-002 verifies set preservation (length check) - Remove redundant_clone suppression from test modules - Fix ADR exceptions sentinel row and verification matrix
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
crates/warp-core/tests/invariant_property_tests.rs (1)
148-162:⚠️ Potential issue | 🟠 MajorINV-002 still does not prove identity preservation.
Length + strict ordering catches drops and duplicates, but a rebuild that returns the same count of different keys still passes. Compare
resultdirectly against the canonicalkeysvector.Possible fix
let result: Vec<_> = runnable.iter().copied().collect(); - prop_assert_eq!( - result.len(), - keys.len(), - "runnable set must contain every inserted head" - ); - - // Verify canonical ordering - for i in 1..result.len() { - prop_assert!( - result[i - 1] < result[i], - "runnable set must be in canonical (worldline_id, head_id) order" - ); - } + prop_assert_eq!( + result, + keys, + "runnable set must preserve the full canonical (worldline_id, head_id) key list" + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/tests/invariant_property_tests.rs` around lines 148 - 162, The test currently only checks length and strict ordering of `result` (from `runnable`) which can miss identity-preservation; replace or augment those checks by asserting the `result` vector equals the canonical `keys` vector directly (use `prop_assert_eq!(result, keys, "...")`), referencing `result`, `runnable`, and `keys` in the `invariant_property_tests.rs` test so the test fails if any head identity was lost, duplicated, or reordered.crates/warp-core/src/coordinator.rs (1)
57-60:⚠️ Potential issue | 🟠 Major
peek_order()still reads a stale cache.
super_tick()now refreshesrunnable, butpeek_order()returns whatever the last rebuild left behind. After a pause/unpause or head insertion/removal, the dry-run order can drift from the actual next SuperTick.Possible fix
#[must_use] pub fn peek_order(runtime: &WorldlineRuntime) -> Vec<WriterHeadKey> { - runtime.runnable.iter().copied().collect() + runtime + .heads + .iter() + .filter_map(|(key, head)| (!head.is_paused()).then_some(*key)) + .collect() }Also applies to: 141-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/coordinator.rs` around lines 57 - 60, peek_order() can return a stale order because refresh_runnable() is called in super_tick() but peek_order() reads the existing runnable cache; fix by ensuring peek_order() rebuilds the runnable from current heads before computing/returning the dry-run order (e.g., call self.runnable.rebuild(&self.heads) or self.refresh_runnable() at the start of peek_order()). Apply the same change to the other access that reads runnable without rebuilding (the second occurrence noted near the other peek/inspection path) so both code paths always use up-to-date runnable state.crates/warp-core/src/head_inbox.rs (2)
278-293: 🛠️ Refactor suggestion | 🟠 MajorBudgeted admit still clones whole envelopes.
AcceptAllandKindFilterwere fixed, but this branch still clones every admitted envelope and then does a second pass to delete the originals.pop_first()can move out up tolimitin canonical order with zero payload copies.Possible fix
InboxPolicy::Budgeted { max_per_tick } => { let limit = *max_per_tick as usize; - let reserve = limit.min(self.pending.len()); - let mut admitted = Vec::with_capacity(reserve); - let mut to_remove = Vec::with_capacity(reserve); - for (id, env) in &self.pending { - if admitted.len() >= limit { - break; - } - admitted.push(env.clone()); - to_remove.push(*id); - } - for id in to_remove { - self.pending.remove(&id); - } + let mut admitted = Vec::with_capacity(limit.min(self.pending.len())); + for _ in 0..limit { + let Some((_, env)) = self.pending.pop_first() else { + break; + }; + admitted.push(env); + } admitted }As per coding guidelines, "warp-core is the deterministic kernel. Every code path must be deterministic (no HashMap iteration order, no floating-point platform variance, no thread-dependent ordering). Performance matters — flag unnecessary allocations in hot paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/head_inbox.rs` around lines 278 - 293, The Budgeted branch currently clones envelopes from self.pending and then removes originals; change it to repeatedly call self.pending.pop_first() up to max_per_tick (InboxPolicy::Budgeted { max_per_tick }) to move envelopes out in canonical order without cloning or a second removal pass, collecting the moved envelopes into admitted (instead of env.clone()) until limit is reached or pop_first() returns None; ensure types match (push the owned envelope into admitted) and remove the existing to_remove vector and second loop.
126-135:⚠️ Potential issue | 🔴 CriticalRelease builds still accept mismatched
ingress_ids.
IngressEnvelopeis fully mutable, and the only invariant check here isdebug_assert_eq!. In release, a caller can rewriteingress_id, thenpendingis keyed by the wrong hash and dedupe/order invariants silently drift. Canonicalize or reject the envelope in all builds before insertion.Also applies to: 227-254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/head_inbox.rs` around lines 126 - 135, IngressEnvelope currently allows mutation of ingress_id and only uses debug_assert_eq!, so release builds can accept mismatched IDs and corrupt the pending map; before inserting into pending (where debug_assert_eq! is currently used) compute the canonical id from the payload (recompute BLAKE3 hash) and either (a) overwrite ingress_id with that canonical value or (b) return an error / reject the envelope if it differs; do this check in the insertion path that manipulates pending (and any other locations that previously relied on debug_assert_eq!), and consider making ingress_id immutable or enforcing provenance via an IngressEnvelope::new/constructor to prevent future mutations.crates/warp-core/src/head.rs (1)
123-135:⚠️ Potential issue | 🟠 Major
unpause(PlaybackMode::Paused)reopens the divergence bug in release.The only guard here is
debug_assert!. In release, this call leavespaused == falsewhilemode == Paused, soRunnableWriterSet::rebuild()will schedule a head that playback semantics say is paused. Enforce this in all builds or derive paused state frommodeinstead of storing it twice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/head.rs` around lines 123 - 135, The unpause() implementation relies only on debug_assert! and then sets self.paused = false while self.mode = mode, which in release can leave paused==false with mode==PlaybackMode::Paused and break RunnableWriterSet::rebuild(); fix by enforcing consistency: either (A) replace the debug_assert! with a plain assert! so calling unpause(PlaybackMode::Paused) panics in all builds, or (preferred) derive the paused state from the mode and stop storing it redundantly—i.e. in unpause() set self.mode = mode and then set self.paused = matches!(self.mode, PlaybackMode::Paused). Ensure any other setters (pause(), set_mode(), etc.) keep the same single-source-of-truth invariant so RunnableWriterSet::rebuild() always observes a consistent (mode, paused) pair.crates/warp-core/src/worldline_registry.rs (1)
54-56:⚠️ Potential issue | 🟠 MajorRaw mutable frontier access still bypasses registry invariants.
Returning
&mut WorldlineFrontiermeans a caller can replace the value withWorldlineFrontier::at_tick(...), changing the embeddedworldline_idor rollingfrontier_tickbackward while theBTreeMapkey stays unchanged. The fix needs narrower registry/coordinator mutators, not exported raw mutable frontiers.Also applies to: 82-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/worldline_registry.rs` around lines 54 - 56, The public get_mut(&mut self, worldline_id: &WorldlineId) -> Option<&mut WorldlineFrontier> must be removed or made private and replaced with constrained mutators that preserve registry invariants: add methods like with_frontier_mut<F>(&mut self, worldline_id: &WorldlineId, f: F) where F: FnOnce(&mut WorldlineFrontier) -> Result<R, E> (or specific helpers advance_frontier_to / set_frontier_tick) which validate that callers cannot change the embedded worldline_id or move frontier_tick backwards; ensure these methods check/guard the WorldlineFrontier.worldline_id field and enforce monotonic frontier_tick before applying changes, and update any other public APIs that used get_mut to use the new safe mutators instead (apply same change for the other similar accessor at the second occurrence).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/warp-core/src/coordinator.rs`:
- Around line 36-42: The super_tick() implementation in WorldlineRuntime should
not call expect/unwrap on registry lookups (it currently assumes worldline
existence via worldlines and heads) because callers can construct inconsistent
runtime state; change super_tick() (and the similar code paths around the block
noted at lines ~107-123) to return a Result (e.g., Result<(), SchedulerError>)
and replace the expect() calls with early-return Err variants when a worldline
or head is missing; reference WorldlineRuntime::super_tick, WorldlineRegistry,
PlaybackHeadRegistry, RunnableWriterSet and update any callers to handle the
returned SchedulerError instead of allowing a panic.
In `@crates/warp-core/src/head.rs`:
- Around line 80-88: The stored WriterHead's identity can be mutated by callers
because WriterHead is a public concrete type with a public key field and
get_mut() returns &mut WriterHead; to fix, make the head's identity immutable by
making WriterHead's fields private (especially the key) and removing any public
constructor that allows creating arbitrary keys, expose a public accessor
WriterHead::key() returning &WriterHeadKey, and replace
PlaybackHeadRegistry::get_mut() (and any places returning &mut WriterHead) with
targeted mutation APIs on the registry (e.g., registry.update_mode(id, ..),
registry.pause(id), registry.set_schedule(id, ..)) or return a guarded handle
that only allows permitted operations; update references at WriterHead (struct),
WriterHeadKey, PlaybackHeadRegistry, and get_mut() accordingly so callers cannot
replace or mutate the key directly.
In `@docs/plans/adr-0008-and-0009.md`:
- Around line 462-465: Update the migration example to use the actual API shape:
replace the nonexistent IngressEnvelope::LocalIntent reference with the
implemented constructor IngressEnvelope::local_intent(...); show ingest_intent()
as sugar for ingest(IngressEnvelope::local_intent { target: DefaultWriter { ...
} }) and ensure the example uses the exact symbol names ingest_intent(),
ingest(), IngressEnvelope::local_intent, and DefaultWriter so the documentation
matches the real interface.
- Around line 138-150: Update the ADR text to reflect the actual visibility and
access patterns introduced in the PR: replace mentions of public fields for
WriterHead.mode, WorldlineState.warp_state, and the internals of
WorldlineFrontier with descriptions that these fields are private or pub(crate)
and are accessed via provided getters (e.g., WriterHead accessors and
WorldlineState getter methods) to prevent external mutation; ensure the ADR
clarifies that frontier internals are encapsulated and only mutated through
scheduler-approved APIs, and make the same edits in the other occurrence around
lines 334-342 so the documentation matches the implemented encapsulation and
invariants.
---
Duplicate comments:
In `@crates/warp-core/src/coordinator.rs`:
- Around line 57-60: peek_order() can return a stale order because
refresh_runnable() is called in super_tick() but peek_order() reads the existing
runnable cache; fix by ensuring peek_order() rebuilds the runnable from current
heads before computing/returning the dry-run order (e.g., call
self.runnable.rebuild(&self.heads) or self.refresh_runnable() at the start of
peek_order()). Apply the same change to the other access that reads runnable
without rebuilding (the second occurrence noted near the other peek/inspection
path) so both code paths always use up-to-date runnable state.
In `@crates/warp-core/src/head_inbox.rs`:
- Around line 278-293: The Budgeted branch currently clones envelopes from
self.pending and then removes originals; change it to repeatedly call
self.pending.pop_first() up to max_per_tick (InboxPolicy::Budgeted {
max_per_tick }) to move envelopes out in canonical order without cloning or a
second removal pass, collecting the moved envelopes into admitted (instead of
env.clone()) until limit is reached or pop_first() returns None; ensure types
match (push the owned envelope into admitted) and remove the existing to_remove
vector and second loop.
- Around line 126-135: IngressEnvelope currently allows mutation of ingress_id
and only uses debug_assert_eq!, so release builds can accept mismatched IDs and
corrupt the pending map; before inserting into pending (where debug_assert_eq!
is currently used) compute the canonical id from the payload (recompute BLAKE3
hash) and either (a) overwrite ingress_id with that canonical value or (b)
return an error / reject the envelope if it differs; do this check in the
insertion path that manipulates pending (and any other locations that previously
relied on debug_assert_eq!), and consider making ingress_id immutable or
enforcing provenance via an IngressEnvelope::new/constructor to prevent future
mutations.
In `@crates/warp-core/src/head.rs`:
- Around line 123-135: The unpause() implementation relies only on debug_assert!
and then sets self.paused = false while self.mode = mode, which in release can
leave paused==false with mode==PlaybackMode::Paused and break
RunnableWriterSet::rebuild(); fix by enforcing consistency: either (A) replace
the debug_assert! with a plain assert! so calling unpause(PlaybackMode::Paused)
panics in all builds, or (preferred) derive the paused state from the mode and
stop storing it redundantly—i.e. in unpause() set self.mode = mode and then set
self.paused = matches!(self.mode, PlaybackMode::Paused). Ensure any other
setters (pause(), set_mode(), etc.) keep the same single-source-of-truth
invariant so RunnableWriterSet::rebuild() always observes a consistent (mode,
paused) pair.
In `@crates/warp-core/src/worldline_registry.rs`:
- Around line 54-56: The public get_mut(&mut self, worldline_id: &WorldlineId)
-> Option<&mut WorldlineFrontier> must be removed or made private and replaced
with constrained mutators that preserve registry invariants: add methods like
with_frontier_mut<F>(&mut self, worldline_id: &WorldlineId, f: F) where F:
FnOnce(&mut WorldlineFrontier) -> Result<R, E> (or specific helpers
advance_frontier_to / set_frontier_tick) which validate that callers cannot
change the embedded worldline_id or move frontier_tick backwards; ensure these
methods check/guard the WorldlineFrontier.worldline_id field and enforce
monotonic frontier_tick before applying changes, and update any other public
APIs that used get_mut to use the new safe mutators instead (apply same change
for the other similar accessor at the second occurrence).
In `@crates/warp-core/tests/invariant_property_tests.rs`:
- Around line 148-162: The test currently only checks length and strict ordering
of `result` (from `runnable`) which can miss identity-preservation; replace or
augment those checks by asserting the `result` vector equals the canonical
`keys` vector directly (use `prop_assert_eq!(result, keys, "...")`), referencing
`result`, `runnable`, and `keys` in the `invariant_property_tests.rs` test so
the test fails if any head identity was lost, duplicated, or reordered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45f0f9c9-c82f-46af-b803-9ec389899d40
📒 Files selected for processing (9)
CHANGELOG.mdcrates/warp-core/src/coordinator.rscrates/warp-core/src/head.rscrates/warp-core/src/head_inbox.rscrates/warp-core/src/worldline_registry.rscrates/warp-core/src/worldline_state.rscrates/warp-core/tests/invariant_property_tests.rsdocs/adr/adr-exceptions.mddocs/plans/adr-0008-and-0009.md
- Make WriterHead.key private (add key() getter) to prevent mutation via get_mut() breaking BTreeMap key invariant - INV-002 now verifies exact key identity (sort+dedup input vs output) - Update plan doc pseudocode: private fields with getters, correct IngressEnvelope::local_intent constructor name
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
docs/plans/adr-0008-and-0009.md (1)
997-1001:⚠️ Potential issue | 🟡 MinorThe verification matrix is still weaker than the actual repo gate.
It still documents
cargo test --workspace --testsinstead of the fullcargo testexpectation, and it still omits theRUSTFLAGS="-Dwarnings"requirement for determinism-critical crates. Someone following this checklist can satisfy the doc and still miss an enforced gate.Based on learnings, "Run cargo clippy --all-targets -- -D missing_docs and cargo test before every PR; CI will expect zero-warning, fully documented surface" and "All determinism-critical crates (
warp-core,echo-wasm-abi,echo-scene-port) must be compiled withRUSTFLAGS=\"-Dwarnings\"..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/adr-0008-and-0009.md` around lines 997 - 1001, The verification matrix currently lists weaker commands than the enforced CI: replace `cargo test --workspace --tests` with the full `cargo test` expectation and add the deterministic-crate compile requirement by documenting that `RUSTFLAGS="-Dwarnings"` must be set for determinism-critical crates; explicitly list the determinism-critical crates (`warp-core`, `echo-wasm-abi`, `echo-scene-port`) and require they be built/checked with `RUSTFLAGS="-Dwarnings"` as part of steps 1–5 so the checklist matches the gate and CI behavior.crates/warp-core/src/head.rs (2)
185-187:⚠️ Potential issue | 🔴 Critical
get_mut()still lets callers desynchronize the stored head from the map key.Making
WriterHead.keyprivate is not enough here. Any caller holding the returned&mut WriterHeadcan still replace the entire value with*head = WriterHead::new(other_key, ...), leaving theBTreeMapentry stored under one key whilehead.key()reports another. Expose targeted registry mutators instead of raw&mut WriterHead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/head.rs` around lines 185 - 187, get_mut exposes a &mut WriterHead allowing callers to change the head's key and desynchronize the BTreeMap; remove or stop exposing get_mut and instead provide targeted registry mutators such as: (1) an update method like update(&mut self, key: &WriterHeadKey, f: impl FnOnce(&mut WriterHead)) that passes a &mut WriterHead into a closure but does not allow replacing the whole value, and (2) a safe replace method replace_checked(&mut self, key: &WriterHeadKey, new_head: WriterHead) that asserts new_head.key() == key before swapping. Apply these changes to the struct that owns heads (the BTreeMap named heads) and remove public get_mut to prevent callers from assigning a different WriterHead with a mismatched key.
138-145:⚠️ Potential issue | 🔴 Critical
unpause(PlaybackMode::Paused)still breaks the pause invariant in release builds.The
debug_assert!disappears outside debug builds, so safe callers can producepaused == falsewhilemode == PlaybackMode::Paused.RunnableWriterSet::rebuild()keys offis_paused(), so that head becomes schedulable even though its mode says paused. Either makemodethe single source of truth or rejectPausedin all builds via a real API contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/head.rs` around lines 138 - 145, The current unpause(&mut self, mode: PlaybackMode) uses debug_assert! so in release builds callers can pass PlaybackMode::Paused and produce paused == false while mode == Paused, breaking invariants checked by RunnableWriterSet::rebuild(); fix by enforcing the contract in all builds: in the unpause method (and any callers) reject PlaybackMode::Paused unconditionally—either replace debug_assert! with a hard assert! that panics in release builds or change unpause to return Result and return an Err when mode == PlaybackMode::Paused; ensure the fields self.paused and self.mode remain consistent (or alternatively make mode the single source of truth and remove the separate paused flag and update is_paused()/rebuild() accordingly) and update callers of unpause and related invariants to match the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/warp-core/src/head.rs`:
- Around line 34-36: HeadId's inner Hash must be made private to enforce
construction via make_head_id(); change pub struct HeadId(pub Hash) to a
private-field tuple/struct (e.g., struct HeadId(Hash)) and add a public
as_bytes() accessor to expose raw bytes, add pub(crate) const MIN and MAX (e.g.,
MIN = [0u8;32], MAX = [0xff;32]) on HeadId for internal use, update callers such
as heads_for_worldline() to use HeadId::MIN / HeadId::MAX instead of fabricating
values, and add rustdoc comments to HeadId and make_head_id() describing intent
and invariants so the public API remains opaque and documented.
---
Duplicate comments:
In `@crates/warp-core/src/head.rs`:
- Around line 185-187: get_mut exposes a &mut WriterHead allowing callers to
change the head's key and desynchronize the BTreeMap; remove or stop exposing
get_mut and instead provide targeted registry mutators such as: (1) an update
method like update(&mut self, key: &WriterHeadKey, f: impl FnOnce(&mut
WriterHead)) that passes a &mut WriterHead into a closure but does not allow
replacing the whole value, and (2) a safe replace method replace_checked(&mut
self, key: &WriterHeadKey, new_head: WriterHead) that asserts new_head.key() ==
key before swapping. Apply these changes to the struct that owns heads (the
BTreeMap named heads) and remove public get_mut to prevent callers from
assigning a different WriterHead with a mismatched key.
- Around line 138-145: The current unpause(&mut self, mode: PlaybackMode) uses
debug_assert! so in release builds callers can pass PlaybackMode::Paused and
produce paused == false while mode == Paused, breaking invariants checked by
RunnableWriterSet::rebuild(); fix by enforcing the contract in all builds: in
the unpause method (and any callers) reject PlaybackMode::Paused
unconditionally—either replace debug_assert! with a hard assert! that panics in
release builds or change unpause to return Result and return an Err when mode ==
PlaybackMode::Paused; ensure the fields self.paused and self.mode remain
consistent (or alternatively make mode the single source of truth and remove the
separate paused flag and update is_paused()/rebuild() accordingly) and update
callers of unpause and related invariants to match the chosen approach.
In `@docs/plans/adr-0008-and-0009.md`:
- Around line 997-1001: The verification matrix currently lists weaker commands
than the enforced CI: replace `cargo test --workspace --tests` with the full
`cargo test` expectation and add the deterministic-crate compile requirement by
documenting that `RUSTFLAGS="-Dwarnings"` must be set for determinism-critical
crates; explicitly list the determinism-critical crates (`warp-core`,
`echo-wasm-abi`, `echo-scene-port`) and require they be built/checked with
`RUSTFLAGS="-Dwarnings"` as part of steps 1–5 so the checklist matches the gate
and CI behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e340b428-df71-4007-947d-945e930ba55b
📒 Files selected for processing (4)
CHANGELOG.mdcrates/warp-core/src/head.rscrates/warp-core/tests/invariant_property_tests.rsdocs/plans/adr-0008-and-0009.md
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/warp-core/src/head.rs (1)
34-36:⚠️ Potential issue | 🟠 Major
HeadIdinner is still public — contradicts "opaque" rustdoc.
pub struct HeadId(pub Hash)lets callers bypassmake_head_id()withHeadId([arbitrary; 32]). The rustdoc claims "Opaque stable identifier" but the type is fully constructible externally.Make the inner private and add min/max constants for
heads_for_worldline:-pub struct HeadId(pub Hash); +pub struct HeadId(Hash); impl HeadId { + /// Minimum key for range queries. + pub(crate) const MIN: Self = Self([0u8; 32]); + /// Maximum key for range queries. + pub(crate) const MAX: Self = Self([0xff; 32]); + #[must_use] pub fn as_bytes(&self) -> &Hash { &self.0 } }Then fix
heads_for_worldline:let start = WriterHeadKey { worldline_id, - head_id: HeadId([0u8; 32]), + head_id: HeadId::MIN, }; let end = WriterHeadKey { worldline_id, - head_id: HeadId([0xff; 32]), + head_id: HeadId::MAX, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/head.rs` around lines 34 - 36, The HeadId type is currently transparent and publicly constructible; change pub struct HeadId(pub Hash) to hide the inner Hash (e.g., pub struct HeadId(Hash)) and update/keep make_head_id(...) as the only constructor; add public constants HEAD_ID_MIN and HEAD_ID_MAX (or similarly named) of type HeadId to represent the inclusive bounds used by heads_for_worldline, then update heads_for_worldline(worldline: u64, ...) to compute and use those min/max HeadId constants instead of directly constructing HeadId from arbitrary bytes so callers cannot bypass make_head_id and the function uses the intended stable range.docs/plans/adr-0008-and-0009.md (1)
337-345:⚠️ Potential issue | 🟡 MinorPhase 1 pseudocode still shows public fields — inconsistent with §Core Runtime Shape.
Lines 138–144 correctly show private fields with getters (
fn state(&self) -> &WorldlineState), but this Phase 1 block still showspub state,pub frontier_tick. If implementers follow Phase 1 literally, they'll expose what the Core Runtime Shape explicitly hides.Sync the pseudocode:
pub struct WorldlineState { - pub warp_state: WarpState, + warp_state: WarpState, + // pub fn warp_state(&self) -> &WarpState } pub struct WorldlineFrontier { pub worldline_id: WorldlineId, - pub state: WorldlineState, - pub frontier_tick: u64, // typed in Phase 6 + state: WorldlineState, // pub(crate); use state() getter + frontier_tick: u64, // pub(crate); use frontier_tick() getter }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/adr-0008-and-0009.md` around lines 337 - 345, The Phase 1 pseudocode exposes struct fields publicly which conflicts with the Core Runtime Shape; update the WorldlineState and WorldlineFrontier definitions to make fields private and add accessor methods matching the rest of the doc—e.g., implement pub struct WorldlineState { warp_state: WarpState } and pub struct WorldlineFrontier { worldline_id: WorldlineId, state: WorldlineState, frontier_tick: u64 } with corresponding getters like fn worldline_id(&self) -> &WorldlineId, fn state(&self) -> &WorldlineState, and fn frontier_tick(&self) -> u64 so the pseudocode aligns with the private-field + getter pattern already shown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/warp-core/src/head.rs`:
- Around line 34-36: The HeadId type is currently transparent and publicly
constructible; change pub struct HeadId(pub Hash) to hide the inner Hash (e.g.,
pub struct HeadId(Hash)) and update/keep make_head_id(...) as the only
constructor; add public constants HEAD_ID_MIN and HEAD_ID_MAX (or similarly
named) of type HeadId to represent the inclusive bounds used by
heads_for_worldline, then update heads_for_worldline(worldline: u64, ...) to
compute and use those min/max HeadId constants instead of directly constructing
HeadId from arbitrary bytes so callers cannot bypass make_head_id and the
function uses the intended stable range.
In `@docs/plans/adr-0008-and-0009.md`:
- Around line 337-345: The Phase 1 pseudocode exposes struct fields publicly
which conflicts with the Core Runtime Shape; update the WorldlineState and
WorldlineFrontier definitions to make fields private and add accessor methods
matching the rest of the doc—e.g., implement pub struct WorldlineState {
warp_state: WarpState } and pub struct WorldlineFrontier { worldline_id:
WorldlineId, state: WorldlineState, frontier_tick: u64 } with corresponding
getters like fn worldline_id(&self) -> &WorldlineId, fn state(&self) ->
&WorldlineState, and fn frontier_tick(&self) -> u64 so the pseudocode aligns
with the private-field + getter pattern already shown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 63d88fb6-6202-4697-9122-6292ee1c28b3
📒 Files selected for processing (4)
CHANGELOG.mdcrates/warp-core/src/head.rscrates/warp-core/tests/invariant_property_tests.rsdocs/plans/adr-0008-and-0009.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (6)
docs/adr/ADR-0008-Worldline-Runtime-Model.md (1)
177-182:⚠️ Potential issue | 🟡 MinorStep 3 should name
HeadInbox, notIntentInbox.
crates/warp-core/src/head_inbox.rsintroducedHeadInbox; there is noIntentInboxtype in this PR. Leaving the normative step list on the old symbol makes the ADR harder to map back to the implementation.Suggested correction
-| 3 | Per-writer-head `IntentInbox` policy | Implemented in `head_inbox.rs` + runtime ingest | +| 3 | Per-writer-head `HeadInbox` policy | Implemented in `head_inbox.rs` + runtime ingest |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/ADR-0008-Worldline-Runtime-Model.md` around lines 177 - 182, Update the ADR step text to reference the actual symbol introduced by the implementation: replace the mention of `IntentInbox` with `HeadInbox` in Step 3 so the ADR maps to the real type; ensure the line reads something like "Per-writer-head `HeadInbox` policy" (reference: HeadInbox in head_inbox.rs) and keep the rest of the step unchanged.crates/warp-core/src/coordinator.rs (3)
103-115:⚠️ Potential issue | 🟠 MajorHide the mutable registries or rebuild routing state before use.
default_writersandpublic_inboxesare derived indexes, butworldlines,heads, andrunnableare public mutable fields. A caller can replaceruntime.headsorruntime.worldlinesdirectly and leave routing state stale, after whichingest()starts returningUnknownHead/MissingDefaultWriterfor a runtime shape the public API itself allowed.Minimal containment direction
pub struct WorldlineRuntime { - pub worldlines: WorldlineRegistry, - pub heads: PlaybackHeadRegistry, - pub runnable: RunnableWriterSet, - pub global_tick: u64, + worldlines: WorldlineRegistry, + heads: PlaybackHeadRegistry, + runnable: RunnableWriterSet, + global_tick: u64, /// Deterministic route table for default writers. default_writers: BTreeMap<WorldlineId, WriterHeadKey>, /// Deterministic route table for named public inboxes. public_inboxes: BTreeMap<(WorldlineId, InboxAddress), WriterHeadKey>, }Expose read-only accessors for inspection and keep all mutations funneled through
register_worldline(),register_writer_head(), and scheduler/runtime methods that can maintain the derived indexes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/coordinator.rs` around lines 103 - 115, Make default_writers and public_inboxes safe from external mutation by preventing callers from replacing worldlines/heads/runnable or by rebuilding routing state before use: make worldlines, heads, and runnable private (or expose only read-only accessors) on WorldlineRuntime and funnel all mutations through register_worldline() and register_writer_head() (and scheduler/runtime methods) so those functions update default_writers and public_inboxes atomically; alternatively, ensure ingest() always recomputes or validates default_writers/public_inboxes from the current worldlines/heads/runnable before using them to avoid UnknownHead / MissingDefaultWriter for a shape the public API permitted.
287-357:⚠️ Potential issue | 🔴 CriticalReject tick overflow before mutating runtime state.
FrontierTickOverflowandGlobalTickOverfloware checked aftercommit_with_state()has already run, and frontier overflow is checked afterrecord_committed_ingress()as well. That letssuper_tick()returnErrwith a drained inbox, mutated worldline state, and ingress ids permanently marked committed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/coordinator.rs` around lines 287 - 357, In super_tick, prevent committing or mutating worldline/inbox state before validating tick overflows: before calling engine.commit_with_state or recording committed ingress, check that the target frontier tick (e.g., frontier.state().tick.checked_add(1) or a peek_next_tick helper) will not overflow and return FrontierTickOverflow if it would; if the frontier is missing, requeue as you already do. Also ensure runtime.global_tick.checked_add(1) is validated (and return GlobalTickOverflow) before applying any mutations that would be irreversible (so requeue admitted envelopes and avoid calling commit_with_state when either overflow check fails). Reference symbols: super_tick, commit_with_state, frontier.advance_tick / frontier.state().record_committed_ingress, and runtime.global_tick.checked_add.
112-115: 🧹 Nitpick | 🔵 TrivialNamed-route resolution still allocates on the ingress hot path.
resolve_target()clonesInboxAddressjust to probepublic_inboxes, because the table is keyed by(WorldlineId, InboxAddress). This is now the live ingress path; every named inbox dispatch pays an avoidable string allocation before dedup or policy checks.Allocation-free lookup direction
- public_inboxes: BTreeMap<(WorldlineId, InboxAddress), WriterHeadKey>, + public_inboxes: BTreeMap<WorldlineId, BTreeMap<InboxAddress, WriterHeadKey>>,Then resolve in two steps so the lookup can borrow
&InboxAddressinstead of cloning it.Also applies to: 231-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/coordinator.rs` around lines 112 - 115, The code allocates by cloning InboxAddress when probing public_inboxes in resolve_target; fix by changing public_inboxes from BTreeMap<(WorldlineId, InboxAddress), WriterHeadKey> to a nested map BTreeMap<WorldlineId, BTreeMap<InboxAddress, WriterHeadKey>> and update resolve_target to first lookup the outer map with &WorldlineId and then inner.get(&InboxAddress) so the InboxAddress can be borrowed (no clone) during the hot ingress path; update any uses of public_inboxes, the map construction, and tests to the nested-map shape and ensure types WriterHeadKey, WorldlineId and InboxAddress are used by reference in the new lookups.CHANGELOG.md (1)
143-145:⚠️ Potential issue | 🟡 MinorDo not describe the whole invariant file as
proptestcoverage.This bullet still reads as if every invariant in
invariant_property_tests.rsis property-based, but the file mixes proptests with fixed regression tests. Narrow the wording so readers do not assume the entire suite is fuzzed.Suggested wording
-- **Added** property test suite (`invariant_property_tests.rs`) enforcing - monotonic worldline ticks, idempotent ingress, cross-worldline isolation, - commit determinism, and provenance immutability via `proptest`. +- **Added** invariant test suite (`invariant_property_tests.rs`) enforcing + monotonic worldline ticks, idempotent ingress, cross-worldline isolation, + commit determinism, and provenance immutability, with property-based checks + where `proptest` is used and fixed regression tests for the remaining cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 143 - 145, The changelog bullet misleadingly implies every test in invariant_property_tests.rs is a property-based (proptest) test; update the bullet to clarify that the file contains a mix of property tests and fixed regression tests by replacing "enforcing ... via `proptest`" with wording such as "includes both property-based tests (using `proptest`) and fixed regression tests to cover monotonic worldline ticks, idempotent ingress, cross-worldline isolation, commit determinism, and provenance immutability" and reference the test suite name invariant_property_tests.rs so readers know the file contains mixed-test styles.crates/warp-core/tests/invariant_property_tests.rs (1)
314-316:⚠️ Potential issue | 🟠 MajorINV-005 still leaves deterministic metadata unchecked.
This only compares
hash,state_root, andpatch_digest. If scheduler ordering or receipt metadata drifts while those three stay stable, the property still passes. Assert the rest of the snapshot digest surface as well — at minimumplan_digest,decision_digest, andrewrites_digest— or switch to the commit API that exposes receipt/patch digests and compare those too.As per coding guidelines:
crates/warp-core/**: warp-core is the deterministic kernel. Every code path must be deterministic (no HashMap iteration order, no floating-point platform variance, no thread-dependent ordering).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/tests/invariant_property_tests.rs` around lines 314 - 316, The test currently only compares snap1.hash, snap1.state_root, and snap1.patch_digest which misses other deterministic metadata; update the assertions to also compare snap1.plan_digest, snap1.decision_digest, and snap1.rewrites_digest (i.e., add prop_assert_eq!(snap1.plan_digest, snap2.plan_digest); prop_assert_eq!(snap1.decision_digest, snap2.decision_digest); prop_assert_eq!(snap1.rewrites_digest, snap2.rewrites_digest);), or alternatively change the test to use the commit API that returns receipt/patch digests and assert equality on those full receipt/patch digest fields instead of the limited snapshot fields so scheduler ordering and receipt metadata are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/warp-core/src/worldline_registry.rs`:
- Around line 112-114: The public method iter_mut() on WorldlineRegistry exposes
&mut WorldlineFrontier to downstream code; make it non-public so mutation goes
through the existing crate-local seam frontier_mut(). Change the visibility of
iter_mut (the function named iter_mut in worldline_registry.rs that returns impl
Iterator<Item = (&WorldlineId, &mut WorldlineFrontier)>) from pub to a
restricted visibility (remove pub or use pub(crate)) or remove it entirely so
only frontier_mut() remains the crate-local mutation entrypoint.
In `@crates/warp-core/tests/invariant_property_tests.rs`:
- Around line 170-176: The test currently exercises Engine::ingest_intent (via
ingest_intent on the Engine produced by EngineBuilder) which only validates the
compatibility ingress path; update the test to drive the production ingress by
calling WorldlineRuntime::ingest (or WorldlineRuntime::ingest(...) on an
instantiated runtime) instead of Engine::ingest_intent so the runtime-owned
routing and per-head idempotence are exercised, or add a new sibling property
test that performs the same assertions using WorldlineRuntime::ingest; locate
uses of EngineBuilder/engine.ingest_intent and replace or duplicate them with
constructing the WorldlineRuntime and invoking WorldlineRuntime::ingest with the
same intent_bytes and assertions.
- Around line 148-153: The test currently derives its oracle by sorting expected
via WriterHeadKey::Ord which couples the test to the same comparator as the
implementation; change the construction of expected so you sort/deduplicate by
the concrete fields instead of calling sort() on WriterHeadKey: specifically,
after cloning keys into expected, sort expected using a comparator that compares
tuple (worldline_id, head_id) extracted from each WriterHeadKey (or map to that
tuple then sort), then dedup, and finally compare to result (from
runnable.iter().copied().collect()) so the property is independent of
WriterHeadKey::Ord.
- Around line 264-283: The test only checks provenance.len(worldline_b) after
appending to worldline_a, which misses in-place corruption of B's last
HashTriplet; before calling provenance.append(worldline_a, ...), capture a copy
of B's last committed triplet (e.g., let before_b =
provenance.get(worldline_b).last().cloned() or retrieve the last HashTriplet via
whatever API returns triplets), perform the append using provenance.append(...),
then assert equality between the saved before_b and the current last triplet for
worldline_b (compare the HashTriplet fields state_root, patch_digest,
commit_hash) in addition to the existing length check to ensure B was not
mutated in-place.
In `@docs/plans/adr-0008-and-0009.md`:
- Around line 284-291: The "Golden vector suite" in the Phase 0 deliverables
lists items that belong to later phases (application-message idempotence,
transport state convergence, explicit conflict artifact recording); update the
document so Phase 0 does not claim ownership of vectors for semantics introduced
in Phases 9C/10/11 — either remove those bullets from the Phase 0 list or move
them to the corresponding phase entries, or change the wording to "define
requirements" for Phase 0 and defer creation of golden vectors to the Phase
9C/10/11 sections; specifically edit the "Golden vector suite" bullet list (the
lines mentioning single-head single-worldline commit determinism,
multi-worldline scheduling order, provenance replay integrity, fork
reproducibility, application-message idempotence, transport state convergence,
explicit conflict artifact recording) and align each item with the phase that
introduces its subsystem (9C, 10, 11) or mark them as future-phase deliverables.
- Around line 201-206: The plan's pseudocode re-exposes IngressEnvelope fields
as public which breaks the runtime invariant; change the IngressEnvelope
definition to make all fields private (ingress_id, target, causal_parents,
payload) and provide a constructor (e.g., new or from_boundary) plus read-only
accessors for each field instead of public fields, matching the hardened API
enforced in crates/warp-core/src/head_inbox.rs where the constructor
validates/derives the canonical ingress_id at the boundary; ensure callers can
only create envelopes via that constructor so forging a mismatched content hash
is impossible.
In `@scripts/hooks/README.md`:
- Around line 5-7: Replace the author-local absolute path in the README (the
string "/Users/james/git/echo/.githooks") with a repo-relative path ".githooks/"
so the canonical hooks location is documented portably; keep the `make hooks`
installation instructions and the reference to configuring `core.hooksPath`
unchanged (edit the README content referencing the path to use ".githooks/"
instead of the absolute filesystem path).
---
Duplicate comments:
In `@CHANGELOG.md`:
- Around line 143-145: The changelog bullet misleadingly implies every test in
invariant_property_tests.rs is a property-based (proptest) test; update the
bullet to clarify that the file contains a mix of property tests and fixed
regression tests by replacing "enforcing ... via `proptest`" with wording such
as "includes both property-based tests (using `proptest`) and fixed regression
tests to cover monotonic worldline ticks, idempotent ingress, cross-worldline
isolation, commit determinism, and provenance immutability" and reference the
test suite name invariant_property_tests.rs so readers know the file contains
mixed-test styles.
In `@crates/warp-core/src/coordinator.rs`:
- Around line 103-115: Make default_writers and public_inboxes safe from
external mutation by preventing callers from replacing worldlines/heads/runnable
or by rebuilding routing state before use: make worldlines, heads, and runnable
private (or expose only read-only accessors) on WorldlineRuntime and funnel all
mutations through register_worldline() and register_writer_head() (and
scheduler/runtime methods) so those functions update default_writers and
public_inboxes atomically; alternatively, ensure ingest() always recomputes or
validates default_writers/public_inboxes from the current
worldlines/heads/runnable before using them to avoid UnknownHead /
MissingDefaultWriter for a shape the public API permitted.
- Around line 287-357: In super_tick, prevent committing or mutating
worldline/inbox state before validating tick overflows: before calling
engine.commit_with_state or recording committed ingress, check that the target
frontier tick (e.g., frontier.state().tick.checked_add(1) or a peek_next_tick
helper) will not overflow and return FrontierTickOverflow if it would; if the
frontier is missing, requeue as you already do. Also ensure
runtime.global_tick.checked_add(1) is validated (and return GlobalTickOverflow)
before applying any mutations that would be irreversible (so requeue admitted
envelopes and avoid calling commit_with_state when either overflow check fails).
Reference symbols: super_tick, commit_with_state, frontier.advance_tick /
frontier.state().record_committed_ingress, and runtime.global_tick.checked_add.
- Around line 112-115: The code allocates by cloning InboxAddress when probing
public_inboxes in resolve_target; fix by changing public_inboxes from
BTreeMap<(WorldlineId, InboxAddress), WriterHeadKey> to a nested map
BTreeMap<WorldlineId, BTreeMap<InboxAddress, WriterHeadKey>> and update
resolve_target to first lookup the outer map with &WorldlineId and then
inner.get(&InboxAddress) so the InboxAddress can be borrowed (no clone) during
the hot ingress path; update any uses of public_inboxes, the map construction,
and tests to the nested-map shape and ensure types WriterHeadKey, WorldlineId
and InboxAddress are used by reference in the new lookups.
In `@crates/warp-core/tests/invariant_property_tests.rs`:
- Around line 314-316: The test currently only compares snap1.hash,
snap1.state_root, and snap1.patch_digest which misses other deterministic
metadata; update the assertions to also compare snap1.plan_digest,
snap1.decision_digest, and snap1.rewrites_digest (i.e., add
prop_assert_eq!(snap1.plan_digest, snap2.plan_digest);
prop_assert_eq!(snap1.decision_digest, snap2.decision_digest);
prop_assert_eq!(snap1.rewrites_digest, snap2.rewrites_digest);), or
alternatively change the test to use the commit API that returns receipt/patch
digests and assert equality on those full receipt/patch digest fields instead of
the limited snapshot fields so scheduler ordering and receipt metadata are
covered.
In `@docs/adr/ADR-0008-Worldline-Runtime-Model.md`:
- Around line 177-182: Update the ADR step text to reference the actual symbol
introduced by the implementation: replace the mention of `IntentInbox` with
`HeadInbox` in Step 3 so the ADR maps to the real type; ensure the line reads
something like "Per-writer-head `HeadInbox` policy" (reference: HeadInbox in
head_inbox.rs) and keep the rest of the step unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d11fc61-872a-40fb-9ba7-198fd160289b
⛔ Files ignored due to path filters (1)
docs/archive/AGENTS.mdis excluded by!docs/archive/**
📒 Files selected for processing (11)
CHANGELOG.mdcrates/warp-core/src/coordinator.rscrates/warp-core/src/worldline_registry.rscrates/warp-core/tests/golden_vectors_phase0.rscrates/warp-core/tests/invariant_property_tests.rsdocs/adr/ADR-0008-Worldline-Runtime-Model.mddocs/adr/adr-exceptions.mddocs/plans/adr-0008-and-0009.mdscripts/hooks/README.mdscripts/hooks/pre-commitscripts/hooks/pre-push
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/warp-core/src/worldline_registry.rs (1)
112-114: 🛠️ Refactor suggestion | 🟠 MajorRestrict
iter_mut()to crate scope.
frontier_mut()is already crate-private, which makes frontier mutation look intentionally runtime-owned.iter_mut()re-exposes&mut WorldlineFrontieracross the public API for every worldline and bypasses that boundary.Minimal containment change
- pub fn iter_mut(&mut self) -> impl Iterator<Item = (&WorldlineId, &mut WorldlineFrontier)> { + pub(crate) fn iter_mut(&mut self) -> impl Iterator<Item = (&WorldlineId, &mut WorldlineFrontier)> { self.worldlines.iter_mut() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/worldline_registry.rs` around lines 112 - 114, iter_mut() currently exposes &mut WorldlineFrontier publicly, bypassing the intended crate-only mutation boundary enforced by frontier_mut(); change iter_mut() to crate-only by making its visibility restricted (e.g., pub(crate) or equivalent) so mutable frontier references are not exposed in the public API, and update any internal callers to use the now-restricted iter_mut() or frontier_mut() on WorldlineFrontier as appropriate; locate the function signature for iter_mut (returning impl Iterator<Item = (&WorldlineId, &mut WorldlineFrontier)>) and adjust its visibility, ensuring consumers outside the crate are not relying on it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/warp-core/src/worldline_registry.rs`:
- Around line 112-114: iter_mut() currently exposes &mut WorldlineFrontier
publicly, bypassing the intended crate-only mutation boundary enforced by
frontier_mut(); change iter_mut() to crate-only by making its visibility
restricted (e.g., pub(crate) or equivalent) so mutable frontier references are
not exposed in the public API, and update any internal callers to use the
now-restricted iter_mut() or frontier_mut() on WorldlineFrontier as appropriate;
locate the function signature for iter_mut (returning impl Iterator<Item =
(&WorldlineId, &mut WorldlineFrontier)>) and adjust its visibility, ensuring
consumers outside the crate are not relying on it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 91847a2e-5d72-4825-bbe8-1bb62ea5e97f
📒 Files selected for processing (1)
crates/warp-core/src/worldline_registry.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/ROADMAP/backlog/README.md`:
- Line 6: The estimated total in the header ("**Priority:** Unscheduled |
**Est:** ~187h") is inconsistent with the sum of the backlog table rows
(36+25+23+14+14+2+11+20+23+12+10+19 = 209h); update either the header estimate
to "~209h" or correct any individual line item(s) so their sum matches the
header. Locate the header string and the backlog table entries in
docs/ROADMAP/backlog/README.md, recalculate the total from the listed items, and
make the header and table consistent (also add a short inline note or comment if
any item was intentionally rounded or omitted).
In `@docs/ROADMAP/backlog/tooling-misc.md`:
- Around line 145-148: The roadmap entry claiming "match CI exactly" is
ambiguous; update the text to either (A) specify the full local parity command
and env vars — include RUSTDOCFLAGS="-D warnings" for rustdoc,
RUSTFLAGS="-Dwarnings" for compile-time policies on determinism-critical crates
(warp-core, echo-wasm-abi, echo-scene-port), and the required pre-PR commands
(cargo clippy --all-targets -- -D missing_docs and cargo test) — or (B) narrow
the scope to explicitly state it's only enforcing rustdoc warnings; also ensure
R1–R4 and the note at lines 168–170 consistently reflect whichever scope you
choose so local instructions match CI behavior.
- Around line 208-213: Update the Test Plan to explicitly require running the
DIND harness: change the "Goldens" line to state that the DIND (Deterministic
Ironclad Nightmare Drills) golden hash-chain harness must be used to verify
changes against canonical golden outputs (e.g., "Use the DIND golden hash-chain
harness to verify any changes against golden hash chains"), and add a note that
determinism/property tests must include DIND runs so helper changes cannot
silently alter canonical outputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a86c0a29-cf5a-4695-a5dc-7e1ae4510ea4
📒 Files selected for processing (2)
docs/ROADMAP/backlog/README.mddocs/ROADMAP/backlog/tooling-misc.md
Owner
Author
|
Addressed on Clarifications for the two remaining design-shaped suggestions:
@coderabbitai review please 🥕 |
Contributor
|
Recording both as learnings and triggering review on ✏️ Learnings added
🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (3)
crates/warp-core/src/engine_impl.rs (1)
998-998:⚠️ Potential issue | 🟠 MajorAvoid the second full-state clone on the live commit path.
Line 998 takes another full
WarpStateclone, and Lines 1037-1044 only use it to rebuildcombined_patch.commit_with_receipt()already clones pre-state once, so runtime commits now pay two O(state) copies per tick on the live ingress path. Reuse a single pre-state snapshot or record runtime-ingress materialization asWarpOps so this stays delta-sized instead of state-sized.As per coding guidelines
crates/warp-core/**: "Performance matters — flag unnecessary allocations in hot paths."Also applies to: 1037-1044
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/engine_impl.rs` at line 998, The extra full-state clone at state_before_runtime_commit is unnecessary — avoid the second O(state) copy by reusing the pre-state snapshot already created by commit_with_receipt (or by changing commit_with_receipt to return the pre-state or the minimal WarpOp materialization instead of forcing a fresh clone); update the code that builds combined_patch (currently using state_before_runtime_commit) to use that returned snapshot or the WarpOp delta so runtime commits remain delta-sized; in short, remove the clone in state_before_runtime_commit, retrieve the pre-state or ops from commit_with_receipt (or add a small API change on commit_with_receipt to return it), and reconstruct combined_patch from that reused snapshot/ops rather than cloning WarpState again.crates/warp-core/src/coordinator.rs (1)
319-335:⚠️ Potential issue | 🔴 CriticalPreflight frontier capacity per worldline, not per head.
Line 332 only rejects
frontier_tick == u64::MAX. If one worldline is atu64::MAX - 1and two runnable heads on that worldline can admit, both heads pass preflight, the first commit advances toMAX, and the second head still mutates state beforeadvance_tick()fails later. Count planned commits per worldline up front and preflightchecked_add(planned_advances)before anyadmit()/commit_with_state()call; add a regression test for the two-head same-worldline overflow case.Possible fix
let mut records = Vec::new(); let keys: Vec<WriterHeadKey> = runtime.runnable.iter().copied().collect(); + let mut planned_advances = BTreeMap::<WorldlineId, u64>::new(); for key in &keys { let head = runtime .heads .get(key) .ok_or(RuntimeError::UnknownHead(*key))?; if !head.inbox().can_admit() { continue; } - let frontier = runtime - .worldlines - .get(&key.worldline_id) - .ok_or(RuntimeError::UnknownWorldline(key.worldline_id))?; - if frontier.frontier_tick() == u64::MAX { - return Err(RuntimeError::FrontierTickOverflow(key.worldline_id)); - } + let planned = planned_advances.entry(key.worldline_id).or_insert(0); + *planned = planned + .checked_add(1) + .ok_or(RuntimeError::FrontierTickOverflow(key.worldline_id))?; + } + + for (worldline_id, planned) in &planned_advances { + let frontier = runtime + .worldlines + .get(worldline_id) + .ok_or(RuntimeError::UnknownWorldline(*worldline_id))?; + frontier + .frontier_tick() + .checked_add(*planned) + .ok_or(RuntimeError::FrontierTickOverflow(*worldline_id))?; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/coordinator.rs` around lines 319 - 335, Preflight per-worldline capacity instead of per-head: before calling head.inbox().can_admit() and before any admit()/commit_with_state() paths, iterate over the candidate `keys` and accumulate a map of planned advances per `key.worldline_id`; for each worldline from `runtime.worldlines.get(&worldline_id)` check `frontier.frontier_tick().checked_add(planned_advances).ok_or(RuntimeError::FrontierTickOverflow(worldline_id))` and fail early if overflow would occur; only after this global preflight allow individual `head.inbox().can_admit()`/`admit()`/`commit_with_state()` to proceed; add a regression test exercising two runnable heads on the same worldline where frontier is `u64::MAX - 1` to ensure the second commit is rejected at preflight.crates/warp-core/src/head_inbox.rs (1)
343-357: 🛠️ Refactor suggestion | 🟠 MajorMove budgeted admissions out of
pendinginstead of cloning them.This branch still clones every admitted
IngressEnvelopeand then does a second removal pass. On the live ingress path that doubles payload copying and extraBTreeMapwork per tick for no functional gain; select the firstlimitids deterministically, thenremove()those entries and move the envelopes intoadmitted. As per coding guidelines,warp-coreis the deterministic kernel and performance matters on hot paths.Possible fix
InboxPolicy::Budgeted { max_per_tick } => { let limit = *max_per_tick as usize; let reserve = limit.min(self.pending.len()); - let mut admitted = Vec::with_capacity(reserve); - let mut to_remove = Vec::with_capacity(reserve); - for (id, env) in &self.pending { - if admitted.len() >= limit { - break; - } - admitted.push(env.clone()); - to_remove.push(*id); - } - for id in to_remove { - self.pending.remove(&id); - } + let to_remove: Vec<_> = self.pending.keys().copied().take(limit).collect(); + let mut admitted = Vec::with_capacity(reserve); + for id in to_remove { + if let Some(env) = self.pending.remove(&id) { + admitted.push(env); + } + } admitted }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/head_inbox.rs` around lines 343 - 357, The Budgeted branch currently clones each admitted IngressEnvelope and then does a second removal pass which doubles copies and map work; change InboxPolicy::Budgeted handling to deterministically pick the first up-to-limit keys from self.pending (using the map's iteration order), collect those ids, then for each id call self.pending.remove(&id) and push the returned envelope into admitted so ownership is moved (no clone) and the extra removal loop disappears; update variables used in this block (limit, reserve, admitted, to_remove) so admitted is populated by moved values and to_remove is only a temporary list of ids.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 24-25: Update the changelog entry to correctly describe the layer
where behavior changed: clarify that WorldlineRegistry::register() still returns
bool and that duplicate registrations are surfaced at the runtime surface as
RuntimeError::DuplicateWorldline, rather than implying the registry API now
throws; replace the current sentence about "now fails loudly" with wording that
states the registry keeps its boolean return and the runtime layer raises
RuntimeError::DuplicateWorldline on duplicate registrations, mentioning
WorldlineRegistry::register() and RuntimeError::DuplicateWorldline by name for
clarity.
- Around line 75-76: Update the CHANGELOG entry to remove the incorrect claim
that SchedulerCoordinator::super_tick() auto-refreshes the runnable set and
instead state that runnable ordering is derived via
SchedulerCoordinator::peek_order() from the head registry, with runnable refresh
remaining explicit; replace the line mentioning auto-refreshing in the
super_tick() note with a sentence describing that peek_order() derives canonical
order from the head registry and that explicit runnable refresh is still
required.
In `@crates/warp-core/src/coordinator.rs`:
- Around line 306-309: super_tick currently applies
commits/committed-ingress/frontier advances as it iterates engines and on
encountering a later EngineError it discards the accumulated StepRecord vector
and returns Err while leaving global_tick mutated, causing a silent partial
commit; fix by making the operation atomic or by surfacing partial progress:
either (A) roll back any side-effects on failure (ensure global_tick,
committed-ingress marks, frontier advances are reverted) so super_tick either
fully succeeds or leaves state unchanged, or (B) change the API to return an
explicit result type that includes partial progress (e.g., a SuperTickResult
enum or Result<(Vec<StepRecord>, CommitState), RuntimeError>) and populate it
when some engines succeeded before an EngineError; locate logic in function
super_tick and the error path that touches EngineError, StepRecord accumulation,
and global_tick mutation and implement one of these two approaches consistently
across that function.
- Around line 249-267: In resolve_target, distinguish an unknown worldline from
a missing route by first checking whether the worldline exists before consulting
the route tables: in the IngressTarget::DefaultWriter branch, check
self.default_writers.contains_key(worldline_id) (or a dedicated worldline
registry like self.worldlines) and return
RuntimeError::UnknownWorldline(*worldline_id) if absent, otherwise proceed to
look up and return MissingDefaultWriter if no writer; similarly in
IngressTarget::InboxAddress, check existence of the worldline (e.g.,
self.public_inboxes.contains_key(worldline_id) or the canonical worldline
registry) and return RuntimeError::UnknownWorldline when missing, and only then
attempt the inbox route lookup to return MissingInboxAddress if the inbox entry
is absent.
In `@crates/warp-core/src/engine_impl.rs`:
- Around line 1957-1965: The code re-creates and sorts cmd_rules (Vec<(Hash,
&'static str)>) on every envelope, causing heavy allocations and O(envelopes ×
rules log rules) cost; instead add and maintain a cached canonical_cmd_rules
(e.g. Vec<(Hash, &'static str)>) on the engine struct that is populated/updated
when rules are registered/changed and used here; change this block to iterate
over self.canonical_cmd_rules (no sort/allocation), and update the rule
registration path (where rules are inserted into self.rules) to rebuild or
incrementally maintain canonical_cmd_rules whenever a rule with name starting
with "cmd/" is added/removed/changed.
In `@crates/warp-core/src/worldline_registry.rs`:
- Around line 60-69: In register(), avoid always calling
WorldlineFrontier::new(worldline_id, state) which hardcodes frontier_tick=0;
instead inspect the supplied WorldlineState (tick_history / last_snapshot) and
derive the initial frontier tick (e.g., use state.last_snapshot.tick or the
latest tick in state.tick_history) and construct the frontier with that tick
(add/use a constructor like WorldlineFrontier::from_state or
WorldlineFrontier::with_tick) so frontier_tick() matches the supplied state, or
explicitly reject non-fresh states in register() if you intend to only accept
fresh states.
In `@crates/warp-core/src/worldline_state.rs`:
- Around line 72-85: Change the worldline state constructor to be fallible and
validate the root invariant instead of accepting any NodeKey: replace the
current pub fn new(warp_state: WarpState, root: NodeKey) -> Self with a function
that returns Result<Self, Error> (or a dedicated error type) and verify the
provided WarpState contains exactly one root instance with a backing store;
reject states with zero or multiple parentless instances. Also remove or
refactor the From<WarpState> implementation so callers must either supply an
explicit validated NodeKey or call the new fallible constructor that enforces
the single-root invariant (refer to symbols: new, From<WarpState>, WarpState,
NodeKey). Ensure all callsites are updated to handle the Result and that no code
fabricates a ("root","root") fallback.
In `@crates/warp-wasm/src/warp_kernel.rs`:
- Around line 74-78: The constructor WarpKernel::with_engine should not panic on
a caller-supplied Engine; replace the assert!(engine.is_fresh_runtime_state(),
...) with an early-return Result error: change the signature of
with_engine(engine: Engine, registry: RegistryInfo) to return Result<Self,
WarpKernelError> (or an existing error enum), check
engine.is_fresh_runtime_state() and return
Err(WarpKernelError::InvalidEngineState) (include contextual info if available)
instead of panicking, and update any callers to propagate or map that Result;
keep the rest of the initialization logic the same but gated behind a successful
check.
In `@docs/adr/ADR-0008-Worldline-Runtime-Model.md`:
- Around line 113-115: The wording about deduplication is too broad: clarify
that ingress_id is payload-only and deduplication must be scoped to the resolved
head so identical payloads are allowed once per distinct resolved head; update
the lines mentioning "Intent identity is content-addressed and deduplicated" and
"No direct App or Janus mutation path may bypass intent admission" to state that
intent identity is content-addressed and deduplicated only with respect to a
single resolved head (i.e., repeat deliveries to the same resolved head are
suppressed, but the same payload may be admitted once for each distinct resolved
head).
- Around line 121-132: The documented per-head APIs (seek, jump_to_frontier,
set_mode) are ambiguous because the rest of the ADR uses a composite head key
(worldline_id, head_id); update these API signatures and descriptions to require
the full head key tuple (worldline_id, head_id) instead of a bare head_id, e.g.,
change `seek(head_id, target_tick)` → `seek((worldline_id, head_id),
target_tick)`, `jump_to_frontier(head_id)` → `jump_to_frontier((worldline_id,
head_id))`, and `set_mode(head_id, mode)` → `set_mode((worldline_id, head_id),
mode)`; retain the behavioral notes (observational-only, no frontier
mutation/clamping, etc.) and ensure text clarifies that forks still use
`fork(worldline_id, fork_tick, new_worldline_id)` (worldline-level) while the
other three operate on a per-head composite key.
In `@docs/plans/adr-0008-and-0009.md`:
- Around line 23-27: Update the wording that currently says "Multiple writer
heads per worldline are allowed from Phase 2 onward" to remove the contradiction
with Phase 1 tests: change it to state that multiple writer heads are
"representable in Phase 1, scheduler-active in Phase 2" (keep the surrounding
claims "One frontier state per worldline" and the note about serial canonical
Phase 1 implementation intact) so the document consistently distinguishes
representation vs. scheduler activation.
- Around line 127-132: WorldlineRuntime currently exposes internal registries
and scheduler state (worldlines, heads, runnable, global_tick) as public fields;
make those fields private and replace direct access with runtime-owned accessor
and mutation methods on WorldlineRuntime (e.g., methods like
get_worldline_registry()/register_worldline(), get_head()/update_head(),
enqueue_runnable()/dequeue_runnable(), and
get_global_tick()/advance_global_tick()) so callers interact through controlled
APIs that enforce invariants and scheduling rules; update all call sites to use
these new methods and ensure any mutation remains encapsulated within
WorldlineRuntime.
---
Duplicate comments:
In `@crates/warp-core/src/coordinator.rs`:
- Around line 319-335: Preflight per-worldline capacity instead of per-head:
before calling head.inbox().can_admit() and before any
admit()/commit_with_state() paths, iterate over the candidate `keys` and
accumulate a map of planned advances per `key.worldline_id`; for each worldline
from `runtime.worldlines.get(&worldline_id)` check
`frontier.frontier_tick().checked_add(planned_advances).ok_or(RuntimeError::FrontierTickOverflow(worldline_id))`
and fail early if overflow would occur; only after this global preflight allow
individual `head.inbox().can_admit()`/`admit()`/`commit_with_state()` to
proceed; add a regression test exercising two runnable heads on the same
worldline where frontier is `u64::MAX - 1` to ensure the second commit is
rejected at preflight.
In `@crates/warp-core/src/engine_impl.rs`:
- Line 998: The extra full-state clone at state_before_runtime_commit is
unnecessary — avoid the second O(state) copy by reusing the pre-state snapshot
already created by commit_with_receipt (or by changing commit_with_receipt to
return the pre-state or the minimal WarpOp materialization instead of forcing a
fresh clone); update the code that builds combined_patch (currently using
state_before_runtime_commit) to use that returned snapshot or the WarpOp delta
so runtime commits remain delta-sized; in short, remove the clone in
state_before_runtime_commit, retrieve the pre-state or ops from
commit_with_receipt (or add a small API change on commit_with_receipt to return
it), and reconstruct combined_patch from that reused snapshot/ops rather than
cloning WarpState again.
In `@crates/warp-core/src/head_inbox.rs`:
- Around line 343-357: The Budgeted branch currently clones each admitted
IngressEnvelope and then does a second removal pass which doubles copies and map
work; change InboxPolicy::Budgeted handling to deterministically pick the first
up-to-limit keys from self.pending (using the map's iteration order), collect
those ids, then for each id call self.pending.remove(&id) and push the returned
envelope into admitted so ownership is moved (no clone) and the extra removal
loop disappears; update variables used in this block (limit, reserve, admitted,
to_remove) so admitted is populated by moved values and to_remove is only a
temporary list of ids.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e61ba712-d3f3-4790-86d7-f211400738aa
📒 Files selected for processing (15)
CHANGELOG.mdcrates/warp-core/src/coordinator.rscrates/warp-core/src/engine_impl.rscrates/warp-core/src/head.rscrates/warp-core/src/head_inbox.rscrates/warp-core/src/worldline_registry.rscrates/warp-core/src/worldline_state.rscrates/warp-core/tests/inbox.rscrates/warp-core/tests/invariant_property_tests.rscrates/warp-wasm/src/warp_kernel.rsdocs/ROADMAP/backlog/README.mddocs/ROADMAP/backlog/tooling-misc.mddocs/adr/ADR-0008-Worldline-Runtime-Model.mddocs/plans/adr-0008-and-0009.mdscripts/hooks/README.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/warp-wasm/src/warp_kernel.rs (1)
268-289:⚠️ Potential issue | 🟠 Major
snapshot_at()is off by one against frontier ticks.Line 280 forwards the public tick straight into
Engine::snapshot_at_state(), but that helper is history-indexed:tick_history[0]is the first committed snapshot, whileHeadInfo.tickstarts at0before any commit. After one step,snapshot_at(0)returns the tick-1 snapshot while reportingtick = 0, and the current frontier tick is unreachable. Translate frontier ticks to history indices (0 => genesis,n > 0 => history[n - 1]) before calling the engine helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-wasm/src/warp_kernel.rs` around lines 268 - 289, snapshot_at() currently forwards the public tick directly into Engine::snapshot_at_state() which expects history indices (history[0]=genesis), causing an off-by-one; update the tick-to-history-index translation before calling engine.snapshot_at_state(): compute a history_index where history_index = 0 for tick == 0 and history_index = tick - 1 for tick > 0, then convert that history_index to usize (with the same AbiError on overflow) and pass it into engine.snapshot_at_state(frontier.state(), history_index_usize); keep HeadInfo.tick set to the original tick value and only translate for the engine call (references: snapshot_at, HeadInfo.tick, engine.snapshot_at_state, frontier.state()).
♻️ Duplicate comments (7)
CHANGELOG.md (2)
93-94:⚠️ Potential issue | 🟡 MinorThe
super_tick()auto-refresh note is stale.The final scheduler fix was deriving canonical runnable order from the head registry, not implicitly refreshing
RunnableWriterSetat the start of each tick. This line documents behavior the Phase 3 implementation explicitly moved away from.Suggested wording
-- **Fixed** `SchedulerCoordinator::super_tick()` now auto-refreshes the runnable - set at the start of each SuperTick, eliminating stale-cache scheduling bugs. +- **Fixed** `SchedulerCoordinator::peek_order()` now derives canonical runnable + order from `PlaybackHeadRegistry`, so scheduling no longer depends on stale + runnable-cache state.Based on learnings,
SchedulerCoordinator::peek_order()derives runnable order from the head registry directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 93 - 94, The changelog entry incorrectly claims SchedulerCoordinator::super_tick() auto-refreshes the RunnableWriterSet each SuperTick; update the text to reflect that runnable order is now derived directly from the head registry via SchedulerCoordinator::peek_order() and remove the stale reference to RunnableWriterSet auto-refresh. Mention the new behavior succinctly: "SchedulerCoordinator::peek_order() derives canonical runnable order from the head registry," and delete or replace the sentence referencing RunnableWriterSet and auto-refresh in the release notes.
42-43:⚠️ Potential issue | 🟡 MinorThis entry still names the wrong layer.
WorldlineRegistry::register()still returnsbool; the typed failure is surfaced by the runtime boundary asRuntimeError::DuplicateWorldline. As written, this reads like a breaking registry API change that did not happen.Suggested wording
-- **Fixed** `WorldlineRegistry::register(...)` now fails loudly with a typed - duplicate-worldline error instead of returning an easy-to-ignore boolean. +- **Fixed** duplicate worldline registration now surfaces as + `RuntimeError::DuplicateWorldline` at the runtime boundary instead of being + silently ignored at the call site.Based on learnings,
WorldlineRegistry::register()intentionally returnsbooland duplicate registration is surfaced asRuntimeError::DuplicateWorldlineat the runtime surface layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 42 - 43, The changelog entry incorrectly states that WorldlineRegistry::register(...) now fails with a typed duplicate-worldline error; instead update the text to say that the duplicate registration is now surfaced at the runtime boundary as RuntimeError::DuplicateWorldline while WorldlineRegistry::register() continues to return bool, so rephrase to clarify no breaking change to the registry API and that the runtime surface now emits RuntimeError::DuplicateWorldline.docs/adr/ADR-0008-Worldline-Runtime-Model.md (1)
68-98:⚠️ Potential issue | 🟡 MinorSection 3 still documents the stale runnable-cache path.
Step 1, the pseudocode, and the implementation-plan row all say
super_tick()iterates a maintainedRunnableWriterSet. The shipped Phase 3 coordinator derives canonical order fromPlaybackHeadRegistryviapeek_order()instead. Leaving the ADR onrunnable_writer_set/ “pre-sorted, no filtering” documents the old cache-backed design as normative.Based on learnings,
SchedulerCoordinator::peek_order()derives runnable order from the head registry directly.Also applies to: 183-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/adr/ADR-0008-Worldline-Runtime-Model.md` around lines 68 - 98, Update the ADR to remove the stale "runnable_writer_set" / cache-backed design and describe the current implementation: state that SchedulerCoordinator::peek_order() derives the canonical runnable order directly from PlaybackHeadRegistry (not from a maintained RunnableWriterSet), update the prose in Step 1 and the implementation-plan to reflect this, and replace the pseudocode `for head in runnable_writer_set` with a call that iterates the order produced by SchedulerCoordinator::peek_order()/PlaybackHeadRegistry.peek_order() (not a persisted BTreeSet), and remove or mark deprecated any language asserting a pre-sorted RunnableWriterSet or zero-filter iteration.crates/warp-core/tests/invariant_property_tests.rs (1)
351-370:⚠️ Potential issue | 🟠 MajorINV-005 still bypasses the live runtime path.
This property builds raw
Engineinstances, goes throughEngine::ingest_intent(), and commits directly. A regression inWorldlineRuntime::ingest()/SchedulerCoordinator::super_tick()ordering or per-head commit bookkeeping can ship while this invariant stays green. Drive the same scenario through runtime-owned ingress and compare the resulting frontier snapshot / receipt surface instead.As per coding guidelines,
crates/warp-core/**: warp-core is the deterministic kernel. Every code path must be deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/tests/invariant_property_tests.rs` around lines 351 - 370, The test currently constructs Engine instances and calls Engine::ingest_intent() and commit directly, which bypasses the live runtime path; instead instantiate and run the runtime path (e.g. WorldlineRuntime or the runtime entrypoint that owns ingress/scheduling), call the runtime-owned ingress method (WorldlineRuntime::ingest or the runtime.ingest wrapper) to submit intent bytes, run the scheduler tick (SchedulerCoordinator::super_tick or the runtime API that advances/super-ticks), then collect the frontier snapshot and receipt/patch digests from the runtime’s exported snapshot/receipt surfaces and assert equality between the two independent runtime instances; replace references to EngineBuilder/Engine::ingest_intent/commit_with_receipt (engine1, engine2, tx1, tx2, snap1/snap2, receipt1/receipt2, patch1/patch2) with the runtime equivalents (WorldlineRuntime, runtime.ingest, runtime.super_tick/sync tick, runtime.snapshot/receipt functions) so the test drives the live runtime ingress + scheduler path and compares the same digests.crates/warp-core/src/worldline_state.rs (1)
318-325:⚠️ Potential issue | 🟠 Major
WorldlineFrontier::new()rewinds restored states to tick 0.Line 324 hardcodes
frontier_tick: 0. Any caller wrapping a restoredWorldlineStategetsfrontier_tick() != state.current_tick(), even thoughWorldlineRegistry::register()already had to special-case this exact mismatch. Derive the initial tick from the state here or make this constructor fresh-state-only and enforce that precondition.Minimal fix
#[must_use] pub fn new(worldline_id: WorldlineId, state: WorldlineState) -> Self { - Self { - worldline_id, - state, - frontier_tick: 0, - } + let frontier_tick = state.current_tick(); + Self { + worldline_id, + state, + frontier_tick, + } }Based on learnings
WorldlineRegistry::register(...)now seeds the frontier withstate.current_tick()so restored worldlines keep their committed frontier tick instead of rewinding to tick 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/worldline_state.rs` around lines 318 - 325, WorldlineFrontier::new currently hardcodes frontier_tick: 0 which rewinds restored WorldlineState instances and causes frontier_tick() to differ from state.current_tick(); update WorldlineFrontier::new to initialize frontier_tick from the provided state by using state.current_tick(), or alternatively make the constructor document and enforce that it only accepts fresh states (and update callers like WorldlineRegistry::register accordingly); prefer the first option so restored worldlines keep their committed frontier tick and remove the need for special-casing in WorldlineRegistry::register.crates/warp-core/src/engine_impl.rs (1)
1010-1012: 🛠️ Refactor suggestion | 🟠 MajorLive runtime commits still pay two full-state clones.
Line 1012 clones the swapped-in
WarpState, andcommit_with_receipt()clones again at Line 1178 before applying rewrites. On the live ingress path that makes every admitted batchO(graph size)even when it only adds a single ingress event. Record runtime-ingress materialization as explicitWarpOps, or at least capture only the touched event-node delta, so the combined patch/rollback path stops taking a second whole-state clone.As per coding guidelines
crates/warp-core/**: "Performance matters — flag unnecessary allocations in hot paths."Also applies to: 1051-1058, 1176-1178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/engine_impl.rs` around lines 1010 - 1012, The current hot-path clones the entire WarpState twice (saved_engine_state via std::mem::replace and then state_before_runtime_commit = self.state.clone()) causing O(full-state) cost on every live runtime commit; change the flow so runtime-ingress materialization is recorded as explicit WarpOp(s) (or produce a compact touched-event-node delta) instead of cloning the whole state: modify the code around saved_engine_state / state_before_runtime_commit and the commit_with_receipt(...) call to accept and apply a delta or a Vec<WarpOp> that describes only the mutated nodes/events, update usages where saved_engine_state and state_before_runtime_commit are compared/rolled back to use the delta/WarpOp sequence, and ensure commit_with_receipt and rollback logic apply/undo only those deltas rather than cloning the entire WarpState.crates/warp-core/src/head_inbox.rs (1)
343-358: 🛠️ Refactor suggestion | 🟠 Major
Budgetedbranch still clones envelopes — usepop_first()for zero-copy drain.Past review established that
BTreeMap::pop_first()(stable since Rust 1.66) removes and returns ownership of the smallest-key entry in O(log n). The current two-pass clone+remove pattern is unnecessary overhead on the admission hot path.Proposed fix
InboxPolicy::Budgeted { max_per_tick } => { let limit = *max_per_tick as usize; let reserve = limit.min(self.pending.len()); let mut admitted = Vec::with_capacity(reserve); - let mut to_remove = Vec::with_capacity(reserve); - for (id, env) in &self.pending { - if admitted.len() >= limit { + for _ in 0..limit { + let Some((_, env)) = self.pending.pop_first() else { break; - } - admitted.push(env.clone()); - to_remove.push(*id); - } - for id in to_remove { - self.pending.remove(&id); + }; + admitted.push(env); } admitted }As per coding guidelines: warp-core is the deterministic kernel — flag unnecessary allocations in hot paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/head_inbox.rs` around lines 343 - 358, The Budgeted branch in InboxPolicy::Budgeted currently iterates self.pending and clones Envelope values into admitted then removes keys, causing unnecessary allocations on the hot path; replace this two-pass clone+remove with a loop that repeatedly calls self.pending.pop_first() to take ownership of the smallest-key entries and push them directly into the admitted Vec (respecting limit/max_per_tick) so you drain entries zero-copy and avoid cloning Envelope; update the logic around admitted and to_remove to use pop_first() until admitted.len() >= limit or pop_first() returns None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/warp-core/src/engine_impl.rs`:
- Around line 1013-1014: The rollback path is reconstructing WorldlineState with
the possibly-updated self.current_root instead of the pre-commit root; capture
the original root before mutating self.current_root (use a saved_root or
original_root variable), set self.current_root = state.root for the forward
path, and on the Err/rollback path restore self.current_root = saved_root and
rebuild WorldlineState using saved_root (not self.current_root or state.root).
Update the rollback branch that constructs WorldlineState to reference the
saved_root variable and ensure any subsequent hashing/commit logic uses that
restored root.
In `@crates/warp-core/src/head_inbox.rs`:
- Around line 31-32: IntentKind currently exposes its inner Hash via a public
tuple field, allowing callers to construct IntentKind([0u8; 32]) and bypass
make_intent_kind's domain separation; change IntentKind to have a private inner
field (e.g., struct IntentKind(Hash)) and provide a public accessor method
(e.g., as_hash or inner) if external read access is required, leaving
make_intent_kind as the only public constructor to enforce domain separation.
In `@crates/warp-core/src/lib.rs`:
- Around line 217-225: The crate root is exposing two ingress APIs without
guidance; add rustdoc comments on the re-exports (e.g., the pub use
head_inbox::{HeadInbox, IngressEnvelope, InboxAddress, InboxPolicy,
IngressPayload, IngressTarget, IntentKind, make_intent_kind} and/or the pub use
head::{WriterHead, HeadId, make_head_id, PlaybackHeadRegistry,
RunnableWriterSet, WriterHeadKey}) that clearly mark which API is the
runtime-owned primary surface (phase‑3) and which is the legacy/compatibility
inbox API, include migration notes and deprecation intent for the legacy
surface, and point callers to the preferred types/functions and invariants so
downstreams don’t adopt the wrong API.
In `@crates/warp-core/tests/inbox.rs`:
- Around line 95-143: The test
runtime_ingest_is_idempotent_per_resolved_head_after_commit currently never
verifies post-commit dedupe for the named inbox; after the accepted named_env
branch add a second commit tick and re-ingest named_env to assert it's treated
as a Duplicate for named_key. Concretely: after the existing assertion that
runtime.ingest(named_env.clone()) returns Accepted, call
SchedulerCoordinator::super_tick(&mut runtime, &mut engine).unwrap(); then call
runtime.ingest(named_env).unwrap() and assert it equals
IngressDisposition::Duplicate { ingress_id: named_env.ingress_id(), head_key:
named_key } so the test fails if committed_ingress is not recorded for named
inboxes.
- Around line 17-27: The test helper empty_engine() uses EngineBuilder defaults
which can drift from the live runtime; update empty_engine() to mirror
WarpKernel::new() by explicitly configuring the builder with
SchedulerKind::Radix and workers(1) (i.e., call
EngineBuilder::new(...).scheduler_kind(SchedulerKind::Radix).workers(1).build()),
so the tests run against the same scheduler kind and worker count as the shipped
WASM/runtime path.
In `@crates/warp-wasm/src/lib.rs`:
- Around line 172-185: The init() logic installs a kernel before encoding and
never clears a previously-installed kernel on error; move the
install_kernel(Box::new(kernel)) call so it runs after encode_ok(&head) (i.e.,
encode the success payload first, then install the kernel), and make the error
branch explicit about kernel state by clearing any existing kernel on failure
(call a clear_kernel()/uninstall_kernel() helper or implement one if missing)
instead of leaving a stale kernel; update the branches around
warp_kernel::WarpKernel::new(), get_head(), install_kernel, encode_ok and
encode_err_raw accordingly.
In `@docs/plans/adr-0008-and-0009.md`:
- Around line 371-372: Update the wording to reflect that Phase 2 does not model
the scheduler on the stale runnable cache: replace or remove claims that
"RunnableWriterSet is an ordered live index of runnable writer heads only" and
that loop-driving is from `runnable.iter()`/`super_tick()` and instead state
that canonical runnable order is derived from the head registry at step time and
that `SchedulerCoordinator::peek_order()` computes runnable order directly from
the head registry rather than from `RunnableWriterSet`/`runnable.iter()`; ensure
the document no longer asserts per-head private state stores are introduced and
adjust the referenced paragraphs (including the section around lines 411-423) to
match this behavior.
In `@docs/ROADMAP/backlog/tooling-misc.md`:
- Around line 187-195: The roadmap text treating the single-worker deterministic
helper migration as entirely pending is inaccurate; update the R1–R4 block and
Acceptance Criteria (AC1/AC2) to clearly scope this to remaining work by (a)
changing wording like "Introduce a shared helper..." to "Introduce/migrate
remaining tests to a shared helper" or similar, (b) adding a short explicit list
of what was already completed (e.g., determinism proptests pinned to
single-worker) and what still remains, and (c) adjusting the checklist items so
completed items are checked and only open tasks remain unchecked; edit the R1–R4
entries and AC1/AC2 lines to reflect these concrete done-vs-open labels.
In `@scripts/hooks/README.md`:
- Around line 9-12: Update the README paragraph to explicitly state that the
shim scripts (scripts/hooks/pre-commit and scripts/hooks/pre-push) run an
independent, smaller subset of checks and do not delegate to or mirror the
canonical .githooks/pre-commit and .githooks/pre-push policies; indicate they
can drift over time and therefore manual execution of these shims is not
equivalent to the repository's official CI or git-hook enforcement. Reference
the exact script names (scripts/hooks/pre-commit, scripts/hooks/pre-push) and
the canonical hook paths (.githooks/pre-commit, .githooks/pre-push) in the
wording so contributors understand the difference.
---
Outside diff comments:
In `@crates/warp-wasm/src/warp_kernel.rs`:
- Around line 268-289: snapshot_at() currently forwards the public tick directly
into Engine::snapshot_at_state() which expects history indices
(history[0]=genesis), causing an off-by-one; update the tick-to-history-index
translation before calling engine.snapshot_at_state(): compute a history_index
where history_index = 0 for tick == 0 and history_index = tick - 1 for tick > 0,
then convert that history_index to usize (with the same AbiError on overflow)
and pass it into engine.snapshot_at_state(frontier.state(),
history_index_usize); keep HeadInfo.tick set to the original tick value and only
translate for the engine call (references: snapshot_at, HeadInfo.tick,
engine.snapshot_at_state, frontier.state()).
---
Duplicate comments:
In `@CHANGELOG.md`:
- Around line 93-94: The changelog entry incorrectly claims
SchedulerCoordinator::super_tick() auto-refreshes the RunnableWriterSet each
SuperTick; update the text to reflect that runnable order is now derived
directly from the head registry via SchedulerCoordinator::peek_order() and
remove the stale reference to RunnableWriterSet auto-refresh. Mention the new
behavior succinctly: "SchedulerCoordinator::peek_order() derives canonical
runnable order from the head registry," and delete or replace the sentence
referencing RunnableWriterSet and auto-refresh in the release notes.
- Around line 42-43: The changelog entry incorrectly states that
WorldlineRegistry::register(...) now fails with a typed duplicate-worldline
error; instead update the text to say that the duplicate registration is now
surfaced at the runtime boundary as RuntimeError::DuplicateWorldline while
WorldlineRegistry::register() continues to return bool, so rephrase to clarify
no breaking change to the registry API and that the runtime surface now emits
RuntimeError::DuplicateWorldline.
In `@crates/warp-core/src/engine_impl.rs`:
- Around line 1010-1012: The current hot-path clones the entire WarpState twice
(saved_engine_state via std::mem::replace and then state_before_runtime_commit =
self.state.clone()) causing O(full-state) cost on every live runtime commit;
change the flow so runtime-ingress materialization is recorded as explicit
WarpOp(s) (or produce a compact touched-event-node delta) instead of cloning the
whole state: modify the code around saved_engine_state /
state_before_runtime_commit and the commit_with_receipt(...) call to accept and
apply a delta or a Vec<WarpOp> that describes only the mutated nodes/events,
update usages where saved_engine_state and state_before_runtime_commit are
compared/rolled back to use the delta/WarpOp sequence, and ensure
commit_with_receipt and rollback logic apply/undo only those deltas rather than
cloning the entire WarpState.
In `@crates/warp-core/src/head_inbox.rs`:
- Around line 343-358: The Budgeted branch in InboxPolicy::Budgeted currently
iterates self.pending and clones Envelope values into admitted then removes
keys, causing unnecessary allocations on the hot path; replace this two-pass
clone+remove with a loop that repeatedly calls self.pending.pop_first() to take
ownership of the smallest-key entries and push them directly into the admitted
Vec (respecting limit/max_per_tick) so you drain entries zero-copy and avoid
cloning Envelope; update the logic around admitted and to_remove to use
pop_first() until admitted.len() >= limit or pop_first() returns None.
In `@crates/warp-core/src/worldline_state.rs`:
- Around line 318-325: WorldlineFrontier::new currently hardcodes frontier_tick:
0 which rewinds restored WorldlineState instances and causes frontier_tick() to
differ from state.current_tick(); update WorldlineFrontier::new to initialize
frontier_tick from the provided state by using state.current_tick(), or
alternatively make the constructor document and enforce that it only accepts
fresh states (and update callers like WorldlineRegistry::register accordingly);
prefer the first option so restored worldlines keep their committed frontier
tick and remove the need for special-casing in WorldlineRegistry::register.
In `@crates/warp-core/tests/invariant_property_tests.rs`:
- Around line 351-370: The test currently constructs Engine instances and calls
Engine::ingest_intent() and commit directly, which bypasses the live runtime
path; instead instantiate and run the runtime path (e.g. WorldlineRuntime or the
runtime entrypoint that owns ingress/scheduling), call the runtime-owned ingress
method (WorldlineRuntime::ingest or the runtime.ingest wrapper) to submit intent
bytes, run the scheduler tick (SchedulerCoordinator::super_tick or the runtime
API that advances/super-ticks), then collect the frontier snapshot and
receipt/patch digests from the runtime’s exported snapshot/receipt surfaces and
assert equality between the two independent runtime instances; replace
references to EngineBuilder/Engine::ingest_intent/commit_with_receipt (engine1,
engine2, tx1, tx2, snap1/snap2, receipt1/receipt2, patch1/patch2) with the
runtime equivalents (WorldlineRuntime, runtime.ingest, runtime.super_tick/sync
tick, runtime.snapshot/receipt functions) so the test drives the live runtime
ingress + scheduler path and compares the same digests.
In `@docs/adr/ADR-0008-Worldline-Runtime-Model.md`:
- Around line 68-98: Update the ADR to remove the stale "runnable_writer_set" /
cache-backed design and describe the current implementation: state that
SchedulerCoordinator::peek_order() derives the canonical runnable order directly
from PlaybackHeadRegistry (not from a maintained RunnableWriterSet), update the
prose in Step 1 and the implementation-plan to reflect this, and replace the
pseudocode `for head in runnable_writer_set` with a call that iterates the order
produced by SchedulerCoordinator::peek_order()/PlaybackHeadRegistry.peek_order()
(not a persisted BTreeSet), and remove or mark deprecated any language asserting
a pre-sorted RunnableWriterSet or zero-filter iteration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76cbed86-152c-4359-9b52-4d56af16b678
📒 Files selected for processing (17)
CHANGELOG.mdcrates/warp-core/src/coordinator.rscrates/warp-core/src/engine_impl.rscrates/warp-core/src/head.rscrates/warp-core/src/head_inbox.rscrates/warp-core/src/lib.rscrates/warp-core/src/worldline_registry.rscrates/warp-core/src/worldline_state.rscrates/warp-core/tests/inbox.rscrates/warp-core/tests/invariant_property_tests.rscrates/warp-wasm/src/lib.rscrates/warp-wasm/src/warp_kernel.rsdocs/ROADMAP/backlog/README.mddocs/ROADMAP/backlog/tooling-misc.mddocs/adr/ADR-0008-Worldline-Runtime-Model.mddocs/plans/adr-0008-and-0009.mdscripts/hooks/README.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 94-95: Update the changelog entry to attribute the canonical
runnable-order derivation to SchedulerCoordinator::peek_order() instead of
SchedulerCoordinator::super_tick(); specifically, change the wording that
currently says "super_tick() now derives canonical runnable order from the head
registry" to state that peek_order() derives the order from the head registry
and that super_tick() calls peek_order() (or simply remove super_tick() as the
actor), so the description accurately references the method that performs the
derivation (peek_order()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5efe6c5e-69f1-40e3-ae88-ff8a35293395
📒 Files selected for processing (1)
CHANGELOG.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
CHANGELOG.md (1)
109-110:⚠️ Potential issue | 🟡 MinorAttribute this to
peek_order(), notsuper_tick().The implementation change was moving runnable-order derivation into
SchedulerCoordinator::peek_order(). This wording is still imprecise, and the newer entry at Lines 85-86 already uses the correct actor.Suggested fix
- **Fixed** `SchedulerCoordinator::super_tick()` now derives canonical runnable - order from the head registry instead of trusting stale runnable-cache state. + **Fixed** `SchedulerCoordinator::super_tick()` now uses canonical runnable + order derived from the head registry via `SchedulerCoordinator::peek_order()` + instead of trusting stale runnable-cache state.Based on learnings,
SchedulerCoordinator::peek_order()derives runnable order from the head registry directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 109 - 110, Update the changelog entry to attribute the behavior change to SchedulerCoordinator::peek_order() (not super_tick()); reword the line to state that peek_order() now derives canonical runnable order from the head registry rather than relying on stale runnable-cache state, so readers can match the implementation change to the correct method (referencing peek_order() and removing attribution to super_tick()).crates/warp-core/src/head_inbox.rs (1)
351-365:⚠️ Potential issue | 🟠 MajorBudgeted admission is still cloning every envelope.
This branch copies each
IngressEnvelopeintoadmitted, allocatesto_remove, and then walkspendingagain to delete the originals.pendingis already ordered, so you can move the firstlimitentries out directly and keep the same deterministic admission order without cloning payloads. If the repo MSRV blocksBTreeMap::pop_first(), use the equivalent first-entry removal path, but the move-out optimization should happen here.
As per coding guidelines,crates/warp-core/**: "Performance matters — flag unnecessary allocations in hot paths."Move-out version
InboxPolicy::Budgeted { max_per_tick } => { let limit = *max_per_tick as usize; - let reserve = limit.min(self.pending.len()); - let mut admitted = Vec::with_capacity(reserve); - let mut to_remove = Vec::with_capacity(reserve); - for (id, env) in &self.pending { - if admitted.len() >= limit { - break; - } - admitted.push(env.clone()); - to_remove.push(*id); - } - for id in to_remove { - self.pending.remove(&id); + let mut admitted = Vec::with_capacity(limit.min(self.pending.len())); + for _ in 0..limit { + let Some((_, env)) = self.pending.pop_first() else { + break; + }; + admitted.push(env); } admitted }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/head_inbox.rs` around lines 351 - 365, The Budgeted branch in InboxPolicy::Budgeted is cloning every IngressEnvelope and building a to_remove list; instead move the first limit entries out of self.pending to admitted to avoid allocations and preserve deterministic order: iterate the first limit entries of self.pending in-order, use an ownership-taking operation (e.g., remove_entry / equivalent first-entry removal if pop_first() is unavailable) to extract each (id, env) into admitted without cloning, and avoid the separate to_remove pass so pending is updated as you take items.crates/warp-core/src/engine_impl.rs (1)
1005-1058:⚠️ Potential issue | 🟠 Major
commit_with_state()still pays two full pre-state clones per tick.Line 1012 clones the whole worldline up front, then
commit_with_receipt()clones it again at Line 1179 and diffs again at Line 1206. On the live ingress path that turns everysuper_tickinto two O(state) clones plus duplicate full-state diff work even whenadmittedis tiny. Reuse one pre-state snapshot across both paths, or split out a commit variant that accepts the caller-supplied pre-state/patch.
As per coding guidelines,crates/warp-core/**: "Performance matters — flag unnecessary allocations in hot paths."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/engine_impl.rs` around lines 1005 - 1058, commit_with_state currently clones the full pre-runtime state into state_before_runtime_commit and commit_with_receipt clones/diffs the pre-state again, causing two O(state) clones per tick; fix by computing one pre-state snapshot and reusing it: either add a commit variant (e.g., commit_with_receipt_with_prestate or commit_with_receipt_prepared) that accepts the caller-supplied pre-state/patch or change commit_with_receipt signature to accept a precommit_state parameter so commit_with_state passes state_before_runtime_commit in instead of letting commit_with_receipt clone again; update uses of commit_with_receipt and remove the redundant clone/diff (diff_state calls) so the pre-state is only captured once and reused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/warp-core/src/engine_impl.rs`:
- Around line 1982-2010: materialize_runtime_ingress_event currently stores only
intent_bytes for IngressPayload::LocalIntent, which loses the IntentKind and
makes distinct ingresses with the same bytes indistinguishable; update
materialize_runtime_ingress_event to also persist the IntentKind alongside
intent_bytes (e.g., as part of the AttachmentValue or a new node field) when
matching the IngressPayload::LocalIntent branch so that downstream logic
(enqueue_first_matching_command, cmd/* matchers, KindFilter, and
compute_ingress_id semantics) can differentiate same-bytes/different-kind events
and maintain the original intent kind during enqueueing and matching.
- Around line 1839-1854: is_fresh_runtime_state() currently only checks
commit/history bookkeeping and can return true even after ingest_intent() or
ingest_inbox_event() have added legacy ingress; update the predicate to also
verify that any legacy inbox/ingress state is empty (e.g., check the engine
fields/collections that represent legacy sim/inbox or pending ingress work) so
callers cannot treat the engine as safe for migration when legacy inbox entries
exist, or alternatively rename the API (e.g., to is_fresh_commit_state) and
update callers to avoid using it as a full runtime-migration guard; change the
implementation of is_fresh_runtime_state() (or replace/rename it) and update all
call sites that rely on it.
In `@docs/ROADMAP/backlog/tooling-misc.md`:
- Line 194: Replace the jargon term "proptests" with the clearer phrase
"property tests" in the sentence "Determinism proptests and golden-vector
harnesses are pinned to single-worker builders." so it reads "Determinism
property tests and golden-vector harnesses are pinned to single-worker
builders." — update the exact phrase "proptests" to "property tests" wherever it
appears in that sentence or nearby instances for consistency.
In `@scripts/hooks/pre-commit`:
- Around line 8-9: The shim computes REPO_ROOT but never changes directory, so
update the pre-commit shim to change into REPO_ROOT before executing the real
hook: after computing REPO_ROOT (the REPO_ROOT variable) run a cd to that
directory and then exec the existing exec "$REPO_ROOT/.githooks/pre-commit" so
the invoked .githooks/pre-commit runs with the repository root as its working
directory.
In `@scripts/verify-local.sh`:
- Around line 346-347: The clippy invocation in run_full_checks omits
non-default targets; update the cargo clippy call used in run_full_checks so it
includes the --all-targets flag and denies missing_docs for those targets:
replace the existing cargo +"$PINNED" clippy "${full_args[@]}" -- -D warnings -D
missing_docs invocation with one that adds --all-targets (e.g., cargo +"$PINNED"
clippy --all-targets "${full_args[@]}" -- -D missing_docs) and also ensure
run_full_checks runs cargo test (cargo +"$PINNED" test) before finishing so
tests are run for all targets as part of the full checks.
- Around line 54-55: run_full_checks() currently marks crates/echo-dind-harness
and crates/echo-dind-tests as requiring full mode but never runs their tests;
update run_full_checks() in scripts/verify-local.sh to execute the DIND harness
tests (e.g., run cargo test or existing test helper against
crates/echo-dind-harness and crates/echo-dind-tests) when full mode is active,
ensuring the determinism harness is exercised; also ensure any helper function
(if present) that runs DIND tests is invoked from run_full_checks() so changes
touching the harness cannot skip those tests.
- Around line 311-315: The current guard searches for #[unsafe(no_mangle)] and
rejects it, but Edition 2024 requires that form; change the check to detect bare
#[no_mangle] instead: update the ripgrep pattern in the match_output assignment
to search for "\#\[\s*no_mangle\s*\]" (or equivalent regex matching
#[no_mangle]) and update the error message to say "pre-push: #[no_mangle] is
invalid; use #[unsafe(no_mangle)]." (keep using the same match_output variable,
echo the match_output to stderr and exit 1).
In `@tests/hooks/test_verify_local.sh`:
- Around line 39-51: The test never verifies deduplication because run_detect is
only given each crate once; update the run_detect invocation (the run_detect
call that assigns reduced_output) to include one crate path twice (e.g., pass
the same crate path duplicate among its arguments) so the subsequent check for
changed_crates=echo-app-core,warp-cli actually exercises deduplication before
the assertions that parse reduced_output and check changed_crates.
---
Duplicate comments:
In `@CHANGELOG.md`:
- Around line 109-110: Update the changelog entry to attribute the behavior
change to SchedulerCoordinator::peek_order() (not super_tick()); reword the line
to state that peek_order() now derives canonical runnable order from the head
registry rather than relying on stale runnable-cache state, so readers can match
the implementation change to the correct method (referencing peek_order() and
removing attribution to super_tick()).
In `@crates/warp-core/src/engine_impl.rs`:
- Around line 1005-1058: commit_with_state currently clones the full pre-runtime
state into state_before_runtime_commit and commit_with_receipt clones/diffs the
pre-state again, causing two O(state) clones per tick; fix by computing one
pre-state snapshot and reusing it: either add a commit variant (e.g.,
commit_with_receipt_with_prestate or commit_with_receipt_prepared) that accepts
the caller-supplied pre-state/patch or change commit_with_receipt signature to
accept a precommit_state parameter so commit_with_state passes
state_before_runtime_commit in instead of letting commit_with_receipt clone
again; update uses of commit_with_receipt and remove the redundant clone/diff
(diff_state calls) so the pre-state is only captured once and reused.
In `@crates/warp-core/src/head_inbox.rs`:
- Around line 351-365: The Budgeted branch in InboxPolicy::Budgeted is cloning
every IngressEnvelope and building a to_remove list; instead move the first
limit entries out of self.pending to admitted to avoid allocations and preserve
deterministic order: iterate the first limit entries of self.pending in-order,
use an ownership-taking operation (e.g., remove_entry / equivalent first-entry
removal if pop_first() is unavailable) to extract each (id, env) into admitted
without cloning, and avoid the separate to_remove pass so pending is updated as
you take items.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f53f812-020d-419a-bd10-cd44ddeb36af
📒 Files selected for processing (17)
.githooks/pre-push.github/workflows/ci.ymlCHANGELOG.mdMakefilecrates/warp-core/src/engine_impl.rscrates/warp-core/src/head_inbox.rscrates/warp-core/src/lib.rscrates/warp-core/tests/inbox.rscrates/warp-wasm/src/lib.rsdocs/ROADMAP/backlog/tooling-misc.mddocs/plans/adr-0008-and-0009.mdscripts/hooks/README.mdscripts/hooks/pre-commitscripts/hooks/pre-pushscripts/verify-local.shtests/hooks/test_hook_issues.shtests/hooks/test_verify_local.sh
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
scripts/verify-local.sh (3)
139-145:⚠️ Potential issue | 🟠 MajorDIND test packages missing from
FULL_LOCAL_TEST_PACKAGES.
echo-dind-harnessandecho-dind-testsare inFULL_CRITICAL_PREFIXESandFULL_LOCAL_PACKAGES, but not inFULL_LOCAL_TEST_PACKAGES. Changes to the determinism harness can pass the pre-push gate without actually running the harness tests.Suggested fix
readonly FULL_LOCAL_TEST_PACKAGES=( "warp-geom" "echo-graph" "echo-scene-port" "echo-scene-codec" "echo-ttd" + "echo-dind-harness" + "echo-dind-tests" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-local.sh` around lines 139 - 145, FULL_LOCAL_TEST_PACKAGES is missing the DIND test packages causing harness changes to skip tests; update the FULL_LOCAL_TEST_PACKAGES array to include "echo-dind-harness" and "echo-dind-tests" (these two are already present in FULL_CRITICAL_PREFIXES and FULL_LOCAL_PACKAGES) so the determinism harness tests run as part of the full local test set.
370-374:⚠️ Potential issue | 🟠 MajorThe
no_mangleguard is inverted for Edition 2024.Rust Edition 2024 requires
#[unsafe(no_mangle)]. The current check rejects the correct form and allows the deprecated bare#[no_mangle]. Flip the regex.Suggested fix
- if match_output=$(rg -n "\\#\[unsafe\(no_mangle\)\]" crates 2>&1); then - echo "pre-push: #[unsafe(no_mangle)] is invalid; use #[no_mangle]." >&2 + if match_output=$(rg -n '#\[no_mangle\]' crates 2>&1); then + echo "pre-push: bare #[no_mangle] is forbidden in Edition 2024; use #[unsafe(no_mangle)]." >&2 echo "$match_output" >&2 exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-local.sh` around lines 370 - 374, The check in the pre-push guard currently searches for "#[unsafe(no_mangle)]" and rejects it, but Edition 2024 requires #[unsafe(no_mangle)] so the logic is inverted; update the rg pattern used in the match (the command that assigns match_output) to search for the deprecated bare attribute "#[no_mangle]" instead, so the script warns and fails on occurrences of "#[no_mangle]" while allowing "#[unsafe(no_mangle)]" (leave the surrounding echo/error handling and variable name match_output unchanged).
405-406:⚠️ Potential issue | 🟠 MajorFull clippy gate missing
--all-targets.
run_full_checks()omits--all-targets, so warnings in tests, benches, examples, and build scripts are invisible. This is weaker thanrun_targeted_checks()(line 324) which includes it.Suggested fix
echo "[verify-local] cargo clippy on critical packages" - cargo +"$PINNED" clippy "${full_args[@]}" -- -D warnings -D missing_docs + cargo +"$PINNED" clippy --all-targets "${full_args[@]}" -- -D warnings -D missing_docs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-local.sh` around lines 405 - 406, The clippy invocation in run_full_checks() misses the --all-targets flag so warnings in tests/benches/examples/build-scripts are not checked; update the cargo clippy call (the line invoking cargo +"$PINNED" clippy "${full_args[@]}" -- -D warnings -D missing_docs) to include --all-targets before the double-dash (e.g. cargo +"$PINNED" clippy "${full_args[@]}" --all-targets -- -D warnings -D missing_docs) so it matches run_targeted_checks() behavior.CHANGELOG.md (1)
115-116: 🧹 Nitpick | 🔵 TrivialAttribution imprecision:
peek_order()derives order, notsuper_tick().Line 115-116 says
super_tick()"derives canonical runnable order" butpeek_order()does the derivation. The wording is still slightly off.Suggested wording
-- **Fixed** `SchedulerCoordinator::super_tick()` now derives canonical runnable - order from the head registry instead of trusting stale runnable-cache state. +- **Fixed** `SchedulerCoordinator::super_tick()` now uses canonical runnable + order derived via `peek_order()` from the head registry instead of trusting + stale runnable-cache state.Based on learnings,
SchedulerCoordinator::peek_order()derives runnable order from the head registry directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 115 - 116, Update the changelog wording to correctly attribute derivation to SchedulerCoordinator::peek_order() (not super_tick()); replace the sentence that says "super_tick() now derives canonical runnable order" with a concise statement that "SchedulerCoordinator::peek_order() derives canonical runnable runnable order from the head registry instead of relying on stale runnable-cache state," ensuring the change references peek_order() and SchedulerCoordinator.tests/hooks/test_verify_local.sh (1)
45-63:⚠️ Potential issue | 🟡 MinorTest does not exercise crate deduplication.
Line 45 passes two different crates. The assertion on line 58 proves sorting but not deduplication. Pass the same crate path twice to verify
sort -uworks.Suggested fix
-reduced_output="$(run_detect crates/warp-cli/src/main.rs crates/echo-app-core/src/lib.rs)" +reduced_output="$(run_detect crates/warp-cli/src/main.rs crates/warp-cli/src/cli.rs crates/echo-app-core/src/lib.rs)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/test_verify_local.sh` around lines 45 - 63, The test currently calls run_detect with two different crate paths (stored in reduced_output) so the '^changed_crates=...' assertion only verifies sorting, not deduplication; update the run_detect invocation (the line that sets reduced_output) to pass the same crate path twice (e.g., repeat crates/echo-app-core) so the subsequent grep for '^changed_crates=echo-app-core,warp-cli$' actually exercises the deduplication performed by sort -u.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/hooks/test_verify_local.sh`:
- Around line 65-85: Tests currently cover crate-prefix and workflow-prefix
classification but miss exact-match critical paths from the FULL_CRITICAL_EXACT
set; add a test case in tests/hooks/test_verify_local.sh that calls run_detect
on an exact file path (e.g., "Cargo.toml") and assert the output contains
classification=full and stamp_suite=full (use the same grep/if pass/fail pattern
used for run_detect crates/warp-core/src/lib.rs and .github/workflows/ci.yml) so
the FULL_CRITICAL_EXACT matching logic is validated.
---
Duplicate comments:
In `@CHANGELOG.md`:
- Around line 115-116: Update the changelog wording to correctly attribute
derivation to SchedulerCoordinator::peek_order() (not super_tick()); replace the
sentence that says "super_tick() now derives canonical runnable order" with a
concise statement that "SchedulerCoordinator::peek_order() derives canonical
runnable runnable order from the head registry instead of relying on stale
runnable-cache state," ensuring the change references peek_order() and
SchedulerCoordinator.
In `@scripts/verify-local.sh`:
- Around line 139-145: FULL_LOCAL_TEST_PACKAGES is missing the DIND test
packages causing harness changes to skip tests; update the
FULL_LOCAL_TEST_PACKAGES array to include "echo-dind-harness" and
"echo-dind-tests" (these two are already present in FULL_CRITICAL_PREFIXES and
FULL_LOCAL_PACKAGES) so the determinism harness tests run as part of the full
local test set.
- Around line 370-374: The check in the pre-push guard currently searches for
"#[unsafe(no_mangle)]" and rejects it, but Edition 2024 requires
#[unsafe(no_mangle)] so the logic is inverted; update the rg pattern used in the
match (the command that assigns match_output) to search for the deprecated bare
attribute "#[no_mangle]" instead, so the script warns and fails on occurrences
of "#[no_mangle]" while allowing "#[unsafe(no_mangle)]" (leave the surrounding
echo/error handling and variable name match_output unchanged).
- Around line 405-406: The clippy invocation in run_full_checks() misses the
--all-targets flag so warnings in tests/benches/examples/build-scripts are not
checked; update the cargo clippy call (the line invoking cargo +"$PINNED" clippy
"${full_args[@]}" -- -D warnings -D missing_docs) to include --all-targets
before the double-dash (e.g. cargo +"$PINNED" clippy "${full_args[@]}"
--all-targets -- -D warnings -D missing_docs) so it matches
run_targeted_checks() behavior.
In `@tests/hooks/test_verify_local.sh`:
- Around line 45-63: The test currently calls run_detect with two different
crate paths (stored in reduced_output) so the '^changed_crates=...' assertion
only verifies sorting, not deduplication; update the run_detect invocation (the
line that sets reduced_output) to pass the same crate path twice (e.g., repeat
crates/echo-app-core) so the subsequent grep for
'^changed_crates=echo-app-core,warp-cli$' actually exercises the deduplication
performed by sort -u.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7d4cfe2-a674-4e7d-af27-bf52ec8c37d9
📒 Files selected for processing (4)
CHANGELOG.mdscripts/hooks/README.mdscripts/verify-local.shtests/hooks/test_verify_local.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
123-142: 🧹 Nitpick | 🔵 TrivialVariant jobs (
test-musl,test-det-fixed,test-musl-det-fixed) not aligned with shard pattern.These jobs test only
warp-coredirectly viacargo test -p warp-core, bypassing the new nextest + doc-test structure. This creates divergence:
- Main
test-warp-coreusescargo nextest run+ explicit--docstep.- Variant jobs use plain
cargo test.If nextest is adopted for better test isolation and output, consider applying it to variant jobs as well—or document why they differ (e.g., nextest doesn't support
--targetcross-compilation to musl cleanly).Also applies to: 144-157, 159-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 123 - 142, The variant GitHub Actions jobs (test-musl, test-det-fixed, test-musl-det-fixed) are running cargo test -p warp-core directly which diverges from the main test-warp-core job that uses cargo nextest run plus a separate --doc step; update these variant jobs to mirror the nextest + doc-test flow (replace cargo test -p warp-core with cargo nextest run --package warp-core and add the same explicit doc-test step), or if nextest cannot be used for cross-compilation targets (e.g., musl), document that limitation in the workflow comments and keep the cargo test command; reference the job names test-musl/test-det-fixed/test-musl-det-fixed and the commands cargo test -p warp-core, cargo nextest run, and the --doc doc-test step when making the change.
♻️ Duplicate comments (1)
CHANGELOG.md (1)
121-122:⚠️ Potential issue | 🟡 MinorCorrect method attribution for canonical runnable ordering.
Line 121-122 credits
SchedulerCoordinator::super_tick()as deriving canonical order, but derivation is performed bySchedulerCoordinator::peek_order();super_tick()consumes that order.Suggested diff
-- **Fixed** `SchedulerCoordinator::super_tick()` now derives canonical runnable - order from the head registry instead of trusting stale runnable-cache state. +- **Fixed** `SchedulerCoordinator::super_tick()` now uses canonical runnable + order derived from the head registry via `SchedulerCoordinator::peek_order()` + instead of trusting stale runnable-cache state.Based on learnings,
SchedulerCoordinator::peek_order()is the method that derives runnable order from the head registry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 121 - 122, Update the changelog entry to correctly attribute canonical runnable ordering to SchedulerCoordinator::peek_order() instead of SchedulerCoordinator::super_tick(); change the sentence so it states that peek_order() derives the canonical runnable order from the head registry and super_tick() consumes that order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 88-91: The workflow step "Install cargo-nextest" currently uses
the floating reference uses: taiki-e/install-action@v2; change this to a pinned
reference (either a specific minor tag like taiki-e/install-action@v2.49.0 or
the exact commit SHA for that v2 release) to ensure reproducible installs and
reduce supply-chain risk; update the uses value in the same step so the action
is locked to the chosen tag or SHA.
In `@CHANGELOG.md`:
- Around line 127-128: Update the Unreleased changelog entry to avoid
"debug_asserts" wording and state that HeadInbox::ingest() enforces canonical
content-hash invariants (not merely a debug-only check); replace the phrase "now
debug_asserts" with language indicating release-build enforcement for
HeadInbox::ingest() hash/ingress_id validation so the notes reflect the shipped
strong invariant.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 123-142: The variant GitHub Actions jobs (test-musl,
test-det-fixed, test-musl-det-fixed) are running cargo test -p warp-core
directly which diverges from the main test-warp-core job that uses cargo nextest
run plus a separate --doc step; update these variant jobs to mirror the nextest
+ doc-test flow (replace cargo test -p warp-core with cargo nextest run
--package warp-core and add the same explicit doc-test step), or if nextest
cannot be used for cross-compilation targets (e.g., musl), document that
limitation in the workflow comments and keep the cargo test command; reference
the job names test-musl/test-det-fixed/test-musl-det-fixed and the commands
cargo test -p warp-core, cargo nextest run, and the --doc doc-test step when
making the change.
---
Duplicate comments:
In `@CHANGELOG.md`:
- Around line 121-122: Update the changelog entry to correctly attribute
canonical runnable ordering to SchedulerCoordinator::peek_order() instead of
SchedulerCoordinator::super_tick(); change the sentence so it states that
peek_order() derives the canonical runnable order from the head registry and
super_tick() consumes that order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a125fb4c-4738-42ef-97e2-236fe1d6afe4
📒 Files selected for processing (2)
.github/workflows/ci.ymlCHANGELOG.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 89-90: Update the Unreleased changelog wording to remove the
contradictory “debug-asserts” phrasing and state that
unpause(PlaybackMode::Paused) deterministically fails/panics in all builds
(matching the behavior documented in crates/warp-core/src/head.rs); change both
occurrences that claim debug-only behavior to indicate a deterministic failure
in all build configurations and ensure the phrasing consistently references
unpause and PlaybackMode::Paused.
In `@crates/warp-core/src/engine_impl.rs`:
- Around line 1007-1136: commit_with_state performs many std::mem::replace()
swaps leaving self and state in a partially-moved, unsafe-to-unwind condition;
wrap those swaps in a RAII Drop guard (e.g., a local struct created at start of
the mutable-swap sequence) that captures saved_engine_state, saved_root,
saved_initial_state, saved_last_snapshot, saved_tick_history,
saved_last_materialization, saved_last_materialization_errors, saved_tx_counter,
saved_scheduler, saved_live_txs and performs the original restore logic in its
Drop implementation to guarantee restoration on panic/unwind; on the normal
success path disarm the guard (e.g., set a flag) after you have rebuilt the
final WorldlineState and manually perform the same moves that the guard would do
so the guard does nothing on drop; ensure the guard uses the same unique symbols
(commit_with_state, saved_engine_state, state_before_runtime_commit,
committed_ingress, original_worldline_tx_counter, current_root, scheduler,
live_txs, bus.clear()) so all restoration work is centralized and unwind-safe.
- Around line 1048-1051: The loop over admitted can enqueue duplicate commands
if the same envelope is present multiple times; change the logic that iterates
admitted (in engine_impl.rs) to first deduplicate envelopes by their ingress
identifier (use the envelope.ingress_id or equivalent field) while preserving
original order (track seen ingress IDs with a HashSet and skip repeats), then
for each unique envelope call materialize_runtime_ingress_event and
enqueue_first_matching_command as before so duplicate envelopes do not produce
multiple candidates for the same scope and perturb receipt.digest()/commit hash.
In `@docs/ROADMAP/backlog/tooling-misc.md`:
- Line 187: Update the R1 line wording: replace the phrase "R1: Introduce a
shared helper for the remaining single-worker deterministic test engines" with
"R1: Introduce a shared helper for single-worker deterministic test engines" so
it reads clearly (leave R2 as the step that covers migrating the remaining
tests).
In `@scripts/verify-local.sh`:
- Around line 177-192: The git diff calls are currently excluding deletions via
the --diff-filter=ACMRTUXB flag so removed files never show up; update every
occurrence of --diff-filter=ACMRTUXB (the git diff --name-only ... and git
diff-tree ... commands in this snippet) to include D (e.g.
--diff-filter=ACMRTUXBD) so deletions are reported and the full suite can be
triggered when critical paths are removed.
- Around line 171-193: The script treats pre-commit like other modes and so
list_changed_files() and the stamp reuse are wrong for staged-only changes; add
handling for the "pre-commit" command in the main case dispatcher and implement
a dedicated path that uses git diff --cached --name-only --diff-filter=ACMRTUXB
(or a new helper list_changed_index_files) to list staged files rather than
comparing against upstream/merge-base or HEAD, and change the stamp key logic
used for caching results when run as pre-commit to be index/tree-based (e.g.,
derive the stamp from the index tree hash via git write-tree or disable stamp
reuse for pre-commit) so staged changes cannot be ignored or a stale stamp
reused; update any references to list_changed_files(), the pre-commit case, and
the stamp-generation code accordingly.
In `@tests/hooks/test_verify_local.sh`:
- Around line 21-27: Add a smoke test that exercises the actual pre-commit hook
path instead of only detect: extend or add a helper like run_detect to create a
tiny pre-commit fixture and invoke scripts/verify-local.sh pre-commit with
VERIFY_CHANGED_FILES_FILE pointing at the temp file (similar to run_detect but
calling "pre-commit"), and ensure the test cleans up the temp file; reference
run_detect, VERIFY_CHANGED_FILES_FILE, and scripts/verify-local.sh pre-commit
(and the .githooks/pre-commit symlink) so the test validates the real hook mode
and catches regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 758a89c7-a7e4-438b-a137-e9e753ac86eb
📒 Files selected for processing (9)
.github/workflows/ci.ymlCHANGELOG.mdcrates/warp-core/src/engine_impl.rscrates/warp-wasm/src/warp_kernel.rsdocs/ROADMAP/backlog/tooling-misc.mdscripts/hooks/README.mdscripts/hooks/pre-commitscripts/verify-local.shtests/hooks/test_verify_local.sh
Comment on lines
+89
to
+90
| `unpause(PlaybackMode::Paused)` panics in all builds instead of only under | ||
| `debug_assert!`. |
Contributor
There was a problem hiding this comment.
Resolve contradictory unpause() behavior wording in Unreleased notes.
Line 89-Line 90 says unpause(PlaybackMode::Paused) “panics in all builds,” while Line 152 says it “debug-asserts.” In the same Unreleased block, this reads as conflicting final behavior and over-specifies panic semantics.
Suggested changelog wording
-- **Fixed** `WriterHead` now derives pause state from `mode`, and
- `unpause(PlaybackMode::Paused)` panics in all builds instead of only under
- `debug_assert!`.
+- **Fixed** `WriterHead` now derives pause state from `mode`, and
+ `unpause(PlaybackMode::Paused)` now fails deterministically in all builds
+ (not only under `debug_assert!`).
-- **Fixed** `unpause()` now debug-asserts that the mode is not `Paused`.
+- **Fixed** `unpause()` initially added a debug-only guard for `Paused`;
+ later hardening made this failure deterministic in all build configurations.Based on learnings, in crates/warp-core/src/head.rs, unpause(PlaybackMode::Paused) is documented as failing deterministically in all build configurations (not debug-only).
Also applies to: 152-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 89 - 90, Update the Unreleased changelog wording
to remove the contradictory “debug-asserts” phrasing and state that
unpause(PlaybackMode::Paused) deterministically fails/panics in all builds
(matching the behavior documented in crates/warp-core/src/head.rs); change both
occurrences that claim debug-only behavior to indicate a deterministic failure
in all build configurations and ensure the phrasing consistently references
unpause and PlaybackMode::Paused.
Comment on lines
+1007
to
+1136
| pub fn commit_with_state( | ||
| &mut self, | ||
| state: &mut WorldlineState, | ||
| admitted: &[IngressEnvelope], | ||
| ) -> Result<CommitOutcome, EngineError> { | ||
| let saved_engine_state = | ||
| std::mem::replace(&mut self.state, std::mem::take(&mut state.warp_state)); | ||
| let state_before_runtime_commit = self.state.clone(); | ||
| let saved_state_root = state.root; | ||
| let saved_root = self.current_root; | ||
| self.current_root = state.root; | ||
| let saved_initial_state = std::mem::replace( | ||
| &mut self.initial_state, | ||
| std::mem::take(&mut state.initial_state), | ||
| ); | ||
| let saved_last_snapshot = | ||
| std::mem::replace(&mut self.last_snapshot, state.last_snapshot.take()); | ||
| let saved_tick_history = std::mem::replace( | ||
| &mut self.tick_history, | ||
| std::mem::take(&mut state.tick_history), | ||
| ); | ||
| let saved_last_materialization = std::mem::replace( | ||
| &mut self.last_materialization, | ||
| std::mem::take(&mut state.last_materialization), | ||
| ); | ||
| let saved_last_materialization_errors = std::mem::replace( | ||
| &mut self.last_materialization_errors, | ||
| std::mem::take(&mut state.last_materialization_errors), | ||
| ); | ||
| let committed_ingress = std::mem::take(&mut state.committed_ingress); | ||
| let saved_tx_counter = self.tx_counter; | ||
| let original_worldline_tx_counter = state.tx_counter; | ||
| self.tx_counter = original_worldline_tx_counter; | ||
| let fresh_scheduler = self.fresh_scheduler(); | ||
| let saved_scheduler = std::mem::replace(&mut self.scheduler, fresh_scheduler); | ||
| let saved_live_txs = std::mem::take(&mut self.live_txs); | ||
| self.bus.clear(); | ||
|
|
||
| let result = (|| { | ||
| let tx = self.begin(); | ||
|
|
||
| for envelope in admitted { | ||
| let event_id = self.materialize_runtime_ingress_event(envelope)?; | ||
| let _ = self.enqueue_first_matching_command(tx, &event_id)?; | ||
| } | ||
|
|
||
| let (mut snapshot, receipt, patch) = self.commit_with_receipt(tx)?; | ||
| let combined_patch = WarpTickPatchV1::new( | ||
| patch.policy_id(), | ||
| patch.rule_pack_id(), | ||
| patch.commit_status(), | ||
| patch.in_slots().to_vec(), | ||
| patch.out_slots().to_vec(), | ||
| diff_state(&state_before_runtime_commit, &self.state), | ||
| ); | ||
| snapshot.patch_digest = combined_patch.digest(); | ||
| snapshot.hash = compute_commit_hash_v2( | ||
| &snapshot.state_root, | ||
| &snapshot.parents, | ||
| &snapshot.patch_digest, | ||
| snapshot.policy_id, | ||
| ); | ||
| self.last_snapshot = Some(snapshot.clone()); | ||
| if let Some((recorded_snapshot, _recorded_receipt, recorded_patch)) = | ||
| self.tick_history.last_mut() | ||
| { | ||
| *recorded_snapshot = snapshot.clone(); | ||
| *recorded_patch = combined_patch.clone(); | ||
| } | ||
| Ok(CommitOutcome { | ||
| snapshot, | ||
| receipt, | ||
| patch: combined_patch, | ||
| }) | ||
| })(); | ||
|
|
||
| match result { | ||
| Ok(outcome) => { | ||
| *state = WorldlineState { | ||
| warp_state: std::mem::replace(&mut self.state, saved_engine_state), | ||
| root: self.current_root, | ||
| initial_state: std::mem::replace(&mut self.initial_state, saved_initial_state), | ||
| last_snapshot: std::mem::replace(&mut self.last_snapshot, saved_last_snapshot), | ||
| tick_history: std::mem::replace(&mut self.tick_history, saved_tick_history), | ||
| last_materialization: std::mem::replace( | ||
| &mut self.last_materialization, | ||
| saved_last_materialization, | ||
| ), | ||
| last_materialization_errors: std::mem::replace( | ||
| &mut self.last_materialization_errors, | ||
| saved_last_materialization_errors, | ||
| ), | ||
| tx_counter: self.tx_counter, | ||
| committed_ingress, | ||
| }; | ||
| self.current_root = saved_root; | ||
| self.tx_counter = saved_tx_counter; | ||
| self.scheduler = saved_scheduler; | ||
| self.live_txs = saved_live_txs; | ||
| self.bus.clear(); | ||
| Ok(outcome) | ||
| } | ||
| Err(err) => { | ||
| *state = WorldlineState { | ||
| warp_state: state_before_runtime_commit, | ||
| root: saved_state_root, | ||
| initial_state: std::mem::replace(&mut self.initial_state, saved_initial_state), | ||
| last_snapshot: std::mem::replace(&mut self.last_snapshot, saved_last_snapshot), | ||
| tick_history: std::mem::replace(&mut self.tick_history, saved_tick_history), | ||
| last_materialization: std::mem::replace( | ||
| &mut self.last_materialization, | ||
| saved_last_materialization, | ||
| ), | ||
| last_materialization_errors: std::mem::replace( | ||
| &mut self.last_materialization_errors, | ||
| saved_last_materialization_errors, | ||
| ), | ||
| tx_counter: original_worldline_tx_counter, | ||
| committed_ingress, | ||
| }; | ||
| self.state = saved_engine_state; | ||
| self.current_root = saved_root; | ||
| self.tx_counter = saved_tx_counter; | ||
| self.scheduler = saved_scheduler; | ||
| self.live_txs = saved_live_txs; | ||
| self.bus.clear(); | ||
| Err(err) | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
commit_with_state() is not unwind-safe.
After the first replace(), self temporarily owns the frontier state and state has already been partially emptied. commit_with_receipt() documents a panic path under delta_validate, and matcher/executor code can panic too. An unwind through this block skips the restore logic and leaves both objects corrupted. Put the swap/restore path behind a Drop guard and only disarm it after the success path has rebuilt the final WorldlineState.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/warp-core/src/engine_impl.rs` around lines 1007 - 1136,
commit_with_state performs many std::mem::replace() swaps leaving self and state
in a partially-moved, unsafe-to-unwind condition; wrap those swaps in a RAII
Drop guard (e.g., a local struct created at start of the mutable-swap sequence)
that captures saved_engine_state, saved_root, saved_initial_state,
saved_last_snapshot, saved_tick_history, saved_last_materialization,
saved_last_materialization_errors, saved_tx_counter, saved_scheduler,
saved_live_txs and performs the original restore logic in its Drop
implementation to guarantee restoration on panic/unwind; on the normal success
path disarm the guard (e.g., set a flag) after you have rebuilt the final
WorldlineState and manually perform the same moves that the guard would do so
the guard does nothing on drop; ensure the guard uses the same unique symbols
(commit_with_state, saved_engine_state, state_before_runtime_commit,
committed_ingress, original_worldline_tx_counter, current_root, scheduler,
live_txs, bus.clear()) so all restoration work is centralized and unwind-safe.
Comment on lines
+1048
to
+1051
| for envelope in admitted { | ||
| let event_id = self.materialize_runtime_ingress_event(envelope)?; | ||
| let _ = self.enqueue_first_matching_command(tx, &event_id)?; | ||
| } |
Contributor
There was a problem hiding this comment.
Deduplicate admitted by ingress_id before enqueueing commands.
This loop will process the same event twice if the caller passes the same envelope twice. Once a cmd/* rule matches, the second pass adds another candidate for the same scope, perturbing receipt.digest() and the commit hash for what should be an idempotent ingress batch.
Deterministic fix sketch
+ let mut seen_ingress = std::collections::BTreeSet::new();
for envelope in admitted {
+ if !seen_ingress.insert(envelope.ingress_id()) {
+ continue;
+ }
let event_id = self.materialize_runtime_ingress_event(envelope)?;
let _ = self.enqueue_first_matching_command(tx, &event_id)?;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/warp-core/src/engine_impl.rs` around lines 1048 - 1051, The loop over
admitted can enqueue duplicate commands if the same envelope is present multiple
times; change the logic that iterates admitted (in engine_impl.rs) to first
deduplicate envelopes by their ingress identifier (use the envelope.ingress_id
or equivalent field) while preserving original order (track seen ingress IDs
with a HashSet and skip repeats), then for each unique envelope call
materialize_runtime_ingress_event and enqueue_first_matching_command as before
so duplicate envelopes do not produce multiple candidates for the same scope and
perturb receipt.digest()/commit hash.
|
|
||
| **Requirements:** | ||
|
|
||
| - R1: Introduce a shared helper for the remaining single-worker deterministic test engines |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Rephrase R1 for clarity.
"for the remaining single-worker deterministic test engines" is awkward—the helper is new, not "remaining." The tests to be migrated are what remains (covered in R2). Simplify R1 to:
"R1: Introduce a shared helper for single-worker deterministic test engines"
Then R2 already handles the migration of remaining tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ROADMAP/backlog/tooling-misc.md` at line 187, Update the R1 line
wording: replace the phrase "R1: Introduce a shared helper for the remaining
single-worker deterministic test engines" with "R1: Introduce a shared helper
for single-worker deterministic test engines" so it reads clearly (leave R2 as
the step that covers migrating the remaining tests).
Comment on lines
+171
to
+193
| list_changed_files() { | ||
| if [[ -n "${VERIFY_CHANGED_FILES_FILE:-}" ]]; then | ||
| cat "$VERIFY_CHANGED_FILES_FILE" | ||
| return | ||
| fi | ||
|
|
||
| if git rev-parse --verify '@{upstream}' >/dev/null 2>&1; then | ||
| git diff --name-only --diff-filter=ACMRTUXB '@{upstream}...HEAD' | ||
| return | ||
| fi | ||
|
|
||
| local candidate | ||
| local merge_base | ||
| for candidate in origin/main main origin/master master; do | ||
| if git rev-parse --verify "$candidate" >/dev/null 2>&1; then | ||
| merge_base="$(git merge-base HEAD "$candidate")" | ||
| git diff --name-only --diff-filter=ACMRTUXB "${merge_base}...HEAD" | ||
| return | ||
| fi | ||
| done | ||
|
|
||
| git diff-tree --root --no-commit-id --name-only -r --diff-filter=ACMRTUXB HEAD | ||
| } |
Contributor
There was a problem hiding this comment.
pre-commit is broken, and an alias to auto would still validate the wrong thing.
.githooks/pre-commit invokes this script with pre-commit, but this case falls into the usage error. Even if you add pre-commit to the pr|auto|pre-push arm, list_changed_files() and the stamp key are branch-HEAD based, so the hook would ignore staged-only changes and can reuse a stale stamp after new files are added to the index on the same HEAD. pre-commit needs its own git diff --cached ... path and either an index/tree-based stamp key or no stamp reuse.
Also applies to: 259-290, 471-497
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/verify-local.sh` around lines 171 - 193, The script treats pre-commit
like other modes and so list_changed_files() and the stamp reuse are wrong for
staged-only changes; add handling for the "pre-commit" command in the main case
dispatcher and implement a dedicated path that uses git diff --cached
--name-only --diff-filter=ACMRTUXB (or a new helper list_changed_index_files) to
list staged files rather than comparing against upstream/merge-base or HEAD, and
change the stamp key logic used for caching results when run as pre-commit to be
index/tree-based (e.g., derive the stamp from the index tree hash via git
write-tree or disable stamp reuse for pre-commit) so staged changes cannot be
ignored or a stale stamp reused; update any references to list_changed_files(),
the pre-commit case, and the stamp-generation code accordingly.
Comment on lines
+177
to
+192
| if git rev-parse --verify '@{upstream}' >/dev/null 2>&1; then | ||
| git diff --name-only --diff-filter=ACMRTUXB '@{upstream}...HEAD' | ||
| return | ||
| fi | ||
|
|
||
| local candidate | ||
| local merge_base | ||
| for candidate in origin/main main origin/master master; do | ||
| if git rev-parse --verify "$candidate" >/dev/null 2>&1; then | ||
| merge_base="$(git merge-base HEAD "$candidate")" | ||
| git diff --name-only --diff-filter=ACMRTUXB "${merge_base}...HEAD" | ||
| return | ||
| fi | ||
| done | ||
|
|
||
| git diff-tree --root --no-commit-id --name-only -r --diff-filter=ACMRTUXB HEAD |
Contributor
There was a problem hiding this comment.
Deleted critical paths bypass escalation.
All three diff probes use --diff-filter=ACMRTUXB, so deletions never reach classification. Removing Cargo.toml, .github/workflows/*, or a determinism guard can therefore skip the full suite entirely.
Minimal fix
- git diff --name-only --diff-filter=ACMRTUXB '@{upstream}...HEAD'
+ git diff --name-only --diff-filter=ACDMRTUXB '@{upstream}...HEAD'
@@
- git diff --name-only --diff-filter=ACMRTUXB "${merge_base}...HEAD"
+ git diff --name-only --diff-filter=ACDMRTUXB "${merge_base}...HEAD"
@@
- git diff-tree --root --no-commit-id --name-only -r --diff-filter=ACMRTUXB HEAD
+ git diff-tree --root --no-commit-id --name-only -r --diff-filter=ACDMRTUXB HEAD📝 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.
Suggested change
| if git rev-parse --verify '@{upstream}' >/dev/null 2>&1; then | |
| git diff --name-only --diff-filter=ACMRTUXB '@{upstream}...HEAD' | |
| return | |
| fi | |
| local candidate | |
| local merge_base | |
| for candidate in origin/main main origin/master master; do | |
| if git rev-parse --verify "$candidate" >/dev/null 2>&1; then | |
| merge_base="$(git merge-base HEAD "$candidate")" | |
| git diff --name-only --diff-filter=ACMRTUXB "${merge_base}...HEAD" | |
| return | |
| fi | |
| done | |
| git diff-tree --root --no-commit-id --name-only -r --diff-filter=ACMRTUXB HEAD | |
| if git rev-parse --verify '@{upstream}' >/dev/null 2>&1; then | |
| git diff --name-only --diff-filter=ACDMRTUXB '@{upstream}...HEAD' | |
| return | |
| fi | |
| local candidate | |
| local merge_base | |
| for candidate in origin/main main origin/master master; do | |
| if git rev-parse --verify "$candidate" >/dev/null 2>&1; then | |
| merge_base="$(git merge-base HEAD "$candidate")" | |
| git diff --name-only --diff-filter=ACDMRTUXB "${merge_base}...HEAD" | |
| return | |
| fi | |
| done | |
| git diff-tree --root --no-commit-id --name-only -r --diff-filter=ACDMRTUXB HEAD |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/verify-local.sh` around lines 177 - 192, The git diff calls are
currently excluding deletions via the --diff-filter=ACMRTUXB flag so removed
files never show up; update every occurrence of --diff-filter=ACMRTUXB (the git
diff --name-only ... and git diff-tree ... commands in this snippet) to include
D (e.g. --diff-filter=ACMRTUXBD) so deletions are reported and the full suite
can be triggered when critical paths are removed.
Comment on lines
+21
to
+27
| run_detect() { | ||
| local tmp | ||
| tmp="$(mktemp)" | ||
| printf '%s\n' "$@" >"$tmp" | ||
| VERIFY_CHANGED_FILES_FILE="$tmp" scripts/verify-local.sh detect | ||
| rm -f "$tmp" | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a smoke test for pre-commit mode.
Everything here goes through detect, but the real hook path is .githooks/pre-commit -> scripts/verify-local.sh pre-commit. A tiny pre-commit fixture using VERIFY_CHANGED_FILES_FILE would have caught the current mode regression immediately.
Also applies to: 29-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hooks/test_verify_local.sh` around lines 21 - 27, Add a smoke test that
exercises the actual pre-commit hook path instead of only detect: extend or add
a helper like run_detect to create a tiny pre-commit fixture and invoke
scripts/verify-local.sh pre-commit with VERIFY_CHANGED_FILES_FILE pointing at
the temp file (similar to run_detect but calling "pre-commit"), and ensure the
test cleans up the temp file; reference run_detect, VERIFY_CHANGED_FILES_FILE,
and scripts/verify-local.sh pre-commit (and the .githooks/pre-commit symlink) so
the test validates the real hook mode and catches regressions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements ADR-0008 Phases 0–3 end to end and moves the live runtime/WASM path onto runtime-owned ingress.
WriterHead,HeadId,PlaybackHeadRegistry,RunnableWriterSet,WorldlineState,WorldlineFrontier, andWorldlineRegistry.SchedulerCoordinatorplusEngine::commit_with_state(...)and kernel stepping throughWorldlineRuntime.dispatch_intent()/step().What Changed In The Final Phase 3 Cut
WorldlineRuntimeis the live ingress path.WorldlineStateand added per-head committed-ingress tracking.WorldlineRuntime::ingest(...),SchedulerCoordinator::super_tick(...),Engine::commit_with_state(...), and state-scoped snapshot helpers.WarpKernelto ownWorldlineRuntime + Engineand route EINT dispatch through runtime ingress instead of the old graph inbox path.Validation
cargo test --workspace --testscargo clippy --workspace --all-targets -- -D warnings -D missing_docscargo fmt --all -- --checkcargo test -p warp-wasm --features engine --lib -- --nocapturepre-commit:cargo clippy,cargo fmtpre-push: workspace testsNotes
Engine::ingest_intent(...),Engine::ingest_inbox_event(...),sys/dispatch_inbox, andsys/ack_pendingremain only for isolated compatibility tests and older spike paths.sim/inbox,edge:pending, ordispatch_next_intent(...).54f4d71and the merged remote branch tip3a6ce05.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores