feat(wasm): app-agnostic WASM ABI v1 with KernelPort trait#296
feat(wasm): app-agnostic WASM ABI v1 with KernelPort trait#296flyingrobots merged 12 commits intomainfrom
Conversation
Replace all placeholder WASM exports in warp-wasm with real implementations backed by the KernelPort trait and WarpKernel engine wrapper. Adds ABI v1 contract (SPEC-0009), CBOR wire envelope protocol, structured error codes, and 14 conformance tests. - KernelPort trait in echo-wasm-abi (app-agnostic boundary contract) - WarpKernel in warp-wasm (engine feature, wraps warp-core::Engine) - init() export + install_kernel() injection API - dispatch_intent, step, drain_view_ops, get_head, snapshot_at wired - execute_query/render_snapshot return NOT_SUPPORTED honestly
… consistency, drain semantics
BLOCKING fixes:
- snapshot_at now saves/restores engine state around jump_to_tick to
prevent corrupting the live state for subsequent operations
- execute_query, snapshot_at, render_snapshot now wrap success results
in OkEnvelope via RawBytesResponse, matching the documented wire
protocol (all Uint8Array returns use { ok: true/false } discriminator)
SHOULD FIX:
- drain_view_ops now tracks drained state; second call without
intervening step returns empty (matches documented contract)
- Move serde-value from dependencies to dev-dependencies (test-only)
New tests:
- snapshot_at_does_not_corrupt_live_state (regression for state restore)
- drain_returns_empty_on_second_call (drain semantics)
Spec updated: SPEC-0009 documents RawBytesResponse wrapper for raw-byte
endpoints.
…add trait defaults - Validate EINT envelope in dispatch_intent before engine ingestion; return INVALID_INTENT (code 2) for malformed envelopes. - Add Display impl for EnvelopeError (no_std compatible). - Make OkEnvelope/ErrEnvelope ok field private with ::new() constructors. - Add default impls for execute_query and render_snapshot (NOT_SUPPORTED). - Remove redundant overrides from WarpKernel. - Add dispatch_invalid_intent_returns_invalid_intent_error test. - Update SPEC-0009 error code 2 description and versioning notes.
…rrors - Clarify OkEnvelope::new docs: T may be a reference for serialization. - Include input byte length in EINT validation error messages for production debugging.
- cfg-gate footprint enforcement internals so they compile out under unsafe_graph without dead-code warnings. - Add #[allow(unused_mut)] on cfg-conditional mutation in engine_impl. - Restructure TtdController WASM bindings with per-method wasm_bindgen and JsValue/JsError wrappers. - Add clippy allow attributes to echo-scene-codec test module. - Move enforcement-only imports into cfg-gated mod in parallel_footprints.
📝 WalkthroughWalkthroughAdds a WASM ABI v1 boundary (KernelPort, CBOR envelopes, error codes) and a WarpKernel implementation exposing CBOR-encoded wasm-bindgen exports. Gates footprint-enforcement code behind cfg flags, splits host vs wasm Ttd bindings, and adds a global-state allowlist entry for Changes
Sequence Diagram(s)sequenceDiagram
participant WASM_Host as WASM Host
participant WASM_Rt as WASM Runtime
participant WarpK as WarpKernel
participant Engine as Engine
participant Envelope as Envelope (CBOR)
WASM_Host->>WASM_Rt: install_kernel(kernel)
WASM_Rt->>WASM_Rt: store kernel in thread_local KERNEL
WASM_Host->>WASM_Rt: init()
WASM_Rt->>WarpK: create or retrieve kernel
WarpK->>Engine: new() or with_engine()
Engine-->>WarpK: Engine instance
WarpK-->>WASM_Rt: HeadInfo
WASM_Rt->>Envelope: encode_ok(HeadInfo)
Envelope-->>WASM_Host: Uint8Array(CBOR)
WASM_Host->>WASM_Rt: dispatch_intent(intent_bytes)
WASM_Rt->>WarpK: dispatch_intent(intent_bytes)
WarpK->>Engine: ingest/validate intent
Engine-->>WarpK: DispatchResponse
WASM_Rt->>Envelope: encode_ok(DispatchResponse)
Envelope-->>WASM_Host: Uint8Array(CBOR)
WASM_Host->>WASM_Rt: step(budget)
WASM_Rt->>WarpK: step(budget)
WarpK->>Engine: begin_tick/process/commit
Engine-->>WarpK: StepResponse (ticks_executed, head)
WASM_Rt->>Envelope: encode_ok(StepResponse)
Envelope-->>WASM_Host: Uint8Array(CBOR)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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: 9384eac54b
ℹ️ 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".
crates/warp-wasm/src/lib.rs
Outdated
| encode_ok(&kernel_port::HeadInfo { | ||
| tick: 0, | ||
| state_root: Vec::new(), | ||
| commit_id: Vec::new(), |
There was a problem hiding this comment.
init() returns a HeadInfo envelope but hard-codes state_root and commit_id to empty vectors here, which violates the HeadInfo contract (32-byte hashes) and gives callers invalid head metadata immediately after initialization. Any host that uses the init() response for initial sync/cache keys will observe incorrect values unless it makes an extra get_head() round-trip.
Useful? React with 👍 / 👎.
crates/warp-wasm/src/warp_kernel.rs
Outdated
| pub fn with_engine(engine: Engine, registry: RegistryInfo) -> Self { | ||
| Self { | ||
| engine, | ||
| tick_count: 0, | ||
| drained: true, |
There was a problem hiding this comment.
WarpKernel::new() registers sys/ack_pending because dispatch_next_intent always applies that rule when a pending intent exists, but with_engine() stores the provided engine as-is. For consumers using this advertised injection path, forgetting to pre-register that system rule causes step() to fail with ENGINE_ERROR after the first dispatched intent, breaking intent processing in custom kernels.
Useful? React with 👍 / 👎.
- Remove redundant explicit type annotation on `get_registry_info()` result and drop now-unused `RegistryInfo` import - Remove unnecessary `#[allow(dead_code)]` on pub `with_engine()` method - Add clarifying comment on abort-after-failed-commit safety
flyingrobots
changed the title
- Return real HeadInfo (32-byte hashes) from init() instead of empty vecs, so hosts don't need an extra get_head() round-trip - Auto-register sys/ack_pending in WarpKernel::with_engine(), silently ignoring duplicates if the caller already registered it - Add .ban-globals-allowlist to exempt warp-wasm boundary (WASM exports require module-scoped thread_local kernel storage) - Add tests for with_engine auto-registration and duplicate tolerance
|
Fixed in 4c52d00: init() returns real head hashes — with_engine() auto-registers ack_pending — Also added |
… test - Fix \bthread_local!\b and \blazy_static!\b patterns in ban-globals.sh: the unescaped ! was not matching in ripgrep regex, and the trailing \b after ! (a non-word char) could never match a word boundary. Use escaped \! without trailing \b. - Add fresh_kernel_head_has_real_hashes regression test verifying that a fresh WarpKernel returns non-empty, non-zero 32-byte hashes (guards against init() returning placeholder empty vecs).
- Replace unwrap_or_else fallback with expect on fresh kernel get_head() — the operation is infallible and a silent fallback would mask bugs - Add CHANGELOG entries for follow-up fixes: ban-globals regex, init() real hashes, with_engine auto-registration, rustdoc link
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/warp-core/src/footprint_guard.rs (1)
220-234:⚠️ Potential issue | 🟠 Major
OpenPortalis under-modeled by the enforcement target extractor.
WarpOp::OpenPortalalso upserts the child warp instance and child root node, butop_write_targets()only reports the parent attachment and parent warp. That leaves a blind spot for undeclared child-root writes and misstates the cross-warp semantics of portal creation.OpTargetsneeds to represent the child instance/root effects explicitly.Also applies to: 290-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-core/src/footprint_guard.rs` around lines 220 - 234, The extractor op_write_targets omits the child-warp and child-root writes for WarpOp::OpenPortal, so update the OpenPortal arm in op_write_targets to include the child instance and child root node in the returned OpTargets (in addition to the existing parent attachment/parent warp entries); specifically, when matching WarpOp::OpenPortal add write targets for the child warp instance and the child root node (the concrete OpTargets fields your crate uses to represent instance writes and node writes) so OpenPortal’s upsert semantics are fully represented—apply the same change for the analogous case around the other OpenPortal handling at the mentioned region (around lines 290–297).
🤖 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-scene-codec/src/mock_adapter.rs`:
- Line 176: Replace the module-level #[allow(clippy::unwrap_used,
clippy::expect_used, clippy::float_cmp)] with targeted fixes: remove the blanket
attribute and add per-line or per-item allows only where needed (e.g., the
specific test cases that call unwrap/expect), change the float_cmp usage on the
adapter.viewport check to an epsilon-based comparison (use f32::EPSILON or
approx crate) instead of silencing the lint, and replace .expect(...) usages in
tests (e.g., calls around apply_scene_delta(&delta)) with explicit assertions
like assert!(apply_scene_delta(...).is_ok()) so test helpers/mocks don’t mask
issues outside tests.
In `@crates/echo-wasm-abi/src/kernel_port.rs`:
- Around line 89-97: The HeadInfo.commit_id currently lacks precise semantics
and is being populated from Engine::snapshot().hash via WarpKernel; change this
by documenting the invariant and updating the population source: add a rustdoc
on struct HeadInfo and the commit_id field that clearly states it is the
canonical last-committed ledger commit hash (stable identifier for the last
committed snapshot), not the transient synthetic state hash, and update
WarpKernel/other callers to set HeadInfo.commit_id from the last committed
ledger snapshot commit ID (e.g., the engine/ledger API that returns the
committed snapshot hash) instead of Engine::snapshot().hash so public APIs carry
a single consistent meaning.
In `@crates/warp-core/src/engine_impl.rs`:
- Around line 1269-1272: Remove the local #[allow(unused_mut)] and instead
create two cfg-separated bindings for units so mutability is only present when
the footprint-enforcement cfg is active: use one #[cfg(...)] branch that
declares a mutable let mut units = build_work_units(items_by_warp) for the same
cfg used by the footprint enforcement code, and a separate #[cfg(not(...))]
branch that declares an immutable let units = build_work_units(items_by_warp);
this eliminates the unused_mut lint suppression while preserving behavior of
build_work_units and keeping the crate warning-clean.
In `@crates/warp-wasm/src/lib.rs`:
- Around line 169-180: init currently constructs a WarpKernel and fabricates a
default HeadInfo on get_head() failure, then installs the kernel and returns
encode_ok(&head); change this so that after creating the kernel with
warp_kernel::WarpKernel::new() you call kernel.get_head() and if it Errs you
return the encoded AbiError (do not call install_kernel) instead of building a
placeholder HeadInfo; only call install_kernel(Box::new(kernel)) and return
encode_ok(&head) when get_head() succeeds. Ensure you use the project’s existing
error-encoding helper (the counterpart to encode_ok) to return the AbiError.
- Around line 112-118: The fallback in encode_err_raw currently returns an empty
Uint8Array which violates the v1 CBOR envelope contract; change the Err match
branch to return a pre-encoded minimal CODEC_ERROR envelope instead. Create or
reuse a constant (e.g., PREENCODED_CODEC_ERROR_BYTES or CODEC_ERROR_ENVELOPE)
that holds the CBOR-encoded bytes for a minimal ErrEnvelope with a codec error
code, and in the Err(_) branch call
bytes_to_uint8array(&PREENCODED_CODEC_ERROR_BYTES) (or its Uint8Array
equivalent) rather than Uint8Array::new_with_length(0); keep the normal
Ok(bytes) => bytes_to_uint8array(&bytes) behavior and reference encode_err_raw,
ErrEnvelope, and echo_wasm_abi::encode_cbor when making the change.
In `@crates/warp-wasm/src/warp_kernel.rs`:
- Around line 213-246: snapshot_at currently calls
self.engine.jump_to_tick(tick) which only rewinds state but not the snapshot
context (last_snapshot), so engine.snapshot() produces a hash against the live
head; fix by obtaining the historical snapshot context instead of hashing the
live head: either read the stored ledger snapshot for that tick directly (e.g.
via the engine/ledger API that returns the saved snapshot/HeadInfo for a given
tick) and use that data for HeadInfo and encoding, or fully rewind the engine's
snapshot context (including last_snapshot/parent pointers) to the requested tick
before calling self.engine.snapshot(), then restore the full snapshot context
along with state after snapshot_at returns; update snapshot_at to use engine's
snapshot-at-tick API or to rewind/restore last_snapshot in addition to state so
the computed state_root/commit_id are derived from the historical context, not
the live head.
In `@docs/spec/SPEC-0009-wasm-abi-v1.md`:
- Around line 106-117: DrainResponse currently only exposes `channels` (or
absence thereof) which hides materialization boundary/finalization errors;
update the ABI docs for `DrainResponse` to add an explicit field (e.g.,
`boundary_errors` or `materialization_errors`) that surfaces any per-channel or
global finalization errors (type: array of error records or bytes plus an
error_code/message) so hosts can distinguish an empty successful drain from a
drain with silent output loss; update the `ChannelData` section if needed to
reference per-channel error linking, and apply the same addition to the other
affected block (lines around the duplicate section noted at 138-148) so both
places document the new error field consistently.
---
Outside diff comments:
In `@crates/warp-core/src/footprint_guard.rs`:
- Around line 220-234: The extractor op_write_targets omits the child-warp and
child-root writes for WarpOp::OpenPortal, so update the OpenPortal arm in
op_write_targets to include the child instance and child root node in the
returned OpTargets (in addition to the existing parent attachment/parent warp
entries); specifically, when matching WarpOp::OpenPortal add write targets for
the child warp instance and the child root node (the concrete OpTargets fields
your crate uses to represent instance writes and node writes) so OpenPortal’s
upsert semantics are fully represented—apply the same change for the analogous
case around the other OpenPortal handling at the mentioned region (around lines
290–297).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3cbdca53-167c-40cd-818d-6ba1f256bc17
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.ban-globals-allowlistCHANGELOG.mdcrates/echo-scene-codec/src/mock_adapter.rscrates/echo-wasm-abi/src/kernel_port.rscrates/echo-wasm-abi/src/lib.rscrates/echo-wasm-bindings/src/ttd.rscrates/warp-core/src/attachment.rscrates/warp-core/src/engine_impl.rscrates/warp-core/src/footprint_guard.rscrates/warp-core/src/parallel/exec.rscrates/warp-core/src/tick_delta.rscrates/warp-core/tests/parallel_footprints.rscrates/warp-wasm/Cargo.tomlcrates/warp-wasm/src/lib.rscrates/warp-wasm/src/warp_kernel.rsdocs/spec/SPEC-0009-wasm-abi-v1.mdscripts/ban-globals.sh
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[allow(clippy::unwrap_used, clippy::expect_used, clippy::float_cmp)] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Blanket test allows are a lazy pattern.
The module-level #[allow(...)] silences lints across 360 lines of test code. While unwrap/expect are idiomatic in tests, blanket allows prevent detection if test helpers or mock utilities later leak into production paths.
Consider alternatives:
- Per-line allows: Apply
#[allow(clippy::unwrap_used)]only to lines that need it (e.g., line 236). - Better float assertions: Line 513 is the sole
float_cmptrigger. Use epsilon-based comparison or theapproxcrate instead of silencing the lint:assert!((adapter.viewport.2 - 2.0).abs() < f32::EPSILON);
- Explicit test assertions: Replace
.expect("...")with patterns like:assert!(adapter.apply_scene_delta(&delta).is_ok());
For test code, unwrap/expect pragmatism is acceptable, but the blanket allow is a maintainability smell.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/echo-scene-codec/src/mock_adapter.rs` at line 176, Replace the
module-level #[allow(clippy::unwrap_used, clippy::expect_used,
clippy::float_cmp)] with targeted fixes: remove the blanket attribute and add
per-line or per-item allows only where needed (e.g., the specific test cases
that call unwrap/expect), change the float_cmp usage on the adapter.viewport
check to an epsilon-based comparison (use f32::EPSILON or approx crate) instead
of silencing the lint, and replace .expect(...) usages in tests (e.g., calls
around apply_scene_delta(&delta)) with explicit assertions like
assert!(apply_scene_delta(...).is_ok()) so test helpers/mocks don’t mask issues
outside tests.
| /// Current head state of the kernel. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct HeadInfo { | ||
| /// Current tick count (number of committed ticks). | ||
| pub tick: u64, | ||
| /// Graph-only state hash (32 bytes). | ||
| pub state_root: Vec<u8>, | ||
| /// Canonical commit hash (32 bytes). | ||
| pub commit_id: Vec<u8>, |
There was a problem hiding this comment.
Define the actual hash semantics of HeadInfo.commit_id.
The docs say this is the canonical commit hash, but WarpKernel currently populates it from Engine::snapshot().hash, which is a synthetic hash of the current state and can differ from the last committed ledger snapshot. Public callers need one stable meaning here, not both. Based on learnings: 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/echo-wasm-abi/src/kernel_port.rs` around lines 89 - 97, The
HeadInfo.commit_id currently lacks precise semantics and is being populated from
Engine::snapshot().hash via WarpKernel; change this by documenting the invariant
and updating the population source: add a rustdoc on struct HeadInfo and the
commit_id field that clearly states it is the canonical last-committed ledger
commit hash (stable identifier for the last committed snapshot), not the
transient synthetic state hash, and update WarpKernel/other callers to set
HeadInfo.commit_id from the last committed ledger snapshot commit ID (e.g., the
engine/ledger API that returns the committed snapshot hash) instead of
Engine::snapshot().hash so public APIs carry a single consistent meaning.
| // Build (warp, shard) work units - canonical ordering preserved | ||
| // mut required when footprint enforcement is active (cfg-gated below). | ||
| #[allow(unused_mut)] | ||
| let mut units = build_work_units(items_by_warp); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the lint suppression and split the binding by cfg.
#[allow(unused_mut)] is only papering over cfg-dependent mutability here. In warp-core this is avoidable by defining the binding separately per cfg, which keeps the crate warning-clean without adding a local allow.
Proposed change
- #[allow(unused_mut)]
- let mut units = build_work_units(items_by_warp);
+ #[cfg(all(
+ any(debug_assertions, feature = "footprint_enforce_release"),
+ not(feature = "unsafe_graph")
+ ))]
+ let mut units = build_work_units(items_by_warp);
+ #[cfg(not(all(
+ any(debug_assertions, feature = "footprint_enforce_release"),
+ not(feature = "unsafe_graph")
+ )))]
+ let units = build_work_units(items_by_warp);Based on learnings: All determinism-critical crates (warp-core, echo-wasm-abi, echo-scene-port) must be compiled with RUSTFLAGS="-Dwarnings" to forbid any compiler warnings, including unused imports, dead code, or silenced lints.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/warp-core/src/engine_impl.rs` around lines 1269 - 1272, Remove the
local #[allow(unused_mut)] and instead create two cfg-separated bindings for
units so mutability is only present when the footprint-enforcement cfg is
active: use one #[cfg(...)] branch that declares a mutable let mut units =
build_work_units(items_by_warp) for the same cfg used by the footprint
enforcement code, and a separate #[cfg(not(...))] branch that declares an
immutable let units = build_work_units(items_by_warp); this eliminates the
unused_mut lint suppression while preserving behavior of build_work_units and
keeping the crate warning-clean.
| /// Low-level error encoding that cannot itself fail (falls back to empty array). | ||
| fn encode_err_raw(code: u32, message: &str) -> Uint8Array { | ||
| let envelope = ErrEnvelope::new(code, message.into()); | ||
| match echo_wasm_abi::encode_cbor(&envelope) { | ||
| Ok(bytes) => bytes_to_uint8array(&bytes), | ||
| Err(_) => Uint8Array::new_with_length(0), | ||
| } |
There was a problem hiding this comment.
Never return a zero-length "error envelope".
Falling back to Uint8Array(0) breaks the v1 contract that every byte-returning export is a CBOR { ok: ... } envelope. If error encoding fails, return a pre-encoded minimal CODEC_ERROR envelope instead of undecodable empty bytes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/warp-wasm/src/lib.rs` around lines 112 - 118, The fallback in
encode_err_raw currently returns an empty Uint8Array which violates the v1 CBOR
envelope contract; change the Err match branch to return a pre-encoded minimal
CODEC_ERROR envelope instead. Create or reuse a constant (e.g.,
PREENCODED_CODEC_ERROR_BYTES or CODEC_ERROR_ENVELOPE) that holds the
CBOR-encoded bytes for a minimal ErrEnvelope with a codec error code, and in the
Err(_) branch call bytes_to_uint8array(&PREENCODED_CODEC_ERROR_BYTES) (or its
Uint8Array equivalent) rather than Uint8Array::new_with_length(0); keep the
normal Ok(bytes) => bytes_to_uint8array(&bytes) behavior and reference
encode_err_raw, ErrEnvelope, and echo_wasm_abi::encode_cbor when making the
change.
| pub fn init() -> Uint8Array { | ||
| #[cfg(feature = "engine")] | ||
| { | ||
| let kernel = warp_kernel::WarpKernel::new(); | ||
| let head = kernel.get_head().unwrap_or_else(|_| kernel_port::HeadInfo { | ||
| tick: 0, | ||
| state_root: Vec::new(), | ||
| commit_id: Vec::new(), | ||
| }); | ||
| install_kernel(Box::new(kernel)); | ||
| encode_ok(&head) | ||
| } |
There was a problem hiding this comment.
Do not fabricate a successful head on initialization failure.
If get_head() fails, init() currently emits { tick: 0, state_root: [], commit_id: [] } and still installs the kernel. That recreates the placeholder-success behavior this PR is supposed to remove. Return the encoded AbiError and skip installation on failure.
Proposed change
#[cfg(feature = "engine")]
{
let kernel = warp_kernel::WarpKernel::new();
- let head = kernel.get_head().unwrap_or_else(|_| kernel_port::HeadInfo {
- tick: 0,
- state_root: Vec::new(),
- commit_id: Vec::new(),
- });
+ let head = match kernel.get_head() {
+ Ok(head) => head,
+ Err(err) => return encode_err(&err),
+ };
install_kernel(Box::new(kernel));
encode_ok(&head)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn init() -> Uint8Array { | |
| #[cfg(feature = "engine")] | |
| { | |
| let kernel = warp_kernel::WarpKernel::new(); | |
| let head = kernel.get_head().unwrap_or_else(|_| kernel_port::HeadInfo { | |
| tick: 0, | |
| state_root: Vec::new(), | |
| commit_id: Vec::new(), | |
| }); | |
| install_kernel(Box::new(kernel)); | |
| encode_ok(&head) | |
| } | |
| pub fn init() -> Uint8Array { | |
| #[cfg(feature = "engine")] | |
| { | |
| let kernel = warp_kernel::WarpKernel::new(); | |
| let head = match kernel.get_head() { | |
| Ok(head) => head, | |
| Err(err) => return encode_err(&err), | |
| }; | |
| install_kernel(Box::new(kernel)); | |
| encode_ok(&head) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/warp-wasm/src/lib.rs` around lines 169 - 180, init currently
constructs a WarpKernel and fabricates a default HeadInfo on get_head() failure,
then installs the kernel and returns encode_ok(&head); change this so that after
creating the kernel with warp_kernel::WarpKernel::new() you call
kernel.get_head() and if it Errs you return the encoded AbiError (do not call
install_kernel) instead of building a placeholder HeadInfo; only call
install_kernel(Box::new(kernel)) and return encode_ok(&head) when get_head()
succeeds. Ensure you use the project’s existing error-encoding helper (the
counterpart to encode_ok) to return the AbiError.
| pub fn with_engine(mut engine: Engine, registry: RegistryInfo) -> Self { | ||
| // Ensure the system inbox rule is present. If the caller already | ||
| // registered it, register_rule returns DuplicateRuleName — ignore. | ||
| let _ = engine.register_rule(inbox::ack_pending_rule()); | ||
|
|
||
| Self { | ||
| engine, | ||
| tick_count: 0, | ||
| drained: true, | ||
| registry, | ||
| } |
There was a problem hiding this comment.
with_engine() is assuming a fresh, already-correct engine.
It hard-codes tick_count = 0 and drained = true, and it also discards the result of register_rule(). Wrapping a pre-stepped engine can misreport the first head/drain, while a real registration failure is silently deferred to a later step() as a generic ENGINE_ERROR. Initialize the mirrored state from the injected Engine, and only swallow the specific duplicate-name case for sys/ack_pending.
| match self.engine.commit(tx) { | ||
| Ok(_snapshot) => { | ||
| self.tick_count += 1; | ||
| ticks_executed += 1; | ||
| self.drained = false; | ||
| } |
There was a problem hiding this comment.
Materialization failures are being dropped at the WASM boundary.
Engine::commit() can succeed with last_materialization_errors() populated. step() still returns success, and drain_view_ops() exposes only successful channels, so strict-channel conflicts turn into silent output loss over the ABI. These boundary errors need to be surfaced explicitly before this contract freezes.
Also applies to: 187-206
| fn snapshot_at(&mut self, tick: u64) -> Result<Vec<u8>, AbiError> { | ||
| let tick_index = usize::try_from(tick).map_err(|_| AbiError { | ||
| code: error_codes::INVALID_TICK, | ||
| message: format!("tick {tick} exceeds addressable range"), | ||
| })?; | ||
|
|
||
| // Save current state — jump_to_tick overwrites engine state with a | ||
| // replayed state. We must restore afterward to keep the live engine | ||
| // consistent with tick_count and subsequent operations. | ||
| let saved_state = self.engine.state().clone(); | ||
|
|
||
| self.engine.jump_to_tick(tick_index).map_err(|e| { | ||
| // Restore even on error (jump_to_tick may have partially mutated). | ||
| *self.engine.state_mut() = saved_state.clone(); | ||
| AbiError { | ||
| code: error_codes::INVALID_TICK, | ||
| message: e.to_string(), | ||
| } | ||
| })?; | ||
|
|
||
| let snap = self.engine.snapshot(); | ||
| let head = HeadInfo { | ||
| tick, | ||
| state_root: snap.state_root.to_vec(), | ||
| commit_id: snap.hash.to_vec(), | ||
| }; | ||
|
|
||
| // Restore live state. | ||
| *self.engine.state_mut() = saved_state; | ||
|
|
||
| echo_wasm_abi::encode_cbor(&head).map_err(|e| AbiError { | ||
| code: error_codes::CODEC_ERROR, | ||
| message: e.to_string(), | ||
| }) |
There was a problem hiding this comment.
Historical snapshot hashes are computed against live context.
jump_to_tick() only rewinds state; it does not rewind last_snapshot. The self.engine.snapshot() call here therefore hashes the requested historical state with the live head’s parent context. This also collapses replay corruption into INVALID_TICK. Either read the stored ledger snapshot directly, or rewind the full snapshot context before encoding.
🤖 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 213 - 246, snapshot_at
currently calls self.engine.jump_to_tick(tick) which only rewinds state but not
the snapshot context (last_snapshot), so engine.snapshot() produces a hash
against the live head; fix by obtaining the historical snapshot context instead
of hashing the live head: either read the stored ledger snapshot for that tick
directly (e.g. via the engine/ledger API that returns the saved
snapshot/HeadInfo for a given tick) and use that data for HeadInfo and encoding,
or fully rewind the engine's snapshot context (including last_snapshot/parent
pointers) to the requested tick before calling self.engine.snapshot(), then
restore the full snapshot context along with state after snapshot_at returns;
update snapshot_at to use engine's snapshot-at-tick API or to rewind/restore
last_snapshot in addition to state so the computed state_root/commit_id are
derived from the historical context, not the live head.
| ### DrainResponse | ||
|
|
||
| | Field | Type | Description | | ||
| | ---------- | ----- | --------------------- | | ||
| | `channels` | array | List of `ChannelData` | | ||
|
|
||
| ### ChannelData | ||
|
|
||
| | Field | Type | Description | | ||
| | ------------ | --------- | ---------------------------------- | | ||
| | `channel_id` | bytes(32) | Materialization channel identifier | | ||
| | `data` | bytes | Raw finalized channel output | |
There was a problem hiding this comment.
Expose materialization boundary errors in ABI v1.
DrainResponse only lets the host observe "channels" or "no channels". The engine can also commit a tick and still produce materialization finalization errors, which is semantically different from an empty drain. Without an explicit field/export for those boundary errors, hosts will read silent output loss as a normal empty response. As per coding guidelines: docs/**/*.md: Echo is a deterministic simulation engine. Documentation accuracy matters — especially anything touching determinism guarantees, hash stability, or canonical ordering.
Also applies to: 138-148
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/spec/SPEC-0009-wasm-abi-v1.md` around lines 106 - 117, DrainResponse
currently only exposes `channels` (or absence thereof) which hides
materialization boundary/finalization errors; update the ABI docs for
`DrainResponse` to add an explicit field (e.g., `boundary_errors` or
`materialization_errors`) that surfaces any per-channel or global finalization
errors (type: array of error records or bytes plus an error_code/message) so
hosts can distinguish an empty successful drain from a drain with silent output
loss; update the `ChannelData` section if needed to reference per-channel error
linking, and apply the same addition to the other affected block (lines around
the duplicate section noted at 138-148) so both places document the new error
field consistently.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/warp-wasm/src/lib.rs (1)
112-118:⚠️ Potential issue | 🟠 MajorNever return an empty
Uint8Arrayfrom the ABI boundary.If error encoding fails here, callers get undecodable bytes instead of the promised
{ ok: false, code, message }envelope. Use a pre-encoded minimalCODEC_ERRORenvelope as the last-resort fallback so every byte-returning export stays self-describing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/warp-wasm/src/lib.rs` around lines 112 - 118, The encode_err_raw function currently returns an empty Uint8Array on encode failure which produces undecodable bytes for callers; instead keep a pre-encoded minimal fallback (e.g. a const CODEC_ERROR: &[u8] representing { ok: false, code: <codec error code>, message: "codec error" }) and convert that to a Uint8Array in the Err arm; update encode_err_raw to call ErrEnvelope::new and echo_wasm_abi::encode_cbor as now, but on Err(_) return bytes_to_uint8array(CODEC_ERROR) so every export always returns a self-describing envelope (reference: function encode_err_raw, type ErrEnvelope, echo_wasm_abi::encode_cbor, and bytes_to_uint8array).
🤖 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-wasm/src/lib.rs`:
- Around line 62-88: with_kernel and with_kernel_ref currently hold a RefCell
borrow while calling user-provided KernelPort code which allows reentrant calls
to panic; fix by extracting the boxed kernel out of the TLS cell before invoking
f (use KERNEL.with(|cell| { let mut borrow = cell.borrow_mut(); let mut
kernel_box = borrow.take().ok_or_else(|| AbiError{...})?; let result =
f(kernel_box.as_mut()); *borrow = Some(kernel_box); result }) ), and likewise
for with_kernel_ref take the Box out, call f(kernel_box.as_ref()), then put it
back; if borrow.take() returns None because the kernel is already taken, return
an AbiError indicating reentrancy (use or add a
kernel_port::error_codes::REENTRANT constant) instead of letting a RefCell
panic.
- Around line 267-303: The three getters (get_codec_id, get_registry_version,
get_schema_sha256_hex) currently collapse kernel-not-initialized errors and
genuinely absent metadata into JsValue::NULL; change them so with_kernel_ref
error paths surface initialization failure separately (e.g., return a distinct
JsValue like JsValue::from_str("NOT_INITIALIZED") or JsValue::UNDEFINED) while
keeping NULL only for missing fields, and add rustdoc to each function (and any
shared helper such as with_kernel_ref) documenting the contract: NULL means
field absent, the distinct value means kernel not initialized; update references
to NOT_INITIALIZED (or add a single constant) so the three functions
consistently return the same sentinel on Err.
- Around line 169-177: The init() entrypoint must not panic: replace the
expect() on kernel.get_head() in init() so failures are converted into a
structured ABI error and returned instead of trapping; only call
install_kernel(Box::new(kernel)) after successfully obtaining the head. Update
init() to propagate or serialize an AbiError/Result (instead of returning
Uint8Array directly with a hard panic), mapping WarpKernel::new()/get_head()
failures into that error path so no unwrap/expect is used.
---
Duplicate comments:
In `@crates/warp-wasm/src/lib.rs`:
- Around line 112-118: The encode_err_raw function currently returns an empty
Uint8Array on encode failure which produces undecodable bytes for callers;
instead keep a pre-encoded minimal fallback (e.g. a const CODEC_ERROR: &[u8]
representing { ok: false, code: <codec error code>, message: "codec error" })
and convert that to a Uint8Array in the Err arm; update encode_err_raw to call
ErrEnvelope::new and echo_wasm_abi::encode_cbor as now, but on Err(_) return
bytes_to_uint8array(CODEC_ERROR) so every export always returns a
self-describing envelope (reference: function encode_err_raw, type ErrEnvelope,
echo_wasm_abi::encode_cbor, and bytes_to_uint8array).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33997c21-cdca-43c9-bca2-e622527833e6
📒 Files selected for processing (2)
CHANGELOG.mdcrates/warp-wasm/src/lib.rs
| fn with_kernel<F, R>(f: F) -> Result<R, AbiError> | ||
| where | ||
| F: FnOnce(&mut dyn KernelPort) -> Result<R, AbiError>, | ||
| { | ||
| KERNEL.with(|cell| { | ||
| let mut borrow = cell.borrow_mut(); | ||
| let kernel = borrow.as_mut().ok_or_else(|| AbiError { | ||
| code: kernel_port::error_codes::NOT_INITIALIZED, | ||
| message: "kernel not initialized; call init() first".into(), | ||
| })?; | ||
| f(kernel.as_mut()) | ||
| }) | ||
| } | ||
|
|
||
| /// Run a closure with an immutable reference to the installed kernel. | ||
| fn with_kernel_ref<F, R>(f: F) -> Result<R, AbiError> | ||
| where | ||
| F: FnOnce(&dyn KernelPort) -> Result<R, AbiError>, | ||
| { | ||
| KERNEL.with(|cell| { | ||
| let borrow = cell.borrow(); | ||
| let kernel = borrow.as_ref().ok_or_else(|| AbiError { | ||
| code: kernel_port::error_codes::NOT_INITIALIZED, | ||
| message: "kernel not initialized; call init() first".into(), | ||
| })?; | ||
| f(kernel.as_ref()) | ||
| }) |
There was a problem hiding this comment.
Make the TLS kernel access reentrancy-safe.
Both helpers keep the RefCell borrow alive while executing arbitrary KernelPort code. Because KernelPort is a public injection point, an implementation can reenter warp_wasm::{dispatch_intent, step, get_head, install_kernel, ...} and trigger a RefCell borrow panic. That breaks the crate-level "no panics" guarantee at the ABI boundary.
Take the boxed kernel out of TLS before invoking user code and put it back afterward, or detect reentrancy and turn it into an AbiError instead of panicking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/warp-wasm/src/lib.rs` around lines 62 - 88, with_kernel and
with_kernel_ref currently hold a RefCell borrow while calling user-provided
KernelPort code which allows reentrant calls to panic; fix by extracting the
boxed kernel out of the TLS cell before invoking f (use KERNEL.with(|cell| { let
mut borrow = cell.borrow_mut(); let mut kernel_box = borrow.take().ok_or_else(||
AbiError{...})?; let result = f(kernel_box.as_mut()); *borrow =
Some(kernel_box); result }) ), and likewise for with_kernel_ref take the Box
out, call f(kernel_box.as_ref()), then put it back; if borrow.take() returns
None because the kernel is already taken, return an AbiError indicating
reentrancy (use or add a kernel_port::error_codes::REENTRANT constant) instead
of letting a RefCell panic.
| pub fn init() -> Uint8Array { | ||
| #[cfg(feature = "engine")] | ||
| { | ||
| let kernel = warp_kernel::WarpKernel::new(); | ||
| #[allow(clippy::expect_used)] // Fresh kernel; infallible in practice. | ||
| let head = kernel | ||
| .get_head() | ||
| .expect("fresh kernel must have valid head"); | ||
| install_kernel(Box::new(kernel)); |
There was a problem hiding this comment.
Remove the panic path from init().
init() is a wasm entrypoint that advertises structured errors. Line 173 converts a kernel failure into a trap instead of an AbiError, so the ABI contract and the new changelog claim about "no panics" both become false on this path.
Suggested fix
#[cfg(feature = "engine")]
{
let kernel = warp_kernel::WarpKernel::new();
- #[allow(clippy::expect_used)] // Fresh kernel; infallible in practice.
- let head = kernel
- .get_head()
- .expect("fresh kernel must have valid head");
+ let head = match kernel.get_head() {
+ Ok(head) => head,
+ Err(err) => return encode_err(&err),
+ };
install_kernel(Box::new(kernel));
encode_ok(&head)
}Based on learnings: Library code must return Result or Option instead of panicking; unwrap() and expect() are forbidden in non-test code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/warp-wasm/src/lib.rs` around lines 169 - 177, The init() entrypoint
must not panic: replace the expect() on kernel.get_head() in init() so failures
are converted into a structured ABI error and returned instead of trapping; only
call install_kernel(Box::new(kernel)) after successfully obtaining the head.
Update init() to propagate or serialize an AbiError/Result (instead of returning
Uint8Array directly with a hard panic), mapping WarpKernel::new()/get_head()
failures into that error path so no unwrap/expect is used.
| /// Get the codec identifier from the installed registry. | ||
| #[wasm_bindgen] | ||
| pub fn get_codec_id() -> JsValue { | ||
| JsValue::NULL | ||
| let result = with_kernel_ref(|k| Ok(k.registry_info())); | ||
| match result { | ||
| Ok(info) => match info.codec_id { | ||
| Some(id) => JsValue::from_str(&id), | ||
| None => JsValue::NULL, | ||
| }, | ||
| Err(_) => JsValue::NULL, | ||
| } | ||
| } | ||
|
|
||
| /// Get the registry version from the installed registry. | ||
| #[wasm_bindgen] | ||
| pub fn get_registry_version() -> JsValue { | ||
| JsValue::NULL | ||
| let result = with_kernel_ref(|k| Ok(k.registry_info())); | ||
| match result { | ||
| Ok(info) => match info.registry_version { | ||
| Some(v) => JsValue::from_str(&v), | ||
| None => JsValue::NULL, | ||
| }, | ||
| Err(_) => JsValue::NULL, | ||
| } | ||
| } | ||
|
|
||
| /// Get the schema hash (hex) from the installed registry. | ||
| #[wasm_bindgen] | ||
| pub fn get_schema_sha256_hex() -> JsValue { | ||
| JsValue::NULL | ||
| let result = with_kernel_ref(|k| Ok(k.registry_info())); | ||
| match result { | ||
| Ok(info) => match info.schema_sha256_hex { | ||
| Some(h) => JsValue::from_str(&h), | ||
| None => JsValue::NULL, | ||
| }, | ||
| Err(_) => JsValue::NULL, | ||
| } |
There was a problem hiding this comment.
Do not collapse NOT_INITIALIZED and "field absent" into the same null.
These getters use JsValue::NULL for two different states: the kernel is missing, or the metadata field is genuinely absent. That makes host-side handshake failures indistinguishable from valid "no metadata" cases and diverges from the documented init-before-use contract for exports.
Either surface initialization failure separately, or reserve null only for absent fields and document that behavior explicitly in the rustdoc.
Based on learnings: Every public API across crates (warp-core, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/warp-wasm/src/lib.rs` around lines 267 - 303, The three getters
(get_codec_id, get_registry_version, get_schema_sha256_hex) currently collapse
kernel-not-initialized errors and genuinely absent metadata into JsValue::NULL;
change them so with_kernel_ref error paths surface initialization failure
separately (e.g., return a distinct JsValue like
JsValue::from_str("NOT_INITIALIZED") or JsValue::UNDEFINED) while keeping NULL
only for missing fields, and add rustdoc to each function (and any shared helper
such as with_kernel_ref) documenting the contract: NULL means field absent, the
distinct value means kernel not initialized; update references to
NOT_INITIALIZED (or add a single constant) so the three functions consistently
return the same sentinel on Err.
1 participant
Summary
echo-wasm-abi) — app-agnostic byte-level boundary contract for WASM host adapters, with ABI response DTOs, error codes, and CBOR wire envelope typeswarp-wasm,enginefeature) — wrapswarp-core::EngineimplementingKernelPort, with deterministic tick execution andsys/ack_pendingsystem ruleinit,dispatch_intent,step,drain_view_ops,get_head,snapshot_at,get_registry_info, and handshake metadata getters now return live CBOR-encoded data{ ok: true/false, ... }) for allUint8Arrayreturnsunsafe_graph, restructureTtdControllerWASM bindingsTest plan
cargo clippy --workspace --all-targets -- -D warnings— zero warningscargo test --workspace— all tests passcargo doc --no-deps -p warp-wasm— no broken intra-doc linksSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests