feat(warp-core): Phase 4 provenance closeout and Phase 5 observation contract#302
Merged
flyingrobots merged 18 commits intomainfrom Mar 17, 2026
Merged
feat(warp-core): Phase 4 provenance closeout and Phase 5 observation contract#302flyingrobots merged 18 commits intomainfrom
flyingrobots merged 18 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an explicit, read-only observation contract and ABI Changes
Sequence Diagram(s)sequenceDiagram
participant Client as "Host / Client"
participant Kernel as "WarpKernel (WASM ABI)"
participant Observation as "ObservationService"
participant Provenance as "ProvenanceService"
participant Runtime as "WorldlineRuntime / Engine"
Client->>Kernel: observe(request_bytes)
Kernel->>Observation: to_core_request(decoded)
Observation->>Provenance: validate worldline, resolve coordinate
Provenance-->>Observation: entry metadata / parents
Observation->>Runtime: read state/head for resolved tick
Runtime-->>Observation: head/snapshot/truth payload
Observation->>Observation: compute artifact_hash (CBOR + blake3)
Observation-->>Kernel: ObservationArtifact (encoded)
Kernel-->>Client: return encoded result
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcb5f65678
ℹ️ 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/ttd-browser/src/lib.rs (1)
482-490:⚠️ Potential issue | 🟠 MajorCompute the real emissions digest from recorded outputs here.
This change now has access to
entry.outputs, but it immediately discards them and keeps returningpatch_digestas a proxy. That makesget_emissions_digest()disagree with the digest produced incommit_inner()and breaks any caller treating this as the actual emissions hash.Suggested fix
- let expected = self - .provenance - .entry(cursor.worldline_id, cursor.tick - 1) - .map(|entry| entry.expected) - .map_err(|e| JsError::new(&e.to_string()))?; - - // Return patch_digest as a proxy; actual emissions_digest would need - // to be computed from the MBUS outputs stored in provenance. - Ok(hash_to_uint8array(&expected.patch_digest)) + let entry = self + .provenance + .entry(cursor.worldline_id, cursor.tick - 1) + .map_err(|e| JsError::new(&e.to_string()))?; + + let finalized_channels: Vec<FinalizedChannel> = entry + .outputs + .into_iter() + .map(|(channel, data)| FinalizedChannel { channel, data }) + .collect(); + let emissions_digest = compute_emissions_digest(&finalized_channels); + Ok(hash_to_uint8array(&emissions_digest))🤖 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 482 - 490, The current code fetches provenance.entry(...) and then returns expected.patch_digest as a proxy; instead compute the real emissions digest from the recorded MBUS outputs on that provenance entry (use the same serialization + hashing logic used in commit_inner) instead of using expected.patch_digest. Locate the call that maps the entry (provenance.entry(cursor.worldline_id, cursor.tick - 1)) and, using the entry.outputs field, reproduce the emissions hash computation from commit_inner (serialize the outputs deterministically and run the same hash function), then return hash_to_uint8array(&emissions_digest) so get_emissions_digest() matches commit_inner’s digest.
🤖 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/echo-wasm-abi/src/kernel_port.rs`:
- Around line 51-68: The numeric value for INVALID_TICK changed (was 4, now 9)
and LEGACY_INVALID_TICK now uses 4 which can break ABI consumers; either restore
INVALID_TICK to its original value or treat this as an intentional ABI change by
incrementing the ABI version and adding migration notes. Concretely, update the
constants in kernel_port.rs so INVALID_TICK has the previous code (4) and move
legacy naming elsewhere, or if you intend the renumbering, bump the ABI_VERSION
constant and add a clear migration note in the release/migration docs and notify
JS callers to handle LEGACY_INVALID_TICK vs INVALID_TICK mappings; reference the
symbols INVALID_TICK, LEGACY_INVALID_TICK and ABI_VERSION when making the
change.
In `@crates/ttd-browser/src/lib.rs`:
- Around line 1375-1391: The test currently constructs a ProvenanceEntry via
ProvenanceEntry::local_commit and appends it with
engine.provenance.append_local_commit(entry) but never asserts the
emissions_digest; decode the resulting receipt bytes into a TTDR frame (parse
the receipt/TTDR header produced by commit_inner), compute the expected digest
with compute_emissions_digest(outputs) and add an assert that
header.emissions_digest == compute_emissions_digest(&outputs); ensure you locate
this check immediately after the append_local_commit call so it fails if
commit_inner writes an all-zero or stale digest.
In `@crates/warp-core/src/coordinator.rs`:
- Around line 353-355: The current SuperTick hot path clones the entire
provenance store (provenance.clone()) into provenance_before (and similarly
clones runtime into runtime_before), causing heavy allocations; change rollback
to take a cheap checkpoint (e.g., record a provenance version/length or call a
lightweight provenance.checkpoint() API) before work and on rollback truncate or
restore to that checkpoint (e.g., provenance.truncate_to(version)) instead of
cloning the whole store, and avoid cloning runtime by snapshotting only the
minimal mutable fields needed for rollback or exposing a lightweight
runtime.checkpoint()/restore() operation; update the SuperTick rollback logic
that currently references provenance_before and runtime_before to use these
checkpoint/truncate APIs.
In `@crates/warp-core/src/observation.rs`:
- Around line 92-121: The match arm with unreachable!() in
ObservationProjectionKind::to_abi assumes the caller always provides a matching
ObservationProjection for the kind; make that invariant explicit and provide a
clearer failure mode: add a short doc comment on the to_abi method stating it is
a programmer-only helper that requires matching variants, then replace
unreachable!() with a debug_assert!(matches!(projection, <expected pattern>))
(or a panic! with a descriptive message) so failures produce a clear diagnostic;
reference the ObservationProjectionKind::to_abi function and the unreachable!()
branch when making the change.
In `@crates/warp-core/src/playback.rs`:
- Around line 457-469: The code currently calls
provenance.parents(self.worldline_id, patch_tick) after already fetching entry
via provenance.entry(...), causing a redundant lookup; replace that second call
by using the parents field on the retrieved entry (the local variable entry)
instead of calling provenance.parents again, e.g. extract commit hashes from
entry.parents (or entry.parents.clone() if ownership is needed) with
.into_iter().map(|parent| parent.commit_hash).collect() so you avoid the
duplicate provenance access while preserving the same error handling and result
shape.
In `@crates/warp-core/src/provenance_store.rs`:
- Around line 975-988: The code currently returns
BtrError::OutputBoundaryHashMismatch when a stored entry does not equal the
incoming entry in the loop over record.payload.entries; change this to a
semantically correct error by adding a new enum variant (e.g.
BtrError::PayloadEntryMismatch { tick: u64 }) and return that from the mismatch
branch in the function that iterates record.payload.entries and calls
self.store.entry; update the BtrError enum (and any match sites) to include the
new PayloadEntryMismatch variant and populate it with the offending
entry/worldline_tick (or include expected/got fields if desired) instead of
using BtrError::OutputBoundaryHashMismatch.
In `@crates/warp-core/tests/inbox.rs`:
- Around line 65-73: The helper mirrored_provenance() only re-registers
worldlines via ProvenanceService::register_worldline and therefore misleads by
its name; either rename it to something like register_worldlines_only or extend
it to truly mirror committed provenance by iterating the runtime's provenance
(e.g., WorldlineRuntime::provenance or its entries API) and copying committed
entries, parent relationships, and any recorded outputs into the new
ProvenanceService (using the appropriate ProvenanceService methods for
registering entries, parents, and outputs) so callers receive a faithful mirror
of provenance state.
In `@docs/plans/adr-0008-and-0009.md`:
- Around line 10-12: The "Companion ADRs in this change set" header lists
ADR-0011 but the "Recommended File Layout" section only references ADR-0010 and
adr-exceptions.md, creating an internal inconsistency; update the plan so the
two places match by either adding
`docs/adr/ADR-0011-explicit-observation-contract.md` to the Recommended File
Layout or removing ADR-0011 from the companion ADR list, and ensure the
"Recommended File Layout" and the header (and any mentions of
`ADR-0010-observational-seek-and-administrative-rewind.md`,
`ADR-0011-explicit-observation-contract.md`, and `adr-exceptions.md`) are
consistent and deterministic in ordering.
- Around line 614-615: The docs currently document a non-existent free function
signature `pub fn observe(request: ObservationRequest) ->
Result<ObservationArtifact, ObservationError>`; update the documentation to
document the actual entrypoint `ObservationService::observe(&runtime,
&provenance, &engine, request)` (i.e., the method on ObservationService that
accepts runtime, provenance, engine, and the ObservationRequest and returns
Result<ObservationArtifact, ObservationError>), and adjust examples and any
cross-references to use `ObservationService::observe` and its correct parameter
order and types so readers are pointed at the real public API.
In `@docs/spec/SPEC-0009-wasm-abi-v1.md`:
- Around line 235-238: The “Not Yet Implemented” section currently lumps
endpoints under error code 5 (NOT_SUPPORTED) but `execute_query` is specified
elsewhere to return 11 (UNSUPPORTED_QUERY); update this section to be
endpoint-specific: change the `execute_query` entry to document error code 11
(UNSUPPORTED_QUERY) and keep `render_snapshot` (and any other endpoints here) as
error code 5 (NOT_SUPPORTED), ensuring the docs reference the exact error symbol
names (`execute_query`, `render_snapshot`, error code 5/NOT_SUPPORTED, error
code 11/UNSUPPORTED_QUERY) so the contract is unambiguous.
---
Outside diff comments:
In `@crates/ttd-browser/src/lib.rs`:
- Around line 482-490: The current code fetches provenance.entry(...) and then
returns expected.patch_digest as a proxy; instead compute the real emissions
digest from the recorded MBUS outputs on that provenance entry (use the same
serialization + hashing logic used in commit_inner) instead of using
expected.patch_digest. Locate the call that maps the entry
(provenance.entry(cursor.worldline_id, cursor.tick - 1)) and, using the
entry.outputs field, reproduce the emissions hash computation from commit_inner
(serialize the outputs deterministically and run the same hash function), then
return hash_to_uint8array(&emissions_digest) so get_emissions_digest() matches
commit_inner’s digest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 20b1730b-4658-43a8-b51c-5d032fd86197
📒 Files selected for processing (28)
CHANGELOG.mdcrates/echo-wasm-abi/src/kernel_port.rscrates/ttd-browser/src/lib.rscrates/warp-core/Cargo.tomlcrates/warp-core/README.mdcrates/warp-core/src/coordinator.rscrates/warp-core/src/lib.rscrates/warp-core/src/materialization/reduce_op.rscrates/warp-core/src/observation.rscrates/warp-core/src/playback.rscrates/warp-core/src/provenance_store.rscrates/warp-core/src/snapshot.rscrates/warp-core/src/worldline.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-wasm/src/lib.rscrates/warp-wasm/src/warp_kernel.rsdocs/adr/ADR-0011-explicit-observation-contract.mddocs/plans/adr-0008-and-0009.mddocs/spec/SPEC-0004-worldlines-playback-truthbus.mddocs/spec/SPEC-0005-provenance-payload.mddocs/spec/SPEC-0009-wasm-abi-v1.md
Owner
Author
|
@coderabbitai review update: fixed the Phase 4/5 provenance integrity issues in Included in this commit:
Local verification passed for the touched surface:
I am following up with a separate local-dev throughput pass next, focused on faster multi-lane verification for iteration speed. |
Contributor
🧩 Analysis chain🏁 Script executed: Length of output: 29 🏁 Script executed: Length of output: 213 🏁 Script executed: Length of output: 1025 🏁 Script executed: Length of output: 977 🏁 Script executed: Length of output: 732
Parent ref resolution + commit hash validation — requiring parent refs to resolve against stored history and match the referenced commit hash exactly is the correct level of strictness here. A ref that resolves to a different hash is a silent lie in the provenance chain; good catch.
ADR-0011 status alignment — should be a no-brainer, but mismatched ADR statuses are a real maintenance hazard. Good to close it out. Push
🧠 Learnings used✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai local-dev throughput update in This change makes local verification explicitly lane-based instead of one giant serialized cargo pass:
First real push after the change completed the local full gate in Branch head: |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 999 🏁 Script executed: Length of output: 12093 🏁 Script executed: Length of output: 8489
1. run_full_lane_clippy_core() {
lane_cargo "full-clippy-core" clippy "${args[@]}" --lib -- -D warnings -D missing_docs
}
2. No SIGINT/SIGTERM trap —
logdir="$(mktemp -d "${TMPDIR:-/tmp}/verify-local-${suite}.XXXXXX")"
# ... background pids, wait loop ...
rm -rf "$logdir" # only reached on clean exitIf the developer hits Ctrl+C during a parallel run, the temp dir survives under trap 'rm -rf "$logdir"' EXITThat fires on EXIT regardless of signal. The explicit 3. Failed lane with no output produces a silent tombstone if [[ ! -s "$logfile" ]]; then
continue
fiA lane that exits non-zero without printing anything (e.g., 4. should_run_parallel_lanes() {
[[ "$VERIFY_LANE_MODE" == "parallel" ]]
}Any value other than What's solid:
Items 2 and 4 are cheap fixes. Item 1 is the intentional trade-off worth being explicit about in a comment near the 🧠 Learnings used |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/provenance_store.rs`:
- Around line 758-769: The fork implementation currently copies
source_history.entries[..end_idx] as-is into the new WorldlineHistory which
preserves source-owned metadata (worldline_id, head_key/tip refs, parent refs)
causing entry(new_id, 0), tip_ref(new_id) and BtrPayload::validate() to fail;
fix by materializing a proper target-owned history in fork(): iterate the
entries being copied, clone each ProvenanceEntry but remap any worldline-local
metadata to the new worldline (set worldline_id -> new_id, rewrite head_key/tip
refs to point at the corresponding entry IDs in the new sequence, and remap
parent refs to the new entry IDs), update checkpoints if they reference old
ticks/refs, then insert that remapped Vec into WorldlineHistory.entries before
calling self.worldlines.insert(new_id, new_history) so build_btr(new_id, ..) and
validate succeed.
- Around line 347-349: The tick arithmetic uses unchecked additions and must use
checked_add to avoid release/debug divergence: change end_tick_exclusive() to
return a Result<u64, BtrError> (or otherwise propagate) and compute
self.start_tick.checked_add(self.entries.len() as
u64).ok_or(BtrError::TickOverflow) so overflow is reported; likewise replace all
unchecked increments of expected_tick (e.g., expected_tick += 1) with
expected_tick = expected_tick.checked_add(1).ok_or(BtrError::TickOverflow) and
propagate the error through the surrounding function(s) so overflow yields
BtrError::TickOverflow rather than wrapping or panicking.
In `@docs/adr/ADR-0011-explicit-observation-contract.md`:
- Around line 41-45: The ADR's canonical signature is stale: replace the free
function form "observe(request: ObservationRequest) ->
Result<ObservationArtifact, ObservationError>" with the actual implementation
signature used in the codebase — e.g. document the method form
ObservationService::observe(runtime, provenance, engine, request) and list the
required parameters (runtime, provenance, engine) alongside the request and
return types (ObservationArtifact, ObservationError) so the ADR reflects the
immutable dependency boundary implied by ObservationService::observe and the
types ObservationRequest/ObservationArtifact/ObservationError.
In `@scripts/verify-local.sh`:
- Around line 495-497: The helper should_run_parallel_lanes currently treats any
non-"parallel" value as sequential; change it to validate VERIFY_LANE_MODE
explicitly and fail on unknown values: in should_run_parallel_lanes check for
the allowed values (explicitly "parallel" -> return true, "sequential" or
"serial" as your canonical sequential string -> return false), and for any other
value emit an error message about the invalid VERIFY_LANE_MODE and exit non‑zero
instead of silently falling through; update any callers/tests that expect silent
fallback if needed.
- Around line 22-23: The current stamping uses a single "full" stamp under
VERIFY_LANE_ROOT regardless of VERIFY_LANE_MODE, so a sequential fallback can
create the same stamp and short-circuit later runs; change stamp paths and any
checks that reference the "full" stamp to include VERIFY_LANE_MODE (e.g.,
incorporate "${VERIFY_LANE_MODE}" into the stamp filename or directory used by
the functions that create/check the full stamp), and update all other
occurrences of the same stamp logic (the block around the shown variables and
the duplicated logic referenced at the other location) to use the mode-keyed
stamp or a separate sequential-specific suite name.
- Around line 430-437: The nextest branch hardcodes "--lib --tests" which breaks
bin-only crates; instead detect actual package targets via the manifest or cargo
metadata and build test/clippy flags accordingly. Update the use_nextest branch
in the script to replace the hardcoded "--lib --tests" with the same resolved
args returned by targeted_test_args_for_crate("$crate") (or refactor
targeted_test_args_for_crate to query cargo metadata and return correct flags),
and similarly update clippy_target_args_for_scope() and
crate_supports_lib_target() so they inspect package target types (via cargo
metadata or parsing Cargo.toml) rather than relying on crate name heuristics.
Ensure cargo nextest run and cargo test/clippy invocations use the derived args
array so bin-only crates do not get "--lib".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0e8a682-0168-440b-abd4-3c5f319da8c1
📒 Files selected for processing (7)
CHANGELOG.mdMakefilecrates/warp-core/src/provenance_store.rsdocs/adr/ADR-0011-explicit-observation-contract.mdscripts/hooks/README.mdscripts/verify-local.shtests/hooks/test_verify_local.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/hooks/test_verify_local.sh (1)
320-324:⚠️ Potential issue | 🟠 MajorCoverage accounting drops
xtask, so this test cannot catch xtask-scope regressions.Line 320-Line 324 only harvests
FULL_CRITICAL_PREFIXESentries starting withcrates/, which excludesxtask/. That can keepmissing_build/missing_clippyempty even when xtask lane selection is broken.Proposed fix
critical_crates = { prefix[len("crates/"):-1] for prefix in parse_array("FULL_CRITICAL_PREFIXES") if prefix.startswith("crates/") } +if "xtask/" in parse_array("FULL_CRITICAL_PREFIXES"): + critical_crates.add("xtask")Also applies to: 333-335
🤖 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 320 - 324, critical_crates currently filters FULL_CRITICAL_PREFIXES to only prefixes starting with "crates/" which excludes "xtask/" and lets xtask regressions slip; update the filtering logic in the critical_crates comprehension (and the similar logic at the second occurrence around lines 333-335) to also include entries that start with "xtask/" (or otherwise normalize/remove the strict "crates/" requirement), e.g. accept both "crates/" and "xtask/" prefixes when computing critical_crates from parse_array("FULL_CRITICAL_PREFIXES") so xtask-scoped entries are harvested for missing_build/missing_clippy checks.scripts/verify-local.sh (1)
382-384:⚠️ Potential issue | 🟠 Major
xtask/changes are currently downgraded totooling-onlyfull scope.Line 382 only extracts crates from
crates/<name>/..., soxtask/...never entersFULL_SCOPE_SELECTED_CRATESand Lines 725-730 can route an xtask change totooling-only, skipping rust clippy/test lanes.Proposed fix
list_changed_crates() { - printf '%s\n' "$CHANGED_FILES" | sed -n 's#^crates/\([^/]*\)/.*#\1#p' | sort -u + { + printf '%s\n' "$CHANGED_FILES" | sed -n 's#^crates/\([^/]*\)/.*#\1#p' + printf '%s\n' "$CHANGED_FILES" | sed -n 's#^xtask/.*#xtask#p' + } | sort -u }Also applies to: 386-394, 721-731
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-local.sh` around lines 382 - 384, The current list_changed_crates function only extracts names under crates/<name>/..., so changes under xtask/ never appear in FULL_SCOPE_SELECTED_CRATES and get misrouted to tooling-only; update list_changed_crates to also detect xtask changes (e.g., return "xtask" for paths starting with xtask/ or capture either crates/<name> or xtask as the changed module) so FULL_SCOPE_SELECTED_CRATES contains xtask when relevant and the downstream routing (the logic referenced by FULL_SCOPE_SELECTED_CRATES used around the routing lines) will include rust clippy/test lanes; adjust the same pattern usage in the other occurrences noted (the similar blocks around where the crate-listing and routing are duplicated).
♻️ Duplicate comments (2)
scripts/verify-local.sh (2)
579-581:⚠️ Potential issue | 🟡 MinorFail closed on invalid
VERIFY_LANE_MODEvalues.Line 579 treats any non-
parallelvalue as sequential. A typo silently changes execution semantics and still reports a normal full verification path.Proposed fix
should_run_parallel_lanes() { - [[ "$VERIFY_LANE_MODE" == "parallel" ]] + case "$VERIFY_LANE_MODE" in + parallel) return 0 ;; + sequential) return 1 ;; + *) + echo "[verify-local] invalid VERIFY_LANE_MODE: $VERIFY_LANE_MODE (expected: parallel|sequential)" >&2 + exit 1 + ;; + esac }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-local.sh` around lines 579 - 581, The function should_run_parallel_lanes currently treats any value other than "parallel" as sequential, which hides typos; add explicit validation of VERIFY_LANE_MODE and fail closed on invalid values by exiting with a non-zero status and an error message; update or augment should_run_parallel_lanes (and any init/validation logic that reads VERIFY_LANE_MODE) to accept only "parallel" or "sequential" (or empty/default), print a clear error including the invalid value, and exit non-zero so the script does not silently proceed with the wrong lane mode.
514-517:⚠️ Potential issue | 🔴 CriticalResolve target kinds from manifests; current
--libassumptions will break bin-only crates.Line 515 hardcodes
cargo nextest ... --lib --tests, and Lines 640-688 infer lib support withcrate != xtaskplus default--libclippy/test args. That still misflags bin-only crates and causes deterministic verifier failures.Proposed fix
@@ - if use_nextest; then - echo "[verify-local] cargo nextest run -p ${crate} --lib --tests" - cargo +"$PINNED" nextest run -p "$crate" --lib --tests + if use_nextest; then + local -a test_args=() + mapfile -t test_args < <(targeted_test_args_for_crate "$crate") + echo "[verify-local] cargo nextest run -p ${crate} ${test_args[*]}" + cargo +"$PINNED" nextest run -p "$crate" "${test_args[@]}" @@ crate_supports_lib_target() { local crate="$1" - [[ "$crate" != "xtask" ]] + local manifest="crates/${crate}/Cargo.toml" + [[ -f "crates/${crate}/src/lib.rs" ]] && return 0 + rg -q '^\[lib\]' "$manifest" +} + +crate_supports_bin_target() { + local crate="$1" + local manifest="crates/${crate}/Cargo.toml" + [[ -f "crates/${crate}/src/main.rs" ]] && return 0 + rg -q '^\[\[bin\]\]' "$manifest" } @@ - if [[ "$crate" == "xtask" ]]; then - printf '%s\n' "--bins" - return - fi - if [[ "$scope" == "full" ]]; then printf '%s\n' "--all-targets" return fi - printf '%s\n' "--lib" + if crate_supports_lib_target "$crate"; then + printf '%s\n' "--lib" + fi + if crate_supports_bin_target "$crate"; then + printf '%s\n' "--bins" + fi if ! crate_is_fast_clippy_lib_only "$crate"; then printf '%s\n' "--tests" fi } @@ - if [[ "$crate" == "xtask" ]]; then - printf '%s\n' "--bins" - return - fi - if crate_supports_lib_target "$crate"; then printf '%s\n' "--lib" fi + if crate_supports_bin_target "$crate"; then + printf '%s\n' "--bins" + fi printf '%s\n' "--tests" }#!/usr/bin/env bash set -euo pipefail echo "[check] crates with no lib target (src/lib.rs or [lib])" while IFS= read -r manifest; do dir="$(dirname "$manifest")" crate="$(basename "$dir")" has_lib=0 [[ -f "$dir/src/lib.rs" ]] && has_lib=1 rg -q '^\[lib\]' "$manifest" && has_lib=1 || true if [[ $has_lib -eq 0 ]]; then printf ' %s\n' "$crate" fi done < <(fd -a Cargo.toml crates | sort) echo echo "[check] current forced --lib call sites" rg -n 'nextest run -p .*--lib --tests|crate_supports_lib_target|clippy_target_args_for_scope|targeted_test_args_for_crate' scripts/verify-local.shAlso applies to: 640-688
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-local.sh` around lines 514 - 517, The script currently forces `--lib` in the `use_nextest` branch (`cargo nextest run -p "$crate" --lib --tests`) and elsewhere assumes lib support via helpers like `crate_supports_lib_target`, `clippy_target_args_for_scope`, and `targeted_test_args_for_crate`; update these call sites to detect whether a crate has a lib target (inspect the crate's Cargo.toml for a [lib] table or check for src/lib.rs) and only append `--lib` when a lib target exists; change the `use_nextest` branch to call `cargo nextest run -p "$crate" ${lib_arg} --tests` (or equivalent) where `lib_arg` is set to `--lib` only if the detection indicates a lib target, and update the mentioned helper functions to perform the same detection so bin-only crates are not passed `--lib`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 34-35: Update the CHANGELOG entry so it accurately reflects that
execute_query is not yet a full one-phase adapter: instead of grouping
`execute_query` with `get_head`, `snapshot_at`, and `drain_view_ops` as "thin
one-phase adapters", clarify that `execute_query(...)` currently lowers through
observation semantics and returns a deterministic UNSUPPORTED_QUERY until full
query support is implemented; edit the sentence mentioning `execute_query` to
explicitly state its temporary behavior and keep `get_head`, `snapshot_at`, and
`drain_view_ops` described as thin one-phase adapters.
- Around line 23-39: Add explicit changelog entries for ADR-0011, the
ttd-browser migration, and Phase 4 hard cut: mention ADR-0011 (the explicit
observation contract) as a documented architectural decision; add a
breaking-change note that ttd-browser was migrated to the entry-based provenance
API; and enumerate removed/deprecated convenience methods (e.g., get_head,
snapshot_at, execute_query, drain_view_ops) and that these are now thin adapters
over ObservationService::observe/WarpKernel observe. Update the top-level
changelog paragraph to call out the breaking API changes and provide migration
guidance references to ADR-0011 and the ttd-browser update.
In `@scripts/verify-local.sh`:
- Around line 583-638: The run_parallel_lanes function is not signal-safe: add a
signal trap inside run_parallel_lanes that catches INT and TERM (and ERR/EXIT if
desired) to kill all background lane PIDs stored in lane_pids, remove the
temporary logdir, and then re-raise or exit; implement a cleanup handler (e.g.,
cleanup_parallel_lanes) referenced by the trap that iterates over lane_pids and
runs kill (ignoring errors) and waits for them, removes "${logdir}" and exits
with non-zero, and ensure the trap is removed/restored (trap - ...) before
normal exit so the handler only runs for interruptions; update references to
lane_pids and logdir in the handler so the temporary logs are always cleaned on
interrupts.
In `@tests/hooks/test_verify_local.sh`:
- Around line 82-176: The test currently only asserts the Makefile target exists
but not that sequential fallback actually avoids parallel fanout; update the
test harness around run_fake_verify to add a case that sets
VERIFY_LANE_MODE=sequential (and KEEP VERIFY_FAKE_CARGO_LOG), runs
./scripts/verify-local.sh "full" (or invokes the verify-full-sequential flow),
and then assert the VERIFY_FAKE_CARGO_LOG shows only a single cargo invocation
(or lacks any parallel lane markers) to prove fanout was bypassed; use the
existing run_fake_verify, VERIFY_FAKE_CARGO_LOG, VERIFY_CHANGED_FILES_FILE and
VERIFY_FORCE variables and check the cargo_log contents rather than merely
testing the Makefile target name.
---
Outside diff comments:
In `@scripts/verify-local.sh`:
- Around line 382-384: The current list_changed_crates function only extracts
names under crates/<name>/..., so changes under xtask/ never appear in
FULL_SCOPE_SELECTED_CRATES and get misrouted to tooling-only; update
list_changed_crates to also detect xtask changes (e.g., return "xtask" for paths
starting with xtask/ or capture either crates/<name> or xtask as the changed
module) so FULL_SCOPE_SELECTED_CRATES contains xtask when relevant and the
downstream routing (the logic referenced by FULL_SCOPE_SELECTED_CRATES used
around the routing lines) will include rust clippy/test lanes; adjust the same
pattern usage in the other occurrences noted (the similar blocks around where
the crate-listing and routing are duplicated).
In `@tests/hooks/test_verify_local.sh`:
- Around line 320-324: critical_crates currently filters FULL_CRITICAL_PREFIXES
to only prefixes starting with "crates/" which excludes "xtask/" and lets xtask
regressions slip; update the filtering logic in the critical_crates
comprehension (and the similar logic at the second occurrence around lines
333-335) to also include entries that start with "xtask/" (or otherwise
normalize/remove the strict "crates/" requirement), e.g. accept both "crates/"
and "xtask/" prefixes when computing critical_crates from
parse_array("FULL_CRITICAL_PREFIXES") so xtask-scoped entries are harvested for
missing_build/missing_clippy checks.
---
Duplicate comments:
In `@scripts/verify-local.sh`:
- Around line 579-581: The function should_run_parallel_lanes currently treats
any value other than "parallel" as sequential, which hides typos; add explicit
validation of VERIFY_LANE_MODE and fail closed on invalid values by exiting with
a non-zero status and an error message; update or augment
should_run_parallel_lanes (and any init/validation logic that reads
VERIFY_LANE_MODE) to accept only "parallel" or "sequential" (or empty/default),
print a clear error including the invalid value, and exit non-zero so the script
does not silently proceed with the wrong lane mode.
- Around line 514-517: The script currently forces `--lib` in the `use_nextest`
branch (`cargo nextest run -p "$crate" --lib --tests`) and elsewhere assumes lib
support via helpers like `crate_supports_lib_target`,
`clippy_target_args_for_scope`, and `targeted_test_args_for_crate`; update these
call sites to detect whether a crate has a lib target (inspect the crate's
Cargo.toml for a [lib] table or check for src/lib.rs) and only append `--lib`
when a lib target exists; change the `use_nextest` branch to call `cargo nextest
run -p "$crate" ${lib_arg} --tests` (or equivalent) where `lib_arg` is set to
`--lib` only if the detection indicates a lib target, and update the mentioned
helper functions to perform the same detection so bin-only crates are not passed
`--lib`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e0faf51c-50eb-4871-af5c-d686c2836763
📒 Files selected for processing (4)
CHANGELOG.mdscripts/hooks/README.mdscripts/verify-local.shtests/hooks/test_verify_local.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
CHANGELOG.md (2)
37-39:⚠️ Potential issue | 🟡 Minor
execute_querybehavior is documented incorrectly.Current text says
execute_queryis a thin adapter; PR objectives state it currently returns deterministicUNSUPPORTED_QUERY. Document that temporary behavior explicitly.Proposed wording fix
- **Changed** `WarpKernel` and the WASM ABI now expose `observe(...)`, while - `get_head`, `snapshot_at`, `execute_query`, and `drain_view_ops` are thin - one-phase adapters over the observation contract. + `get_head`, `snapshot_at`, and `drain_view_ops` are thin one-phase adapters + over the observation contract. `execute_query` currently returns + deterministic `UNSUPPORTED_QUERY` until full query support lands.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 37 - 39, Update the CHANGELOG entry to correctly document execute_query's current behavior: instead of saying it is a "thin one-phase adapter," explicitly state that execute_query currently returns a deterministic UNSUPPORTED_QUERY placeholder value (temporary behavior), while observe(...) and the other adapters (get_head, snapshot_at, drain_view_ops) remain as described; reference WarpKernel and the WASM ABI in the same sentence so readers know which interfaces exhibit this temporary execute_query behavior.
27-43:⚠️ Potential issue | 🟠 MajorMissing release-note items for shipped contract artifacts and migration impact.
This entry omits three material changes called out in the PR objectives: ADR-0011 adoption,
ttd-browsermigration, and the Phase 4 hard-cut/removal impact. Add them explicitly so consumers can map behavior changes to migration docs.Proposed changelog patch
### feat(warp-core): close Phase 4 and pivot reads to observe +- **Added** ADR-0011 documenting the explicit Phase 5 observation contract. - **Changed** Phase 4 provenance/BTR work is now the documented substrate baseline: provenance is entry-based, parent refs are stored explicitly, and the standalone `ProvenanceService` owns authoritative worldline history. @@ - **Changed** `drain_view_ops()` is now legacy adapter/debug behavior only: it reads recorded truth through `observe(...)` and tracks only adapter-local drain state instead of mutating runtime-owned materialization state. +- **Changed** `ttd-browser` migrated to the entry-based provenance API after + the Phase 4 hard cut removed old convenience methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 27 - 43, The changelog entry omits three required release-note items; update the "feat(warp-core): close Phase 4 and pivot reads to observe" section to explicitly list (1) ADR-0011 adoption (note the contract/patterns now required by ADR-0011), (2) ttd-browser migration guidance (what consumers must change and any compatibility shims), and (3) Phase 4 hard-cut/removal impact (which APIs/behaviors are removed and links to migration docs); mention shipped contract artifacts (ObservationService::observe, error codes like INVALID_WORLDLINE/INVALID_TICK) and succinctly state migration impact and where to find the migration docs/ADR for each item so downstream consumers can map behavior changes to migration steps.scripts/verify-local.sh (1)
651-654:⚠️ Potential issue | 🟡 MinorManifest-based detection needed for
crate_supports_lib_target.Currently relies on name matching (
!= "xtask"). Bin-only crates—echo-session-service,echo-session-ws-gateway,echo-wesley-gen,warp-benches,warp-viewer—should return false but only do so incidentally by exclusion fromFULL_LOCAL_PACKAGES. If a bin-only crate is ever added toFULL_LOCAL_PACKAGES,cargo test --libwill fail silently or with a confusing error.Inspect
Cargo.tomlfor[lib]section instead of name pattern to make the check deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-local.sh` around lines 651 - 654, The current crate_supports_lib_target function wrongly infers lib-support by name; change it to deterministically inspect the crate's Cargo.toml for a [lib] table (or an explicit package.lib entry) instead of the name check. Update crate_supports_lib_target to locate the crate directory from the given $crate, read its Cargo.toml, and return success only if it contains a [lib] section (or a package.lib key); otherwise return false so bin-only crates like echo-session-service are correctly identified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/verify-local.sh`:
- Around line 667-685: The function clippy_target_args_for_scope unconditionally
emits "--lib" for non-xtask crates which breaks on bin-only crates; update it to
call crate_supports_lib_target "$crate" before printing "--lib" and, if that
returns false, print "--bins" instead; also ensure the conditional that prints
"--tests" only runs when the crate supports a lib/targets (use
crate_supports_lib_target and crate_is_fast_clippy_lib_only together so you
don't emit "--tests" for crates that are bin-only or fast-clippy-lib-only).
- Around line 320-339: The array expansion in array_contains and append_unique
(and call sites such as filter_package_set_by_selection) can trigger unbound
variable errors under set -u on older bash when arrays are empty; fix by using
the portable expansion pattern when passing or expanding arrays (e.g. replace
uses of "${array_ref[@]}" with the safe form ${array_ref[@]+"${array_ref[@]}"}
wherever array_ref (or other arrays) may be empty) and update all call sites of
array_contains and append_unique to pass arrays with the same pattern so
empty-array calls do not fail.
---
Duplicate comments:
In `@CHANGELOG.md`:
- Around line 37-39: Update the CHANGELOG entry to correctly document
execute_query's current behavior: instead of saying it is a "thin one-phase
adapter," explicitly state that execute_query currently returns a deterministic
UNSUPPORTED_QUERY placeholder value (temporary behavior), while observe(...) and
the other adapters (get_head, snapshot_at, drain_view_ops) remain as described;
reference WarpKernel and the WASM ABI in the same sentence so readers know which
interfaces exhibit this temporary execute_query behavior.
- Around line 27-43: The changelog entry omits three required release-note
items; update the "feat(warp-core): close Phase 4 and pivot reads to observe"
section to explicitly list (1) ADR-0011 adoption (note the contract/patterns now
required by ADR-0011), (2) ttd-browser migration guidance (what consumers must
change and any compatibility shims), and (3) Phase 4 hard-cut/removal impact
(which APIs/behaviors are removed and links to migration docs); mention shipped
contract artifacts (ObservationService::observe, error codes like
INVALID_WORLDLINE/INVALID_TICK) and succinctly state migration impact and where
to find the migration docs/ADR for each item so downstream consumers can map
behavior changes to migration steps.
In `@scripts/verify-local.sh`:
- Around line 651-654: The current crate_supports_lib_target function wrongly
infers lib-support by name; change it to deterministically inspect the crate's
Cargo.toml for a [lib] table (or an explicit package.lib entry) instead of the
name check. Update crate_supports_lib_target to locate the crate directory from
the given $crate, read its Cargo.toml, and return success only if it contains a
[lib] section (or a package.lib key); otherwise return false so bin-only crates
like echo-session-service are correctly identified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 57710df1-eb4f-4f37-bd25-f581a46af511
📒 Files selected for processing (4)
CHANGELOG.mdscripts/hooks/README.mdscripts/verify-local.shtests/hooks/test_verify_local.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
scripts/verify-local.sh (4)
333-340:⚠️ Potential issue | 🟡 MinorEmpty array expansion fails under
set -uon bash < 4.4.Line 337 expands
"${array_ref[@]}"directly. If the array is empty, bash versions before 4.4 raise an unbound variable error underset -u. Same issue infilter_package_set_by_selectionat line 716.Portable fix
append_unique() { local value="$1" local array_name="$2" local -n array_ref="$array_name" - if ! array_contains "$value" "${array_ref[@]}"; then + if ! array_contains "$value" ${array_ref[@]+"${array_ref[@]}"}; then array_ref+=("$value") fi }Apply the same
${arr[@]+"${arr[@]}"}pattern at line 716 infilter_package_set_by_selection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-local.sh` around lines 333 - 340, The append_unique function and filter_package_set_by_selection use "${array_ref[@]}" which under set -u on bash <4.4 can raise an unbound variable error when the array is empty; change the expansions to the portable form ${array_ref[@]+"${array_ref[@]}"} (and the analogous ${arr[@]+"${arr[@]}"} in filter_package_set_by_selection) so empty arrays don't trigger unbound-variable errors while preserving normal expansion behavior.
597-599:⚠️ Potential issue | 🟡 MinorUnknown
VERIFY_LANE_MODEsilently falls back to sequential.Any typo or invalid value falls through to sequential execution without warning. This can mask configuration errors.
Proposed fix
should_run_parallel_lanes() { - [[ "$VERIFY_LANE_MODE" == "parallel" ]] + case "$VERIFY_LANE_MODE" in + parallel) return 0 ;; + sequential) return 1 ;; + *) + echo "[verify-local] invalid VERIFY_LANE_MODE: $VERIFY_LANE_MODE" >&2 + exit 1 + ;; + esac }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-local.sh` around lines 597 - 599, The helper should_run_parallel_lanes currently treats any non-"parallel" VERIFY_LANE_MODE as sequential silently; update it to validate VERIFY_LANE_MODE explicitly and emit a clear warning or error on unknown values: in the should_run_parallel_lanes function check for allowed values ("parallel" and "sequential"), return true only for "parallel", return false for "sequential", and for any other value log a warning (or exit with non-zero) indicating an invalid VERIFY_LANE_MODE so typos/configuration mistakes are surfaced; reference the VERIFY_LANE_MODE variable and should_run_parallel_lanes function when making this change.
601-656:⚠️ Potential issue | 🟠 MajorParallel lane cleanup is not signal-safe.
run_parallel_lanesspawns background jobs but lacks INT/TERM traps. Ctrl+C leaves cargo processes alive and leaks the temp logdir.Proposed fix
run_parallel_lanes() { local suite="$1" shift local logdir logdir="$(mktemp -d "${TMPDIR:-/tmp}/verify-local-${suite}.XXXXXX")" local -a lane_names=() local -a lane_funcs=() local -a lane_pids=() local i + cleanup_parallel_lanes() { + local pid + for pid in "${lane_pids[@]:-}"; do + kill "$pid" 2>/dev/null || true + done + rm -rf "$logdir" + } + trap 'cleanup_parallel_lanes; exit 130' INT TERM + while [[ $# -gt 0 ]]; doAnd before the final
rm -rf "$logdir":+ trap - INT TERM rm -rf "$logdir" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-local.sh` around lines 601 - 656, The run_parallel_lanes function can leak child processes and the temporary logdir on SIGINT/SIGTERM; add a signal trap that, when triggered, iterates over lane_pids to kill (and wait for) each background PID, prints which lane was interrupted using lane_names, cleans up the logdir, and then exits with a non-zero status; ensure you set the trap after logdir is created and before launching children and restore/clear the trap before the normal exit path so normal completion still performs the existing rm -rf "$logdir".
532-540:⚠️ Potential issue | 🟠 MajorNextest path hardcodes
--lib --testswithout checking for lib targets.Line 533-534 unconditionally passes
--lib --teststo nextest. Bin-only crates (e.g.,xtask) will fail. The non-nextest path at line 537 usestargeted_test_args_for_cratewhich handles this, but the nextest branch does not.Proposed fix
if use_nextest; then - echo "[verify-local] cargo nextest run -p ${crate} --lib --tests" - cargo +"$PINNED" nextest run -p "$crate" --lib --tests + local -a test_args=() + mapfile -t test_args < <(targeted_test_args_for_crate "$crate") + echo "[verify-local] cargo nextest run -p ${crate} ${test_args[*]}" + cargo +"$PINNED" nextest run -p "$crate" "${test_args[@]}" else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/verify-local.sh` around lines 532 - 540, The nextest branch unconditionally adds "--lib --tests" causing failures for bin-only crates; change the cargo nextest invocation in the use_nextest true branch to compute test args the same way as the non-nextest branch by calling targeted_test_args_for_crate "$crate" (or reusing its output) and pass those args to cargo +"$PINNED" nextest run -p "$crate" instead of hardcoding --lib --tests; ensure you preserve echo/logging of the exact command and use "${test_args[@]}" when invoking nextest so bin-only crates won't receive invalid flags.tests/hooks/test_verify_local.sh (1)
365-369: 🧹 Nitpick | 🔵 TrivialSequential fallback behavior remains untested.
The test only verifies the Makefile target exists. It does not confirm that
VERIFY_LANE_MODE=sequentialactually bypasses parallel fanout.Proposed test
if grep -q '^verify-full-sequential:' Makefile; then pass "Makefile exposes a sequential fallback for the parallel full verifier" else fail "Makefile should expose verify-full-sequential as a fallback path" fi + +# Test that sequential mode actually skips parallel fanout +fake_seq_output="$( + cd "$tmp" && \ + PATH="$tmp/bin:$PATH" \ + VERIFY_FORCE=1 \ + VERIFY_LANE_MODE=sequential \ + VERIFY_STAMP_SUBJECT="test-head" \ + VERIFY_CHANGED_FILES_FILE="$changed" \ + VERIFY_FAKE_CARGO_LOG="$cargo_log" \ + ./scripts/verify-local.sh full 2>&1 +)" || true +if printf '%s\n' "$fake_seq_output" | grep -q '\[verify-local\] full: launching '; then + fail "sequential mode should not launch parallel lanes" +else + pass "sequential mode bypasses parallel lane fanout" +fi🤖 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 365 - 369, Add a runtime assertion that VERIFY_LANE_MODE=sequential actually triggers the sequential codepath: invoke the same Makefile verification target with VERIFY_LANE_MODE=sequential (e.g., run VERIFY_LANE_MODE=sequential make verify or the repo's verification command used elsewhere in this script), capture stdout/stderr, and assert the output indicates the sequential fallback (or the verify-full-sequential target was run) and does not contain parallel/fanout indicators; update the test around verify-full-sequential to run this command and use the existing pass()/fail() helpers to fail if the output shows parallel fanout or fails to show the sequential marker.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/hooks/test_verify_local.sh`:
- Around line 532-537: The test currently asserts exact lane ordering by
grepping for the string 'lanes=fmt guards hook-tests' against
fake_tooling_output; change this to assert presence of each expected lane
independently (e.g., check fake_tooling_output contains 'fmt', 'guards', and
'hook-tests') rather than exact sequence so the test is robust to reordering or
extra lanes—update the conditional around fake_tooling_output (the grep/check
logic) to test for each lane token separately and only fail if any expected
token is missing.
---
Duplicate comments:
In `@scripts/verify-local.sh`:
- Around line 333-340: The append_unique function and
filter_package_set_by_selection use "${array_ref[@]}" which under set -u on bash
<4.4 can raise an unbound variable error when the array is empty; change the
expansions to the portable form ${array_ref[@]+"${array_ref[@]}"} (and the
analogous ${arr[@]+"${arr[@]}"} in filter_package_set_by_selection) so empty
arrays don't trigger unbound-variable errors while preserving normal expansion
behavior.
- Around line 597-599: The helper should_run_parallel_lanes currently treats any
non-"parallel" VERIFY_LANE_MODE as sequential silently; update it to validate
VERIFY_LANE_MODE explicitly and emit a clear warning or error on unknown values:
in the should_run_parallel_lanes function check for allowed values ("parallel"
and "sequential"), return true only for "parallel", return false for
"sequential", and for any other value log a warning (or exit with non-zero)
indicating an invalid VERIFY_LANE_MODE so typos/configuration mistakes are
surfaced; reference the VERIFY_LANE_MODE variable and should_run_parallel_lanes
function when making this change.
- Around line 601-656: The run_parallel_lanes function can leak child processes
and the temporary logdir on SIGINT/SIGTERM; add a signal trap that, when
triggered, iterates over lane_pids to kill (and wait for) each background PID,
prints which lane was interrupted using lane_names, cleans up the logdir, and
then exits with a non-zero status; ensure you set the trap after logdir is
created and before launching children and restore/clear the trap before the
normal exit path so normal completion still performs the existing rm -rf
"$logdir".
- Around line 532-540: The nextest branch unconditionally adds "--lib --tests"
causing failures for bin-only crates; change the cargo nextest invocation in the
use_nextest true branch to compute test args the same way as the non-nextest
branch by calling targeted_test_args_for_crate "$crate" (or reusing its output)
and pass those args to cargo +"$PINNED" nextest run -p "$crate" instead of
hardcoding --lib --tests; ensure you preserve echo/logging of the exact command
and use "${test_args[@]}" when invoking nextest so bin-only crates won't receive
invalid flags.
In `@tests/hooks/test_verify_local.sh`:
- Around line 365-369: Add a runtime assertion that VERIFY_LANE_MODE=sequential
actually triggers the sequential codepath: invoke the same Makefile verification
target with VERIFY_LANE_MODE=sequential (e.g., run VERIFY_LANE_MODE=sequential
make verify or the repo's verification command used elsewhere in this script),
capture stdout/stderr, and assert the output indicates the sequential fallback
(or the verify-full-sequential target was run) and does not contain
parallel/fanout indicators; update the test around verify-full-sequential to run
this command and use the existing pass()/fail() helpers to fail if the output
shows parallel fanout or fails to show the sequential marker.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e31738d-4f61-4631-aea6-6b7fd79d9e67
📒 Files selected for processing (4)
CHANGELOG.mdscripts/hooks/README.mdscripts/verify-local.shtests/hooks/test_verify_local.sh
Owner
Author
|
@coderabbitai follow-up fixes are pushed in |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 785 🏁 Script executed: Length of output: 9578
Fork namespace rewrite — confirmed correct
Minor: the frontier tick is observed once via Shell syntax check expansion — one latent bug
.githooks/*|scripts/*.sh|scripts/hooks/*|tests/hooks/*.sh)
# ^^^^^ no .sh filter
.githooks/*|scripts/*.sh|scripts/hooks/*.sh|tests/hooks/*.sh)The .githooks/*.sh|scripts/*.sh|scripts/hooks/*.sh|tests/hooks/*.sh)Everything else is clean. The fork + drain fixes are the right fixes and the test coverage is solid for both. 🧠 Learnings used |
Owner
Author
|
@coderabbitai review please 🥕 Follow-up fixes are pushed in What changed:
Verification that passed:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1731 🏁 Script executed: Length of output: 2413
The two-path detection is right:
Shebang regex: Verified against the relevant cases:
One non-issue: the Regression tests — confirmed correct Both behaviors are directly exercised:
This is the right level of coverage for a tooling correctness fix — prove the bad path is gone and the fallback path fires. No remaining issues. All four flagged items from the
✏️ Learnings added
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
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
This branch closes the Phase 4 provenance/BTR substrate work and immediately pivots Echo’s read side to an explicit observation contract for Phase 5.
The main effect is that reads stop pretending to be timeless snapshots. The canonical internal read path is now
observe(...), which always makes the worldline, coordinate, frame, and projection explicit.What Changed
warp_core::ObservationServiceand the new observation types inwarp-corewarp-wasmthroughobserve(...)as the canonical read boundaryget_head,snapshot_at,execute_query, anddrain_view_opsas one-phase adapters onlyRecordedTruthstrictly provenance-backed; no hidden engine reruns during observationttd-browserto the entry-based provenance API after the Phase 4 hard cut removed the old convenience methodsNotes
execute_query(...)now lowers through observation semantics and returns deterministicUNSUPPORTED_QUERYuntil real query support lands.drain_view_ops()is legacy adapter/debug territory only in this phase. It reads recorded truth through observation and uses adapter-local bookkeeping instead of mutating runtime/provenance state.Validation
cargo test -p warp-core observation::tests --libcargo test -p warp-wasm --features engine --libcargo test -p echo-wasm-abicargo test -p ttd-browser --lib --testscargo clippy -p echo-wasm-abi --all-targets -- -D warnings -D missing_docscargo clippy -p warp-core --lib -- -D warnings -D missing_docscargo clippy -p warp-wasm --lib --features engine -- -D warnings -D missing_docsSummary by CodeRabbit
New Features
Refactor
Documentation
Chores / Tools