feat: rework manager api for cross-major calls - #8
Conversation
📝 WalkthroughWalkthroughThe executor now routes ChangesRuntime and protocol changes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CallContract
participant Host
participant NestedExecutor
CallContract->>Host: resolve_callcontract_executor
Host-->>CallContract: return routing payload
CallContract->>Host: run_nested with NestedRunEnvelope
Host->>NestedExecutor: forward nested execution
NestedExecutor-->>Host: return NestedRunReply
Host-->>CallContract: return nested reply
CallContract->>CallContract: validate reply and update small hash
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
30fcc9e to
96e7581
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
executor/src/wasi/genlayer_sdk/tests.rs (1)
166-178: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the other
nested_run_okrejection paths.
small_hash.len() != 32,ResultCode::InternalError, and the non-stringVmErrorpayload are all invariants worth pinning, since they guard cross-executor hash agreement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/wasi/genlayer_sdk/tests.rs` around lines 166 - 178, Expand the tests around nested_run_ok to cover each remaining rejection path: a small_hash whose length is not 32, a nested result with ResultCode::InternalError, and a VmError result carrying a non-string payload. Assert that nested_run_ok returns an error for each case, reusing the existing NestedRunReply setup and keeping the effect_free rejection test unchanged.executor/src/rt/vm/storage.rs (1)
485-494: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
check_majordoesn't useSelf/HS— consider a free function.Being an inherent associated fn of
Storage<HS>forces callers into an unrelated turbofish (Storage::<StorageHostHolder>::check_major(..)inexecutor/src/wasi/genlayer_sdk/run.rslines 148 and 501). A module-levelpub fn check_major(contract_major: u8)inrt::vm::storagewould read better and drop the bogus type parameter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/rt/vm/storage.rs` around lines 485 - 494, Move check_major out of the Storage<HS> inherent implementation into a module-level public function in rt::vm::storage, since it does not use Self or HS. Update all callers, including the usages in run.rs, to invoke the module function without the Storage::<...> turbofish while preserving the existing validation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@executor/src/wasi/genlayer_sdk/run.rs`:
- Around line 188-199: Update the nested protocol’s ResultCode::VmError handling
in the run flow to normalize and validate the materialized error code before
constructing public_abi::VmError. Reuse the existing strip_vm_error_detail and
validate_leader_vm_error helpers in this file, preserving the existing rejection
of non-string data and ensuring only canonical VM error codes reach the caller
and determinant fingerprint.
In `@support/scripts/check-source-text.py`:
- Around line 37-40: Extend the source-policy regexes in the check-source-text
logic to detect unqualified libc environment mutations, including direct calls,
imported symbols such as use libc::setenv, and aliased imports/calls for setenv,
unsetenv, and putenv. Add regression tests covering qualified, imported, and
aliased forms while preserving the existing env and libc-qualified matches.
---
Nitpick comments:
In `@executor/src/rt/vm/storage.rs`:
- Around line 485-494: Move check_major out of the Storage<HS> inherent
implementation into a module-level public function in rt::vm::storage, since it
does not use Self or HS. Update all callers, including the usages in run.rs, to
invoke the module function without the Storage::<...> turbofish while preserving
the existing validation behavior.
In `@executor/src/wasi/genlayer_sdk/tests.rs`:
- Around line 166-178: Expand the tests around nested_run_ok to cover each
remaining rejection path: a small_hash whose length is not 32, a nested result
with ResultCode::InternalError, and a VmError result carrying a non-string
payload. Assert that nested_run_ok returns an error for each case, reusing the
existing NestedRunReply setup and keeping the effect_free rejection test
unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e0b170c9-9185-4b4e-bc8c-0489152c49b3
⛔ Files ignored due to path filters (56)
executor/Cargo.lockis excluded by!**/*.lock,!**/*.lockexecutor/crates/common/Cargo.lockis excluded by!**/*.lock,!**/*.lockexecutor/crates/modules-interfaces/Cargo.lockis excluded by!**/*.lock,!**/*.locktests/integration/claude/AGENT_PROMPT.mdis excluded by!tests/**tests/integration/claude/README.mdis excluded by!tests/**tests/integration/claude/agent/float_math/README.mdis excluded by!tests/**tests/integration/claude/agent/float_math/contract.pyis excluded by!tests/**tests/integration/claude/agent/float_math/float_math.jsonnetis excluded by!tests/**tests/integration/claude/example/README.mdis excluded by!tests/**tests/integration/claude/example/example.jsonnetis excluded by!tests/**tests/integration/claude/intelligence/EXPLORED_PATHS.mdis excluded by!tests/**tests/integration/claude/intelligence/TODO.mdis excluded by!tests/**tests/integration/claude/run-manager.shis excluded by!tests/**tests/integration/claude/run_test.pyis excluded by!tests/**tests/integration/exploit/disagree_in_sandbox/disagree_in_sandbox.jsonnetis excluded by!tests/**tests/integration/exploit/unservable_major/unservable_major.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.0_0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.1.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.1_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.1_0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.jsonnetis excluded by!tests/**tests/integration/exploit/unservable_major/unservable_major_from.pyis excluded by!tests/**tests/integration/exploit/unservable_major/unservable_major_to.pyis excluded by!tests/**tests/integration/hello-world/hello_world/hello_world.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/hello-world/hello_world/hello_world.jsonnetis excluded by!tests/**tests/integration/hello-world/hello_world/hello_world_trivial.pyis excluded by!tests/**tests/integration/hello-world/hello_world_class_nondet/hello_world_class_nondet.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/hello-world/hello_world_class_nondet/hello_world_class_nondet.jsonnetis excluded by!tests/**tests/integration/hello-world/hello_world_class_nondet/hello_world_class_nondet.pyis excluded by!tests/**tests/integration/message/use_balance_zero_budget/use_balance_zero_budget.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/message/use_balance_zero_budget/use_balance_zero_budget.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/message/use_balance_zero_budget/use_balance_zero_budget.jsonnetis excluded by!tests/**tests/integration/message/use_balance_zero_budget/use_balance_zero_budget.pyis excluded by!tests/**tests/integration/misc/routed_call/routed_call.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/misc/routed_call/routed_call.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/misc/routed_call/routed_call.0_0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/misc/routed_call/routed_call.jsonnetis excluded by!tests/**tests/integration/misc/routed_call/routed_call_from.pyis excluded by!tests/**tests/integration/misc/routed_call/routed_call_to.pyis excluded by!tests/**tests/integration/nasty-determinism/floats/contract.pyis excluded by!tests/**tests/integration/nasty-determinism/floats/floats.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/nasty-determinism/floats/floats.jsonnetis excluded by!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0_1.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0_2.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.jsonnetis excluded by!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.pyis excluded by!tests/**tests/integration/runner/major_mismatch/major_mismatch.3.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/major_mismatch/major_mismatch.3_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/major_mismatch/major_mismatch.jsonnetis excluded by!tests/**tests/integration/storage/hello_world_class/hello_world_class.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/storage/hello_world_class/hello_world_class.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/storage/hello_world_class/hello_world_class.jsonnetis excluded by!tests/**tests/integration/storage/hello_world_class/hello_world_class.pyis excluded by!tests/**
📒 Files selected for processing (31)
.genvm-tool.py.gitignoreexecutor/Cargo.tomlexecutor/codegen/data/host-fns.jsonexecutor/codegen/data/public-abi.jsonexecutor/crates/common/src/expr/evaluator.rsexecutor/crates/common/src/expr/lexer.rsexecutor/crates/common/src/expr/tokenizer.rsexecutor/crates/common/src/expr/value.rsexecutor/crates/common/src/host_fns.rsexecutor/crates/common/src/lib.rsexecutor/crates/common/tests/expr.rsexecutor/crates/modules-interfaces/Cargo.tomlexecutor/src/exe/precompile.rsexecutor/src/exe/run.rsexecutor/src/host/mod.rsexecutor/src/lib.rsexecutor/src/rt/memlimiter.rsexecutor/src/rt/mod.rsexecutor/src/rt/supervisor/actions.rsexecutor/src/rt/supervisor/mod.rsexecutor/src/rt/vm/mod.rsexecutor/src/rt/vm/storage.rsexecutor/src/wasi/genlayer_sdk/message.rsexecutor/src/wasi/genlayer_sdk/mod.rsexecutor/src/wasi/genlayer_sdk/run.rsexecutor/src/wasi/genlayer_sdk/tests.rsexecutor/src/wasi/mod.rsexecutor/tests/precompile_lookup.rsflake.nixsupport/scripts/check-source-text.py
💤 Files with no reviewable changes (3)
- executor/codegen/data/host-fns.json
- executor/crates/common/src/host_fns.rs
- executor/src/wasi/mod.rs
| let run_ok = match reply.result.kind { | ||
| genvm_modules_interfaces::ResultCode::Return => rt::vm::RunOk::Return(reply.result.data), | ||
| genvm_modules_interfaces::ResultCode::UserError => { | ||
| rt::vm::RunOk::UserError(reply.result.data) | ||
| } | ||
| genvm_modules_interfaces::ResultCode::VmError => { | ||
| let data = reply.result.data.materialize()?; | ||
| let calldata::Value::Str(code) = data else { | ||
| anyhow::bail!("nested CallContract VM error is not a string"); | ||
| }; | ||
| rt::vm::RunOk::VMError(public_abi::VmError(std::borrow::Cow::Owned(code)), None) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Nested VmError codes are trusted verbatim.
The in-process/leader paths normalize and validate VM error codes (strip_vm_error_detail / validate_leader_vm_error in this file), but a code arriving over the nested protocol is wrapped into public_abi::VmError unchecked. An unknown or detail-carrying string then reaches the caller and the det fingerprint, and can trip the debug_assert! in strip_vm_error_detail if it later flows through that path. Consider validating/stripping here too.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/src/wasi/genlayer_sdk/run.rs` around lines 188 - 199, Update the
nested protocol’s ResultCode::VmError handling in the run flow to normalize and
validate the materialized error code before constructing public_abi::VmError.
Reuse the existing strip_vm_error_detail and validate_leader_vm_error helpers in
this file, preserving the existing rejection of non-string data and ensuring
only canonical VM error codes reach the caller and determinant fingerprint.
| re.compile(r'\benv::(?:set_var|remove_var)\b'), | ||
| re.compile(r'\blibc::(?:setenv|unsetenv|putenv)\b'), | ||
| # ... including when imported, which hides the `env::` prefix at the call. | ||
| re.compile(r'\buse\s+(?:std::)?env::\{?[^;]*\b(?:set_var|remove_var)\b'), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cover imported and unqualified libc environment mutations.
The libc pattern only matches libc::setenv, libc::unsetenv, and libc::putenv. It misses common forms such as use libc::setenv; setenv(...) and direct setenv(...) calls, so prohibited environment mutation can bypass this source-policy check. Add matching patterns and regression tests for qualified, imported, and aliased calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@support/scripts/check-source-text.py` around lines 37 - 40, Extend the
source-policy regexes in the check-source-text logic to detect unqualified libc
environment mutations, including direct calls, imported symbols such as use
libc::setenv, and aliased imports/calls for setenv, unsetenv, and putenv. Add
regression tests covering qualified, imported, and aliased forms while
preserving the existing env and libc-qualified matches.
96e7581 to
42b1524
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
executor/src/rt/vm/storage.rs (1)
485-494: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
check_majordoesn't touchselforHS. Keeping it inside the generic impl forces callers intoStorage::<StorageHostHolder>::check_major(..)turbofish; a free function in this module would read better.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/rt/vm/storage.rs` around lines 485 - 494, Move check_major out of the generic Storage implementation into a module-level free function, since it does not use self or HS. Preserve its existing version comparison and error behavior, allowing callers to invoke it without Storage::<StorageHostHolder> turbofish syntax.executor/src/wasi/genlayer_sdk/tests.rs (1)
171-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the other
nested_run_okinvariants. Onlyeffect_freeis tested; the 32-bytesmall_hashguard, theVmError-not-a-string bail, and theInternalErrorbail are untested despite being the paths that shape the det fingerprint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/wasi/genlayer_sdk/tests.rs` around lines 171 - 183, Extend the nested_run_ok tests beyond nested_result_must_be_effect_free to cover the remaining invariants: reject an invalid small_hash length, bail when a VmError is not represented as a string, and bail for InternalError results. Construct replies that isolate each condition and assert the expected error outcome, preserving the existing effect_free coverage.executor/tests/precompile_lookup.rs (1)
44-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTemp dir leaks if an assertion fails.
remove_dir_allonly runs on the success path; a guard type (ortempfile::TempDir, already common in Rust test setups) would clean up on unwind too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/tests/precompile_lookup.rs` around lines 44 - 62, Update the test setup in custom_runner_is_not_in_the_registry so temporary-directory cleanup occurs during both successful completion and assertion unwinding. Replace the manually retained root path and final remove_dir_all call with the existing guard-based approach, such as tempfile::TempDir, while preserving the current registry assertions.executor/src/exe/precompile.rs (1)
45-55: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClose the temp file before renaming, and consider fsyncing the parent directory.
fileis still open whenstd::fs::renameruns (fails on Windows with a sharing violation); and without an fsync of the containing directory the rename itself isn't durable across a crash, which is the failure this helper targets.🛠️ Proposed adjustment
let write_tmp = || -> std::io::Result<()> { - let mut file = std::fs::File::create(&tmp_path)?; - file.write_all(data)?; - file.sync_all()?; - std::fs::rename(&tmp_path, path) + { + let mut file = std::fs::File::create(&tmp_path)?; + file.write_all(data)?; + file.sync_all()?; + } + std::fs::rename(&tmp_path, path)?; + if let Some(dir) = path.parent() { + let _ = std::fs::File::open(dir).and_then(|d| d.sync_all()); + } + Ok(()) };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/exe/precompile.rs` around lines 45 - 55, Update the write_tmp closure to explicitly drop the file after sync_all and before std::fs::rename, ensuring Windows-compatible renaming. Then fsync the containing parent directory after the rename, propagating any resulting error so the atomic write is durable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@executor/src/rt/vm/mod.rs`:
- Around line 326-331: Update the RunOk::VMError branch of the small_hash call
to construct a materialized calldata::Value::Str from the VM error code,
matching the FullResult::data conversion and ensuring hashing is independent of
the outer Maybe representation.
---
Nitpick comments:
In `@executor/src/exe/precompile.rs`:
- Around line 45-55: Update the write_tmp closure to explicitly drop the file
after sync_all and before std::fs::rename, ensuring Windows-compatible renaming.
Then fsync the containing parent directory after the rename, propagating any
resulting error so the atomic write is durable.
In `@executor/src/rt/vm/storage.rs`:
- Around line 485-494: Move check_major out of the generic Storage
implementation into a module-level free function, since it does not use self or
HS. Preserve its existing version comparison and error behavior, allowing
callers to invoke it without Storage::<StorageHostHolder> turbofish syntax.
In `@executor/src/wasi/genlayer_sdk/tests.rs`:
- Around line 171-183: Extend the nested_run_ok tests beyond
nested_result_must_be_effect_free to cover the remaining invariants: reject an
invalid small_hash length, bail when a VmError is not represented as a string,
and bail for InternalError results. Construct replies that isolate each
condition and assert the expected error outcome, preserving the existing
effect_free coverage.
In `@executor/tests/precompile_lookup.rs`:
- Around line 44-62: Update the test setup in
custom_runner_is_not_in_the_registry so temporary-directory cleanup occurs
during both successful completion and assertion unwinding. Replace the manually
retained root path and final remove_dir_all call with the existing guard-based
approach, such as tempfile::TempDir, while preserving the current registry
assertions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2771bc4c-377b-421b-b683-056c2dde06ae
⛔ Files ignored due to path filters (56)
executor/Cargo.lockis excluded by!**/*.lock,!**/*.lockexecutor/crates/common/Cargo.lockis excluded by!**/*.lock,!**/*.lockexecutor/crates/modules-interfaces/Cargo.lockis excluded by!**/*.lock,!**/*.locktests/integration/claude/AGENT_PROMPT.mdis excluded by!tests/**tests/integration/claude/README.mdis excluded by!tests/**tests/integration/claude/agent/float_math/README.mdis excluded by!tests/**tests/integration/claude/agent/float_math/contract.pyis excluded by!tests/**tests/integration/claude/agent/float_math/float_math.jsonnetis excluded by!tests/**tests/integration/claude/example/README.mdis excluded by!tests/**tests/integration/claude/example/example.jsonnetis excluded by!tests/**tests/integration/claude/intelligence/EXPLORED_PATHS.mdis excluded by!tests/**tests/integration/claude/intelligence/TODO.mdis excluded by!tests/**tests/integration/claude/run-manager.shis excluded by!tests/**tests/integration/claude/run_test.pyis excluded by!tests/**tests/integration/exploit/disagree_in_sandbox/disagree_in_sandbox.jsonnetis excluded by!tests/**tests/integration/exploit/unservable_major/unservable_major.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.0_0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.1.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.1_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.1_0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.jsonnetis excluded by!tests/**tests/integration/exploit/unservable_major/unservable_major_from.pyis excluded by!tests/**tests/integration/exploit/unservable_major/unservable_major_to.pyis excluded by!tests/**tests/integration/hello-world/hello_world/hello_world.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/hello-world/hello_world/hello_world.jsonnetis excluded by!tests/**tests/integration/hello-world/hello_world/hello_world_trivial.pyis excluded by!tests/**tests/integration/hello-world/hello_world_class_nondet/hello_world_class_nondet.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/hello-world/hello_world_class_nondet/hello_world_class_nondet.jsonnetis excluded by!tests/**tests/integration/hello-world/hello_world_class_nondet/hello_world_class_nondet.pyis excluded by!tests/**tests/integration/message/use_balance_zero_budget/use_balance_zero_budget.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/message/use_balance_zero_budget/use_balance_zero_budget.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/message/use_balance_zero_budget/use_balance_zero_budget.jsonnetis excluded by!tests/**tests/integration/message/use_balance_zero_budget/use_balance_zero_budget.pyis excluded by!tests/**tests/integration/misc/routed_call/routed_call.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/misc/routed_call/routed_call.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/misc/routed_call/routed_call.0_0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/misc/routed_call/routed_call.jsonnetis excluded by!tests/**tests/integration/misc/routed_call/routed_call_from.pyis excluded by!tests/**tests/integration/misc/routed_call/routed_call_to.pyis excluded by!tests/**tests/integration/nasty-determinism/floats/contract.pyis excluded by!tests/**tests/integration/nasty-determinism/floats/floats.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/nasty-determinism/floats/floats.jsonnetis excluded by!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0_1.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0_2.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.jsonnetis excluded by!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.pyis excluded by!tests/**tests/integration/runner/major_mismatch/major_mismatch.3.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/major_mismatch/major_mismatch.3_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/major_mismatch/major_mismatch.jsonnetis excluded by!tests/**tests/integration/storage/hello_world_class/hello_world_class.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/storage/hello_world_class/hello_world_class.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/storage/hello_world_class/hello_world_class.jsonnetis excluded by!tests/**tests/integration/storage/hello_world_class/hello_world_class.pyis excluded by!tests/**
📒 Files selected for processing (31)
.genvm-tool.py.gitignoreexecutor/Cargo.tomlexecutor/codegen/data/host-fns.jsonexecutor/codegen/data/public-abi.jsonexecutor/crates/common/src/expr/evaluator.rsexecutor/crates/common/src/expr/lexer.rsexecutor/crates/common/src/expr/tokenizer.rsexecutor/crates/common/src/expr/value.rsexecutor/crates/common/src/host_fns.rsexecutor/crates/common/src/lib.rsexecutor/crates/common/tests/expr.rsexecutor/crates/modules-interfaces/Cargo.tomlexecutor/src/exe/precompile.rsexecutor/src/exe/run.rsexecutor/src/host/mod.rsexecutor/src/lib.rsexecutor/src/rt/memlimiter.rsexecutor/src/rt/mod.rsexecutor/src/rt/supervisor/actions.rsexecutor/src/rt/supervisor/mod.rsexecutor/src/rt/vm/mod.rsexecutor/src/rt/vm/storage.rsexecutor/src/wasi/genlayer_sdk/message.rsexecutor/src/wasi/genlayer_sdk/mod.rsexecutor/src/wasi/genlayer_sdk/run.rsexecutor/src/wasi/genlayer_sdk/tests.rsexecutor/src/wasi/mod.rsexecutor/tests/precompile_lookup.rsflake.nixsupport/scripts/check-source-text.py
💤 Files with no reviewable changes (3)
- executor/codegen/data/host-fns.json
- executor/crates/common/src/host_fns.rs
- executor/src/wasi/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- flake.nix
- .gitignore
- executor/codegen/data/public-abi.json
- executor/src/wasi/genlayer_sdk/message.rs
42b1524 to
88c5b00
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
executor/src/rt/supervisor/actions.rs (1)
177-181: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftAdd regression coverage for the runner-resolution and precompile trust boundary.
Cover these cases:
- An absent built-in returns
malformed_runnerwith the canonicalname:hash_gvm32message.- A
custom:<hash>runner skips native precompiled loading and uses normal compilation.- A missing non-deterministic artifact returns
Ok(None).These tests protect the manager error contract and prevent unsafe native artifact loading from regressing.
Also applies to: 789-798, 819-819
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@executor/src/rt/supervisor/actions.rs` around lines 177 - 181, Add regression tests covering runner resolution and precompile trust-boundary behavior: verify an absent built-in returns a malformed_runner error with the canonical name:hash_gvm32 message, custom:<hash> bypasses native precompiled loading and uses normal compilation, and a missing non-deterministic artifact returns Ok(None). Place the coverage around the runner-resolution and precompile-loading paths in the supervisor actions implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@executor/src/rt/supervisor/actions.rs`:
- Around line 177-181: Add regression tests covering runner resolution and
precompile trust-boundary behavior: verify an absent built-in returns a
malformed_runner error with the canonical name:hash_gvm32 message, custom:<hash>
bypasses native precompiled loading and uses normal compilation, and a missing
non-deterministic artifact returns Ok(None). Place the coverage around the
runner-resolution and precompile-loading paths in the supervisor actions
implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8eb0f9d3-14fe-4a45-8df4-f365b3711e2b
⛔ Files ignored due to path filters (28)
executor/Cargo.lockis excluded by!**/*.lock,!**/*.lockexecutor/crates/common/Cargo.lockis excluded by!**/*.lock,!**/*.lockexecutor/crates/modules-interfaces/Cargo.lockis excluded by!**/*.lock,!**/*.locktests/integration/claude/intelligence/EXPLORED_PATHS.mdis excluded by!tests/**tests/integration/exploit/unservable_major/unservable_major.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.0_0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.1.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.1_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.1_0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.jsonnetis excluded by!tests/**tests/integration/exploit/unservable_major/unservable_major_from.pyis excluded by!tests/**tests/integration/exploit/unservable_major/unservable_major_to.pyis excluded by!tests/**tests/integration/misc/routed_call/routed_call.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/misc/routed_call/routed_call.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/misc/routed_call/routed_call.0_0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/misc/routed_call/routed_call.jsonnetis excluded by!tests/**tests/integration/misc/routed_call/routed_call_from.pyis excluded by!tests/**tests/integration/misc/routed_call/routed_call_to.pyis excluded by!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0_1.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0_2.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.jsonnetis excluded by!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.pyis excluded by!tests/**tests/integration/runner/major_mismatch/major_mismatch.3.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/major_mismatch/major_mismatch.3_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/major_mismatch/major_mismatch.jsonnetis excluded by!tests/**
📒 Files selected for processing (18)
executor/codegen/data/host-fns.jsonexecutor/codegen/data/public-abi.jsonexecutor/crates/common/src/host_fns.rsexecutor/crates/common/src/lib.rsexecutor/crates/modules-interfaces/Cargo.tomlexecutor/src/exe/run.rsexecutor/src/host/mod.rsexecutor/src/lib.rsexecutor/src/rt/memlimiter.rsexecutor/src/rt/mod.rsexecutor/src/rt/supervisor/actions.rsexecutor/src/rt/supervisor/mod.rsexecutor/src/rt/vm/mod.rsexecutor/src/rt/vm/storage.rsexecutor/src/wasi/genlayer_sdk/message.rsexecutor/src/wasi/genlayer_sdk/mod.rsexecutor/src/wasi/genlayer_sdk/run.rsexecutor/src/wasi/genlayer_sdk/tests.rs
💤 Files with no reviewable changes (2)
- executor/codegen/data/host-fns.json
- executor/crates/common/src/host_fns.rs
🚧 Files skipped from review as they are similar to previous changes (15)
- executor/crates/common/src/lib.rs
- executor/codegen/data/public-abi.json
- executor/crates/modules-interfaces/Cargo.toml
- executor/src/rt/supervisor/mod.rs
- executor/src/rt/vm/mod.rs
- executor/src/wasi/genlayer_sdk/message.rs
- executor/src/rt/vm/storage.rs
- executor/src/exe/run.rs
- executor/src/lib.rs
- executor/src/rt/mod.rs
- executor/src/rt/memlimiter.rs
- executor/src/host/mod.rs
- executor/src/wasi/genlayer_sdk/tests.rs
- executor/src/wasi/genlayer_sdk/mod.rs
- executor/src/wasi/genlayer_sdk/run.rs
88c5b00 to
23eb849
Compare
* feat(sdk): call contracts of another major through the host ✨ * refactor(rt): bound recursion with a budget minted by the chain root ♻️ * refactor(exe): carry nested execution state as one explicit group ♻️ * fix(exe): fold the callee's small hash on the nested route 🐛🔒️ * feat(exe): refuse a crossing call while custom runners are loaded ✨ * fix(exe): hand a major this line cannot serve to the manager 🐛 * feat(host): write caller-supplied hello bytes and drop notify_finished ✨ * fix(supervisor): load precompiled modules only for registry runners 🔒️ * fix(fees): stop a zero execution budget aborting internally 🐛 * chore(exe): drop the unreachable dlsym linker helper 🔥 * test(integration): add routed, unservable-major and mismatch cases ✅
23eb849 to
ef7dd2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@executor/crates/common/src/public_abi_pending.rs`:
- Line 30: In the error formatting logic around the detail values used by
val_str, replace the debug-only assertion with release-active validation so
empty details cannot produce invalid VmError strings. Apply the same change to
both affected checks, including the path around the encoded digest detail, and
reject or handle empty values before constructing the “leader_output
uses_this_error” and “leader_output extra” messages.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: baac4297-1793-48de-813f-f11ea1e4bb79
⛔ Files ignored due to path filters (28)
executor/Cargo.lockis excluded by!**/*.lock,!**/*.lockexecutor/crates/common/Cargo.lockis excluded by!**/*.lock,!**/*.lockexecutor/crates/modules-interfaces/Cargo.lockis excluded by!**/*.lock,!**/*.locktests/integration/claude/intelligence/EXPLORED_PATHS.mdis excluded by!tests/**tests/integration/exploit/unservable_major/unservable_major.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.0_0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.1.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.1_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.1_0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/exploit/unservable_major/unservable_major.jsonnetis excluded by!tests/**tests/integration/exploit/unservable_major/unservable_major_from.pyis excluded by!tests/**tests/integration/exploit/unservable_major/unservable_major_to.pyis excluded by!tests/**tests/integration/misc/routed_call/routed_call.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/misc/routed_call/routed_call.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/misc/routed_call/routed_call.0_0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/misc/routed_call/routed_call.jsonnetis excluded by!tests/**tests/integration/misc/routed_call/routed_call_from.pyis excluded by!tests/**tests/integration/misc/routed_call/routed_call_to.pyis excluded by!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0_1.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.0_2.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.jsonnetis excluded by!tests/**tests/integration/runner/absent_runner_id/absent_runner_id.pyis excluded by!tests/**tests/integration/runner/major_mismatch/major_mismatch.3.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/major_mismatch/major_mismatch.3_0.stdoutis excluded by!**/*.stdout,!tests/**tests/integration/runner/major_mismatch/major_mismatch.jsonnetis excluded by!tests/**
📒 Files selected for processing (19)
executor/codegen/data/host-fns.jsonexecutor/codegen/data/public-abi.jsonexecutor/crates/common/src/host_fns.rsexecutor/crates/common/src/lib.rsexecutor/crates/common/src/public_abi_pending.rsexecutor/crates/modules-interfaces/Cargo.tomlexecutor/src/exe/run.rsexecutor/src/host/mod.rsexecutor/src/lib.rsexecutor/src/rt/memlimiter.rsexecutor/src/rt/mod.rsexecutor/src/rt/supervisor/actions.rsexecutor/src/rt/supervisor/mod.rsexecutor/src/rt/vm/mod.rsexecutor/src/rt/vm/storage.rsexecutor/src/wasi/genlayer_sdk/message.rsexecutor/src/wasi/genlayer_sdk/mod.rsexecutor/src/wasi/genlayer_sdk/run.rsexecutor/src/wasi/genlayer_sdk/tests.rs
💤 Files with no reviewable changes (2)
- executor/crates/common/src/host_fns.rs
- executor/codegen/data/host-fns.json
🚧 Files skipped from review as they are similar to previous changes (16)
- executor/codegen/data/public-abi.json
- executor/src/rt/vm/mod.rs
- executor/crates/modules-interfaces/Cargo.toml
- executor/crates/common/src/lib.rs
- executor/src/rt/supervisor/actions.rs
- executor/src/rt/memlimiter.rs
- executor/src/rt/supervisor/mod.rs
- executor/src/wasi/genlayer_sdk/tests.rs
- executor/src/wasi/genlayer_sdk/mod.rs
- executor/src/rt/mod.rs
- executor/src/exe/run.rs
- executor/src/rt/vm/storage.rs
- executor/src/lib.rs
- executor/src/host/mod.rs
- executor/src/wasi/genlayer_sdk/run.rs
- executor/src/wasi/genlayer_sdk/message.rs
|
|
||
| impl LeaderOutputUsesThisError { | ||
| pub fn val_str(&self, v: &str) -> VmError { | ||
| debug_assert!(!v.is_empty(), "leader_output uses_this_error needs a non-empty description"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 4 \
'uses_this_error\(\).*val_str|extra\(\).*val_str|val_str\(\s*""\s*\)' \
executor --glob '*.rs'
rg -n -C 4 \
'is_valid_|leader_output uses_this_error |leader_output extra ' \
executor --glob '*.rs'Repository: genlayerlabs/genvm-executor
Length of output: 165
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Repository files matching public_abi_pending.rs:\n'
fd -a 'public_abi_pending\.rs$' . || true
printf '\nRelevant candidate files:\n'
git ls-files | rg 'executor/.*/public_abi_pending\.rs|leader_output|LeaderOutput' || true
printf '\nSearching symbols in tracked files:\n'
rg -n -C 3 'LeaderOutput|uses_this_error|extra|is_valid_|leader_output uses_this_error|leader_output extra|val_str' . | head -n 240 || trueRepository: genlayerlabs/genvm-executor
Length of output: 20283
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file='executor/crates/common/src/public_abi_pending.rs'
printf 'File exists and line count:\n'
wc -l "$file"
printf '\nFile content:\n'
cat -n "$file"
printf '\nFocused occurrences in executor directory (case-sensitive exact calls):\n'
rg -n -C 3 'uses_this_error\s*\(|\bextra\s*\(|leader_output uses_this_error |leader_output extra |LeaderOutputUsesThisError|LeaderOutputExtra' executor || true
printf '\nAll occurrences in tracked files:\n'
rg -n -C 3 'uses_this_error\s*\(|\bextra\s*\(|leader_output uses_this_error |leader_output extra |LeaderOutputUsesThisError|LeaderOutputExtra' --glob '!runners/cpython/**/*.nix' --glob '!runners/cpython/**/*.wat' . | head -n 240 || trueRepository: genlayerlabs/genvm-executor
Length of output: 16122
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Generate the relevant validation behavior as a read-only probe:\n'
python3 - <<'PY'
def prefix(s, name):
return "leader_output " + name + " "
def is_valid(s):
if s in {"malformed_entry", "leader_output malformed"}:
return True
if s.startswith(prefix("uses_this_error")):
return s[len(prefix("uses_this_error")):] != ""
if s.startswith(prefix("extra")):
return s[len(prefix("extra")):] != ""
return False
for code in ["uses_this_error", "extra", "fix_point", "abc", "erntd8"]:
s = prefix(code) if code in {"uses_this_error", "extra"} else code
print(code, "=>", s, "valid:", is_valid(s), "len:", len(code))
PY
printf '\nInspect generation references/source for public_abi_pending.rs:\n'
rg -n -C 4 'public_abi_pending|LeaderOutputUsesThisError|LeaderOutputExtra|leader_output (uses_this_error|extra)' --glob '!executor/crates/common/src/public_abi_pending.rs' . | head -n 240 || trueRepository: genlayerlabs/genvm-executor
Length of output: 370
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Generate the relevant validation behavior as a read-only probe:\n'
python3 - <<'PY'
def make(s):
return "leader_output " + s
def is_valid(whole):
for name in ["uses_this_error", "extra"]:
prefix = make(name) + " "
if whole.startswith(prefix) and whole[len(prefix):] != "":
return True
return whole in {"malformed_entry", "leader_output malformed"}
for code in ["uses_this_error", "extra", "fix_point", "abc", "erntd8"]:
whole = make(code)
print(code, "=>", whole, "valid:", is_valid(whole), "detail_empty:", len(code) == 0)
PY
printf '\nInspect all non-generated references to public_abi_pending.rs and generated ABI prefixes:\n'
rg -n -C 4 'public_abi_pending|LeaderOutputUsesThisError|LeaderOutputExtra|leader_output uses_this_error|leader_output extra' --glob '!executor/crates/common/src/public_abi_pending.rs' . | head -n 240 || true
printf '\nInspect callers around VmError construction in executor/src/rt/errors.rs:\n'
sed -n '500,575p' executor/src/rt/errors.rsRepository: genlayerlabs/genvm-executor
Length of output: 15640
Enforce the non-empty detail on release paths.
debug_assert! is removed from release builds, so val_str("") still creates leader_output uses_this_error or leader_output extra . Those values fail VmError::is_valid_ and can be produced from encoded digest details. Move the non-empty check out of debug_assert or use release-active validation before formatting the error.
Also applies to: 39-39
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/crates/common/src/public_abi_pending.rs` at line 30, In the error
formatting logic around the detail values used by val_str, replace the
debug-only assertion with release-active validation so empty details cannot
produce invalid VmError strings. Apply the same change to both affected checks,
including the path around the encoded digest detail, and reject or handle empty
values before constructing the “leader_output uses_this_error” and
“leader_output extra” messages.
Auto-opened executor mirror of genlayerlabs/genvm-manager#9.
Carries the executor-side work for that manager PR. Auto-closed as merged when the manager PR lands (its
pr/v0.3/feat/rework-manager-apibranch is moved ontov0.3-dev).Summary by CodeRabbit
New Features
Bug Fixes
Documentation