fix(runtime): align sandbox execution, timeout control, and observability#14
fix(runtime): align sandbox execution, timeout control, and observability#14
Conversation
…lity Harden runtime execution behavior with Tier-B identity/elevation updates, timeout and cancellation handling improvements, and supporting test/documentation adjustments for stable tool execution paths. Made-with: Cursor
📝 WalkthroughWalkthroughThis pull request applies extensive formatting and whitespace normalization across the OpenSkills runtime codebase. Changes include reordering imports, converting expressions between single-line and multi-line formats for readability, normalizing trailing whitespace, and reformatting test assertions. No functional logic, API signatures, or control flow are altered. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes 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. Important Merge conflicts detected (Beta)
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
runtime/src/permissions.rs (1)
78-83: Optional: Avoid string allocation in the hot path.The
format!(".{}", allowed)creates a new String on each iteration. While the performance impact is minimal for small allow lists and infrequent permission checks, you could eliminate the allocation:♻️ Optional optimization to avoid format!() allocation
- Ok(self - .wasm_config - .network - .allow - .iter() - .any(|allowed| host == allowed || host.ends_with(&format!(".{}", allowed)))) + Ok(self + .wasm_config + .network + .allow + .iter() + .any(|allowed| { + host == allowed + || (host.len() > allowed.len() + && host.ends_with(allowed) + && host.as_bytes()[host.len() - allowed.len() - 1] == b'.') + }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/src/permissions.rs` around lines 78 - 83, The network allow check in the wasm_config (the expression using wasm_config.network.allow.iter().any(...) with host.ends_with(&format!(".{}", allowed))) allocates a temporary String each iteration; change the logic in that permission check to avoid format! by testing either host == allowed, or host ends with allowed and the character immediately before the matching suffix is '.' (i.e., ensure host.len() > allowed.len() and the byte at host.len()-allowed.len()-1 is '.'), so you compare suffixes without allocating; update the closure passed to .any() where host and allowed are used to implement this allocation-free suffix check.runtime/tests/skill_session_tests.rs (1)
257-257: Consider consistent formatting forstart_skill_sessioncalls.This call is formatted as single-line, while in
runtime/tests/integration_tests.rs(lines 195-199), an identicalstart_skill_sessioncall with similar arguments was reformatted to multi-line. Consider applying consistent formatting direction across the test suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@runtime/tests/skill_session_tests.rs` at line 257, The call to start_skill_session in skill_session_tests.rs is formatted as a single-line while a similar call in runtime/tests/integration_tests.rs uses a multi-line style; update the start_skill_session("fork-test", Some(json!({ "test": "data" })), None) invocation to match the multi-line formatting used elsewhere so test formatting is consistent (locate the call to start_skill_session in the test function and break the arguments across lines similar to the integration_tests example).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@runtime/src/permissions.rs`:
- Around line 78-83: The network allow check in the wasm_config (the expression
using wasm_config.network.allow.iter().any(...) with
host.ends_with(&format!(".{}", allowed))) allocates a temporary String each
iteration; change the logic in that permission check to avoid format! by testing
either host == allowed, or host ends with allowed and the character immediately
before the matching suffix is '.' (i.e., ensure host.len() > allowed.len() and
the byte at host.len()-allowed.len()-1 is '.'), so you compare suffixes without
allocating; update the closure passed to .any() where host and allowed are used
to implement this allocation-free suffix check.
In `@runtime/tests/skill_session_tests.rs`:
- Line 257: The call to start_skill_session in skill_session_tests.rs is
formatted as a single-line while a similar call in
runtime/tests/integration_tests.rs uses a multi-line style; update the
start_skill_session("fork-test", Some(json!({ "test": "data" })), None)
invocation to match the multi-line formatting used elsewhere so test formatting
is consistent (locate the call to start_skill_session in the test function and
break the arguments across lines similar to the integration_tests example).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b524232e-40db-4f7d-a6c9-79072b78b6e2
📒 Files selected for processing (47)
bindings/python/src/lib.rsbindings/ts/src/lib.rsruntime/src/audit.rsruntime/src/bin/openskills-runtime.rsruntime/src/build/config.rsruntime/src/build/mod.rsruntime/src/build/plugins/adapter.rsruntime/src/build/plugins/assemblyscript.rsruntime/src/build/plugins/javy.rsruntime/src/build/plugins/mod.rsruntime/src/build/plugins/quickjs.rsruntime/src/build/registry.rsruntime/src/context.rsruntime/src/deps_check.rsruntime/src/errors.rsruntime/src/executor.rsruntime/src/hook_runner.rsruntime/src/host_policy.rsruntime/src/lib.rsruntime/src/manifest.rsruntime/src/native_runner.rsruntime/src/permission_callback.rsruntime/src/permissions.rsruntime/src/registry.rsruntime/src/skill_parser.rsruntime/src/validator.rsruntime/src/wasm_runner.rsruntime/tests/always_loaded_tests.rsruntime/tests/claude_official_skills_tests.rsruntime/tests/compatibility_tests.rsruntime/tests/deps_check_tests.rsruntime/tests/edge_case_tests.rsruntime/tests/error_handling_tests.rsruntime/tests/file_io_tests.rsruntime/tests/hook_tests.rsruntime/tests/host_policy_tests.rsruntime/tests/integration_tests.rsruntime/tests/linux_sandbox_tests.rsruntime/tests/registry_tests.rsruntime/tests/resource_tests.rsruntime/tests/sandbox_command_tests.rsruntime/tests/seatbelt_tests.rsruntime/tests/skill_session_tests.rsruntime/tests/spec_conformance_tests.rsruntime/tests/system_prompt_tests.rsruntime/tests/target_execution_tests.rsruntime/tests/wasm_tests.rs
Summary
Test plan
runtime/tests/*) and verify pass rate against current main baselineMade with Cursor
Summary by CodeRabbit