feat(conductor): Source/JS CSE machine evaluators and plugin - #2004
Closed
Akshay-2007-1 wants to merge 10 commits into
Closed
Akshay-2007-1 wants to merge 10 commits into
Akshay-2007-1 wants to merge 10 commits into
Conversation
…lugin Mirrors the local working state from the upstream js-slang checkout onto a forkable branch so it can be turned into PRs. - src/conductor: SourceEvaluator1 + JSCseEvaluator3/4 conductor evaluators, initialise/evaluator entrypoints, and JsCseMachinePlugin (sends CseSnapshots over the __cse channel) - JSCseEvaluator: synchronous for...of snapshot collection with step-limit cap (via /__cse_config__), and currentLine (context.runtime.nodes[0] line) per snapshot for the editor's blue current-line highlight - build tooling: rollup.config.evaluator.mjs + scripts/build-evaluators.mjs (+ evaluator shims) to bundle the IIFE worker evaluators - package.json: @sourceacademy/conductor via portal:../conductor (local) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ackages - Remove JsCseMachinePlugin.ts; types now come from @sourceacademy/common-cse-machine - JSCseEvaluator.ts imports CseMachinePlugin from @sourceacademy/runner-cse-machine - All local CseSnapshot / CseSerialized* type definitions removed (canonical in common pkg) - Uses cast for IPlugin id/name compat until conductor PR merges - Add portal: deps for common-cse-machine and runner-cse-machine in package.json Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces standalone conductor evaluators for the CSE machine (chapters 3 and 4) and Source 1, along with a Rollup build configuration and script to bundle them. The review feedback focuses on improving robustness and cross-platform compatibility. Key recommendations include handling destructuring and default values in closure parameter serialization, adding defensive checks for env.heap and JSON parsing of configRaw, wrapping runFilesInContext in a try...catch block to prevent worker crashes, and executing Rollup via its JS entry point to ensure the build script runs successfully on Windows.
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.
Comment on lines
+23
to
+27
| function serializeValue(v: Value, depth = 0): CseSerializedValue { | ||
| if (v instanceof Closure) { | ||
| const paramNames = v.node.params.map((p: Identifier | RestElement) => | ||
| p.type === 'RestElement' ? '...' + (p.argument as Identifier).name : p.name, | ||
| ); |
Contributor
There was a problem hiding this comment.
When serializing closure parameters, the current implementation assumes parameters are only Identifier or RestElement nodes. However, in JavaScript/Source, parameters can also be AssignmentPattern (for default values), ObjectPattern, or ArrayPattern (for destructuring). If these patterns are encountered, p.name will be undefined, leading to incorrect serialization or potential visualizer issues. We should use a robust helper function to extract parameter names recursively.
function getParamName(p: any): string {
if (!p) return '?';
switch (p.type) {
case 'Identifier':
return p.name;
case 'RestElement':
return '...' + getParamName(p.argument);
case 'AssignmentPattern':
return getParamName(p.left);
case 'ObjectPattern':
return '{...}';
case 'ArrayPattern':
return '[...]';
default:
return '?';
}
}
function serializeValue(v: Value, depth = 0): CseSerializedValue {
if (v instanceof Closure) {
const paramNames = v.node.params.map(getParamName);…tructor in SourceEvaluator1 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- SourceEvaluator1: wrap evaluateChunk in try/catch, use parseError for errors - JSCseEvaluator: guard env.heap before calling getHeap() - JSCseEvaluator: isolate JSON.parse of cse config with graceful fallback to 1000 - build-evaluators: use rollup JS entry point for cross-platform compatibility Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Coverage Report for CI Build 27940446406Coverage remained the same at 78.53%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
This was referenced Sep 21, 2026
martin-henz
added a commit
that referenced
this pull request
Sep 21, 2026
…ance sweep (#2058) * feat(conductor): Source §1-4 evaluators on the transpiler First slice of the Conductor migration (#2051): the workhorse evaluator path only. No CSE machine, no stepper, no modules, no non-default variants — each of those gets its own issue. SourceEvaluator1..4 extend BasicEvaluator and implement evaluateChunk alone. As of conductor 0.8.3 the base class already owns the REPL loop and emits EVAL_READY/RUNNING/ERROR around each chunk, and calls sendResult with whatever evaluateChunk returns — so the evaluator returns the program's value rather than sending it, or every run would appear twice in the host's REPL. display()/prompt()/alert() are wired through externalBuiltIns to sendOutput/tryRequestInput. tryRequestInput is conductor's synchronous counterpart to requestInput, which matches js-slang's synchronous CustomBuiltIns.prompt exactly, so no async plumbing is needed here. Without this wiring js-slang's default rawDisplay applies, which is console.log — inside a Worker, i.e. output that never reaches the REPL. Errors are sent one per SourceError rather than as a single bulk parseError string, so each keeps its own position: EvaluatorError takes (message, line, column, fileName) and renders the location itself. Warnings go to stdout, since the host renders the error channel in red. executionMethod is pinned to 'native'. The default 'auto' silently falls back to the CSE machine on a debugger; statement or verbose errors, which this evaluator does not support. Note this is the ExecutionMethod named 'native' (compile to JS and eval, as opposed to interpreting), not Variant.NATIVE — the variant stays DEFAULT, so the full Source instrumentation applies: operator type checks, boolean checks, array guards, proper tail calls and the execution timeout. Build notes: - @ts-morph/bootstrap is aliased to a throwing stub. It is reached only through FullTSParser (Chapter.FULL_TS), which these evaluators never select, and it drags in the whole TypeScript compiler. Stubbing the package rather than making the import dynamic avoids turning the synchronous parse() async. Result: 1.4MB bundles, no compiler. - require('acorn-class-fields') in typeParser.ts is rewritten to an ESM import at build time. @rollup/plugin-commonjs leaves it alone even with transformMixedEsModules, and in an IIFE bundle `require` is a free variable, so the worker would have died with a ReferenceError at load, before conductor could report it. The source cannot simply use an import: the package is CJS with no .default, and this project compiles with esModuleInterop off. Approach borrowed from #2004. - src/conductor is excluded from tsconfig.prod.json. Conductor is ESM-only and build:slang emits CommonJS, so it must not reach dist/. Verified in the frontend against a local language-directory: display output, red errors with positions, chapter restrictions, REPL context persistence across chunks, and 200k-deep tail recursion returning rather than overflowing the stack. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: spec-driven operator conformance sweep Adds a table-driven conformance suite for Source's operator typing, transcribed from the language specifications: docs/specs/source_typing.tex - §1/§2 docs/specs/source_typing_3.tex - §3/§4 The .tex files stay the human source of truth; operatorSpec.ts is a transcription to be updated in the same PR as any table change. The .tex is deliberately not parsed at test time — a LaTeX parser in the harness buys a guarantee that only looks stronger than a reviewed transcription, and costs a fragile parser. Same arrangement as py-slang's src/tests/operator-spec.ts. The tables do not state the type universe, but the prose above each does: source_typing.tex opens "Expressions evaluate to numbers, boolean values, strings or function values" and source_typing_3.tex adds arrays. That is encoded as universeForChapter. The sweep covers every operator x left-type x right-type combination per chapter. Each either matches the prescribed result type or errors — the specs' own wording for a combination no row admits is that implementations must "generate an error message" — so nothing can fall silently between the two. &&/|| assert only admittance, since bool x any -> any fixes no result type; their left operand is chosen so the right is actually evaluated rather than short-circuited away. This is not covered by src/utils/__tests__/rttc.test.ts, which unit-tests checkBinaryExpression in isolation. Whether an engine calls it — for every operator, on both operands, at the right chapter — is a different claim, and it is the one that breaks: transformUnaryAndBinaryOperationsToFunctionCalls is skipped entirely by transpileToFullJS, so an engine can pass every rttc unit test while enforcing none of this table. Runs against the transpiler, the engine the Conductor evaluators use. As further engines arrive (the CSE machine first), each should get its own sweep taking expected values by running this engine fresh rather than reading the table again, pinning it to the reference implementation instead of a second transcription that could drift. 1002 tests. Checked to be non-vacuous by mutation: claiming === is any x any at §1/§2 fails 56 cases; claiming string + string -> number fails 4. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(stepper): say "alternative", not "alternate", in the conditional explanation The conditional-expression stepper explanation read "condition is false, alternate evaluated". "Alternative" is the noun; "alternate" as a noun means a stand-in or substitute, and as an adjective means "every other". SICP JS calls this branch the alternative throughout. The ESTree field is still spelled `alternate` and stays that way — that is the AST property name, not user-facing text. Snapshots regenerated: 379 occurrences of the phrase, wording only, no other change. py-slang made the same correction in source-academy/py-slang#458. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(conductor): correct what entrypoint capture actually affects The comment claimed capturing the host's entrypoint name made reported errors "carry the right file". It does not: runFilesInContext defaults shouldAddFileName to `Object.keys(files).length > 1` (src/index.ts:258) and this evaluator always passes exactly one file, so the linker parses without `sourceFile`, error.location.source stays undefined, and toConductorError renders `1:3: ...` with no filename. That is the behaviour we want while there is only ever one file — js-slang's own default exists to keep a redundant filename off every single-file diagnostic — so the fix is to the comment, not the option. Noted inline that shouldAddFileName should become true at the same time this evaluator grows real multi-file support, when a filename starts disambiguating something. The entrypoint capture itself stays: it is the file map key and the entrypoint argument, which validateFilePath and the module preprocessor's resolution both key off. Found by Codex review on #2058. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: build the Conductor evaluator bundles `yarn build` is tsc for the published library and does not touch the evaluator bundles, and no test exercises them either — so a change that breaks browser bundling would land green and only surface when someone actually ran a worker. Both bundling hazards this slice already hit are exactly of that kind and invisible to tsc and to vitest: a newly reachable Node builtin (@ts-morph/bootstrap dragging in the TypeScript compiler) and an inline require() that rollup cannot rewrite (acorn-class-fields), the latter failing at worker load with a ReferenceError before conductor has any channel to report it on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 21, 2026
martin-henz
added a commit
that referenced
this pull request
Sep 21, 2026
* feat(conductor): CSE machine evaluators for Source §3-§4 Implements #2060. SourceCseEvaluator3/4 drive js-slang's CSE machine and ship one CseSnapshot per step to the host's CSE tab. §3/§4 only: the frontend gates the tab at chapter >= SOURCE_3 (ApplicationTypes.ts:245) and, under Conductor, shows it when the language offers an evaluator carrying EvaluatorCapability.CSE. The evaluator is hidden from the dropdown and auto-selected when the tab opens, like the stepper. The machine stays synchronous. In js-slang the CSE machine is the visualiser, not the workhorse, so unlike py-slang — whose CSE machine is its primary engine and is async throughout — nothing here needs to await the host. generateCSEMachineStateStream is a plain function* and is used as one. Only the serialiser is new: the plugin trio in source-academy/plugins is 264 LOC of pure transport, and the renderer stays in the frontend. For Source this is a round trip — serialise js-slang state, then have the host's CseSnapshotAdapter rebuild js-slang-shaped objects for the same renderer the non-conductor tab uses — so the adapter, not the protocol types, is the real contract. Three conventions it depends on, all easy to get silently wrong, are documented in serialize.ts: control and stash are top-first, InstrType is a string enum passed through untouched, and ENVIRONMENT instructions are matched on displayText rather than on instrType. Deliberately unlike #2004's serialiser: - arrays serialise to full depth, with cycle detection, rather than truncating at depth 2 (which silently flattens ordinary list programs) - steps are not deduplicated by rendered text: two steps can look identical while differing in the environments, and collapsing them desynchronises the step counter from the machine's own numbering - closures send the machine's own rendering as displayValue, so the stash reads identically to the non-conductor tab Two divergences are documented in SourceCseEvaluator.ts rather than papered over: the empty list (the adapter maps label 'null' to a Python None stand-in, so Source's null is labelled 'empty_list' and currently falls through to the string fallback), and the global frame (the adapter injects a sentinel binding that suits py-slang's empty global frame but is spurious for js-slang, whose builtins really do live there). The latter needs a side-by-side against the non-conductor tab to settle, and the deployment pins conductor.enable, so that is not yet done. Verified live: the CSE tab renders, steps and scrubs for a closure program over 24 steps. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(conductor): parity and cross-engine checks for the CSE evaluator Three suites, all aimed at the same bar: agreement with the engine being visualised, not "the tab shows something". collectSnapshots.test.ts compares the serialised sequence against a raw CSE machine run over the same program — step count, control depth per step, and stash contents — rather than snapshotting the serialiser's own output, which would only ever confirm it still does what it did. It caught a real divergence while being written: closures were sending a truncated name where the machine renders full source, fixed in the serialiser rather than by relaxing the assertion. Also covers full-depth array serialisation, cycle termination, builtins labelled distinctly from closures, the step limit, the absence of deduplication, and debugger; reported as a breakpoint step. operator-conformance-cse.test.ts sweeps the operator x type x type cross product through the CSE machine and requires it to match the transpiler's outcome, taking the expected value by running the transpiler fresh rather than reading docs/specs/source_typing*.tex a second time — pinning the two engines to each other instead of to two transcriptions that could drift while both still looked right. 610 cases; measured spread at §3 is 211 errors to 64 successes, so both branches are genuinely exercised. SourceEvaluator.test.ts gives the transpiler evaluator its first unit tests, against a stub of the IRunnerPlugin surface it actually touches: display output reaching the host, syntax errors on the error channel rather than stdout, errors not leaking into the next chunk, chunk-to- chunk context reuse, and the new debugger; hint — including that it points at the CSE evaluator only at §3+, where one exists, and that `debugger` inside a string does not trigger it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(conductor): warn that `debugger;` is ignored by the transpiler Both pre-existing behaviours were invisible. The legacy 'auto' execution method silently *switched engines* when it saw a debugger; statement (determineExecutionMethod), and slice 1's pinned 'native' silently ignores it. Neither tells the user anything. Now it emits a note on stdout — not the error channel, since the program is not wrong and the host renders errors in red — pointing at the CSE machine evaluator. Only at §3/§4, because that is the only place a CSE evaluator exists; below that it says the statement is ignored and stops there rather than sending the user somewhere absent from their dropdown. Detection walks the AST (mirroring determineExecutionMethod's own check) rather than matching source text, so `debugger` inside a string or a comment does not trigger it. A parse failure is ignored here; the real run reports it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(conductor): address Codex review on #2061 Five findings, all valid. 1. stepIndex collided with the initial snapshot. `steps` is incremented *before* each yield, so the generator's first yield reports 1 and `steps - 1` gave 0 — the same index as the step-0 snapshot pushed ahead of the loop. Every run therefore contained two snapshots numbered 0, and every recorded breakpoint step pointed one position behind the state it described. Now uses `steps`, which also makes stepIndex equal to the array position. 2. Editor gutter breakpoints were ignored. `breakpointLines` arrived in /__cse_config__ and was never read, so only literal `debugger;` statements were reported and the tab's breakpoint navigation could not stop anywhere the user had clicked. Now threaded into collectSnapshots, which reports any node starting on a breakpoint line, deduplicated by node identity so one breakpoint that stays on top across several steps is reported once. 3. Runtime errors were reported twice. handleRuntimeError both pushes onto context.errors and throws, so the catch sent the drained diagnostic and then the thrown value again — the second time as a generic message with no location. The fallback now fires only when the thrown value was not already drained. 4. Debugger detection polluted the shared context. The preliminary parse used to look for `debugger;` ran against this.context, appending its diagnostics there; runFilesInContext then parsed the same chunk again and appended the same ones, so an invalid chunk reported every error to the host twice. It now parses into a throwaway context. 5. The environment walk skipped arrays. An array carries the environment it was created in and may hold closures, and serializeValue emits both ids, so a frame reachable only through an array could be absent from `environments` and unresolvable by the host adapter. The walk now follows array environments and elements, with cycle protection. Regression tests for 1-4, each verified by mutation: reverting any of those four fixes fails 9 tests between them. 5 is covered only by an invariant check — every referenced frame id resolves within its snapshot — and the test says so plainly, because in the programs tried the frame stayed reachable through the call stack anyway, so it does not yet distinguish the array-following walk from one without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
martin-henz
marked this pull request as draft
September 21, 2026 14:24
Member
|
Closing as superseded — with thanks, this PR got the Conductor migration moving. Source now runs under Conductor via three slices that landed since:
Two things from here were carried forward directly, and both were the non-obvious parts: the rollup browser shims ( The evaluators are deployed at https://source-academy.github.io/js-slang/ and Source §1–§4 are back in the language directory. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds conductor-based evaluators for Source 1–4 and a JS CSE machine plugin, enabling the CSE machine visualiser to work with Source/JS programs via the Conductor framework.
New files
src/conductor/JSCseEvaluator.ts: ExtendsBasicEvaluatorto run Source/JS code through the CSE engine. Registers aCseMachinePlugin(from@sourceacademy/runner-cse-machine) and sends a full batch ofCseSnapshots to the host after each evaluation.src/conductor/SourceEvaluator1.ts: Thin evaluator for Source 1 (non-CSE chapters) via conductor.src/conductor/evaluator.ts/index.ts/initialise.ts: Entry points and exports for the conductor evaluator bundle.rollup.config.evaluator.mjs/scripts/build-evaluators.mjs: Build configuration for producing the Web Worker evaluator bundles.