Phase 7: full WorldlineState replay and checkpoint-backed playback#304
Open
flyingrobots wants to merge 20 commits intomainfrom
Open
Phase 7: full WorldlineState replay and checkpoint-backed playback#304flyingrobots wants to merge 20 commits intomainfrom
flyingrobots wants to merge 20 commits intomainfrom
Conversation
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 local git-hook timing instrumentation and PR-status tooling; migrates runtime ticks to typed logical counters (WorldlineTick/GlobalTick/RunId) and threads them through provenance, playback, and worldline state; introduces ABI v3 (observe/dispatch_intent/scheduler_status) with privileged control-intent handling; updates tests, docs, and tooling. Changes
Sequence Diagram(s)sequenceDiagram
actor Host as Client/Host
participant Kernel as WarpKernel
participant Intent as Intent Decoder
participant Scheduler as SchedulerCoordinator
participant Runtime as WorldlineRuntime
Host->>Kernel: dispatch_intent(bytes)
Kernel->>Intent: unpack_intent_v1(bytes)
alt control intent (op_id == CONTROL_INTENT_V1_OP_ID)
Intent-->>Kernel: ControlIntentV1
Kernel->>Scheduler: apply_control_intent(intent)
Scheduler->>Runtime: run super_tick cycles / update heads
Runtime-->>Scheduler: commit_global_tick / state updates
Scheduler-->>Kernel: SchedulerStatus
Kernel-->>Host: DispatchResponse { accepted: true, scheduler_status }
else domain intent
Intent-->>Kernel: Domain intent
Kernel->>Runtime: ingest intent -> IngressTarget
Runtime-->>Kernel: IngressDisposition
Kernel-->>Host: DispatchResponse { accepted/declined, scheduler_status }
end
sequenceDiagram
actor User as Client
participant Cursor as PlaybackCursor
participant Prov as ProvenanceStore
participant Replay as Replay Engine
participant State as WorldlineState
User->>Cursor: seek_to(target_worldline_tick)
Cursor->>Prov: checkpoint_state_before(worldline_id, target)
Prov-->>Cursor: Option<ReplayCheckpoint>
Cursor->>Replay: replay_worldline_state_at_from_provenance(...)
Replay->>Prov: entry(worldline_id, tick)
Prov-->>Replay: ProvenanceEntry { ops, commit_global_tick, digest }
Replay->>State: apply_ops_to_state(ops)
State-->>Replay: hydrated WorldlineState at target
Replay-->>Cursor: WorldlineState
Cursor-->>User: CursorReceipt { worldline_tick, commit_global_tick, state_root }
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 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
|
![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: 9f624382df
ℹ️ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/warp-core/tests/slice_theorem_proof.rs (1)
569-583:⚠️ Potential issue | 🟠 MajorThese fixtures stamp impossible
commit_global_tickvalues.
SchedulerCoordinator::super_tick()incrementsglobal_tickbefore recording a commit, so a fresh run's first commit isGlobalTick(1), notGlobalTick(0). The loop here starts at zero, and the dependent-chain fixture hard-codesZEROfor what is modeled as the second runtime commit. That makes the proof data inconsistent with real provenance and can hide off-by-one bugs in code that consumes commit metadata.Suggested fix
- commit_global_tick: warp_core::GlobalTick::from_raw(tick), + commit_global_tick: warp_core::GlobalTick::from_raw(tick + 1),- commit_global_tick: warp_core::GlobalTick::ZERO, + commit_global_tick: warp_core::GlobalTick::from_raw(2),Also applies to: 796-804
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/tests/slice_theorem_proof.rs` around lines 569 - 583, The test fixtures create impossible commit_global_tick values because SchedulerCoordinator::super_tick() advances ticks before the first commit (so the first real commit is GlobalTick(1)), but the loop that constructs WorldlineTickPatchV1 uses tick starting at 0; update the construction of WorldlineTickHeaderV1.commit_global_tick (or the loop that provides `tick`) to reflect real provenance by using GlobalTick::from_raw(tick + 1) (or start the loop at 1) so commits are 1-based, and apply the same fix to the other occurrence in the dependent-chain fixture referenced around the later block (lines 796-804).crates/warp-core/tests/checkpoint_fork_tests.rs (1)
148-205:⚠️ Potential issue | 🟠 MajorThe “full replay” arm is not actually checkpoint-free.
Both cursors read from the same
provenance, which already contains checkpoints, so the supposed U0 baseline can also hydrate from a checkpoint. The second arm then starts from a cursor already materialized at tick 20, so it never proves direct checkpoint restoration for tick 23 either. Use one provenance store without checkpoints for the baseline and a fresh cursor that seeks straight towt(23)for the accelerated path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/tests/checkpoint_fork_tests.rs` around lines 148 - 205, The full-replay branch incorrectly reuses the same provenance that contains checkpoints and a pre-materialized cursor; instead create/use a provenance without checkpoints for the baseline and use a fresh cursor for the accelerated path: call a setup helper that yields a provenance with no checkpoints (e.g., setup_worldline_without_checkpoints or another setup_worldline_* variant) and use that provenance when constructing the full_replay_cursor passed to PlaybackCursor::new and seek_to(wt(23)); for the checkpoint path, keep using the original provenance that contains checkpoints but instantiate a new PlaybackCursor and call seek_to(wt(23)) directly (not reusing the cursor already materialized at tick 20) so you validate direct checkpoint restoration via checkpoint_before and subsequent seek_to.crates/warp-core/tests/common/mod.rs (1)
760-769:⚠️ Potential issue | 🟠 MajorThese fixture helpers still collapse
WorldlineTickandGlobalTick.
create_add_node_patch()feeds the 0-based fixture tick intocommit_global_tick, andfixture_entry()then derivesworldline_tickback from that global stamp. That gives the first committed patch an impossible global tick of0, and it breaks as soon as global ticks stop matching local worldline ticks 1:1.Also applies to: 803-825, 908-914
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/tests/common/mod.rs` around lines 760 - 769, The helper functions incorrectly treat the provided worldline/local tick as a global tick (producing commit_global_tick == 0 for the first patch); update test_header (and the other helpers at the other ranges) to convert the worldline tick into a proper global tick before assigning commit_global_tick—e.g. use GlobalTick::from_raw(tick + 1) (or the project's canonical worldline->global conversion if one exists) instead of GlobalTick::from_raw(tick); ensure create_add_node_patch, fixture_entry and any other fixture helpers that set commit_global_tick follow the same conversion so worldline_tick and commit_global_tick no longer collapse.crates/echo-wasm-abi/src/kernel_port.rs (1)
337-375:⚠️ Potential issue | 🟡 MinorUse one
state_rootmeaning throughout the observation artifact.
ResolvedObservationCoordinate.state_rootis documented as the canonical root at the observed coordinate, butHeadObservation.state_rootandSnapshotObservation.state_rootare still described as “Graph-only”. Under the Phase 7 full-state model, that leaves the wire contract ambiguous about whether those hashes should match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/echo-wasm-abi/src/kernel_port.rs` around lines 337 - 375, The docs are inconsistent: ResolvedObservationCoordinate.state_root is the canonical root while HeadObservation.state_root and SnapshotObservation.state_root are labeled “Graph-only”, creating ambiguity; update the field documentation so all three structs (ResolvedObservationCoordinate.state_root, HeadObservation.state_root, SnapshotObservation.state_root) use the same, unambiguous meaning (canonical/full-state root under Phase 7) or else introduce/rename a distinct field for a graph-only hash; ensure the comments for HeadObservation and SnapshotObservation match ResolvedObservationCoordinate or add a clearly named separate field if both hashes must be carried.docs/plans/adr-0008-and-0009.md (1)
724-759:⚠️ Potential issue | 🟡 MinorPhase 7 is still under-documented relative to what this PR implements.
This section is marked implemented, but it only talks about snapshot/fork replay. The implemented Phase 7 surface also moved playback seek/step onto checkpoint-backed provenance replay; leaving that out makes the implementation plan stale against the code.
As per coding guidelines, documentation accuracy matters — especially anything touching determinism guarantees, hash stability, or canonical ordering. Flag factual errors and stale cross-references.
🤖 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 724 - 759, The Phase 7 section is stale and incomplete: update the implemented "Phase 7" text to reflect that playback seek/step were moved onto checkpoint-backed provenance replay (in addition to snapshot/fork replay), and correct any cross-references or claims about where replay helpers (e.g., apply_to_worldline_state, commit_with_state, snapshot_at, fork) are used versus excluded (live writer-advance paths); ensure the doc explicitly states the new replay surface (checkpoint-backed provenance replay for playback seek/step), notes any removed/unsupported ops, and fixes wording about determinism/hash stability/canonical ordering to match the current code behavior.
🤖 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 49-53: Add a brief doc comment to HeadId::from_bytes clarifying
its intended use for round-trip deserialization/persistence and warning that it
does not perform the same creation semantics/validations as make_head_id (or
other constructors); update the doc on the function named from_bytes (type
HeadId) to state it reconstructs a HeadId from canonical bytes for
persistence/transport and should be used only for deserialization, not for
generating new IDs from untrusted input.
In `@crates/warp-core/src/observation.rs`:
- Around line 198-202: The struct changes (new fields like
resolved_worldline_tick, commit_global_tick, observed_after_global_tick and the
other added fields in the ranges you flagged) alter the serialized CBOR/hash
input, so keepers/consumers must not advertise observation_version = 1; update
the observation_version constant/advertisement to a new version (e.g., 2) and
any place that emits or checks observation_version to reflect the new schema;
ensure tests and any serialization/compatibility code that branches on
observation_version (search for observation_version, OBSERVATION_VERSION, or
similar in this crate) are updated to handle the bumped version and/or provide
explicit backward-compat handling for v1 vs v2 payloads.
In `@crates/warp-core/src/playback.rs`:
- Around line 607-618: The StepForward branch currently calls
self.tick.checked_increment() and then errors with
SeekError::PinnedFrontierExceeded when a reader is parked at pin_max_tick;
change it to mirror Play's behavior: if self.role == CursorRole::Reader and
self.tick == self.pin_max_tick return StepResult::ReachedFrontier (or the
equivalent early-return) instead of attempting to increment, otherwise perform
the checked_increment and call seek_to as before; update the logic around
PlaybackMode::StepForward, CursorRole::Reader, tick.checked_increment(),
pin_max_tick, and seek_to to detect the pinned frontier first and avoid raising
PinnedFrontierExceeded.
- Around line 419-438: The constructor PlaybackCursor::new currently sets tick
to WorldlineTick::ZERO but blindly clones initial_state into state, which can
violate the tick/state invariant; update new() to validate or normalize
initial_state so state corresponds to tick ZERO: either assert/return an error
if initial_state.tick() != WorldlineTick::ZERO (e.g.
assert_eq!(initial_state.tick(), WorldlineTick::ZERO) or change the signature to
return Result and return an Err) or transform initial_state into the replay base
(normalize/rewind it) before assigning to self.state; ensure you reference the
new(), tick, state and initial_state symbols while making the change.
- Around line 519-537: The code currently calls checkpoint_state_before(...) to
decide whether to restore, which clones a full ReplayCheckpoint (and its
WorldlineState) and then replay_worldline_state_at_from_provenance(...) may
clone it again—avoid the double allocation by probing with the lightweight
checkpoint_before(...) (or an equivalent opt-check) and only
materializing/fetching the full ReplayCheckpoint once when you actually perform
the restore; update the conditional to use checkpoint_before(...) (or store the
Option without cloning) and change the restore path to obtain the full
checkpoint just once (either by calling checkpoint_state_before only in the
restore branch or by passing the fetched checkpoint into
replay_worldline_state_at_from_provenance/replay helper) while keeping
advance_replay_state usage unchanged and preserving error mapping via
Self::map_replay_error.
In `@crates/warp-core/src/provenance_store.rs`:
- Around line 757-775: The code currently only compares
checkpoint.state.state_root() to checkpoint.checkpoint.state_hash (both from the
checkpoint artifact); instead, fetch the authoritative boundary/root for the
checkpoint's tick from provenance (e.g. call a boundary lookup like
provenance.boundary_at(worldline_id, checkpoint_tick) or
provenance.boundary_for(worldline_id, checkpoint_tick)), compare that
authoritative root to the restored actual_state_root, and return
ReplayError::CheckpointStateRootMismatch if they differ; keep the existing
hydrate_replay_metadata and return path only when the restored root matches the
authoritative boundary for checkpoint.checkpoint.worldline_tick.
- Around line 678-680: The code uses unchecked arithmetic when constructing
TxId::from_raw(tick.as_u64() + 1), which can overflow and behave
non-deterministically; replace this with a checked increment on the tick (e.g.,
use tick.checked_increment() or tick.checked_add(1)) and propagate a
deterministic error (using ok_or(...) or ? returning the appropriate error)
before creating the TickReceipt (TickReceipt::new), ensuring no unchecked
as_u64()+1 remains.
In `@crates/warp-core/src/worldline_state.rs`:
- Around line 207-233: from_root_store currently never verifies that the
provided root_node exists in the supplied GraphStore, allowing construction of a
WorldlineState with a dangling root; update from_root_store (before creating
NodeKey/WarpInstance and calling Self::new) to query the GraphStore to confirm
the node exists (e.g. call whatever lookup method the store exposes such as
get_node/node_exists/get_node_by_id and handle its Option/Result), and if the
node is missing return a descriptive WorldlineStateError (add a new variant like
MissingRoot or reuse an appropriate error) instead of proceeding to
upsert_instance and Self::new; reference functions/types: from_root_store,
GraphStore (store), store.warp_id(), NodeKey, WarpInstance,
WarpState::upsert_instance, and WorldlineStateError.
In `@crates/warp-core/src/worldline.rs`:
- Around line 132-150: apply_to_worldline_state currently only replays graph ops
via apply_ops_to_state(&mut state.warp_state, &self.ops) leaving WorldlineState
bookkeeping (tick_history, last_snapshot, etc.) stale; update the function to
also update the full-state bookkeeping after applying ops — e.g., apply or merge
any tick-history/snapshot info carried by the patch into state.tick_history and
state.last_snapshot or recompute/advance them based on self.ops so the
WorldlineState is fully consistent after apply_to_worldline_state returns
(referencing apply_to_worldline_state, WorldlineState, tick_history,
last_snapshot, apply_ops_to_state, and self.ops).
In `@crates/warp-core/tests/playback_cursor_tests.rs`:
- Around line 250-336: The test currently can pass by replaying the single
provenance entry rather than using the checkpoint; modify the test so the seek
target is strictly beyond the checkpoint (or remove the prefix history) so
success requires restoring the ReplayCheckpoint state. Concretely, keep using
add_checkpoint(…, ReplayCheckpoint { checkpoint: CheckpointRef { worldline_tick:
wt(1), … }, state: checkpoint_state }) but append the provenance entry at a
later tick (e.g. wt(2)) or set the cursor to start at wt(0) and call
PlaybackCursor::seek_to(...) with a target > wt(1), so PlaybackCursor::seek_to
must consult the ReplayCheckpoint rather than simply replaying the lone
append_fixture_entry; update uses of append_fixture_entry, PlaybackCursor::new,
and seek_to/wt accordingly.
In `@crates/warp-wasm/src/lib.rs`:
- Around line 273-277: Update the rustdoc for the public function
scheduler_status() (the wasm binding that returns a Uint8Array via
encode_result(with_kernel_ref(|k| k.scheduler_status()))) to explicitly document
the ABI contract: state that it returns a CBOR-encoded SchedulerStatus envelope
(schema/semantics) and that callers will get a NOT_INITIALIZED error until a
kernel is installed; also note that the returned Uint8Array is read-only and
describe expected consumer behavior (e.g., decode with CBOR to SchedulerStatus).
Keep the comment adjacent to the #[wasm_bindgen] pub fn scheduler_status()
signature so the contract is clear to downstream consumers.
In `@crates/warp-wasm/src/warp_kernel.rs`:
- Around line 363-379: The code sets self.scheduler_status (state, active_mode,
run_id) before entering the loop but returns early on errors from
SchedulerCoordinator::super_tick, leaving a stale "running" state; update the
error path so that when super_tick returns Err you first clear the running state
by setting self.scheduler_status.state = SchedulerState::Stopped,
self.scheduler_status.active_mode = None, self.scheduler_status.run_id = None
and then call self.refresh_scheduler_status() before returning the AbiError;
apply the same clearing+refresh logic to the later error paths around where
scheduler cleanup currently happens (the logic that updates scheduler_status at
the end of the run) so no error-returning path leaves a cached active run.
- Around line 388-398: The loop under Start::UntilIdle can hang when work
becomes WorkState::BlockedOnly (e.g., head marked Dormant) because super_tick
yields no records and cycle_limit is None; change the loop in the Run/dispatch
logic to detect BlockedOnly/no-progress and exit like Quiescent: call
current_work_state() and if it returns WorkState::BlockedOnly (or the head is
Dormant) AND a call to super_tick() produced no records (or
dispatch_intent(Start) would block), set scheduler_status.last_run_completion to
a new appropriate RunCompletion (e.g., BlockedOrDormant) and break;
alternatively ensure dispatch_intent(Start) returns immediately for Dormant
heads—update places referencing Start::UntilIdle, current_work_state(),
super_tick(), dispatch_intent(Start), WorkState::BlockedOnly and
scheduler_status.last_run_completion to implement this early-exit behavior so
the loop cannot hang when cycle_limit is None.
In `@docs/adr/ADR-0010-observational-seek-and-administrative-rewind.md`:
- Line 6: The ADR titled "ADR-0010-observational-seek-and-administrative-rewind"
is marked "Status: Accepted" yet later contains the conditional phrase "If
accepted…" which is now stale; update the sentence containing "If accepted…" to
unconditional wording (e.g., "This ADR is accepted, so …" or remove the
conditional clause) so the document consistently reflects the accepted status.
In `@docs/march-16.plan.md`:
- Around line 140-145: The "Goals:" section is stale and contradictory: it still
instructs bumping the WASM ABI to v2 and marking phases 0–5 while later text
references ABI v3 and Phase 7; update the document so it no longer misstates
migration state by either (A) rewriting the "Goals:" and surrounding narrative
as retrospective context that accurately records historical ABI v2 intent and
the completed ABI v3/Phase 7 work, or (B) moving the file to an archive location
and adding a short header noting it is archived/stale. Specifically, edit the
"Goals:" bullet list to reflect ABI v3 and that Phase 7 is complete (replace
"bump the ABI to v2" and "mark only phases 0–5 as implemented"), reconcile all
mentions of "tick semantics" to state the current type-system decision, and fix
stale cross-references elsewhere in the doc (the other contradictory passages
that mention ABI/phase state) so the document is internally consistent or
clearly marked as archival.
In `@docs/plans/adr-0008-and-0009.md`:
- Around line 364-367: The docs contain a stale signature for frontier_tick that
conflicts with the new typed-clock type: update the earlier runtime sketch where
it still reads "fn frontier_tick(&self) -> u64;" to use the new WorldlineTick
type (i.e., make the signature consistent with the later declaration of
frontier_tick: WorldlineTick and any trait methods like fn frontier_tick(&self)
-> WorldlineTick), and check related cross-references to WorldlineId and
WorldlineState to ensure all signatures and comments reference the same types.
In `@Makefile`:
- Around line 31-32: The pr-status recipe currently expands the Make variable PR
unquoted which allows shell metacharacters to be interpreted; update the
pr-status target (pr-status) to pass PR safely to the script by quoting the
expansion and/or using a separator, e.g. invoke ./scripts/pr-status.sh with
"$(PR)" (or -- "$(PR)") so the PR value is treated as a single literal argument
and prevents shell command injection when the variable contains special
characters.
In `@scripts/pr-status.sh`:
- Around line 9-10: Remove the unused shell variables OWNER and REPO from
scripts/pr-status.sh by deleting their declarations (OWNER="flyingrobots" and
REPO="echo"); if they are meant for future use instead, add a short comment
explaining intent or prefix them with an underscore (e.g., _OWNER) and/or
export/use them where needed to avoid shellcheck warnings—make sure no other
symbols reference OWNER/REPO before removing.
In `@scripts/verify-local.sh`:
- Around line 1440-1443: The logic sets VERIFY_STAMP_SUBJECT to the committed
tree (git rev-parse HEAD^{tree}) for cases that should use the actual
working-tree, causing MODE=full to be cached by the committed-tree; change the
conditional so that when VERIFY_MODE_CONTEXT is "pre-commit" or "working-tree"
OR when VERIFY_MODE equals "full" you set VERIFY_STAMP_SUBJECT="$(git
write-tree)" (instead of using git rev-parse HEAD^{tree}); update the same
pattern at the later block referenced (lines 1480-1483) so full runs are keyed
to the current working tree rather than the committed tree.
In `@tests/hooks/test_pr_status.sh`:
- Around line 70-73: Replace the hardcoded absolute repo path in the subshell
that runs PATH="$tmp/bin:$PATH" ./scripts/pr-status.sh 302 with a relative or
variable-based path so the test is portable; remove "/Users/james/git/echo" and
instead use the repo root already established by the earlier cd (e.g., use "."
or "$PWD" or a REPO_ROOT variable) before invoking ./scripts/pr-status.sh,
ensuring the subshell still sets PATH="$tmp/bin:$PATH" and runs the same script
invocation.
---
Outside diff comments:
In `@crates/echo-wasm-abi/src/kernel_port.rs`:
- Around line 337-375: The docs are inconsistent:
ResolvedObservationCoordinate.state_root is the canonical root while
HeadObservation.state_root and SnapshotObservation.state_root are labeled
“Graph-only”, creating ambiguity; update the field documentation so all three
structs (ResolvedObservationCoordinate.state_root, HeadObservation.state_root,
SnapshotObservation.state_root) use the same, unambiguous meaning
(canonical/full-state root under Phase 7) or else introduce/rename a distinct
field for a graph-only hash; ensure the comments for HeadObservation and
SnapshotObservation match ResolvedObservationCoordinate or add a clearly named
separate field if both hashes must be carried.
In `@crates/warp-core/tests/checkpoint_fork_tests.rs`:
- Around line 148-205: The full-replay branch incorrectly reuses the same
provenance that contains checkpoints and a pre-materialized cursor; instead
create/use a provenance without checkpoints for the baseline and use a fresh
cursor for the accelerated path: call a setup helper that yields a provenance
with no checkpoints (e.g., setup_worldline_without_checkpoints or another
setup_worldline_* variant) and use that provenance when constructing the
full_replay_cursor passed to PlaybackCursor::new and seek_to(wt(23)); for the
checkpoint path, keep using the original provenance that contains checkpoints
but instantiate a new PlaybackCursor and call seek_to(wt(23)) directly (not
reusing the cursor already materialized at tick 20) so you validate direct
checkpoint restoration via checkpoint_before and subsequent seek_to.
In `@crates/warp-core/tests/common/mod.rs`:
- Around line 760-769: The helper functions incorrectly treat the provided
worldline/local tick as a global tick (producing commit_global_tick == 0 for the
first patch); update test_header (and the other helpers at the other ranges) to
convert the worldline tick into a proper global tick before assigning
commit_global_tick—e.g. use GlobalTick::from_raw(tick + 1) (or the project's
canonical worldline->global conversion if one exists) instead of
GlobalTick::from_raw(tick); ensure create_add_node_patch, fixture_entry and any
other fixture helpers that set commit_global_tick follow the same conversion so
worldline_tick and commit_global_tick no longer collapse.
In `@crates/warp-core/tests/slice_theorem_proof.rs`:
- Around line 569-583: The test fixtures create impossible commit_global_tick
values because SchedulerCoordinator::super_tick() advances ticks before the
first commit (so the first real commit is GlobalTick(1)), but the loop that
constructs WorldlineTickPatchV1 uses tick starting at 0; update the construction
of WorldlineTickHeaderV1.commit_global_tick (or the loop that provides `tick`)
to reflect real provenance by using GlobalTick::from_raw(tick + 1) (or start the
loop at 1) so commits are 1-based, and apply the same fix to the other
occurrence in the dependent-chain fixture referenced around the later block
(lines 796-804).
In `@docs/plans/adr-0008-and-0009.md`:
- Around line 724-759: The Phase 7 section is stale and incomplete: update the
implemented "Phase 7" text to reflect that playback seek/step were moved onto
checkpoint-backed provenance replay (in addition to snapshot/fork replay), and
correct any cross-references or claims about where replay helpers (e.g.,
apply_to_worldline_state, commit_with_state, snapshot_at, fork) are used versus
excluded (live writer-advance paths); ensure the doc explicitly states the new
replay surface (checkpoint-backed provenance replay for playback seek/step),
notes any removed/unsupported ops, and fixes wording about determinism/hash
stability/canonical ordering to match the current code behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d911773-3781-45ab-8b86-ab7a6c761de2
📒 Files selected for processing (52)
.githooks/_timing.sh.githooks/commit-msg.githooks/pre-commit.githooks/pre-push.githooks/pre-push-parallel.githooks/pre-push-sequential.githooks/pre-rebase.gitignoreCHANGELOG.mdMakefilecrates/echo-wasm-abi/src/kernel_port.rscrates/echo-wasm-abi/src/lib.rscrates/warp-core/README.mdcrates/warp-core/src/clock.rscrates/warp-core/src/coordinator.rscrates/warp-core/src/head.rscrates/warp-core/src/lib.rscrates/warp-core/src/observation.rscrates/warp-core/src/playback.rscrates/warp-core/src/provenance_store.rscrates/warp-core/src/tick_patch.rscrates/warp-core/src/worldline.rscrates/warp-core/src/worldline_registry.rscrates/warp-core/src/worldline_state.rscrates/warp-core/tests/checkpoint_fork_tests.rscrates/warp-core/tests/common/mod.rscrates/warp-core/tests/golden_vectors_phase0.rscrates/warp-core/tests/inbox.rscrates/warp-core/tests/invariant_property_tests.rscrates/warp-core/tests/outputs_playback_tests.rscrates/warp-core/tests/playback_cursor_tests.rscrates/warp-core/tests/slice_theorem_proof.rscrates/warp-core/tests/view_session_tests.rscrates/warp-wasm/README.mdcrates/warp-wasm/src/lib.rscrates/warp-wasm/src/warp_kernel.rsdocs/ROADMAP/first-light/browser-visualization.mddocs/ROADMAP/first-light/wasm-runtime.mddocs/adr/ADR-0010-observational-seek-and-administrative-rewind.mddocs/adr/ADR-0011-explicit-observation-contract.mddocs/march-16.plan.mddocs/plans/adr-0008-and-0009.mddocs/spec/SPEC-0009-wasm-abi-v1.mddocs/spec/SPEC-0009-wasm-abi-v3.mdscripts/hooks/README.mdscripts/plot-prepush-timing.mjsscripts/pr-status.shscripts/verify-local.shtests/hooks/test_hook_issues.shtests/hooks/test_hook_timing.shtests/hooks/test_pr_status.shtests/hooks/test_verify_local.sh
💤 Files with no reviewable changes (1)
- docs/spec/SPEC-0009-wasm-abi-v1.md
Owner
Author
|
@coderabbitai Thanks. This one is a mix-up rather than a replay bug. In |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
Owner
Author
|
@coderabbitai review please 🥕 |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/hooks/test_verify_local.sh (1)
224-340: 🧹 Nitpick | 🔵 TrivialSignificant stub duplication between
run_fake_full_stamp_sequenceandrun_fake_verify.The fake
git,cargo,rg,npx, andrustupstubs are copy-pasted. If stub behavior changes (e.g., new git subcommand handling), both functions need identical updates. Consider extracting stub setup into a shared helper.Extract common stub scaffolding
# Add helper near top of file setup_fake_toolchain_stubs() { local tmp="$1" mkdir -p "$tmp/bin" cat >"$tmp/bin/cargo" <<'EOF' #!/usr/bin/env bash set -euo pipefail printf '%s|%s\n' "${CARGO_TARGET_DIR:-}" "$*" >>"${VERIFY_FAKE_CARGO_LOG}" exit 0 EOF # ... rest of stubs ... chmod +x "$tmp/bin/cargo" "$tmp/bin/cargo-nextest" "$tmp/bin/rustup" "$tmp/bin/rg" "$tmp/bin/npx" "$tmp/bin/git" } # Then in run_fake_verify and run_fake_full_stamp_sequence: setup_fake_toolchain_stubs "$tmp"🤖 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 224 - 340, The run_fake_full_stamp_sequence function duplicates the fake tool stubs (cargo, rustup, rg, npx, git) that are also used by run_fake_verify; extract those into a shared helper (e.g., setup_fake_toolchain_stubs) that takes the temporary directory as an argument, creates $tmp/bin, writes each stub (preserving use of VERIFY_FAKE_CARGO_LOG, VERIFY_FAKE_NPX_LOG, VERIFY_FAKE_GIT_HEAD, VERIFY_FAKE_GIT_TREE) and makes them executable, then replace the inline stub blocks in run_fake_full_stamp_sequence and run_fake_verify with a call to setup_fake_toolchain_stubs "$tmp".crates/warp-core/src/worldline_state.rs (1)
383-388:⚠️ Potential issue | 🟠 MajorInitialize the frontier from the state's actual tick.
WorldlineFrontier::new()can now receive aWorldlineStatewith non-emptytick_history, but it still hard-resetsfrontier_ticktoZERO. Any restored/checkpointed state built through this constructor will report itself at tick 0 even whenstate.current_tick()is higher.Patch
pub fn new(worldline_id: WorldlineId, state: WorldlineState) -> Self { + let frontier_tick = state.current_tick(); Self { worldline_id, state, - frontier_tick: WorldlineTick::ZERO, + frontier_tick, } }🤖 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 383 - 388, WorldlineFrontier::new currently initializes frontier_tick to WorldlineTick::ZERO which loses restored progress; instead set frontier_tick from the provided state's current tick (call state.current_tick() / WorldlineState::current_tick()) so a deserialized/restored WorldlineFrontier reflects the actual checkpointed tick. Update the constructor in new(worldline_id: WorldlineId, state: WorldlineState) to compute frontier_tick = state.current_tick() (or equivalent) while keeping worldline_id and state assignment intact.crates/warp-core/src/provenance_store.rs (2)
1205-1222:⚠️ Potential issue | 🟠 MajorForks drop the checkpoint at the copied tip.
fork_tickis an inclusive entry index, but checkpoint coordinates are cursor ticks. Copying entries throughfork_tickwhile filtering checkpoints with<= fork_tickdiscards the valid checkpoint atfork_tick + 1(state after the copied tip), so checkpoint-backed replay on the forked worldline falls back to replaying the whole prefix.Suggested fix
let end_idx = fork_tick .checked_increment() .ok_or(HistoryError::HistoryUnavailable { tick: fork_tick })? .as_u64() as usize; + let checkpoint_limit = fork_tick + .checked_increment() + .ok_or(HistoryError::HistoryUnavailable { tick: fork_tick })?; let new_history = WorldlineHistory { u0_ref: source_history.u0_ref, initial_boundary_hash: source_history.initial_boundary_hash, entries: source_history.entries[..end_idx] .iter() @@ checkpoints: source_history .checkpoints .iter() - .filter(|c| c.checkpoint.worldline_tick <= fork_tick) + .filter(|c| c.checkpoint.worldline_tick <= checkpoint_limit) .cloned() .collect(), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/provenance_store.rs` around lines 1205 - 1222, The checkpoint filter is off-by-one: since fork_tick is an inclusive entry index but checkpoints use cursor ticks, we must include the checkpoint at the cursor immediately after the copied tip. Change the checkpoints filter in the WorldlineHistory construction to compare checkpoint.worldline_tick <= fork_tick.checked_increment() (or use the same checked_increment result used to compute end_idx) and propagate the same overflow/error handling, so the checkpoint at fork_tick+1 is retained for the forked history.
1324-1335:⚠️ Potential issue | 🟠 MajorReject patch-digest mismatches before appending history.
replay_artifacts_for_entry()now treatsexpected.patch_digest != entry.patch.patch_digestas fatal, butappend_local_commit()still stores that inconsistency. One malformed local commit makes the authoritative worldline unreplayable and the failure only appears later during seek/playback. Validate the patch digest here and return aHistoryErrorinstead of poisoning the store.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/provenance_store.rs` around lines 1324 - 1335, append_local_commit currently appends entries even when the incoming entry.patch.patch_digest disagrees with the expected patch digest used by replay_artifacts_for_entry, which later makes the worldline unreplayable; before pushing the entry (after computing expected_tick and validating via validate_local_commit_entry), call replay_artifacts_for_entry(entry.worldline_id, expected_tick) to obtain the expected patch digest and compare it to entry.patch.patch_digest, and if they differ return an appropriate HistoryError instead of pushing to history.entries so malformed local commits are rejected immediately.
♻️ Duplicate comments (2)
crates/warp-core/tests/playback_cursor_tests.rs (1)
289-311:⚠️ Potential issue | 🟠 MajorThis test still passes without using the checkpoint.
With one committed patch and a target of
wt(1),seek_to()can rebuild from U0 and populate the same snapshot/materialization metadata. To actually verify checkpoint hydration, seek past the checkpoint or remove/corrupt the prefix so onlyReplayCheckpoint.statemakes the call succeed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/tests/playback_cursor_tests.rs` around lines 289 - 311, The test currently still passes without using the ReplayCheckpoint because PlaybackCursor::seek_to can rebuild state from U0; update the test where add_checkpoint(...) and PlaybackCursor::new(...) are used so that the cursor actually has to hydrate from ReplayCheckpoint.state — for example, change the seek target to a tick beyond the checkpoint (seek_to wt(>1)) or corrupt/remove the prefix/materialization data in initial_state before calling cursor.seek_to so rebuilding from U0 fails and only ReplayCheckpoint.state can satisfy the seek; ensure the referenced symbols are PlaybackCursor::new, cursor.seek_to, add_checkpoint, and ReplayCheckpoint.state.crates/warp-core/tests/checkpoint_fork_tests.rs (1)
180-201:⚠️ Potential issue | 🟠 MajorThis still doesn’t prove
seek_to()restored from the checkpoint.
seek_to(wt(23), ...)will also pass if the cursor ignores checkpoints and simply replays ticks 0..22 from U0. The extracheckpoint_state_before(...)assertion only re-reads provenance metadata. Make the prefix before tick 20 unavailable/corrupt, or otherwise force success to depend on hydratingReplayCheckpoint.state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/tests/checkpoint_fork_tests.rs` around lines 180 - 201, The test currently doesn't prove PlaybackCursor::seek_to used the checkpoint because seek could have simply replayed ticks 0..22; modify the test so replay from U0 fails unless the checkpoint's hydrated state is used—e.g., make the prefix before tick 20 unavailable/corrupt in provenance (so provenance events or U0 data before worldline_tick 20 cannot be read) or change initial_state such that replayed application of ticks 0..22 would not reach the expected state; then call checkpoint_cursor.seek_to(wt(23), ...) and assert the cursor's restored state matches the checkpoint's hydrated state (compare checkpoint_cursor.current_state_root() to the state/root in checkpoint_restore_state.checkpoint or ReplayCheckpoint.state) to prove seek relied on ReplayCheckpoint.state rather than full replay.
🤖 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/ttd-browser/src/lib.rs`:
- Around line 155-170: The code is synthesizing a fake U0 root store
(`make_node_id("root")`, `make_type_id("world")`,
`WorldlineState::from_root_store`) before calling
`register_worldline_with_boundary`, which breaks real initial state handling;
instead, use the initial WorldlineState or its exact boundary hash provided by
the caller rather than constructing a synthetic one: remove the artificial
`initial_store`/`initial_state` creation, accept (or forward) the
caller-provided `WorldlineState` or its `state_root()` into
`self.provenance.register_worldline_with_boundary(worldline, warp,
initial_state.state_root())`, and store that same `initial_state` into
`self.initial_stores.insert(worldline, initial_state)` so the registered
boundary matches the persisted initial state (update `register_worldline`/call
sites to pass the real initial state or boundary if needed).
In `@crates/warp-core/src/head.rs`:
- Around line 346-350: The test suite currently only verifies pause-based
exclusion but rebuild() now filters by head.is_admitted() as well; add a
regression test that uses PlaybackHeadRegistry and the scheduler admission API
to create a head, mark it dormant (or otherwise set it non-admitted), assert
that rebuild() excludes it, then readmit it and assert rebuild() includes it
again—i.e., add a dormant/readmit roundtrip in the existing test that exercises
pause handling so admission gating (is_admitted()) cannot regress independently
of is_paused().
In `@crates/warp-core/src/playback.rs`:
- Around line 531-543: The code clones the entire WorldlineState before
advancing on the fast path (see advance_replay_state and
replay_worldline_state_at_from_provenance), causing an O(state_size) allocation
per forward tick; instead, avoid full-state clone on the hot path by mutating
state in-place when replay failures can invalidate the cursor (apply patches
directly to self.state) and only fall back to cloning when you must preserve the
original (i.e. when you need transactional buffering); move any transactional
buffering or copy-on-write logic below the point where you currently clone the
whole state so Play/StepForward call advance_replay_state on a mutable reference
to self.state unless preserving the prior state is mandatory, and update the
other affected branches (the similar blocks around the 589-605 and 620-629
regions) to the same in-place-first strategy.
In `@crates/warp-core/src/provenance_store.rs`:
- Around line 814-819: The returned checkpoint reuses checkpoint.state (an
acceleration artifact) but only calls hydrate_replay_metadata(provenance,
worldline_id, &mut replayed, checkpoint_tick) which fills missing history; if
checkpoint.state.tick_history already equals checkpoint_tick stale/corrupt
history remains. Before hydrating (in the block that sets let mut replayed =
checkpoint.state and clears last_materialization_errors/committed_ingress),
fully rebuild replayed.tick_history (and any other provenance-derived metadata
used by last_snapshot() / snapshot_at_state()) from provenance for the given
worldline_id and checkpoint_tick, or clear replayed.tick_history so
hydrate_replay_metadata must repopulate it; ensure the logic touches the same
replayed struct passed to hydrate_replay_metadata so subsequent calls to
last_snapshot() / snapshot_at_state() reflect provenance-only metadata.
- Around line 1062-1065: The cast parent.worldline_tick.as_u64() as usize
overflows on 32-bit targets; replace those direct casts with
usize::try_from(<tick>.as_u64()) and propagate failures into the appropriate
HistoryError: map conversion failures to HistoryError::MissingParentRef in
append_local_commit/parent validation and entry(), and to
HistoryError::HistoryUnavailable for hydrate_replay_metadata (target_len) and
fork_at (end_idx). Update the four sites referenced (hydrate_replay_metadata
target_len, append_local_commit parent validation, fork_at end_idx, and entry())
to perform try_from, handle the Result, and return the matching HistoryError
variant on Err so behavior is deterministic across architectures.
In `@crates/warp-core/tests/common/mod.rs`:
- Around line 760-763: The helper fixture_commit_global_tick currently
hard-couples GlobalTick to WorldlineTick by returning
GlobalTick::from_raw(worldline_tick.saturating_add(1)), which breaks when
reusing fixtures across existing history or forks; change it to stop assuming a
+1 offset — either remove the implicit mapping and require callers to supply the
desired GlobalTick explicitly, or compute the GlobalTick from the actual
fixture/header metadata (e.g., read the header.commit_global_tick or a provided
base offset) instead of using worldline_tick.saturating_add(1); update any
callers of fixture_commit_global_tick to pass the correct commit global tick (or
to use the header’s commit_global_tick) so tests reflect realistic timelines.
In `@crates/warp-core/tests/slice_theorem_proof.rs`:
- Line 535: Remove the redundant clone() calls on the store when constructing
WorldlineState and similar calls: call WorldlineState::from_root_store(store,
root) (and the other occurrences around the second instance at lines 814-815)
instead of passing store.clone(), because the original store is not used
afterwards and the clone triggers clippy::redundant_clone; update any other
calls to WorldlineState::from_root_store or equivalent constructors in this file
to accept the owned store value rather than cloning it.
- Around line 585-590: The tests currently derive expected state_root from
wl_patch.apply_to_worldline_state(&mut replay_state) and compare against a
replayed substrate, which can hide bugs; after each patch application (the block
using wl_patch.apply_to_worldline_state, replay_state, state_root,
recorded_roots) materialize an independent WorldlineState from the committed
engine state (e.g., build a WorldlineState from the engine/committed store at
the same sequence index) and assert that its state_root equals the
replay-derived state_root; do the same addition for the second occurrence noted
around the 812-820 region so each step has one independent runtime-side check
that the committed engine state root matches the replayed root (also reference
seek_to if applicable when locating the replay vs committed cursor).
In `@crates/warp-wasm/src/lib.rs`:
- Around line 568-577: The fixture is advertising the wrong observation schema
version: update the ResolvedObservationCoordinate instance used in the test to
set observation_version: 2 (since it contains the v2 fields
resolved_worldline_tick, commit_global_tick, and observed_after_global_tick) so
the test payload matches the v2 contract; locate the
ResolvedObservationCoordinate creation in the test (the struct with fields
observation_version, worldline_id, requested_at, resolved_worldline_tick,
commit_global_tick, observed_after_global_tick, state_root, commit_hash) and
change observation_version from 1 to 2, and if there are any assertions
explicitly expecting version 1 update them accordingly.
In `@crates/warp-wasm/src/warp_kernel.rs`:
- Around line 436-441: The current map_err maps all runtime errors from
self.runtime.set_head_eligibility(key, eligibility) to ENGINE_ERROR; change the
error mapping so that RuntimeError::UnknownHead(_) (from set_head_eligibility)
is translated to AbiError with code error_codes::INVALID_CONTROL and the error
message, and preserve ENGINE_ERROR for other RuntimeError variants—i.e., replace
the blanket closure with a match on the runtime error (checking for UnknownHead)
and return INVALID_CONTROL for that case and ENGINE_ERROR otherwise (referencing
set_head_eligibility and AbiError/error_codes symbols).
- Around line 372-425: The current Start::UntilIdle implementation runs the
entire scheduler loop synchronously (calling SchedulerCoordinator::super_tick)
and calls clear_active_run_state before returning, so a subsequent
ControlIntentV1::Stop cannot interrupt that run; fix by making runs persist
across calls or by removing the "stop at next boundary" ABI promise. Concretely,
either (A) persist the active run state: keep
scheduler_status.run_id/active_mode/stopping flag set and move
clear_active_run_state(false) out of this synchronous loop so the loop can be
resumed/observed across dispatch_intent calls (ensure dispatch_intent sets a
stopping flag the loop checks each iteration and read scheduler_status in the
loop to honor Stop), or (B) change Start::UntilIdle to a cooperative stepper
that returns after one super_tick and document/remove the contract that Stop
interrupts an in-flight synchronous run; update clear_active_run_state,
dispatch_intent, Start::UntilIdle, SchedulerCoordinator::super_tick, and
scheduler_status fields accordingly to match the chosen approach.
In `@docs/plans/adr-0008-and-0009.md`:
- Around line 662-670: The docs reintroduce a removed adapter name: replace the
Phase 7 mention of the deleted WASM entrypoint snapshot_at(...) with the correct
"observation-backed snapshot" path (or its canonical function/adapter name used
elsewhere) so the migration notes reflect the actual surface API; update the
Phase 7 migration bullet and the same references at lines 755-757 to reference
the observation-backed snapshot implementation instead of snapshot_at to
maintain accuracy about entrypoints and determinism guarantees.
In `@scripts/pr-status.sh`:
- Around line 62-76: The python3 -c snippet that populates VIEW_JSON into read
can fail and leave PR_NUMBER, PR_URL, HEAD_SHA, REVIEW_DECISION, MERGE_STATE
empty; modify the block around the read/`python3 -c` invocation to detect python
failures (check the python command exit status and/or capture its
stdout/stderr), and on error print a clear error to stderr and exit non-zero
instead of letting `read -r PR_NUMBER ...` continue with empty values; reference
the existing `read -r PR_NUMBER PR_URL HEAD_SHA REVIEW_DECISION MERGE_STATE
<<EOF` block and the `VIEW_JSON="$VIEW_JSON" python3 -c` command when adding the
error check and early exit or fallback defaults.
In `@scripts/verify-local.sh`:
- Around line 780-790: The subshell that runs each lane (the block invoking
"${lane_funcs[$i]}" and capturing its exit into rc) intentionally uses "set -uo
pipefail" without "-e" so the script can capture the command exit code via "||
rc=$?"; add a brief inline comment directly above or beside the "set -uo
pipefail" line explaining that "-e is omitted on purpose to allow explicit rc
capture (used by the printf/write to ${logdir}/${lane_names[$i]}.elapsed and
subsequent exit $rc)", so future readers understand this is intentional when
inspecting lane_funcs, lane_names, lane_pids and the per-lane logs.
- Around line 56-75: The JSON written by append_timing_record() can be broken if
any field contains quotes, backslashes or newlines; update append_timing_record
to properly JSON-escape all interpolated values (e.g., name, MODE,
VERIFY_MODE_CONTEXT, VERIFY_CLASSIFICATION, VERIFY_RUN_CACHE_STATE,
VERIFY_STAMP_SUBJECT and any timestamps/elapsed/exit strings) before writing the
record—either by using jq (pipe each value through jq -R . to produce a safe
JSON string) when available, or add a small shell helper (json_escape) that
replaces backslash, double-quote and newline with their JSON escape sequences
and use that helper on each variable used in the printf in append_timing_record
to ensure well-formed JSON.
In `@tests/hooks/test_pr_status.sh`:
- Around line 21-78: The temp directory created in run_with_fake_gh may leak if
the subshell exits early; after creating tmp="$(mktemp -d)" (and after mkdir -p
"$tmp/bin") add a trap to always remove it on exit (e.g. trap 'rm -rf "$tmp"'
EXIT) so cleanup runs even on failure, and then remove the explicit rm -rf
"$tmp" at the end of the function; ensure the trap is defined inside
run_with_fake_gh so it references the local tmp variable and is active for all
exit paths.
---
Outside diff comments:
In `@crates/warp-core/src/provenance_store.rs`:
- Around line 1205-1222: The checkpoint filter is off-by-one: since fork_tick is
an inclusive entry index but checkpoints use cursor ticks, we must include the
checkpoint at the cursor immediately after the copied tip. Change the
checkpoints filter in the WorldlineHistory construction to compare
checkpoint.worldline_tick <= fork_tick.checked_increment() (or use the same
checked_increment result used to compute end_idx) and propagate the same
overflow/error handling, so the checkpoint at fork_tick+1 is retained for the
forked history.
- Around line 1324-1335: append_local_commit currently appends entries even when
the incoming entry.patch.patch_digest disagrees with the expected patch digest
used by replay_artifacts_for_entry, which later makes the worldline
unreplayable; before pushing the entry (after computing expected_tick and
validating via validate_local_commit_entry), call
replay_artifacts_for_entry(entry.worldline_id, expected_tick) to obtain the
expected patch digest and compare it to entry.patch.patch_digest, and if they
differ return an appropriate HistoryError instead of pushing to history.entries
so malformed local commits are rejected immediately.
In `@crates/warp-core/src/worldline_state.rs`:
- Around line 383-388: WorldlineFrontier::new currently initializes
frontier_tick to WorldlineTick::ZERO which loses restored progress; instead set
frontier_tick from the provided state's current tick (call state.current_tick()
/ WorldlineState::current_tick()) so a deserialized/restored WorldlineFrontier
reflects the actual checkpointed tick. Update the constructor in
new(worldline_id: WorldlineId, state: WorldlineState) to compute frontier_tick =
state.current_tick() (or equivalent) while keeping worldline_id and state
assignment intact.
In `@tests/hooks/test_verify_local.sh`:
- Around line 224-340: The run_fake_full_stamp_sequence function duplicates the
fake tool stubs (cargo, rustup, rg, npx, git) that are also used by
run_fake_verify; extract those into a shared helper (e.g.,
setup_fake_toolchain_stubs) that takes the temporary directory as an argument,
creates $tmp/bin, writes each stub (preserving use of VERIFY_FAKE_CARGO_LOG,
VERIFY_FAKE_NPX_LOG, VERIFY_FAKE_GIT_HEAD, VERIFY_FAKE_GIT_TREE) and makes them
executable, then replace the inline stub blocks in run_fake_full_stamp_sequence
and run_fake_verify with a call to setup_fake_toolchain_stubs "$tmp".
---
Duplicate comments:
In `@crates/warp-core/tests/checkpoint_fork_tests.rs`:
- Around line 180-201: The test currently doesn't prove PlaybackCursor::seek_to
used the checkpoint because seek could have simply replayed ticks 0..22; modify
the test so replay from U0 fails unless the checkpoint's hydrated state is
used—e.g., make the prefix before tick 20 unavailable/corrupt in provenance (so
provenance events or U0 data before worldline_tick 20 cannot be read) or change
initial_state such that replayed application of ticks 0..22 would not reach the
expected state; then call checkpoint_cursor.seek_to(wt(23), ...) and assert the
cursor's restored state matches the checkpoint's hydrated state (compare
checkpoint_cursor.current_state_root() to the state/root in
checkpoint_restore_state.checkpoint or ReplayCheckpoint.state) to prove seek
relied on ReplayCheckpoint.state rather than full replay.
In `@crates/warp-core/tests/playback_cursor_tests.rs`:
- Around line 289-311: The test currently still passes without using the
ReplayCheckpoint because PlaybackCursor::seek_to can rebuild state from U0;
update the test where add_checkpoint(...) and PlaybackCursor::new(...) are used
so that the cursor actually has to hydrate from ReplayCheckpoint.state — for
example, change the seek target to a tick beyond the checkpoint (seek_to wt(>1))
or corrupt/remove the prefix/materialization data in initial_state before
calling cursor.seek_to so rebuilding from U0 fails and only
ReplayCheckpoint.state can satisfy the seek; ensure the referenced symbols are
PlaybackCursor::new, cursor.seek_to, add_checkpoint, and ReplayCheckpoint.state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4536be97-98d5-4a03-9ebe-df15bb898d85
📒 Files selected for processing (21)
Makefilecrates/echo-wasm-abi/src/kernel_port.rscrates/ttd-browser/src/lib.rscrates/warp-core/src/head.rscrates/warp-core/src/observation.rscrates/warp-core/src/playback.rscrates/warp-core/src/provenance_store.rscrates/warp-core/src/worldline_state.rscrates/warp-core/tests/checkpoint_fork_tests.rscrates/warp-core/tests/common/mod.rscrates/warp-core/tests/playback_cursor_tests.rscrates/warp-core/tests/slice_theorem_proof.rscrates/warp-wasm/src/lib.rscrates/warp-wasm/src/warp_kernel.rsdocs/adr/ADR-0010-observational-seek-and-administrative-rewind.mddocs/march-16.plan.mddocs/plans/adr-0008-and-0009.mdscripts/pr-status.shscripts/verify-local.shtests/hooks/test_pr_status.shtests/hooks/test_verify_local.sh
Comment on lines
+155
to
+170
| let root = make_node_id("root"); | ||
| let mut initial_store = GraphStore::new(warp); | ||
| initial_store.insert_node( | ||
| root, | ||
| NodeRecord { | ||
| ty: make_type_id("world"), | ||
| }, | ||
| ); | ||
| let initial_state = WorldlineState::from_root_store(initial_store, root) | ||
| .map_err(|e| JsError::new(&e.to_string()))?; | ||
|
|
||
| self.provenance | ||
| .register_worldline(worldline, warp) | ||
| .register_worldline_with_boundary(worldline, warp, initial_state.state_root()) | ||
| .map_err(|e| JsError::new(&e.to_string()))?; | ||
|
|
||
| // Create and store initial empty GraphStore for this worldline | ||
| let initial_store = GraphStore::new(warp); | ||
| self.initial_stores.insert(worldline, initial_store); | ||
| self.initial_stores.insert(worldline, initial_state); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Do not synthesize U0 inside register_worldline().
After the replay API change, this WorldlineState is the actual seek base, and its state_root() is the boundary persisted in provenance. Hardcoding a synthetic "root"/"world" store here means the browser engine cannot correctly register any worldline whose real initial state differs from that shape. Take the initial WorldlineState or exact boundary hash from the caller instead of manufacturing one internally.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/ttd-browser/src/lib.rs` around lines 155 - 170, The code is
synthesizing a fake U0 root store (`make_node_id("root")`,
`make_type_id("world")`, `WorldlineState::from_root_store`) before calling
`register_worldline_with_boundary`, which breaks real initial state handling;
instead, use the initial WorldlineState or its exact boundary hash provided by
the caller rather than constructing a synthetic one: remove the artificial
`initial_store`/`initial_state` creation, accept (or forward) the
caller-provided `WorldlineState` or its `state_root()` into
`self.provenance.register_worldline_with_boundary(worldline, warp,
initial_state.state_root())`, and store that same `initial_state` into
`self.initial_stores.insert(worldline, initial_state)` so the registered
boundary matches the persisted initial state (update `register_worldline`/call
sites to pass the real initial state or boundary if needed).
Comment on lines
+662
to
+670
| - Phase 6 Slice A deletes: | ||
| - `get_head` | ||
| - `snapshot_at` | ||
| - `drain_view_ops` | ||
| - `execute_query` | ||
| - `render_snapshot` | ||
| - Phase 6 bumps the ABI version to 2 before other Phase 6 work proceeds. | ||
| - Phase 6 now carries a second intentional break: | ||
| - Slice A removed the legacy read adapters in ABI v2 | ||
| - the typed-clock and intent-shaped scheduler/control rewrite lands in ABI v3 |
Contributor
There was a problem hiding this comment.
Phase 7 reintroduces a deleted adapter name.
The Phase 5/6 notes say snapshot_at(...) was a temporary adapter and then removed from the WASM surface, but the Phase 7 migration bullet names it again as if it were still a live entrypoint. Use the observation-backed snapshot path here instead.
As per coding guidelines, documentation accuracy matters — especially anything touching determinism guarantees, hash stability, or canonical ordering.
Also applies to: 755-757
🤖 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 662 - 670, The docs reintroduce
a removed adapter name: replace the Phase 7 mention of the deleted WASM
entrypoint snapshot_at(...) with the correct "observation-backed snapshot" path
(or its canonical function/adapter name used elsewhere) so the migration notes
reflect the actual surface API; update the Phase 7 migration bullet and the same
references at lines 755-757 to reference the observation-backed snapshot
implementation instead of snapshot_at to maintain accuracy about entrypoints and
determinism guarantees.
Comment on lines
+56
to
+75
| append_timing_record() { | ||
| local record_type="$1" | ||
| local name="$2" | ||
| local elapsed_seconds="$3" | ||
| local exit_status="$4" | ||
|
|
||
| mkdir -p "$(dirname "$VERIFY_TIMING_FILE")" | ||
| printf \ | ||
| '{"ts":"%s","record_type":"%s","mode":"%s","context":"%s","classification":"%s","name":"%s","elapsed_seconds":%s,"exit_status":%s,"cache":"%s","subject":"%s"}\n' \ | ||
| "$(utc_timestamp)" \ | ||
| "$record_type" \ | ||
| "$MODE" \ | ||
| "${VERIFY_MODE_CONTEXT:-unknown}" \ | ||
| "${VERIFY_CLASSIFICATION:-unknown}" \ | ||
| "$name" \ | ||
| "$elapsed_seconds" \ | ||
| "$exit_status" \ | ||
| "${VERIFY_RUN_CACHE_STATE:-fresh}" \ | ||
| "${VERIFY_STAMP_SUBJECT:-unknown}" >>"$VERIFY_TIMING_FILE" | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
JSON output lacks escaping for special characters.
If any of these values ever contain ", \, or newlines, the resulting JSON will be malformed. All current inputs are script-controlled identifiers (lane names, modes, git hashes), so risk is low. If this timing data is consumed by stricter JSON parsers, consider escaping or using jq when available.
Example defensive escaping
append_timing_record() {
local record_type="$1"
local name="$2"
local elapsed_seconds="$3"
local exit_status="$4"
+
+ # Simple escape for JSON string values (handles quotes and backslashes)
+ escape_json() { printf '%s' "$1" | sed 's/\\/\\\\/g; s/"/\\"/g'; }
mkdir -p "$(dirname "$VERIFY_TIMING_FILE")"
printf \
'{"ts":"%s","record_type":"%s","mode":"%s","context":"%s","classification":"%s","name":"%s","elapsed_seconds":%s,"exit_status":%s,"cache":"%s","subject":"%s"}\n' \
"$(utc_timestamp)" \
- "$record_type" \
- "$MODE" \
+ "$(escape_json "$record_type")" \
+ "$(escape_json "$MODE")" \
...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/verify-local.sh` around lines 56 - 75, The JSON written by
append_timing_record() can be broken if any field contains quotes, backslashes
or newlines; update append_timing_record to properly JSON-escape all
interpolated values (e.g., name, MODE, VERIFY_MODE_CONTEXT,
VERIFY_CLASSIFICATION, VERIFY_RUN_CACHE_STATE, VERIFY_STAMP_SUBJECT and any
timestamps/elapsed/exit strings) before writing the record—either by using jq
(pipe each value through jq -R . to produce a safe JSON string) when available,
or add a small shell helper (json_escape) that replaces backslash, double-quote
and newline with their JSON escape sequences and use that helper on each
variable used in the printf in append_timing_record to ensure well-formed JSON.
Comment on lines
780
to
790
| for i in "${!lane_names[@]}"; do | ||
| ( | ||
| set -euo pipefail | ||
| "${lane_funcs[$i]}" | ||
| set -uo pipefail | ||
| started_at="$(now_seconds)" | ||
| rc=0 | ||
| "${lane_funcs[$i]}" || rc=$? | ||
| finished_at="$(now_seconds)" | ||
| printf '%s\n' "$((finished_at - started_at))" >"${logdir}/${lane_names[$i]}.elapsed" | ||
| exit "$rc" | ||
| ) >"${logdir}/${lane_names[$i]}.log" 2>&1 & | ||
| lane_pids+=("$!") |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Subshell uses set -uo pipefail without -e—intentional for exit-code capture but worth a comment.
The missing -e flag is correct here since you're capturing rc explicitly via || rc=$?. A brief inline comment would clarify this isn't an oversight for future readers.
Add clarifying comment
(
- set -uo pipefail
+ # Deliberately omit -e: we capture the lane's exit code explicitly below.
+ set -uo pipefail
started_at="$(now_seconds)"
rc=0
"${lane_funcs[$i]}" || rc=$?📝 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
| for i in "${!lane_names[@]}"; do | |
| ( | |
| set -euo pipefail | |
| "${lane_funcs[$i]}" | |
| set -uo pipefail | |
| started_at="$(now_seconds)" | |
| rc=0 | |
| "${lane_funcs[$i]}" || rc=$? | |
| finished_at="$(now_seconds)" | |
| printf '%s\n' "$((finished_at - started_at))" >"${logdir}/${lane_names[$i]}.elapsed" | |
| exit "$rc" | |
| ) >"${logdir}/${lane_names[$i]}.log" 2>&1 & | |
| lane_pids+=("$!") | |
| for i in "${!lane_names[@]}"; do | |
| ( | |
| # Deliberately omit -e: we capture the lane's exit code explicitly below. | |
| set -uo pipefail | |
| started_at="$(now_seconds)" | |
| rc=0 | |
| "${lane_funcs[$i]}" || rc=$? | |
| finished_at="$(now_seconds)" | |
| printf '%s\n' "$((finished_at - started_at))" >"${logdir}/${lane_names[$i]}.elapsed" | |
| exit "$rc" | |
| ) >"${logdir}/${lane_names[$i]}.log" 2>&1 & | |
| lane_pids+=("$!") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/verify-local.sh` around lines 780 - 790, The subshell that runs each
lane (the block invoking "${lane_funcs[$i]}" and capturing its exit into rc)
intentionally uses "set -uo pipefail" without "-e" so the script can capture the
command exit code via "|| rc=$?"; add a brief inline comment directly above or
beside the "set -uo pipefail" line explaining that "-e is omitted on purpose to
allow explicit rc capture (used by the printf/write to
${logdir}/${lane_names[$i]}.elapsed and subsequent exit $rc)", so future readers
understand this is intentional when inspecting lane_funcs, lane_names, lane_pids
and the per-lane logs.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/warp-core/tests/invariant_property_tests.rs (1)
128-148: 🧹 Nitpick | 🔵 TrivialComment is slightly misleading but logic is correct.
The comment says "re-append at existing tick" but
dup_tick = num_ticks - 1is before the current tip, not at an existing tick. Both gap and dup cases fail withHistoryError::TickGapbecauseexpected_tick != provided_tick. The invariant test is valid — just note that "duplicate" isn't quite the right term here; it's "out-of-order append."Suggested comment clarification
- // Invariant: attempting to re-append at an existing tick must fail + // Invariant: attempting to append at an out-of-order tick must fail let dup_tick = num_ticks - 1;🤖 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 128 - 148, Update the test comment to accurately describe the failure scenario: replace "re-append at existing tick must fail" with wording that indicates an out-of-order append / tick gap (e.g., dup_tick is set to num_ticks - 1 which is before the current tip so append_local_commit returns HistoryError::TickGap). Reference the variables and functions in the test (dup_tick, num_ticks, ProvenanceEntry::local_commit, provenance.append_local_commit) so readers understand this is an out-of-order append check rather than a true duplicate-at-same-tick test.
♻️ Duplicate comments (1)
crates/warp-core/tests/slice_theorem_proof.rs (1)
585-590:⚠️ Potential issue | 🟠 MajorReplay-only root derivation still lacks an independent runtime oracle.
The test computes
state_rootfromapply_to_worldline_state(&mut replay_state)and later verifies that the cursor (which also usesapply_to_worldline_state) matches. A bug shared by both paths would pass self-consistently.Lines 598-607 compare
snapshot_commit_hashagainstsnapshot.hash, butsnapshot.state_root(the engine's actual committed root) is never compared againstreplay_state.state_root(). Add:assert_eq!( snapshot.state_root, state_root, "engine state_root must match replay state_root at tick {tick}" );This provides the independent execution-side check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/tests/slice_theorem_proof.rs` around lines 585 - 590, The test currently derives state_root by applying wl_patch to replay_state (via wl_patch.apply_to_worldline_state(&mut replay_state)) and validates snapshot_commit_hash but never compares the engine's committed root to the replayed root; add an assertion comparing snapshot.state_root to the computed state_root (from replay_state.state_root()) with a descriptive message referencing tick to ensure the engine's actual committed state root matches the replay-derived root at each tick.
🤖 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/tests/playback_cursor_tests.rs`:
- Around line 366-373: The vec! entry unnecessarily clones
checkpoint_output_bytes before calling append_fixture_entry; remove the .clone()
and pass checkpoint_output_bytes directly (in the call to append_fixture_entry
with the tuple (checkpoint_output_channel, checkpoint_output_bytes)), since
checkpoint_output_bytes is not used afterwards and the clone is redundant.
---
Outside diff comments:
In `@crates/warp-core/tests/invariant_property_tests.rs`:
- Around line 128-148: Update the test comment to accurately describe the
failure scenario: replace "re-append at existing tick must fail" with wording
that indicates an out-of-order append / tick gap (e.g., dup_tick is set to
num_ticks - 1 which is before the current tip so append_local_commit returns
HistoryError::TickGap). Reference the variables and functions in the test
(dup_tick, num_ticks, ProvenanceEntry::local_commit,
provenance.append_local_commit) so readers understand this is an out-of-order
append check rather than a true duplicate-at-same-tick test.
---
Duplicate comments:
In `@crates/warp-core/tests/slice_theorem_proof.rs`:
- Around line 585-590: The test currently derives state_root by applying
wl_patch to replay_state (via wl_patch.apply_to_worldline_state(&mut
replay_state)) and validates snapshot_commit_hash but never compares the
engine's committed root to the replayed root; add an assertion comparing
snapshot.state_root to the computed state_root (from replay_state.state_root())
with a descriptive message referencing tick to ensure the engine's actual
committed state root matches the replay-derived root at each tick.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d92cf6f9-c55a-46c0-974e-13d736599094
📒 Files selected for processing (3)
crates/warp-core/tests/invariant_property_tests.rscrates/warp-core/tests/playback_cursor_tests.rscrates/warp-core/tests/slice_theorem_proof.rs
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/warp-core/tests/inbox.rs (1)
296-311: 🧹 Nitpick | 🔵 TrivialStop rebuilding the committed patch by hand.
This test already has
patch, but the block below re-encodes the fullWorldlineTickPatchV1schema. That duplicates ABI shape in the test and guarantees churn on every header rename likeglobal_tick→commit_global_tick. Compare againstpatchdirectly and keep this test focused on provenance mirroring.♻️ Proposed simplification
- let expected_patch = WorldlineTickPatchV1 { - header: warp_core::WorldlineTickHeaderV1 { - commit_global_tick: runtime.global_tick(), - policy_id: patch.policy_id(), - rule_pack_id: patch.rule_pack_id(), - plan_digest: snapshot.plan_digest, - decision_digest: snapshot.decision_digest, - rewrites_digest: snapshot.rewrites_digest, - }, - warp_id: snapshot.root.warp_id, - ops: patch.ops().to_vec(), - in_slots: patch.in_slots().to_vec(), - out_slots: patch.out_slots().to_vec(), - patch_digest: patch.digest(), - }; - assert_eq!(entry.patch, Some(expected_patch)); + assert_eq!(entry.patch, Some(patch));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/tests/inbox.rs` around lines 296 - 311, The test rebuilds a WorldlineTickPatchV1 by hand which duplicates the ABI and is brittle; instead remove the manual construction of expected_patch and assert that entry.patch equals the existing patch value (e.g., replace the block building WorldlineTickPatchV1 and the assert with assert_eq!(entry.patch, Some(patch.clone())) or an equivalent direct comparison), referencing WorldlineTickPatchV1, entry.patch and patch to locate the change.docs/plans/adr-0008-and-0009.md (1)
563-607:⚠️ Potential issue | 🟡 MinorPhase 5 still documents the v1 observation hash domain.
This section was updated for observation contract v2, but the same Phase 5 block still says
echo:observation-artifact:v1\0. That gives readers the wrong hash-domain/version for the current artifact identity.As per coding guidelines, documentation accuracy matters — especially anything touching determinism guarantees, hash stability, or canonical ordering.
Also applies to: 656-670
🤖 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 563 - 607, The Phase 5 documentation still references the old observation artifact hash domain "echo:observation-artifact:v1\0" which is inconsistent with the new observation contract types (ObservationCoordinate, ObservationAt, ObservationFrame, ObservationProjection, ObservationRequest, ResolvedObservationCoordinate); update the Phase 5 block to use the v2 hash-domain/identifier for observation artifacts and correct any other occurrences (notably the section around the later block noted by the reviewer) so the documented artifact identity/version matches the v2 contract.
♻️ Duplicate comments (14)
tests/hooks/test_pr_status.sh (1)
26-27:⚠️ Potential issue | 🟡 MinorGuarantee temp-directory cleanup on early failure.
If
./scripts/pr-status.shexits non-zero in thesuccessfixture,set -eaborts before Line 77 and leavestmpbehind. Make cleanup trap-backed instead of relying on the finalrm -rf.Suggested fix
repo_root="$(pwd)" tmp="$(mktemp -d)" + trap "rm -rf -- $(printf '%q' "$tmp")" RETURN EXIT mkdir -p "$tmp/bin" @@ *) echo "unknown fixture: $fixture" >&2 - rm -rf "$tmp" exit 1 ;; esac @@ ( cd "$repo_root" PATH="$tmp/bin:$PATH" ./scripts/pr-status.sh 302 2>&1 ) - - rm -rf "$tmp" }Also applies to: 72-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/test_pr_status.sh` around lines 26 - 27, The test creates a temporary dir in the variable tmp with mktemp and later removes it unconditionally, but with set -e an early non-zero exit can leave tmp behind; add a trap to ensure cleanup on EXIT/ERR by registering a cleanup function that rm -rfs "$tmp" and set trap 'cleanup' EXIT (or trap 'rm -rf "$tmp"' EXIT) immediately after tmp="$(mktemp -d)" (and similarly for the second tmp usage around the block that currently does rm -rf in the 72-77 region) so the temp directory is always removed even on early failure.crates/warp-core/src/head.rs (2)
49-52: 🛠️ Refactor suggestion | 🟠 MajorDocument
HeadId::from_bytes()as deserialization-only.This public constructor bypasses
make_head_id()’s domain-separated creation path, but the rustdoc only says “from bytes.” Add the intended round-trip/deserialization use and point callers tomake_head_id()for fresh IDs.Suggested fix
- /// Constructs a head id from its canonical 32-byte hash representation. + /// Reconstructs a head id from its canonical 32-byte hash representation. + /// + /// Intended for round-trip deserialization and persistence. Use + /// [`make_head_id`] when creating a fresh identifier from a label. #[must_use] pub fn from_bytes(bytes: Hash) -> Self { Self(bytes) }Based on learnings, Applies to crates/**/*.rs : Every public API across crates (warp-core, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage.
🤖 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 49 - 52, Update the rustdoc for HeadId::from_bytes to state it is intended only for deserialization/round-trip from the canonical 32-byte hash and that it bypasses the domain-separated construction path; instruct callers to use make_head_id() when creating fresh/derived HeadId values and mention any invariants preserved by from_bytes (e.g., canonical hash form). Reference HeadId::from_bytes() and make_head_id() in the doc comment so callers know which to use.
346-350:⚠️ Potential issue | 🟠 MajorAdd a runnable-set regression for
HeadEligibility.
RunnableWriterSet::rebuild()now gates onhead.is_admitted()as well as pause state, but this file still only tests pause-based exclusion. Add oneDormant -> Admittedroundtrip so scheduler admission cannot regress independently ofPlaybackMode.Suggested test
#[test] fn paused_heads_excluded_from_runnable() { let mut reg = PlaybackHeadRegistry::new(); @@ assert_eq!(runnable.len(), 1); assert_eq!(*runnable.iter().next().unwrap(), k1); } + + #[test] + fn dormant_heads_excluded_until_readmitted() { + let mut reg = PlaybackHeadRegistry::new(); + let key = WriterHeadKey { + worldline_id: wl(1), + head_id: hd("dormant"), + }; + + reg.insert(make_head(key, PlaybackMode::Play)); + + let mut runnable = RunnableWriterSet::new(); + runnable.rebuild(®); + assert_eq!(runnable.iter().copied().collect::<Vec<_>>(), vec![key]); + + reg.get_mut(&key) + .unwrap() + .set_eligibility(HeadEligibility::Dormant); + runnable.rebuild(®); + assert!(runnable.is_empty()); + + reg.get_mut(&key) + .unwrap() + .set_eligibility(HeadEligibility::Admitted); + runnable.rebuild(®); + assert_eq!(runnable.iter().copied().collect::<Vec<_>>(), vec![key]); + }🤖 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 346 - 350, Add a test that verifies RunnableWriterSet::rebuild respects HeadEligibility admission in addition to pause state by exercising a Dormant -> Admitted transition: create a PlaybackHeadRegistry entry initially Dormant (or not admitted) and not paused, confirm it is excluded after rebuild, then change its eligibility to Admitted (e.g., call the method that flips admission or set the head to admitted) and run RunnableWriterSet::rebuild again and assert the key is now included. Reference RunnableWriterSet::rebuild, PlaybackHeadRegistry, and the head methods is_admitted()/is_paused() when locating where to add the test and when toggling the head state.crates/warp-core/tests/slice_theorem_proof.rs (1)
585-590:⚠️ Potential issue | 🟠 MajorThese proofs still use replay to validate replay.
Both blocks derive
state_rootfromapply_to_worldline_state(...)and then compare the cursor against that same substrate. A bug shared by patch application andseek_to()will pass self-consistently. Keep the runtime commit as an independent oracle here with an explicitassert_eq!(state_root, snapshot.state_root)after each apply.Also applies to: 812-819
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/tests/slice_theorem_proof.rs` around lines 585 - 590, The test currently derives state_root from wl_patch.apply_to_worldline_state(...) and then compares the cursor against that same derived substrate, so add an independent oracle check by asserting the runtime commit snapshot's root equals the computed root after each apply: after calling wl_patch.apply_to_worldline_state(&mut replay_state) and computing let state_root = replay_state.state_root(), immediately assert_eq!(state_root, snapshot.state_root) (reference symbols: wl_patch.apply_to_worldline_state, replay_state.state_root, snapshot.state_root); do this in both places mentioned (around the existing block and the similar block at lines roughly 812-819) so replay is validated against the runtime snapshot rather than only self-consistency.crates/warp-core/tests/playback_cursor_tests.rs (2)
428-434:⚠️ Potential issue | 🟠 MajorThis still does not prove the checkpoint state was restored.
Asserting
checkpoint_state_before:3only proves the lookup happened. A regression that consults the checkpoint and then replays ticks0and1from U0 still satisfies every assertion below. Also assert that the prefix entry is not fetched/reapplied, e.g. noentry:0event.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/tests/playback_cursor_tests.rs` around lines 428 - 434, The test currently only checks that a checkpoint lookup occurred via provenance.events() containing "checkpoint_state_before:3" but doesn't verify the checkpoint's state was used; update the test to also assert that the prefix entry 0 was not re-fetched/reapplied by ensuring provenance.events() does NOT contain "entry:0" (i.e., add a negative assertion alongside the existing any(...) check on events to confirm "entry:0" is absent).
366-372:⚠️ Potential issue | 🔴 CriticalRemove the redundant clone; CI is already red on it.
checkpoint_output_bytesis not used after this append, so.clone()here tripsclippy::redundant_clone.Patch
append_fixture_entry( &mut provenance, worldline_id, patch, triplet, - vec![(checkpoint_output_channel, checkpoint_output_bytes.clone())], + vec![(checkpoint_output_channel, checkpoint_output_bytes)], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/tests/playback_cursor_tests.rs` around lines 366 - 372, The call to append_fixture_entry is passing checkpoint_output_bytes.clone() redundantly; remove the unnecessary .clone() since checkpoint_output_bytes is not used afterward and clippy flags clippy::redundant_clone. Update the invocation of append_fixture_entry (the vector element containing (checkpoint_output_channel, checkpoint_output_bytes.clone())) to pass checkpoint_output_bytes by value without cloning, ensuring the tuple becomes (checkpoint_output_channel, checkpoint_output_bytes); verify types still match the function signature for append_fixture_entry and adjust ownership if needed.docs/plans/adr-0008-and-0009.md (1)
755-757:⚠️ Potential issue | 🟡 MinorDo not reintroduce
snapshot_at(...)as a Phase 7 migration target.This document already says the WASM adapter was deleted in Phase 6. Listing
snapshot_at(...)here again makes the migration story internally inconsistent; reference the observation-backed snapshot path instead.As per coding guidelines, documentation accuracy matters — especially anything touching determinism guarantees, hash stability, or canonical ordering.
🤖 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 755 - 757, Update the bullet that lists migration targets to remove the reintroduction of snapshot_at(...); instead reference the observation-backed snapshot path and clarify that the WASM adapter was deleted in Phase 6 so snapshot_at(...) is not a Phase 7 target, and remove any implication that reader-side GraphStore replay fallbacks or snapshot_at(...) are being reintroduced; update mentions of fork(...), playback seek/step, and "full-state replay" to explicitly point to the observation-backed snapshot flow and Phase 6 removal to keep the migration story consistent.crates/warp-core/src/provenance_store.rs (2)
722-723:⚠️ Potential issue | 🔴 CriticalRemove the remaining
u64 -> usizetruncating casts.
as usizehere still aliases highWorldlineTicks on wasm32/other 32-bit targets, so replay lookup, parent validation, forking, and direct entry access diverge by architecture. Switch these sites tousize::try_from(...)and map overflow into the existing deterministic error at each call site instead of indexing the wrong slot.#!/bin/bash rg -n 'as_u64\(\) as usize' crates/warp-core/src/provenance_store.rsAs per coding guidelines, "warp-core is the deterministic kernel. Every code path must be deterministic."
Also applies to: 1062-1065, 1205-1208, 1308-1313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/provenance_store.rs` around lines 722 - 723, The code casts WorldlineTick values with "as_u64() as usize" which truncates on 32-bit targets; replace each such cast (e.g., the target_len computation using target_tick.as_u64() as usize and similar sites around replay lookup/parent validation/forking/direct entry access that reference replayed.tick_history) with a fallible conversion using usize::try_from(target_tick.as_u64()) and map the Err branch into the existing deterministic error path already used at those call sites (return or propagate the same deterministic overflow/invalid-tick error), ensuring you update all occurrences (including the other reported blocks at roughly 1062–1065, 1205–1208, 1308–1313) so indexing into replayed.tick_history uses a validated usize.
814-819:⚠️ Potential issue | 🔴 CriticalClear checkpoint-sourced replay metadata before hydrating.
hydrate_replay_metadata()only truncates/fillstick_history. If the stored checkpoint already carriescheckpoint_tickentries, staletick_historyandlast_snapshotsurvive and historical snapshot APIs can read checkpoint-sourced metadata that never came from provenance.Based on learnings, replay metadata is rebuilt exclusively from stored provenance via `hydrate_replay_metadata(...)` in the checkpoint restore path.Suggested fix
let checkpoint_tick = checkpoint.checkpoint.worldline_tick; let mut replayed = checkpoint.state; + replayed.tick_history.clear(); + replayed.last_snapshot = None; replayed.last_materialization_errors.clear(); replayed.committed_ingress.clear(); hydrate_replay_metadata(provenance, worldline_id, &mut replayed, checkpoint_tick)?; return Ok((replayed, checkpoint_tick));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/provenance_store.rs` around lines 814 - 819, The checkpoint restore must clear any checkpoint-sourced replay metadata before calling hydrate_replay_metadata; in the block that currently sets replayed = checkpoint.state and clears last_materialization_errors and committed_ingress, also clear replayed.tick_history and set replayed.last_snapshot = None (or its zero/empty value) so hydrate_replay_metadata(provenance, worldline_id, &mut replayed, checkpoint_tick)? rebuilds metadata exclusively from provenance; keep the existing clears of last_materialization_errors and committed_ingress and then call hydrate_replay_metadata as before.scripts/verify-local.sh (1)
1440-1443:⚠️ Potential issue | 🟠 Major
fullmode is still cached by the index, not the worktree.
git write-treehashes the current index only. An unstaged edit or new untracked file leavesVERIFY_STAMP_SUBJECTunchanged, so the cachedfullpath can still skip verification for stale bytes. Keepgit write-treeforpre-commit, but derivefull/ working-tree subjects from actual worktree contents.Does `git write-tree` include unstaged working tree changes, or only the current index?One way to key `full` runs to the real worktree
-elif [[ "$VERIFY_MODE_CONTEXT" == "pre-commit" || "$VERIFY_MODE_CONTEXT" == "working-tree" || "$MODE" == "full" ]]; then - VERIFY_STAMP_SUBJECT="$(git write-tree)" +elif [[ "$VERIFY_MODE_CONTEXT" == "pre-commit" ]]; then + VERIFY_STAMP_SUBJECT="$(git write-tree)" +elif [[ "$VERIFY_MODE_CONTEXT" == "working-tree" || "$MODE" == "full" ]]; then + VERIFY_STAMP_SUBJECT="$(worktree_tree)"worktree_tree() { local tmp_index tmp_index="$(mktemp "${TMPDIR:-/tmp}/verify-local-index.XXXXXX")" cp .git/index "$tmp_index" GIT_INDEX_FILE="$tmp_index" git add -A -- . >/dev/null GIT_INDEX_FILE="$tmp_index" git write-tree rm -f "$tmp_index" }Also applies to: 1480-1483
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-local.sh` around lines 1440 - 1443, The current logic uses git write-tree for "full" which only hashes the index and misses unstaged/untracked changes; keep git write-tree for pre-commit but for "full" and "working-tree" derive VERIFY_STAMP_SUBJECT from the actual worktree by creating a temporary index, running GIT_INDEX_FILE="$tmp" git add -A -- . and then GIT_INDEX_FILE="$tmp" git write-tree (see a helper like worktree_tree()), then remove the temp index; replace the branches that set VERIFY_STAMP_SUBJECT (the checks against VERIFY_MODE_CONTEXT and MODE and the identical block at the later site) to call that worktree-based helper for full/working-tree and retain git write-tree only for pre-commit.crates/warp-wasm/src/warp_kernel.rs (2)
348-425:⚠️ Potential issue | 🟠 Major
Stopcannot stopStart::UntilIdlein this run model.
dispatch_intent(Start)runs the scheduler loop to completion before returning and then clears active state. In single-threaded WASM, a laterStopcall can only reportRunCompletion::Stoppedfor a run that already finished. RejectStopwhile inactive, or make runs resumable across calls.Also applies to: 426-429
🤖 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 348 - 425, The Start::UntilIdle path runs to completion synchronously and clears active state, so a later ControlIntentV1::Stop cannot meaningfully stop that run; update the Stop handling in dispatch_intent to reject Stop when there is no active run (i.e., when self.scheduler_status.state is not SchedulerState::Running or SchedulerState::Stopping) by returning an AbiError with code error_codes::INVALID_CONTROL and an appropriate message, rather than accepting Stop for an already-inactive run—adjust the Stop branch in the same module (referencing dispatch_intent, ControlIntentV1::Stop, and self.scheduler_status.state) and mirror this check where similar Stop handling appears (the duplicate block mentioned).
436-441:⚠️ Potential issue | 🟡 MinorMap unknown heads to
INVALID_CONTROL.
RuntimeError::UnknownHead(_)is bad caller input, not an engine fault. ReturningENGINE_ERRORforces clients to key off the message text instead of the ABI error code.Suggested fix
self.runtime .set_head_eligibility(key, eligibility) .map_err(|e| AbiError { - code: error_codes::ENGINE_ERROR, + code: match e { + RuntimeError::UnknownHead(_) => error_codes::INVALID_CONTROL, + _ => error_codes::ENGINE_ERROR, + }, message: e.to_string(), })?;🤖 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 436 - 441, The mapping of RuntimeError::UnknownHead in the set_head_eligibility call is currently converted into an AbiError with code error_codes::ENGINE_ERROR; change the mapping so that when self.runtime.set_head_eligibility(...) returns an error matching RuntimeError::UnknownHead(_) you map it to an AbiError with code error_codes::INVALID_CONTROL (preserving the message), and only use ENGINE_ERROR for true engine faults—update the closure in the map_err on set_head_eligibility to match on the error variant (RuntimeError::UnknownHead => INVALID_CONTROL, _ => ENGINE_ERROR) while keeping the existing message formatting.crates/warp-core/tests/common/mod.rs (1)
760-763:⚠️ Potential issue | 🟡 MinorDon't bake
commit_global_tick = worldline_tick + 1into shared fixtures.This helper still manufactures impossible timelines once callers build fixtures after existing history or on forks. Shared test utilities should take the commit-global tick explicitly, or derive it from the caller's setup, so metadata tests match histories the runtime can actually emit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/tests/common/mod.rs` around lines 760 - 763, The helper fixture_commit_global_tick currently hardcodes commit_global_tick = worldline_tick + 1 which produces impossible timelines; change the API so the commit global tick is provided by the caller (e.g., replace fn fixture_commit_global_tick(worldline_tick: u64) -> GlobalTick with a function that accepts the explicit commit_global_tick or return GlobalTick::from_raw(worldline_tick) only if the caller promises that mapping), update all callers to pass the correct commit_global_tick derived from their test setup (or compute it from the caller's base history) and remove the baked +1 arithmetic inside fixture_commit_global_tick so shared fixtures no longer manufacture impossible timelines.crates/ttd-browser/src/lib.rs (1)
742-754: 🧹 Nitpick | 🔵 TrivialRedundant tick-0 check in
commit_inner.Line 742 checks
cursor.tick == WorldlineTick::ZEROand returns early. Then line 750-753 doeschecked_sub(1)which would only fail at tick 0 - but we already exited. The second check is dead code.Not a bug, but the duplication is noise. Consider removing either the early guard or the
ok_or_elsewrapper.♻️ Remove redundant check
fn commit_inner(&mut self, tx_id: u64) -> Result<Vec<u8>, String> { ... - if cursor.tick == WorldlineTick::ZERO { - return Err("cannot commit at tick 0".to_string()); - } let expected = self .provenance .entry( cursor.worldline_id, cursor .tick .checked_sub(1) .ok_or_else(|| "cannot commit at tick 0".to_string())?, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ttd-browser/src/lib.rs` around lines 742 - 754, Redundant tick-0 check: remove the early guard "if cursor.tick == WorldlineTick::ZERO { return Err(...); }" in commit_inner and keep the existing checked_sub(1).ok_or_else(...) on cursor.tick so the subtraction check remains the single source of truth for the tick-0 error (look for identifiers commit_inner, cursor.tick, WorldlineTick::ZERO, provenance.entry, checked_sub(1), ok_or_else).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.githooks/pre-push-parallel:
- Around line 9-10: The timing trap is installed too late: call the trap
installation routine immediately after the signal handler definitions and before
calling hook_timing_prepare or any early-exit checks so timing is always
recorded; specifically, move the trap setup currently done around the trap lines
(the trap installation block referenced in lines 69–71) to directly follow the
signal handler definitions (the handler function/definitions used by
hook_timing_prepare), and ensure this runs before the missing-dependencies
check, toolchain check, and mktemp usage so no early exit can bypass the timing
handler.
- Around line 54-71: The on_signal handler calls cleanup() and then exits, which
re-triggers the on_exit EXIT trap and causes cleanup to run twice; in the
on_signal function (referencing on_signal, on_exit, and cleanup) disarm the EXIT
trap before invoking cleanup (e.g., call trap - EXIT) so cleanup is only
executed once, then proceed to call cleanup "$exit_code" and exit "$exit_code".
In @.gitignore:
- Around line 14-15: The .gitignore entries `.dx-debug/*` and `blog/*` only
ignore top-level files; change them to recursive directory patterns to cover
nested files (e.g., replace `.dx-debug/*` with `.dx-debug/` or `.dx-debug/**`
and `blog/*` with `blog/` or `blog/**`) so paths like
`.dx-debug/2026/timing.csv` and nested files under `blog/` are ignored.
In `@crates/echo-wasm-abi/src/lib.rs`:
- Around line 130-146: Add ABI-level unit tests covering pack_control_intent_v1
and unpack_control_intent_v1: (1) a round-trip test that encodes a
kernel_port::ControlIntentV1 with pack_control_intent_v1 then decodes with
unpack_control_intent_v1 and asserts equality; (2) a wrong-op-id test that calls
unpack_control_intent_v1 on an envelope whose op_id has been mutated (use
pack_intent_v1/CUSTOM op id or mutate the packed bytes) and assert it returns
EnvelopeError::Malformed; and (3) a malformed-CBOR test that passes an envelope
with CONTROL_INTENT_V1_OP_ID but invalid CBOR payload and assert decode returns
EnvelopeError::Malformed. Use the existing helpers encode_cbor/decode_cbor,
CONTROL_INTENT_V1_OP_ID, pack_intent_v1, and unpack_intent_v1 to construct the
inputs and error assertions.
- Around line 47-48: pack_intent_v1 currently allows u32::MAX
(CONTROL_INTENT_V1_OP_ID) and thus lets callers create reserved control
envelopes; fix by rejecting that reserved id in pack_intent_v1: add a guard that
if op_id == CONTROL_INTENT_V1_OP_ID the function returns an error (or
Result::Err) rather than packing, and update callers accordingly, or
alternatively make pack_intent_v1 non-public (crate/private) and expose a typed
public helper (e.g., pack_control_intent_v1 / pack_typed_intent) that uses
CONTROL_INTENT_V1_OP_ID internally—refer to the symbols CONTROL_INTENT_V1_OP_ID
and pack_intent_v1 to locate the change and ensure the public API cannot accept
u32::MAX.
In `@crates/warp-core/src/worldline_state.rs`:
- Around line 300-305: Remove the unnecessary clippy allow in current_tick:
delete the #[allow(clippy::cast_possible_truncation)] block and directly return
WorldlineTick::from_raw(self.tick_history.len() as u64); this cleans up noise
around the cast of tick_history.len() to u64 while keeping the same behavior.
In `@crates/warp-core/tests/checkpoint_fork_tests.rs`:
- Around line 190-201: The test currently only inspects the checkpoint table
after checkpoint_cursor.seek_to(...) which doesn't prove PlaybackCursor::seek_to
used the checkpoint; assert a observable effect that can only happen when the
checkpoint path is used—for example, after checkpoint_cursor.seek_to(wt(23),
...) verify that the cursor reports it restored from checkpoint_20 (e.g. compare
checkpoint_cursor.selected_checkpoint_tick or
checkpoint_restore_state.checkpoint.worldline_tick to
checkpoint_20.worldline_tick) and that the replayed suffix length equals wt(24)
- checkpoint_20.worldline_tick (or an equivalent cursor API like
checkpoint_cursor.replayed_suffix_len()/last_replay_start_tick()) so the test
fails if seek_to had replayed from tick 0 instead of using the checkpoint.
In `@crates/warp-core/tests/invariant_property_tests.rs`:
- Around line 114-125: The test constructs a malformed commit (changing
worldline_tick, global_tick, and hash triplet together) so append_local_commit
may fail before hitting the worldline-tick invariant; fix by creating a normal
"next commit" via ProvenanceEntry::local_commit (use the same valid global tick,
root/commit hashes, and patch headers as a genuine successor) and then only
override the worldline_tick value for gap_entry before calling
provenance.append_local_commit(gap_entry); do the same adjustment for the other
similar case that varies the three fields together so the append is rejected
solely by the worldline-tick monotonicity check.
In `@crates/warp-wasm/README.md`:
- Around line 15-18: The sentence claims `observe(...)` is the "only public read
export" while `scheduler_status()` is also a public read-only export; update the
README sentence to remove the contradiction by either dropping "only" or
narrowing the scope to state that `observe(...)` is the "only public world-state
read export" and keep `scheduler_status()` described as a separate read-only
scheduler metadata export; edit the line referencing `observe(...)` and
`scheduler_status()` so it reads e.g. "`observe(...)` is the only public
world-state read export, `scheduler_status()` is the read-only scheduler
metadata export, and all external writes or scheduler control requests flow
through `dispatch_intent(...)`" to ensure both symbols are accurately described.
In `@docs/ROADMAP/first-light/browser-visualization.md`:
- Around line 8-15: Add a clear one-sentence notice near the top of this roadmap
page that every concrete browser/WASM symbol mentioned here is historical and
not part of ABI v3; explicitly mark legacy ABI names such as step(...),
drain_view_ops, bridge.step(1), and bridge.getHead().tick as obsolete/removed to
prevent readers from treating them as current API, and ensure the banner or
first paragraph states that these symbols are retained for planning archaeology
only and do not reflect the live WASM API contract.
In `@scripts/plot-prepush-timing.mjs`:
- Around line 58-68: The current code builds modeNames by filtering
preferredModes which drops any new modes in runRecords; change this so modeNames
is derived from the observed modes in runRecords (e.g., collect unique d.mode
from runRecords) and then sort that list using preferredModes as an ordering
preference (put known preferredModes first, then any unknown modes
alphabetically) before iterating; update the block that references
preferredModes, modeNames, runRecords, series, and legends so all valid modes
are processed while preserving the intended display order.
In `@scripts/pr-status.sh`:
- Around line 78-89: The GraphQL query truncates reviewThreads at first:100 and
misses hasNextPage, so update the logic around THREADS_JSON/UNRESOLVED_THREADS
to paginate the reviewThreads connection (or at minimum detect truncation):
modify the gh api graphql call used to fetch review threads to request pageInfo
{ hasNextPage endCursor } and loop calling the query with after:$cursor until
hasNextPage is false, accumulating nodes before computing UNRESOLVED_THREADS;
alternatively, if you choose not to implement full pagination, check hasNextPage
and surface a warning or non-zero exit when true so callers know counts are
incomplete — update the script variables (THREADS_JSON, PR_NUMBER,
UNRESOLVED_THREADS) and the Python step to operate on the accumulated result set
or to handle the hasNextPage flag.
In `@tests/hooks/test_hook_timing.sh`:
- Around line 58-70: The test loop verifies hook_timing_prepare is present in
all hooks but only a subset get runtime execution; add runtime fixture tests for
the two un-executed hooks (pre-push-parallel and pre-push-sequential) mirroring
the existing runtime tests for commit-msg, pre-commit, pre-push and pre-rebase
so their behavior is validated at runtime; update the test suite to invoke those
hooks in isolation (using the same fixture patterns and assertions used by the
existing runtime tests) and ensure any unique parallel/sequential behavior is
asserted specifically for pre-push-parallel and pre-push-sequential.
In `@tests/hooks/test_pr_status.sh`:
- Around line 114-121: The current check only verifies the presence of bucket
names in $status_output but not that checks are grouped under the correct bucket
headings; update the test in tests/hooks/test_pr_status.sh (the conditional that
inspects $status_output) to assert heading→item structure by matching each
bucket header followed by at least one indented item line (for example, use
multi-line grep or awk/grep -P with a pattern like '^-
Tests$([\r\n]+[[:space:]]+- .+)' to ensure "Tests" header is immediately
followed by check entries), and do the same for "Clippy" and "Determinism
Guards" so the test fails if scripts/pr-status.sh prints headers but does not
group items under them.
---
Outside diff comments:
In `@crates/warp-core/tests/inbox.rs`:
- Around line 296-311: The test rebuilds a WorldlineTickPatchV1 by hand which
duplicates the ABI and is brittle; instead remove the manual construction of
expected_patch and assert that entry.patch equals the existing patch value
(e.g., replace the block building WorldlineTickPatchV1 and the assert with
assert_eq!(entry.patch, Some(patch.clone())) or an equivalent direct
comparison), referencing WorldlineTickPatchV1, entry.patch and patch to locate
the change.
In `@docs/plans/adr-0008-and-0009.md`:
- Around line 563-607: The Phase 5 documentation still references the old
observation artifact hash domain "echo:observation-artifact:v1\0" which is
inconsistent with the new observation contract types (ObservationCoordinate,
ObservationAt, ObservationFrame, ObservationProjection, ObservationRequest,
ResolvedObservationCoordinate); update the Phase 5 block to use the v2
hash-domain/identifier for observation artifacts and correct any other
occurrences (notably the section around the later block noted by the reviewer)
so the documented artifact identity/version matches the v2 contract.
---
Duplicate comments:
In `@crates/ttd-browser/src/lib.rs`:
- Around line 742-754: Redundant tick-0 check: remove the early guard "if
cursor.tick == WorldlineTick::ZERO { return Err(...); }" in commit_inner and
keep the existing checked_sub(1).ok_or_else(...) on cursor.tick so the
subtraction check remains the single source of truth for the tick-0 error (look
for identifiers commit_inner, cursor.tick, WorldlineTick::ZERO,
provenance.entry, checked_sub(1), ok_or_else).
In `@crates/warp-core/src/head.rs`:
- Around line 49-52: Update the rustdoc for HeadId::from_bytes to state it is
intended only for deserialization/round-trip from the canonical 32-byte hash and
that it bypasses the domain-separated construction path; instruct callers to use
make_head_id() when creating fresh/derived HeadId values and mention any
invariants preserved by from_bytes (e.g., canonical hash form). Reference
HeadId::from_bytes() and make_head_id() in the doc comment so callers know which
to use.
- Around line 346-350: Add a test that verifies RunnableWriterSet::rebuild
respects HeadEligibility admission in addition to pause state by exercising a
Dormant -> Admitted transition: create a PlaybackHeadRegistry entry initially
Dormant (or not admitted) and not paused, confirm it is excluded after rebuild,
then change its eligibility to Admitted (e.g., call the method that flips
admission or set the head to admitted) and run RunnableWriterSet::rebuild again
and assert the key is now included. Reference RunnableWriterSet::rebuild,
PlaybackHeadRegistry, and the head methods is_admitted()/is_paused() when
locating where to add the test and when toggling the head state.
In `@crates/warp-core/src/provenance_store.rs`:
- Around line 722-723: The code casts WorldlineTick values with "as_u64() as
usize" which truncates on 32-bit targets; replace each such cast (e.g., the
target_len computation using target_tick.as_u64() as usize and similar sites
around replay lookup/parent validation/forking/direct entry access that
reference replayed.tick_history) with a fallible conversion using
usize::try_from(target_tick.as_u64()) and map the Err branch into the existing
deterministic error path already used at those call sites (return or propagate
the same deterministic overflow/invalid-tick error), ensuring you update all
occurrences (including the other reported blocks at roughly 1062–1065,
1205–1208, 1308–1313) so indexing into replayed.tick_history uses a validated
usize.
- Around line 814-819: The checkpoint restore must clear any checkpoint-sourced
replay metadata before calling hydrate_replay_metadata; in the block that
currently sets replayed = checkpoint.state and clears
last_materialization_errors and committed_ingress, also clear
replayed.tick_history and set replayed.last_snapshot = None (or its zero/empty
value) so hydrate_replay_metadata(provenance, worldline_id, &mut replayed,
checkpoint_tick)? rebuilds metadata exclusively from provenance; keep the
existing clears of last_materialization_errors and committed_ingress and then
call hydrate_replay_metadata as before.
In `@crates/warp-core/tests/common/mod.rs`:
- Around line 760-763: The helper fixture_commit_global_tick currently hardcodes
commit_global_tick = worldline_tick + 1 which produces impossible timelines;
change the API so the commit global tick is provided by the caller (e.g.,
replace fn fixture_commit_global_tick(worldline_tick: u64) -> GlobalTick with a
function that accepts the explicit commit_global_tick or return
GlobalTick::from_raw(worldline_tick) only if the caller promises that mapping),
update all callers to pass the correct commit_global_tick derived from their
test setup (or compute it from the caller's base history) and remove the baked
+1 arithmetic inside fixture_commit_global_tick so shared fixtures no longer
manufacture impossible timelines.
In `@crates/warp-core/tests/playback_cursor_tests.rs`:
- Around line 428-434: The test currently only checks that a checkpoint lookup
occurred via provenance.events() containing "checkpoint_state_before:3" but
doesn't verify the checkpoint's state was used; update the test to also assert
that the prefix entry 0 was not re-fetched/reapplied by ensuring
provenance.events() does NOT contain "entry:0" (i.e., add a negative assertion
alongside the existing any(...) check on events to confirm "entry:0" is absent).
- Around line 366-372: The call to append_fixture_entry is passing
checkpoint_output_bytes.clone() redundantly; remove the unnecessary .clone()
since checkpoint_output_bytes is not used afterward and clippy flags
clippy::redundant_clone. Update the invocation of append_fixture_entry (the
vector element containing (checkpoint_output_channel,
checkpoint_output_bytes.clone())) to pass checkpoint_output_bytes by value
without cloning, ensuring the tuple becomes (checkpoint_output_channel,
checkpoint_output_bytes); verify types still match the function signature for
append_fixture_entry and adjust ownership if needed.
In `@crates/warp-core/tests/slice_theorem_proof.rs`:
- Around line 585-590: The test currently derives state_root from
wl_patch.apply_to_worldline_state(...) and then compares the cursor against that
same derived substrate, so add an independent oracle check by asserting the
runtime commit snapshot's root equals the computed root after each apply: after
calling wl_patch.apply_to_worldline_state(&mut replay_state) and computing let
state_root = replay_state.state_root(), immediately assert_eq!(state_root,
snapshot.state_root) (reference symbols: wl_patch.apply_to_worldline_state,
replay_state.state_root, snapshot.state_root); do this in both places mentioned
(around the existing block and the similar block at lines roughly 812-819) so
replay is validated against the runtime snapshot rather than only
self-consistency.
In `@crates/warp-wasm/src/warp_kernel.rs`:
- Around line 348-425: The Start::UntilIdle path runs to completion
synchronously and clears active state, so a later ControlIntentV1::Stop cannot
meaningfully stop that run; update the Stop handling in dispatch_intent to
reject Stop when there is no active run (i.e., when self.scheduler_status.state
is not SchedulerState::Running or SchedulerState::Stopping) by returning an
AbiError with code error_codes::INVALID_CONTROL and an appropriate message,
rather than accepting Stop for an already-inactive run—adjust the Stop branch in
the same module (referencing dispatch_intent, ControlIntentV1::Stop, and
self.scheduler_status.state) and mirror this check where similar Stop handling
appears (the duplicate block mentioned).
- Around line 436-441: The mapping of RuntimeError::UnknownHead in the
set_head_eligibility call is currently converted into an AbiError with code
error_codes::ENGINE_ERROR; change the mapping so that when
self.runtime.set_head_eligibility(...) returns an error matching
RuntimeError::UnknownHead(_) you map it to an AbiError with code
error_codes::INVALID_CONTROL (preserving the message), and only use ENGINE_ERROR
for true engine faults—update the closure in the map_err on set_head_eligibility
to match on the error variant (RuntimeError::UnknownHead => INVALID_CONTROL, _
=> ENGINE_ERROR) while keeping the existing message formatting.
In `@docs/plans/adr-0008-and-0009.md`:
- Around line 755-757: Update the bullet that lists migration targets to remove
the reintroduction of snapshot_at(...); instead reference the observation-backed
snapshot path and clarify that the WASM adapter was deleted in Phase 6 so
snapshot_at(...) is not a Phase 7 target, and remove any implication that
reader-side GraphStore replay fallbacks or snapshot_at(...) are being
reintroduced; update mentions of fork(...), playback seek/step, and "full-state
replay" to explicitly point to the observation-backed snapshot flow and Phase 6
removal to keep the migration story consistent.
In `@scripts/verify-local.sh`:
- Around line 1440-1443: The current logic uses git write-tree for "full" which
only hashes the index and misses unstaged/untracked changes; keep git write-tree
for pre-commit but for "full" and "working-tree" derive VERIFY_STAMP_SUBJECT
from the actual worktree by creating a temporary index, running
GIT_INDEX_FILE="$tmp" git add -A -- . and then GIT_INDEX_FILE="$tmp" git
write-tree (see a helper like worktree_tree()), then remove the temp index;
replace the branches that set VERIFY_STAMP_SUBJECT (the checks against
VERIFY_MODE_CONTEXT and MODE and the identical block at the later site) to call
that worktree-based helper for full/working-tree and retain git write-tree only
for pre-commit.
In `@tests/hooks/test_pr_status.sh`:
- Around line 26-27: The test creates a temporary dir in the variable tmp with
mktemp and later removes it unconditionally, but with set -e an early non-zero
exit can leave tmp behind; add a trap to ensure cleanup on EXIT/ERR by
registering a cleanup function that rm -rfs "$tmp" and set trap 'cleanup' EXIT
(or trap 'rm -rf "$tmp"' EXIT) immediately after tmp="$(mktemp -d)" (and
similarly for the second tmp usage around the block that currently does rm -rf
in the 72-77 region) so the temp directory is always removed even on early
failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd0da86e-8c5e-443f-bc27-4df670123ab7
📒 Files selected for processing (53)
.githooks/_timing.sh.githooks/commit-msg.githooks/pre-commit.githooks/pre-push.githooks/pre-push-parallel.githooks/pre-push-sequential.githooks/pre-rebase.gitignoreCHANGELOG.mdMakefilecrates/echo-wasm-abi/src/kernel_port.rscrates/echo-wasm-abi/src/lib.rscrates/ttd-browser/src/lib.rscrates/warp-core/README.mdcrates/warp-core/src/clock.rscrates/warp-core/src/coordinator.rscrates/warp-core/src/head.rscrates/warp-core/src/lib.rscrates/warp-core/src/observation.rscrates/warp-core/src/playback.rscrates/warp-core/src/provenance_store.rscrates/warp-core/src/tick_patch.rscrates/warp-core/src/worldline.rscrates/warp-core/src/worldline_registry.rscrates/warp-core/src/worldline_state.rscrates/warp-core/tests/checkpoint_fork_tests.rscrates/warp-core/tests/common/mod.rscrates/warp-core/tests/golden_vectors_phase0.rscrates/warp-core/tests/inbox.rscrates/warp-core/tests/invariant_property_tests.rscrates/warp-core/tests/outputs_playback_tests.rscrates/warp-core/tests/playback_cursor_tests.rscrates/warp-core/tests/slice_theorem_proof.rscrates/warp-core/tests/view_session_tests.rscrates/warp-wasm/README.mdcrates/warp-wasm/src/lib.rscrates/warp-wasm/src/warp_kernel.rsdocs/ROADMAP/first-light/browser-visualization.mddocs/ROADMAP/first-light/wasm-runtime.mddocs/adr/ADR-0010-observational-seek-and-administrative-rewind.mddocs/adr/ADR-0011-explicit-observation-contract.mddocs/march-16.plan.mddocs/plans/adr-0008-and-0009.mddocs/spec/SPEC-0009-wasm-abi-v1.mddocs/spec/SPEC-0009-wasm-abi-v3.mdscripts/hooks/README.mdscripts/plot-prepush-timing.mjsscripts/pr-status.shscripts/verify-local.shtests/hooks/test_hook_issues.shtests/hooks/test_hook_timing.shtests/hooks/test_pr_status.shtests/hooks/test_verify_local.sh
💤 Files with no reviewable changes (1)
- docs/spec/SPEC-0009-wasm-abi-v1.md
Comment on lines
+54
to
+71
| on_exit() { | ||
| local exit_code=$? | ||
| cleanup "$exit_code" | ||
| } | ||
| on_signal() { | ||
| local signal_name="$1" | ||
| local exit_code=1 | ||
| if [[ "$signal_name" == "INT" ]]; then | ||
| exit_code=130 | ||
| elif [[ "$signal_name" == "TERM" ]]; then | ||
| exit_code=143 | ||
| fi | ||
| cleanup "$exit_code" | ||
| exit "$exit_code" | ||
| } | ||
| trap cleanup EXIT INT TERM | ||
| trap on_exit EXIT | ||
| trap 'on_signal INT' INT | ||
| trap 'on_signal TERM' TERM |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '[repo]\n'
sed -n '54,71p' .githooks/pre-push-parallel
printf '\n[demo]\n'
bash <<'BASH' || true
set -euo pipefail
count=0
cleanup() { count=$((count + 1)); printf 'cleanup:%s\n' "$count"; }
on_exit() { cleanup; }
on_signal() { cleanup; exit 130; }
trap on_exit EXIT
trap 'on_signal' INT
kill -INT $$
BASHRepository: flyingrobots/echo
Length of output: 453
🏁 Script executed:
#!/bin/bash
# Examine the full cleanup function and timing append logic
cat -n .githooks/pre-push-parallel | sed -n '1,100p'Repository: flyingrobots/echo
Length of output: 3715
🏁 Script executed:
#!/bin/bash
# Check for early exits before trap registration (line 69)
grep -n "exit\|return" .githooks/pre-push-parallel | head -20Repository: flyingrobots/echo
Length of output: 536
🏁 Script executed:
#!/bin/bash
# Look for hook_timing_append to understand the timing side effects
rg "hook_timing_append|LOGDIR" .githooks/pre-push-parallel -A 2 -B 2Repository: flyingrobots/echo
Length of output: 1428
🏁 Script executed:
#!/bin/bash
# Examine the timing module to understand append side effects
cat .githooks/_timing.shRepository: flyingrobots/echo
Length of output: 1561
Disarm the EXIT trap in the signal path.
Line 66 calls cleanup, and line 67 immediately exits into the EXIT trap on line 69. Interrupted runs therefore execute cleanup() twice. While the timing guard in _timing.sh prevents duplicate CSV rows, the double cleanup still wastes cycles killing already-dead jobs and deleting the already-removed temp directory.
🔧 Proposed fix
on_signal() {
local signal_name="$1"
local exit_code=1
if [[ "$signal_name" == "INT" ]]; then
exit_code=130
elif [[ "$signal_name" == "TERM" ]]; then
exit_code=143
fi
+ trap - EXIT
cleanup "$exit_code"
exit "$exit_code"
}Run the demo in the original comment to confirm Bash trap behavior.
📝 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
| on_exit() { | |
| local exit_code=$? | |
| cleanup "$exit_code" | |
| } | |
| on_signal() { | |
| local signal_name="$1" | |
| local exit_code=1 | |
| if [[ "$signal_name" == "INT" ]]; then | |
| exit_code=130 | |
| elif [[ "$signal_name" == "TERM" ]]; then | |
| exit_code=143 | |
| fi | |
| cleanup "$exit_code" | |
| exit "$exit_code" | |
| } | |
| trap cleanup EXIT INT TERM | |
| trap on_exit EXIT | |
| trap 'on_signal INT' INT | |
| trap 'on_signal TERM' TERM | |
| on_exit() { | |
| local exit_code=$? | |
| cleanup "$exit_code" | |
| } | |
| on_signal() { | |
| local signal_name="$1" | |
| local exit_code=1 | |
| if [[ "$signal_name" == "INT" ]]; then | |
| exit_code=130 | |
| elif [[ "$signal_name" == "TERM" ]]; then | |
| exit_code=143 | |
| fi | |
| trap - EXIT | |
| cleanup "$exit_code" | |
| exit "$exit_code" | |
| } | |
| trap on_exit EXIT | |
| trap 'on_signal INT' INT | |
| trap 'on_signal TERM' TERM |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.githooks/pre-push-parallel around lines 54 - 71, The on_signal handler
calls cleanup() and then exits, which re-triggers the on_exit EXIT trap and
causes cleanup to run twice; in the on_signal function (referencing on_signal,
on_exit, and cleanup) disarm the EXIT trap before invoking cleanup (e.g., call
trap - EXIT) so cleanup is only executed once, then proceed to call cleanup
"$exit_code" and exit "$exit_code".
Comment on lines
+14
to
+15
| .dx-debug/* | ||
| blog/* |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Gitignore patterns may not cover nested subdirectories.
.dx-debug/* and blog/* only match entries directly under those directories. If nested paths like .dx-debug/2026/timing.csv exist, they won't be ignored. Use .dx-debug/ (trailing slash, no asterisk) or .dx-debug/** for recursive coverage.
-.dx-debug/*
-blog/*
+.dx-debug/
+blog/📝 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
| .dx-debug/* | |
| blog/* | |
| .dx-debug/ | |
| blog/ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 14 - 15, The .gitignore entries `.dx-debug/*` and
`blog/*` only ignore top-level files; change them to recursive directory
patterns to cover nested files (e.g., replace `.dx-debug/*` with `.dx-debug/` or
`.dx-debug/**` and `blog/*` with `blog/` or `blog/**`) so paths like
`.dx-debug/2026/timing.csv` and nested files under `blog/` are ignored.
Comment on lines
+47
to
+48
| /// Reserved EINT op id for privileged control intents. | ||
| pub const CONTROL_INTENT_V1_OP_ID: u32 = u32::MAX; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep the reserved control op-id out of the blind public helper.
CONTROL_INTENT_V1_OP_ID is now protocol-reserved, but pack_intent_v1() still accepts u32::MAX with arbitrary payload bytes. That leaves the public low-level API able to mint untyped control envelopes and keeps the application op-id space ambiguous. Either reject the reserved id in pack_intent_v1(), or demote that function to an explicitly raw/internal constructor and keep the typed helper as the only public control path.
Also applies to: 130-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/echo-wasm-abi/src/lib.rs` around lines 47 - 48, pack_intent_v1
currently allows u32::MAX (CONTROL_INTENT_V1_OP_ID) and thus lets callers create
reserved control envelopes; fix by rejecting that reserved id in pack_intent_v1:
add a guard that if op_id == CONTROL_INTENT_V1_OP_ID the function returns an
error (or Result::Err) rather than packing, and update callers accordingly, or
alternatively make pack_intent_v1 non-public (crate/private) and expose a typed
public helper (e.g., pack_control_intent_v1 / pack_typed_intent) that uses
CONTROL_INTENT_V1_OP_ID internally—refer to the symbols CONTROL_INTENT_V1_OP_ID
and pack_intent_v1 to locate the change and ensure the public API cannot accept
u32::MAX.
Comment on lines
+130
to
+146
| /// Packs a privileged control intent into an EINT envelope v1. | ||
| pub fn pack_control_intent_v1( | ||
| intent: &kernel_port::ControlIntentV1, | ||
| ) -> Result<Vec<u8>, EnvelopeError> { | ||
| let bytes = encode_cbor(intent).map_err(|_| EnvelopeError::Malformed)?; | ||
| pack_intent_v1(CONTROL_INTENT_V1_OP_ID, &bytes) | ||
| } | ||
|
|
||
| /// Unpacks and decodes a privileged control intent from an EINT envelope v1. | ||
| pub fn unpack_control_intent_v1( | ||
| bytes: &[u8], | ||
| ) -> Result<kernel_port::ControlIntentV1, EnvelopeError> { | ||
| let (op_id, vars) = unpack_intent_v1(bytes)?; | ||
| if op_id != CONTROL_INTENT_V1_OP_ID { | ||
| return Err(EnvelopeError::Malformed); | ||
| } | ||
| decode_cbor(vars).map_err(|_| EnvelopeError::Malformed) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add ABI-level tests for the control envelope helpers.
This file’s unit suite still only covers generic pack_intent_v1() / unpack_intent_v1(). Add local round-trip, wrong-op-id, and malformed-CBOR cases here so control-envelope drift fails in echo-wasm-abi before it reaches downstream crates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/echo-wasm-abi/src/lib.rs` around lines 130 - 146, Add ABI-level unit
tests covering pack_control_intent_v1 and unpack_control_intent_v1: (1) a
round-trip test that encodes a kernel_port::ControlIntentV1 with
pack_control_intent_v1 then decodes with unpack_control_intent_v1 and asserts
equality; (2) a wrong-op-id test that calls unpack_control_intent_v1 on an
envelope whose op_id has been mutated (use pack_intent_v1/CUSTOM op id or mutate
the packed bytes) and assert it returns EnvelopeError::Malformed; and (3) a
malformed-CBOR test that passes an envelope with CONTROL_INTENT_V1_OP_ID but
invalid CBOR payload and assert decode returns EnvelopeError::Malformed. Use the
existing helpers encode_cbor/decode_cbor, CONTROL_INTENT_V1_OP_ID,
pack_intent_v1, and unpack_intent_v1 to construct the inputs and error
assertions.
Comment on lines
+58
to
+70
| for hook in commit-msg pre-commit pre-push pre-push-parallel pre-push-sequential pre-rebase; do | ||
| if rg -q "hook_timing_prepare \"\\\$REPO_ROOT\" \"$hook\"" ".githooks/$hook"; then | ||
| pass ".githooks/$hook enables hook timing" | ||
| else | ||
| fail ".githooks/$hook should enable hook timing" | ||
| fi | ||
| done | ||
|
|
||
| if rg -q '^\.dx-debug/\*$' .gitignore; then | ||
| pass ".gitignore ignores .dx-debug timing artifacts" | ||
| else | ||
| fail ".gitignore should ignore .dx-debug timing artifacts" | ||
| fi |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Static checks cover all hooks, but only a subset gets runtime testing.
The loop at lines 58–64 verifies hook_timing_prepare presence in all six hooks, but only commit-msg, pre-rebase, pre-push, and pre-commit have actual runtime fixture tests. pre-push-parallel and pre-push-sequential are checked statically but never executed in isolation.
If these hooks have distinct runtime behavior, consider adding fixture tests for them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hooks/test_hook_timing.sh` around lines 58 - 70, The test loop verifies
hook_timing_prepare is present in all hooks but only a subset get runtime
execution; add runtime fixture tests for the two un-executed hooks
(pre-push-parallel and pre-push-sequential) mirroring the existing runtime tests
for commit-msg, pre-commit, pre-push and pre-rebase so their behavior is
validated at runtime; update the test suite to invoke those hooks in isolation
(using the same fixture patterns and assertions used by the existing runtime
tests) and ensure any unique parallel/sequential behavior is asserted
specifically for pre-push-parallel and pre-push-sequential.
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 `@crates/warp-core/tests/playback_cursor_tests.rs`:
- Around line 281-284: The assertion comparing hash_at_3 and hash_at_8 in
playback_cursor_tests.rs is fragile because it relies on the fixture behavior
(create_add_node_patch) to leave the reachable state root unchanged; update the
test to be explicit: either add a clear comment in the test module header
documenting the assumption that create_add_node_patch produces no
reachable-state-root change across ticks, or replace the fixture usage so the
test exercises a patch that deterministically changes (or deterministically
preserves) the reachable root across those ticks (i.e., use a different fixture
or construct a patch that is known to produce distinct state roots), and update
any assertion messaging to reflect the chosen guarantee.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 19af4168-3a7a-4258-8250-088c3bb8a985
📒 Files selected for processing (1)
crates/warp-core/tests/playback_cursor_tests.rs
Owner
Author
|
@coderabbitai review please 🥕 |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
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/lib.rs (1)
10-16:⚠️ Potential issue | 🟡 MinorThe module rustdoc overstates the envelope contract.
get_codec_id(),get_registry_version(), andget_schema_sha256_hex()still return rawJsValue, so "All exports return CBOR-encoded bytes" is false. Narrow this to the ABI byte exports, or call out those helpers as exceptions.Based on learnings: Applies to crates/**/*.rs : Every public API across crates (warp-core, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-wasm/src/lib.rs` around lines 10 - 16, The module-level rustdoc incorrectly claims "All exports return CBOR-encoded bytes" while helper functions return raw JS values; update the crate documentation so it only states that the ABI byte exports return CBOR-encoded envelopes and explicitly list exceptions: get_codec_id, get_registry_version, and get_schema_sha256_hex (and any similar helpers) which return raw JsValue strings/values; locate the module docblock in lib.rs and reword to either narrow the claim to "ABI byte exports" or add a parenthetical noting these three helpers are exceptions and describe their return types/intent briefly.
♻️ Duplicate comments (13)
scripts/pr-status.sh (1)
62-76:⚠️ Potential issue | 🟡 MinorPython parsing failure still leaves variables empty.
If
python3fails (malformed JSON, missing keys), the heredoc outputs nothing andreadsilently assigns empty strings. The script continues printingPR #with no number.The prior review marked this addressed, but the current code shows no error handling around the python3 invocation.
Add error checking
-read -r PR_NUMBER PR_URL HEAD_SHA REVIEW_DECISION MERGE_STATE <<EOF -$(VIEW_JSON="$VIEW_JSON" python3 -c ' +parse_output="$(VIEW_JSON="$VIEW_JSON" python3 -c ' import json import os data = json.loads(os.environ["VIEW_JSON"]) print( data["number"], data["url"], data["headRefOid"][:12], data.get("reviewDecision") or "NONE", data.get("mergeStateStatus") or "UNKNOWN", ) -') -EOF +')" || { echo "Failed to parse PR JSON" >&2; exit 1; } + +read -r PR_NUMBER PR_URL HEAD_SHA REVIEW_DECISION MERGE_STATE <<< "$parse_output"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pr-status.sh` around lines 62 - 76, The heredoc that runs python3 to populate PR_NUMBER, PR_URL, HEAD_SHA, REVIEW_DECISION, MERGE_STATE is unguarded: if python3 fails it emits nothing and read silently assigns empties; update the script to capture the python3 output into a variable, check the python3 exit status (or that the output is non-empty/contains expected fields), and on failure print an error including $VIEW_JSON context and exit non-zero; locate the block that invokes "python3 -c '...'" and the read -r PR_NUMBER PR_URL ... EOF and implement this validation/early-exit so downstream code never proceeds with empty PR_* variables..githooks/pre-push-parallel (1)
58-71:⚠️ Potential issue | 🟡 MinorDouble cleanup: disarm EXIT trap in signal handler.
on_signalcallscleanup()at line 66, thenexitat line 67 re-triggers theEXITtrap, which callson_exit()→cleanup()again. The timing guard in_timing.shprevents duplicate CSV rows, but cleanup still wastes cycles attempting to kill already-dead jobs and remove an already-deleted temp directory.🔧 Proposed fix
on_signal() { local signal_name="$1" local exit_code=1 if [[ "$signal_name" == "INT" ]]; then exit_code=130 elif [[ "$signal_name" == "TERM" ]]; then exit_code=143 fi + trap - EXIT cleanup "$exit_code" exit "$exit_code" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.githooks/pre-push-parallel around lines 58 - 71, The signal handler on_signal currently calls cleanup() and then exits, which re-triggers the on_exit EXIT trap and causes cleanup() to run twice; fix by disarming the EXIT trap at the start of on_signal (use the shell trap command to remove the EXIT handler) so that after doing "trap - EXIT" the handler can call cleanup() and exit without re-invoking on_exit; update references in the same file to on_signal, on_exit, and cleanup to ensure the EXIT trap is removed before calling cleanup and exiting.crates/echo-wasm-abi/src/lib.rs (2)
130-147:⚠️ Potential issue | 🟡 MinorAdd ABI-level tests for control-envelope helpers.
pack_control_intent_v1(...)/unpack_control_intent_v1(...)are untested in this module. Add round-trip, wrong-op-id, and malformed-payload cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/echo-wasm-abi/src/lib.rs` around lines 130 - 147, Add ABI-level unit tests for pack_control_intent_v1 and unpack_control_intent_v1: write a round-trip test that constructs a valid kernel_port::ControlIntentV1, calls pack_control_intent_v1 then unpack_control_intent_v1 and asserts equality; write a wrong-op-id test by taking a valid packed envelope and altering the op id bytes (not equal to CONTROL_INTENT_V1_OP_ID) and assert unpack_control_intent_v1 returns Err(EnvelopeError::Malformed); write a malformed-payload test by constructing an envelope with the correct op id but invalid/malformed CBOR payload bytes and assert it returns Err(EnvelopeError::Malformed). Use the existing helper functions pack_control_intent_v1, unpack_control_intent_v1 and the constant CONTROL_INTENT_V1_OP_ID to locate code under test and assert on EnvelopeError::Malformed for failure cases.
47-48:⚠️ Potential issue | 🟠 MajorPrevent raw packing of the reserved control op-id.
CONTROL_INTENT_V1_OP_IDis protocol-reserved, but untyped callers can still mint it viapack_intent_v1(...), bypassing the typed control helper path.Proposed fix
pub enum EnvelopeError { @@ PayloadTooLarge, + ReservedOpId, } @@ match self { @@ Self::PayloadTooLarge => f.write_str("payload exceeds u32::MAX"), + Self::ReservedOpId => f.write_str("reserved op_id"), } } } @@ pub fn pack_intent_v1(op_id: u32, vars: &[u8]) -> Result<Vec<u8>, EnvelopeError> { + if op_id == CONTROL_INTENT_V1_OP_ID { + return Err(EnvelopeError::ReservedOpId); + } let vars_len: u32 = vars .len() .try_into() .map_err(|_| EnvelopeError::PayloadTooLarge)?;Also applies to: 130-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/echo-wasm-abi/src/lib.rs` around lines 47 - 48, Prevent callers from packing the reserved control op id by adding a runtime check in pack_intent_v1 (and any other raw packing helpers) that rejects or returns an error when the provided op_id equals CONTROL_INTENT_V1_OP_ID; update pack_intent_v1 to validate the op_id parameter and return an Err/Result or panic with a clear message directing users to the typed control helper (the control-intent constructor) instead of allowing untyped minting of CONTROL_INTENT_V1_OP_ID. Ensure the same guard is added wherever raw op ids are accepted so the protocol-reserved constant CONTROL_INTENT_V1_OP_ID cannot be produced via pack_intent_v1 or similar functions.crates/warp-wasm/README.md (1)
15-18:⚠️ Potential issue | 🟡 MinorFix contradictory read-export wording.
Line 16 says
observe(...)is the only public read export, while Line 16-Line 17 also listsscheduler_status()as a read-only export.Suggested wording
-- Exposes the ABI v3 observation-first and intent-shaped control surface: - `observe(...)` is the only public read export, `scheduler_status()` is the - read-only scheduler metadata export, and all external writes or scheduler - control requests flow through `dispatch_intent(...)`. +- Exposes the ABI v3 observation-first and intent-shaped control surface: + `observe(...)` is the only public world-state read export, `scheduler_status()` + is a separate read-only scheduler metadata export, and all external writes or + scheduler control requests flow through `dispatch_intent(...)`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-wasm/README.md` around lines 15 - 18, The README line is contradictory about read exports; update the wording so it consistently describes which functions are public read exports: change the sentence to state that both observe(...) and scheduler_status() are public read exports (e.g., "observe(...) and scheduler_status() are the public read exports"), and keep the rest intact so all external writes or scheduler control requests flow through dispatch_intent(...).docs/plans/adr-0008-and-0009.md (1)
755-757:⚠️ Potential issue | 🟡 MinorPhase 7 still names a deleted adapter.
This migration bullet reintroduces
snapshot_at(...)as if it were still a live surface, even though Phase 6 just deleted that adapter. Point this at the observation-backed snapshot path instead, or the plan contradicts the current ABI again.As per coding guidelines, documentation accuracy matters — especially anything touching determinism guarantees, hash stability, or canonical ordering.
🤖 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 755 - 757, The migration bullet incorrectly references the deleted adapter `snapshot_at(...)`; update the text to point to the observation-backed snapshot path instead (replace `snapshot_at(...)` with the canonical observation-backed snapshot surface name used elsewhere in the ADR), and ensure Phase 7's description no longer implies the removed adapter exists—also verify related mentions of `fork(...)`, playback seek/step, replay paths, and reader-side `GraphStore` replay fallbacks align with Phase 6's removals so the ABI and determinism guarantees remain consistent.crates/warp-core/src/head.rs (1)
345-350:⚠️ Potential issue | 🟡 MinorAdd a dormant/readmit regression for
rebuild().
RunnableWriterSet::rebuild()now filters onhead.is_admitted()as well as pause state, but this file still only exercises pause-based exclusion. Add one roundtrip that sets a head toDormant, assertsrebuild()drops it, then readmits it and asserts it reappears.Based on learnings: Applies to docs/archive/crates/**/*.rs : Tests and benchmarks are mandatory for runtime changes once the harness exists
🤖 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 345 - 350, Add a regression test that exercises RunnableWriterSet::rebuild() for Dormant/readmit semantics: create a PlaybackHeadRegistry with a head, ensure it is initially admitted and present after rebuild(), then transition the head to Dormant (use the API that marks a head Dormant), call RunnableWriterSet::rebuild(®istry) and assert the key is removed, then readmit the same head, call rebuild() again and assert the key is present; place this test alongside the existing pause-state test and use the same helper setup utilities to locate the head and keys.crates/warp-core/src/provenance_store.rs (2)
722-723:⚠️ Potential issue | 🔴 CriticalDo not cast
WorldlineTicktousizewithas.These
as_u64() as usizeconversions truncate on 32-bit targets. A high tick can then alias a different entry/checkpoint index on wasm32 while 64-bit builds reject it, which makes provenance validation and replay architecture-dependent. Useusize::try_from(...)and map overflow to the context-appropriateHistoryError.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)."
Also applies to: 1062-1065, 1205-1208, 1308-1313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/provenance_store.rs` around lines 722 - 723, The code casts WorldlineTick via target_tick.as_u64() as usize (e.g., building target_len) which truncates on 32-bit targets; replace these casts with fallible conversions using usize::try_from(target_tick.as_u64()) and map any Err(Overflow) into the appropriate HistoryError variant used by this module so provenance validation is platform-deterministic; apply the same fix to the other occurrences that convert WorldlineTick to usize (the places around where replayed.tick_history.len() is compared/indexed and the other listed sites) so all index/length computations use try_from and propagate HistoryError on overflow.
722-753:⚠️ Potential issue | 🔴 CriticalClear checkpoint-sourced replay metadata before hydrating.
hydrate_replay_metadata()only fixes the suffix it has to append. Any existingtick_historyprefix orlast_snapshotalready present incheckpoint.statesurvives unchanged, solast_snapshot()/ historical snapshot reads can still reflect artifact data instead of provenance.🔧 Proposed fix
let checkpoint_tick = checkpoint.checkpoint.worldline_tick; let mut replayed = checkpoint.state; + replayed.tick_history.clear(); + replayed.last_snapshot = None; replayed.last_materialization_errors.clear(); replayed.committed_ingress.clear(); hydrate_replay_metadata(provenance, worldline_id, &mut replayed, checkpoint_tick)?;Based on learnings, replay metadata is rebuilt exclusively from stored provenance via
hydrate_replay_metadata(...); checkpoint restore should not trust artifact-sourcedtick_historyorlast_snapshot.Also applies to: 814-818
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/provenance_store.rs` around lines 722 - 753, hydrate_replay_metadata currently only appends a suffix to replayed.tick_history and leaves any checkpoint-sourced prefix or last_snapshot in place; before rebuilding from provenance you must clear existing checkpoint-derived replay metadata: call replayed.tick_history.clear(), set replayed.last_snapshot = None, replayed.last_materialization.clear(), replayed.last_materialization_errors.clear(), and replayed.tx_counter = 0 prior to the loop that pushes replay artifacts (the code around hydrate_replay_metadata / replayed usage and the block that iterates raw_tick to target_tick), and apply the same clearing at the other hydrate/replay branch noted in the review so no artifact-sourced state survives.scripts/verify-local.sh (1)
688-711:⚠️ Potential issue | 🔴 CriticalDisable
set -edisables error propagation in multi-command lanes.
run_timed_step()and the parallel worker both invoke lanes witherrexitoff (lines 689, 782). Multi-command lanes likerun_full_lane_guards(),run_full_lane_tests_runtime(), andrun_full_lane_tests_warp_core()can then continue past the first failing command and return the status of a later success/no-op, so the verifier reports a passing lane after a failed check. Wrap each lane invocation in an inner subshell withset -euo pipefailand capture that subshell's exit code instead.Proposed fix
local started_at started_at="$(now_seconds)" local rc=0 - set +e - "$step_func" "$@" - rc=$? - set -e + ( + set -euo pipefail + "$step_func" "$@" + ) || rc=$? local finished_at finished_at="$(now_seconds)" local elapsed_seconds=$((finished_at - started_at))( set -uo pipefail started_at="$(now_seconds)" rc=0 - "${lane_funcs[$i]}" || rc=$? + ( + set -euo pipefail + "${lane_funcs[$i]}" + ) || rc=$? finished_at="$(now_seconds)" printf '%s\n' "$((finished_at - started_at))" >"${logdir}/${lane_names[$i]}.elapsed" exit "$rc"Also applies to: 782-788
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-local.sh` around lines 688 - 711, The run_timed_step function currently calls the lane function with errexit disabled (set +e), which allows multi-command lane functions (e.g., run_full_lane_guards, run_full_lane_tests_runtime, run_full_lane_tests_warp_core) to continue past a failing command and produce a misleading rc; to fix, invoke the lane in an inner subshell that sets strict failure handling (set -euo pipefail) so the first failing command causes the subshell to exit, capture that subshell's exit code into rc, then continue to compute timing and call report_lane_timing and append_timing_record as before; update the invocation inside run_timed_step (and the parallel worker block that mirrors it) to replace the direct "$step_func" call with a subshell executing "set -euo pipefail; \"$step_func\" \"$@\"" and assign its exit status to rc.crates/warp-wasm/src/warp_kernel.rs (2)
436-441:⚠️ Potential issue | 🟡 MinorMap unknown heads to
INVALID_CONTROL.
RuntimeError::UnknownHead(_)comes from a bad caller-suppliedAbiHeadKey. ReturningENGINE_ERRORhere misclassifies invalid input as a kernel fault.Suggested mapping
self.runtime .set_head_eligibility(key, eligibility) - .map_err(|e| AbiError { - code: error_codes::ENGINE_ERROR, - message: e.to_string(), - })?; + .map_err(|e| { + let code = match &e { + RuntimeError::UnknownHead(_) => error_codes::INVALID_CONTROL, + _ => error_codes::ENGINE_ERROR, + }; + AbiError { + code, + message: e.to_string(), + } + })?;🤖 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 436 - 441, The current map_err in the self.runtime.set_head_eligibility(...) call wraps all runtime errors as error_codes::ENGINE_ERROR; change the map_err closure to inspect the runtime error (match RuntimeError::UnknownHead(_)) and return an AbiError with code error_codes::INVALID_CONTROL for that variant, otherwise retain error_codes::ENGINE_ERROR, keeping the same e.to_string() as the message; locate the call to self.runtime.set_head_eligibility, the AbiError construction, and use RuntimeError::UnknownHead and error_codes::INVALID_CONTROL in the match.
351-358:⚠️ Potential issue | 🟠 Major
Stop/Stoppingare dead states in this synchronous loop.
Start(UntilIdle)runs the entire scheduler loop beforedispatch_intent()returns and clears active state on exit. In single-threaded WASM, callers never observeRunning/Stopping, soControlIntentV1::Stopcannot interrupt at a scheduler boundary; it only rewrites cached completion after the run is already over. Persist runs across calls or remove that contract from the ABI surface.Also applies to: 372-425, 426-429
🤖 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 351 - 358, The code assumes ControlIntentV1::Stop can interrupt a running scheduler by checking scheduler_status.state for SchedulerState::Running/Stopping (see scheduler_status, SchedulerState, dispatch_intent()), but in single-threaded WASM Start(UntilIdle) completes synchronously so Running/Stopping are never observed; fix by choosing one of two concrete fixes: (A) make runs persistent across calls so the scheduler truly stays Running/Stopping (e.g., move the loop into a background task/thread and update scheduler_status transitions accordingly so Stop can interrupt), or (B) remove/disable the interrupt contract from the ABI by changing dispatch_intent()/ControlIntentV1::Stop handling to treat Stop as unsupported/no-op or return an explicit error and remove checks that expect runtime interruption (update places using SchedulerState::Running | SchedulerState::Stopping to match the chosen model). Ensure all related branches (including the blocks around the earlier SchedulerState check and the later handling of ControlIntentV1::Stop) are updated consistently.crates/warp-core/src/playback.rs (1)
531-543:⚠️ Potential issue | 🟠 MajorThe forward replay fast path still clones the full
WorldlineState.
PlayandStepForwardboth land here. Cloningself.statebefore applying a recorded suffix adds O(state_size) work to every reader tick and undercuts the checkpoint optimization. The cursor is already documented as undefined after replay errors, so mutateself.statein place on the forward branch and only materialize a replacement when rebuilding from U0/checkpoints. As per coding guidelines, warp-core is the deterministic kernel. Performance matters — flag unnecessary allocations in hot paths.Possible shape
- let next_state = if target < self.tick || should_restore_from_checkpoint { - replay_worldline_state_at_from_provenance( - provenance, - self.worldline_id, - initial_state, - target, - ) - .map_err(|error| Self::map_replay_error(target, error))? - } else { - let mut state = self.state.clone(); - advance_replay_state(provenance, self.worldline_id, &mut state, self.tick, target) - .map_err(|error| Self::map_replay_error(target, error))?; - state - }; - - // Update cursor position - self.state = next_state; + if target < self.tick || should_restore_from_checkpoint { + self.state = replay_worldline_state_at_from_provenance( + provenance, + self.worldline_id, + initial_state, + target, + ) + .map_err(|error| Self::map_replay_error(target, error))?; + } else { + advance_replay_state( + provenance, + self.worldline_id, + &mut self.state, + self.tick, + target, + ) + .map_err(|error| Self::map_replay_error(target, error))?; + }Also applies to: 547-548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/playback.rs` around lines 531 - 543, The forward-replay branch currently clones the entire WorldlineState (self.state.clone()) before calling advance_replay_state, causing O(state_size) work per tick; instead mutate self.state in place when target >= self.tick by passing &mut self.state to advance_replay_state and only materialize/clone a replacement for the branch that rebuilds from provenance via replay_worldline_state_at_from_provenance; update both occurrences that clone self.state (the forward branch around advance_replay_state and the duplicate at 547-548) to avoid the clone and rely on in-place mutation, preserving the existing map_replay_error handling for errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.githooks/_timing.sh:
- Around line 49-52: The header-write has a race: two concurrent hook runs can
both see no file and append duplicate headers; fix by obtaining an exclusive
lock on "$csv_file" before checking/writing (use a file descriptor with flock),
e.g. open "$csv_file" for append (exec 200>>"$csv_file"), flock 200, then check
if the file is empty/has no header (e.g. [[ ! -s "$csv_file" ]] or grep for the
header) and only then write the header to the locked FD (printf
'timestamp_utc,elapsed_ms,exit_code,pid\n' >&200), finally release the lock
(flock -u) and close the FD; reference variables: csv_file, csv_dir and the
header string 'timestamp_utc,elapsed_ms,exit_code,pid\n'.
- Around line 5-15: The current hook_timing_now_ns may use python3 or date
depending on availability, which can change between hook_timing_prepare and
hook_timing_append and produce invalid deltas; modify hook_timing_prepare to
detect and export a consistent timing method (e.g. set
DX_HOOK_TIMING_METHOD="monotonic_ns" or "epoch_s_as_ns") and then update
hook_timing_now_ns to read DX_HOOK_TIMING_METHOD and always use the cached
method (calling python3 only if DX_HOOK_TIMING_METHOD indicates monotonic_ns,
otherwise using date), ensuring prepare/append use the same clock source; keep
existing negative-clamp logic but remove reliance on ephemeral command detection
at call time.
In `@crates/echo-wasm-abi/src/kernel_port.rs`:
- Around line 189-209: HeadDisposition currently lacks a Dormant variant even
though ControlIntentV1::SetHeadEligibility can set a head to Dormant; add a
Dormant variant to the HeadDisposition enum (preserving #[serde(rename_all =
"snake_case")] and enum attributes) and then update any match arms, conversions,
and uses of HeadDisposition (e.g., functions that map eligibility to disposition
or serialize/deserialize code paths) to handle HeadDisposition::Dormant so the
ABI can represent a dormant scheduler state without breaking serde or pattern
matches.
In `@crates/ttd-browser/src/lib.rs`:
- Around line 742-754: Remove the redundant explicit check against tick-0: drop
the initial if cursor.tick == WorldlineTick::ZERO { ... } block and instead rely
on the single guarded subtraction used when building the provenance key; use
cursor.tick.checked_sub(1).ok_or_else(|| "cannot commit at tick 0".to_string())?
when calling self.provenance.entry(...) so the zero-check is centralized and
consistent with the existing checked_sub logic.
In `@crates/warp-core/src/clock.rs`:
- Around line 34-50: Add unit tests that assert the overflow/underflow behavior
of the counter helpers: verify the ZERO and MAX constants behave as expected,
that checked_add(MAX, 1) returns None and checked_add on a non-edge value
returns Some(new_value), that checked_sub(ZERO, 1) returns None and checked_sub
on a non-edge value returns Some(new_value), and that checked_increment on MAX
returns None while checked_increment on a non-edge value returns
Some(new_value); target the functions/constants ZERO, MAX, checked_add,
checked_sub, and checked_increment so the tests will fail if a macro edit
changes their arithmetic semantics.
In `@crates/warp-core/src/playback.rs`:
- Around line 349-352: Make the invariant-bearing fields (e.g., tick:
WorldlineTick and the state/store fields referenced by focused_store() and
current_state_root()) private so external code cannot mutate them directly;
expose only safe accessors already implemented (focused_store(),
current_state_root(), and any read-only getters) and update any callers to use
those helpers. Change the visibility of the fields declared as pub (including
the ones in the other referenced ranges) to private, remove direct external
field accesses, and ensure methods that need to update tick/state are the sole
mutators (preserve existing mutating APIs or add controlled setters on the
struct). Verify all usages in the codebase call the accessors (not the fields)
and keep WorldlineTick and state mutations encapsulated to prevent
desynchronization of replay/seek invariants.
In `@crates/warp-core/src/provenance_store.rs`:
- Around line 338-345: The from_state constructor for
CheckpointRef/ReplayCheckpoint returns the full WorldlineState while
authenticating only state_root(), which allows extra detached nodes/warp
instances to be carried and later leak; update from_state (and the analogous
code at the 792-812 region) to either compute and store a deterministic
full-state hash (covering the entire replay checkpoint contents) instead of
using state_root(), or strip the returned payload to the authoritative subset
used by ReplayCheckpoint (e.g., remove detached nodes/warp instances) before
persisting/returning; locate and change the functions worldline::from_state /
WorldlineState::replay_checkpoint_clone and ensure the checkpoint field uses the
new full-state hash or the reduced, canonical replay payload so that
authentication covers everything restored.
In `@tests/hooks/test_hook_timing.sh`:
- Around line 86-91: The current subshell runs ./.githooks/pre-rebase in an if
and treats a zero exit as failure, which is fragile if the hook is missing or
non-executable; change the block to run ./.githooks/pre-rebase, capture its exit
code (e.g., rc=$?), assert rc is non-zero (fail the test if rc==0), and also
explicitly check the hook file exists and is executable before running it so
missing/invocation errors are surfaced; reference the subshell that invokes
./.githooks/pre-rebase in the test_hook_timing.sh snippet and replace the
if-based inversion with the explicit rc check and existence/executable checks.
- Around line 46-53: The fixture_root function creates a temporary directory in
variable tmp but does not ensure it is removed on failures; add deterministic
cleanup by registering a trap that removes "$tmp" on EXIT/RETURN (or on ERR)
before returning the temp path, or alternatively push tmp into a global cleanup
list and add a top-level trap in tests/hooks/test_hook_timing.sh to iterate and
rm -rf each entry; update fixture_root (and its tmp variable) to ensure the temp
dir is always deleted on test failure or exit.
---
Outside diff comments:
In `@crates/warp-wasm/src/lib.rs`:
- Around line 10-16: The module-level rustdoc incorrectly claims "All exports
return CBOR-encoded bytes" while helper functions return raw JS values; update
the crate documentation so it only states that the ABI byte exports return
CBOR-encoded envelopes and explicitly list exceptions: get_codec_id,
get_registry_version, and get_schema_sha256_hex (and any similar helpers) which
return raw JsValue strings/values; locate the module docblock in lib.rs and
reword to either narrow the claim to "ABI byte exports" or add a parenthetical
noting these three helpers are exceptions and describe their return types/intent
briefly.
---
Duplicate comments:
In @.githooks/pre-push-parallel:
- Around line 58-71: The signal handler on_signal currently calls cleanup() and
then exits, which re-triggers the on_exit EXIT trap and causes cleanup() to run
twice; fix by disarming the EXIT trap at the start of on_signal (use the shell
trap command to remove the EXIT handler) so that after doing "trap - EXIT" the
handler can call cleanup() and exit without re-invoking on_exit; update
references in the same file to on_signal, on_exit, and cleanup to ensure the
EXIT trap is removed before calling cleanup and exiting.
In `@crates/echo-wasm-abi/src/lib.rs`:
- Around line 130-147: Add ABI-level unit tests for pack_control_intent_v1 and
unpack_control_intent_v1: write a round-trip test that constructs a valid
kernel_port::ControlIntentV1, calls pack_control_intent_v1 then
unpack_control_intent_v1 and asserts equality; write a wrong-op-id test by
taking a valid packed envelope and altering the op id bytes (not equal to
CONTROL_INTENT_V1_OP_ID) and assert unpack_control_intent_v1 returns
Err(EnvelopeError::Malformed); write a malformed-payload test by constructing an
envelope with the correct op id but invalid/malformed CBOR payload bytes and
assert it returns Err(EnvelopeError::Malformed). Use the existing helper
functions pack_control_intent_v1, unpack_control_intent_v1 and the constant
CONTROL_INTENT_V1_OP_ID to locate code under test and assert on
EnvelopeError::Malformed for failure cases.
- Around line 47-48: Prevent callers from packing the reserved control op id by
adding a runtime check in pack_intent_v1 (and any other raw packing helpers)
that rejects or returns an error when the provided op_id equals
CONTROL_INTENT_V1_OP_ID; update pack_intent_v1 to validate the op_id parameter
and return an Err/Result or panic with a clear message directing users to the
typed control helper (the control-intent constructor) instead of allowing
untyped minting of CONTROL_INTENT_V1_OP_ID. Ensure the same guard is added
wherever raw op ids are accepted so the protocol-reserved constant
CONTROL_INTENT_V1_OP_ID cannot be produced via pack_intent_v1 or similar
functions.
In `@crates/warp-core/src/head.rs`:
- Around line 345-350: Add a regression test that exercises
RunnableWriterSet::rebuild() for Dormant/readmit semantics: create a
PlaybackHeadRegistry with a head, ensure it is initially admitted and present
after rebuild(), then transition the head to Dormant (use the API that marks a
head Dormant), call RunnableWriterSet::rebuild(®istry) and assert the key is
removed, then readmit the same head, call rebuild() again and assert the key is
present; place this test alongside the existing pause-state test and use the
same helper setup utilities to locate the head and keys.
In `@crates/warp-core/src/playback.rs`:
- Around line 531-543: The forward-replay branch currently clones the entire
WorldlineState (self.state.clone()) before calling advance_replay_state, causing
O(state_size) work per tick; instead mutate self.state in place when target >=
self.tick by passing &mut self.state to advance_replay_state and only
materialize/clone a replacement for the branch that rebuilds from provenance via
replay_worldline_state_at_from_provenance; update both occurrences that clone
self.state (the forward branch around advance_replay_state and the duplicate at
547-548) to avoid the clone and rely on in-place mutation, preserving the
existing map_replay_error handling for errors.
In `@crates/warp-core/src/provenance_store.rs`:
- Around line 722-723: The code casts WorldlineTick via target_tick.as_u64() as
usize (e.g., building target_len) which truncates on 32-bit targets; replace
these casts with fallible conversions using
usize::try_from(target_tick.as_u64()) and map any Err(Overflow) into the
appropriate HistoryError variant used by this module so provenance validation is
platform-deterministic; apply the same fix to the other occurrences that convert
WorldlineTick to usize (the places around where replayed.tick_history.len() is
compared/indexed and the other listed sites) so all index/length computations
use try_from and propagate HistoryError on overflow.
- Around line 722-753: hydrate_replay_metadata currently only appends a suffix
to replayed.tick_history and leaves any checkpoint-sourced prefix or
last_snapshot in place; before rebuilding from provenance you must clear
existing checkpoint-derived replay metadata: call replayed.tick_history.clear(),
set replayed.last_snapshot = None, replayed.last_materialization.clear(),
replayed.last_materialization_errors.clear(), and replayed.tx_counter = 0 prior
to the loop that pushes replay artifacts (the code around
hydrate_replay_metadata / replayed usage and the block that iterates raw_tick to
target_tick), and apply the same clearing at the other hydrate/replay branch
noted in the review so no artifact-sourced state survives.
In `@crates/warp-wasm/README.md`:
- Around line 15-18: The README line is contradictory about read exports; update
the wording so it consistently describes which functions are public read
exports: change the sentence to state that both observe(...) and
scheduler_status() are public read exports (e.g., "observe(...) and
scheduler_status() are the public read exports"), and keep the rest intact so
all external writes or scheduler control requests flow through
dispatch_intent(...).
In `@crates/warp-wasm/src/warp_kernel.rs`:
- Around line 436-441: The current map_err in the
self.runtime.set_head_eligibility(...) call wraps all runtime errors as
error_codes::ENGINE_ERROR; change the map_err closure to inspect the runtime
error (match RuntimeError::UnknownHead(_)) and return an AbiError with code
error_codes::INVALID_CONTROL for that variant, otherwise retain
error_codes::ENGINE_ERROR, keeping the same e.to_string() as the message; locate
the call to self.runtime.set_head_eligibility, the AbiError construction, and
use RuntimeError::UnknownHead and error_codes::INVALID_CONTROL in the match.
- Around line 351-358: The code assumes ControlIntentV1::Stop can interrupt a
running scheduler by checking scheduler_status.state for
SchedulerState::Running/Stopping (see scheduler_status, SchedulerState,
dispatch_intent()), but in single-threaded WASM Start(UntilIdle) completes
synchronously so Running/Stopping are never observed; fix by choosing one of two
concrete fixes: (A) make runs persistent across calls so the scheduler truly
stays Running/Stopping (e.g., move the loop into a background task/thread and
update scheduler_status transitions accordingly so Stop can interrupt), or (B)
remove/disable the interrupt contract from the ABI by changing
dispatch_intent()/ControlIntentV1::Stop handling to treat Stop as
unsupported/no-op or return an explicit error and remove checks that expect
runtime interruption (update places using SchedulerState::Running |
SchedulerState::Stopping to match the chosen model). Ensure all related branches
(including the blocks around the earlier SchedulerState check and the later
handling of ControlIntentV1::Stop) are updated consistently.
In `@docs/plans/adr-0008-and-0009.md`:
- Around line 755-757: The migration bullet incorrectly references the deleted
adapter `snapshot_at(...)`; update the text to point to the observation-backed
snapshot path instead (replace `snapshot_at(...)` with the canonical
observation-backed snapshot surface name used elsewhere in the ADR), and ensure
Phase 7's description no longer implies the removed adapter exists—also verify
related mentions of `fork(...)`, playback seek/step, replay paths, and
reader-side `GraphStore` replay fallbacks align with Phase 6's removals so the
ABI and determinism guarantees remain consistent.
In `@scripts/pr-status.sh`:
- Around line 62-76: The heredoc that runs python3 to populate PR_NUMBER,
PR_URL, HEAD_SHA, REVIEW_DECISION, MERGE_STATE is unguarded: if python3 fails it
emits nothing and read silently assigns empties; update the script to capture
the python3 output into a variable, check the python3 exit status (or that the
output is non-empty/contains expected fields), and on failure print an error
including $VIEW_JSON context and exit non-zero; locate the block that invokes
"python3 -c '...'" and the read -r PR_NUMBER PR_URL ... EOF and implement this
validation/early-exit so downstream code never proceeds with empty PR_*
variables.
In `@scripts/verify-local.sh`:
- Around line 688-711: The run_timed_step function currently calls the lane
function with errexit disabled (set +e), which allows multi-command lane
functions (e.g., run_full_lane_guards, run_full_lane_tests_runtime,
run_full_lane_tests_warp_core) to continue past a failing command and produce a
misleading rc; to fix, invoke the lane in an inner subshell that sets strict
failure handling (set -euo pipefail) so the first failing command causes the
subshell to exit, capture that subshell's exit code into rc, then continue to
compute timing and call report_lane_timing and append_timing_record as before;
update the invocation inside run_timed_step (and the parallel worker block that
mirrors it) to replace the direct "$step_func" call with a subshell executing
"set -euo pipefail; \"$step_func\" \"$@\"" and assign its exit status to rc.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2be3855-ec87-4b89-9d4a-479c392b73a7
📒 Files selected for processing (53)
.githooks/_timing.sh.githooks/commit-msg.githooks/pre-commit.githooks/pre-push.githooks/pre-push-parallel.githooks/pre-push-sequential.githooks/pre-rebase.gitignoreCHANGELOG.mdMakefilecrates/echo-wasm-abi/src/kernel_port.rscrates/echo-wasm-abi/src/lib.rscrates/ttd-browser/src/lib.rscrates/warp-core/README.mdcrates/warp-core/src/clock.rscrates/warp-core/src/coordinator.rscrates/warp-core/src/head.rscrates/warp-core/src/lib.rscrates/warp-core/src/observation.rscrates/warp-core/src/playback.rscrates/warp-core/src/provenance_store.rscrates/warp-core/src/tick_patch.rscrates/warp-core/src/worldline.rscrates/warp-core/src/worldline_registry.rscrates/warp-core/src/worldline_state.rscrates/warp-core/tests/checkpoint_fork_tests.rscrates/warp-core/tests/common/mod.rscrates/warp-core/tests/golden_vectors_phase0.rscrates/warp-core/tests/inbox.rscrates/warp-core/tests/invariant_property_tests.rscrates/warp-core/tests/outputs_playback_tests.rscrates/warp-core/tests/playback_cursor_tests.rscrates/warp-core/tests/slice_theorem_proof.rscrates/warp-core/tests/view_session_tests.rscrates/warp-wasm/README.mdcrates/warp-wasm/src/lib.rscrates/warp-wasm/src/warp_kernel.rsdocs/ROADMAP/first-light/browser-visualization.mddocs/ROADMAP/first-light/wasm-runtime.mddocs/adr/ADR-0010-observational-seek-and-administrative-rewind.mddocs/adr/ADR-0011-explicit-observation-contract.mddocs/march-16.plan.mddocs/plans/adr-0008-and-0009.mddocs/spec/SPEC-0009-wasm-abi-v1.mddocs/spec/SPEC-0009-wasm-abi-v3.mdscripts/hooks/README.mdscripts/plot-prepush-timing.mjsscripts/pr-status.shscripts/verify-local.shtests/hooks/test_hook_issues.shtests/hooks/test_hook_timing.shtests/hooks/test_pr_status.shtests/hooks/test_verify_local.sh
💤 Files with no reviewable changes (1)
- docs/spec/SPEC-0009-wasm-abi-v1.md
Comment on lines
+5
to
+15
| hook_timing_now_ns() { | ||
| if command -v python3 >/dev/null 2>&1; then | ||
| python3 - <<'PY' | ||
| import time | ||
|
|
||
| print(time.monotonic_ns()) | ||
| PY | ||
| else | ||
| printf '%s000000000\n' "$(date +%s)" | ||
| fi | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fallback mixing could produce invalid deltas if python3 availability changes mid-hook.
If python3 is available at hook_timing_prepare but not at hook_timing_append (or vice versa), you'll subtract epoch-seconds-as-nanoseconds from monotonic nanoseconds, producing garbage. The negative clamp at line 41-43 catches the symptom but masks the cause.
This is local dev tooling and the scenario is unlikely, so low priority.
Possible fix: cache the timing method at prepare time
hook_timing_prepare() {
DX_HOOK_REPO_ROOT="$1"
DX_HOOK_NAME="$2"
+ if command -v python3 >/dev/null 2>&1; then
+ DX_HOOK_TIMING_METHOD="python"
+ else
+ DX_HOOK_TIMING_METHOD="date"
+ fi
DX_HOOK_START_NS="$(hook_timing_now_ns)"
DX_HOOK_TIMING_RECORDED=0
}Then use DX_HOOK_TIMING_METHOD in hook_timing_now_ns to ensure consistent method.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.githooks/_timing.sh around lines 5 - 15, The current hook_timing_now_ns may
use python3 or date depending on availability, which can change between
hook_timing_prepare and hook_timing_append and produce invalid deltas; modify
hook_timing_prepare to detect and export a consistent timing method (e.g. set
DX_HOOK_TIMING_METHOD="monotonic_ns" or "epoch_s_as_ns") and then update
hook_timing_now_ns to read DX_HOOK_TIMING_METHOD and always use the cached
method (calling python3 only if DX_HOOK_TIMING_METHOD indicates monotonic_ns,
otherwise using date), ensuring prepare/append use the same clock source; keep
existing negative-clamp logic but remove reliance on ephemeral command detection
at call time.
Comment on lines
+49
to
+52
| mkdir -p "$csv_dir" 2>/dev/null || return 0 | ||
| if [[ ! -f "$csv_file" ]]; then | ||
| printf 'timestamp_utc,elapsed_ms,exit_code,pid\n' >>"$csv_file" 2>/dev/null || return 0 | ||
| fi |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
CSV header write has a minor race condition.
Two hooks starting simultaneously could both see a missing file and both append headers. For local dev tooling this is cosmetic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.githooks/_timing.sh around lines 49 - 52, The header-write has a race: two
concurrent hook runs can both see no file and append duplicate headers; fix by
obtaining an exclusive lock on "$csv_file" before checking/writing (use a file
descriptor with flock), e.g. open "$csv_file" for append (exec
200>>"$csv_file"), flock 200, then check if the file is empty/has no header
(e.g. [[ ! -s "$csv_file" ]] or grep for the header) and only then write the
header to the locked FD (printf 'timestamp_utc,elapsed_ms,exit_code,pid\n'
>&200), finally release the lock (flock -u) and close the FD; reference
variables: csv_file, csv_dir and the header string
'timestamp_utc,elapsed_ms,exit_code,pid\n'.
Comment on lines
+189
to
+209
| /// Declarative host intent for head admission. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum HeadEligibility { | ||
| /// Head is intentionally excluded from scheduling. | ||
| Dormant, | ||
| /// Head is admitted and may participate if otherwise runnable. | ||
| Admitted, | ||
| } | ||
|
|
||
| /// Runtime truth about a head's scheduler disposition. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum HeadDisposition { | ||
| /// Head is currently runnable by the scheduler. | ||
| Runnable, | ||
| /// Head is admitted but cannot currently run. | ||
| Blocked, | ||
| /// Head has been retired and cannot be reactivated. | ||
| Retired, | ||
| } |
Contributor
There was a problem hiding this comment.
HeadDisposition cannot represent Dormant.
ControlIntentV1::SetHeadEligibility can make a head dormant, and Blocked is explicitly documented as "admitted but cannot currently run". That leaves no legal HeadDisposition for a dormant head. Fix the enum now, before ABI v3 hardens an unrepresentable scheduler state.
Suggested shape
pub enum HeadDisposition {
+ /// Head is intentionally excluded from scheduling.
+ Dormant,
/// Head is currently runnable by the scheduler.
Runnable,
/// Head is admitted but cannot currently run.
Blocked,
/// Head has been retired and cannot be reactivated.
Retired,
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/echo-wasm-abi/src/kernel_port.rs` around lines 189 - 209,
HeadDisposition currently lacks a Dormant variant even though
ControlIntentV1::SetHeadEligibility can set a head to Dormant; add a Dormant
variant to the HeadDisposition enum (preserving #[serde(rename_all =
"snake_case")] and enum attributes) and then update any match arms, conversions,
and uses of HeadDisposition (e.g., functions that map eligibility to disposition
or serialize/deserialize code paths) to handle HeadDisposition::Dormant so the
ABI can represent a dormant scheduler state without breaking serde or pattern
matches.
Comment on lines
+742
to
+754
| if cursor.tick == WorldlineTick::ZERO { | ||
| return Err("cannot commit at tick 0".to_string()); | ||
| } | ||
|
|
||
| let expected = self | ||
| .provenance | ||
| .entry(cursor.worldline_id, cursor.tick - 1) | ||
| .entry( | ||
| cursor.worldline_id, | ||
| cursor | ||
| .tick | ||
| .checked_sub(1) | ||
| .ok_or_else(|| "cannot commit at tick 0".to_string())?, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Explicit tick-0 rejection in commit path.
Both the pre-check at line 742-744 and the checked_sub(1) at line 751-753 guard against committing at tick 0. The double-check is redundant but harmless.
// Generate a Light mode receipt for the current cursor position
if cursor.tick == WorldlineTick::ZERO {
return Err("cannot commit at tick 0".to_string());
}
let expected = self
.provenance
- .entry(
- cursor.worldline_id,
- cursor
- .tick
- .checked_sub(1)
- .ok_or_else(|| "cannot commit at tick 0".to_string())?,
- )
+ .entry(cursor.worldline_id, committed_tick_for_cursor(cursor)?)
.map_err(|e| e.to_string())?;Reuse the helper for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/ttd-browser/src/lib.rs` around lines 742 - 754, Remove the redundant
explicit check against tick-0: drop the initial if cursor.tick ==
WorldlineTick::ZERO { ... } block and instead rely on the single guarded
subtraction used when building the provenance key; use
cursor.tick.checked_sub(1).ok_or_else(|| "cannot commit at tick 0".to_string())?
when calling self.provenance.entry(...) so the zero-check is centralized and
consistent with the existing checked_sub logic.
Comment on lines
+34
to
+50
| /// Adds `rhs`, returning `None` on overflow. | ||
| #[must_use] | ||
| pub fn checked_add(self, rhs: u64) -> Option<Self> { | ||
| self.0.checked_add(rhs).map(Self) | ||
| } | ||
|
|
||
| /// Subtracts `rhs`, returning `None` on underflow. | ||
| #[must_use] | ||
| pub fn checked_sub(self, rhs: u64) -> Option<Self> { | ||
| self.0.checked_sub(rhs).map(Self) | ||
| } | ||
|
|
||
| /// Increments by one, returning `None` on overflow. | ||
| #[must_use] | ||
| pub fn checked_increment(self) -> Option<Self> { | ||
| self.checked_add(1) | ||
| } |
Contributor
There was a problem hiding this comment.
Add direct overflow/underflow tests for the counter helpers.
These methods now define the failure mode for every typed tick/run counter. Add focused tests for ZERO, MAX, checked_add, checked_sub, and checked_increment boundary cases here so a macro edit cannot silently change scheduler/playback arithmetic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/warp-core/src/clock.rs` around lines 34 - 50, Add unit tests that
assert the overflow/underflow behavior of the counter helpers: verify the ZERO
and MAX constants behave as expected, that checked_add(MAX, 1) returns None and
checked_add on a non-edge value returns Some(new_value), that checked_sub(ZERO,
1) returns None and checked_sub on a non-edge value returns Some(new_value), and
that checked_increment on MAX returns None while checked_increment on a non-edge
value returns Some(new_value); target the functions/constants ZERO, MAX,
checked_add, checked_sub, and checked_increment so the tests will fail if a
macro edit changes their arithmetic semantics.
Comment on lines
349
to
+352
| /// Invariant: `tick` always reflects the number of patches that have been applied | ||
| /// to `store` since its initial state. Modifying `tick` without correspondingly | ||
| /// updating `store` (or vice versa) will cause seek/replay hash verification to fail. | ||
| pub tick: u64, | ||
| /// to `state` since its initial state. Modifying `tick` without correspondingly | ||
| /// updating `state` (or vice versa) will cause seek/replay hash verification to fail. | ||
| pub tick: WorldlineTick, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Seal the cursor's materialized state behind accessors.
tick and state now have a hard coupled invariant, but both are still pub. Any downstream mutation can desynchronize replay state and make later seeks or receipts invalid. The new focused_store()/current_state_root() helpers are the right read surface; keep the invariant-bearing fields private.
Also applies to: 366-375, 449-459
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/warp-core/src/playback.rs` around lines 349 - 352, Make the
invariant-bearing fields (e.g., tick: WorldlineTick and the state/store fields
referenced by focused_store() and current_state_root()) private so external code
cannot mutate them directly; expose only safe accessors already implemented
(focused_store(), current_state_root(), and any read-only getters) and update
any callers to use those helpers. Change the visibility of the fields declared
as pub (including the ones in the other referenced ranges) to private, remove
direct external field accesses, and ensure methods that need to update
tick/state are the sole mutators (preserve existing mutating APIs or add
controlled setters on the struct). Verify all usages in the codebase call the
accessors (not the fields) and keep WorldlineTick and state mutations
encapsulated to prevent desynchronization of replay/seek invariants.
Comment on lines
+338
to
+345
| pub fn from_state(state: &WorldlineState) -> Self { | ||
| Self { | ||
| checkpoint: CheckpointRef { | ||
| worldline_tick: state.current_tick(), | ||
| state_hash: state.state_root(), | ||
| }, | ||
| state: state.replay_checkpoint_clone(), | ||
| } |
Contributor
There was a problem hiding this comment.
state_root() is too weak to authenticate a full replay checkpoint.
ReplayCheckpoint stores and restore validates only state.state_root(), but this path returns the entire WorldlineState. In this codebase state_root() is reachable-only, so a checkpoint can carry extra detached nodes / warp instances, still satisfy the boundary hash, and then leak non-provenance state back out on an exact-checkpoint restore. Either commit a deterministic full-state hash or strip checkpoint payloads down to the authoritative subset before persisting/restoring them.
Also applies to: 792-812
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/warp-core/src/provenance_store.rs` around lines 338 - 345, The
from_state constructor for CheckpointRef/ReplayCheckpoint returns the full
WorldlineState while authenticating only state_root(), which allows extra
detached nodes/warp instances to be carried and later leak; update from_state
(and the analogous code at the 792-812 region) to either compute and store a
deterministic full-state hash (covering the entire replay checkpoint contents)
instead of using state_root(), or strip the returned payload to the
authoritative subset used by ReplayCheckpoint (e.g., remove detached nodes/warp
instances) before persisting/returning; locate and change the functions
worldline::from_state / WorldlineState::replay_checkpoint_clone and ensure the
checkpoint field uses the new full-state hash or the reduced, canonical replay
payload so that authentication covers everything restored.
Comment on lines
+46
to
+53
| fixture_root() { | ||
| local tmp | ||
| tmp="$(mktemp -d)" | ||
| mkdir -p "$tmp/.githooks" "$tmp/scripts" "$tmp/bin" | ||
| cp .githooks/_timing.sh "$tmp/.githooks/_timing.sh" | ||
| chmod +x "$tmp/.githooks/_timing.sh" | ||
| printf '%s\n' "$tmp" | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Temp directories leak on test failure.
If any assertion fails or the script exits early due to set -e, temp directories created by fixture_root are not cleaned up. Consider adding a trap or restructuring cleanup.
+CLEANUP_DIRS=()
+cleanup() {
+ for d in "${CLEANUP_DIRS[@]}"; do
+ rm -rf "$d" 2>/dev/null || true
+ done
+}
+trap cleanup EXIT
+
fixture_root() {
local tmp
tmp="$(mktemp -d)"
+ CLEANUP_DIRS+=("$tmp")
mkdir -p "$tmp/.githooks" "$tmp/scripts" "$tmp/bin"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hooks/test_hook_timing.sh` around lines 46 - 53, The fixture_root
function creates a temporary directory in variable tmp but does not ensure it is
removed on failures; add deterministic cleanup by registering a trap that
removes "$tmp" on EXIT/RETURN (or on ERR) before returning the temp path, or
alternatively push tmp into a global cleanup list and add a top-level trap in
tests/hooks/test_hook_timing.sh to iterate and rm -rf each entry; update
fixture_root (and its tmp variable) to ensure the temp dir is always deleted on
test failure or exit.
Comment on lines
+86
to
+91
| ( | ||
| cd "$tmp" | ||
| if ./.githooks/pre-rebase; then | ||
| exit 1 | ||
| fi | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inverted exit check for pre-rebase is correct but fragile.
The test expects pre-rebase to fail (exit non-zero). The current pattern works but can silently pass if the hook is missing or non-executable. Consider an explicit exit-code check:
(
cd "$tmp"
- if ./.githooks/pre-rebase; then
- exit 1
- fi
+ ./.githooks/pre-rebase && exit 1 || true
)Or capture the exit code explicitly for clearer intent.
📝 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
| ( | |
| cd "$tmp" | |
| if ./.githooks/pre-rebase; then | |
| exit 1 | |
| fi | |
| ) | |
| ( | |
| cd "$tmp" | |
| ./.githooks/pre-rebase && exit 1 || true | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/hooks/test_hook_timing.sh` around lines 86 - 91, The current subshell
runs ./.githooks/pre-rebase in an if and treats a zero exit as failure, which is
fragile if the hook is missing or non-executable; change the block to run
./.githooks/pre-rebase, capture its exit code (e.g., rc=$?), assert rc is
non-zero (fail the test if rc==0), and also explicitly check the hook file
exists and is executable before running it so missing/invocation errors are
surfaced; reference the subshell that invokes ./.githooks/pre-rebase in the
test_hook_timing.sh snippet and replace the if-based inversion with the explicit
rc check and existence/executable checks.
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
WorldlineStatefrom provenance instead of keeping any reader-side store-only replay pathPhases 0-7 implementedWhat changed
warp-corereplay now rebuilds portal and warp-instance history through fullWorldlineStatereplayVerification
cargo test -p warp-core --message-format shortbash tests/hooks/test_verify_local.shfmt,guards,hook-tests,clippy-core,tests-runtime,tests-warp-core,rustdocSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests