feat: Pyodide Python runtime via OS-isolated Deno subprocess#124
Open
NeilMazumdar wants to merge 16 commits into
Open
feat: Pyodide Python runtime via OS-isolated Deno subprocess#124NeilMazumdar wants to merge 16 commits into
NeilMazumdar wants to merge 16 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughReplace sandbox boolean ChangesPyodide runtime rollout
Estimated code review effort Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
NeilMazumdar
force-pushed
the
feature/pyodide-python-runtime
branch
from
22cf3fd to
69dd10c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
clients/python/tests/test_client.py (1)
1-667: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftTest file exceeds 300-line limit — split by concern.
As per coding guidelines, test files should be kept under 300 lines and split by concern (e.g.,
test_client.auth.py,test_client.sandboxes.py,test_client.files.py,test_client.exec.py). This file is 667 lines (223% over the limit).🤖 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 `@clients/python/tests/test_client.py` around lines 1 - 667, The test module is too large (667 lines) and must be split into smaller files by concern; move related test functions into new modules such as test_client.auth.py (e.g. test_bootstrap_exchanges_auth_secret_for_token, test_bootstrap_admin_secret_uses_admin_endpoint, test_constructor_requires_credentials, test_constructor_requires_sub_with_secret), test_client.sandboxes.py (all sandbox CRUD tests: test_list_sandboxes, test_create_sandbox_returns_handle, test_create_sandbox_with_network_passes_flag, test_create_sandbox_network_default_omitted, test_get_sandbox_404_raises_notfound, test_delete_sandbox), test_client.files.py (file-related tests: test_read_file_with_stat_header, test_write_file_sends_octet_stream, test_write_files_bulk, test_delete_recursive_passes_query_param, test_delete_nonempty_dir_raises_conflict, test_mkdir_with_recursive, test_tree_returns_typed_entries, test_ingest_files_base64_encodes), test_client.exec.py (exec-related and retry-policy tests: test_exec_returns_flat_result, test_exec_timeout_raises_typed_exception, test_exec_batch, test_exec_batch_read_only_forwards_flag, test_exec_stream_yields_events_until_exit, test_exec_read_only_sends_flag, test_exec_read_only_violation_raises_validation_error, test_exec_batch_read_only_sends_flag, test_exec_default_does_not_retry_5xx, test_exec_ecoherence_never_retries_even_with_retry_on_5xx, test_exec_read_only_retries_5xx_automatically, test_exec_read_only_retries_ecoherence, test_exec_retry_on_5xx_opt_in_retries_runtime_busy, test_exec_retry_on_5xx_default_false_does_not_send_flag, test_exec_batch_retry_on_5xx_replays_whole_batch, test_exec_batch_default_does_not_retry_5xx) and test_client.errors.py (error mapping tests: test_400_maps_to_validation_error, test_403_maps_to_auth_error, test_429_maps_to_rate_limit_with_retry_after); for each new file, preserve imports (pytest, respx, httpx, make_client, BASE_URL, and exception classes), ensure module-level fixtures/constants remain available or are duplicated, and run the test suite to verify all imports and respx decorators still work.Source: Coding guidelines
src/api/mcp/tools.ts (1)
37-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpose
networkconsistently in the MCP sandbox contract.This tool still hard-codes
network: falseand dropsnetworkfrom the returned JSON, while the sandbox metadata surface treats it as part of the runtime state. That means MCP callers cannot create network-enabled JS sandboxes here, andsandbox_listalso loses information for sandboxes created through the HTTP API.Based on the runtime-contract propagation called out in the PR objectives, MCP should stay aligned with the public sandbox metadata surface.
Also applies to: 62-67, 97-105
🤖 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 `@src/api/mcp/tools.ts` around lines 37 - 56, The MCP sandbox creation currently hard-codes network: false and omits network from persisted/returned metadata; update the handler to read args.network (optional boolean) and propagate it into runtimeOptions.network instead of the hard-coded false, include that network value when calling sessionManager.getOrCreate and sessionManager.persistSandboxMeta (add network to the persisted object alongside python_runtime and javascript), and ensure any response object returned by this endpoint includes the network field; apply the same change pattern to the other similar MCP handlers referenced (the blocks around the other getOrCreate/persistSandboxMeta calls).
🤖 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 `@clients/typescript/CHANGELOG.md`:
- Around line 5-18: Revert the manual edits in CHANGELOG.md and instead create a
proper changeset entry describing the release changes (e.g., mention the python
→ python_runtime rename, SandboxRecord.network addition, and exported
PythonRuntime type) using the Changesets workflow; run pnpm changeset to add the
.md file under .changeset, commit that changeset, then run pnpm
changeset:version (or your repo's release step) to update CHANGELOG.md
automatically—do not keep manual edits in CHANGELOG.md.
In `@clients/typescript/src/models.ts`:
- Line 91: The assignment "python_runtime: (payload.python_runtime ?? null) as
PythonRuntime" unsafely asserts the API value; replace it with a validation step
that checks payload.python_runtime against the PythonRuntime enum/allowed values
(or a shared isPythonRuntime(value) type-guard/validator) before casting; if
invalid, set null or handle error consistently. Locate the mapping where
"python_runtime" is assigned in models.ts (the payload-to-model mapper) and add
the guard/validator, reusing or adding a small helper function (e.g.,
isPythonRuntime) if this pattern appears elsewhere.
In `@plugins/sql-fs/skills/api/ref/bash.md`:
- Around line 48-53: The current paragraph conflates performance/behavior for
both python_runtime modes; split the guidance into two distinct bullet blocks
labeled for python_runtime: "stdlib" and "pyodide" (reference the existing
`"stdlib"` and `"pyodide"` labels) and clearly state for `"stdlib"` that it is a
fresh CPython WASM interpreter per invocation with cold-start cost, limited RAM,
no persistence across calls and strict concurrency limits, while for `"pyodide"`
state that it runs in a long-lived Deno subprocess with preloaded
numpy/pandas/scipy/openpyxl, lower per-call cold-starts, larger
memory/concurrency capacity, and file I/O behavior (files under cwd are drained
back to sandbox FS); update or remove any shared bullets that imply the WASM
cold-start/RAM/concurrency profile applies to both modes.
In `@scripts/build-pyodide-lock.mjs`:
- Around line 53-57: The parseNameVersion function currently assumes
fileName.split("-") yields at least two segments; add defensive validation in
parseNameVersion to verify the split result contains a non-empty name and
version (and optionally validate .whl suffix or match against the same findWheel
regex) and if invalid throw or return a clear error so downstream lockfile
generation cannot be corrupted; update callers (e.g., where findWheel is used)
to handle the thrown error or skip invalid files.
In `@src/api/pyodide/ipc.ts`:
- Around line 136-148: assertFsEntry currently only checks that FsEntry.data is
a string, allowing non-base64 payloads to pass; update assertFsEntry to validate
that when entry.kind === "file" the data is valid base64 (and still require
empty string for dirs). Implement this by verifying entry.data matches a base64
pattern or attempting a safe decode and rejecting on failure, and throw
IpcIntegrityError(`${label}: invalid/missing base64 data`) from assertFsEntry to
ensure malformed payloads are rejected before drain.
In `@src/api/pyodide/manager.ts`:
- Around line 339-365: The write to child.stdin in `#sendRunFrame` (and the other
write site around lines 419-445) can emit async stream errors that bypass the
synchronous try/catch; before calling child.stdin.write(encodeFrame(frame))
attach temporary 'error' and 'close' handlers on child.stdin that call
this.#failOwned(err instanceof Error ? err : new Error(String(err)), true) (or
pass a PyodideChildExitError when appropriate), and remove those handlers
immediately after the write completes or the callback fires to avoid leaks;
ensure you still keep the existing null/undefined check for child.stdin and
perform the same handler setup/cleanup at both write sites so any
broken-pipe/destroyed stream is routed through `#failOwned`().
In `@src/api/pyodide/tests/integration/frame-forgery.integration.test.ts`:
- Around line 91-96: Add an afterEach that ensures the session created in
beforeEach is deleted: afterEach(async () => { if (session) await
sm.delete(session); session = undefined; }); This should reference the existing
session variable created by sm.getOrCreate(TENANT, ...) and use the
session-manager API (sm.delete) to remove the sandbox immediately after each
test so tests don't accumulate sessions; include a null-check and await the
deletion to avoid race conditions.
In `@src/api/tests/unit/session-manager.test.ts`:
- Around line 737-907: The Pyodide-specific tests grouped under the describe
titles "SessionManager Pyodide semaphore (MAX_CONCURRENT_PYODIDE) — Phase 6" and
"SessionManager Pyodide residency startup invariant — Phase 6" should be
extracted into the existing session-manager.pyodide.test.ts to keep concerns
separated; copy the entire describe blocks (including helper functions
stubBashExec, setEnv, PYODIDE const and any uses of T, makeCreateFs,
SessionManager, and the afterEach/active teardown usage) into that file, update
imports/fixtures there so T and makeCreateFs are available, and delete the
copied blocks from session-manager.test.ts so the original file drops below the
300-line guideline while preserving test behavior and names.
In `@thoughts/issue-118-pyodide-runtime/design.md`:
- Around line 173-178: This design currently requires shipping Pyodide (WASM)
which conflicts with the repo's "no WASM dependencies" constraint; update the
design.md to either (a) add an explicit decision record/exception that
authorizes including Pyodide (state who approved, scope, security/size
tradeoffs, and mitigation), or (b) provide an alternative non-WASM path (e.g.,
native CPython runtime or remote execution) and describe fallback behavior for
loadPyodide({ indexURL, lockFileURL, packageBaseUrl }) and the packaging steps
that use micropip.freeze; reference the Pyodide version pin (0.29.x), the
Docker/packaging step, and the preload list (numpy,pandas,scipy,openpyxl) so
reviewers can see where the policy applies.
In `@thoughts/issue-118-pyodide-runtime/spikes/s1-pyodide-deno.sh`:
- Line 71: Replace the ls|wc invocation with a robust find-based count: instead
of echoing the output of ls "${PYODIDE_DIR}" | wc -l, run a find rooted at
"${PYODIDE_DIR}" that counts regular files (and optionally directories) to avoid
miscounting with strange filenames; update the echo line that references
PYODIDE_DIR and the diagnostic text "[s1] pyodide assets: … files in
${PYODIDE_DIR}" so it prints the result of the find-based count rather than the
ls-based count.
In `@thoughts/issue-118-pyodide-runtime/spikes/S3-findings.md`:
- Around line 10-14: The fenced code block containing the cgroup/prlimit
findings should include a language specifier to satisfy the linter and improve
rendering; edit the block that currently starts with triple backticks and the
lines "cgroup_write_denied=1 rlimit_as_unusable=1 ..." and change the opening
fence to include a language token such as text or console (e.g., ```text) so the
block is properly annotated.
In `@tsconfig.json`:
- Line 22: tsconfig.json currently excludes "src/pyodide-runner/runner.ts",
bypassing type-checks for a security-critical entrypoint; either remove that
path from the "exclude" array so runner.ts is included in the TypeScript project
(ensuring the compiler options like strict and noUncheckedIndexedAccess apply),
or if exclusion is required, add a dedicated CI gate that runs tsc (or a
targeted tsc --project) against src/pyodide-runner/runner.ts and document this
new CI check in the repo README; reference the "exclude" entry and the file
src/pyodide-runner/runner.ts when making the change.
---
Outside diff comments:
In `@clients/python/tests/test_client.py`:
- Around line 1-667: The test module is too large (667 lines) and must be split
into smaller files by concern; move related test functions into new modules such
as test_client.auth.py (e.g. test_bootstrap_exchanges_auth_secret_for_token,
test_bootstrap_admin_secret_uses_admin_endpoint,
test_constructor_requires_credentials,
test_constructor_requires_sub_with_secret), test_client.sandboxes.py (all
sandbox CRUD tests: test_list_sandboxes, test_create_sandbox_returns_handle,
test_create_sandbox_with_network_passes_flag,
test_create_sandbox_network_default_omitted,
test_get_sandbox_404_raises_notfound, test_delete_sandbox), test_client.files.py
(file-related tests: test_read_file_with_stat_header,
test_write_file_sends_octet_stream, test_write_files_bulk,
test_delete_recursive_passes_query_param,
test_delete_nonempty_dir_raises_conflict, test_mkdir_with_recursive,
test_tree_returns_typed_entries, test_ingest_files_base64_encodes),
test_client.exec.py (exec-related and retry-policy tests:
test_exec_returns_flat_result, test_exec_timeout_raises_typed_exception,
test_exec_batch, test_exec_batch_read_only_forwards_flag,
test_exec_stream_yields_events_until_exit, test_exec_read_only_sends_flag,
test_exec_read_only_violation_raises_validation_error,
test_exec_batch_read_only_sends_flag, test_exec_default_does_not_retry_5xx,
test_exec_ecoherence_never_retries_even_with_retry_on_5xx,
test_exec_read_only_retries_5xx_automatically,
test_exec_read_only_retries_ecoherence,
test_exec_retry_on_5xx_opt_in_retries_runtime_busy,
test_exec_retry_on_5xx_default_false_does_not_send_flag,
test_exec_batch_retry_on_5xx_replays_whole_batch,
test_exec_batch_default_does_not_retry_5xx) and test_client.errors.py (error
mapping tests: test_400_maps_to_validation_error, test_403_maps_to_auth_error,
test_429_maps_to_rate_limit_with_retry_after); for each new file, preserve
imports (pytest, respx, httpx, make_client, BASE_URL, and exception classes),
ensure module-level fixtures/constants remain available or are duplicated, and
run the test suite to verify all imports and respx decorators still work.
In `@src/api/mcp/tools.ts`:
- Around line 37-56: The MCP sandbox creation currently hard-codes network:
false and omits network from persisted/returned metadata; update the handler to
read args.network (optional boolean) and propagate it into
runtimeOptions.network instead of the hard-coded false, include that network
value when calling sessionManager.getOrCreate and
sessionManager.persistSandboxMeta (add network to the persisted object alongside
python_runtime and javascript), and ensure any response object returned by this
endpoint includes the network field; apply the same change pattern to the other
similar MCP handlers referenced (the blocks around the other
getOrCreate/persistSandboxMeta calls).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bf216db2-3162-451e-8a20-ddaa584b8f30
⛔ Files ignored due to path filters (1)
clients/python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (83)
.changeset/python-runtime-enum.md.dockerignore.gitignoreCLAUDE.mdDockerfilebiome.jsonclients/python/CHANGELOG.mdclients/python/README.mdclients/python/examples/quickstart.pyclients/python/pyproject.tomlclients/python/src/sqlfs/__init__.pyclients/python/src/sqlfs/_version.pyclients/python/src/sqlfs/client.pyclients/python/src/sqlfs/models.pyclients/python/tests/test_client.pyclients/typescript/CHANGELOG.mdclients/typescript/README.mdclients/typescript/examples/e2e-local.tsclients/typescript/examples/quickstart.tsclients/typescript/package.jsonclients/typescript/src/client.tsclients/typescript/src/index.tsclients/typescript/src/models.tsclients/typescript/src/version.tsclients/typescript/tests/sandboxes.test.tspackage.jsonplugins/sql-fs/skills/api/SETUP.mdplugins/sql-fs/skills/api/ref/bash.mdplugins/sql-fs/skills/api/ref/endpoints.mdplugins/sql-fs/skills/py-sdk/SKILL.mdplugins/sql-fs/skills/py-sdk/ref/client.mdplugins/sql-fs/skills/py-sdk/ref/models.mdscripts/build-pyodide-lock.mjsscripts/copy-pyodide-runner.mjsscripts/fetch-pyodide-assets.mjssrc/api/commands/pyodide-command.tssrc/api/mcp/tools.tssrc/api/openapi-spec.tssrc/api/pyodide/ipc.tssrc/api/pyodide/manager.tssrc/api/pyodide/residency.tssrc/api/pyodide/tests/integration/escape.integration.test.tssrc/api/pyodide/tests/integration/frame-forgery.integration.test.tssrc/api/pyodide/tests/unit/fake-child.tssrc/api/pyodide/tests/unit/ipc.test.tssrc/api/pyodide/tests/unit/manager.test.tssrc/api/pyodide/tests/unit/pyodide-command.test.tssrc/api/pyodide/tests/unit/residency.test.tssrc/api/routes/sandboxes.tssrc/api/session-manager.tssrc/api/tests/integration/migrations.integration.test.tssrc/api/tests/integration/pyodide.integration.test.tssrc/api/tests/session-manager.script-tx.test.tssrc/api/tests/unit/exec-batch.test.tssrc/api/tests/unit/exec.test.tssrc/api/tests/unit/files.test.tssrc/api/tests/unit/ingest.test.tssrc/api/tests/unit/mcp-tools.test.tssrc/api/tests/unit/sandboxes.test.tssrc/api/tests/unit/session-manager.pyodide.test.tssrc/api/tests/unit/session-manager.rehydrate.test.tssrc/api/tests/unit/session-manager.test.tssrc/pyodide-runner/protocol.tssrc/pyodide-runner/runner.tssrc/sql-fs/dialects/postgres.tssrc/sql-fs/dialects/tests/unit/postgres.advisory-lock.test.tssrc/sql-fs/migrations/postgres/0006_python_runtime.sqlsrc/sql-fs/types.tsthoughts/issue-118-pyodide-runtime/design.mdthoughts/issue-118-pyodide-runtime/plan.mdthoughts/issue-118-pyodide-runtime/questions.mdthoughts/issue-118-pyodide-runtime/research.mdthoughts/issue-118-pyodide-runtime/spikes/.gitignorethoughts/issue-118-pyodide-runtime/spikes/S1-findings.mdthoughts/issue-118-pyodide-runtime/spikes/S2-findings.mdthoughts/issue-118-pyodide-runtime/spikes/S3-findings.mdthoughts/issue-118-pyodide-runtime/spikes/s1-pyodide-deno.shthoughts/issue-118-pyodide-runtime/spikes/s1_runner.tsthoughts/issue-118-pyodide-runtime/spikes/s2-ipc.tsthoughts/issue-118-pyodide-runtime/spikes/s3-memory.shthoughts/issue-118-pyodide-runtime/structure.mdthoughts/issue-118-pyodide-runtime/task.mdtsconfig.json
Owner
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69dd10c00b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
plugins/sql-fs/skills/api/ref/bash.md (1)
60-73:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSplit the performance and concurrency guidance by runtime mode.
The performance section still conflates stdlib and pyodide behavior. Lines 60-61 state "Every call pays the WASM cold-boot cost (~1.4 s)" which only applies to
python_runtime: "stdlib", not to the Pyodide subprocess model. Similarly, the RAM cost (line 73) and concurrency limits (line 72) should clarify whether they apply to one or both modes.According to the PR objectives, Pyodide has "~5–8s cold start" (subprocess spawn + V8 + Pyodide init), "~350–400 MB per execution", and separate
MAX_CONCURRENT_PYODIDE(default 2). These are materially different from the stdlib figures shown here.Split this section into mode-specific guidance or add inline qualifiers (e.g., "stdlib: ~1.4s, pyodide: ~5–8s on cold start").
🤖 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 `@plugins/sql-fs/skills/api/ref/bash.md` around lines 60 - 73, Update the performance/concurrency guidance so it distinguishes runtime modes: change the claim "Every call pays the WASM cold-boot cost (~1.4 s)" and the RAM/concurrency figures into explicit, per-mode statements (e.g., for python_runtime == "stdlib": cold start ~1.4s, ~80 MB per python3 process, server-wide concurrency cap 5; for pyodide subprocess mode: cold start ~5–8s, ~350–400 MB per execution, and respect MAX_CONCURRENT_PYODIDE default 2). Edit the paragraph(s) in plugins/sql-fs/skills/api/ref/bash.md that reference the cold-boot, RAM (~80 MB) and concurrency (5) so they either become two short mode-specific bullet points or include inline qualifiers referencing python_runtime and MAX_CONCURRENT_PYODIDE, and ensure the guidance about persisting state and avoiding pip/os.system/subprocess remains unchanged.src/api/openapi-spec.ts (1)
745-756:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocument
perScriptTimeoutMsin the batch exec schema.
src/api/routes/exec.tsacceptsperScriptTimeoutMs, but the OpenAPI request body omits it. That breaks generated clients and hides the new timeout mode from API consumers.🤖 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 `@src/api/openapi-spec.ts` around lines 745 - 756, The OpenAPI schema for the batch exec request is missing the perScriptTimeoutMs property even though routes/exec.ts accepts it; update the schema in src/api/openapi-spec.ts (the same object containing timeoutMs, readOnly, retryOn5xx) to add a perScriptTimeoutMs field (type: integer, provide example and sensible minimum/maximum and a short description explaining its per-script timeout semantics) so generated clients and docs include the new timeout mode; ensure the new property name matches the parameter used in routes/exec.ts exactly and is documented alongside timeoutMs.src/api/mcp/tools.ts (1)
250-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap internal Pyodide timeouts to MCP timeouts here as well.
This catch still only reports timeout when the local timer fired.
execWithRuntimeThrottle()can now throwcode === "EPYODIDE_TIMEOUT"first, which meansbash_execreturns a generic internal error while the HTTP and batch paths returntimeout.🤖 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 `@src/api/mcp/tools.ts` around lines 250 - 263, The catch block inside bash_exec should treat internal Pyodide timeouts the same as the local timer timeout: after clearTimeout(timer) and extra.signal.removeEventListener("abort", onDisconnect), detect if the caught error has e?.code === "EPYODIDE_TIMEOUT" (or equivalent) and return the same timeout result object (stdout:"", stderr:"timeout", exitCode:-1, exitSignal:null, timedOut:true, durationMs: Date.now() - startMs) instead of throwing; only rethrow for other errors. Ensure you reference the existing symbols timer, extra.signal, onDisconnect, startMs, and the function execWithRuntimeThrottle in your change.src/api/routes/exec.ts (1)
291-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winA resolved streamed exec should never be rewritten to
timeout.Unlike
/exec-sync, the SSE path still checkstimedOutafter a successfulexecWithRuntimeThrottle()return. If the timer fires in the same tick after completion, the client sees a timeout even though the script already finished and may have committed writes.🤖 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 `@src/api/routes/exec.ts` around lines 291 - 299, The code currently may set timedOut in the timer callback after execWithRuntimeThrottle() already completed and sent an exit event; add a separate "completed" (or "finished") boolean flag that you set to true immediately before sending any exit SSE in the successful exec path (where stream.writeSSE is called after execWithRuntimeThrottle()), and update the timer callback to check completed before treating the run as a timeout (i.e., only set timedOut/send timeout SSE if completed is false). Ensure all code paths that send an exit SSE (the post-exec success path and any error/early exit paths) set completed = true before returning so the timer cannot overwrite a resolved run.src/api/routes/sandboxes.ts (2)
46-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat malformed JSON as an empty create request.
A body like
{still falls into this broadcatchand the handler proceeds with all defaults, soPOST /v1/sandboxesreturns201and creates a sandbox instead of400. That bypasses the new strictpython_runtimevalidation path and turns client parse errors into side effects. Only an actually empty body should be ignored; parse failures should returnINVALID_INPUT.🤖 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 `@src/api/routes/sandboxes.ts` around lines 46 - 58, The request handler currently swallows JSON parse errors in the try/catch around c.req.json() (used with createBodySchema), allowing malformed JSON like "{" to be treated as an empty create and create a sandbox; change the flow so you first read the raw request body as text, if raw.trim() === "" treat it as an empty body (keep existing defaults), otherwise attempt JSON.parse or c.req.json() and if parsing fails return c.json({ error: "validation_error", code: "INVALID_INPUT", details: [...] }, 400) instead of proceeding; update the block around c.req.json(), createBodySchema.safeParse, and the catch to differentiate empty body vs parse failure and ensure parse failures always return INVALID_INPUT.
187-195:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize legacy metadata on the cold-replica GET path.
This fallback now serializes
meta.python_runtime,meta.javascript, andmeta.networkas-is. IfgetSandboxMeta()returns a pre-migration/legacy entry, those values areundefined, and the JSON response drops the fields entirely. That breaks the new API contract exactly on the rolling-deploy path this PR is trying to keep compatible. Default the booleans tofalseand derivepython_runtimefrom legacy metadata when present.Based on the rolling-deploy compatibility objective for
python_runtime, this fallback still needs to handle pre-migration metadata.🤖 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 `@src/api/routes/sandboxes.ts` around lines 187 - 195, The cold-replica GET response currently returns meta.python_runtime, meta.javascript, and meta.network as-is, which causes those fields to be omitted when getSandboxMeta() returns legacy metadata; update the fallback serialization in the endpoint that constructs the JSON response (the block returning c.json with id/name/owner/createdAt/lastUsedAt) to default javascript and network to false when undefined and to derive python_runtime from legacy metadata (e.g., coerce legacy values into the new python_runtime boolean) so that the response always includes python_runtime, javascript, and network (booleans) even for pre-migration entries.src/api/tests/unit/sandboxes.test.ts (1)
261-293: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd direct route coverage for the new
python_runtimecontract.This file only updates a fixture to the renamed metadata shape. It still does not assert that
POST /v1/sandboxesaccepts"pyodide"/null, rejects legacypython, and thatGET /v1/sandboxes/:idreturnspython_runtimeon both warm and cold paths. Those are the load-bearing API changes in this PR.As per coding guidelines, unit tests should cover both success and error cases with exact assertions; the PR objective here is the
python_runtimeAPI migration.🤖 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 `@src/api/tests/unit/sandboxes.test.ts` around lines 261 - 293, Add unit tests that directly assert the new python_runtime contract: in src/api/tests/unit/sandboxes.test.ts add cases using makeTestApp, SessionManager, makeToken and InMemoryFs to (1) POST /v1/sandboxes succeeds when body.python_runtime is "pyodide" and when it is null, (2) POST /v1/sandboxes returns a 4xx when body.python_runtime is the legacy "python" value, and (3) GET /v1/sandboxes/:id returns a python_runtime field on both warm paths (sandbox present in the SessionManager pool) and cold paths (only getSandboxMetaFn provides metadata) asserting the exact returned value matches the stored metadata; reuse the existing helpers (makeTestApp, SessionManager constructor/getSandboxMetaFn, makeToken) and mirror the style of the existing test that checks createdAt for setup and assertions.Source: Coding guidelines
♻️ Duplicate comments (5)
tsconfig.json (1)
22-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExclusion of runner.ts still lacks documented CI type-checking gate.
The exclusion of
src/pyodide-runner/runner.tsfrom TypeScript compilation remains without a documented alternative validation path. While the exclusion may be justified (Deno vs Node module systems), the past review's requirement for "a dedicated CI gate that fails on runner type errors" appears unmet based on the provided context.Either:
- Remove the exclusion and add a Deno-compatible tsconfig, or
- Document the CI gate that validates runner.ts types (e.g.,
deno checkin the build pipeline)The runner is security-critical (IPC integrity, realm lockdown) and should not bypass static validation.
🤖 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 `@tsconfig.json` at line 22, The tsconfig.json currently excludes "src/pyodide-runner/runner.ts" via the "exclude" entry which bypasses TypeScript checks for the security-critical runner; either remove that exclusion and add a Deno-compatible tsconfig (or adjust existing tsconfig settings and module resolution) so runner.ts is compiled by your CI, or keep the exclusion but add and document a CI gate that type-checks runner.ts (e.g., run `deno check` or an equivalent Deno type-check step in the pipeline) and link that CI job in the repo docs; update tsconfig.json and repository CI/docs accordingly and reference the exact excluded path "src/pyodide-runner/runner.ts" when making the change.clients/typescript/CHANGELOG.md (1)
5-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCHANGELOG should be managed via Changesets, not manually edited.
Per coding guidelines and learnings,
CHANGELOG.mdis generated through the Changesets workflow. In a feature PR, only the.changeset/*.mdfile should be committed. The CHANGELOG.md update should happen in a separate release PR viapnpm changeset:version. Manual edits here bypass the project's release automation.🤖 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 `@clients/typescript/CHANGELOG.md` around lines 5 - 18, Revert the manual edits to CHANGELOG.md and instead add a Changeset file under .changeset describing the 0.4.0 changes (mentioning the python -> python_runtime migration, SandboxRecord.python_runtime, SandboxRecord.network, and exporting PythonRuntime) with the appropriate release type; commit only the .changeset/*.md in this feature PR and remove CHANGELOG.md from the commit, then for the release flow run the project's release step (pnpm changeset:version) in the release PR to regenerate CHANGELOG.md via the Changesets workflow.Source: Learnings
clients/typescript/src/models.ts (1)
91-91:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate API response before type assertion.
Line 91 casts
payload.python_runtimetoPythonRuntimewithout validation. If the API returns an unexpected value (e.g.,"invalid",123), the type system accepts it but runtime behavior may be incorrect.🛡️ Add validation guard
export function sandboxRecordFromApi(payload: ApiObject): SandboxRecord { + const rawRuntime = payload.python_runtime ?? null; + const validRuntimes = new Set<PythonRuntime>(["stdlib", "pyodide", null]); + if (!validRuntimes.has(rawRuntime as PythonRuntime)) { + throw new Error(`Invalid python_runtime from API: ${String(rawRuntime)}`); + } return { id: String(payload.id), name: payload.name == null ? null : String(payload.name), owner: String(payload.owner), createdAt: String(payload.createdAt), - python_runtime: (payload.python_runtime ?? null) as PythonRuntime, + python_runtime: rawRuntime as PythonRuntime, javascript: Boolean(payload.javascript), network: Boolean(payload.network), }; }🤖 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 `@clients/typescript/src/models.ts` at line 91, The assignment that casts payload.python_runtime to PythonRuntime must validate the value first: add a guard (e.g., isValidPythonRuntime or use an enum validator in the same module) and check payload.python_runtime is a string and one of the allowed PythonRuntime values before casting; if validation fails, set python_runtime to null or a safe default (or throw) instead of unconditionally casting. Update the code paths that reference python_runtime (the payload variable and the PythonRuntime type usage in the same model factory/constructor) to use this validated value.src/api/pyodide/ipc.ts (1)
136-148:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing base64 validation in
assertFsEntryallows forged file payloads.The past review concern is valid:
assertFsEntryonly checkstypeof entry.data === "string"but does not validate thatentry.datais valid base64 whenentry.kind === "file". A forgedresultframe with non-base64datawould pass integrity validation and reach the drain layer, where decoding garbage could corrupt persisted files.Add base64 validation for file entries to reject malformed payloads before they reach the drain.
🛡️ Defensive validation
function assertFsEntry(entry: unknown, label: string): void { if (!isObject(entry)) throw new IpcIntegrityError(`${label}: entry is not an object`); if (typeof entry.path !== "string" || entry.path.length === 0) { throw new IpcIntegrityError(`${label}: missing/invalid path`); } if (entry.kind !== "file" && entry.kind !== "dir") throw new IpcIntegrityError(`${label}: invalid kind`); if (typeof entry.mode !== "number" || !Number.isInteger(entry.mode) || entry.mode < 0) { throw new IpcIntegrityError(`${label}: missing/invalid mode`); } if (typeof entry.data !== "string") throw new IpcIntegrityError(`${label}: missing/invalid data`); + if (entry.kind === "file" && entry.data.length > 0) { + // Validate base64: should only contain [A-Za-z0-9+/] and optional trailing "=" padding + if (!/^[A-Za-z0-9+/]*={0,2}$/.test(entry.data)) { + throw new IpcIntegrityError(`${label}: file data is not valid base64`); + } + } if (entry.kind === "dir" && entry.data !== "") throw new IpcIntegrityError(`${label}: dir entry must have empty data`); }🤖 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 `@src/api/pyodide/ipc.ts` around lines 136 - 148, The assertFsEntry function currently only checks that entry.data is a string; update assertFsEntry to also validate that when entry.kind === "file" the entry.data is valid base64 and reject otherwise by throwing IpcIntegrityError. Locate assertFsEntry and add a check for file entries that attempts a safe base64 validation (e.g. test against a base64 regex or attempt a decode with Buffer.from(..., "base64")/atob and verify the round-trip or that decoding doesn't throw) and throw new IpcIntegrityError(`${label}: invalid base64 data`) on failure so malformed/non-base64 payloads are rejected before they reach the drain.src/api/pyodide/manager.ts (1)
367-396:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd async error handlers for
child.stdinto prevent unhandled stream failures.The past review concern remains valid:
child.stdin.write(encodeFrame(frame))at line 392 is wrapped in synchronoustry/catch, but there are nochild.stdin'error'or'close'event handlers. If the child process terminates between the null-check at line 387 and the write at line 392, the writable stream may emit async errors (e.g.,EPIPE,ERR_STREAM_DESTROYED) that bypass the try/catch and are not routed through#failOwned().Attach temporary error/close handlers to
child.stdinbefore writing, and remove them after the write completes to ensure broken-pipe scenarios are properly handled.🔒 Proposed fix for stdin error handling
`#sendRunFrame`(op: OwnedOp): void { if (op.done) return; const frame: RunRequest = { type: "run", requestId: this.#randomRequestId(), seq: ++this.#seqCounter, generation: this.#generation, code: op.input.code, argv: op.input.argv, stdin: op.input.stdin, files: op.input.files, cwd: op.input.cwd, env: op.input.env, }; op.requestId = frame.requestId; op.seq = frame.seq; op.stage = "response"; this.#pending = { requestId: frame.requestId, seq: frame.seq }; this.#state = "busy"; const child = this.#child; if (child?.stdin === null || child?.stdin === undefined) { this.#failOwned(new PyodideChildExitError(null, null), true); return; } + const onStdinError = (err: Error): void => { + this.#failOwned(err, true); + }; + const onStdinClose = (): void => { + this.#failOwned(new PyodideChildExitError(null, null), true); + }; + child.stdin.once("error", onStdinError); + child.stdin.once("close", onStdinClose); try { child.stdin.write(encodeFrame(frame)); + child.stdin.removeListener("error", onStdinError); + child.stdin.removeListener("close", onStdinClose); } catch (err) { + child.stdin.removeListener("error", onStdinError); + child.stdin.removeListener("close", onStdinClose); this.#failOwned(err instanceof Error ? err : new Error(String(err)), 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 `@src/api/pyodide/manager.ts` around lines 367 - 396, In sendRunFrame, add temporary async handlers on child.stdin (use child.stdin.once('error', ...) and once('close', ...)) before calling child.stdin.write(encodeFrame(frame)) so async stream errors (EPIPE / ERR_STREAM_DESTROYED) get routed to this.#failOwned(err, true); ensure these handlers are removed after the write completes (use the write callback or drain/finish event) to avoid leaking listeners; keep the existing null-check for child.stdin and still wrap the synchronous write in try/catch, but rely on the temporary listeners to catch asynchronous failures and forward them to `#failOwned`; reference sendRunFrame, child.stdin, encodeFrame, and `#failOwned` when applying the change.
🤖 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 `@Dockerfile`:
- Around line 9-10: The RUN apt-get install invocation installs curl, unzip, and
bzip2 without version pins; change the RUN line that installs these build-time
packages (the RUN ... apt-get install -y --no-install-recommends curl unzip
bzip2 ...) to pin explicit Debian package versions (e.g. curl=<version>
unzip=<version> bzip2=<version>) so builds are reproducible — determine the
exact versions from the base image (using apt-cache policy in node:22-bookworm
or equivalent) and update the package list invocation to install those pinned
versions and still clean /var/lib/apt/lists/* afterward.
In `@plugins/sql-fs/skills/py-sdk/ref/client.md`:
- Line 57: Remove the empty blank line separating the two consecutive blockquote
blocks in plugins/sql-fs/skills/py-sdk/ref/client.md so the blockquotes are
directly adjacent (this fixes MD028); locate the two successive blockquote
sections in that file and delete the intervening blank line between them.
In `@scripts/fetch-pyodide-assets.mjs`:
- Around line 98-120: In fetchDeno(), add integrity verification for the
downloaded archive by computing a SHA-256 checksum of the downloaded zip (or the
extracted DENO_BIN) and comparing it to a trusted expected value before
unpacking or using the binary; introduce or use a checksum mapping keyed by
PINS.deno/denoTarget() (e.g., DENO_SHA_MAP or a manifest fetched alongside the
release) and if the computed checksum does not match the expected value throw an
error and abort; ensure you compute the hash from the same file referenced by
zip (or DENO_BIN after extraction) using a stable Node crypto hash, compare
constant-time, and only call execFileSync("unzip", ...) / chmodSync(DENO_BIN,
...) / log success after the verification passes.
In `@src/api/commands/pyodide-command.ts`:
- Around line 474-489: The guard currently only lstats the final resolved path;
change Pyodide drain logic to walk each ancestor segment of resolved (up to but
not including root) and lstat each parent to detect any symbolic links before
performing mkdir/writeFile/chmod; if any parent lstat shows a symlink, throw the
same PyodideDrainError (use the same message pattern referencing entry.path).
Ensure you perform this ancestor-symlink check prior to the existing entry.kind
"dir" branch and before writing/chmod so no symlink ancestors are followed.
- Around line 363-369: The walk function currently swallows errors from
fs.readdir by returning in the catch, causing incomplete staged snapshots;
change the catch in walk (the async function walk(dir: string)) to surface
filesystem errors instead of returning silently — either rethrow the caught
error or throw a new descriptive Error including the original error and the dir
(do not swallow), so callers (Pyodide exec / staged snapshot logic) will fail
fast on unreadable directories rather than proceed with a partial tree.
In `@src/api/pyodide/residency.ts`:
- Around line 173-180: The idle sweeper method `#sweep`() currently
fire-and-forgets worker.dispose(), which can produce unhandled promise
rejections; update the disposal call inside the loop that removes from
this.#residents to handle rejections (e.g., append a .catch(...) to the promise
or await in a safe async helper) and log the error (or surface it via your
existing logger) so disposal failures are captured; reference the `#sweep` method,
this.#residents iteration, the `#evictable` check, this.#idleMs timeout logic, and
the worker.dispose() call when making this change.
In `@src/api/pyodide/tests/integration/escape.integration.test.ts`:
- Around line 45-61: The test suite sets process.env.PYODIDE_ASSET_DIR and
process.env.DENO_BIN_PATH in beforeAll but only restores AUTH_SECRET in
afterAll; capture the original values (e.g., savedPyodideAssetDir and
savedDenoBin) alongside savedSecret in the beforeAll setup, and in afterAll
restore them (or delete the env prop if undefined) just like AUTH_SECRET is
handled so PYODIDE_ASSET_DIR and DENO_BIN_PATH do not leak to other tests;
update the beforeAll/afterAll blocks surrounding SessionManager/session setup
and shutdown to use these saved variables.
In `@src/api/pyodide/tests/integration/frame-forgery.integration.test.ts`:
- Around line 81-89: Save the original environment values for PYODIDE_ASSET_DIR
and DENO_BIN_PATH at the start of the suite (in the same scope as beforeAll),
then restore them in afterAll; specifically, in beforeAll capture previous
values (e.g. prevPyodideAssetDir, prevDenoBinPath) before mutating process.env,
and in afterAll restore process.env.PYODIDE_ASSET_DIR and
process.env.DENO_BIN_PATH back to those captured values (setting to
undefined/null if they were unset) and do the restoration in a finally block
around sm.shutdown() so restoration always runs even if shutdown fails.
In `@src/api/tests/integration/pyodide.integration.test.ts`:
- Around line 33-52: The test suite currently only cleans sandboxes in afterAll
which can leak state; add an afterEach that iterates the cleanup array and
destroys each sandbox (using destroySandbox("postgres", id)) and then clears the
cleanup array, and update individual tests that create sandboxes to use
try/finally so they push the created id onto the shared cleanup array and ensure
removal in the finally block; reference the existing cleanup array,
destroySandbox, and SessionManager/describe.skipIf block when making these
changes.
In `@src/api/tests/unit/mcp-tools.test.ts`:
- Around line 120-128: The test "sandbox_create echoes the python_runtime it was
given" only asserts python_runtime and can miss regressions in the full tool
payload; update the test that calls captureToolHandler("sandbox_create") /
getHandler() so it parses result.content[0].text and uses
expect(parsedBody).toEqual(expectedPayload) with a complete expected object
(include id, name, javascript, python_runtime and any wrapper shape returned)
instead of just toBe on python_runtime; apply the same change to the related
test block mentioned (lines ~130-151) to assert the full JSON response payload
for exact-match assertions.
---
Outside diff comments:
In `@plugins/sql-fs/skills/api/ref/bash.md`:
- Around line 60-73: Update the performance/concurrency guidance so it
distinguishes runtime modes: change the claim "Every call pays the WASM
cold-boot cost (~1.4 s)" and the RAM/concurrency figures into explicit, per-mode
statements (e.g., for python_runtime == "stdlib": cold start ~1.4s, ~80 MB per
python3 process, server-wide concurrency cap 5; for pyodide subprocess mode:
cold start ~5–8s, ~350–400 MB per execution, and respect MAX_CONCURRENT_PYODIDE
default 2). Edit the paragraph(s) in plugins/sql-fs/skills/api/ref/bash.md that
reference the cold-boot, RAM (~80 MB) and concurrency (5) so they either become
two short mode-specific bullet points or include inline qualifiers referencing
python_runtime and MAX_CONCURRENT_PYODIDE, and ensure the guidance about
persisting state and avoiding pip/os.system/subprocess remains unchanged.
In `@src/api/mcp/tools.ts`:
- Around line 250-263: The catch block inside bash_exec should treat internal
Pyodide timeouts the same as the local timer timeout: after clearTimeout(timer)
and extra.signal.removeEventListener("abort", onDisconnect), detect if the
caught error has e?.code === "EPYODIDE_TIMEOUT" (or equivalent) and return the
same timeout result object (stdout:"", stderr:"timeout", exitCode:-1,
exitSignal:null, timedOut:true, durationMs: Date.now() - startMs) instead of
throwing; only rethrow for other errors. Ensure you reference the existing
symbols timer, extra.signal, onDisconnect, startMs, and the function
execWithRuntimeThrottle in your change.
In `@src/api/openapi-spec.ts`:
- Around line 745-756: The OpenAPI schema for the batch exec request is missing
the perScriptTimeoutMs property even though routes/exec.ts accepts it; update
the schema in src/api/openapi-spec.ts (the same object containing timeoutMs,
readOnly, retryOn5xx) to add a perScriptTimeoutMs field (type: integer, provide
example and sensible minimum/maximum and a short description explaining its
per-script timeout semantics) so generated clients and docs include the new
timeout mode; ensure the new property name matches the parameter used in
routes/exec.ts exactly and is documented alongside timeoutMs.
In `@src/api/routes/exec.ts`:
- Around line 291-299: The code currently may set timedOut in the timer callback
after execWithRuntimeThrottle() already completed and sent an exit event; add a
separate "completed" (or "finished") boolean flag that you set to true
immediately before sending any exit SSE in the successful exec path (where
stream.writeSSE is called after execWithRuntimeThrottle()), and update the timer
callback to check completed before treating the run as a timeout (i.e., only set
timedOut/send timeout SSE if completed is false). Ensure all code paths that
send an exit SSE (the post-exec success path and any error/early exit paths) set
completed = true before returning so the timer cannot overwrite a resolved run.
In `@src/api/routes/sandboxes.ts`:
- Around line 46-58: The request handler currently swallows JSON parse errors in
the try/catch around c.req.json() (used with createBodySchema), allowing
malformed JSON like "{" to be treated as an empty create and create a sandbox;
change the flow so you first read the raw request body as text, if raw.trim()
=== "" treat it as an empty body (keep existing defaults), otherwise attempt
JSON.parse or c.req.json() and if parsing fails return c.json({ error:
"validation_error", code: "INVALID_INPUT", details: [...] }, 400) instead of
proceeding; update the block around c.req.json(), createBodySchema.safeParse,
and the catch to differentiate empty body vs parse failure and ensure parse
failures always return INVALID_INPUT.
- Around line 187-195: The cold-replica GET response currently returns
meta.python_runtime, meta.javascript, and meta.network as-is, which causes those
fields to be omitted when getSandboxMeta() returns legacy metadata; update the
fallback serialization in the endpoint that constructs the JSON response (the
block returning c.json with id/name/owner/createdAt/lastUsedAt) to default
javascript and network to false when undefined and to derive python_runtime from
legacy metadata (e.g., coerce legacy values into the new python_runtime boolean)
so that the response always includes python_runtime, javascript, and network
(booleans) even for pre-migration entries.
In `@src/api/tests/unit/sandboxes.test.ts`:
- Around line 261-293: Add unit tests that directly assert the new
python_runtime contract: in src/api/tests/unit/sandboxes.test.ts add cases using
makeTestApp, SessionManager, makeToken and InMemoryFs to (1) POST /v1/sandboxes
succeeds when body.python_runtime is "pyodide" and when it is null, (2) POST
/v1/sandboxes returns a 4xx when body.python_runtime is the legacy "python"
value, and (3) GET /v1/sandboxes/:id returns a python_runtime field on both warm
paths (sandbox present in the SessionManager pool) and cold paths (only
getSandboxMetaFn provides metadata) asserting the exact returned value matches
the stored metadata; reuse the existing helpers (makeTestApp, SessionManager
constructor/getSandboxMetaFn, makeToken) and mirror the style of the existing
test that checks createdAt for setup and assertions.
---
Duplicate comments:
In `@clients/typescript/CHANGELOG.md`:
- Around line 5-18: Revert the manual edits to CHANGELOG.md and instead add a
Changeset file under .changeset describing the 0.4.0 changes (mentioning the
python -> python_runtime migration, SandboxRecord.python_runtime,
SandboxRecord.network, and exporting PythonRuntime) with the appropriate release
type; commit only the .changeset/*.md in this feature PR and remove CHANGELOG.md
from the commit, then for the release flow run the project's release step (pnpm
changeset:version) in the release PR to regenerate CHANGELOG.md via the
Changesets workflow.
In `@clients/typescript/src/models.ts`:
- Line 91: The assignment that casts payload.python_runtime to PythonRuntime
must validate the value first: add a guard (e.g., isValidPythonRuntime or use an
enum validator in the same module) and check payload.python_runtime is a string
and one of the allowed PythonRuntime values before casting; if validation fails,
set python_runtime to null or a safe default (or throw) instead of
unconditionally casting. Update the code paths that reference python_runtime
(the payload variable and the PythonRuntime type usage in the same model
factory/constructor) to use this validated value.
In `@src/api/pyodide/ipc.ts`:
- Around line 136-148: The assertFsEntry function currently only checks that
entry.data is a string; update assertFsEntry to also validate that when
entry.kind === "file" the entry.data is valid base64 and reject otherwise by
throwing IpcIntegrityError. Locate assertFsEntry and add a check for file
entries that attempts a safe base64 validation (e.g. test against a base64 regex
or attempt a decode with Buffer.from(..., "base64")/atob and verify the
round-trip or that decoding doesn't throw) and throw new
IpcIntegrityError(`${label}: invalid base64 data`) on failure so
malformed/non-base64 payloads are rejected before they reach the drain.
In `@src/api/pyodide/manager.ts`:
- Around line 367-396: In sendRunFrame, add temporary async handlers on
child.stdin (use child.stdin.once('error', ...) and once('close', ...)) before
calling child.stdin.write(encodeFrame(frame)) so async stream errors (EPIPE /
ERR_STREAM_DESTROYED) get routed to this.#failOwned(err, true); ensure these
handlers are removed after the write completes (use the write callback or
drain/finish event) to avoid leaking listeners; keep the existing null-check for
child.stdin and still wrap the synchronous write in try/catch, but rely on the
temporary listeners to catch asynchronous failures and forward them to
`#failOwned`; reference sendRunFrame, child.stdin, encodeFrame, and `#failOwned`
when applying the change.
In `@tsconfig.json`:
- Line 22: The tsconfig.json currently excludes "src/pyodide-runner/runner.ts"
via the "exclude" entry which bypasses TypeScript checks for the
security-critical runner; either remove that exclusion and add a Deno-compatible
tsconfig (or adjust existing tsconfig settings and module resolution) so
runner.ts is compiled by your CI, or keep the exclusion but add and document a
CI gate that type-checks runner.ts (e.g., run `deno check` or an equivalent Deno
type-check step in the pipeline) and link that CI job in the repo docs; update
tsconfig.json and repository CI/docs accordingly and reference the exact
excluded path "src/pyodide-runner/runner.ts" when making the change.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 50b88281-c087-498b-88ac-6dc9f7a75ef2
⛔ Files ignored due to path filters (1)
clients/python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (86)
.changeset/python-runtime-enum.md.dockerignore.gitignoreCLAUDE.mdDockerfilebiome.jsonclients/python/CHANGELOG.mdclients/python/README.mdclients/python/examples/quickstart.pyclients/python/pyproject.tomlclients/python/src/sqlfs/__init__.pyclients/python/src/sqlfs/_version.pyclients/python/src/sqlfs/client.pyclients/python/src/sqlfs/models.pyclients/python/tests/test_client.pyclients/typescript/CHANGELOG.mdclients/typescript/README.mdclients/typescript/examples/e2e-local.tsclients/typescript/examples/quickstart.tsclients/typescript/package.jsonclients/typescript/src/client.tsclients/typescript/src/index.tsclients/typescript/src/models.tsclients/typescript/src/version.tsclients/typescript/tests/sandboxes.test.tspackage.jsonplugins/sql-fs/skills/api/SETUP.mdplugins/sql-fs/skills/api/ref/bash.mdplugins/sql-fs/skills/api/ref/endpoints.mdplugins/sql-fs/skills/py-sdk/SKILL.mdplugins/sql-fs/skills/py-sdk/ref/client.mdplugins/sql-fs/skills/py-sdk/ref/models.mdscripts/build-pyodide-lock.mjsscripts/copy-pyodide-runner.mjsscripts/fetch-pyodide-assets.mjssrc/api/commands/pyodide-command.tssrc/api/lib/batch-exec.tssrc/api/mcp/tools.tssrc/api/openapi-spec.tssrc/api/pyodide-runtime-context.tssrc/api/pyodide/ipc.tssrc/api/pyodide/manager.tssrc/api/pyodide/residency.tssrc/api/pyodide/tests/integration/escape.integration.test.tssrc/api/pyodide/tests/integration/frame-forgery.integration.test.tssrc/api/pyodide/tests/unit/fake-child.tssrc/api/pyodide/tests/unit/ipc.test.tssrc/api/pyodide/tests/unit/manager.test.tssrc/api/pyodide/tests/unit/pyodide-command.test.tssrc/api/pyodide/tests/unit/residency.test.tssrc/api/routes/exec.tssrc/api/routes/sandboxes.tssrc/api/session-manager.tssrc/api/tests/integration/migrations.integration.test.tssrc/api/tests/integration/pyodide.integration.test.tssrc/api/tests/session-manager.script-tx.test.tssrc/api/tests/unit/exec-batch.test.tssrc/api/tests/unit/exec.test.tssrc/api/tests/unit/files.test.tssrc/api/tests/unit/ingest.test.tssrc/api/tests/unit/mcp-tools.test.tssrc/api/tests/unit/sandboxes.test.tssrc/api/tests/unit/session-manager.pyodide.test.tssrc/api/tests/unit/session-manager.rehydrate.test.tssrc/api/tests/unit/session-manager.test.tssrc/pyodide-runner/protocol.tssrc/pyodide-runner/runner.tssrc/sql-fs/dialects/postgres.tssrc/sql-fs/dialects/tests/unit/postgres.advisory-lock.test.tssrc/sql-fs/migrations/postgres/0006_python_runtime.sqlsrc/sql-fs/types.tsthoughts/issue-118-pyodide-runtime/design.mdthoughts/issue-118-pyodide-runtime/plan.mdthoughts/issue-118-pyodide-runtime/questions.mdthoughts/issue-118-pyodide-runtime/research.mdthoughts/issue-118-pyodide-runtime/spikes/.gitignorethoughts/issue-118-pyodide-runtime/spikes/S1-findings.mdthoughts/issue-118-pyodide-runtime/spikes/S2-findings.mdthoughts/issue-118-pyodide-runtime/spikes/S3-findings.mdthoughts/issue-118-pyodide-runtime/spikes/s1-pyodide-deno.shthoughts/issue-118-pyodide-runtime/spikes/s1_runner.tsthoughts/issue-118-pyodide-runtime/spikes/s2-ipc.tsthoughts/issue-118-pyodide-runtime/spikes/s3-memory.shthoughts/issue-118-pyodide-runtime/structure.mdthoughts/issue-118-pyodide-runtime/task.mdtsconfig.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
plugins/sql-fs/skills/api/ref/bash.md (1)
60-73:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSplit the performance and concurrency guidance by runtime mode.
The performance section still conflates stdlib and pyodide behavior. Lines 60-61 state "Every call pays the WASM cold-boot cost (~1.4 s)" which only applies to
python_runtime: "stdlib", not to the Pyodide subprocess model. Similarly, the RAM cost (line 73) and concurrency limits (line 72) should clarify whether they apply to one or both modes.According to the PR objectives, Pyodide has "~5–8s cold start" (subprocess spawn + V8 + Pyodide init), "~350–400 MB per execution", and separate
MAX_CONCURRENT_PYODIDE(default 2). These are materially different from the stdlib figures shown here.Split this section into mode-specific guidance or add inline qualifiers (e.g., "stdlib: ~1.4s, pyodide: ~5–8s on cold start").
🤖 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 `@plugins/sql-fs/skills/api/ref/bash.md` around lines 60 - 73, Update the performance/concurrency guidance so it distinguishes runtime modes: change the claim "Every call pays the WASM cold-boot cost (~1.4 s)" and the RAM/concurrency figures into explicit, per-mode statements (e.g., for python_runtime == "stdlib": cold start ~1.4s, ~80 MB per python3 process, server-wide concurrency cap 5; for pyodide subprocess mode: cold start ~5–8s, ~350–400 MB per execution, and respect MAX_CONCURRENT_PYODIDE default 2). Edit the paragraph(s) in plugins/sql-fs/skills/api/ref/bash.md that reference the cold-boot, RAM (~80 MB) and concurrency (5) so they either become two short mode-specific bullet points or include inline qualifiers referencing python_runtime and MAX_CONCURRENT_PYODIDE, and ensure the guidance about persisting state and avoiding pip/os.system/subprocess remains unchanged.src/api/openapi-spec.ts (1)
745-756:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocument
perScriptTimeoutMsin the batch exec schema.
src/api/routes/exec.tsacceptsperScriptTimeoutMs, but the OpenAPI request body omits it. That breaks generated clients and hides the new timeout mode from API consumers.🤖 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 `@src/api/openapi-spec.ts` around lines 745 - 756, The OpenAPI schema for the batch exec request is missing the perScriptTimeoutMs property even though routes/exec.ts accepts it; update the schema in src/api/openapi-spec.ts (the same object containing timeoutMs, readOnly, retryOn5xx) to add a perScriptTimeoutMs field (type: integer, provide example and sensible minimum/maximum and a short description explaining its per-script timeout semantics) so generated clients and docs include the new timeout mode; ensure the new property name matches the parameter used in routes/exec.ts exactly and is documented alongside timeoutMs.src/api/mcp/tools.ts (1)
250-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMap internal Pyodide timeouts to MCP timeouts here as well.
This catch still only reports timeout when the local timer fired.
execWithRuntimeThrottle()can now throwcode === "EPYODIDE_TIMEOUT"first, which meansbash_execreturns a generic internal error while the HTTP and batch paths returntimeout.🤖 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 `@src/api/mcp/tools.ts` around lines 250 - 263, The catch block inside bash_exec should treat internal Pyodide timeouts the same as the local timer timeout: after clearTimeout(timer) and extra.signal.removeEventListener("abort", onDisconnect), detect if the caught error has e?.code === "EPYODIDE_TIMEOUT" (or equivalent) and return the same timeout result object (stdout:"", stderr:"timeout", exitCode:-1, exitSignal:null, timedOut:true, durationMs: Date.now() - startMs) instead of throwing; only rethrow for other errors. Ensure you reference the existing symbols timer, extra.signal, onDisconnect, startMs, and the function execWithRuntimeThrottle in your change.src/api/routes/exec.ts (1)
291-299:⚠️ Potential issue | 🟠 Major | ⚡ Quick winA resolved streamed exec should never be rewritten to
timeout.Unlike
/exec-sync, the SSE path still checkstimedOutafter a successfulexecWithRuntimeThrottle()return. If the timer fires in the same tick after completion, the client sees a timeout even though the script already finished and may have committed writes.🤖 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 `@src/api/routes/exec.ts` around lines 291 - 299, The code currently may set timedOut in the timer callback after execWithRuntimeThrottle() already completed and sent an exit event; add a separate "completed" (or "finished") boolean flag that you set to true immediately before sending any exit SSE in the successful exec path (where stream.writeSSE is called after execWithRuntimeThrottle()), and update the timer callback to check completed before treating the run as a timeout (i.e., only set timedOut/send timeout SSE if completed is false). Ensure all code paths that send an exit SSE (the post-exec success path and any error/early exit paths) set completed = true before returning so the timer cannot overwrite a resolved run.src/api/routes/sandboxes.ts (2)
46-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't treat malformed JSON as an empty create request.
A body like
{still falls into this broadcatchand the handler proceeds with all defaults, soPOST /v1/sandboxesreturns201and creates a sandbox instead of400. That bypasses the new strictpython_runtimevalidation path and turns client parse errors into side effects. Only an actually empty body should be ignored; parse failures should returnINVALID_INPUT.🤖 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 `@src/api/routes/sandboxes.ts` around lines 46 - 58, The request handler currently swallows JSON parse errors in the try/catch around c.req.json() (used with createBodySchema), allowing malformed JSON like "{" to be treated as an empty create and create a sandbox; change the flow so you first read the raw request body as text, if raw.trim() === "" treat it as an empty body (keep existing defaults), otherwise attempt JSON.parse or c.req.json() and if parsing fails return c.json({ error: "validation_error", code: "INVALID_INPUT", details: [...] }, 400) instead of proceeding; update the block around c.req.json(), createBodySchema.safeParse, and the catch to differentiate empty body vs parse failure and ensure parse failures always return INVALID_INPUT.
187-195:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize legacy metadata on the cold-replica GET path.
This fallback now serializes
meta.python_runtime,meta.javascript, andmeta.networkas-is. IfgetSandboxMeta()returns a pre-migration/legacy entry, those values areundefined, and the JSON response drops the fields entirely. That breaks the new API contract exactly on the rolling-deploy path this PR is trying to keep compatible. Default the booleans tofalseand derivepython_runtimefrom legacy metadata when present.Based on the rolling-deploy compatibility objective for
python_runtime, this fallback still needs to handle pre-migration metadata.🤖 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 `@src/api/routes/sandboxes.ts` around lines 187 - 195, The cold-replica GET response currently returns meta.python_runtime, meta.javascript, and meta.network as-is, which causes those fields to be omitted when getSandboxMeta() returns legacy metadata; update the fallback serialization in the endpoint that constructs the JSON response (the block returning c.json with id/name/owner/createdAt/lastUsedAt) to default javascript and network to false when undefined and to derive python_runtime from legacy metadata (e.g., coerce legacy values into the new python_runtime boolean) so that the response always includes python_runtime, javascript, and network (booleans) even for pre-migration entries.src/api/tests/unit/sandboxes.test.ts (1)
261-293: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd direct route coverage for the new
python_runtimecontract.This file only updates a fixture to the renamed metadata shape. It still does not assert that
POST /v1/sandboxesaccepts"pyodide"/null, rejects legacypython, and thatGET /v1/sandboxes/:idreturnspython_runtimeon both warm and cold paths. Those are the load-bearing API changes in this PR.As per coding guidelines, unit tests should cover both success and error cases with exact assertions; the PR objective here is the
python_runtimeAPI migration.🤖 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 `@src/api/tests/unit/sandboxes.test.ts` around lines 261 - 293, Add unit tests that directly assert the new python_runtime contract: in src/api/tests/unit/sandboxes.test.ts add cases using makeTestApp, SessionManager, makeToken and InMemoryFs to (1) POST /v1/sandboxes succeeds when body.python_runtime is "pyodide" and when it is null, (2) POST /v1/sandboxes returns a 4xx when body.python_runtime is the legacy "python" value, and (3) GET /v1/sandboxes/:id returns a python_runtime field on both warm paths (sandbox present in the SessionManager pool) and cold paths (only getSandboxMetaFn provides metadata) asserting the exact returned value matches the stored metadata; reuse the existing helpers (makeTestApp, SessionManager constructor/getSandboxMetaFn, makeToken) and mirror the style of the existing test that checks createdAt for setup and assertions.Source: Coding guidelines
♻️ Duplicate comments (5)
tsconfig.json (1)
22-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExclusion of runner.ts still lacks documented CI type-checking gate.
The exclusion of
src/pyodide-runner/runner.tsfrom TypeScript compilation remains without a documented alternative validation path. While the exclusion may be justified (Deno vs Node module systems), the past review's requirement for "a dedicated CI gate that fails on runner type errors" appears unmet based on the provided context.Either:
- Remove the exclusion and add a Deno-compatible tsconfig, or
- Document the CI gate that validates runner.ts types (e.g.,
deno checkin the build pipeline)The runner is security-critical (IPC integrity, realm lockdown) and should not bypass static validation.
🤖 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 `@tsconfig.json` at line 22, The tsconfig.json currently excludes "src/pyodide-runner/runner.ts" via the "exclude" entry which bypasses TypeScript checks for the security-critical runner; either remove that exclusion and add a Deno-compatible tsconfig (or adjust existing tsconfig settings and module resolution) so runner.ts is compiled by your CI, or keep the exclusion but add and document a CI gate that type-checks runner.ts (e.g., run `deno check` or an equivalent Deno type-check step in the pipeline) and link that CI job in the repo docs; update tsconfig.json and repository CI/docs accordingly and reference the exact excluded path "src/pyodide-runner/runner.ts" when making the change.clients/typescript/CHANGELOG.md (1)
5-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCHANGELOG should be managed via Changesets, not manually edited.
Per coding guidelines and learnings,
CHANGELOG.mdis generated through the Changesets workflow. In a feature PR, only the.changeset/*.mdfile should be committed. The CHANGELOG.md update should happen in a separate release PR viapnpm changeset:version. Manual edits here bypass the project's release automation.🤖 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 `@clients/typescript/CHANGELOG.md` around lines 5 - 18, Revert the manual edits to CHANGELOG.md and instead add a Changeset file under .changeset describing the 0.4.0 changes (mentioning the python -> python_runtime migration, SandboxRecord.python_runtime, SandboxRecord.network, and exporting PythonRuntime) with the appropriate release type; commit only the .changeset/*.md in this feature PR and remove CHANGELOG.md from the commit, then for the release flow run the project's release step (pnpm changeset:version) in the release PR to regenerate CHANGELOG.md via the Changesets workflow.Source: Learnings
clients/typescript/src/models.ts (1)
91-91:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate API response before type assertion.
Line 91 casts
payload.python_runtimetoPythonRuntimewithout validation. If the API returns an unexpected value (e.g.,"invalid",123), the type system accepts it but runtime behavior may be incorrect.🛡️ Add validation guard
export function sandboxRecordFromApi(payload: ApiObject): SandboxRecord { + const rawRuntime = payload.python_runtime ?? null; + const validRuntimes = new Set<PythonRuntime>(["stdlib", "pyodide", null]); + if (!validRuntimes.has(rawRuntime as PythonRuntime)) { + throw new Error(`Invalid python_runtime from API: ${String(rawRuntime)}`); + } return { id: String(payload.id), name: payload.name == null ? null : String(payload.name), owner: String(payload.owner), createdAt: String(payload.createdAt), - python_runtime: (payload.python_runtime ?? null) as PythonRuntime, + python_runtime: rawRuntime as PythonRuntime, javascript: Boolean(payload.javascript), network: Boolean(payload.network), }; }🤖 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 `@clients/typescript/src/models.ts` at line 91, The assignment that casts payload.python_runtime to PythonRuntime must validate the value first: add a guard (e.g., isValidPythonRuntime or use an enum validator in the same module) and check payload.python_runtime is a string and one of the allowed PythonRuntime values before casting; if validation fails, set python_runtime to null or a safe default (or throw) instead of unconditionally casting. Update the code paths that reference python_runtime (the payload variable and the PythonRuntime type usage in the same model factory/constructor) to use this validated value.src/api/pyodide/ipc.ts (1)
136-148:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing base64 validation in
assertFsEntryallows forged file payloads.The past review concern is valid:
assertFsEntryonly checkstypeof entry.data === "string"but does not validate thatentry.datais valid base64 whenentry.kind === "file". A forgedresultframe with non-base64datawould pass integrity validation and reach the drain layer, where decoding garbage could corrupt persisted files.Add base64 validation for file entries to reject malformed payloads before they reach the drain.
🛡️ Defensive validation
function assertFsEntry(entry: unknown, label: string): void { if (!isObject(entry)) throw new IpcIntegrityError(`${label}: entry is not an object`); if (typeof entry.path !== "string" || entry.path.length === 0) { throw new IpcIntegrityError(`${label}: missing/invalid path`); } if (entry.kind !== "file" && entry.kind !== "dir") throw new IpcIntegrityError(`${label}: invalid kind`); if (typeof entry.mode !== "number" || !Number.isInteger(entry.mode) || entry.mode < 0) { throw new IpcIntegrityError(`${label}: missing/invalid mode`); } if (typeof entry.data !== "string") throw new IpcIntegrityError(`${label}: missing/invalid data`); + if (entry.kind === "file" && entry.data.length > 0) { + // Validate base64: should only contain [A-Za-z0-9+/] and optional trailing "=" padding + if (!/^[A-Za-z0-9+/]*={0,2}$/.test(entry.data)) { + throw new IpcIntegrityError(`${label}: file data is not valid base64`); + } + } if (entry.kind === "dir" && entry.data !== "") throw new IpcIntegrityError(`${label}: dir entry must have empty data`); }🤖 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 `@src/api/pyodide/ipc.ts` around lines 136 - 148, The assertFsEntry function currently only checks that entry.data is a string; update assertFsEntry to also validate that when entry.kind === "file" the entry.data is valid base64 and reject otherwise by throwing IpcIntegrityError. Locate assertFsEntry and add a check for file entries that attempts a safe base64 validation (e.g. test against a base64 regex or attempt a decode with Buffer.from(..., "base64")/atob and verify the round-trip or that decoding doesn't throw) and throw new IpcIntegrityError(`${label}: invalid base64 data`) on failure so malformed/non-base64 payloads are rejected before they reach the drain.src/api/pyodide/manager.ts (1)
367-396:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd async error handlers for
child.stdinto prevent unhandled stream failures.The past review concern remains valid:
child.stdin.write(encodeFrame(frame))at line 392 is wrapped in synchronoustry/catch, but there are nochild.stdin'error'or'close'event handlers. If the child process terminates between the null-check at line 387 and the write at line 392, the writable stream may emit async errors (e.g.,EPIPE,ERR_STREAM_DESTROYED) that bypass the try/catch and are not routed through#failOwned().Attach temporary error/close handlers to
child.stdinbefore writing, and remove them after the write completes to ensure broken-pipe scenarios are properly handled.🔒 Proposed fix for stdin error handling
`#sendRunFrame`(op: OwnedOp): void { if (op.done) return; const frame: RunRequest = { type: "run", requestId: this.#randomRequestId(), seq: ++this.#seqCounter, generation: this.#generation, code: op.input.code, argv: op.input.argv, stdin: op.input.stdin, files: op.input.files, cwd: op.input.cwd, env: op.input.env, }; op.requestId = frame.requestId; op.seq = frame.seq; op.stage = "response"; this.#pending = { requestId: frame.requestId, seq: frame.seq }; this.#state = "busy"; const child = this.#child; if (child?.stdin === null || child?.stdin === undefined) { this.#failOwned(new PyodideChildExitError(null, null), true); return; } + const onStdinError = (err: Error): void => { + this.#failOwned(err, true); + }; + const onStdinClose = (): void => { + this.#failOwned(new PyodideChildExitError(null, null), true); + }; + child.stdin.once("error", onStdinError); + child.stdin.once("close", onStdinClose); try { child.stdin.write(encodeFrame(frame)); + child.stdin.removeListener("error", onStdinError); + child.stdin.removeListener("close", onStdinClose); } catch (err) { + child.stdin.removeListener("error", onStdinError); + child.stdin.removeListener("close", onStdinClose); this.#failOwned(err instanceof Error ? err : new Error(String(err)), 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 `@src/api/pyodide/manager.ts` around lines 367 - 396, In sendRunFrame, add temporary async handlers on child.stdin (use child.stdin.once('error', ...) and once('close', ...)) before calling child.stdin.write(encodeFrame(frame)) so async stream errors (EPIPE / ERR_STREAM_DESTROYED) get routed to this.#failOwned(err, true); ensure these handlers are removed after the write completes (use the write callback or drain/finish event) to avoid leaking listeners; keep the existing null-check for child.stdin and still wrap the synchronous write in try/catch, but rely on the temporary listeners to catch asynchronous failures and forward them to `#failOwned`; reference sendRunFrame, child.stdin, encodeFrame, and `#failOwned` when applying the change.
🤖 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 `@Dockerfile`:
- Around line 9-10: The RUN apt-get install invocation installs curl, unzip, and
bzip2 without version pins; change the RUN line that installs these build-time
packages (the RUN ... apt-get install -y --no-install-recommends curl unzip
bzip2 ...) to pin explicit Debian package versions (e.g. curl=<version>
unzip=<version> bzip2=<version>) so builds are reproducible — determine the
exact versions from the base image (using apt-cache policy in node:22-bookworm
or equivalent) and update the package list invocation to install those pinned
versions and still clean /var/lib/apt/lists/* afterward.
In `@plugins/sql-fs/skills/py-sdk/ref/client.md`:
- Line 57: Remove the empty blank line separating the two consecutive blockquote
blocks in plugins/sql-fs/skills/py-sdk/ref/client.md so the blockquotes are
directly adjacent (this fixes MD028); locate the two successive blockquote
sections in that file and delete the intervening blank line between them.
In `@scripts/fetch-pyodide-assets.mjs`:
- Around line 98-120: In fetchDeno(), add integrity verification for the
downloaded archive by computing a SHA-256 checksum of the downloaded zip (or the
extracted DENO_BIN) and comparing it to a trusted expected value before
unpacking or using the binary; introduce or use a checksum mapping keyed by
PINS.deno/denoTarget() (e.g., DENO_SHA_MAP or a manifest fetched alongside the
release) and if the computed checksum does not match the expected value throw an
error and abort; ensure you compute the hash from the same file referenced by
zip (or DENO_BIN after extraction) using a stable Node crypto hash, compare
constant-time, and only call execFileSync("unzip", ...) / chmodSync(DENO_BIN,
...) / log success after the verification passes.
In `@src/api/commands/pyodide-command.ts`:
- Around line 474-489: The guard currently only lstats the final resolved path;
change Pyodide drain logic to walk each ancestor segment of resolved (up to but
not including root) and lstat each parent to detect any symbolic links before
performing mkdir/writeFile/chmod; if any parent lstat shows a symlink, throw the
same PyodideDrainError (use the same message pattern referencing entry.path).
Ensure you perform this ancestor-symlink check prior to the existing entry.kind
"dir" branch and before writing/chmod so no symlink ancestors are followed.
- Around line 363-369: The walk function currently swallows errors from
fs.readdir by returning in the catch, causing incomplete staged snapshots;
change the catch in walk (the async function walk(dir: string)) to surface
filesystem errors instead of returning silently — either rethrow the caught
error or throw a new descriptive Error including the original error and the dir
(do not swallow), so callers (Pyodide exec / staged snapshot logic) will fail
fast on unreadable directories rather than proceed with a partial tree.
In `@src/api/pyodide/residency.ts`:
- Around line 173-180: The idle sweeper method `#sweep`() currently
fire-and-forgets worker.dispose(), which can produce unhandled promise
rejections; update the disposal call inside the loop that removes from
this.#residents to handle rejections (e.g., append a .catch(...) to the promise
or await in a safe async helper) and log the error (or surface it via your
existing logger) so disposal failures are captured; reference the `#sweep` method,
this.#residents iteration, the `#evictable` check, this.#idleMs timeout logic, and
the worker.dispose() call when making this change.
In `@src/api/pyodide/tests/integration/escape.integration.test.ts`:
- Around line 45-61: The test suite sets process.env.PYODIDE_ASSET_DIR and
process.env.DENO_BIN_PATH in beforeAll but only restores AUTH_SECRET in
afterAll; capture the original values (e.g., savedPyodideAssetDir and
savedDenoBin) alongside savedSecret in the beforeAll setup, and in afterAll
restore them (or delete the env prop if undefined) just like AUTH_SECRET is
handled so PYODIDE_ASSET_DIR and DENO_BIN_PATH do not leak to other tests;
update the beforeAll/afterAll blocks surrounding SessionManager/session setup
and shutdown to use these saved variables.
In `@src/api/pyodide/tests/integration/frame-forgery.integration.test.ts`:
- Around line 81-89: Save the original environment values for PYODIDE_ASSET_DIR
and DENO_BIN_PATH at the start of the suite (in the same scope as beforeAll),
then restore them in afterAll; specifically, in beforeAll capture previous
values (e.g. prevPyodideAssetDir, prevDenoBinPath) before mutating process.env,
and in afterAll restore process.env.PYODIDE_ASSET_DIR and
process.env.DENO_BIN_PATH back to those captured values (setting to
undefined/null if they were unset) and do the restoration in a finally block
around sm.shutdown() so restoration always runs even if shutdown fails.
In `@src/api/tests/integration/pyodide.integration.test.ts`:
- Around line 33-52: The test suite currently only cleans sandboxes in afterAll
which can leak state; add an afterEach that iterates the cleanup array and
destroys each sandbox (using destroySandbox("postgres", id)) and then clears the
cleanup array, and update individual tests that create sandboxes to use
try/finally so they push the created id onto the shared cleanup array and ensure
removal in the finally block; reference the existing cleanup array,
destroySandbox, and SessionManager/describe.skipIf block when making these
changes.
In `@src/api/tests/unit/mcp-tools.test.ts`:
- Around line 120-128: The test "sandbox_create echoes the python_runtime it was
given" only asserts python_runtime and can miss regressions in the full tool
payload; update the test that calls captureToolHandler("sandbox_create") /
getHandler() so it parses result.content[0].text and uses
expect(parsedBody).toEqual(expectedPayload) with a complete expected object
(include id, name, javascript, python_runtime and any wrapper shape returned)
instead of just toBe on python_runtime; apply the same change to the related
test block mentioned (lines ~130-151) to assert the full JSON response payload
for exact-match assertions.
---
Outside diff comments:
In `@plugins/sql-fs/skills/api/ref/bash.md`:
- Around line 60-73: Update the performance/concurrency guidance so it
distinguishes runtime modes: change the claim "Every call pays the WASM
cold-boot cost (~1.4 s)" and the RAM/concurrency figures into explicit, per-mode
statements (e.g., for python_runtime == "stdlib": cold start ~1.4s, ~80 MB per
python3 process, server-wide concurrency cap 5; for pyodide subprocess mode:
cold start ~5–8s, ~350–400 MB per execution, and respect MAX_CONCURRENT_PYODIDE
default 2). Edit the paragraph(s) in plugins/sql-fs/skills/api/ref/bash.md that
reference the cold-boot, RAM (~80 MB) and concurrency (5) so they either become
two short mode-specific bullet points or include inline qualifiers referencing
python_runtime and MAX_CONCURRENT_PYODIDE, and ensure the guidance about
persisting state and avoiding pip/os.system/subprocess remains unchanged.
In `@src/api/mcp/tools.ts`:
- Around line 250-263: The catch block inside bash_exec should treat internal
Pyodide timeouts the same as the local timer timeout: after clearTimeout(timer)
and extra.signal.removeEventListener("abort", onDisconnect), detect if the
caught error has e?.code === "EPYODIDE_TIMEOUT" (or equivalent) and return the
same timeout result object (stdout:"", stderr:"timeout", exitCode:-1,
exitSignal:null, timedOut:true, durationMs: Date.now() - startMs) instead of
throwing; only rethrow for other errors. Ensure you reference the existing
symbols timer, extra.signal, onDisconnect, startMs, and the function
execWithRuntimeThrottle in your change.
In `@src/api/openapi-spec.ts`:
- Around line 745-756: The OpenAPI schema for the batch exec request is missing
the perScriptTimeoutMs property even though routes/exec.ts accepts it; update
the schema in src/api/openapi-spec.ts (the same object containing timeoutMs,
readOnly, retryOn5xx) to add a perScriptTimeoutMs field (type: integer, provide
example and sensible minimum/maximum and a short description explaining its
per-script timeout semantics) so generated clients and docs include the new
timeout mode; ensure the new property name matches the parameter used in
routes/exec.ts exactly and is documented alongside timeoutMs.
In `@src/api/routes/exec.ts`:
- Around line 291-299: The code currently may set timedOut in the timer callback
after execWithRuntimeThrottle() already completed and sent an exit event; add a
separate "completed" (or "finished") boolean flag that you set to true
immediately before sending any exit SSE in the successful exec path (where
stream.writeSSE is called after execWithRuntimeThrottle()), and update the timer
callback to check completed before treating the run as a timeout (i.e., only set
timedOut/send timeout SSE if completed is false). Ensure all code paths that
send an exit SSE (the post-exec success path and any error/early exit paths) set
completed = true before returning so the timer cannot overwrite a resolved run.
In `@src/api/routes/sandboxes.ts`:
- Around line 46-58: The request handler currently swallows JSON parse errors in
the try/catch around c.req.json() (used with createBodySchema), allowing
malformed JSON like "{" to be treated as an empty create and create a sandbox;
change the flow so you first read the raw request body as text, if raw.trim()
=== "" treat it as an empty body (keep existing defaults), otherwise attempt
JSON.parse or c.req.json() and if parsing fails return c.json({ error:
"validation_error", code: "INVALID_INPUT", details: [...] }, 400) instead of
proceeding; update the block around c.req.json(), createBodySchema.safeParse,
and the catch to differentiate empty body vs parse failure and ensure parse
failures always return INVALID_INPUT.
- Around line 187-195: The cold-replica GET response currently returns
meta.python_runtime, meta.javascript, and meta.network as-is, which causes those
fields to be omitted when getSandboxMeta() returns legacy metadata; update the
fallback serialization in the endpoint that constructs the JSON response (the
block returning c.json with id/name/owner/createdAt/lastUsedAt) to default
javascript and network to false when undefined and to derive python_runtime from
legacy metadata (e.g., coerce legacy values into the new python_runtime boolean)
so that the response always includes python_runtime, javascript, and network
(booleans) even for pre-migration entries.
In `@src/api/tests/unit/sandboxes.test.ts`:
- Around line 261-293: Add unit tests that directly assert the new
python_runtime contract: in src/api/tests/unit/sandboxes.test.ts add cases using
makeTestApp, SessionManager, makeToken and InMemoryFs to (1) POST /v1/sandboxes
succeeds when body.python_runtime is "pyodide" and when it is null, (2) POST
/v1/sandboxes returns a 4xx when body.python_runtime is the legacy "python"
value, and (3) GET /v1/sandboxes/:id returns a python_runtime field on both warm
paths (sandbox present in the SessionManager pool) and cold paths (only
getSandboxMetaFn provides metadata) asserting the exact returned value matches
the stored metadata; reuse the existing helpers (makeTestApp, SessionManager
constructor/getSandboxMetaFn, makeToken) and mirror the style of the existing
test that checks createdAt for setup and assertions.
---
Duplicate comments:
In `@clients/typescript/CHANGELOG.md`:
- Around line 5-18: Revert the manual edits to CHANGELOG.md and instead add a
Changeset file under .changeset describing the 0.4.0 changes (mentioning the
python -> python_runtime migration, SandboxRecord.python_runtime,
SandboxRecord.network, and exporting PythonRuntime) with the appropriate release
type; commit only the .changeset/*.md in this feature PR and remove CHANGELOG.md
from the commit, then for the release flow run the project's release step (pnpm
changeset:version) in the release PR to regenerate CHANGELOG.md via the
Changesets workflow.
In `@clients/typescript/src/models.ts`:
- Line 91: The assignment that casts payload.python_runtime to PythonRuntime
must validate the value first: add a guard (e.g., isValidPythonRuntime or use an
enum validator in the same module) and check payload.python_runtime is a string
and one of the allowed PythonRuntime values before casting; if validation fails,
set python_runtime to null or a safe default (or throw) instead of
unconditionally casting. Update the code paths that reference python_runtime
(the payload variable and the PythonRuntime type usage in the same model
factory/constructor) to use this validated value.
In `@src/api/pyodide/ipc.ts`:
- Around line 136-148: The assertFsEntry function currently only checks that
entry.data is a string; update assertFsEntry to also validate that when
entry.kind === "file" the entry.data is valid base64 and reject otherwise by
throwing IpcIntegrityError. Locate assertFsEntry and add a check for file
entries that attempts a safe base64 validation (e.g. test against a base64 regex
or attempt a decode with Buffer.from(..., "base64")/atob and verify the
round-trip or that decoding doesn't throw) and throw new
IpcIntegrityError(`${label}: invalid base64 data`) on failure so
malformed/non-base64 payloads are rejected before they reach the drain.
In `@src/api/pyodide/manager.ts`:
- Around line 367-396: In sendRunFrame, add temporary async handlers on
child.stdin (use child.stdin.once('error', ...) and once('close', ...)) before
calling child.stdin.write(encodeFrame(frame)) so async stream errors (EPIPE /
ERR_STREAM_DESTROYED) get routed to this.#failOwned(err, true); ensure these
handlers are removed after the write completes (use the write callback or
drain/finish event) to avoid leaking listeners; keep the existing null-check for
child.stdin and still wrap the synchronous write in try/catch, but rely on the
temporary listeners to catch asynchronous failures and forward them to
`#failOwned`; reference sendRunFrame, child.stdin, encodeFrame, and `#failOwned`
when applying the change.
In `@tsconfig.json`:
- Line 22: The tsconfig.json currently excludes "src/pyodide-runner/runner.ts"
via the "exclude" entry which bypasses TypeScript checks for the
security-critical runner; either remove that exclusion and add a Deno-compatible
tsconfig (or adjust existing tsconfig settings and module resolution) so
runner.ts is compiled by your CI, or keep the exclusion but add and document a
CI gate that type-checks runner.ts (e.g., run `deno check` or an equivalent Deno
type-check step in the pipeline) and link that CI job in the repo docs; update
tsconfig.json and repository CI/docs accordingly and reference the exact
excluded path "src/pyodide-runner/runner.ts" when making the change.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 50b88281-c087-498b-88ac-6dc9f7a75ef2
⛔ Files ignored due to path filters (1)
clients/python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (86)
.changeset/python-runtime-enum.md.dockerignore.gitignoreCLAUDE.mdDockerfilebiome.jsonclients/python/CHANGELOG.mdclients/python/README.mdclients/python/examples/quickstart.pyclients/python/pyproject.tomlclients/python/src/sqlfs/__init__.pyclients/python/src/sqlfs/_version.pyclients/python/src/sqlfs/client.pyclients/python/src/sqlfs/models.pyclients/python/tests/test_client.pyclients/typescript/CHANGELOG.mdclients/typescript/README.mdclients/typescript/examples/e2e-local.tsclients/typescript/examples/quickstart.tsclients/typescript/package.jsonclients/typescript/src/client.tsclients/typescript/src/index.tsclients/typescript/src/models.tsclients/typescript/src/version.tsclients/typescript/tests/sandboxes.test.tspackage.jsonplugins/sql-fs/skills/api/SETUP.mdplugins/sql-fs/skills/api/ref/bash.mdplugins/sql-fs/skills/api/ref/endpoints.mdplugins/sql-fs/skills/py-sdk/SKILL.mdplugins/sql-fs/skills/py-sdk/ref/client.mdplugins/sql-fs/skills/py-sdk/ref/models.mdscripts/build-pyodide-lock.mjsscripts/copy-pyodide-runner.mjsscripts/fetch-pyodide-assets.mjssrc/api/commands/pyodide-command.tssrc/api/lib/batch-exec.tssrc/api/mcp/tools.tssrc/api/openapi-spec.tssrc/api/pyodide-runtime-context.tssrc/api/pyodide/ipc.tssrc/api/pyodide/manager.tssrc/api/pyodide/residency.tssrc/api/pyodide/tests/integration/escape.integration.test.tssrc/api/pyodide/tests/integration/frame-forgery.integration.test.tssrc/api/pyodide/tests/unit/fake-child.tssrc/api/pyodide/tests/unit/ipc.test.tssrc/api/pyodide/tests/unit/manager.test.tssrc/api/pyodide/tests/unit/pyodide-command.test.tssrc/api/pyodide/tests/unit/residency.test.tssrc/api/routes/exec.tssrc/api/routes/sandboxes.tssrc/api/session-manager.tssrc/api/tests/integration/migrations.integration.test.tssrc/api/tests/integration/pyodide.integration.test.tssrc/api/tests/session-manager.script-tx.test.tssrc/api/tests/unit/exec-batch.test.tssrc/api/tests/unit/exec.test.tssrc/api/tests/unit/files.test.tssrc/api/tests/unit/ingest.test.tssrc/api/tests/unit/mcp-tools.test.tssrc/api/tests/unit/sandboxes.test.tssrc/api/tests/unit/session-manager.pyodide.test.tssrc/api/tests/unit/session-manager.rehydrate.test.tssrc/api/tests/unit/session-manager.test.tssrc/pyodide-runner/protocol.tssrc/pyodide-runner/runner.tssrc/sql-fs/dialects/postgres.tssrc/sql-fs/dialects/tests/unit/postgres.advisory-lock.test.tssrc/sql-fs/migrations/postgres/0006_python_runtime.sqlsrc/sql-fs/types.tsthoughts/issue-118-pyodide-runtime/design.mdthoughts/issue-118-pyodide-runtime/plan.mdthoughts/issue-118-pyodide-runtime/questions.mdthoughts/issue-118-pyodide-runtime/research.mdthoughts/issue-118-pyodide-runtime/spikes/.gitignorethoughts/issue-118-pyodide-runtime/spikes/S1-findings.mdthoughts/issue-118-pyodide-runtime/spikes/S2-findings.mdthoughts/issue-118-pyodide-runtime/spikes/S3-findings.mdthoughts/issue-118-pyodide-runtime/spikes/s1-pyodide-deno.shthoughts/issue-118-pyodide-runtime/spikes/s1_runner.tsthoughts/issue-118-pyodide-runtime/spikes/s2-ipc.tsthoughts/issue-118-pyodide-runtime/spikes/s3-memory.shthoughts/issue-118-pyodide-runtime/structure.mdthoughts/issue-118-pyodide-runtime/task.mdtsconfig.json
🛑 Comments failed to post (10)
Dockerfile (1)
9-10: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider pinning versions for build-time dependencies.
Hadolint flags
apt-get installwithout version pins. Whilecurl,unzip, andbzip2are stable POSIX utilities used only in the discarded builder stage, pinning versions ensures fully reproducible builds and guards against supply-chain concerns.🔒 Example with pinned versions
-RUN apt-get update && apt-get install -y --no-install-recommends curl unzip bzip2 \ +RUN apt-get update && apt-get install -y --no-install-recommends \ + curl=7.88.1-10+deb12u8 \ + unzip=6.0-28 \ + bzip2=1.0.8-5+b1 \ && rm -rf /var/lib/apt/lists/*Note: Pin to versions available in
node:22-bookworm(Debian 12). Check withdocker run --rm node:22-bookworm apt-cache policy curl unzip bzip2.🧰 Tools
🪛 Hadolint (2.14.0)
[warning] 9-9: Pin versions in apt get install. Instead of
apt-get install <package>useapt-get install <package>=<version>(DL3008)
🤖 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 `@Dockerfile` around lines 9 - 10, The RUN apt-get install invocation installs curl, unzip, and bzip2 without version pins; change the RUN line that installs these build-time packages (the RUN ... apt-get install -y --no-install-recommends curl unzip bzip2 ...) to pin explicit Debian package versions (e.g. curl=<version> unzip=<version> bzip2=<version>) so builds are reproducible — determine the exact versions from the base image (using apt-cache policy in node:22-bookworm or equivalent) and update the package list invocation to install those pinned versions and still clean /var/lib/apt/lists/* afterward.Source: Linters/SAST tools
plugins/sql-fs/skills/py-sdk/ref/client.md (1)
57-57: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
Remove blank line between blockquotes per MD028.
Markdownlint flags a blank line between consecutive blockquotes. Remove the empty line between the two blockquote blocks (lines 53–56 and 58–61).
🧹 Proposed fix
> you pass `allow_oversized=True`. - > Sizing note: the server caps the whole HTTP request body (default 256 MB) and📝 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.> you pass `allow_oversized=True`. > Sizing note: the server caps the whole HTTP request body (default 256 MB) and🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 57-57: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 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 `@plugins/sql-fs/skills/py-sdk/ref/client.md` at line 57, Remove the empty blank line separating the two consecutive blockquote blocks in plugins/sql-fs/skills/py-sdk/ref/client.md so the blockquotes are directly adjacent (this fixes MD028); locate the two successive blockquote sections in that file and delete the intervening blank line between them.Source: Linters/SAST tools
scripts/fetch-pyodide-assets.mjs (1)
98-120:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the relevant region sed -n '1,200p' scripts/fetch-pyodide-assets.mjs | nl -ba | sed -n '70,160p' # Search for any SHA/checksum verification related to Deno rg -n --hidden --no-ignore-vcs "DENO|deno.*sha|sha.*deno|checksum|checksums|integrity|SRI|sha256" scripts/fetch-pyodide-assets.mjs # Find where the downloaded Deno binary is later used/executed rg -n --hidden --no-ignore-vcs "DENO_BIN|deno\b|\bexec.*deno\b|\bspawn.*deno\b|${?DENO_BIN}|\bDENO_DIR\b" scripts/fetch-pyodide-assets.mjs || trueRepository: Hazzng/sql-fs
Length of output: 100
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show the relevant region with line numbers (without nl) awk '{print NR "\t" $0}' scripts/fetch-pyodide-assets.mjs | sed -n '70,160p' # Search for any SHA/checksum verification related to Deno rg -n --hidden --no-ignore-vcs "DENO|deno.*sha|sha.*deno|checksum|checksums|integrity|SRI|sha256" scripts/fetch-pyodide-assets.mjs # Find where the downloaded Deno binary is later used/executed rg -n --hidden --no-ignore-vcs "DENO_BIN|deno\b|\bexec.*deno\b|\bspawn.*deno\b|\bdeno run\b|\$\{?DENO_BIN\}?|\bDENO_DIR\b" scripts/fetch-pyodide-assets.mjs || trueRepository: Hazzng/sql-fs
Length of output: 5632
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show the file header / constants including PINS and SHA256 awk '{print NR "\t" $0}' scripts/fetch-pyodide-assets.mjs | sed -n '1,90p' # Find any SHA256 map entries and whether they mention Deno/deno-zip/deno binary rg -n "const SHA256|SHA256\s*=\s*{|deno.*sha|SHA256\[[^\]]*deno|verify\([^)]*DENO|verify\([^)]*deno" scripts/fetch-pyodide-assets.mjs # Specifically look for any verify() calls after fetchDeno() rg -n "fetchDeno\(\)|verify\(" scripts/fetch-pyodide-assets.mjs # Show remainder to see if Deno is ever executed later awk '{print NR "\t" $0}' scripts/fetch-pyodide-assets.mjs | sed -n '160,260p'Repository: Hazzng/sql-fs
Length of output: 4328
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show rest of denoTarget/fetchDeno area awk '{print NR "\t" $0}' scripts/fetch-pyodide-assets.mjs | sed -n '80,170p' # Show the end of the file (where deno/pyodide are probably used) awk '{print NR "\t" $0}' scripts/fetch-pyodide-assets.mjs | sed -n '170,260p' # Simple searches rg -n "verify\(" scripts/fetch-pyodide-assets.mjs rg -n "fetchDeno" scripts/fetch-pyodide-assets.mjs rg -n "DENO_BIN|DENO_DIR" scripts/fetch-pyodide-assets.mjs rg -n "SHA256\\[" scripts/fetch-pyodide-assets.mjsRepository: Hazzng/sql-fs
Length of output: 4334
🏁 Script executed:
#!/bin/bash set -euo pipefail # Search for uses of the vendored deno binary across repo rg -n --hidden --no-ignore-vcs "vendor/deno/deno|vendor/deno|DENO_BIN|deno-(v|)(run|$)|deno run|execFileSync\\(.*deno|spawn\\(.*deno" . # Also see where this script is invoked rg -n --hidden --no-ignore-vcs "fetch-pyodide-assets\\.mjs" . || trueRepository: Hazzng/sql-fs
Length of output: 13089
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show deno resolution/spawn logic in manager.ts sed -n '1,260p' src/api/pyodide/manager.ts | awk '{print NR "\t" $0}' | sed -n '120,260p' # Show the portion where spawn/exec uses denoBin sed -n '260,520p' src/api/pyodide/manager.ts | awk '{print NR "\t" $0}' | sed -n '1,260p' # Inspect runner for how it invokes deno (if at all) sed -n '1,240p' src/pyodide-runner/runner.ts | awk '{print NR "\t" $0}' | sed -n '1,120p' # Search for checksum/integrity logic in the pyodide module rg -n "sha256|checksum|integrity|verify\\(" src/api/pyodide src/pyodide-runnerRepository: Hazzng/sql-fs
Length of output: 23773
Verify Deno archive/binary integrity (add per-target SHA-256 or checksum-manifest verification).
fetchDeno()only checks that the extractedvendor/deno/denoexists (plus chmod and macOS xattr removal); it does not SHA-256 pin or validate the downloadeddeno-${target}.zip/ binary. Thatvendor/deno/denois later spawned byPyodideSandbox.#spawnChild()using the vendoredDENO_BIN_PATH, so a tampered Deno binary would compromise the host at runtime.🤖 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 `@scripts/fetch-pyodide-assets.mjs` around lines 98 - 120, In fetchDeno(), add integrity verification for the downloaded archive by computing a SHA-256 checksum of the downloaded zip (or the extracted DENO_BIN) and comparing it to a trusted expected value before unpacking or using the binary; introduce or use a checksum mapping keyed by PINS.deno/denoTarget() (e.g., DENO_SHA_MAP or a manifest fetched alongside the release) and if the computed checksum does not match the expected value throw an error and abort; ensure you compute the hash from the same file referenced by zip (or DENO_BIN after extraction) using a stable Node crypto hash, compare constant-time, and only call execFileSync("unzip", ...) / chmodSync(DENO_BIN, ...) / log success after the verification passes.src/api/commands/pyodide-command.ts (2)
363-369:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't silently drop unreadable directories from the staged snapshot.
Returning from the
readdir()catch makes Pyodide run against a partial cwd tree. In the locked session that turns a real filesystem error into false ENOENT/stale-state behavior instead of failing the exec.🤖 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 `@src/api/commands/pyodide-command.ts` around lines 363 - 369, The walk function currently swallows errors from fs.readdir by returning in the catch, causing incomplete staged snapshots; change the catch in walk (the async function walk(dir: string)) to surface filesystem errors instead of returning silently — either rethrow the caught error or throw a new descriptive Error including the original error and the dir (do not swallow), so callers (Pyodide exec / staged snapshot logic) will fail fast on unreadable directories rather than proceed with a partial tree.
474-489:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock ancestor symlinks during drain, not just the leaf path.
The current guard only
lstat()sresolveditself. If/cwd/linkis a symlink, draininglink/out.txtwill still follow that ancestor symlink and write through a path this layer is supposed to deny. Walk each parent segment beforemkdir/writeFile/chmod.🤖 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 `@src/api/commands/pyodide-command.ts` around lines 474 - 489, The guard currently only lstats the final resolved path; change Pyodide drain logic to walk each ancestor segment of resolved (up to but not including root) and lstat each parent to detect any symbolic links before performing mkdir/writeFile/chmod; if any parent lstat shows a symlink, throw the same PyodideDrainError (use the same message pattern referencing entry.path). Ensure you perform this ancestor-symlink check prior to the existing entry.kind "dir" branch and before writing/chmod so no symlink ancestors are followed.src/api/pyodide/residency.ts (1)
173-180:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatch idle-sweep disposal failures.
#sweep()fire-and-forgetsworker.dispose(). If that promise rejects, the interval callback produces an unhandled rejection from a background timer, which can destabilize the process under stricter Node settings.🩹 Suggested fix
if (now - touched >= this.#idleMs) { this.#residents.delete(worker); - void worker.dispose(); + void worker.dispose().catch(() => { + // best-effort — the worker is already deregistered + }); }📝 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.`#sweep`(): void { const now = Date.now(); // Snapshot entries — dispose() mutates the map mid-iteration. for (const [worker, touched] of [...this.#residents]) { if (!this.#evictable(worker)) continue; if (now - touched >= this.#idleMs) { this.#residents.delete(worker); void worker.dispose().catch(() => { // best-effort — the worker is already deregistered });🤖 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 `@src/api/pyodide/residency.ts` around lines 173 - 180, The idle sweeper method `#sweep`() currently fire-and-forgets worker.dispose(), which can produce unhandled promise rejections; update the disposal call inside the loop that removes from this.#residents to handle rejections (e.g., append a .catch(...) to the promise or await in a safe async helper) and log the error (or surface it via your existing logger) so disposal failures are captured; reference the `#sweep` method, this.#residents iteration, the `#evictable` check, this.#idleMs timeout logic, and the worker.dispose() call when making this change.src/api/pyodide/tests/integration/escape.integration.test.ts (1)
45-61:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore
PYODIDE_ASSET_DIRandDENO_BIN_PATHafter the suite.This suite overwrites both env vars in
beforeAllbut only restoresAUTH_SECRET. Any later test file that runs in the same worker inherits these vendored paths, so test order can change behavior. Snapshot and restore/delete both vars inafterAll.🤖 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 `@src/api/pyodide/tests/integration/escape.integration.test.ts` around lines 45 - 61, The test suite sets process.env.PYODIDE_ASSET_DIR and process.env.DENO_BIN_PATH in beforeAll but only restores AUTH_SECRET in afterAll; capture the original values (e.g., savedPyodideAssetDir and savedDenoBin) alongside savedSecret in the beforeAll setup, and in afterAll restore them (or delete the env prop if undefined) just like AUTH_SECRET is handled so PYODIDE_ASSET_DIR and DENO_BIN_PATH do not leak to other tests; update the beforeAll/afterAll blocks surrounding SessionManager/session setup and shutdown to use these saved variables.src/api/pyodide/tests/integration/frame-forgery.integration.test.ts (1)
81-89:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore the vendored runtime env after the suite.
beforeAllmutatesPYODIDE_ASSET_DIRandDENO_BIN_PATH, butafterAllnever restores the previous values. That leaks configuration into later suites in the same worker and makes their behavior depend on execution order.🤖 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 `@src/api/pyodide/tests/integration/frame-forgery.integration.test.ts` around lines 81 - 89, Save the original environment values for PYODIDE_ASSET_DIR and DENO_BIN_PATH at the start of the suite (in the same scope as beforeAll), then restore them in afterAll; specifically, in beforeAll capture previous values (e.g. prevPyodideAssetDir, prevDenoBinPath) before mutating process.env, and in afterAll restore process.env.PYODIDE_ASSET_DIR and process.env.DENO_BIN_PATH back to those captured values (setting to undefined/null if they were unset) and do the restoration in a finally block around sm.shutdown() so restoration always runs even if shutdown fails.src/api/tests/integration/pyodide.integration.test.ts (1)
33-52:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove sandbox cleanup to
afterEachand use per-testtry/finally.These cases create real sandboxes, but cleanup only happens once in
afterAll. A failing test can leak state into later cases, and this breaks the integration-test cleanup contract for the repo.As per coding guidelines, "
Every test that creates a sandbox must delete it in afterEach" and integration tests should "Usetry/finally."🤖 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 `@src/api/tests/integration/pyodide.integration.test.ts` around lines 33 - 52, The test suite currently only cleans sandboxes in afterAll which can leak state; add an afterEach that iterates the cleanup array and destroys each sandbox (using destroySandbox("postgres", id)) and then clears the cleanup array, and update individual tests that create sandboxes to use try/finally so they push the created id onto the shared cleanup array and ensure removal in the finally block; reference the existing cleanup array, destroySandbox, and SessionManager/describe.skipIf block when making these changes.Source: Coding guidelines
src/api/tests/unit/mcp-tools.test.ts (1)
120-128: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Assert the full tool payloads, not just
python_runtime.These tests still pass if
id,name,javascript, or the response wrapping regresses, so they don't really pin the echoed contract. PrefertoEqual(...)on the full parsed JSON body. As per coding guidelines, "Assert full output: Prefer exact match (toBe,toEqual) over partial (toContain)."Also applies to: 130-151
🤖 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 `@src/api/tests/unit/mcp-tools.test.ts` around lines 120 - 128, The test "sandbox_create echoes the python_runtime it was given" only asserts python_runtime and can miss regressions in the full tool payload; update the test that calls captureToolHandler("sandbox_create") / getHandler() so it parses result.content[0].text and uses expect(parsedBody).toEqual(expectedPayload) with a complete expected object (include id, name, javascript, python_runtime and any wrapper shape returned) instead of just toBe on python_runtime; apply the same change to the related test block mentioned (lines ~130-151) to assert the full JSON response payload for exact-match assertions.Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@clients/typescript/src/models.ts`:
- Around line 85-90: The thrown Error in function toPythonRuntime currently
lacks a `code` property; update the error creation in toPythonRuntime(value:
unknown) so it constructs an Error object with the message `unexpected
python_runtime from server: ...` and assign a standard error code (e.g.,
`EINVALID` or another appropriate code) to its `code` property before throwing;
ensure the thrown value remains an Error instance (not a raw string) and keep
the existing JSON.stringify(value) message content.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 71b5347e-2491-43b7-9286-71d8f7d09c29
📒 Files selected for processing (19)
clients/typescript/src/models.tspackage.jsonscripts/build-pyodide-lock.mjsscripts/fetch-pyodide-assets.mjssrc/api/commands/pyodide-command.tssrc/api/pyodide/ipc.tssrc/api/pyodide/manager.tssrc/api/pyodide/residency.tssrc/api/pyodide/tests/integration/escape.integration.test.tssrc/api/pyodide/tests/integration/frame-forgery.integration.test.tssrc/api/pyodide/tests/unit/ipc.test.tssrc/api/pyodide/tests/unit/pyodide-command.test.tssrc/api/pyodide/tests/unit/residency.test.tssrc/api/session-manager.tssrc/api/tests/integration/pyodide.integration.test.tssrc/api/tests/unit/mcp-tools.test.tssrc/pyodide-runner/runner.tsthoughts/issue-118-pyodide-runtime/spikes/S3-findings.mdthoughts/issue-118-pyodide-runtime/spikes/s1-pyodide-deno.sh
Adds the full question/research/design/structure/plan artifact set for the Pyodide Python runtime work: python_runtime enum migration, OS-isolated Deno subprocess execution, committed IPC with frame-integrity invariants, file diff-and-drain, concurrency/residency caps, and the adversarial escape suite.
…hild memory Validate the three architecture gates for the Pyodide runtime (issue Hazzng#118) before building product phases. All three spike scripts pass (exit 0): - S1: Pyodide 0.29.4 loads fully offline on Deno v2.8.2 under the committed deny-belt and runs a pandas->openpyxl xlsx round-trip with zero network. - S2: realm lockdown + frame-integrity validation; surfaced that lockdown is not stdout containment (import(node:fs).writeSync reaches stdout), so Node-side validation with secret requestId/seq/generation is load-bearing. - S3: non-root cannot set cgroup memory.max and prlimit --as is unusable for RSS (V8/WASM vaddr >> RSS) -> container memory limit is the guard. Records findings per spike and propagates the S2 finding into the Phase 3/4/7 plan and design wording (no false stdout-containment claims). Assets are git-ignored and reproduced by the spike scripts.
…ib + null end-to-end)
…ory posture Bound in-flight pyodide execs (pyodideSem, MAX_CONCURRENT_PYODIDE) and resident subprocesses (PyodideResidency LRU, MAX_RESIDENT_PYODIDE) independently. Admission is lazy and post-semaphore so it always holds a slot, guaranteeing resident subprocesses never exceed MAX_RESIDENT; eviction/idle-kill disposes idle workers and the owning session re-admits a fresh manager on its next exec. Enforce the MAX_RESIDENT_PYODIDE >= MAX_CONCURRENT_PYODIDE startup invariant. Document the accepted memory posture (container limit is the guard; per-child OOM isolation is not provided) and all new env vars.
…e gate) Add the security sign-off suites that run untrusted Python in the real OS-isolated Deno child. escape.integration proves each escape fails closed (no secret read, no network, no host-FS, no subprocess, no FFI; fresh-globals isolation). frame-forgery.integration proves the load-bearing IPC integrity control: a forged frame injected via the real node:fs.writeSync(1,…) escape hatch is rejected on its unguessable integrity fields, killing the child and draining nothing, then the session cold-starts a fresh generation. Extend the test:integration script to include the new pyodide integration directory.
Addresses the maintainer + Codex review findings on the pyodide runtime: - Runner: explicit ordered init sequence with a pre-`ready` adversarial self-test and an init try/catch that fails the spawn on error; snapshot and restore the FULL os.environ per run so env is strictly per-execution. - Commands: read script files through one capped + symlink-refusing helper before decoding; reject read-only execs that report any mutation before touching the FS; stage out-of-cwd scripts at a reserved non-drainable path; validate the full drain manifest (uniqueness, reserved-prefix, write/delete ancestor conflicts, file-as-directory) before any write. - Timeout: surface the internal EPYODIDE_TIMEOUT as a fatal throw that aborts the script transaction, mapped to 408 / SSE / batch timeout. - Manager: resolve a bare Deno binary to an absolute path (the child env omits PATH); inject exported bash env into Python os.environ per run. - Residency: clarify that admission is lazy/post-semaphore and the spawn thunk is cheap (no expensive init under the mutex). Adds unit + integration coverage and updates the design record.
- runner: set/restore sys.path[0] for file scripts (sibling-import parity); track entry KIND in the diff baseline so file↔dir replacements drain. - drain: validate file payloads are base64 (ipc); reject symlinked ancestors and handle wrong-kind targets; stop swallowing readdir() staging errors. - residency: refresh the idle clock per Pyodide command (not just per bash.exec); catch dispose() rejections in the idle sweep. - manager: handle async child.stdin stream errors (broken pipe). - supply chain: pin the Deno binary sha256 for all four supported targets; fetch hard-fails on an unpinned or mismatched binary. - tooling: add typecheck:runner (deno check) gate for the tsc-excluded runner; TS SDK validates python_runtime; parseNameVersion guards wheels. - tests: per-execution env isolation + sys.path + file↔dir coverage; restore PYODIDE_ASSET_DIR/DENO_BIN_PATH and clean up sandboxes per test.
Per the error-handling guideline, sandboxRecordFromApi and streamEventFromApi now throw Error instances carrying a code (EINVALID_PYTHON_RUNTIME / EUNKNOWN_SSE_EVENT) instead of bare Errors.
Upstream added 0006_blob_last_referenced_at.sql; renumber the python_runtime migration to avoid the duplicate 0006 prefix. Content unchanged (idempotent; runs after the blob-gc migration).
NeilMazumdar
force-pushed
the
feature/pyodide-python-runtime
branch
from
6ad66e6 to
baecea7
Compare
Owner
🐛
|
| Artifact | Pinned | Downloaded | |
|---|---|---|---|
pyodide.mjs (17 KB JS loader) |
c8dffeef…65461 |
8fdfed5e…4a751 |
❌ mismatch |
pyodide.asm.wasm (8.6 MB runtime) |
10090fe4…81e3 |
10090fe4…81e3 |
✅ exact |
python_stdlib.zip (2.4 MB) |
92cb24fa…0efb4 |
92cb24fa…0efb4 |
✅ exact |
The wasm runtime and stdlib (the security-critical execution payload) are byte-identical to the pins — only the small JS bootstrap loader re-rolled upstream within the 0.29.4 release after the pin was recorded. So this reads as a benign upstream loader rebuild, not tampering; the pin is just stale.
Suggested fix
Re-pin SHA256["pyodide.mjs"] to 8fdfed5eaf81bde14bcdeaeea11f2672675b2362248f8537446b6fda5e4a4751 (after independently confirming against Pyodide's published checksums, since it's a supply-chain pin). pyodide.asm.wasm and python_stdlib.zip need no change.
Once re-pinned, provisioning proceeds (re-running the fetch skips the ~408 MB re-download and just verifies + grabs the openpyxl/et_xmlfile wheels). Happy to PR the pin update if useful.
- runner: preload numpy/pandas/matplotlib (swap scipy out) so the pyodide runtime covers CSV/Excel analysis + plotting; loaded by name from the stock lock with automatic dependency resolution. - fetch-pyodide-assets: re-pin pyodide.mjs to the actual 0.29.4 GitHub release hash. The previous pin was stale (only the JS loader re-rolled upstream; pyodide.asm.wasm + python_stdlib.zip are byte-identical), which hard-failed the offline checksum guard during provisioning. - README: document the python -> python_runtime breaking change, the stdlib vs pyodide runtimes, the preloaded package set, and memory/cap sizing (16 MiB file cap + MAX_CONCURRENT_PYODIDE=1 on a 2 GB host).
Owner
Local testing findings + suggested changes (memory, packages, plotting)Ran the runtime end-to-end locally (macOS, Neon DB, Deno v2.8.2 + Pyodide 0.29.4 vendored). Sharing the changes I made + the empirical memory characterization. I can't push to the fork, so the diffs are inline below. 1. Provisioning blocker — re-pin
|
| Scenario | Worker peak | Combined peak |
|---|---|---|
| Idle (fresh container) | — | ~98 MiB |
| Pyodide floor, trivial script (numpy/pandas preloaded) | ~580 MB | — |
| stdlib-only Pyodide (packages NOT preloaded) | ~317 MB | — |
| pandas on 18 MB CSV (290k rows, 70 MB DataFrame) | ~1.0 GB | ~1.6 GB |
| + matplotlib plot | ~1.0 GB | ~1.2 GB |
| 50 MB CSV (raised stage cap) | ~1.6–2.0 GB | ~3.0 GB |
| mixed: 4 plots + 40 bash @cap=2 | ~2.0 GB (2 workers) | ~2.5 GB |
| mixed: 4 plots + 40 bash @cap=1 | ~1.2 GB (1 worker) | ~1.6 GB |
Notes:
- bash + the
stdlibruntime are ~free — all real memory pressure is Pyodide workers; the cap bounds it (workers × ~1 GB). chunksize/dtype-downcast do NOT reduce Pyodide memory — WASM heap never shrinks (peak = high-water) and the file is fully staged regardless; chunked variants measured higher (~2.0 GB vs ~1.6 GB naive). Reduce columns/rows before upload instead.- The node ingest spike scales ~5× file size (50 MB CSV → ~2.6 GB transient on the node side from base64 buffering) — worth streaming large uploads.
Sizing rec: 2 GB host → MAX_CONCURRENT_PYODIDE=1 + PYODIDE_MAX_FILE_BYTES=16777216 (16 MiB). The shipped 32 MiB default is borderline for 2 GB (a 50 MB file is rejected by it, by design — confirmed EPYODIDE_DRAIN ... exceeds the per-file stage cap).
4. Plot retrieval works end-to-end
matplotlib savefig under cwd → drains back to the sandbox FS → retrievable via GET /v1/sandboxes/{id}/files/<path> (200, image/png, valid PNG). No dedicated export needed.
Profiling an 18.8 MB-CSV workload showed the spike was IPC/staging machinery, not pandas. Three measured fixes: - O(n) chunk-list framing in the Deno runner stdin loop and the Node manager #onStdoutData (was O(n^2): a full-buffer realloc+copy per pipe chunk for a ~25 MB base64 frame). Steady state ~1316MB -> ~804MB child. - Streamed SHA-256 diff baseline in runOne: store size+hash per staged file instead of holding every file's bytes for the whole run and re-reading the tree afterwards for a byte compare. - Lazy offline package loading: only PYODIDE_PRELOAD_PACKAGES (default numpy,pandas) is resident at init; other distribution packages load on first import from the local lock. matplotlib defaults to the headless Agg backend so savefig() works in the child (webagg needs the DOM). New env vars PYODIDE_PRELOAD_PACKAGES and PYODIDE_MAX_CHILD_RSS_BYTES (optional RSS-based child retirement after a high-water run). Adds the memory-analysis and streaming/staging follow-up plan under thoughts/. US-118: pyodide runtime memory optimization
The drain response is one length-prefixed frame carrying all created/ modified files, but the inbound per-frame cap defaulted to 64 MiB while the staging total cap is 128 MiB — so a drain above ~64 MiB was killed before the documented total was reachable. - Raise PYODIDE_MAX_FRAME_BYTES_DEFAULT 64 -> 192 MiB and PYODIDE_MAX_AGGREGATE_BYTES_DEFAULT 96 -> 256 MiB (clears 128 MiB x ~1.33 base64 expansion; aggregate stays >= frame; both below the protocol 256 MiB ceiling). - Wire PYODIDE_MAX_FRAME_BYTES / PYODIDE_MAX_AGGREGATE_BYTES env overrides in the manager constructor — they were documented but never read. Interim stopgap; the per-file streaming that removes the monolithic- frame coupling is tracked as Phase 1 in the follow-up plan. US-118: pyodide IPC framing
Owner
Changes from commits 10c8ef3 + 2bc9c79 (US-118)Two follow-up commits landed on this branch since last review. Summary below.
|
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
2 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.
Summary
pythoncapability withpython_runtime: "stdlib" | "pyodide" | null.stdlibkeeps the existing air-gapped CPython-emscriptenpython3;pyodideruns numpy/pandas/scipy/openpyxl fully offline inside an OS-isolated Deno subprocess;nullmeans no Python.pyodidesandbox → upload a CSV →python3 analyze.pyrunsimport pandas, writesout.xlsx, and the file is retrievable via the files API.Design Decisions
Full rationale in
thoughts/issue-118-pyodide-runtime/design.md. The decisions reviewers should understand:pyodideruntime spawns a Deno child with a scrubbed env (env:{}+DENO_NO_UPDATE_CHECK=1) under a committed deny-belt —--no-prompt --deny-net --deny-run --deny-write --deny-env --deny-ffi --deny-sys --deny-import --no-remote --no-npm --cached-only --no-config— with--allow-readscoped to the vendored asset dir only. A full Python→JS escape lands capability-less. A Deno/V8 runtime escape is explicitly out of the threat model (compensating controls: no secrets in the child, read-only asset scope); gVisor/seccomp is a recommended operator add-on, not a hard requirement. This reverses the prior C1 audit decision and needs maintainer/security sign-off.requestId, monotonicseq, message type, childgeneration) plus per-frame/aggregate size caps and exactly-one-response-per-request. The Node side kills the child on any malformed/oversized/duplicate/out-of-sequence/wrong-generation frame. Spike S2 finding A: escaped JS can write raw bytes to stdout (import("node:fs").writeSync(1, …)), but it cannot produce an accepted frame — the integrity secrets are never exposed to untrusted code. This is the load-bearing control.python_runtime TEXT CHECK IN ('stdlib','pyodide'), idempotent backfillWHERE python_runtime IS NULL(re-runs safely every boot under the no-tracking-table migration model). Mixed-version reads useCOALESCE(python_runtime, CASE WHEN python THEN 'stdlib' END); new writes dual-write the legacypythonboolean so old replicas keep working. Thepythoncolumn drop is deferred to a later release (N+1).MAX_RESIDENT_PYODIDEindependent ofSESSION_IDLE_MS, with atomic admission (lazy, post-semaphore) so concurrent cold starts can never exceed the cap.busy/startingworkers are never evictable.node:22-slimnon-root cannot write cgroupmemory.maxandRLIMIT_ASis unusable against V8's huge virtual reservation. The operator-set container memory limit is the only real guard; size it asMAX_RESIDENT_PYODIDE × per-proc ceiling+ Node baseline + DB/Redis pools.Changes
python_runtimefield through Zod validation,types.ts, Postgres dialect (read COALESCE + dual-write), migration0006_python_runtime.sql; create/list/get all echo it (fixes the prior single-GET omission); both SDKs (TS + Python), MCP tools, OpenAPI spec, and skill docs updated together (coordinated breaking release).src/pyodide-runner/(Deno-side harness + protocol),src/api/pyodide/(Node-sidePyodideSandboxmanager, IPC client, residency LRU),src/api/commands/pyodide-command.ts(thepython3/pythoncustom commands + diff-and-drain file staging).MAX_CONCURRENT_PYODIDEsemaphore (+ queue/wait-timeout env vars) routed independently of thestdlibpython semaphore; startup invariantMAX_RESIDENT_PYODIDE >= MAX_CONCURRENT_PYODIDE.micropip.freezetooling); Dockerfile + build scripts.How to Verify
pnpm typecheck && pnpm lint && pnpm test:unit(all green; 973 unit tests).pnpm test:integration— the escape/forgery suites run only when the vendored Deno + Pyodide assets are present (skip cleanly otherwise). Note: on hosts with a low Postgresmax_connections, run the multi-replica integration tests with--maxWorkers=2to avoid connection-slot exhaustion (environmental, not a code issue).escape.integration.test.ts+frame-forgery.integration.test.ts; confirm coverage of the C1-reversal surface and accept the single-layer threat model (Deno/V8 escape out of scope) for the deployment.python3 analyze.py→ downloadout.xlsx) through the product UI / LibreChat.References
thoughts/issue-118-pyodide-runtime/design.mdthoughts/issue-118-pyodide-runtime/plan.mdthoughts/issue-118-pyodide-runtime/spikes/S1-findings.md,S2-findings.md,S3-findings.mdNote for reviewers
Rebased onto current
upstream/main(tip72a8857) — the branch was previously ~4 commits behind (MCP static-header auth #120, dev-postgres setup #122, SDK 0.3.1 #121, ingest guard #116). Conflicts in the SDK version/changelog files were resolved by keeping both entries (the SDK CHANGELOGs now carry upstream's0.3.1ingest-guard entry and this PR's0.4.0python_runtimebreak); the ingest-guard feature insandbox.ts/sandbox.pyis preserved. The PR is now conflict-free (MERGEABLE); post-rebasetypecheck+lint+test:unit(1013 tests) all pass. History is intentionally one commit per phase (Phase 0 spikes → Phase 7 security gate) for independent review/revert.Summary by CodeRabbit
Breaking Changes
New Features
Documentation