Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors dynamic eval to return updated workspace vars and handles execution differently on wasm vs native (threaded). Adds replace_workspace_target_vars_and_state to apply owned vars. Adds a regression test ensuring eval errors don't leave partial mutations. Updates CHANGELOG with v0.5.0. ChangesDynamic eval refactoring with workspace state replacement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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: 6
🤖 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 `@crates/runmat-accelerate/tests/qr_precision_boundary.rs`:
- Around line 10-13: The test mutates the global RUNMAT_WGPU_FORCE_PRECISION
environment variable which can race with other tests; instead, wrap the change
so it is restored after the test (save
std::env::var_os("RUNMAT_WGPU_FORCE_PRECISION") before setting, set the desired
value, run the test logic, then restore or remove the env var in a
finally/guard), or run the precision-specific logic in an isolated process
(spawn a Command with the env set) or serialize the test using a global test
mutex; update the test in qr_precision_boundary.rs to use one of these
approaches and ensure the original environment value is always restored.
In `@crates/runmat-core/src/tests.rs`:
- Around line 1548-1571: Extract the duplicated assert_named_path closure into a
single shared test helper function (e.g., fn assert_named_path(outcome:
&OutcomeType, name: &str, expected: PathBuf)) and replace both inline closures
with calls to that helper; the helper should use outcome_named_upsert_value,
perform the same Value::String extraction, call comparable_path_text on actual
and expected, and include outcome.workspace_delta.upserts and
outcome.diagnostics in panic/assert messages so behavior and platform-normalized
path comparison remain identical to the original closure.
- Around line 118-133: The lookup in outcome_named_upsert_value is
non-deterministic when both SourceBinding and Interactive upserts share a name;
update the function to prefer abi::WorkspaceBindingKey::SourceBinding matches
over abi::WorkspaceBindingKey::Interactive by first scanning
outcome.workspace_delta.upserts for a SourceBinding with binding.0 == name and
returning its upsert.value if found, and only if none are found, scan for
Interactive matches (matching binding_name.0 == name); keep the existing
handling for Global/Persistent as non-matches.
In `@crates/runmat-runtime/src/builtins/plotting/ops/print.rs`:
- Around line 616-631: The sanitized safe_thread_name can collide across threads
and tests, causing stale-file propagation; update the filename generation (the
code around thread_name, safe_thread_name and the path.push that formats
"runmat_print_test_{}_{}_{}") to add a high-entropy uniqueness token (e.g.,
include std::thread::current().id() or a monotonic timestamp plus a short
random/UUID suffix) so filenames remain unique even when sanitized names
collide; ensure the uniqueness token is generated per-invocation and appended to
the formatted name to avoid cross-test collisions.
In `@crates/runmat-vm/src/call/builtins.rs`:
- Around line 468-477: The async function interpret_dynamic_workspace_eval
currently spawns a scoped thread with std::thread::scope and calls handle.join()
which blocks the async executor and mutates caller-owned target_vars by
reference; change it to copy an owned WorkspaceSnapshot (names + assigned +
vars) and run the eval inside a detached thread or task that only receives owned
data, then use a non-blocking oneshot channel (the same handoff pattern used in
run.rs around the run session) to receive the result; only on successful
completion replace_workspace_target_state(target, names, assigned) and swap the
vars back into the caller/target atomically, and on panic or error ensure no
partial mutations to caller-owned Vec<Value> occur (map panics to
DynamicWorkspaceEvalThreadPanic but do not leave workspace state mutated).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: b6c8374d-140f-4025-a6ce-763447ec5ba6
📒 Files selected for processing (12)
crates/runmat-accelerate/tests/qr_precision_boundary.rscrates/runmat-cli/tests/cli.rscrates/runmat-config/src/project/mod.rscrates/runmat-config/tests/project_manifest.rscrates/runmat-core/src/session/compile.rscrates/runmat-core/src/tests.rscrates/runmat-runtime/src/builtins/introspection/mfilename.rscrates/runmat-runtime/src/builtins/io/tabular/dlmwrite.rscrates/runmat-runtime/src/builtins/plotting/ops/print.rscrates/runmat-runtime/src/source_context.rscrates/runmat-vm/Cargo.tomlcrates/runmat-vm/src/call/builtins.rs
Comment on lines
+10
to
13
| #[cfg(target_os = "windows")] | ||
| std::env::set_var("RUNMAT_WGPU_FORCE_PRECISION", "f32"); | ||
| #[cfg(not(target_os = "windows"))] | ||
| std::env::set_var("RUNMAT_WGPU_FORCE_PRECISION", "f64"); |
Contributor
There was a problem hiding this comment.
[high] Process-wide env mutation here can race with parallel tests and leak stale precision state.
RUNMAT_WGPU_FORCE_PRECISION is global and is never restored, so concurrent tests can observe the wrong precision path and fail nondeterministically.
Suggested fix
+use std::sync::{Mutex, OnceLock};
+
+fn env_lock() -> &'static Mutex<()> {
+ static LOCK: OnceLock<Mutex<()>> = OnceLock::new();
+ LOCK.get_or_init(|| Mutex::new(()))
+}
+
fn register_provider() -> &'static dyn AccelProvider {
+ let _guard = env_lock().lock().expect("env lock poisoned");
+ let previous = std::env::var("RUNMAT_WGPU_FORCE_PRECISION").ok();
#[cfg(target_os = "windows")]
std::env::set_var("RUNMAT_WGPU_FORCE_PRECISION", "f32");
#[cfg(not(target_os = "windows"))]
std::env::set_var("RUNMAT_WGPU_FORCE_PRECISION", "f64");
let _ = provider::register_wgpu_provider(WgpuProviderOptions::default());
- runmat_accelerate_api::provider().expect("wgpu provider registered")
+ let p = runmat_accelerate_api::provider().expect("wgpu provider registered");
+ if let Some(v) = previous {
+ std::env::set_var("RUNMAT_WGPU_FORCE_PRECISION", v);
+ } else {
+ std::env::remove_var("RUNMAT_WGPU_FORCE_PRECISION");
+ }
+ p
}As per coding guidelines, this follows: "Prioritize catching: race conditions, stale state propagation, async ordering issues."
🤖 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 `@crates/runmat-accelerate/tests/qr_precision_boundary.rs` around lines 10 -
13, The test mutates the global RUNMAT_WGPU_FORCE_PRECISION environment variable
which can race with other tests; instead, wrap the change so it is restored
after the test (save std::env::var_os("RUNMAT_WGPU_FORCE_PRECISION") before
setting, set the desired value, run the test logic, then restore or remove the
env var in a finally/guard), or run the precision-specific logic in an isolated
process (spawn a Command with the env set) or serialize the test using a global
test mutex; update the test in qr_precision_boundary.rs to use one of these
approaches and ensure the original environment value is always restored.
Comment on lines
+176
to
+183
| #[cfg(not(target_arch = "wasm32"))] | ||
| { | ||
| if let (Ok(left), Ok(right)) = (std::fs::canonicalize(left), std::fs::canonicalize(right)) { | ||
| if left == right { | ||
| return true; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for existing tests that exercise path equivalence under various filesystem conditions
# Check for existing tests with symlinks, relative paths, or canonicalization
rg -n -C3 'canonicalize|symlink|\.\./' --glob '**/tests/**/*.rs' --glob '**/src/**/test*.rs'
# Look for companion source discovery tests
ast-grep --pattern 'fn $_($$) {
$$$
discover_companion_$$$
$$$
}'Repository: runmat-org/runmat
Length of output: 4832
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the relevant section of compile.rs (and nearby callers)
file="crates/runmat-core/src/session/compile.rs"
echo "== $file =="
wc -l "$file" || true
nl -ba "$file" | sed -n '130,260p'
# 2) Find where `private_source_visible_to` is defined/used
echo
echo "== private_source_visible_to usages =="
rg -n "private_source_visible_to" -S crates/runmat-core/src || true
# 3) Find all canonicalize uses in runmat-core (to gauge security/logic impact)
echo
echo "== canonicalize usages in crates/runmat-core/src =="
rg -n "canonicalize\\(" crates/runmat-core/src || true
# 4) Search tests for symlink usage and any path-equivalence assertions
echo
echo "== symlink usage in tests =="
rg -n "symlink" crates/**/tests/**/*.rs crates/**/src/**/test*.rs || true
echo
echo "== path equivalence / canonicalization assertions in tests =="
rg -n "(canonicalize\\(\\)|equival|path equiv|same path|source_visible|private_source_visible_to)" crates/**/tests/**/*.rs || trueRepository: runmat-org/runmat
Length of output: 217
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/runmat-core/src/session/compile.rs"
echo "== $file =="
wc -l "$file" || true
echo
echo "== compile.rs lines 150-240 (context) =="
awk 'NR>=150 && NR<=240 {printf "%d:%s\n", NR, $0}' "$file" | sed -n '1,120p'
echo
echo "== private_source_visible_to usages =="
rg -n "private_source_visible_to" -S crates/runmat-core/src || true
echo
echo "== canonicalize usages in crates/runmat-core/src =="
rg -n "canonicalize\\(" crates/runmat-core/src || true
echo
echo "== symlink usage in tests (any crate) =="
rg -n "symlink" crates/**/tests/**/*.rs crates/**/src/**/test*.rs || true
echo
echo "== path equivalence / canonicalization assertions in tests =="
rg -n "(canonicalize\\(\\)|equival|path equiv|same path|source_visible|private_source_visible_to)" crates/**/tests/**/*.rs || trueRepository: runmat-org/runmat
Length of output: 4166
[low] TOCTOU race risk in path equivalence for private-source visibility
source_paths_equivalent() canonicalizes both path candidates (non-wasm) and compares equality; private_source_visible_to() then uses that equivalence to decide whether primary_source_path.parent() is allowed to “see” a source_path under a private directory. If symlinks/paths change between the two canonicalize calls, the computed equivalence could be wrong, affecting private-source visibility decisions. No tests were found covering symlink/path-equivalence edge cases, so consider hardening (or documenting the threat model that assumes no adversarial concurrent filesystem changes).
Comment on lines
+118
to
+133
| fn outcome_named_upsert_value<'a>( | ||
| outcome: &'a abi::ExecutionOutcome, | ||
| name: &str, | ||
| ) -> Option<&'a runmat_builtins::Value> { | ||
| outcome.workspace_delta.upserts.iter().find_map(|upsert| { | ||
| let matches_name = match &upsert.key { | ||
| abi::WorkspaceBindingKey::Interactive { | ||
| name: binding_name, .. | ||
| } => binding_name.0 == name, | ||
| abi::WorkspaceBindingKey::SourceBinding { binding, .. } => binding.0 == name, | ||
| abi::WorkspaceBindingKey::Global { .. } | ||
| | abi::WorkspaceBindingKey::Persistent { .. } => false, | ||
| }; | ||
| matches_name.then_some(&upsert.value) | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
[medium] Make named-upsert lookup deterministic by preferring source-scoped bindings
outcome_named_upsert_value currently returns the first matching name across Interactive and SourceBinding keys (Line 122 onward). If both are present, assertions can read the wrong scope and silently validate stale state.
Proposed fix
fn outcome_named_upsert_value<'a>(
outcome: &'a abi::ExecutionOutcome,
name: &str,
) -> Option<&'a runmat_builtins::Value> {
- outcome.workspace_delta.upserts.iter().find_map(|upsert| {
- let matches_name = match &upsert.key {
- abi::WorkspaceBindingKey::Interactive {
- name: binding_name, ..
- } => binding_name.0 == name,
- abi::WorkspaceBindingKey::SourceBinding { binding, .. } => binding.0 == name,
- abi::WorkspaceBindingKey::Global { .. }
- | abi::WorkspaceBindingKey::Persistent { .. } => false,
- };
- matches_name.then_some(&upsert.value)
- })
+ let mut interactive_match: Option<&'a runmat_builtins::Value> = None;
+ for upsert in &outcome.workspace_delta.upserts {
+ match &upsert.key {
+ abi::WorkspaceBindingKey::SourceBinding { binding, .. } if binding.0 == name => {
+ return Some(&upsert.value);
+ }
+ abi::WorkspaceBindingKey::Interactive {
+ name: binding_name, ..
+ } if binding_name.0 == name => {
+ interactive_match.get_or_insert(&upsert.value);
+ }
+ abi::WorkspaceBindingKey::Global { .. }
+ | abi::WorkspaceBindingKey::Persistent { .. }
+ | abi::WorkspaceBindingKey::SourceBinding { .. }
+ | abi::WorkspaceBindingKey::Interactive { .. } => {}
+ }
+ }
+ interactive_match
}As per coding guidelines, "Prioritize catching: stale state propagation."
🤖 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 `@crates/runmat-core/src/tests.rs` around lines 118 - 133, The lookup in
outcome_named_upsert_value is non-deterministic when both SourceBinding and
Interactive upserts share a name; update the function to prefer
abi::WorkspaceBindingKey::SourceBinding matches over
abi::WorkspaceBindingKey::Interactive by first scanning
outcome.workspace_delta.upserts for a SourceBinding with binding.0 == name and
returning its upsert.value if found, and only if none are found, scan for
Interactive matches (matching binding_name.0 == name); keep the existing
handling for Global/Persistent as non-matches.
Comment on lines
+1548
to
+1571
| let assert_named_path = |name: &str, expected: PathBuf| { | ||
| let actual = outcome_named_upsert_value(&outcome, name) | ||
| .and_then(|value| { | ||
| if let runmat_builtins::Value::String(text) = value { | ||
| Some(text.as_str()) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .unwrap_or_else(|| { | ||
| panic!( | ||
| "expected {name} to contain a string path; upserts={:?}; diagnostics={:?}", | ||
| outcome.workspace_delta.upserts, outcome.diagnostics | ||
| ) | ||
| }); | ||
| let expected = expected.to_string_lossy().to_string(); | ||
| assert_eq!( | ||
| comparable_path_text(actual), | ||
| comparable_path_text(&expected), | ||
| "expected {name} path {expected:?}, got {actual:?}; upserts={:?}; diagnostics={:?}", | ||
| outcome.workspace_delta.upserts, | ||
| outcome.diagnostics | ||
| ); | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
[low] Extract duplicated assert_named_path closure into a shared test helper
The same assertion block appears twice with identical behavior. Centralizing it reduces divergence risk for cross-platform path checks.
Also applies to: 9495-9518
🤖 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 `@crates/runmat-core/src/tests.rs` around lines 1548 - 1571, Extract the
duplicated assert_named_path closure into a single shared test helper function
(e.g., fn assert_named_path(outcome: &OutcomeType, name: &str, expected:
PathBuf)) and replace both inline closures with calls to that helper; the helper
should use outcome_named_upsert_value, perform the same Value::String
extraction, call comparable_path_text on actual and expected, and include
outcome.workspace_delta.upserts and outcome.diagnostics in panic/assert messages
so behavior and platform-normalized path comparison remain identical to the
original closure.
Comment on lines
+616
to
631
| let thread = std::thread::current(); | ||
| let thread_name = thread.name().unwrap_or("thread"); | ||
| let safe_thread_name: String = thread_name | ||
| .chars() | ||
| .map(|ch| match ch { | ||
| '<' | '>' | ':' | '"' | '/' | '\\' | '|' | '?' | '*' => '_', | ||
| ch if ch.is_control() => '_', | ||
| ch => ch, | ||
| }) | ||
| .collect(); | ||
| path.push(format!( | ||
| "runmat_print_test_{}_{}_{}", | ||
| stem, | ||
| std::process::id(), | ||
| std::thread::current().name().unwrap_or("thread") | ||
| safe_thread_name | ||
| )); |
Contributor
There was a problem hiding this comment.
[medium] Sanitization increases temp-path collision risk across parallel tests.
Replacing multiple characters with _ can make different thread names identical, and the path key is only {stem, pid, safe_thread_name}. That can cause stale-file propagation between tests.
Suggested fix
fn unique_temp_path(stem: &str) -> PathBuf {
+ static NEXT: AtomicU64 = AtomicU64::new(0);
let mut path = std::env::temp_dir();
@@
path.push(format!(
- "runmat_print_test_{}_{}_{}",
+ "runmat_print_test_{}_{}_{}_{}",
stem,
std::process::id(),
- safe_thread_name
+ safe_thread_name,
+ NEXT.fetch_add(1, Ordering::Relaxed)
));
path
}As per coding guidelines, this follows: "Prioritize catching: race conditions, stale state propagation, async ordering issues."
🤖 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 `@crates/runmat-runtime/src/builtins/plotting/ops/print.rs` around lines 616 -
631, The sanitized safe_thread_name can collide across threads and tests,
causing stale-file propagation; update the filename generation (the code around
thread_name, safe_thread_name and the path.push that formats
"runmat_print_test_{}_{}_{}") to add a high-entropy uniqueness token (e.g.,
include std::thread::current().id() or a monotonic timestamp plus a short
random/UUID suffix) so filenames remain unique even when sanitized names
collide; ensure the uniqueness token is generated per-invocation and appended to
the formatted name to avoid cross-test collisions.
adding changelog entry for 0.5.0 runtime release
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/CHANGELOG.md`:
- Line 7: Verify whether the release date in docs/CHANGELOG.md (the line "_June
3, 2026_") is correct; if the release was actually cut on the PR opening/merge
date, update that single line to the correct date (e.g., "_June 5, 2026_") or
otherwise add a short note clarifying that the changelog entry was backdated,
then commit the corrected CHANGELOG.md.
- Line 13: Add a blank line before each markdown subsection heading (e.g., the
"#### Added" heading and the other `####` subsection headings mentioned) so that
there's an empty line separating the previous paragraph/list and each `####`
heading to satisfy MD022 and improve readability; update all occurrences flagged
(the "#### Added" heading plus the other `####` headings called out) to have a
single blank line immediately preceding them.
- Around line 26-29: Add a concise entry to the "Changed" section documenting
that eval() now runs on a separate thread with a copied source catalog which
changes mfilename() and source-context introspection behavior, and list the new
error types DynamicWorkspaceEvalThreadSpawnFailed and
DynamicWorkspaceEvalThreadPanic as possible failure modes when calling eval();
mention this is a behavioral change for users upgrading to v0.5.0 so they can
update introspection-dependent code and handle the new thread-related errors.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 2d59119a-bd6c-4458-a8c3-ccc68a7b85ea
📒 Files selected for processing (1)
docs/CHANGELOG.md
|
|
||
| ## [v0.5.0](https://github.com/runmat-org/runmat/compare/v0.4.9...v0.5.0) | ||
|
|
||
| _June 3, 2026_ |
Contributor
There was a problem hiding this comment.
[low] Verify release date accuracy.
The changelog lists the release date as June 3, 2026, but the PR opening timestamp is June 5, 2026. Confirm whether the release was indeed cut on June 3 (and this PR is merging it to main after the fact), or if the date should be updated to reflect the actual merge date.
🤖 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 `@docs/CHANGELOG.md` at line 7, Verify whether the release date in
docs/CHANGELOG.md (the line "_June 3, 2026_") is correct; if the release was
actually cut on the PR opening/merge date, update that single line to the
correct date (e.g., "_June 5, 2026_") or otherwise add a short note clarifying
that the changelog entry was backdated, then commit the corrected CHANGELOG.md.
|
|
||
| ### Runtime | ||
|
|
||
| #### Added |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
[nit] Consider adding blank lines around subsection headings for markdown lint compliance.
The markdown linter flags missing blank lines before several #### headings (lines 13, 25, 36, 41, 46). While the current format is consistent with existing changelog entries, adding blank lines would improve readability and satisfy the MD022 rule.
📋 Optional formatting improvement
### Runtime
+
#### Added
- Add [project composition]...Apply similar spacing for other subsection headings on lines 25, 36, 41, and 46.
Also applies to: 25-25, 36-36, 41-41, 46-46
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 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 `@docs/CHANGELOG.md` at line 13, Add a blank line before each markdown
subsection heading (e.g., the "#### Added" heading and the other `####`
subsection headings mentioned) so that there's an empty line separating the
previous paragraph/list and each `####` heading to satisfy MD022 and improve
readability; update all occurrences flagged (the "#### Added" heading plus the
other `####` headings called out) to have a single blank line immediately
preceding them.
Comment on lines
+26
to
+29
| - Migrate the compiler and runtime to a staged semantic [pipeline](/docs/runtime/compiler) — `source -> AST -> semantic HIR -> MIR -> MIR analysis -> VM layout + bytecode -> runtime/providers` — so names, functions, classes, output counts, indexing context, and workspace layout are resolved once and shared by the interpreter, language server, JIT, and acceleration planner | ||
| - Remove the legacy AST-to-bytecode execution paths in favor of the semantic pipeline | ||
| - Compile calls from typed callable identities and fallback policies — bound functions, builtins, dynamic names, external qualified names, methods, super calls, and `feval` targets share one [dispatch](/docs/runtime/vm/dispatch) model, preserving function-handle identity | ||
| - Make workspace-visible bindings explicit — script exports, function locals, `ans`, and dynamic workspace operations route through VM workspace context |
Contributor
There was a problem hiding this comment.
[medium] Document dynamic eval behavior changes and new failure modes.
The PR objectives explicitly flag "medium risk: behavior change for introspection inside eval and new spawn/join failure modes." The changelog describes the staged pipeline and workspace-visible bindings but does not mention:
- Breaking change:
mfilename()and source context insideeval()now execute on a separate thread with a copied source catalog, which may affect introspection-dependent code. - New failure modes: Thread spawn failures (
DynamicWorkspaceEvalThreadSpawnFailed) and thread panics (DynamicWorkspaceEvalThreadPanic) are now possible when callingeval().
Users upgrading to v0.5.0 should be aware of these behavior changes and potential new error conditions.
📝 Suggested addition to "Changed" section
Add a note about the eval threading changes:
#### Changed
- Migrate the compiler and runtime to a staged semantic [pipeline](/docs/runtime/compiler) — `source -> AST -> semantic HIR -> MIR -> MIR analysis -> VM layout + bytecode -> runtime/providers` — so names, functions, classes, output counts, indexing context, and workspace layout are resolved once and shared by the interpreter, language server, JIT, and acceleration planner
- Remove the legacy AST-to-bytecode execution paths in favor of the semantic pipeline
+- Change `eval` execution model — dynamic workspace evaluation now runs on a dedicated thread with a larger stack and a copied source catalog, so `mfilename()` and source introspection inside `eval()` reflect the eval source; introduces new spawn/panic failure modes
- Compile calls from typed callable identities and fallback policies — bound functions, builtins, dynamic names, external qualified names, methods, super calls, and `feval` targets share one [dispatch](/docs/runtime/vm/dispatch) model, preserving function-handle identity🤖 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 `@docs/CHANGELOG.md` around lines 26 - 29, Add a concise entry to the "Changed"
section documenting that eval() now runs on a separate thread with a copied
source catalog which changes mfilename() and source-context introspection
behavior, and list the new error types DynamicWorkspaceEvalThreadSpawnFailed and
DynamicWorkspaceEvalThreadPanic as possible failure modes when calling eval();
mention this is a behavioral change for users upgrading to v0.5.0 so they can
update introspection-dependent code and handle the new thread-related errors.
…d can leave workspace state stale on panic
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 07711c5. Configure here.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/runmat-vm/src/runtime/workspace.rs (1)
416-439:⚠️ Potential issue | 🟠 Major | ⚡ Quick win[medium]
removed_slots_this_executionandassigned_*_this_executionnot cleared on state replacementWhen
replace_workspace_target_vars_and_statereplaces the frame'snames,assigned, andvars, the execution-tracking fields (assigned_names_this_execution,assigned_ids_this_execution,removed_slots_this_execution) are left unchanged. After replacement, slot indices in these fields may reference stale or non-existent slots, causing incorrectWorkspaceAssignedReportdata when the frame is dropped.Consider clearing or resynchronizing these fields:
🛠️ Proposed fix
frame.state.names = names; frame.state.assigned = assigned; frame.state.slot_lifecycle = lifecycle_from_names(&frame.state.names, &frame.state.assigned); + frame.state.assigned_names_this_execution.clear(); + frame.state.assigned_ids_this_execution.clear(); + frame.state.removed_slots_this_execution.clear(); frame.state.data_ptr = target_vars.as_ptr(); frame.state.len = target_vars.len();🤖 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 `@crates/runmat-vm/src/runtime/workspace.rs` around lines 416 - 439, replace_workspace_target_vars_and_state updates frame.vars_ptr, frame.state.names, frame.state.assigned, slot_lifecycle, data_ptr and len but does not clear execution-tracking fields, so stale indices may survive; update the function to also clear or reinitialize frame.state.assigned_names_this_execution, frame.state.assigned_ids_this_execution, and frame.state.removed_slots_this_execution (e.g., set to empty collections or re-synchronize them with the new names/assigned) immediately after assigning frame.state.assigned and frame.state.names and before computing lifecycle_from_names to ensure WorkspaceAssignedReport will not contain stale slot references when the frame is dropped.
🤖 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 `@crates/runmat-core/src/tests.rs`:
- Around line 9355-9390: Extend the test
dynamic_workspace_eval_error_does_not_commit_partial_mutations to also verify
rollback when eval overwrites an existing binding: initialize a workspace
binding (e.g., publish "partial = 42;" via execute_text_request or include it in
the initial source using RunMatSession/execute_text_request), then run the
failing eval that assigns partial = 1; error(...); and assert after the failed
eval that the durable workspace still contains the original value (Num(42.0))
and that no stale mutation was committed; use the same helpers
(execute_text_request, outcome_has_upsert_name/outcome_has_named_upsert, and
outcome.flow.durable_workspace_value()) and keep the existing assertions for the
new-binding case.
In `@crates/runmat-vm/src/call/builtins.rs`:
- Line 467: The code dereferences frame.vars_ptr after releasing the
WORKSPACE_STACK.with() borrow, which is fragile; change
workspace_target_snapshot() (or its call site) to clone the target vars while
the WORKSPACE_STACK borrow is still held so no raw pointer is used afterward:
either update workspace_target_snapshot() to return an owned Vec/Vars clone (or
a struct containing the cloned vars) or call WORKSPACE_STACK.with(|stack| { let
snapshot = workspace_target_snapshot_inner(stack); snapshot.clone_vars() }) and
assign that owned clone to target_vars before leaving the closure; locate this
behavior around workspace_target_snapshot(), WORKSPACE_STACK.with(),
frame.vars_ptr and the let target_vars = unsafe { (*frame.vars_ptr).clone() }
line and replace the unsafe deref with the pre-cloned owned data.
In `@docs/CHANGELOG.md`:
- Line 5: The markdown header contains a trailing space inside the link text
"[v0.5.0 ](https://github.com/runmat-org/runmat/compare/v0.4.9...v0.5.0)";
remove the extra space so the link text reads "[v0.5.0](...)" to satisfy MD039
and stop the lint/CI noise—update the line containing that link text
accordingly.
---
Outside diff comments:
In `@crates/runmat-vm/src/runtime/workspace.rs`:
- Around line 416-439: replace_workspace_target_vars_and_state updates
frame.vars_ptr, frame.state.names, frame.state.assigned, slot_lifecycle,
data_ptr and len but does not clear execution-tracking fields, so stale indices
may survive; update the function to also clear or reinitialize
frame.state.assigned_names_this_execution,
frame.state.assigned_ids_this_execution, and
frame.state.removed_slots_this_execution (e.g., set to empty collections or
re-synchronize them with the new names/assigned) immediately after assigning
frame.state.assigned and frame.state.names and before computing
lifecycle_from_names to ensure WorkspaceAssignedReport will not contain stale
slot references when the frame is dropped.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e8abda65-cb58-4a37-9fe1-778f07c56b62
📒 Files selected for processing (4)
crates/runmat-core/src/tests.rscrates/runmat-vm/src/call/builtins.rscrates/runmat-vm/src/runtime/workspace.rsdocs/CHANGELOG.md
Comment on lines
+9355
to
+9390
| #[test] | ||
| fn dynamic_workspace_eval_error_does_not_commit_partial_mutations() { | ||
| let mut session = RunMatSession::with_snapshot_bytes(false, false, None).expect("session init"); | ||
| let source = r#" | ||
| seed = 5; | ||
| try | ||
| eval('partial = 1; error("RunMat:eval:boom", "boom");'); | ||
| err = "NOERR"; | ||
| catch e | ||
| err = e.identifier; | ||
| end | ||
| after = seed + 1; | ||
| "#; | ||
| let outcome = execute_text_request(&mut session, source).expect("exec succeeds"); | ||
| assert!(outcome_has_named_upsert( | ||
| &outcome, | ||
| "err", | ||
| &runmat_builtins::Value::String("RunMat:eval:boom".into()) | ||
| )); | ||
| assert!(outcome_has_named_upsert( | ||
| &outcome, | ||
| "after", | ||
| &runmat_builtins::Value::Num(6.0) | ||
| )); | ||
| assert!( | ||
| !outcome_has_upsert_name(&outcome, "partial"), | ||
| "failed eval must not publish partially assigned workspace variables" | ||
| ); | ||
|
|
||
| let outcome = execute_text_request(&mut session, "exist('partial')").expect("read workspace"); | ||
| let value = outcome | ||
| .flow | ||
| .durable_workspace_value() | ||
| .expect("exist result should be readable"); | ||
| assert_eq!(value.to_string(), "0"); | ||
| } |
Contributor
There was a problem hiding this comment.
[low] Add rollback coverage for overwriting an existing binding inside failing eval
This test only proves failed eval does not publish a new binding (partial). It does not verify rollback when eval mutates an existing binding, which is another stale-state propagation path for this PR.
Suggested test extension
fn dynamic_workspace_eval_error_does_not_commit_partial_mutations() {
@@
assert_eq!(value.to_string(), "0");
+
+ let overwrite = execute_text_request(
+ &mut session,
+ r#"
+ seed = 5;
+ try
+ eval('seed = 99; error("RunMat:eval:boom", "boom");');
+ catch
+ end
+ rolled_back = seed;
+ "#,
+ )
+ .expect("overwrite rollback exec succeeds");
+ assert!(outcome_has_named_upsert(
+ &overwrite,
+ "rolled_back",
+ &runmat_builtins::Value::Num(5.0)
+ ));
}As per coding guidelines, "Prioritize catching: stale state propagation" and "rollback inconsistencies."
🤖 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 `@crates/runmat-core/src/tests.rs` around lines 9355 - 9390, Extend the test
dynamic_workspace_eval_error_does_not_commit_partial_mutations to also verify
rollback when eval overwrites an existing binding: initialize a workspace
binding (e.g., publish "partial = 42;" via execute_text_request or include it in
the initial source using RunMatSession/execute_text_request), then run the
failing eval that assigns partial = 1; error(...); and assert after the failed
eval that the durable workspace still contains the original value (Num(42.0))
and that no stale mutation was committed; use the same helpers
(execute_text_request, outcome_has_upsert_name/outcome_has_named_upsert, and
outcome.flow.durable_workspace_value()) and keep the existing assertions for the
new-binding case.
| return Ok(Value::OutputList(Vec::new())); | ||
| }; | ||
| let target_vars = unsafe { &mut *frame.vars_ptr }; | ||
| let target_vars = unsafe { (*frame.vars_ptr).clone() }; |
Contributor
There was a problem hiding this comment.
[medium] Unsafe dereference of vars_ptr after releasing workspace stack borrow
Line 467 dereferences frame.vars_ptr outside the WORKSPACE_STACK.with() borrow that obtained the snapshot. While currently safe (no intervening workspace mutations), this creates a fragile invariant. Any future code inserted between workspace_target_snapshot() (line 455) and this dereference that modifies the workspace stack could invalidate the pointer.
Consider cloning vars within workspace_target_snapshot() while the borrow is held, or documenting this invariant explicitly.
🤖 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 `@crates/runmat-vm/src/call/builtins.rs` at line 467, The code dereferences
frame.vars_ptr after releasing the WORKSPACE_STACK.with() borrow, which is
fragile; change workspace_target_snapshot() (or its call site) to clone the
target vars while the WORKSPACE_STACK borrow is still held so no raw pointer is
used afterward: either update workspace_target_snapshot() to return an owned
Vec/Vars clone (or a struct containing the cloned vars) or call
WORKSPACE_STACK.with(|stack| { let snapshot =
workspace_target_snapshot_inner(stack); snapshot.clone_vars() }) and assign that
owned clone to target_vars before leaving the closure; locate this behavior
around workspace_target_snapshot(), WORKSPACE_STACK.with(), frame.vars_ptr and
the let target_vars = unsafe { (*frame.vars_ptr).clone() } line and replace the
unsafe deref with the pre-cloned owned data.
| _What's new across RunMat. See [GitHub Releases](https://github.com/runmat-org/runmat/releases) for runtime release binaries._ | ||
|
|
||
| ## [v0.4.9](https://github.com/runmat-org/runmat/compare/v0.4.8...v0.4.9) | ||
| ## [v0.5.0 ](https://github.com/runmat-org/runmat/compare/v0.4.9...v0.5.0) - June 2026 |
Contributor
There was a problem hiding this comment.
[low] Remove trailing space inside the v0.5.0 link text.
[v0.5.0 ](...) violates markdown lint (MD039) and can cause noisy CI/docs checks.
Suggested fix
-## [v0.5.0 ](https://github.com/runmat-org/runmat/compare/v0.4.9...v0.5.0) - June 2026
+## [v0.5.0](https://github.com/runmat-org/runmat/compare/v0.4.9...v0.5.0) - June 2026📝 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
| ## [v0.5.0 ](https://github.com/runmat-org/runmat/compare/v0.4.9...v0.5.0) - June 2026 | |
| ## [v0.5.0](https://github.com/runmat-org/runmat/compare/v0.4.9...v0.5.0) - June 2026 |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 5-5: Spaces inside link text
(MD039, no-space-in-links)
🤖 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 `@docs/CHANGELOG.md` at line 5, The markdown header contains a trailing space
inside the link text "[v0.5.0
](https://github.com/runmat-org/runmat/compare/v0.4.9...v0.5.0)"; remove the
extra space so the link text reads "[v0.5.0](...)" to satisfy MD039 and stop the
lint/CI noise—update the line containing that link text accordingly.
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
3 participants
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.
Note
Medium Risk
Touches core dynamic workspace/eval commit semantics and native eval threading; incorrect ordering could regress eval/evalin behavior or workspace visibility.
Overview
Fixes dynamic
eval/evalinworkspace updates so a failing evaluation no longer leaves partial assignments visible in the durable workspace (e.g. variables set beforeerror()inside atry/catch).Dynamic eval now clones the target workspace variable vector, runs interpretation on that copy, and only after a completed outcome applies changes through
replace_workspace_target_vars_and_state(replacing both slot values and name/assigned metadata). Workspace metadata updates are no longer applied on the in-flight buffer while the eval may still error.On native (non-WASM) hosts, the dynamic eval worker moves from
spawn_scopedto a detached thread that returns results over a oneshot channel, still with a dedicated stack size and source-catalog restoration.Adds an integration regression test for the partial-mutation case and expands
docs/CHANGELOG.mdwith a v0.5.0 release section plus dated headings on older entries.Reviewed by Cursor Bugbot for commit 1d8cfda. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests