feat(code-runner): run untrusted Node.js and Python in-process - #771
Conversation
One worker exposing `sandbox-code-runner`'s API — `run` / `register_function` / `teardown` / `inject-guidance` — over two in-process engines: untrusted JavaScript in deno_core V8 isolates, and untrusted Python as CPython compiled to WebAssembly inside wasmtime. No microVM and no /dev/kvm, which is the reason it exists. The engine internals are two new bus-free libraries, `crates/node-core` (`iii-node-core`) and `crates/python-core` (`iii-python-core`). Neither depends on `iii-sdk` at all — not feature-gated, dependency absent — so neither can collide with the pin the workers carry. `code-runner` owns the only bus seam and translates core errors into its own taxonomy. The load-bearing semantic, inherited from `sandbox-code-runner`: a failing script is a RESPONSE, not an error. A tenant exception returns a resolved call with `exit_code: 1` and the traceback in `stderr`; errors are reserved for infrastructure. Host-derived kill signals (`timed_out`, memory, disk) are checked BEFORE any guest-written byte is parsed, because a tenant controls every byte under its output directory and could otherwise forge past an infrastructure kill. Both languages get a capped private working directory and a guest `iii` global. Node reaches the host through ops; Python cannot — `python.wasm` imports one module and exports only `_start` — so its bridge rides `fd_write`/`fd_read` with sentinel framing, and the host publishes registered handlers on the guest's behalf. A kept Python runtime persists its interpreter as well as its files: the wrapper parks on stdin between calls rather than letting `_start` return, so globals stay bound and modules stay imported. Function ids from both engines are claimed in ONE registry, and not under the bare namespace — that is node's own pre-runtime placeholder owner, and a python id claimed there is silently reclaimed by node and then aborts the process from `op_iii_register`'s non-unwinding V8 callback. 366 tests (node-core 217, python-core 86, code-runner 63), fmt and clippy clean, plus a 24-case e2e suite that runs both languages against a real engine. Every containment test names the mutation that makes it fail.
Ships two assets into any running console — `code-runner/page.js` over
`console:script` and `code-runner/styles.css` over `console:style` — built
from `ui/` by esbuild and embedded in the binary, so there is nothing to
install and nothing to serve separately. One function-trigger renderer per
op replaces the console's raw-JSON card, which turns `code` into a single
escaped line and buries the verdict.
Ported from `sandbox-code-runner/ui`, which serves the same API, with three
deliberate divergences for this worker's wire:
**`result` gets its own block.** The completion value is the field this
worker's wire adds, and a null one is information rather than an absence
(run.rs never skips it), so it renders explicitly with the engine's return
convention beside it — node code is a function body (`return 2 + 2`),
python code is a module (assign `result`). That mismatch is the usual
reason a call "worked" and came back null, and it was previously invisible.
**No network chip.** `sandbox-code-runner`'s `network` is a real create-time
flag; this worker has no such field because neither engine has any network.
An "off" chip would imply a knob exists, so there is none — asserted by a
test, since the port would otherwise carry it silently.
**In-process wording throughout.** Teardown disposes a V8 isolate or a
CPython interpreter, not a microVM; a python namespace's interpreter is
pinned, so teardown-by-namespace is the only thing that reclaims it.
`runtime_id` is never rendered in full — it is a capability, so it appears
only as a truncated click-to-copy chip, and every other string is filtered
first: stdout, stderr, the error messages that quote it by design, the
submitted source, the completion value, and the `raw json` tab the console
mounts regardless of what a card does.
Adding the UI reopened a process-abort hole, found by probing rather than
assumed: `ConsoleUi` publishes `code-runner::ui-content`, which was not in
`STATIC_IDS` and so was not seeded into the id registry. Because
`register_function` lets a caller choose any namespace — `code-runner::`
included — an unseeded worker id is claimable, and the claim reaches the
SDK's `register_function` on an already-registered id, whose duplicate-id
panic aborts the process from a non-unwinding V8 callback.
`register_function("code-runner::ui-content", …)` returned
`registered: true` before the fix. There is now a `seeded_ids()` that is
strictly larger than `STATIC_IDS`, used by both production and the test
harness so they cannot diverge, and a test that walks every owned id.
72 worker tests (up from 64) and 11 vitest cases; the mutations for the
seeding fix, the result redaction, the null-result block and the
convention hint were each verified to fail their test. fmt, clippy, both CI
validators and biome on `ui/` all clean.
…gine name Observed live: an agent wrote Node-idiom `global.counter`, got `ReferenceError: global is not defined`, and the stack frame said `[node-engine:eval]` — a worker name that no longer exists on the bus. Sweeping that defect class off every user-visible surface: - guidance: name the `global`/`globalThis` trap (`lang: "node"` primes the Node mental model, and 'NOT Node' alone didn't transfer), and quote the REAL id-clash wire code — `code-runner::invalid_request`, not the `id_taken` code this worker's taxonomy never had - script origins: `[code-runner:eval]`/`:prelude`/`:invoke`/`:namespace`, with the definition-error frame matcher renamed in lockstep (mutation re-verified: a leaking formatCause fails the test on the NEW names) - prelude: `iii.shutdown()` now points at `code-runner::teardown` (the old message named a function nobody can call), `String(iii)` introduces a code-runner host client, registerFunction's http refusal ditto - default namespace mint: `code-runner::<runtime_id>::` — guests were publishing live bus ids under the retired prefix - error seams: NodeEngineError::message() is now pub, and ops + translate surface it instead of Display — callers saw double-coded messages like `code-runner::invalid_request: node-engine::id_taken: …`; new test pins that re-coded messages never carry the core's own prefix - tenant log stream: the truncation notice, thread names, tempdir prefix and tracing target follow The core's internal `node-engine::<code>` tags stay: after the seam fix they reach no user surface, and re-coding is the worker's job.
…e-only Stale since the python registration path landed; the catalog description steered agents away from a working feature. Golden regenerated — the one description string, no schema drift.
… to the catalog
engine::functions::info showed REQUEST/RESPONSE "any" for every dynamic
registration: the SDK auto-extracts schemas from the handler's types, and a
dynamic handler is Value → Value. The SDK's escape hatch already existed —
RegisterFunction::request_format/response_format override the extraction —
nothing here reached it.
Two optional wire fields, `request_format`/`response_format` (a superset
of sandbox-code-runner's contract, like `result`), threaded to that
builder:
- python: one hop — the host publishes directly through `Engine::register`,
which gains the two params (IIIEngine, FakeEngine, TestBus follow)
- node: the whole guest chain — wire → `wrap_register` embeds them as JSON
literals in the generated `__def` → the prelude's options object →
`op_iii_register` (two new args, "" = absent) → `Engine::register`
- byproduct: guest code gets the same surface for free —
`iii.registerFunction(id, h, {request_format, response_format})`
One validation rule at both trust boundaries (wire::register::
validate_format, shared so they cannot drift): a JSON OBJECT carrying at
least one schema-defining keyword (an empty object IS the "any" being
replaced), 16 KiB serialized cap. The worker wire checks before anything is
claimed or booted; the op re-checks because the prelude's own check runs on
tenant-replaceable builtins.
The op's redeploy early-return is extended: it skipped the bus write
whenever the DESCRIPTION was absent, which would silently discard formats —
re-registration is a wholesale metadata replacement, never a merge, and the
comment now says so.
Mutations run for real: node arm dropping the fields from core_req → the
both-languages capture test fails; the swap-branch condition left on
description alone → the redeploy-republishes test fails. The final hop
(IIIEngine's builder calls) has no unit seam — the new e2e case covers it
against a live engine by reading functions::info back.
node-core 227 tests (was 217), code-runner 68+7 (was 66+7); goldens
regenerated (new request fields + REGISTER_DESC sentence, no other drift);
guidance, README and worker catalog updated.
…nventions up front Two dogfooding findings, both documentation holes: - request_format/response_format read as a contract but are catalog metadata: nothing on this bus validates payloads against them, and an agent that reads the schema in functions::info skips its own input checks. REGISTER_DESC and the guidance now say so and tell handlers to validate. - the return-vs-result convention was taught only reactively (the null card, python's SyntaxError in stderr): the injected guidance mentioned python ZERO times, and RUN_DESC never stated either convention, so an agent's first python run was written blind. RUN_DESC now names both (node = function body, return x; python = module, result = x) and the guidance gains its first python clause — lang, conventions, sync iii, /work. Guidance needles pin both clauses; goldens regenerated, descriptions only.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (30)
📝 WalkthroughWalkthroughThe PR adds a new Rust ChangesWorker contracts and bootstrap
Node and Python engines
Worker orchestration
Console UI
Validation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
skill-check — worker0 verified, 58 skipped (no docs/).
Four for four. Nicely done. |
The committed config.yaml is gone; the configuration worker's code-runner entry is the authoritative runtime config (--config is a one-time seed). Output caps and timeouts are load()ed per call from an ArcSwap snapshot the configuration:updated trigger swaps, so they hot-apply; the engine-structural fields stay boot-captured and the reload handler warns that they apply at the next restart. on-config-change is seeded into the runtime-id registry and denied to agents.
host.configForms.register('code-runner', …) replaces the generic
schema-driven form on the Workers tab: sections carry the reload split
(output caps + timeouts hot-apply on save, runtime/memory/scratch apply at
the next restart), byte fields show KiB/token hints, and the scratch section
computes the worst-case host footprint.
The injected harness guidance already teaches the full surface; the catalog description now carries contract essentials only (~1/3 the tokens per read) and no longer points at other workers — the sandbox-code-runner mentions are gone from RUN_DESC and the Lang schema doc.
inject_guidance (default true, hot-apply) silences the pre-generate guidance hook: disabled, the handler answers with the no-op mutation so the harness prompt is untouched. The hook stays bound — one no-op roundtrip per generation while off. Toggle ships in the console config form under a new agent-guidance section.
each_turn_is_budgeted_on_its_own_timeout gave the first turn 600ms, which a trivial turn on a slow shared CI runner blows (3/3 CI failures; passes on a fast local box). First-turn budget 600ms -> 3s and the second turn's sleep 1.5s -> 4s, preserving the discrimination: the sleep still exceeds the first-turn budget, so a leaked budget still trips the assertion.
# Conflicts: # .github/release-workers.yaml
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
code-runner/src/functions/run.rs-8-10 (1)
8-10: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the language-specific execution contract.
Line 8 states that code runs as a whole file. Lines 8-10 state that every runtime uses
iii.files. Line 21 instead describes Node as an async function body and Python as a module with/workinstead ofiii.files. These public contracts cannot both be correct.
code-runner/src/functions/run.rs#L8-L10: document the actual Node and Python execution and filesystem behavior separately.code-runner/src/functions/inject_guidance.rs#L21-L21: match the same contract and add coverage for the filesystem and execution wording.🤖 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 `@code-runner/src/functions/run.rs` around lines 8 - 10, Align the public execution-contract documentation in code-runner/src/functions/run.rs lines 8-10 and code-runner/src/functions/inject_guidance.rs line 21: describe Node as an async function body and Python as a module, and document each language’s actual filesystem behavior consistently instead of claiming all runtimes use iii.files. Add or update coverage for both the execution and filesystem wording, with both files expressing the same contract.code-runner/src/functions/register.rs-8-10 (1)
8-10: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument namespace ownership per language.
code-runner/src/manager.rssupports Node and Python runtimes under the same namespace. Line 10 instead says later IDs must share the first registration language. Lines 40-42 and line 21 describe one runtime per namespace. This prevents callers from using a supported registration layout.
code-runner/src/functions/register.rs#L8-L10: state that a namespace can contain registrations for both languages.code-runner/src/functions/mod.rs#L39-L45: replace “one persistent runtime per namespace” with one runtime per(namespace, lang).code-runner/src/functions/inject_guidance.rs#L21-L21: describe the same per-language runtime model and namespace teardown behavior.🤖 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 `@code-runner/src/functions/register.rs` around lines 8 - 10, Update the documentation in code-runner/src/functions/register.rs lines 8-10 to state that a namespace may contain registrations for both supported languages. In code-runner/src/functions/mod.rs lines 39-45, describe runtime ownership as one runtime per (namespace, lang), not one per namespace. In code-runner/src/functions/inject_guidance.rs line 21, document the same per-language runtime model and clarify namespace teardown behavior.code-runner/README.md-59-59 (1)
59-59: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSpecify a language for both fenced code blocks.
Markdownlint reports MD040 for these fences. Use
textfor the output examples.Also applies to: 78-78
🤖 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 `@code-runner/README.md` at line 59, Update both fenced code blocks in the README to specify the text language by adding the text fence identifier, including the output example near the referenced second location, while leaving their contents unchanged.Source: Linters/SAST tools
code-runner/build.rs-23-31 (1)
23-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTrack
packages/console-uiwhen deciding whether to rebuild UI assets.
ui/package.jsonlinks the workspace package, but neither invalidation path watches it. A change inpackages/console-uican leave stalepage.jsembedded in the worker.Add the package directory to
cargo:rerun-if-changedand todist_is_fresh.Proposed fix
println!("cargo:rerun-if-changed=ui/package.json"); +println!("cargo:rerun-if-changed=../packages/console-ui"); println!("cargo:rerun-if-changed=../pnpm-lock.yaml");- for dir in [ui_dir.join("src")] { + for dir in [ + ui_dir.join("src"), + ui_dir.join("../../packages/console-ui"), + ] {Also applies to: 107-131
🤖 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 `@code-runner/build.rs` around lines 23 - 31, Update the build script’s UI change tracking to include the packages/console-ui directory: add it to the cargo:rerun-if-changed declarations and include the same path in dist_is_fresh so changes to the linked workspace package trigger rebuilding embedded UI assets.code-runner/tests/e2e/workers/harness/src/cases-errors.ts-57-59 (1)
57-59: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the configured stream cap.
Line 58 states that each stream has a 1 MiB cap. Line 59 accepts up to 2 MiB. A regression that doubles the stdout cap passes this test. Set the bound to 1 MiB, plus a defined truncation-marker allowance if the response adds one.
🤖 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 `@code-runner/tests/e2e/workers/harness/src/cases-errors.ts` around lines 57 - 59, Update the stdout length assertion in the error-case harness to enforce the stated 1 MiB stream cap, allowing only a clearly defined additional amount if truncation adds a marker. Keep the assertion aligned with the cap described by the surrounding comment so a doubled stdout limit fails.code-runner/tests/e2e/run-tests.sh-121-121 (1)
121-121: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the missing-result diagnostic.
At Line 121,
grepexits with status 1 when the harness produces noHARNESS_DONEline. Witherrexitenabled, the script exits before Lines 125-127 print the intended failure message.Proposed fix
-SUITE_LINE="$(grep -a 'HARNESS_DONE' "$LOGS/harness.log" | tail -1)" +SUITE_LINE="$(grep -a 'HARNESS_DONE' "$LOGS/harness.log" | tail -1 || true)"🤖 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 `@code-runner/tests/e2e/run-tests.sh` at line 121, Update the HARNESS_DONE lookup in the test script so a missing match does not trigger errexit before the diagnostic handling at Lines 125-127 runs. Preserve the existing SUITE_LINE assignment and ensure the no-result path reaches the intended failure message.code-runner/tests/e2e/workers/harness/src/cases-register.ts-142-145 (1)
142-145: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the schema assertion effective.
After Line 144 passes,
raw.includes('fmt_marker_n')is true. Therefore, Line 145 always passes and does not detect an"any"schema. Assert the selected catalog entry has the expected request schema and does not retain the fallback schema.Proposed fix
- expect(!raw.includes('"any"') || raw.includes('fmt_marker_n'), 'schema should replace any') + expect(!raw.includes('"any"'), 'schema should replace any')🤖 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 `@code-runner/tests/e2e/workers/harness/src/cases-register.ts` around lines 142 - 145, Update the assertions in the test case around the engine info request so they inspect the selected catalog entry’s request schema directly. Assert that the schema contains the expected fmt_marker_n value and separately assert that it does not contain the fallback "any" schema; do not combine the checks with an OR that becomes true whenever fmt_marker_n is present.crates/node-core/src/wire/run.rs-21-30 (1)
21-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRetired
node-enginename reaches callers through the derived schemas. These doc comments become field descriptions in the schemars-derived JSON Schema, andcode-runner/src/functions/mod.rspublishes those schemas in the worker catalog. This PR retiresnode-engine, so callers read a worker name and a function id that no longer exist. The internalnode-engine::<code>error strings incrates/node-core/src/error.rsare a separate, documented decision and are out of scope here.
crates/node-core/src/wire/run.rs#L21-L30: replacenode-engine::teardownwith the id the worker actually registers, and correct the stated default namespace prefixnode-engine::<runtime_id>::after confirming the valuemanager.rsproduces.crates/node-core/src/wire/register.rs#L6-L9: replace "node-engine keeps one runtime per namespace" with thecode-runnerworker name.Regenerate the golden schemas under
code-runner/tests/golden/schemas/after the edit, because the description text is part of those fixtures.🤖 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/node-core/src/wire/run.rs` around lines 21 - 30, Update the field documentation in crates/node-core/src/wire/run.rs#L21-L30 to use the actual registered code-runner worker name and function id instead of retired node-engine identifiers, and verify the namespace prefix against manager.rs before correcting it; update crates/node-core/src/wire/register.rs#L6-L9 to name the code-runner worker. Regenerate the golden schemas under code-runner/tests/golden/schemas/ so the published descriptions match.code-runner/src/ui.rs-131-134 (1)
131-134: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis assertion matches more than a bare
reactimport.The raw string
r#"from "react"#ends before the closing quote, so it matchesfrom "react. It also matchesfrom "react-dom/client"andfrom "react-is". A bundle that imports onlyreact-domwhile inlining React source would pass. Match the full specifier.Proposed fix
- assert!( - PAGE_JS.contains(r#"from "react"#), - "react should be imported, not bundled" - ); + assert!( + PAGE_JS.contains(r#"from "react""#), + "react should be imported, not bundled" + );🤖 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 `@code-runner/src/ui.rs` around lines 131 - 134, Update the PAGE_JS assertion to match the complete React module specifier, including the closing quote, so imports such as react-dom/client or react-is do not satisfy it; keep the assertion anchored to the existing “react should be imported, not bundled” check.code-runner/src/manager.rs-546-569 (1)
546-569: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA registered python handler pins its timeout at registration time.
py_handlerreadsself.cfg.load().default_timeout_msonce, outside the closure, and captures the value. A laterconfiguration:updatedswap does not reach handlers that are already registered. The struct doc on Lines 38-42 states that per-call knobs, including timeouts, areload()ed at each use so they hot-reload.py_define_handlerfollows that rule; this path does not.Proposed fix: read the snapshot per invocation
fn py_handler(&self, python: &Arc<PythonManager>, runtime_id: &str) -> ProxyHandler { let python = python.clone(); let runtime_id = runtime_id.to_string(); - let timeout_ms = self.cfg.load().default_timeout_ms; + let cfg = self.cfg.clone(); Arc::new(move |payload: serde_json::Value| { let python = python.clone(); let runtime_id = runtime_id.clone(); + let timeout_ms = cfg.load().default_timeout_ms; Box::pin(async move {🤖 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 `@code-runner/src/manager.rs` around lines 546 - 569, Update py_handler so default_timeout_ms is loaded from self.cfg for each handler invocation rather than captured during registration. Capture or clone the configuration handle in the closure, then read the current snapshot inside the async call when constructing RunRequest; preserve the existing timeout and result/error behavior.crates/python-core/src/manager.rs-548-566 (1)
548-566: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe byte cap does not count the appended newlines.
The check at Line 554 compares
kept.len() + line.len()againstMAX_LOG_BYTES, but Line 559 appends a'\n'that the check never accounts for. A stream of many short lines can therefore exceedMAX_LOG_BYTESby up toMAX_LOG_LINESbytes.text.lines()also strips the original line terminators, so a stream that ends without a newline gains one.🐛 Proposed fix
for (n, line) in text.lines().enumerate() { - if n >= MAX_LOG_LINES || kept.len() + line.len() > MAX_LOG_BYTES { + if n >= MAX_LOG_LINES || kept.len() + line.len() + 1 > MAX_LOG_BYTES { truncated = true; break; }🤖 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/python-core/src/manager.rs` around lines 548 - 566, Update the cap closure in capture_streams to include the appended newline in the MAX_LOG_BYTES check, including for the final unterminated line produced by text.lines(). Preserve the existing MAX_LOG_LINES limit and set truncated when adding the line plus its newline would exceed the byte cap.crates/python-core/src/artifact.rs-31-39 (1)
31-39: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDo not panic when
HOMEis unset.
ensure_extractedruns on the worker boot path. If neitherXDG_CACHE_HOMEnorHOMEis set,expectpanics and aborts the worker instead of returning an error. Containers frequently run withoutHOME. Makecache_rootfall back to a temporary directory, or returnResultso the caller reports a normal engine start failure.🛡️ Proposed fix
fn cache_root() -> PathBuf { let base = std::env::var_os("XDG_CACHE_HOME") .map(PathBuf::from) - .unwrap_or_else(|| { - let home = std::env::var_os("HOME").expect("HOME must be set"); - PathBuf::from(home).join(".cache") - }); + .or_else(|| std::env::var_os("HOME").map(|h| PathBuf::from(h).join(".cache"))) + .unwrap_or_else(std::env::temp_dir); base.join("iii").join("python-engine").join(ZIP_SHA256) }🤖 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/python-core/src/artifact.rs` around lines 31 - 39, Update cache_root so it does not call expect when HOME is unset. Prefer falling back to an appropriate temporary directory when neither XDG_CACHE_HOME nor HOME is available, while preserving the existing cache subpath; alternatively, change cache_root and its ensure_extracted caller to return and propagate a normal error instead of panicking.
🧹 Nitpick comments (17)
code-runner/ui/src/configuration/index.tsx (1)
43-49: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
bytesHintreports~0 tokensfor small non-zero caps.
Math.round(bytes / 4 / 100) * 100rounds to zero for any cap below 200 bytes. An operator who setsmax_stream_bytesto 128 then reads "≈0.1 KiB (~0 tokens)", which reads as "off" even though the cap is active. Consider a floor of 100 tokens for non-zero caps.🤖 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 `@code-runner/ui/src/configuration/index.tsx` around lines 43 - 49, Update bytesHint so any non-zero byte cap reports at least 100 tokens, while preserving the existing rounded estimate for larger caps and the special “off” message for zero bytes.code-runner/ui/page.tsx (1)
25-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollect the
configFormsremover in the teardown.
host.configForms.registerreturns a remover (packages/console-ui/index.d.ts:201-226). Line 27 discards it, so the early teardown removes the three renderers but leaves thecode-runnerconfig form registered. If the loader disposal is the only cleanup for the form, the returned function is inconsistent with its own comment.♻️ Proposed fix
export default function setup(host: Host) { - const removers = createCodeRunnerRenderers(host).map((renderer) => host.functionTriggers.register(renderer)) - - host.configForms.register('code-runner', CodeRunnerConfigForm) + const removers = createCodeRunnerRenderers(host).map((renderer) => host.functionTriggers.register(renderer)) + + removers.push(host.configForms.register('code-runner', CodeRunnerConfigForm)) // The loader already disposes every registration; returning the removers // makes an early teardown (hot reload mid-session) explicit and ordered. return () => { for (const remove of removers) remove() } }🤖 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 `@code-runner/ui/page.tsx` around lines 25 - 33, Capture the remover returned by host.configForms.register in the setup flow alongside the renderer removers, then invoke it during the returned teardown so both renderer registrations and the code-runner config form are removed in order.code-runner/ui/styles.css (1)
64-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the infinite pulse animation with
prefers-reduced-motion.
.cr-ui-msg-note.pulseanimates opacity forever while a call is in flight. Users who request reduced motion get no way to stop it.♻️ Proposed fix
`@keyframes` cr-ui-pulse { 0%, 100% { opacity: 1 } 50% { opacity: 0.35 } } +@media (prefers-reduced-motion: reduce) { + [data-iii-ui="code-runner"] .cr-ui-msg-note.pulse { + animation: none; + } +}🤖 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 `@code-runner/ui/styles.css` around lines 64 - 70, Guard the infinite animation on .cr-ui-msg-note.pulse with a prefers-reduced-motion media query so users who request reduced motion do not receive the pulse effect. Keep the existing cr-ui-pulse animation unchanged for users without that preference.code-runner/ui/src/lib/shared.tsx (1)
147-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the textarea on the
execCommandfailure path.If
ta.select()ordocument.execCommand('copy')throws, line 165 never runs and the hidden textarea stays indocument.body. Every failed copy then adds one node. Move the removal into afinallyblock.🤖 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 `@code-runner/ui/src/lib/shared.tsx` around lines 147 - 170, Update copyText so the temporary textarea created in the fallback path is always removed, including when ta.select() or document.execCommand('copy') throws. Move document.body.removeChild(ta) into a finally block while preserving the existing success result and false-on-error behavior.code-runner/tests/e2e/workers/harness/src/cases-run.ts (1)
27-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert that the
runspec was found.If
engine::functions::inforeturns entries without afunction_idfield,findreturnsundefinedand line 29 throws a TypeError. The case then reports a property-access error instead of a readable assertion.♻️ Proposed guard
const run = found.find((f: any) => f.function_id === 'code-runner::run') + expect(!!run, `functions::info returned no entry for code-runner::run: ${JSON.stringify(found)}`) expect(🤖 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 `@code-runner/tests/e2e/workers/harness/src/cases-run.ts` around lines 27 - 31, Assert that the `run` result from `found.find(...)` exists before accessing `run.request_schema`, using a clear failure message; keep the existing required-`code` assertion for the found spec unchanged.code-runner/tests/schemas.rs (1)
100-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
support::golden_root()andgolden_file_namefor the on-disk checks.Lines 106, 107, and 131 hardcode CWD-relative paths and repeat the
::→.mapping already implemented at line 13. The paths resolve only because Cargo sets the test working directory to the package root.♻️ Proposed refactor
- assert!(std::path::Path::new("tests/golden/schemas/code-runner.run.json").exists()); - assert!(!std::path::Path::new("tests/golden/schemas/code-runner.eval.json").exists()); + assert!(support::golden_root().join(golden_file_name("code-runner::run")).exists()); + assert!(!support::golden_root().join(golden_file_name("code-runner::eval")).exists());- let golden = format!("tests/golden/schemas/{}.json", gone.replace("::", ".")); + let golden = support::golden_root().join(golden_file_name(gone)); assert!( - !std::path::Path::new(&golden).exists(), - "{golden} still exists on disk" + !golden.exists(), + "{} still exists on disk", + golden.display() );🤖 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 `@code-runner/tests/schemas.rs` around lines 100 - 137, Update the on-disk golden checks in the_advertised_ids_agree_across_catalog_static_ids_and_goldens and removed_functions_are_absent_from_the_catalog to use support::golden_root() with golden_file_name instead of hardcoded CWD-relative paths and manual "::" to "." conversion. Preserve the existing existence and absence assertions.code-runner/tests/e2e/workers/harness/src/runner.ts (1)
76-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the TTY color constants into one shared helper. Both files derive the same ANSI escapes from
process.stdout.isTTY, so the palette is maintained twice.
code-runner/tests/e2e/workers/harness/src/runner.ts#L76-L80: move this block into a small exported helper (for examplecolors()incases.tsor a newcolors.ts) and consume it here.code-runner/tests/e2e/workers/harness/src/worker.ts#L19-L22: import the same helper instead of redeclaring GREEN, RED, and RESET.🤖 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 `@code-runner/tests/e2e/workers/harness/src/runner.ts` around lines 76 - 80, Extract the shared TTY-based ANSI palette from runner.ts lines 76-80 into one exported helper, such as colors(), and update runner.ts to consume it. In worker.ts lines 19-22, import and use the same helper instead of redeclaring GREEN, RED, and RESET; preserve the current non-TTY empty-string behavior.crates/node-core/src/ids.rs (1)
36-85: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the lock acquisitions panic-free.
The module doc states that a panic reaching an
extern "C"op callback aborts the process and kills every tenant's runtime..lock().unwrap()panics if the mutex is poisoned, so the current code depends on no critical section ever panicking. Recover the guard instead. The map stays consistent, because every critical section here is a complete insert, remove, or retain.♻️ Proposed change to recover a poisoned guard
-use std::sync::{Arc, Mutex}; +use std::sync::{Arc, Mutex, MutexGuard, PoisonError}; + +/// A poisoned guard is still a consistent map: every critical section below +/// is one complete insert, remove or retain. Recovering beats `unwrap`, +/// which would panic out of `op_iii_register` and abort the process. +fn lock<T>(m: &Mutex<T>) -> MutexGuard<'_, T> { + m.lock().unwrap_or_else(PoisonError::into_inner) +}pub fn claim(&self, id: &str, owner: &str) -> bool { - let mut map = self.0.lock().unwrap(); + let mut map = lock(&self.0);Apply the same substitution in
claim_all,release_ids,release_owner, and theDebugimpl.🤖 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/node-core/src/ids.rs` around lines 36 - 85, Replace every `.lock().unwrap()` in the IDs implementation, including `claim`, `claim_all`, `release_ids`, `release_owner`, and the `Debug` implementation, with poisoned-mutex recovery that extracts and reuses the inner guard. Preserve the existing critical-section behavior and return values while ensuring lock acquisition cannot panic.crates/node-core/src/error.rs (1)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the module documentation with the conversion path
code-runnerpasses recoded errors througherr.message(), notError::Handler(e.to_string()). Update the module documentation to describe the actualtranslate::node_errpath.🤖 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/node-core/src/error.rs` around lines 1 - 9, Update the module-level documentation in error.rs to describe the actual translate::node_err conversion path used by code-runner, including that recoded errors pass through err.message(). Remove the inaccurate reference to Error::Handler(e.to_string()) while preserving the stable wire-code taxonomy description.code-runner/src/truncate.rs (1)
160-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen this test's assertions.
cappedis aString, sois_char_boundary(0)andfrom_utf8(...).is_ok()are always true. The test still catches a boundary bug, because slicing at a non-boundary panics, but the assertions themselves prove nothing. Assert the observable contract instead: the marker is present, and the head and tail hold wholeécharacters.Proposed stronger assertions
let s: String = "é".repeat(20_000); // 2-byte chars let capped = cap_stream("stdout", s, 16_384); - assert!(capped.is_char_boundary(0) && std::str::from_utf8(capped.as_bytes()).is_ok()); + let (head, rest) = capped.split_once("\n[…stdout").expect("marker present"); + let tail = rest.split_once("]\n").expect("marker closed").1; + assert!(head.chars().all(|c| c == 'é'), "head lost a character"); + assert!(tail.chars().all(|c| c == 'é'), "tail lost a character");🤖 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 `@code-runner/src/truncate.rs` around lines 160 - 165, Strengthen stream_truncation_respects_utf8_boundaries by removing the tautological UTF-8 checks and asserting cap_stream’s observable output: verify the truncation marker is present, and confirm both the retained head and tail consist of complete é characters rather than partial bytes.crates/python-core/tests/runner.rs (1)
69-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe assertion depends on Wasmtime's trap text.
msg.contains("wasm trap: interrupt")couples this test to a Wasmtime display string. The exact pinwasmtime = "=47.0.3"protects it today, but a pin bump can break the test for a wording change rather than a behavior change. IfExitKind::Trapcan carry a structured cause, match on that instead. Otherwise add a comment that links the assertion to the pin.🤖 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/python-core/tests/runner.rs` around lines 69 - 75, Update the ExitKind::Trap assertion in the runner test to validate a structured trap cause instead of matching Wasmtime’s display text, if that information is available. If ExitKind::Trap only exposes the rendered message, add a concise comment documenting its dependency on the exact wasmtime = "=47.0.3" pin.crates/python-core/src/wrapper.py (2)
46-52: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueA malformed bridge config kills the run before any envelope is written.
Line 47 catches only
OSError. Ifiii.jsonis present but truncated,json.loadraisesJSONDecodeError, and Line 52 raisesKeyErrorwhensentinelis absent. Both escapemain, so the guest exits without writing/out/result.json, and the host reports an unattributable exit. The host writes this file, so the case is not tenant-reachable, but a partial write during a crash is possible. Catch(OSError, ValueError, KeyError)and treat it as "no bridge".🤖 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/python-core/src/wrapper.py` around lines 46 - 52, Update the bridge config loading in main to catch OSError, ValueError, and KeyError from opening, parsing, or reading cfg["sentinel"], and return (False, None) for any malformed or unavailable iii.json so the run proceeds as “no bridge.”
21-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
write_errordoes not captb_text.The module comment states that the cap lives here "so no future caller can forget", but only
messageis capped.tb_textis truncated at the call site at Line 136. A future caller that passes an untruncated traceback builds an envelope the host may refuse. Move the traceback cap intowrite_error.♻️ Proposed refactor
def write_error(out_dir, kind, message, tb_text=None): if len(message) > MAX_MESSAGE_CHARS: message = message[:MAX_MESSAGE_CHARS] + "... message truncated" + if tb_text is not None and len(tb_text) > MAX_TRACEBACK_CHARS: + tb_text = tb_text[:MAX_TRACEBACK_CHARS] + "\n... traceback truncated" with open(out_dir + "/result.json", "w") as f:🤖 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/python-core/src/wrapper.py` around lines 21 - 25, Update write_error to apply MAX_MESSAGE_CHARS truncation to tb_text before serializing result.json, using the same truncation behavior as message. Remove the redundant traceback truncation at the caller so write_error is the single enforcement point for all traceback inputs.crates/python-core/src/manager.rs (1)
399-438: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
work_diris alwaysNoneat the only call site.
runis the sole caller ofrun_with, and it passesNone. The parameter adds an unused path through validation and permit acquisition. If no other caller is planned, inline the body intorunand drop the 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 `@crates/python-core/src/manager.rs` around lines 399 - 438, The run_with method receives work_dir as None from its sole caller, so remove the unnecessary parameter and inline its validation, permit acquisition, RunSpec construction, runner invocation, and classify flow into run. Update the run call path accordingly while preserving the existing behavior and error handling.crates/python-core/tests/wrapper.rs (1)
176-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
run_wrapperinstead of duplicating its setup.Lines 186-205 repeat the whole harness from
run_wrapperonly to learnout_dirbefore the run. Giverun_wrapperan optional placeholder substitution, or return the directories, and the duplicate block disappears.♻️ Sketch
-fn run_wrapper(code: &str, payload: Option<&str>) -> Run { +/// `code` may contain the literal `OUT`, which is replaced with the real +/// output directory before the run. +fn run_wrapper(code: &str, payload: Option<&str>) -> Run { let dir = tempfile::tempdir().unwrap(); let run_dir = dir.path().join("run"); let out_dir = dir.path().join("out"); fs::create_dir_all(&run_dir).unwrap(); fs::create_dir_all(&out_dir).unwrap(); fs::write(run_dir.join("main.py"), WRAPPER).unwrap(); - fs::write(run_dir.join("code.py"), code).unwrap(); + fs::write( + run_dir.join("code.py"), + code.replace("OUT", out_dir.to_str().unwrap()), + ) + .unwrap();🤖 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/python-core/tests/wrapper.rs` around lines 176 - 210, Refactor envelope_is_written_after_tenant_code_finishes to reuse the existing run_wrapper test helper instead of duplicating temporary-directory creation, wrapper setup, Python lookup, command execution, and output parsing. Extend run_wrapper with the minimal optional placeholder-substitution or directory-return capability needed for the tenant code to target out_dir, while preserving the test’s assertion that the final envelope contains “genuine”.crates/python-core/tests/artifact.rs (1)
26-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe test name claims a cache hit that the assertions do not prove.
The second
load_modulecall only asserts success. A regression that ignored the persistedcwasmand recompiled would still pass. Compare the file modification time, or measure that the second load is materially faster, to make the cache-hit claim observable.🤖 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/python-core/tests/artifact.rs` around lines 26 - 40, Update the test module_compiles_and_second_load_hits_cwasm_cache to record the persisted cwasm modification time after the first load, then verify after the second load that the file remains unchanged, proving the cached artifact was reused rather than rewritten by recompilation.crates/python-core/src/config.rs (1)
17-49: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider validating
max_concurrent_runsandmax_runtimesagainst zero as well.
Semaphore::new(0)inManager::newpermits nothing, so a config ofmax_concurrent_runs: 0makes every run hang until shutdown rather than fail. Amax_runtimes: 0makes everycreate_runtimereturnCapacity. A customDeserializecheck, or avalidate()called at load time, turns both into a loud startup error.🤖 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/python-core/src/config.rs` around lines 17 - 49, Validate PythonEngineConfig values so max_concurrent_runs and max_runtimes must both be greater than zero before Manager::new or runtime creation uses them. Add the check through the existing deserialization or configuration-loading path, ensuring invalid zero values produce a startup error rather than silently disabling runs or capacity.
🔇 Additional comments (58)
code-runner/ui/build.mjs (1)
26-47: LGTM!code-runner/ui/package.json (1)
1-23: LGTM!code-runner/ui/src/function-trigger-message/index.tsx (1)
43-47: LGTM!code-runner/ui/src/function-trigger-message/register-function.tsx (2)
113-137: LGTM!Also applies to: 214-258, 283-305
90-100: 🎯 Functional CorrectnessDo not change
str; TypeScript 5.9.3 narrowsobj[k]for this unmodified parameter.> Likely an incorrect or invalid review comment.code-runner/ui/src/function-trigger-message/result.test.tsx (1)
37-43: LGTM!Also applies to: 85-187
code-runner/ui/src/function-trigger-message/run.tsx (1)
94-103: LGTM!Also applies to: 154-182, 201-240, 278-334
code-runner/ui/src/function-trigger-message/teardown.tsx (1)
60-90: LGTM!Also applies to: 92-137, 168-212
code-runner/ui/src/lib/shared.tsx (1)
88-145: LGTM!Also applies to: 306-349, 361-394
code-runner/ui/tsconfig.json (1)
2-13: LGTM!code-runner/ui/styles.css (1)
27-32: 📐 Maintainability & Code QualityNo token changes needed.
The console defines all listed tokens, including
--color-ringand--color-rule-2.code-runner/iii.worker.yaml (1)
1-29: LGTM!code-runner/src/configuration.rs (1)
1-191: LGTM!code-runner/src/error.rs (1)
1-167: LGTM!code-runner/src/lib.rs (1)
1-20: LGTM!code-runner/src/main.rs (1)
1-74: LGTM!Also applies to: 76-93, 99-149, 154-180
code-runner/tests/e2e/workers/harness/src/cases-run.ts (1)
35-112: LGTM!code-runner/tests/e2e/workers/harness/src/groups.ts (1)
1-13: LGTM!code-runner/tests/e2e/workers/harness/src/runner.ts (1)
26-73: LGTM!Also applies to: 82-111
code-runner/tests/golden/schemas/code-runner.inject-guidance.json (1)
1-56: LGTM!code-runner/tests/golden/schemas/code-runner.register_function.json (1)
1-75: LGTM!code-runner/tests/golden/schemas/code-runner.run.json (1)
1-106: LGTM!code-runner/tests/golden/schemas/code-runner.teardown.json (1)
1-61: LGTM!code-runner/tests/manifest.rs (1)
5-31: LGTM!code-runner/tests/schemas.rs (1)
12-94: LGTM!code-runner/tests/support/mod.rs (1)
17-118: LGTM!code-runner/tests/e2e/workers/harness/tsconfig.json (1)
10-10: 📐 Maintainability & Code QualityNo change needed.
code-runner/tests/e2e/workers/harness/package.jsondeclares@types/nodeindevDependencies, so"types": ["node"]is resolvable for this harness.> Likely an incorrect or invalid review comment.crates/node-core/src/allocator.rs (1)
56-95: LGTM!Also applies to: 102-126, 142-163, 180-190
crates/node-core/src/config.rs (1)
11-60: LGTM!Also applies to: 87-115
crates/node-core/src/lib.rs (1)
11-20: LGTM!crates/node-core/src/protocol.rs (1)
17-34: LGTM!Also applies to: 38-67, 157-239
crates/node-core/src/wire/mod.rs (1)
10-12: LGTM!crates/node-core/src/wire/teardown.rs (1)
5-15: 🎯 Functional CorrectnessNo change needed: exactly-one validation is enforced.
RuntimeManager::teardownreturnsinvalid_requestwhen both selectors are absent or both are present.> Likely an incorrect or invalid review comment.crates/node-core/Cargo.toml (1)
28-29: 📐 Maintainability & Code QualityConfirm that
deno_core = "=0.409.0"exposes the requiredv8allocator APIs.crates/node-core/src/prelude.js (2)
443-472:new Functionhere is the worker's purpose: it compiles guest source inside a V8 isolate whose escape surface is theiiiops. The static analysis code-injection findings on Lines 445 and 452 do not apply to this design.Source: Linters/SAST tools
1-128: LGTM!Also applies to: 130-205, 206-263, 265-331, 333-413, 415-514, 516-552
crates/node-core/src/engine.rs (1)
14-153: LGTM!Also applies to: 155-518, 520-621
crates/node-core/src/ops.rs (2)
690-710: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.A cancelled
op_iii_register_triggerleaks apending_registrationsslot.
pending_registrationsis incremented before the await and decremented only after it returns. If the op future is dropped before completion — for example when the watchdog terminates the eval while the runtime is kept for later calls — the counter stays high. The runtime then holds a permanently reduced share ofMAX_REGISTRATIONS_PER_RUNTIMEfor its whole life.inflight_callsdocuments that a leaked counter "dies with theOpsStateit lives in", which holds for a one-shot runtime but not for a kept one.Consider releasing the reservation from a drop guard on the isolate thread, or recomputing the reservation count from a per-call token list.
18-171: LGTM!Also applies to: 173-250, 251-362, 364-428, 430-610, 612-758, 760-928, 930-1036, 1038-1103, 1105-1279, 1281-1308, 1310-1688
crates/node-core/tests/golden/iii-surface.txt (1)
1-13: LGTM!code-runner/src/node_bus.rs (2)
179-186: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.The result of
register_trigger_typeis discarded.
let _ = self.iii.register_trigger_type(...)drops whatever the SDK returns. If that value carries a failure, the caller still receives success:op_iii_register_trigger_typehas already claimed the id, pushed an entry intounregisters, and reported the type inregistered. The guest then believes a trigger type is published while the bus holds nothing.If the SDK returns a
Result, propagate the failure instead. Confirm the return type before changing the code.
1-95: LGTM!Also applies to: 96-178, 188-341
code-runner/src/python_bus.rs (2)
33-53: 🩺 Stability & Availability | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm that
timeout_msis clamped before it reaches this bridge.The node side clamps a guest-supplied timeout to
max_timeout_msinsideop_iii_call. This bridge forwardstimeout_msunchanged. If guest Python can influence the value through its owniii.trigger, a guest can request an engine call far longer than the configured ceiling.
1-31: LGTM!crates/python-core/build.rs (1)
6-24: LGTM!Also applies to: 26-62
code-runner/src/manager.rs (1)
30-137: LGTM!Also applies to: 139-259, 261-304, 315-395, 419-456, 458-492, 494-545, 571-590, 592-726, 728-742, 744-1724
code-runner/src/translate.rs (1)
26-105: LGTM!Also applies to: 107-204, 206-278, 280-341
code-runner/src/truncate.rs (1)
30-55: LGTM!Also applies to: 57-93, 95-123, 129-158
code-runner/src/ui.rs (1)
34-62: LGTM!Also applies to: 68-130, 137-174
crates/python-core/Cargo.toml (1)
41-41: 📐 Maintainability & Code Quality | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that these major versions exist as stable releases.
zip = "8",sha2 = "0.11", andwhich = "8"request major versions that were not stable at my knowledge cutoff.sha2in particular had a long0.11.0-pre.*series, and a caret requirement does not resolve pre-release versions. Confirm each resolves to a published stable release.Also applies to: 47-47, 50-50
crates/python-core/src/artifact.rs (1)
13-13: 🎯 Functional Correctness | 💤 Low value
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that the sidecar digest file carries no trailing newline.
ZIP_SHA256is embedded withinclude_str!and then used as a filesystem path component at Line 38. Ifbuild.rswrites the digest with a trailing newline, the cache directory name contains that newline, and the equality assertion incrates/python-core/tests/artifact.rsagainst the bare hex literal fails.build.rsis not in this review context. Confirm the sidecar content, or trim it at use time.Also applies to: 38-38
crates/python-core/src/error.rs (1)
7-104: LGTM!crates/python-core/src/lib.rs (1)
8-12: LGTM!crates/python-core/src/manager.rs (1)
265-294: LGTM!Also applies to: 314-327, 336-341
crates/python-core/tests/manager.rs (1)
11-37: LGTM!Also applies to: 39-233, 235-273, 280-436, 444-476, 481-661
crates/python-core/tests/persistence.rs (1)
11-45: LGTM!Also applies to: 48-146, 152-209, 223-302, 306-426
crates/python-core/tests/runner.rs (1)
26-49: LGTM!Also applies to: 89-125, 142-194, 196-230, 232-273, 278-325, 331-400, 406-439
crates/python-core/tests/wrapper.rs (1)
17-48: 📐 Maintainability & Code Quality | 💤 Low value
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that the host
python3version matches the bundled interpreter.These tests execute
wrapper.pywith the hostpython3, but production runs it under the bundled CPython-WASM build.crates/python-core/tests/artifact.rsasserts alib/python3.14path, so the bundle is 3.14. A host on an older minor version can accept or reject wrapper syntax differently, which makes this suite pass while the real guest fails. Consider asserting a minimum host version, or documenting the accepted drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ee799a3-925c-4a7f-90de-487036421eef
⛔ Files ignored due to path filters (5)
code-runner/Cargo.lockis excluded by!**/*.lockcode-runner/tests/e2e/workers/harness/package-lock.jsonis excluded by!**/package-lock.jsoncrates/node-core/Cargo.lockis excluded by!**/*.lockcrates/python-core/Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (92)
README.mdcode-runner/Cargo.tomlcode-runner/README.mdcode-runner/build.rscode-runner/iii.worker.yamlcode-runner/src/config.rscode-runner/src/configuration.rscode-runner/src/error.rscode-runner/src/functions/inject_guidance.rscode-runner/src/functions/mod.rscode-runner/src/functions/register.rscode-runner/src/functions/run.rscode-runner/src/functions/teardown.rscode-runner/src/lang.rscode-runner/src/lib.rscode-runner/src/main.rscode-runner/src/manager.rscode-runner/src/manifest.rscode-runner/src/node_bus.rscode-runner/src/python_bus.rscode-runner/src/translate.rscode-runner/src/truncate.rscode-runner/src/ui.rscode-runner/tests/e2e/.gitignorecode-runner/tests/e2e/README.mdcode-runner/tests/e2e/code-runner.config.yamlcode-runner/tests/e2e/config.yamlcode-runner/tests/e2e/reports/.gitkeepcode-runner/tests/e2e/run-tests.shcode-runner/tests/e2e/workers/harness/iii.worker.yamlcode-runner/tests/e2e/workers/harness/package.jsoncode-runner/tests/e2e/workers/harness/src/cases-errors.tscode-runner/tests/e2e/workers/harness/src/cases-keep.tscode-runner/tests/e2e/workers/harness/src/cases-register.tscode-runner/tests/e2e/workers/harness/src/cases-run.tscode-runner/tests/e2e/workers/harness/src/cases.tscode-runner/tests/e2e/workers/harness/src/groups.tscode-runner/tests/e2e/workers/harness/src/runner.tscode-runner/tests/e2e/workers/harness/src/worker.tscode-runner/tests/e2e/workers/harness/tsconfig.jsoncode-runner/tests/golden/schemas/code-runner.inject-guidance.jsoncode-runner/tests/golden/schemas/code-runner.register_function.jsoncode-runner/tests/golden/schemas/code-runner.run.jsoncode-runner/tests/golden/schemas/code-runner.teardown.jsoncode-runner/tests/manifest.rscode-runner/tests/schemas.rscode-runner/tests/support/mod.rscode-runner/ui/build.mjscode-runner/ui/package.jsoncode-runner/ui/page.tsxcode-runner/ui/src/configuration/index.tsxcode-runner/ui/src/function-trigger-message/index.tsxcode-runner/ui/src/function-trigger-message/register-function.tsxcode-runner/ui/src/function-trigger-message/result.test.tsxcode-runner/ui/src/function-trigger-message/run.tsxcode-runner/ui/src/function-trigger-message/teardown.tsxcode-runner/ui/src/lib/shared.tsxcode-runner/ui/styles.csscode-runner/ui/tsconfig.jsoncrates/node-core/Cargo.tomlcrates/node-core/src/allocator.rscrates/node-core/src/config.rscrates/node-core/src/engine.rscrates/node-core/src/error.rscrates/node-core/src/ids.rscrates/node-core/src/lib.rscrates/node-core/src/manager.rscrates/node-core/src/ops.rscrates/node-core/src/prelude.jscrates/node-core/src/protocol.rscrates/node-core/src/runtime.rscrates/node-core/src/wire/mod.rscrates/node-core/src/wire/register.rscrates/node-core/src/wire/run.rscrates/node-core/src/wire/teardown.rscrates/node-core/tests/golden/iii-surface.txtcrates/python-core/Cargo.tomlcrates/python-core/build.rscrates/python-core/src/artifact.rscrates/python-core/src/config.rscrates/python-core/src/error.rscrates/python-core/src/lib.rscrates/python-core/src/manager.rscrates/python-core/src/runner.rscrates/python-core/src/wrapper.pycrates/python-core/tests/artifact.rscrates/python-core/tests/manager.rscrates/python-core/tests/persistence.rscrates/python-core/tests/runner.rscrates/python-core/tests/wrapper.rsiii-permissions.yamlpnpm-workspace.yaml
Registration + reload correctness: - py namespaces track in-flight registrations: a failing registration no longer destroys the interpreter a concurrent sibling is still defining on (deterministic regression test, mutation-verified), and a register racing a teardown errors cleanly instead of panicking on the map entry - py_handler loads default_timeout_ms per invocation so registered handlers honour hot config swaps, per the Manager struct contract - one post-registration refresh closes the boot window where configuration:updated could fire before the trigger existed; refreshes serialize fetch->store so an older get cannot overwrite a newer snapshot - manifest default_config is serialized from the struct (the hand list had drifted by four keys); its test now compares keysets to the schema Core crates: - python clamp_timeout/clamp_memory floor a zero ceiling instead of panicking (u64::clamp with min > max); Semaphore::new(0) hang guarded - wrapper.py: allow_nan=False so NaN/inf results become null instead of poisoning the envelope (bridge frames too); the traceback cap moved into write_error; malformed iii.json degrades to no-bridge - cache_root falls back to the temp dir when HOME/XDG_CACHE_HOME are unset instead of panicking on the boot path - capture_streams counts the appended newline against MAX_LOG_BYTES - run_with inlined into run (sole caller, work_dir always None) - wire docs: node-engine::<runtime_id>:: / node-engine::teardown -> code-runner names (goldens regenerated) Docs/catalog contract: - run.code and register.function_id descriptions state the real model: async fn body vs module, iii.files vs /work, and one runtime per (namespace, lang) — the language gate they described was rolled back - scratch_* documented as node-only (python /work is a fixed engine budget) in the schema docs, README, and config form Console UI: - CSS.escape on the focusField deep-link before querySelector, so a hostile fragment cannot unmount the form - errorInfo keeps message total when error is undefined - configForms remover joins the teardown list; copyText removes its textarea in finally; pulse honours prefers-reduced-motion - build.rs watches packages/console-ui so edits there rebuild page.js Tests/tooling: - run-tests.sh: --filter without a value exits 2 (was an infinite loop); a missing HARNESS_DONE reaches its diagnostic under set -e - stream-cap e2e asserts the real 16 KiB max_stream_bytes default (was <=2 MiB); the register e2e 'any' assertion is de-tautologized; the truncate UTF-8 test asserts the observable contract; the react import assertion matches the full specifier Verified: fmt/clippy clean; node-core 227, python-core 76, code-runner 90 tests; UI tsc + vitest 11/11; e2e suite 25/25.
1 participant
Fixes MOT-4399. Related: MOT-3971 (sandbox-code-runner — separate sandboxed-VM worker; this is the in-process engine).
What
A new
code-runnerworker that runs untrusted Node.js and Python in-process, replacing the retired node-engine and python-engine workers:code-runner::runexecutes guest code in V8 isolates (Node) or an embedded interpreter with cross-call persistence (Python). Both languages get a realiiiclient.register_functionlets guest code register live bus functions, optionally carrying request/response JSON Schemas to the catalog (functions::info). Schemas are catalog metadata, not call-time validation — docs say so explicitly. Namespaces are permissive; only the exact seeded worker ids are protected.max_result_bytes32 KiB /max_stream_bytes16 KiB,0= off): oversized results are replaced by a marker string naming the size and shape with aniii.filesrecovery hint; stdout/stderr keep a 60/40 head+tail around a truncation marker. Motivated by a live session where a 100k-element array echoed a 589 KB result (~40k tokens) into context. The cap seam isManager::run's single Ok-return, covering all translate builders and both engines; transport errors carry no payload.configurationworker (Tier 1 per docs/sops/configuration.md): no committedconfig.yaml;--configis a one-time seed. The output caps and timeouts are read per call from an ArcSwap snapshot theconfiguration:updatedtrigger swaps, so they hot-apply on save; the engine-structural fields (runtime count, V8 memory, scratch) are boot-captured and apply at the next restart — the reload handler logs that split.code-runner::on-config-changeis seeded into the runtime-id registry and denied iniii-permissions.yaml.host.configForms.register('code-runner', …): sections labeled hot-apply vs restart-applied, byte fields with KiB/token hints, worst-case scratch footprint computed inline.Notes
node-coreno longer name the retired node-engine worker (script origins, shutdown hints, error codes, default namespace mints).manager.rs(cap seam mutation-tested, plus a hot-swap test proving a config swap changes the very next response);tests/e2eexists but is CI-unwired..github/release-workers.yaml; CI worker discovery verified locally.Summary by CodeRabbit
New Features
Documentation