Skip to content

Branch remote runner plugin fixed#54

Draft
proto-aiken-13 wants to merge 33 commits into
source-academy:mainfrom
proto-aiken-13:branch-remoteRunnerPlugin-fixed
Draft

Branch remote runner plugin fixed#54
proto-aiken-13 wants to merge 33 commits into
source-academy:mainfrom
proto-aiken-13:branch-remoteRunnerPlugin-fixed

Conversation

@proto-aiken-13

Copy link
Copy Markdown
Contributor

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.

proto-aiken-13 and others added 30 commits June 22, 2026 23:05
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.
@changeset-bot

changeset-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: f32084b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

💥 An error occurred when fetching the changed packages and changesets in this PR
Some errors occurred when validating the changesets config:
The package or glob expression "@sourceacademy/web-stepper" is specified in the `ignore` option but it is not found in the project. You may have misspelled the package name or provided an invalid glob expression. Note that glob expressions must be defined according to https://www.npmjs.com/package/micromatch

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/runner/remoteExecution/index.ts Outdated
import { CHANNEL_ID, RUNNER_ID, type PySlangMessage } from "@sourceacademy/common-test";
import type { IPlugin, IChannel, IConduit } from "@sourceacademy/conductor/conduit";
import { EV3Engine } from "py-slang/src/engines/ev3/EV3Engine";
import { RemoteExecutionPlugin } from "./src";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The file src/runner/remoteExecution/index.ts still contains the old remoteRunnerPlugin class and imports EV3Engine from py-slang. Since py-slang has been removed from package.json dependencies, this will cause compilation and runtime errors.

Since the new RemoteExecutionPlugin is implemented in src/runner/remoteExecution/src/index.ts, this file should be cleaned up to only export RemoteExecutionPlugin from ./src and the old class and imports should be removed.

Suggested Clean File Content:

export { RemoteExecutionPlugin } from "./src";

Comment on lines +1 to +38
import commonjs from "@rollup/plugin-commonjs";
import nodeResolve from "@rollup/plugin-node-resolve";
import terser from "@rollup/plugin-terser";
import typescript from "@rollup/plugin-typescript";
import esbuild from "rollup-plugin-esbuild";

/**
* @type {import('rollup').RollupOptions}
*/
export default {
input: "src/index.ts",
external: id => id.includes("py-slang"),
output: [
{
file: "dist/index.cjs",
format: "cjs",
export default [
// existing build (npm package output) — unchanged
{
input: "src/index.ts",
external: id => id.includes("py-slang"),
output: [
{ file: "dist/index.cjs", format: "cjs" },
{ file: "dist/index.mjs", format: "esm" },
],
plugins: [nodeResolve(), typescript(), terser()],
},
// EV3 remote runner worker bundle (used by frontend Web Worker)
{
input: "src/entry.ts",
output: {
file: "dist/ev3-remote-runner.js",
format: "iife",
},
{
file: "dist/index.mjs",
format: "esm",
},
],
plugins: [nodeResolve(), typescript(), terser()],
};
plugins: [
nodeResolve({
preferBuiltins: false,
exportConditions: ["import", "require", "default"],
extensions: [".ts", ".js"],
}),
commonjs(),
esbuild({
target: "es2020",
}),
terser(),
],
},
]; No newline at end of file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There is a contradiction between the PR description and the changes in rollup.config.mjs. The PR description states that src/entry.ts and its corresponding rollup build target (dist/ev3-remote-runner.js) have been removed. However, the changes in rollup.config.mjs actually add this target back.

If src/entry.ts has indeed been removed, the rollup build will fail with a 'Could not resolve entry module' error. Please remove this second build target and its unused imports (commonjs, esbuild) from rollup.config.mjs to align with the PR's objective.

import nodeResolve from "@rollup/plugin-node-resolve";
import terser from "@rollup/plugin-terser";
import typescript from "@rollup/plugin-typescript";

export default {
  input: "src/index.ts",
  external: id => id.includes("py-slang"),
  output: [
    {
      file: "dist/index.cjs",
      format: "cjs",
    },
    {
      file: "dist/index.mjs",
      format: "esm",
    },
  ],
  plugins: [nodeResolve(), typescript(), terser()],
};

Comment on lines 25 to 27
"peerDependencies": {
"@sourceacademy/conductor": ">=0.3.0"
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The package imports from @sourceacademy/common-remote-execution in src/runner/remoteExecution/src/index.ts, but this dependency is not declared in package.json (neither in dependencies nor peerDependencies). This will cause module resolution errors during build or runtime, especially in a monorepo environment with strict dependency resolution (like Yarn PnP). Please add @sourceacademy/common-remote-execution to peerDependencies.

  "peerDependencies": {
    "@sourceacademy/common-remote-execution": "workspace:*",
    "@sourceacademy/conductor": ">=0.3.0"
  },

Comment on lines +29 to +38
"@rollup/plugin-commonjs": "^29.0.3",
"@rollup/plugin-node-resolve": "^16.0.3",
"@rollup/plugin-terser": "^1.0.0",
"@rollup/plugin-typescript": "^12.3.0",
"@sourceacademy/common-test": "workspace:*",
"@sourceacademy/conductor": ">=0.3.0",
"@vitest/coverage-istanbul": "^4.1.9",
"rollup": "^4.60.2",
"esbuild": "^0.28.1",
"rollup": "^4.62.2",
"rollup-plugin-esbuild": "^6.2.1",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If the second rollup build target for the worker bundle is removed from rollup.config.mjs (as described in the PR description), these devDependencies (@rollup/plugin-commonjs, esbuild, and rollup-plugin-esbuild) are no longer needed and should be removed to keep the package clean.

Suggested change
"@rollup/plugin-commonjs": "^29.0.3",
"@rollup/plugin-node-resolve": "^16.0.3",
"@rollup/plugin-terser": "^1.0.0",
"@rollup/plugin-typescript": "^12.3.0",
"@sourceacademy/common-test": "workspace:*",
"@sourceacademy/conductor": ">=0.3.0",
"@vitest/coverage-istanbul": "^4.1.9",
"rollup": "^4.60.2",
"esbuild": "^0.28.1",
"rollup": "^4.62.2",
"rollup-plugin-esbuild": "^6.2.1",
"@rollup/plugin-node-resolve": "^16.0.3",
"@rollup/plugin-terser": "^1.0.0",
"@rollup/plugin-typescript": "^12.3.0",
"@sourceacademy/common-test": "workspace:*",
"@sourceacademy/conductor": ">=0.3.0",
"@vitest/coverage-istanbul": "^4.1.9",
"rollup": "^4.62.2",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants