Phase 7: full WorldlineState replay and checkpoint-backed playback#304
Open
flyingrobots wants to merge 17 commits intomainfrom
Open
Phase 7: full WorldlineState replay and checkpoint-backed playback#304flyingrobots wants to merge 17 commits intomainfrom
flyingrobots wants to merge 17 commits intomainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (53)
📝 WalkthroughWalkthroughThis PR introduces hook timing instrumentation and a new pr-status helper, migrates runtime tick concepts to typed logical counters (WorldlineTick/GlobalTick/RunId), reworks provenance/replay/checkpoint APIs and playback to use WorldlineState, and bumps the WASM ABI to v3 (removing legacy read exports and adding control-intent handling and scheduler_status), with extensive test/docs/tooling updates. 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: inspect/update heads, run super_tick loops
Runtime-->>Scheduler: GlobalTick / state updates
Scheduler-->>Kernel: SchedulerStatus
Kernel-->>Host: DispatchResponse { scheduler_status, accepted }
else domain intent
Intent-->>Kernel: Domain intent
Kernel->>Runtime: ingest intent -> IngressTarget
Runtime-->>Kernel: IngressDisposition
Kernel-->>Host: DispatchResponse { disposition, scheduler_status }
end
sequenceDiagram
actor Client as User
participant Cursor as PlaybackCursor
participant Prov as ProvenanceStore
participant Replay as Replay Engine
participant State as WorldlineState
Client->>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-->>Client: CursorReceipt { worldline_tick, commit_global_tick, state_root }
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ 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
346
to
350
| pub fn rebuild(&mut self, registry: &PlaybackHeadRegistry) { | ||
| self.keys.clear(); | ||
| for (key, head) in registry.iter() { | ||
| if !head.is_paused() { | ||
| if head.is_admitted() && !head.is_paused() { | ||
| self.keys.push(*key); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a regression for the new admission gate.
rebuild() now depends on is_admitted() as well as pause state, but the tests still only exercise pause-based exclusion. Add one dormant/readmit roundtrip so scheduler admission cannot regress independently of pause handling.
🧰 Tools
🪛 GitHub Actions: CI
[error] Command failed: cargo clippy --all-targets -- -D warnings -D missing_docs. could not compile warp-core (test "slice_theorem_proof") due to 2 previous errors. Process exited with code 101.
🤖 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, 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().
Comment on lines
+531
to
+543
| 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 |
Contributor
There was a problem hiding this comment.
Reader playback now clones the full WorldlineState on every forward tick.
Both Play and StepForward route normal reader advancement through this branch, so applying one recorded patch clones the entire materialized worldline first. With full-state replay this is O(state_size) extra work per step and undermines the checkpoint optimization. If replay failures are allowed to invalidate the cursor, mutate in place; otherwise move the transactional buffering below whole-state clones.
As per coding guidelines, warp-core is the deterministic kernel. Performance matters — flag unnecessary allocations in hot paths.
Also applies to: 589-605, 620-629
🧰 Tools
🪛 GitHub Actions: CI
[error] Command failed: cargo clippy --all-targets -- -D warnings -D missing_docs. could not compile warp-core (test "slice_theorem_proof") due to 2 previous errors. Process exited with code 101.
🤖 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 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.
Comment on lines
1062
to
1065
| let stored = worldlines | ||
| .get(&parent.worldline_id) | ||
| .and_then(|history| history.entries.get(parent.worldline_tick as usize)) | ||
| .and_then(|history| history.entries.get(parent.worldline_tick.as_u64() as usize)) | ||
| .ok_or(HistoryError::MissingParentRef { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the file structure and get line counts
wc -l crates/warp-core/src/provenance_store.rsRepository: flyingrobots/echo
Length of output: 106
🏁 Script executed:
# Read the code around lines 1062-1065
sed -n '1055,1075p' crates/warp-core/src/provenance_store.rsRepository: flyingrobots/echo
Length of output: 967
🏁 Script executed:
# Read the code around lines 1305-1320 to find line 1311
sed -n '1305,1320p' crates/warp-core/src/provenance_store.rsRepository: flyingrobots/echo
Length of output: 531
🏁 Script executed:
# Find the type definition of worldline_tick and related structures
rg -A 5 "pub.*worldline_tick" crates/warp-core/src/Repository: flyingrobots/echo
Length of output: 4262
🏁 Script executed:
# Search for other occurrences of similar casts in provenance_store.rs
rg "as_u64\(\) as usize" crates/warp-core/src/provenance_store.rs -nRepository: flyingrobots/echo
Length of output: 302
🏁 Script executed:
# Find WorldlineTick type definition
rg -A 3 "type WorldlineTick|struct WorldlineTick" crates/warp-core/src/Repository: flyingrobots/echo
Length of output: 676
🏁 Script executed:
# Look at the full context around line 722 to understand constraints
sed -n '715,730p' crates/warp-core/src/provenance_store.rsRepository: flyingrobots/echo
Length of output: 721
🏁 Script executed:
# Look at full context around line 1208
sed -n '1200,1215p' crates/warp-core/src/provenance_store.rsRepository: flyingrobots/echo
Length of output: 776
🏁 Script executed:
# Check if there's any validation or bounding logic for ticks in the registry
rg -B 5 -A 5 "worldline_tick" crates/warp-core/src/provenance_store.rs | head -60Repository: flyingrobots/echo
Length of output: 2064
🏁 Script executed:
# Find the actual WorldlineTick type definition
fd -e rs . crates/warp-core/src | xargs rg "^pub.*type WorldlineTick|^pub.*struct WorldlineTick|^pub.*newtype WorldlineTick"Repository: flyingrobots/echo
Length of output: 197
🏁 Script executed:
# Search for WorldlineTick definition more broadly
rg "WorldlineTick" crates/warp-core/src/ | grep -E "type|struct|newtype|=|from_raw" | head -20Repository: flyingrobots/echo
Length of output: 1846
🏁 Script executed:
# Look at head.rs and coordinator.rs for WorldlineTick definition
cat crates/warp-core/src/head.rs | grep -A 5 "WorldlineTick"Repository: flyingrobots/echo
Length of output: 43
🏁 Script executed:
# Check the tick type module
fd -e rs tick crates/warp-core/src/Repository: flyingrobots/echo
Length of output: 130
🏁 Script executed:
# Search all Rust files for WorldlineTick definition
rg -A 2 "pub.*WorldlineTick" crates/warp-core/src/*.rs | grep -E "^[^:]*:(pub|type|struct)" | head -30Repository: flyingrobots/echo
Length of output: 276
🏁 Script executed:
# Look at the WorldlineTick definition in clock.rs
cat crates/warp-core/src/clock.rs | head -200Repository: flyingrobots/echo
Length of output: 2441
🏁 Script executed:
# Search specifically for WorldlineTick definition
rg -A 10 "pub.*struct WorldlineTick|pub.*type WorldlineTick" crates/warp-core/src/clock.rsRepository: flyingrobots/echo
Length of output: 43
🏁 Script executed:
# Check if there are any bounds on tick values or entries vector size
rg -B 3 -A 3 "entries\[|entries\.get" crates/warp-core/src/provenance_store.rs | head -40Repository: flyingrobots/echo
Length of output: 1383
🏁 Script executed:
# Check for any overflow protection or bounds checks around the problematic lines
sed -n '1058,1070p' crates/warp-core/src/provenance_store.rsRepository: flyingrobots/echo
Length of output: 643
🏁 Script executed:
# Check the full context of the entry() method at line 1311
sed -n '1308,1320p' crates/warp-core/src/provenance_store.rsRepository: flyingrobots/echo
Length of output: 474
🏁 Script executed:
# Search for any constraints or documentation about max tick values
rg -i "max.*tick|bound.*tick|tick.*limit" crates/warp-core/src/provenance_store.rsRepository: flyingrobots/echo
Length of output: 133
🏁 Script executed:
# Check if there's a usize bounds check anywhere in the validation path
rg -B 5 "MissingParentRef" crates/warp-core/src/provenance_store.rs | head -30Repository: flyingrobots/echo
Length of output: 758
Fix tick-to-index cast overflow on 32-bit targets; affects provenance validation determinism.
parent.worldline_tick.as_u64() as usize truncates on wasm32/other 32-bit platforms where usize < u64. A tick above 2^32-1 will alias to a lower entry; if the commit hash matches, invalid provenance becomes authoritative on 32-bit builds and fails on 64-bit. This violates determinism in the kernel.
Replace with usize::try_from(tick.as_u64()) at all four locations:
- Line 722 (hydrate_replay_metadata
target_len) - Line 1064 (parent validation in append_local_commit)
- Line 1208 (fork_at
end_idx) - Line 1311 (entry() method)
Surface conversion failure as HistoryError::HistoryUnavailable or MissingParentRef depending on context.
🧰 Tools
🪛 GitHub Actions: CI
[error] Command failed: cargo clippy --all-targets -- -D warnings -D missing_docs. could not compile warp-core (test "slice_theorem_proof") due to 2 previous errors. Process exited with code 101.
🤖 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 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.
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.
Comment on lines
+21
to
+78
| run_with_fake_gh() { | ||
| local fixture="$1" | ||
| local tmp | ||
| local repo_root | ||
| repo_root="$(pwd)" | ||
| tmp="$(mktemp -d)" | ||
| mkdir -p "$tmp/bin" | ||
|
|
||
| case "$fixture" in | ||
| success) | ||
| cat >"$tmp/bin/gh" <<'EOF' | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| if [[ "${1:-}" == "pr" && "${2:-}" == "view" ]]; then | ||
| cat <<'JSON' | ||
| {"number":302,"url":"https://github.com/flyingrobots/echo/pull/302","headRefOid":"123456789abcdeffedcba9876543210abcdef123","reviewDecision":"APPROVED","mergeStateStatus":"CLEAN"} | ||
| JSON | ||
| exit 0 | ||
| fi | ||
| if [[ "${1:-}" == "pr" && "${2:-}" == "checks" ]]; then | ||
| cat <<'JSON' | ||
| [{"name":"Tests","bucket":"pass","state":"SUCCESS"},{"name":"Clippy","bucket":"pending","state":"PENDING"},{"name":"Determinism Guards","bucket":"fail","state":"FAILURE"}] | ||
| JSON | ||
| exit 0 | ||
| fi | ||
| if [[ "${1:-}" == "api" && "${2:-}" == "graphql" ]]; then | ||
| cat <<'JSON' | ||
| {"data":{"repository":{"pullRequest":{"reviewThreads":{"nodes":[{"isResolved":true},{"isResolved":false},{"isResolved":false}]}}}}} | ||
| JSON | ||
| exit 0 | ||
| fi | ||
| echo "unexpected gh invocation: $*" >&2 | ||
| exit 1 | ||
| EOF | ||
| ;; | ||
| auth-error) | ||
| cat >"$tmp/bin/gh" <<'EOF' | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| echo "gh: authentication required" >&2 | ||
| exit 1 | ||
| EOF | ||
| ;; | ||
| *) | ||
| echo "unknown fixture: $fixture" >&2 | ||
| rm -rf "$tmp" | ||
| exit 1 | ||
| ;; | ||
| esac | ||
|
|
||
| chmod +x "$tmp/bin/gh" | ||
| ( | ||
| cd "$repo_root" | ||
| PATH="$tmp/bin:$PATH" ./scripts/pr-status.sh 302 2>&1 | ||
| ) | ||
|
|
||
| rm -rf "$tmp" | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Temp directory may leak on failure.
If the subshell exits non-zero before reaching rm -rf "$tmp", the temp directory persists. Consider a trap for robust cleanup:
tmp="$(mktemp -d)"
+ trap 'rm -rf "$tmp"' EXIT
mkdir -p "$tmp/bin"Then remove the explicit rm -rf "$tmp" at the end.
🤖 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 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.
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
Comment on lines
+366
to
+373
| append_fixture_entry( | ||
| &mut provenance, | ||
| worldline_id, | ||
| patch, | ||
| triplet, | ||
| vec![(checkpoint_output_channel, checkpoint_output_bytes.clone())], | ||
| ) | ||
| .expect("append should succeed"); |
Contributor
There was a problem hiding this comment.
Remove redundant clone — CI is failing on this.
checkpoint_output_bytes is not used after this point; the clone is unnecessary.
Fix
append_fixture_entry(
&mut provenance,
worldline_id,
patch,
triplet,
- vec![(checkpoint_output_channel, checkpoint_output_bytes.clone())],
+ vec![(checkpoint_output_channel, checkpoint_output_bytes)],
)
.expect("append should succeed");📝 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
| append_fixture_entry( | |
| &mut provenance, | |
| worldline_id, | |
| patch, | |
| triplet, | |
| vec![(checkpoint_output_channel, checkpoint_output_bytes.clone())], | |
| ) | |
| .expect("append should succeed"); | |
| append_fixture_entry( | |
| &mut provenance, | |
| worldline_id, | |
| patch, | |
| triplet, | |
| vec![(checkpoint_output_channel, checkpoint_output_bytes)], | |
| ) | |
| .expect("append should succeed"); |
🧰 Tools
🪛 GitHub Actions: CI
[error] 371-371: clippy failed: redundant clone. vec![(checkpoint_output_channel, checkpoint_output_bytes.clone())]. Help: remove this; lint: clippy::redundant_clone (implied by -D clippy::nursery).
🤖 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 - 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.
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
+68
| const preferredModes = ['full', 'pre-push', 'pr', 'fast', 'ultra-fast']; | ||
| const modeNames = preferredModes.filter(mode => runRecords.some(d => d.mode === mode)); | ||
| for (const mode of modeNames) { | ||
| const records = runRecords.filter(d => d.mode === mode); | ||
| series.push(records.map(d => d.elapsed_seconds)); | ||
| legends.push({ | ||
| label: mode, | ||
| color: ['🔴', '🟢', '🔵', '🟡', '🟣'][legends.length] || '⚪', | ||
| stats: records.filter(d => d.exit_status === 0).map(d => d.elapsed_seconds), | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Do not drop valid timing modes outside the preferred list.
preferredModes.filter(...) only keeps five hard-coded names. If the JSONL contains a new mode, the script silently ignores those runs and can fall through to “No timing data found” even with valid data present. Build modeNames from the observed modes, and use preferredModes only to order known ones.
Minimal fix
- const preferredModes = ['full', 'pre-push', 'pr', 'fast', 'ultra-fast'];
- const modeNames = preferredModes.filter(mode => runRecords.some(d => d.mode === mode));
+ const preferredModes = ['full', 'pre-push', 'pr', 'fast', 'ultra-fast'];
+ const seenModes = [...new Set(runRecords.map(d => d.mode).filter(Boolean))];
+ const modeNames = [
+ ...preferredModes.filter(mode => seenModes.includes(mode)),
+ ...seenModes.filter(mode => !preferredModes.includes(mode)).sort(),
+ ];📝 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
| const preferredModes = ['full', 'pre-push', 'pr', 'fast', 'ultra-fast']; | |
| const modeNames = preferredModes.filter(mode => runRecords.some(d => d.mode === mode)); | |
| for (const mode of modeNames) { | |
| const records = runRecords.filter(d => d.mode === mode); | |
| series.push(records.map(d => d.elapsed_seconds)); | |
| legends.push({ | |
| label: mode, | |
| color: ['🔴', '🟢', '🔵', '🟡', '🟣'][legends.length] || '⚪', | |
| stats: records.filter(d => d.exit_status === 0).map(d => d.elapsed_seconds), | |
| }); | |
| } | |
| const preferredModes = ['full', 'pre-push', 'pr', 'fast', 'ultra-fast']; | |
| const seenModes = [...new Set(runRecords.map(d => d.mode).filter(Boolean))]; | |
| const modeNames = [ | |
| ...preferredModes.filter(mode => seenModes.includes(mode)), | |
| ...seenModes.filter(mode => !preferredModes.includes(mode)).sort(), | |
| ]; | |
| for (const mode of modeNames) { | |
| const records = runRecords.filter(d => d.mode === mode); | |
| series.push(records.map(d => d.elapsed_seconds)); | |
| legends.push({ | |
| label: mode, | |
| color: ['🔴', '🟢', '🔵', '🟡', '🟣'][legends.length] || '⚪', | |
| stats: records.filter(d => d.exit_status === 0).map(d => d.elapsed_seconds), | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/plot-prepush-timing.mjs` around lines 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.
Comment on lines
+78
to
+89
| if ! THREADS_JSON="$(gh_run gh api graphql -F number="$PR_NUMBER" -f query='query($number:Int!) { repository(owner:"flyingrobots", name:"echo") { pullRequest(number:$number) { reviewThreads(first:100) { nodes { isResolved } } } } }')"; then | ||
| exit 1 | ||
| fi | ||
| UNRESOLVED_THREADS="$( | ||
| THREADS_JSON="$THREADS_JSON" python3 -c ' | ||
| import json | ||
| import os | ||
|
|
||
| data = json.loads(os.environ["THREADS_JSON"]) | ||
| threads = data["data"]["repository"]["pullRequest"]["reviewThreads"]["nodes"] | ||
| print(sum(1 for thread in threads if not thread["isResolved"])) | ||
| ')" |
Contributor
There was a problem hiding this comment.
Review-thread counts are truncated at 100.
reviewThreads(first:100) undercounts large PRs, which is exactly when this helper is most useful. If there are more than 100 threads, the current output silently reports a smaller unresolved-thread total. Paginate the connection or at least surface truncation when hasNextPage is true.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 78-78: Expressions don't expand in single quotes, use double quotes for that.
(SC2016)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/pr-status.sh` around lines 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.
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.
Comment on lines
+114
to
+121
| if printf '%s\n' "$status_output" | grep -q '^- Tests$' && \ | ||
| printf '%s\n' "$status_output" | grep -q '^- Clippy$' && \ | ||
| printf '%s\n' "$status_output" | grep -q '^- Determinism Guards$'; then | ||
| pass "pr-status groups checks by bucket" | ||
| else | ||
| fail "pr-status should group checks by bucket" | ||
| printf '%s\n' "$status_output" | ||
| fi |
Contributor
There was a problem hiding this comment.
This assertion does not test bucket grouping.
Lines 114-116 only prove that Tests, Clippy, and Determinism Guards appear somewhere in the output. The test still passes if scripts/pr-status.sh drops bucket headings or prints every check in one section. Assert the heading→item structure for pass/pending/fail instead of item presence alone.
🤖 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 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.
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