Execute Codex exec snippets in a sandboxed QuickJS engine (replace the Tree-sitter analyzer)#291
Open
cboos wants to merge 9 commits into
Open
Execute Codex exec snippets in a sandboxed QuickJS engine (replace the Tree-sitter analyzer)#291cboos wants to merge 9 commits into
exec snippets in a sandboxed QuickJS engine (replace the Tree-sitter analyzer)#291cboos wants to merge 9 commits into
Conversation
Add codex_quickjs.py — the sandboxed-execution replacement for the
tree-sitter static analyzer (spec: work/codex-quickjs.md). Standalone so
far (consumers still import codex_javascript); wiring in is step 2.
- Engine: quickjs-ng behind _run_snippet(code)->report; per-snippet
memory(64MB)/time(2s) limits + pending-job bound; fail-closed to None.
- Instrumentation prelude (validated): recording `tools` proxy, `text()`,
ALL_TOOLS/setTimeout/exit/console stand-ins, no host callables. Sentinel
delimiter is U+E000 (private-use) not NUL, so JSON.stringify(r) emissions
keep their provenance markers instead of escaping them to \uXXXX.
- Mapper: sentinel R<i>[.path] parsing (raw, JSON-string, and JSON-object
forms) → JavaScriptToolBatch, reproducing result_indexes / prefixes /
synthetic wait / session markers / RESULT_N markers mode / object-key
projection; re-emission of one call keeps its output row.
- stubs/quickjs.pyi for the untyped quickjs-ng surface.
Status: 21/24 spec-by-example cases (test_codex_javascript) pass against
the new evaluator. Remaining 3: engine-bounded-loop test-flip (capability
gain), and the {name,...r} spread / explicit-result-field object
projections (need the prelude ownKeys refinement the spec anticipates).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Finish the sentinel→batch mapper so all recognized emission shapes decode
under real execution, and clear pyright strict on the new module.
- _project_result_object: recognize the two Codex result-object shapes,
split by ref-path — a whole-result BUNDLE ({presence:p, mail:m}: several
calls, one row, keyed per field) vs a single-call PROJECTION ({name,...r}
or {name, exit_code:r.exit_code, output:r.output}: collapse to the call's
`output` field + a leading-static JSON prefix). Reachable for both
JSON.stringify strings and direct dict text() values (out.forEach(text)).
- Prelude: magic Proxy gains ownKeys/getOwnPropertyDescriptor exposing a
single canonical `output` key, so {...r} spread degrades to a whole-result
projection (parity with the tree-sitter _mapped_result_object, not JS
key-set perfection). Target switched to an arrow fn so ownKeys need not
report a non-configurable `prototype`.
- cast() the json.loads / report boundaries for pyright strict (0 errors).
Validation: 23/24 analyze_javascript_tools spec cases pass against the new
evaluator. The 1 remaining is a sanctioned capability gain — an oversized
static loop under max_loop_iterations=2 now CAPTURES (engine time-limit
supersedes the fixed count); that rejection test flips in step 2 when the
consumers repoint. Dynamic (undefined-ids) loops still fail closed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Swap the two analyzer consumers (codex_tools.py, codex.py) from the tree-sitter codex_javascript to codex_quickjs, and migrate the spec tests. - _correlate: object_key now comes ONLY from an explicit object-emission ref (bundle / projection), never derived from a bare ref's property path. A plain text(r.output) output row is the raw result text, so its key stays None; deriving "output" made the downstream _object_batch_result try to JSON-project a plain string and drop the whole batch to raw fallback. Caught by the adversarial pairing tests, not the spec-by-example suite. - test_codex_javascript.py -> test_codex_quickjs.py, repointed at the new module. Dropped the three tree-sitter-internal parse_javascript tests (no analog under execution); the parser-error fail-closed guard becomes an engine-error fail-closed guard (monkeypatch _run_snippet to raise). - Sanctioned flip: the oversized-static-loop rejection splits into a dynamic-loop rejection (still fails closed) and a capability-gain capture (a static loop past the legacy max_loop_iterations cap now expands, bounded by the engine time limit instead of a fixed count). codex_javascript.py is now orphaned (no consumers, no dedicated tests) and is removed in step 4 after corpus parity. Full unit suite green (2573), pyright strict clean across providers/. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Execution changes the threat surface, so pin the fail-closed contract for
hostile transcript JS in test_codex_quickjs_adversarial.py (13 tests):
- Engine bounds: infinite loop (time-limit bounded, ~2s, not a Python hang),
allocation bomb, and unbounded recursion each fail closed with no host
RecursionError/crash.
- No host escape: process/require/fetch/globalThis.process are undefined;
a snippet reaching for require('fs') fails closed.
- Sandbox isolation: globalThis pollution in one snippet does not leak into
the next (fresh Context per run).
- Sentinel forgery (U+E000): a forged ref to a nonexistent call, a forged
sentinel spliced into a tool argument, and a literal provenance char in a
source string all fail closed; a hostile toJSON returning the sentinel does
not crash. Non-fabrication invariant asserted directly — no materialized
call input ever carries the raw sentinel (it routes real output rows, it is
never itself displayed).
- _build_batch forced to raise → still None (top-level guard is the boundary).
_S is bound from the module via getattr so a change to the sentinel can't
silently render the forgery guards vacuous. pyright strict + ruff clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…probe Parity is green (the migrated spec suite decodes every shape the tree-sitter analyzer did, plus the sanctioned capability gains, zero regressions), so retire the old path: - Remove claude_code_log/providers/codex_javascript.py (orphaned since step 2 — no consumers, no tests). - Drop the tree-sitter / tree-sitter-javascript dependencies from pyproject and the lockfile; quickjs-ng is now the only analyzer engine. - Add scripts/codex_snippet_coverage.py: a dev-only probe that runs the QuickJS analyzer over a LOCAL corpus of exec snippets (nothing private committed) and reports decoded-vs-fallback coverage. --compare runs the tree-sitter analyzer too while it exists and flags regressions; it now degrades gracefully to QuickJS-only. Self-check (no corpus) shows 0 regressions and 1 capability gain. Full unit suite green (2586). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Execution decodes snippet classes the tree-sitter analyzer had to reject. Pin them as synthetic spec-by-example fixtures (test_codex_quickjs_capabilities), each asserting the materialized tool input so the test is real only if the engine actually evaluated the expression: - concatenated / template-literal / array-join / reduce / repeat commands, - ternary flag selection and computed numeric arguments, - loop-built command batches, - conditional loop bodies (only taken branches are recorded). Plus a boundary marker: an ALL_TOOLS-driven pipeline still fails closed (a real registry view is deferred future work), documented so a later capability change is a deliberate, visible flip rather than a silent one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… bounds Follow-up hardening on the QuickJS analyzer (no design change): - Pin three load-bearing fail-closed guards that were previously unpinned (each materialized a WRONG batch when neutralized): a forged text ref to a setTimeout wait record -> None (is_wait leg; mutation-verified RED, producing a cross-attributed [wait, a] batch), throw-after-consistent-emissions -> None (errors leg), and a never-resolving run -> None (done leg). Each is paired with a control probe proving the None is attributable to the target guard, not to an upstream rejection. - Restore cap coverage: >64 KB source -> None, and a static 129-iteration loop (>128 real calls) -> None (128 -> batch boundary checked; the records-level and param-level caps are redundant defense in depth). - Generation-layer string cap: __safe now caps each recorded string at 64 KB with a truncation marker (mirroring its depth/breadth caps). A ~90-byte snippet could otherwise synthesize a multi-megabyte tool input into the generated HTML regardless of display folding. - The prelude sentinel and string cap are now genuinely spliced from the Python constants, so those constants are the single source of truth for both sides. - dev-docs/tools-coverage.md: rewrote the exec-JS section for the execution model (previously described as parsed with Tree-sitter, never executed). - quickjs-ng pinned <0.16 -- an engine executing untrusted input deserves an upper bound. - set_max_stack_size is now called (512 KB) and the missing-setter path fails closed instead of running untrusted code without a time limit. Full CI green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`_truncated_object_batch_result` recovers the final intact property from
truncated JSON by reverse-scanning every `"key":` occurrence and
`raw_decode`-ing from each -- O(matches) matches x O(N) per decode = O(N^2),
and worse on nested same-key inputs (`{"k":`*N) where each reversed match
re-parses the surviving subtree (~2.9s at 40 KB).
Fix -- two bounds, both kept:
- Cap the reverse raw_decode attempts to a small constant K=16. Only the FINAL
top-level property is ever recoverable (the closing-brace tail check), so
every earlier reversed match is a nested same-key occurrence inside that
final value (realistic depth 0-3); K=16 covers any real nesting with wide
margin, and exceeding it falls back to the existing truncation placeholder --
never wrong data, never a crash. This bounds the nested re-parse.
- Replace the `output[end:].lstrip()/.rstrip()` tail check (an O(N) slice per
match) with an anchored, non-slicing `_OUTER_BRACE_TAIL_RE.match(output, end)`
-- same semantics, short-circuits O(1) on the common failing char. This
bounds the flat-shape slice.
Together they turn the loop O(N) overall. Tests: a quadratic probe pinned via a
raw_decode call-count spy (20000 nested matches -> <=16 decodes, result None)
and a legitimate-recovery regression; the existing end-to-end
truncation-recovery tests remain green.
Full CI green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe Codex JavaScript analyzer now executes snippets in sandboxed QuickJS, records tool calls and emissions, correlates results, and fails closed on unsafe or ambiguous execution. Provider wiring, dependencies, typing, documentation, coverage tooling, tests, and bounded JSON recovery are updated. Codex analysis pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CodexProvider
participant QuickJSContext
participant ToolInstrumentation
participant ResultCorrelator
CodexProvider->>QuickJSContext: analyze exec snippet
QuickJSContext->>ToolInstrumentation: record tools calls and text emissions
ToolInstrumentation-->>QuickJSContext: emit serialized execution report
QuickJSContext-->>CodexProvider: return report or failure
CodexProvider->>ResultCorrelator: build JavaScriptToolBatch
ResultCorrelator-->>CodexProvider: correlated calls and output rows
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@scripts/codex_snippet_coverage.py`:
- Around line 54-58: Update the path collection loop in the script’s main
input-processing flow to expand glob-pattern arguments before checking whether
each path is a file or directory, so quoted patterns such as sessions/**/*.jsonl
resolve matching JSONL files. Preserve existing directory recursion and
direct-file handling, and retain documented glob support.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4391f84e-41f6-4140-8566-527ea8da7882
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
claude_code_log/providers/codex.pyclaude_code_log/providers/codex_javascript.pyclaude_code_log/providers/codex_quickjs.pyclaude_code_log/providers/codex_tools.pydev-docs/tools-coverage.mdpyproject.tomlscripts/codex_snippet_coverage.pystubs/quickjs.pyitest/test_codex_adversarial.pytest/test_codex_quickjs.pytest/test_codex_quickjs_adversarial.pytest/test_codex_quickjs_capabilities.py
💤 Files with no reviewable changes (1)
- claude_code_log/providers/codex_javascript.py
The dev-only coverage probe advertised glob patterns (e.g. `sessions/**/*.jsonl`) but classified each argument only as a literal file or directory, so a quoted glob was neither and silently contributed zero snippets — indistinguishable from a real 0% run. Expand shell-style globs (recursive `**` supported) via glob.has_magic()/glob.glob(), and report on stderr any pattern that matches nothing or any path that is neither a file nor a directory, so an empty corpus is never mistaken for a real result. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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
Codex persists multi-tool orchestration as JavaScript inside a custom
execcall. This PR replaces the static Tree-sitter analyzer (#288) with real
sandboxed execution of the snippet in a QuickJS engine (
quickjs-ng), usinginstrumented
tools/text()stand-ins that record what the snippet actuallydid. Because arguments are captured after the snippet's own JS evaluated them,
expression-built values (string concat, templates,
.join(),reduce,ternaries, computed arguments, conditional branches) resolve for free — the
fragile static-expansion whitelist disappears.
Evolution
The
exec-JavaScript recognizer has moved through three stages:a whitelisted subset; transcript code was never executed.
recording is mapped back to a tool batch.
On a corpus of 2,265 unique real
execsnippets, execution decodes 99.96%(2,264 / 2,265; the single miss is genuinely invalid JS → correct raw fallback)
versus 91.1% for the Tree-sitter analyzer.
Safety model
Transcript JavaScript is untrusted input.
QuickJS interpreter only.
pending-job cap, a 64 KB source cap, a 128 expanded-call cap, and a per-string
materialization cap (64 KB). If the limit setters are unavailable the run fails
closed rather than executing unbounded.
JSON.stringifypasses through verbatim (a NUL would be escaped and lost).
never resolves, or a shape the mapper cannot correlate — fails closed to the
raw-script
ToolExecutionfallback, exactly as before.Adversarial coverage includes engine-class cases (infinite loops, allocation
bombs, deep recursion), host-escape probes, cross-snippet isolation, and
sentinel forgery — each pinned to the fail-closed contract, several
mutation-verified.
New capabilities
Shapes the static analyzer had to reject now decode: concatenated /
template-literal / array-join / reduce / repeat commands, ternary flag
selection, computed numeric arguments, loop-built command batches, and
conditional loop bodies (only taken branches recorded).
ALL_TOOLS-registrypipelines remain a deliberate fail-closed boundary (a registry view is future
work).
DoS hardening
Bounds the
_truncated_object_batch_resulttruncation-recovery loop, whichreverse-scanned every
"key":occurrence andraw_decoded from each —O(matches) × O(N) = O(N²), and worse on nested same-key inputs where each match
re-parsed the surviving subtree. Capped to a small constant number of reverse
attempts plus a non-slicing tail check, turning it O(N) overall while preserving
recovery of the final intact property.
Notes
tree-sitter/tree-sitter-javascriptdependencies are removed;
quickjs-ngis the only engine.main(which carries theRenderingDepthrename and itsfollow-up plugin fix); there is no overlap with those changes.
Testing
Full CI green: unit, TUI, browser, integration, and benchmark suites, plus
ruff,pyright(strict), andty.🤖 Generated with Claude Code
Summary by CodeRabbit
execJavaScript tool analysis via a sandboxed QuickJS engine, enabling more expression types, loops/conditionals, and asynchronous/result mapping behaviors.