Branch remote runner plugin fixed - #54
Open
proto-aiken-13 wants to merge 36 commits into
Open
Conversation
Set up the remote runner plugin, and associated directory and index files to be linked to the ev3 engine in py-slang
Updated the directory of the remote runner plugin
Modified the remoteRunnerPlugin repository structure and added the ev3Engine usage with the relevant test suite.
The updated directory structure is as follows:
plugins/
├── src/
│ ├── common/
│ │ └── test/
│ │ └── src/
│ │ └── index.ts # Added PySlangMessage type
│ ├── runner/
│ │ ├── remoteExecution/ # New package
│ │ │ ├── src/
│ │ │ │ └── index.ts # Entry point, exports remoteRunnerPlugin
│ │ │ ├── index.ts # Abstract remoteRunnerPlugin class
│ │ │ ├── jest.config.cjs
│ │ │ ├── manifest.json
│ │ │ ├── package.json
│ │ │ ├── rollup.config.mjs
│ │ │ └── tsconfig.json
│ │ └── test/
│ │ └── src/
│ │ ├── __tests__/
│ │ │ └── runner.test.ts # Tests engine-plugin connection
│ │ └── index.ts
└── lib/
└── build.ts # Fixed Windows yarn spawn issue
…onfigs - Replace jest/ts-jest/@types/jest with vitest in remoteExecution package - Add proper installable plugin fields (files, exports, publishConfig, peerDeps) - Remove spurious `abstract` from remoteRunnerPlugin (has no abstract methods) - Add `type` modifier to interface imports in remoteExecution/index.ts - Fix both rollup configs to use typescript() without __dirname (not valid in ESM) - Remove hardcoded Windows path from remoteExecution/tsconfig.json Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…/wasm-util Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Move @sourceacademy/wasm-util before @sourceacademy/web-* (alphabetical) - Merge commander@^2.19.0 range (nearley dep) - Add fast-levenshtein@^3.0.0 and fastest-levenshtein@^1.0.7 (py-slang dep) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
py-slang is imported from source (py-slang/src/...) and its internal imports don't use type-only syntax, which violates the repo's verbatimModuleSyntax:true setting. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lity py-slang/src uses BigInt (requires ES2020+) and imports untyped modules (moo, nearley, fast-levenshtein). Both remoteExecution and runner-test pull in py-slang source transitively, so both need these overrides. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Importing from py-slang/src/ pulls in untyped modules (moo, nearley) and namespace patterns that don't work under our strict tsconfig. noCheck:true compiles without type-checking, which is appropriate until py-slang exposes EV3Engine through its public API with proper types. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
runner-test/src/index.ts re-exported remoteRunnerPlugin via a relative path into another package's source, which rollup can't resolve when building the bundle. The test file imports it directly from the package so the re-export is redundant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Change inline type qualifiers to top-level import type (no-import-type-side-effects) - Add eslint-disable comments for required `as any` mock casts in tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Without .ts in the extensions list, @rollup/plugin-node-resolve could not find py-slang/src/engines/ev3/EV3Engine.ts and left it as an unresolved external. This caused the bundle to emit a bare require() pointing at the TypeScript source, which fails at test time with a SyntaxError. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@rollup/plugin-typescript only transforms files that are part of the TS program (files in tsconfig include). Without this, when nodeResolve finds EV3Engine.ts from node_modules, rollup's default acorn parser receives the TypeScript file and fails. Adding py-slang to include lets the TypeScript compiler process and transpile it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pileModule @rollup/plugin-typescript excludes node_modules from its TypeScript program, so py-slang's TypeScript source files (EV3Engine.ts, etc.) never get transformed — rollup's acorn parser receives raw TypeScript and fails. Add a lightweight inline plugin that calls ts.transpileModule() on any .ts file from node_modules before the main TypeScript plugin runs. This strips TypeScript syntax without needing a full type-checking program. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
py-slang depends on moo and nearley, which are CommonJS UMD modules. When rollup bundles them as ESM it can't resolve their default exports without the commonjs interop plugin. Adding commonjs() before our TypeScript transform plugin resolves the 'default is not exported' error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ntry Yarn lockfile workspace entry must mirror package.json; adding the dep to package.json without updating yarn.lock triggers YN0028. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Bundling py-slang TypeScript source causes CJS interop and circular dependency issues at runtime. Since py-slang is a listed dependency (installed alongside the plugin), it's correct to keep it external. At test time, vitest's server.deps.inline transforms py-slang with esbuild, handling TypeScript source and CJS interop cleanly without the issues rollup's commonjs plugin introduces. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The pre-built dist/index.mjs has py-slang as an external import, and Node.js can't load TypeScript source files natively. Using vi.mock to stub EV3Engine tests the plugin's channel subscription behavior without needing to load py-slang's TypeScript at test time. Also fixes the debug FORCE_FAIL assertion and adds a meaningful result assertion. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nt rule Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a new conductor-compatible EV3 evaluator to the remoteExecution runner package: - Ev3Evaluator.ts: BasicEvaluator subclass that calls EV3Engine from py-slang to compile Python code to SVML, returning the result via conductor's sendResult channel back to the frontend host - entry.ts: bundle entry point that initialises the Ev3Evaluator via conductor's initialise utility - rollup.config.mjs: updated to include a second build target that bundles entry.ts into a self-contained iife output (ev3-pyslang.js) for use as a Web Worker in the frontend
…g dependency refactor(remoteExecution): make plugin transport-only, remove py-slang coupling The remoteExecution plugin previously imported EV3Engine (py-slang) directly and executed code inside the plugin itself, via a bespoke run/result channel protocol. This inverted the intended dependency direction: a runner-side transport plugin should be reusable by any evaluator, the same way CseMachinePlugin is reusable across js-slang/py-slang without depending on either. This commit: - Rewrites `RemoteExecutionPlugin` (formerly `remoteRunnerPlugin`) as a transport-only IPlugin: it exposes `sendConnectionStatus(status)` and forwards it to the host over CHANNEL_ID. It no longer imports EV3Engine, parses/compiles/executes code, or knows py-slang exists. - Removes execution-result message handling from this package; evaluators are responsible for their own execution and for calling sendConnectionStatus when connection state changes. - Removes the py-slang dependency from package.json. - Updates the package's public index.ts to export RemoteExecutionPlugin and connection-status message types only. No changes to conduit/channel semantics — this plugin registers and behaves like any other IPlugin (see CseMachinePlugin for the reference pattern).
remoteRunnerPlugin previously imported py-slang's EV3Engine directly and
executed code inside this package, using a bespoke {type:'run'}/{type:'result'}
channel protocol. This inverted the intended dependency direction for a
runner-side transport plugin - compare CseMachinePlugin, which has zero
knowledge of js-slang/py-slang and only forwards snapshots the evaluator
gives it. A generic transport plugin package should never need an evaluator
as a dependency.
Renames remoteRunnerPlugin -> RemoteExecutionPlugin and narrows its
responsibility to transport only: it exposes sendConnectionStatus(status)
and forwards it over CHANNEL_ID to the host. It no longer imports EV3Engine,
parses/compiles/executes code, or knows py-slang exists.
Removes src/entry.ts and its rollup build target. This package no longer
builds a standalone EV3 worker bundle (dist/ev3-remote-runner.js) - worker
composition for EV3 execution now lives entirely in py-slang
(Ev3ExecutionPlugin + engines/ev3/entry.ts), the only place that can
legitimately depend on EV3Engine.
Removes py-slang from this package's dependencies entirely, and updates
the outer index.ts to export RemoteExecutionPlugin and its connection
status types instead of the old plugin/protocol.
…proto-aiken-13/plugins into branch-remoteRunnerPlugin-fixed
|
![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 refactors the remote execution runner by introducing the RemoteExecutionPlugin and updating the Rollup configuration to support a worker bundle. However, several issues were identified: the old remoteRunnerPlugin class and its py-slang import should be removed from index.ts to prevent compilation errors, the imported @sourceacademy/common-remote-execution package is missing from package.json dependencies, and there is a contradiction regarding the addition of the worker bundle target in rollup.config.mjs which may lead to build failures if the entry file is missing.
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.
…ution package, fix any-type lint, port stale test off removed remoteRunnerPlugin - Creates @sourceacademy/common-remote-execution (mirrors common-test's shape) exporting CHANNEL_ID/RUNNER_ID/WEB_ID, MESSAGE_TYPE_CONNECTION_STATUS, and the ConnectionStatus/ConnectionStatusMessage types that RemoteExecutionPlugin already imported but that never actually existed - this is what CI's "has no exported member" / "dependency not declared" failures were about. - Fixes the ESLint no-explicit-any violation in RemoteExecutionPlugin's constructor by using the already-correctly-typed IChannel<ConnectionStatusMessage> instead of IChannel<any>. - Rewrites runner.test.ts, which still tested the old remoteRunnerPlugin (mocking py-slang's EV3Engine directly) that this PR's whole point was to remove. Now tests RemoteExecutionPlugin's actual transport-only behavior. - Drops runner-test's leftover `"py-slang": "github:proto-aiken-13/py-slang#branch-ev3-engine"` dependency, now dead weight since the test no longer touches py-slang. ConnectionStatus is typed as 'CONNECTING' | 'CONNECTED' | 'FAILED' to match the existing DeviceConnection status union in frontend's remoteExecution feature, so a future web-side consumer composes cleanly with it. Verified: yarn build (full monorepo), yarn test (55/55 passing), and eslint on all touched files all clean. The one remaining lint failure (SubstVisualizer.tsx) predates this branch and is unrelated to remote execution. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fixed-54 # Conflicts: # src/runner/cse-machine/src/__tests__/runner.test.ts # src/runner/remoteExecution/package.json # yarn.lock
Akshay-2007-1
marked this pull request as ready for review
September 7, 2026 08:19
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
remoteRunnerPlugin was doing two unrelated things: forwarding status-like data over a channel (legitimate transport-plugin work), and directly constructing and executing py-slang's EV3Engine (execution work, which belongs to the evaluator). That coupling meant this package could never actually be used without py-slang installed, and it made the package's stated purpose - a generic, reusable runner-side plugin - false in practice.
This PR narrows the plugin to transport only, following the same shape as CseMachinePlugin: pure channel forwarding, with the evaluator (in py-slang) deciding when and what to send.
What changed
src/index.ts: remoteRunnerPlugin → RemoteExecutionPlugin. Removed the EV3Engine import and all code-execution message handling. Added sendConnectionStatus(status: ConnectionStatus): void, mirroring CseMachinePlugin.sendSnapshots.
Removed the {type:'run', code} → {type:'result', output} protocol from this package entirely — that protocol now lives in py-slang's Ev3ExecutionPlugin, which owns EV3Engine.
Removed src/entry.ts and its corresponding rollup.config.mjs build target (dist/ev3-remote-runner.js). This package no longer produces a worker bundle at all.
package.json: removed py-slang as a dependency.
Public exports updated: RemoteExecutionPlugin, ConnectionStatus, ConnectionStatusMessage. Removed the old PySlangMessage-shaped exports.