Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a storage subsystem and SDK storage APIs with examples and fuzzing, updates LLM provider JSON handling and interfaces, bumps several crate versions and metadata, adjusts CI/docs env handling, and adds tests, fuzz corpus, and packaging/gitignore updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Contract
participant SDK as "genlayer_sdk (storage handles)"
participant WASI as "WASI host imports"
participant Host as "Host storage backend"
Contract->>SDK: call storage handle (e.g., DynArray::append_slot / TreeMap::insert)
SDK->>WASI: call imported host function (__genvm_storage_read/write or storage_write/read)
WASI->>Host: perform low-level read/write (slot, offset, buf)
Host-->>WASI: return bytes/length
WASI-->>SDK: return result/status
SDK-->>Contract: materialized Handle/Value or result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/implementation/src/llm/providers.rs (2)
333-399:⚠️ Potential issue | 🟠 MajorKeep JSON mode validating the returned payload.
This now returns raw
message.contentverbatim. Becausemodules/implementation/src/llm/ctx.rsnow routesExtendedOutputFormat::JSONthrough this method, OpenAI-compatible backends can leak fenced JSON, leading prose, or otherwise non-parseable content to callers that explicitly asked for JSON.🛠️ Preserve the JSON contract while still returning text
let response = res .body .pointer("/choices/0/message/content") .and_then(|v| v.as_str()) .ok_or_else(|| anyhow::anyhow!("can't get response field {}", &res.body))?; - Ok(ProviderResponse::new(response.to_owned(), tokens)) + let json_str = sanitize_json_str(response); + serde_json::from_str::<serde_json::Map<String, serde_json::Value>>(&json_str) + .with_context(|| format!("parsing {json_str:?}"))?; + + Ok(ProviderResponse::new(json_str, tokens))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/implementation/src/llm/providers.rs` around lines 333 - 399, The method exec_prompt_json_as_text currently returns the raw message.content string, which can leak non-JSON text; update the code right after extracting response (in exec_prompt_json_as_text) to validate that response is valid JSON: attempt to parse response with serde_json::from_str::<serde_json::Value>(), return an error (or ModuleResult::Err) if parsing fails, and then pass the canonical serialized JSON string (or the parsed Value re-serialized if you prefer deterministic formatting) into ProviderResponse::new along with tokens; keep extract_openai_tokens and the rest unchanged.
113-121: 🛠️ Refactor suggestion | 🟠 MajorBreak the recursive fallback between the JSON helpers.
With this default body,
exec_prompt_json_as_text()now callsexec_prompt_json(), while the defaultexec_prompt_json()on Line 130 still callsexec_prompt_json_as_text(). Any future provider impl that overrides neither method will compile and then recurse until stack overflow on the first JSON request.♻️ One way to make the trait safe by default
#[async_trait::async_trait] pub trait Provider { async fn exec_prompt_text( &self, ctx: &scripting::CtxPart, prompt: &prompt::Internal, model: &str, ) -> ModuleResult<ProviderResponse<String>>; - async fn exec_prompt_json_as_text( - &self, - ctx: &scripting::CtxPart, - prompt: &prompt::Internal, - model: &str, - ) -> ModuleResult<ProviderResponse<String>> { - let res = self.exec_prompt_json(ctx, prompt, model).await?; - let serialized = serde_json::to_string(&res.result)?; - Ok(ProviderResponse::new(serialized, res.tokens)) - } + async fn exec_prompt_json_as_text( + &self, + ctx: &scripting::CtxPart, + prompt: &prompt::Internal, + model: &str, + ) -> ModuleResult<ProviderResponse<String>>; async fn exec_prompt_json( &self, ctx: &scripting::CtxPart, prompt: &prompt::Internal, model: &str,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/implementation/src/llm/providers.rs` around lines 113 - 121, The two default helpers are mutually recursive (exec_prompt_json_as_text calls exec_prompt_json while exec_prompt_json’s default calls exec_prompt_json_as_text), so break the cycle by choosing one primitive: make exec_prompt_json_as_text the primitive and change the default exec_prompt_json implementation to call exec_prompt_json_as_text and then serde_json::from_str the returned text into the JSON type (use the ProviderResponse<String> -> ProviderResponse<T> conversion pattern and preserve tokens), and remove or stop using exec_prompt_json inside exec_prompt_json_as_text so only exec_prompt_json (the JSON-returning helper) is implemented by delegating to the text primitive.
🧹 Nitpick comments (8)
tests/cases/stable/storage/storage_root_rust/storage_root_rust.jsonnet (1)
7-9: Consider using inline string for calldata.Same as the
storage_dyn_array_rust.jsonnetcase - the heredoc adds a trailing newline. Consider using inline"calldata": "{}"for consistency with the template's default format.♻️ Suggested change
simple.run('${jsonnetDir}/storage_root.wasm') { - "calldata": ||| - {} - |||, + "calldata": "{}", stable_hash: false, }])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cases/stable/storage/storage_root_rust/storage_root_rust.jsonnet` around lines 7 - 9, The heredoc used for the "calldata" field in tests/cases/stable/storage/storage_root_rust/storage_root_rust.jsonnet (the `"calldata": ||| {} |||` block) introduces a trailing newline; replace that heredoc with an inline JSON string `"calldata": "{}"` to match storage_dyn_array_rust.jsonnet and the template default so the value has no extra newline and remains consistent across tests.tests/cases/stable/storage/storage_dyn_array_rust/storage_dyn_array_rust.jsonnet (1)
7-9: Consider using inline string for calldata.The heredoc syntax
|||...|||adds a trailing newline to the calldata value, resulting in{}\ninstead of{}. The template's default uses"calldata": "{}"(inline string). While this may not cause issues if the consumer trims whitespace, using inline format would be more consistent with the template's default behavior.♻️ Suggested change
simple.run('${jsonnetDir}/storage_dyn_array.wasm') { - "calldata": ||| - {} - |||, + "calldata": "{}", stable_hash: false, }])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cases/stable/storage/storage_dyn_array_rust/storage_dyn_array_rust.jsonnet` around lines 7 - 9, The "calldata" heredoc currently produces a trailing newline ("{}\n"); replace the heredoc (|||...|||) with an inline JSON string value so "calldata" is exactly "{}" to match the template default and avoid extra whitespace. Locate the test case for storage_dyn_array_rust and update the "calldata" key to use an inline string value rather than the heredoc.tests/cases/stable/storage/storage_root_rust/prepare.py (1)
31-34: Consider using try/finally for intermediate file cleanup.If
wat2wasmfails (line 33), the intermediate.watfile will remain on disk. While minor for test infrastructure, wrapping in try/finally ensures cleanup:♻️ Suggested improvement
# Round-trip through wabt to normalize to MVP format subprocess.run(['wasm2wat', str(src), '-o', str(wat)], check=True) -subprocess.run(['wat2wasm', str(wat), '-o', str(dst)], check=True) -wat.unlink() +try: + subprocess.run(['wat2wasm', str(wat), '-o', str(dst)], check=True) +finally: + wat.unlink(missing_ok=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cases/stable/storage/storage_root_rust/prepare.py` around lines 31 - 34, The intermediate .wat file created in prepare.py is removed unconditionally after running wat2wasm, which leaves it on disk if wat2wasm fails; wrap the wasm2wat/wat2wasm calls in a try/finally (or try/except/finally) around the subprocess.run calls that create and consume the Path variable wat so that wat.unlink() is executed in the finally block (referencing the wat Path and the subprocess.run calls) to ensure the .wat is always cleaned up even on failure.executor/crates/sdk-rs/src/storage/array.rs (3)
26-29: Offset calculation could overflow for large arrays.The expression
idx as u32 * T::SIZEcould overflow ifNis large andT::SIZEis non-trivial. Since bounds checking passes, the overflow would produce incorrect storage access rather than a panic.Consider using
checked_mulwith explicit error handling for defensive robustness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor/crates/sdk-rs/src/storage/array.rs` around lines 26 - 29, The index method can compute a wrapped product when calculating the byte offset (idx as u32 * T::SIZE) which can silently overflow; update the offset calculation in Array::index to perform a checked multiplication (e.g., idx as u32 checked_mul T::SIZE) and handle the None case by panicking or returning a clear error before calling T::handle_at(self.slot, ...); ensure you still assert idx < N (or replace with a single validated path) and use the checked result to compute self.offset + checked_product to avoid incorrect memory access.
161-162:SIZE = u32::MAXas sentinel needs documentation.Using
u32::MAXas a sentinel for "unknown/variable size" is a reasonable pattern, but should be documented. Consumers (like therecord!macro) need to handle this case to prevent VLA from being embedded in fixed-layout records.📝 Suggested documentation
impl<T: StorageType> StorageType for VLA<T> { + /// Sentinel value indicating variable size. VLA cannot be embedded in + /// fixed-layout records; it must be the last field or use indirection. const SIZE: u32 = u32::MAX;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor/crates/sdk-rs/src/storage/array.rs` around lines 161 - 162, The impl StorageType for VLA<T> currently sets const SIZE: u32 = u32::MAX as a sentinel for variable/unknown length but lacks documentation and consumer handling; add a clear doc comment on the impl or the SIZE constant explaining that u32::MAX denotes a variable-length array and must not be treated as a fixed-size field, and update consumers (e.g., the record! macro and any layout/serialization code) to explicitly check for StorageType::SIZE == u32::MAX and reject or special-case VLA<T> embedding in fixed-layout records (provide a descriptive error or alternative handling path) so VLAs cannot be accidentally included in fixed-size record layouts.
76-86: Document thatappend_slotreturns uninitialized storage.The
append_slotmethod returns a handle to storage that may contain stale data from a previous value. This is fine for performance, but callers must be aware they need to initialize it.Also,
idx * T::SIZEat line 78 has the same potential overflow concern as fixed arrays.📝 Suggested documentation
- /// Grow by one and return a handle to the new (uninitialized) element. + /// Grow by one and return a handle to the new element slot. + /// + /// **Important**: The returned slot may contain stale data and must be + /// explicitly initialized by the caller before being read. pub fn append_slot(&self) -> T::Handle {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor/crates/sdk-rs/src/storage/array.rs` around lines 76 - 86, Document that append_slot returns an uninitialized handle and warn callers they'll need to initialize it: add a doc comment on pub fn append_slot(&self) -> T::Handle explaining the returned handle points to potentially stale/uninitialized storage and must be written before use; also add the same note (or a shared remark) for any APIs that expose raw handles from T::handle_at. Additionally, address the integer overflow risk in index and append_slot where idx * T::SIZE and len * T::SIZE are computed by documenting the assumption (e.g., that idx/len and T::SIZE fit in u32) or adding a checked cast/comment referencing T::SIZE, T::handle_at, slot.indirect, offset, len(), set_len() so future maintainers know the bounds/overflow constraints.executor/crates/sdk-rs/src/storage/tree_map.rs (2)
467-499: Consider iterative traversal for very large trees.The recursive in-order traversal is clean and correct. Since AVL trees guarantee O(log n) height, stack overflow is unlikely in practice. However, for maximum robustness with very large datasets, an iterative approach using an explicit stack could be considered in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor/crates/sdk-rs/src/storage/tree_map.rs` around lines 467 - 499, The current recursive in-order traversal in visit_impl can risk deep recursion for extremely large trees; replace visit_impl with an iterative in-order traversal using an explicit stack (Vec<u32>) to avoid recursion: start from self.root_idx().get(), loop pushing current idx and descending left via node(idx).left().get() until 0, then pop an idx, call f(self.node(idx)), then set current to node(idx).right().get() and continue until both stack is empty and current is 0; ensure the for_each_node and for_each callers remain unchanged and preserve the same handling of idx==0 and use of K::storage_get and V::storage_get when invoking f in for_each.
97-177: AVL rotation logic appears correct but is complex.The single and double rotation implementations follow standard AVL patterns. The balance factor updates handle both insertion (where child balance ≠ 0) and deletion (where child balance = 0) cases correctly.
Consider adding unit tests specifically for the rotation methods to ensure edge cases are covered, especially the balance factor updates after double rotations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@executor/crates/sdk-rs/src/storage/tree_map.rs` around lines 97 - 177, Add unit tests that exercise the AVL rotation helpers rot_left, rot_right, rot_right_left, and rot_left_right directly (or via small insertion/deletion sequences that force those rotations) to validate pointer rearrangements and balance factor outcomes, including edge cases where the middle node's balance == 0 (deletion-case) and >0/<0 (insertion-cases); ensure tests assert the parent/child links (left/right) and balances for nodes gpar, par, cur after each rotation to catch regressions in the balance updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/bcfe859de59988b8007acbcf91bb8da8b39e6582a3335fc378bf4d8ebcbcc8ee`:
- Around line 1-49: The file
bcfe859de59988b8007acbcf91bb8da8b39e6582a3335fc378bf4d8ebcbcc8ee in
inputs-gvm-tree-map is a fuzzer_stats run-report containing telemetry and
host-specific paths; remove it from the fuzz corpus so the inputs directory
contains only raw testcases, and put such run telemetry into ignored build
artifacts instead (e.g., delete the file from the repo, add a rule to ignore or
move fuzzer_stats-style files generated by afl to the artifacts or .gitignore,
and ensure any fuzzing scripts write runtime reports outside
inputs-gvm-tree-map).
In `@executor/crates/sdk-rs/src/storage/array.rs`:
- Around line 143-146: The index method can overflow when computing self.offset
+ 4 + idx * T::SIZE; update executor/crates/sdk-rs/src/storage/array.rs inside
the index function to perform checked arithmetic (e.g., use checked_mul for idx
* T::SIZE and checked_add for subsequent additions) and assert or return a clear
error if any checked operation returns None; keep the existing bounds check
using self.len(), and reference T::SIZE, self.offset, idx and
T::handle_at/self.slot when adding the overflow checks so you only call
T::handle_at with a validated, non-overflowing offset.
- Around line 32-33: The current impl for StorageType on [T; N] computes SIZE as
T::SIZE * N as u32 which can overflow for large N; change the const SIZE
calculation to perform a checked multiplication using a wider integer and fail
at compile time on overflow (e.g. compute (T::SIZE as u64).checked_mul(N as
u64).expect("array SIZE overflow") and cast back to u32) or add a const
assertion that the product fits in u32, updating the StorageType impl for [T; N]
and the constant SIZE to use the checked operation or const_assert to prevent
silent wraparound.
In `@executor/crates/sdk-rs/src/storage/core.rs`:
- Around line 251-268: The current slice and index funcs operate on raw bytes
and can break UTF-8 sequences: update src methods to either be explicitly
byte-level or become character-aware; specifically, rename slice -> slice_bytes
and index -> byte_at (and update all callsites and docs/tests) if you intend to
keep byte semantics, or implement UTF-8-safe variants by validating boundaries
with Rust's UTF-8 helpers (e.g., ensure start/end are char boundaries via
str::is_char_boundary or convert the whole buffer to &str before slicing) and
return char (or String) for character-aware APIs (add new methods like
slice_chars/char_at and keep existing byte APIs or deprecate them). Ensure the
functions that use self.slot.indirect(self.offset).read validate/convert the
read bytes to a valid UTF-8 str before returning and update comments/tests
accordingly.
In `@runners/genlayer-py-std/src/genlayer/nondet/__init__.py`:
- Around line 112-123: Rename the local variable named format to response_format
to avoid shadowing the built-in and update all uses in this block (the dict
passed to gl_call.gl_call_generic and the conditional selecting
_decode_nondet_json vs _decode_nondet); add a runtime validation after reading
response_format = config.get('response_format', 'text') that checks
response_format is one of the supported values (e.g., 'json' or 'text') and
raise a ValueError with a clear message if not to fail fast.
- Around line 43-45: _decode_nondet_json currently calls json.loads directly and
will raise json.JSONDecodeError; wrap the json.loads call in a try/except that
catches JSONDecodeError (and any other decoding errors) and re-raises a
NondetException (the same error type produced by _decode_nondet) so callers
using _decode_nondet_json as a decoder (see its usage as decoder at line 122)
always receive NondetException; reference the functions _decode_nondet_json,
_decode_nondet and the NondetException class when applying the change.
In `@support/ci/pipelines/docs.sh`:
- Around line 6-7: The export of COPYRIGHT_YEAR currently masks failures because
it's done inline: export COPYRIGHT_YEAR=$(date -d "$DATE" +%Y); instead split
the steps so failures propagate: first compute DATE with DATE=$(git log -1
--format=%as) and then compute YEAR=$(date -d "$DATE" +%Y) checking the command
exit status, and only then export COPYRIGHT_YEAR="$YEAR" (or compute YEAR
directly from git and export) so any failure in date/git will cause the CI to
fail; update references to the symbols COPYRIGHT_YEAR and DATE in
support/ci/pipelines/docs.sh accordingly.
---
Outside diff comments:
In `@modules/implementation/src/llm/providers.rs`:
- Around line 333-399: The method exec_prompt_json_as_text currently returns the
raw message.content string, which can leak non-JSON text; update the code right
after extracting response (in exec_prompt_json_as_text) to validate that
response is valid JSON: attempt to parse response with
serde_json::from_str::<serde_json::Value>(), return an error (or
ModuleResult::Err) if parsing fails, and then pass the canonical serialized JSON
string (or the parsed Value re-serialized if you prefer deterministic
formatting) into ProviderResponse::new along with tokens; keep
extract_openai_tokens and the rest unchanged.
- Around line 113-121: The two default helpers are mutually recursive
(exec_prompt_json_as_text calls exec_prompt_json while exec_prompt_json’s
default calls exec_prompt_json_as_text), so break the cycle by choosing one
primitive: make exec_prompt_json_as_text the primitive and change the default
exec_prompt_json implementation to call exec_prompt_json_as_text and then
serde_json::from_str the returned text into the JSON type (use the
ProviderResponse<String> -> ProviderResponse<T> conversion pattern and preserve
tokens), and remove or stop using exec_prompt_json inside
exec_prompt_json_as_text so only exec_prompt_json (the JSON-returning helper) is
implemented by delegating to the text primitive.
---
Nitpick comments:
In `@executor/crates/sdk-rs/src/storage/array.rs`:
- Around line 26-29: The index method can compute a wrapped product when
calculating the byte offset (idx as u32 * T::SIZE) which can silently overflow;
update the offset calculation in Array::index to perform a checked
multiplication (e.g., idx as u32 checked_mul T::SIZE) and handle the None case
by panicking or returning a clear error before calling T::handle_at(self.slot,
...); ensure you still assert idx < N (or replace with a single validated path)
and use the checked result to compute self.offset + checked_product to avoid
incorrect memory access.
- Around line 161-162: The impl StorageType for VLA<T> currently sets const
SIZE: u32 = u32::MAX as a sentinel for variable/unknown length but lacks
documentation and consumer handling; add a clear doc comment on the impl or the
SIZE constant explaining that u32::MAX denotes a variable-length array and must
not be treated as a fixed-size field, and update consumers (e.g., the record!
macro and any layout/serialization code) to explicitly check for
StorageType::SIZE == u32::MAX and reject or special-case VLA<T> embedding in
fixed-layout records (provide a descriptive error or alternative handling path)
so VLAs cannot be accidentally included in fixed-size record layouts.
- Around line 76-86: Document that append_slot returns an uninitialized handle
and warn callers they'll need to initialize it: add a doc comment on pub fn
append_slot(&self) -> T::Handle explaining the returned handle points to
potentially stale/uninitialized storage and must be written before use; also add
the same note (or a shared remark) for any APIs that expose raw handles from
T::handle_at. Additionally, address the integer overflow risk in index and
append_slot where idx * T::SIZE and len * T::SIZE are computed by documenting
the assumption (e.g., that idx/len and T::SIZE fit in u32) or adding a checked
cast/comment referencing T::SIZE, T::handle_at, slot.indirect, offset, len(),
set_len() so future maintainers know the bounds/overflow constraints.
In `@executor/crates/sdk-rs/src/storage/tree_map.rs`:
- Around line 467-499: The current recursive in-order traversal in visit_impl
can risk deep recursion for extremely large trees; replace visit_impl with an
iterative in-order traversal using an explicit stack (Vec<u32>) to avoid
recursion: start from self.root_idx().get(), loop pushing current idx and
descending left via node(idx).left().get() until 0, then pop an idx, call
f(self.node(idx)), then set current to node(idx).right().get() and continue
until both stack is empty and current is 0; ensure the for_each_node and
for_each callers remain unchanged and preserve the same handling of idx==0 and
use of K::storage_get and V::storage_get when invoking f in for_each.
- Around line 97-177: Add unit tests that exercise the AVL rotation helpers
rot_left, rot_right, rot_right_left, and rot_left_right directly (or via small
insertion/deletion sequences that force those rotations) to validate pointer
rearrangements and balance factor outcomes, including edge cases where the
middle node's balance == 0 (deletion-case) and >0/<0 (insertion-cases); ensure
tests assert the parent/child links (left/right) and balances for nodes gpar,
par, cur after each rotation to catch regressions in the balance updates.
In
`@tests/cases/stable/storage/storage_dyn_array_rust/storage_dyn_array_rust.jsonnet`:
- Around line 7-9: The "calldata" heredoc currently produces a trailing newline
("{}\n"); replace the heredoc (|||...|||) with an inline JSON string value so
"calldata" is exactly "{}" to match the template default and avoid extra
whitespace. Locate the test case for storage_dyn_array_rust and update the
"calldata" key to use an inline string value rather than the heredoc.
In `@tests/cases/stable/storage/storage_root_rust/prepare.py`:
- Around line 31-34: The intermediate .wat file created in prepare.py is removed
unconditionally after running wat2wasm, which leaves it on disk if wat2wasm
fails; wrap the wasm2wat/wat2wasm calls in a try/finally (or try/except/finally)
around the subprocess.run calls that create and consume the Path variable wat so
that wat.unlink() is executed in the finally block (referencing the wat Path and
the subprocess.run calls) to ensure the .wat is always cleaned up even on
failure.
In `@tests/cases/stable/storage/storage_root_rust/storage_root_rust.jsonnet`:
- Around line 7-9: The heredoc used for the "calldata" field in
tests/cases/stable/storage/storage_root_rust/storage_root_rust.jsonnet (the
`"calldata": ||| {} |||` block) introduces a trailing newline; replace that
heredoc with an inline JSON string `"calldata": "{}"` to match
storage_dyn_array_rust.jsonnet and the template default so the value has no
extra newline and remains consistent across tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c4be691-5944-41a4-a144-3ce66312e33f
⛔ Files ignored due to path filters (5)
doc/website/sphinx-err-tm1s_x28.logis excluded by!**/*.logexecutor/Cargo.lockis excluded by!**/*.lockexecutor/crates/calldata/Cargo.lockis excluded by!**/*.lockexecutor/crates/sdk-rs/Cargo.lockis excluded by!**/*.lockmodules/implementation/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (66)
.gitignoredoc/website/src/conf.pyexecutor/Cargo.tomlexecutor/crates/calldata/Cargo.tomlexecutor/crates/calldata/LICENSE.txtexecutor/crates/sdk-rs/.ya-test-config.jsonexecutor/crates/sdk-rs/Cargo.tomlexecutor/crates/sdk-rs/examples/storage_dyn_array.rsexecutor/crates/sdk-rs/examples/storage_root.rsexecutor/crates/sdk-rs/fuzz/gvm-tree-map.rsexecutor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/06bf706b95ef039be4fd5d27876a80d246ac0b53689f743c91c59b2e07dce47dexecutor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/13626dcd6e9c9b80652a6ec6ff3e09583825cb8bd10216edac54b1b34fe71f84executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/15e75ab26bce0bbcec48076e85e1df4d5a8fe145d53a8f2e15469ddcbb997bc1executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/16ff5f865c8931620cbbfb4be603037909806fcdf5a07cbe1f4254372a99d942executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/28e459619a2eed99102830de6905611118f9a93477bf5b4ffda6f7bc3834c156executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/2f002715316a6584b162d7ef973363d231b76be0edca99799f2278e342c4fb0fexecutor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/36d59cebc7be1b8fdb0be89b737293504055334c3344ad6acafe0e75353f3000executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/3734dda7ac70a63554c58f2a6cddc2f1852cdcc838728c0c0144caabf6bf4afaexecutor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/405de5958f5e1a62339cb512b67619dba3432fbd73d47349af30198f47723cf2executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/40b64828f926a7a2a5550d0ced930df92944a8c47eac3d8f4686909ce3bf5fb6executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/4f52a1b5dad497fa91c08ba19d139f9af8fc934d3962f72bbb78dfdcdcb731d8executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/52cb07109bb8b897a0520be40dbdfa940b06c0c7b531e122e9b83b8ca00e2a1bexecutor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/5b965dc27ab7d44e4ae08567af3bac838a9381a8add883b349a623cd176dc19eexecutor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/692a7aef65a9540c332ccb0510816dd1974d1a70d61bb89461a495031609e0c6executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/6ecec5abea7c0910aa70611de047b8c1b693333bdde613d8c6438f3611209f44executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/729b0e7584815906122215442c1612166bd602f6cf60a69809b2ce1791535976executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/8426f8ad50f9f478c6400c3f00ff5503ddbe242ae2002e8a54087741255dca8dexecutor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/857da7e3f1e4c714a65b5fa191d6d626dc1362749c75a4cc64beda4a635ea8abexecutor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/a63bac3bd3da3270f434b81e778b953809c4d9d0243661f4f36c9db817848c80executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/a6b2a65760ebc6741c1dcbc53e416a261bf484cb8f38cf9affa114e7d7564236executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/b2b81cb269091ae713698d9e6603e18c1bcb68aefc6896ae7ad1cfd51b238f37executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/b684bac922ed878281605e6deb3fa777073cfa69f370d046fe9428d7d4928192executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/bcb43812d8a094ae43d4e05206fc0d62b10b8ccd1ccf09b2677513dcc3d0d2f3executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/bcfe859de59988b8007acbcf91bb8da8b39e6582a3335fc378bf4d8ebcbcc8eeexecutor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/bdb6ea571276879dbb982d9af337abd281a6d98fe2518bc2f1a94678a2016076executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/d59deb58e505f32c96bfdecc96c588452cd1f508890e8d78510b05e1fabf54a8executor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/e53faecf732b3682f891e75ce94848061194a6db05f22032cc4d03689785010dexecutor/crates/sdk-rs/fuzz/inputs-gvm-tree-map/fbd0d4accb2d7afc56dcf694df661a9adc2bee8025f0a7dc1fdc8050e98e64e3executor/crates/sdk-rs/src/abi/wasi.rsexecutor/crates/sdk-rs/src/lib.rsexecutor/crates/sdk-rs/src/storage/array.rsexecutor/crates/sdk-rs/src/storage/core.rsexecutor/crates/sdk-rs/src/storage/mod.rsexecutor/crates/sdk-rs/src/storage/record.rsexecutor/crates/sdk-rs/src/storage/tree_map.rsmodules/implementation/fuzz/features.txtmodules/implementation/src/llm/ctx.rsmodules/implementation/src/llm/providers.rsmodules/install/data/manifest.yamlmodules/interfaces/src/lib.rsrunners/genlayer-py-std/fuzz/resave.pyrunners/genlayer-py-std/src/genlayer/nondet/__init__.pyrunners/support/current/hashes.nixsupport/ci/pipelines/docs.shtests/cases/.gitignoretests/cases/semi-stable/llm/issue_288.0.stdouttests/cases/semi-stable/llm/issue_288.jsonnettests/cases/semi-stable/llm/issue_288.pytests/cases/stable/nondet/metod_det_get_webpage.0_0.hashtests/cases/stable/storage/storage_dyn_array_rust/prepare.pytests/cases/stable/storage/storage_dyn_array_rust/storage_dyn_array_rust.0.stdouttests/cases/stable/storage/storage_dyn_array_rust/storage_dyn_array_rust.jsonnettests/cases/stable/storage/storage_root_rust/prepare.pytests/cases/stable/storage/storage_root_rust/storage_root_rust.0.stdouttests/cases/stable/storage/storage_root_rust/storage_root_rust.jsonnettests/runner/ya_test_runner_plugins/cargo.py
💤 Files with no reviewable changes (2)
- modules/implementation/fuzz/features.txt
- modules/interfaces/src/lib.rs
Comment on lines
+32
to
+33
| impl<T: StorageType, const N: usize> StorageType for [T; N] { | ||
| const SIZE: u32 = T::SIZE * N as u32; |
Contributor
There was a problem hiding this comment.
Potential overflow in SIZE calculation for large arrays.
T::SIZE * N as u32 could overflow for very large N values, resulting in incorrect storage layout. Consider using checked_mul or adding a compile-time assertion.
🛡️ Suggested overflow check
impl<T: StorageType, const N: usize> StorageType for [T; N] {
- const SIZE: u32 = T::SIZE * N as u32;
+ const SIZE: u32 = {
+ // Compile-time overflow check
+ let size = T::SIZE as u64 * N as u64;
+ assert!(size <= u32::MAX as u64, "array size overflow");
+ size as u32
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| impl<T: StorageType, const N: usize> StorageType for [T; N] { | |
| const SIZE: u32 = T::SIZE * N as u32; | |
| impl<T: StorageType, const N: usize> StorageType for [T; N] { | |
| const SIZE: u32 = { | |
| // Compile-time overflow check | |
| let size = T::SIZE as u64 * N as u64; | |
| assert!(size <= u32::MAX as u64, "array size overflow"); | |
| size as u32 | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@executor/crates/sdk-rs/src/storage/array.rs` around lines 32 - 33, The
current impl for StorageType on [T; N] computes SIZE as T::SIZE * N as u32 which
can overflow for large N; change the const SIZE calculation to perform a checked
multiplication using a wider integer and fail at compile time on overflow (e.g.
compute (T::SIZE as u64).checked_mul(N as u64).expect("array SIZE overflow") and
cast back to u32) or add a const assertion that the product fits in u32,
updating the StorageType impl for [T; N] and the constant SIZE to use the
checked operation or const_assert to prevent silent wraparound.
Comment on lines
+143
to
+146
| pub fn index(&self, idx: u32) -> T::Handle { | ||
| assert!(idx < self.len(), "index out of bounds"); | ||
| T::handle_at(self.slot, self.offset + 4 + idx * T::SIZE) | ||
| } |
Contributor
There was a problem hiding this comment.
Multiple potential overflow points in VLA offset calculation.
Line 145 computes self.offset + 4 + idx * T::SIZE. For large idx values or large T::SIZE, either the multiplication or the subsequent additions could overflow, leading to incorrect storage access.
🛡️ Suggested defensive check
pub fn index(&self, idx: u32) -> T::Handle {
assert!(idx < self.len(), "index out of bounds");
- T::handle_at(self.slot, self.offset + 4 + idx * T::SIZE)
+ let element_offset = idx.checked_mul(T::SIZE)
+ .and_then(|v| v.checked_add(4))
+ .and_then(|v| self.offset.checked_add(v))
+ .expect("offset overflow");
+ T::handle_at(self.slot, element_offset)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn index(&self, idx: u32) -> T::Handle { | |
| assert!(idx < self.len(), "index out of bounds"); | |
| T::handle_at(self.slot, self.offset + 4 + idx * T::SIZE) | |
| } | |
| pub fn index(&self, idx: u32) -> T::Handle { | |
| assert!(idx < self.len(), "index out of bounds"); | |
| let element_offset = idx.checked_mul(T::SIZE) | |
| .and_then(|v| v.checked_add(4)) | |
| .and_then(|v| self.offset.checked_add(v)) | |
| .expect("offset overflow"); | |
| T::handle_at(self.slot, element_offset) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@executor/crates/sdk-rs/src/storage/array.rs` around lines 143 - 146, The
index method can overflow when computing self.offset + 4 + idx * T::SIZE; update
executor/crates/sdk-rs/src/storage/array.rs inside the index function to perform
checked arithmetic (e.g., use checked_mul for idx * T::SIZE and checked_add for
subsequent additions) and assert or return a clear error if any checked
operation returns None; keep the existing bounds check using self.len(), and
reference T::SIZE, self.offset, idx and T::handle_at/self.slot when adding the
overflow checks so you only call T::handle_at with a validated, non-overflowing
offset.
Comment on lines
+251
to
+268
| pub fn slice(&self, start: u32, end: u32) -> String { | ||
| assert!(start <= end && end <= self.len()); | ||
| let len = (end - start) as usize; | ||
| if len == 0 { | ||
| return String::new(); | ||
| } | ||
| let indirect = self.slot.indirect(self.offset); | ||
| let mut buf = vec![0u8; len]; | ||
| indirect.read(start, &mut buf); | ||
| String::from_utf8(buf).expect("invalid utf-8 in storage string slice") | ||
| } | ||
|
|
||
| pub fn index(&self, idx: u32) -> u8 { | ||
| assert!(idx < self.len(), "index out of bounds"); | ||
| let mut buf = [0u8; 1]; | ||
| self.slot.indirect(self.offset).read(idx, &mut buf); | ||
| buf[0] | ||
| } |
Contributor
There was a problem hiding this comment.
slice and index operate on bytes, not characters - may cause UTF-8 issues.
slice(start, end) reads arbitrary byte ranges and calls from_utf8, which will panic if the range splits a multi-byte UTF-8 character. Similarly, index(idx) returns a raw byte, which may be part of a multi-byte sequence.
Consider either:
- Documenting these as byte-level operations (rename to
slice_bytes/byte_at) - Adding character-aware slicing that validates boundaries
📝 Suggested documentation clarification
+ /// Returns a substring by **byte** indices. Panics if indices split a UTF-8 codepoint.
pub fn slice(&self, start: u32, end: u32) -> String {
assert!(start <= end && end <= self.len());
// ...
}
+ /// Returns the raw byte at the given **byte** index.
pub fn index(&self, idx: u32) -> u8 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@executor/crates/sdk-rs/src/storage/core.rs` around lines 251 - 268, The
current slice and index funcs operate on raw bytes and can break UTF-8
sequences: update src methods to either be explicitly byte-level or become
character-aware; specifically, rename slice -> slice_bytes and index -> byte_at
(and update all callsites and docs/tests) if you intend to keep byte semantics,
or implement UTF-8-safe variants by validating boundaries with Rust's UTF-8
helpers (e.g., ensure start/end are char boundaries via str::is_char_boundary or
convert the whole buffer to &str before slicing) and return char (or String) for
character-aware APIs (add new methods like slice_chars/char_at and keep existing
byte APIs or deprecate them). Ensure the functions that use
self.slot.indirect(self.offset).read validate/convert the read bytes to a valid
UTF-8 str before returning and update comments/tests accordingly.
Comment on lines
+43
to
+45
| def _decode_nondet_json(buf): | ||
| return json.loads(_decode_nondet(buf)) | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current exception-handling patterns around nondet decoding.
rg -nP '_decode_nondet_json|_decode_nondet|NondetException|JSONDecodeError' --type=py -C2Repository: genlayerlabs/genvm
Length of output: 5681
🏁 Script executed:
#!/bin/bash
# Check for all uses of _decode_nondet_json in the codebase
rg -n '_decode_nondet_json' --type=py -B2 -A2Repository: genlayerlabs/genvm
Length of output: 809
🏁 Script executed:
#!/bin/bash
# Get broader context around line 122 to understand the decoder usage pattern
sed -n '115,130p' runners/genlayer-py-std/src/genlayer/nondet/__init__.pyRepository: genlayerlabs/genvm
Length of output: 271
🏁 Script executed:
#!/bin/bash
# Verify the exact structure and line numbers of the target file
sed -n '36,45p' runners/genlayer-py-std/src/genlayer/nondet/__init__.py | cat -nRepository: genlayerlabs/genvm
Length of output: 352
Normalize JSON decode failures to NondetException.
On line 44, invalid JSON currently raises json.JSONDecodeError directly, which is inconsistent with the module's error contract where _decode_nondet normalizes all errors to NondetException. When _decode_nondet_json is used as a decoder at line 122, callers expecting NondetException will not catch malformed JSON responses. Wrap decode errors to keep exception handling consistent.
Proposed fix
def _decode_nondet_json(buf):
- return json.loads(_decode_nondet(buf))
+ try:
+ return json.loads(_decode_nondet(buf))
+ except json.JSONDecodeError as exc:
+ raise NondetException(f'Invalid JSON response: {exc}') from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@runners/genlayer-py-std/src/genlayer/nondet/__init__.py` around lines 43 -
45, _decode_nondet_json currently calls json.loads directly and will raise
json.JSONDecodeError; wrap the json.loads call in a try/except that catches
JSONDecodeError (and any other decoding errors) and re-raises a NondetException
(the same error type produced by _decode_nondet) so callers using
_decode_nondet_json as a decoder (see its usage as decoder at line 122) always
receive NondetException; reference the functions _decode_nondet_json,
_decode_nondet and the NondetException class when applying the change.
Comment on lines
+112
to
123
| format = config.get('response_format', 'text') | ||
|
|
||
| return gl_call.gl_call_generic( | ||
| { | ||
| 'ExecPrompt': { | ||
| 'prompt': prompt, | ||
| 'response_format': config.get('response_format', 'text'), | ||
| 'response_format': format, | ||
| 'images': images, | ||
| } | ||
| }, | ||
| _decode_nondet, | ||
| _decode_nondet_json if format == 'json' else _decode_nondet, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify callsites and response_format literals used across Python code.
rg -nP 'exec_prompt\s*\(.*response_format\s*=' --type=py -C2
rg -nP '\bformat\s*=' runners/genlayer-py-std/src/genlayer/nondet/__init__.pyRepository: genlayerlabs/genvm
Length of output: 544
🏁 Script executed:
#!/bin/bash
# Read the full function context around lines 112-123
sed -n '100,135p' runners/genlayer-py-std/src/genlayer/nondet/__init__.pyRepository: genlayerlabs/genvm
Length of output: 581
🏁 Script executed:
#!/bin/bash
# Find all exec_prompt calls to see what values are passed to response_format
rg -n 'exec_prompt' --type=py -B2 -A2Repository: genlayerlabs/genvm
Length of output: 6082
🏁 Script executed:
#!/bin/bash
# Check if there are any other usages of 'format' in the function or file
rg -n '\bformat\b' runners/genlayer-py-std/src/genlayer/nondet/__init__.pyRepository: genlayerlabs/genvm
Length of output: 215
Rename format to avoid builtin shadowing and add runtime validation for response_format.
Line 112 shadows Python's builtin format. The variable should be renamed to response_format, and validation should be added to reject unsupported values early. All three occurrences of the variable (lines 112, 118, 122) must be updated.
Proposed refactor
- format = config.get('response_format', 'text')
+ response_format = config.get('response_format', 'text')
+ if response_format not in ('text', 'json'):
+ raise ValueError("response_format must be 'text' or 'json'")
@@
- 'response_format': format,
+ 'response_format': response_format,
@@
- _decode_nondet_json if format == 'json' else _decode_nondet,
+ _decode_nondet_json if response_format == 'json' else _decode_nondet,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| format = config.get('response_format', 'text') | |
| return gl_call.gl_call_generic( | |
| { | |
| 'ExecPrompt': { | |
| 'prompt': prompt, | |
| 'response_format': config.get('response_format', 'text'), | |
| 'response_format': format, | |
| 'images': images, | |
| } | |
| }, | |
| _decode_nondet, | |
| _decode_nondet_json if format == 'json' else _decode_nondet, | |
| ) | |
| response_format = config.get('response_format', 'text') | |
| if response_format not in ('text', 'json'): | |
| raise ValueError("response_format must be 'text' or 'json'") | |
| return gl_call.gl_call_generic( | |
| { | |
| 'ExecPrompt': { | |
| 'prompt': prompt, | |
| 'response_format': response_format, | |
| 'images': images, | |
| } | |
| }, | |
| _decode_nondet_json if response_format == 'json' else _decode_nondet, | |
| ) |
🧰 Tools
🪛 Ruff (0.15.10)
[error] 112-112: Variable format is shadowing a Python builtin
(A001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@runners/genlayer-py-std/src/genlayer/nondet/__init__.py` around lines 112 -
123, Rename the local variable named format to response_format to avoid
shadowing the built-in and update all uses in this block (the dict passed to
gl_call.gl_call_generic and the conditional selecting _decode_nondet_json vs
_decode_nondet); add a runtime validation after reading response_format =
config.get('response_format', 'text') that checks response_format is one of the
supported values (e.g., 'json' or 'text') and raise a ValueError with a clear
message if not to fail fast.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
kp2pml30
force-pushed
the
feat/rust-sdk-storage
branch
from
8a8a27d to
88747a0
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
kp2pml30
force-pushed
the
feat/rust-sdk-storage
branch
from
88747a0 to
0ea62c2
Compare
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.
Closes GVM-206
Closes GVM-235
Summary by CodeRabbit
New Features
Bug Fixes
Chores