refactor(node-repl)!: deliver the persistent Node REPL as a standalone MCP server - #9499
Conversation
github-actions
Bot
added
the
review/self-reported
E2E test reportEnvironment: macOS arm64, Node.js v22.23.1. The branch includes current Runtime and quality gates
Built-artifact behavior
Runtime asset identity
The source runtime, core build, root bundle, and SDK bundle copies are byte-identical for the applicable assets. Provider-dependent resultRegistry and permission-path checks passed for all three deferred tools. A minimal real model request did not reach a tool call because the provider returned HTTP 403 with sanitized code |
|
Thanks for the PR — this is a substantial piece of work, and it shows. Template: complete ✓ (all sections, bilingual, reviewer test plan included). Problem: this implements the feature requested in #9333 — a real, linked roadmap item (Stage 1 of the persistent-kernel Computer Use roadmap), not a hypothetical. One important caveat: that issue is still labeled Direction: the area itself is aligned — Computer Use is an active investment here (the 35 Size: core paths are touched. Production logic: 4,949 lines (4,768 in Approach: the implementation is internally focused (no drive-by changes, no unrelated churn), but two scope questions worth a maintainer's eye:
Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk paths (no shell/mcp/acp/sandbox/streaming-parser surfaces touched). Moving on to code review, but note: because this lands a 4,900+ production-line feature in maintainer-gated core while the parent issue's discussion is still open, final approval here requires a maintainer decision — the gate will not auto-approve regardless of code quality. 中文说明感谢贡献 —— 这是一个工作量很大、完成度也很高的 PR。 模板：完整 ✓（各节齐全、中英双语、含 reviewer 测试计划）。 问题：本 PR 实现的是 #9333 请求的功能 —— 一个真实关联的路线图事项（持久内核 Computer Use 路线图的 Stage 1），不是假设性需求。但有一个重要前提：该 issue 目前仍带有 方向：领域本身是对齐的 —— Computer Use 在本仓库是持续投入（35 个 规模：触及核心路径。生产逻辑 4,949 行（ 方案：实现本身是聚焦的（无顺手改动、无无关重构），但有两个值得维护者过目的范围问题：
风险：无升级风险信号 —— 变更文件均未命中与 revert 相关的高风险路径（未触及 shell/mcp/acp/sandbox/流式解析器等面）。 进入代码审查；但请注意：由于本 PR 在父 issue 讨论尚未结束时就向 maintainer-gated 的核心落地了 4,900+ 生产行功能，最终是否合入需要维护者决定 —— 无论代码质量如何，门禁都不会自动批准。 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI read the full diff against the base tree. My independent baseline for "persistent REPL substrate" would have been a single lazy tool + child process with a JSON-lines protocol, What stood out, concretely:
No critical blockers found in static review. That said: statically reading ~5,000 lines of new security-sensitive runtime is not proof that it behaves as claimed — that is exactly what the sandboxed-verification line below is for. Non-blocking observations: the trusted bridge (~1/5 of the production lines) has zero consumers until #9334, and Files changed (30 of 33 shown)
Execution flow for reviewers who want the short version: sequenceDiagram
participant P1 as Model tool call
participant P2 as NodeReplKernelManager
participant P3 as Kernel child process
participant P4 as VM cell
P1->>P2: exec with code and timeout
P2->>P2: tree-sitter cell transform
P2->>P3: spawn lazily if no live generation
P2->>P3: exec frame on the host-to-kernel pipe
P3->>P4: link with @prev bindings, then evaluate
P4-->>P3: console, write, image, result events
P3-->>P2: outputs plus execResult on the kernel-to-host pipe
P2-->>P1: ToolResult with text, images, metadata
Note over P2,P3: timeout, cancel, or crash kills the process tree and revokes the generation
Test evidence (the PR's own CI)Evidence carried here is the PR's own CI check results, fetched via the API — per the unattended-CI rules I did not build or run any PR code. The main unit suite on The author posted a detailed self-reported E2E report (macOS arm64: 108/108 core tests, 285 built-artifact checks, lifecycle and concurrency probes, npm dry-run with asset hashes). That is the author's claim, not independently verified evidence — and it was run on one OS, with the provider-driven path blocked by an HTTP 403 before any model response. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the behavioural claims: Not verified: Windows/Linux behavior (author tested macOS only), the provider-driven tool-call path (author blocked by HTTP 403), and real-scenario TUI usage (non-UI change; no tmux run driven). 中文说明代码审查我对照 base 分支通读了全部 diff。我对"持久 REPL 底座"的独立基线是：单个延迟工具 + 子进程 JSON-lines 协议、 具体要点：
**静态审查未发现阻断性问题。**但需要说明：静态阅读约 5,000 行新的安全敏感运行时，不等于证明其行为与声明一致 —— 这正是下方"沙箱验证"一行的用途。非阻断观察：可信桥（约占生产行数五分之一）在 #9334 之前没有任何使用者； （文件变更表与时序图见上文英文版，此处不重复。） 测试证据此处证据为 PR 自身 CI 的检查结果（经 API 获取）—— 按无人值守 CI 规则，我没有构建或运行任何 PR 代码。主单测 作者发布了一份详细的自述 E2E 报告（macOS arm64：108/108 核心测试、285 项构建产物检查、生命周期与并发探针、npm dry-run 资产哈希）。那是作者自述，不是独立核实的证据 —— 且只在一个操作系统上运行，供应商驱动路径在模型响应前被 HTTP 403 阻断。 沙箱验证可以解决行为性声明： 未验证：Windows/Linux 行为（作者仅在 macOS 测试）、供应商驱动的工具调用路径（作者被 HTTP 403 阻断）、真实场景 TUI 使用（非 UI 变更，未驱动 tmux）。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — the engineering is unusually strong for a 5,000-line fork PR, but this lands a maintainer-gated core feature while its parent issue is still waiting on maintainer sign-off, so it caps at a defer no matter how clean the review reads. Stepping back:
⏸️ Deferring to @yiliang114 @doudouOUC @wenshao — needs a human call on this one. Specifically:
No correctness blockers were found in static review, so this is a defer, not a request for changes: if the maintainers confirm the direction and the CI + 中文说明置信度：3/5 —— 以一个 5,000 行的 fork PR 而言工程质量非常出色，但它在父 issue 仍在等待维护者拍板时，向 maintainer-gated 的核心落地了一整个功能，因此无论审查看起来多干净，都只能封顶在"暂缓"。 退一步看：
⏸️ 转交 @yiliang114 @doudouOUC @wenshao —— 需要人工拍板。具体原因：
静态审查未发现正确性阻断，因此这是"暂缓"而非"请求修改"：如果维护者确认方向，且 CI 与 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) and the merge_group-only macOS/Windows test lanes were skipped on the PR; the integration suite did not run locally.
Not explored to full depth (tool budget reached): chunk 16: live vitest run of security-policy.test.ts (no node_modules in worktree or parent checkout; static verification only); chunk 5: did not execute kernel-manager.test.ts itself — the worktree has no node_modules and npm ci + npm run build for the monorepo exceeded the remaining tool…; chunk 8: actually executing the two test files (the worktree has no node_modules / dist ; npm ci + build + kernel-spawning run exceeds the review budget) — verificati…; chunk 10: running result-converter.test.ts via vitest (worktree has no node_modules; fresh install + build exceeds the review budget).
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) and the merge_group-only macOS/Windows test lanes were skipped on the PR; the integration suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 16：live vitest run of security-policy.test.ts (no node_modules in worktree or parent checkout; static verification only)；chunk 5：did not execute kernel-manager.test.ts itself — the worktree has no node_modules and npm ci + npm run build for the monorepo exceeded the remaining tool…；chunk 8：actually executing the two test files (the worktree has no node_modules / dist ; npm ci + build + kernel-spawning run exceeds the review budget) — verificati…；chunk 10：running result-converter.test.ts via vitest (worktree has no node_modules; fresh install + build exceeds the review budget)。
未审查：反向审计——在 3 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| function collectHoistedVarNames( | ||
| node: Parser.SyntaxNode, | ||
| names: Set<string>, | ||
| ): void { | ||
| if (VAR_SCOPE_BOUNDARY_TYPES.has(node.type)) return; | ||
| if (node.type === 'variable_declaration') { |
There was a problem hiding this comment.
[Critical] R1-37: var bindings declared in for-in/for-of/for-await-of headers are never collected — the pinned grammar emits a for_in_statement whose loop variable is a bare identifier/pattern with an anonymous kind token and no variable_declaration child — so such bindings are never exported, snapshotted, or carried, and redeclaring a carried name through such a loop hard-fails the cell. Probed with this PR's own grammar and the real kernel: cell 1 for (var x of [1,2,3]) {} commits no binding; cell 2 x; → ReferenceError (the Node REPL returns 3); and after committing fo2, for (var fo2 of [5]) {} fo2 fails at module link with "Identifier 'fo2' has already been declared" (the prelude let fo2 = prev['fo2'] collides with the cell's own module-scope var) — where Node returns 5. Classic for (var k = 0; …) works (that head does produce a variable_declaration). Fix: special-case for_in_statement in collectHoistedVarNames — take childForFieldName('left') and, when the anonymous kind keyword is var, collectPatternNames(left, names); add a persistence test and a redeclare-carried-name test.
中文说明
在 for-in/for-of/for-await-of 头部声明的 var 绑定完全不被收集——所用 grammar 对这类语句生成 for_in_statement，其循环变量是带匿名 kind token 的裸 identifier/pattern，没有 variable_declaration 子节点——因此这些绑定既不会被导出、快照，也不会被携带；而通过这类循环重声明一个已携带的名字会直接让 cell 失败。已用本 PR 自带的 grammar 和真实内核探针验证：cell 1 for (var x of [1,2,3]) {} 不提交任何绑定；cell 2 x; → ReferenceError（Node REPL 返回 3）；提交 fo2 后执行 for (var fo2 of [5]) {} fo2 在模块链接阶段报 "Identifier 'fo2' has already been declared"（前导的 let fo2 = prev['fo2'] 与 cell 自身的模块级 var 冲突）——而 Node 返回 5。经典 for (var k = 0; …) 正常（其头部确实生成 variable_declaration）。修复：在 collectHoistedVarNames 中特判 for_in_statement——取 childForFieldName('left')，当匿名 kind 关键字为 var 时 collectPatternNames(left, names)；补充持久化测试与重声明已携带名字的测试。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| for (const item of sourceItems) { | ||
| collectDeclarationNames(item, currentNames); | ||
| collectHoistedVarNames(item, currentNames); |
There was a problem hiding this comment.
[Critical] R1-3: A cell that throws before its declaring statement commits silently loses any value assigned to a hoisted var — statement-boundary snapshots only carry hoisted-var bindings after the declaring item is processed, and kernel.mjs's catch path wholesale-replaces bindings with the last snapshot. With no prior y, run y = 42; then throw new Error('boom'); then var y;: plain Node keeps y === 42 (var hoists, the assignment ran), but here the next cell's y; throws ReferenceError — silent REPL state loss diverging from Node semantics and from the tool's own "a failed cell keeps bindings committed before the failure" contract. Probe witness through the real kernel: after the failing cell getBindingNames() = [] and the next cell's y; → ReferenceError: y is not defined; the control without the throw keeps the value. Fix: compute the cell-wide hoisted-var set once before the item loop and initialize activeNames with carriedNames ∪ hoistedVars, so every snapshot — including the last one before a failure — carries hoisted-var bindings.
中文说明
在声明语句提交之前抛错的 cell 会静默丢失已赋值给 hoisted var 的值——语句边界快照只有在处理到声明项之后才携带 hoisted var 绑定，而 kernel.mjs 的 catch 分支会用最后一个快照整体替换绑定。在没有既有绑定 y 时执行 y = 42; / throw new Error('boom'); / var y;：原生 Node 保留 y === 42（var 提升且赋值已执行），但这里下一个 cell 的 y; 抛 ReferenceError——静默的 REPL 状态丢失，既背离 Node 语义，也违背工具自身"失败 cell 保留失败前已提交绑定"的契约。探针经真实内核验证：失败 cell 之后 getBindingNames() = []，下一 cell y; → ReferenceError: y is not defined；不抛错的对照保留该值。修复：在 item 循环前一次性计算整个 cell 的 hoisted-var 集合，并用 carriedNames ∪ hoistedVars 初始化 activeNames，使每个快照（包括失败前最后一个）都携带 hoisted var 绑定。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| const carriedNames = previousNames.filter( | ||
| (name) => !currentNames.has(name), | ||
| ); |
There was a problem hiding this comment.
[Critical] R1-21: Redeclaring an inherited binding with var silently loses the carried value — hoisted var names land in currentNames, so the @prev prelude carry is suppressed and reads before the var statement see undefined. Probe through the real kernel: cell 1 var x = 5;, cell 2 console.log(x); var x = x * 2; prints undefined and corrupts x to NaN; the Node REPL prints 5 and yields 10. The tool description itself recommends this pattern ("When rerunning code, use var or a block scope"), and no test pins the interaction (the redeclaration test uses const; the var-collection test passes previousBindingNames: []). Fix: for names whose only in-cell declaration is var, emit var name = previousNamespace["name"]; in the prelude (legal alongside the user's var), keeping let carry for names not redeclared; add a regression test with previousBindingNames: ['x'] and cell x; var x = 1;.
中文说明
用 var 重声明一个继承的绑定会静默丢失继承值——hoisted var 名进入 currentNames，导致 @prev 前导携带被抑制，var 语句之前的读取看到 undefined。经真实内核探针验证：cell 1 var x = 5;，cell 2 console.log(x); var x = x * 2; 输出 undefined 并把 x 损坏为 NaN；Node REPL 输出 5 并得到 10。工具描述本身就推荐这种写法（"重跑代码时使用 var 或块级作用域"），且没有测试固定该交互（重声明测试只用 const；var 收集测试传入 previousBindingNames: []）。修复：对 cell 内唯一声明为 var 的名字，在前导中生成 var name = previousNamespace["name"];（与用户的 var 合法共存），未重声明的名字保留 let 携带；补充 previousBindingNames: ['x'] + cell x; var x = 1; 的回归测试。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| typeof message.execId !== 'string' || | ||
| message.execId !== this.inflight?.execId || |
There was a problem hiding this comment.
[Critical] R1-52: The audit-frame validator treats any trusted-module load not attributed to the currently inflight exec as a fatal protocol violation, but the kernel legitimately emits audit frames outside an active exec: onTrustedModuleLoad sends {type:'audit', execId: currentExecId()} with no activeExec guard — the only kernel→host emission lacking one (emitText/emitImage/setResponseMeta/callHostCapability all guard). Probe with a hash-pinned trusted fixture: a cell commits bindings ['anchor','scheduled'] and schedules setTimeout(() => import('trusted-delayed'), 10); after the timer fires: kernel pid null, bindingNames: [], generation 2 — all session bindings silently wiped (debug-level warn only); adding the activeExec guard restores pid/bindings/generation. Trigger: a trusted package with allowModelImport: true — a supported configuration exercised by ~9 tests in this PR; the production registry ships empty in phase 1, so this is latent until trusted packages land, at which point the frame semantics are live. If a later exec is inflight when the frame lands, it fails with the misleading "The node_repl protocol failed: invalid audit frame". Fix: make late/out-of-exec audit frames non-fatal (log and drop when the execId does not match) and/or suppress them kernel-side when !activeExec — a bookkeeping frame must not be able to revoke a generation.
中文说明
audit 帧校验器把任何不能归属到当前 in-flight exec 的可信模块加载都当作致命协议违规，但内核完全可能在无活动 exec 时合法发出 audit 帧：onTrustedModuleLoad 发送 {type:'audit', execId: currentExecId()} 时没有 activeExec 守卫——这是唯一缺少该守卫的 kernel→host 发送点（emitText/emitImage/setResponseMeta/callHostCapability 都有）。用哈希固定的可信 fixture 探针验证：cell 提交绑定 ['anchor','scheduled'] 并调度 setTimeout(() => import('trusted-delayed'), 10)；定时器触发后：内核 pid 为 null、bindingNames: []、generation 变为 2——整个会话的绑定被静默清空（只有 debug 级警告）；补上 activeExec 守卫后 pid/绑定/generation 全部恢复。触发路径：allowModelImport: true 的可信包——本 PR 约 9 个测试使用的受支持配置；生产注册表在 phase 1 为空，因此在可信包落地前该问题是潜伏的，届时该帧语义即刻生效。若该帧到达时恰有后续 exec 在执行，该 exec 会以误导性的 "The node_repl protocol failed: invalid audit frame" 失败。修复：让迟到的/exec 外的 audit 帧不再致命（execId 不匹配时记录并丢弃），和/或在内核侧 !activeExec 时抑制发送——记账帧不应有能力吊销整个代际。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| function capChars(text: string, limit: number): string { | ||
| return text.length <= limit ? text : `${text.slice(0, limit)}…`; | ||
| } |
There was a problem hiding this comment.
[Critical] R1-25: All text truncation paths (capChars, addBudgetedText's slice) cut on UTF-16 code units and can emit a lone unpaired high surrogate into llmContent. Deterministic trigger: a cell console.log('🚀'.repeat(30000)) — the budget cut lands at index 39,945 (40,000 minus the 55-char notice), between the two halves of an emoji; probe: loneSurrogateFound: true at index 39944, code unit 0xd83d, in both the text-only and with-image shapes. Nothing repairs it downstream (convertToFunctionResponse passes the string through; finalizeToolResponses only re-slices over-budget slots), so the next model request serializes the unpaired surrogate as a \ud83d escape, which strict protobuf-JSON provider frontends reject (INVALID_ARGUMENT) while the result stays in history. The codebase already fixed this class elsewhere (sliceStartWithoutBrokenSurrogate/sliceEndWithoutBrokenSurrogate in tool-response-finalizer.ts; session-tracing.ts: "strict JSON parsers reject lone surrogates"). Fix: back off one unit when the cut index lands on a high surrogate (0xD800–0xDBFF), reusing those slicers; apply in capChars, addBudgetedText, and the display cap.
中文说明
所有文本截断路径（capChars、addBudgetedText 的切片）按 UTF-16 码元切割，可能向 llmContent 发射孤立的高位代理项。确定性触发：cell console.log('🚀'.repeat(30000))——预算切割点落在索引 39,945（40,000 减去 55 字符提示），正好在一个 emoji 的两个半区之间；探针：无图与带图两种形态下均在 39944 处发现孤立代理项（码元 0xd83d）。下游无任何修复（convertToFunctionResponse 原样传递；finalizeToolResponses 只重切超预算槽位），因此下一次模型请求会把该孤立代理项序列化为 \ud83d 转义，被严格的 protobuf-JSON 供应商前端拒绝（INVALID_ARGUMENT），而该结果仍留在历史中。代码库已在别处修复过此类问题（tool-response-finalizer.ts 的 sliceStartWithoutBrokenSurrogate/sliceEndWithoutBrokenSurrogate；session-tracing.ts 注释"严格的 JSON 解析器拒绝孤立代理项"）。修复：切割点落在高位代理项（0xD800–0xDBFF）时回退一个码元（复用上述切割器），并在 capChars、addBudgetedText 与显示上限处统一应用。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| getDescription(): string { | ||
| if (this.params.title) return this.params.title; | ||
| const firstLine = this.params.code.split('\n', 1)[0] ?? ''; |
There was a problem hiding this comment.
[Suggestion] R1-13: The interactive 'ask' confirmation for arbitrary code execution shows only the 80-char title or the first 64 characters of line 1, because NodeReplInvocation relies on the generic ToolInfoConfirmationDetails fallback whose prompt is getDescription() — a multi-line payload hides everything after line 1 from the approving user. The comparable execution tool sets the precedent the other way: ShellToolInvocation overrides getConfirmationDetails to surface the actual command. Override getConfirmationDetails here to present the full code (or a substantially larger bounded prefix) — combine with the type: 'exec' override needed for the AUTO_EDIT finding.
中文说明
任意代码执行的交互式 'ask' 确认只显示 80 字符标题或第 1 行的前 64 字符，因为 NodeReplInvocation 依赖通用的 ToolInfoConfirmationDetails 兜底（其 prompt 为 getDescription()）——多行载荷中第 1 行之后的内容对批准用户完全不可见。可比的执行工具恰恰示范了相反做法：ShellToolInvocation 覆写 getConfirmationDetails 以展示实际命令。请在这里覆写 getConfirmationDetails 呈现完整代码（或显著更大的有限前缀）——并与 AUTO_EDIT 发现所需的 type: 'exec' 覆写合并实现。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| return errorResult( | ||
| `node_repl_add_node_module_dir failed: ${ | ||
| e instanceof Error ? e.message : String(e) | ||
| }`, | ||
| ToolErrorType.INVALID_TOOL_PARAMS, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-62: NodeReplAddNodeModuleDirInvocation.execute() classifies every addModuleRoot failure — including kernel-side failures — as INVALID_TOOL_PARAMS, unlike its siblings NodeReplInvocation/NodeReplResetInvocation, which map runtime failures to EXECUTION_FAILED. addModuleRoot runs sendAddRoot when a kernel is live; on a registration timeout (ADD_ROOT_TIMEOUT_MS = 10_000, e.g. a kernel stalled behind a heavy serialized exec on operationChain) or any protocol/child error, sendAddRoot also invalidates the kernel — yet the catch-all reports invalid_tool_params even though the path was valid and approved. The model's natural response to an invalid-params error is to retry with mutated parameters, re-invoking the tool with different paths and re-triggering 'ask' approval prompts for a transient execution failure. Split the catch: keep INVALID_TOOL_PARAMS for validation rejections (the two "canonical target changed" errors and policy validateModuleRoot throws), return ToolErrorType.EXECUTION_FAILED for the rest.
中文说明
NodeReplAddNodeModuleDirInvocation.execute() 把 addModuleRoot 的一切失败——包括内核侧失败——都归类为 INVALID_TOOL_PARAMS，而同级的 NodeReplInvocation/NodeReplResetInvocation 对运行时失败映射 EXECUTION_FAILED。addModuleRoot 在内核存活时运行 sendAddRoot；注册超时（ADD_ROOT_TIMEOUT_MS = 10_000，例如内核被 operationChain 上排队的重 exec 拖住）或任何协议/子进程错误时，sendAddRoot 还会使内核失效——而 catch-all 却在路径有效且已获批准的情况下报 invalid_tool_params。模型对 invalid-params 的自然反应是换参数重试，会用不同路径反复调用工具，为一次瞬时执行失败反复触发 'ask' 审批弹窗。请拆分 catch：校验拒绝（两个"canonical target changed"错误与策略 validateModuleRoot 抛错）保留 INVALID_TOOL_PARAMS，其余返回 ToolErrorType.EXECUTION_FAILED。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| expect(packageScript).toContain("'node-repl-runtime/kernel.mjs'"); | ||
| expect(packageScript).toContain("'node-repl-runtime/module-loader.mjs'"); |
There was a problem hiding this comment.
[Suggestion] R1-18: The assertions this diff adds live outside every npm workspace, so no workspace test script ever collects this file — verification relies solely on the root test:scripts script chained from test:ci/test:release. npm test (root) runs npm run test --workspaces --if-present, so any flow gating on plain npm test ships these packaging-script regressions untested. Confirm test:scripts actually runs in this PR's CI check set; optionally wire it into the default test script.
中文说明
本 diff 新增的断言不在任何 npm workspace 内，因此没有 workspace 的 test 脚本会收集该文件——验证只依赖根级 test:scripts 脚本（由 test:ci/test:release 串起）。npm test（根级）运行 npm run test --workspaces --if-present，所以任何以普通 npm test 为门槛的流程都会在这些打包脚本回归未被测试的情况下放行。请确认本 PR 的 CI 检查组确实运行 test:scripts；可选将其并入默认 test 脚本。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| ).toBe(true); | ||
| }); | ||
|
|
||
| it('copies node_repl runtime modules into the bundle asset directory', () => { |
There was a problem hiding this comment.
[Suggestion] R1-19: Same collection gap as install-script.test.js: the new asset-copy tests for the node_repl runtime modules sit outside every npm workspace and are only collected by root test:scripts. A regression in scripts/copy_bundle_assets.js/copy_files.js that drops kernel.mjs/module-loader.mjs/the tree-sitter wasm from bundle assets is invisible to every workspace-scoped test command — including this review's seven-suite run; only npm run test:scripts via test:ci/test:release would catch it. Confirm test:scripts is in this PR's CI check set.
中文说明
与 install-script.test.js 相同的收集缺口：node_repl 运行时模块的新资产复制测试不在任何 npm workspace 内，只被根级 test:scripts 收集。scripts/copy_bundle_assets.js/copy_files.js 中任何丢掉 kernel.mjs/module-loader.mjs/tree-sitter wasm 的回归对所有按 workspace 划分的测试命令都不可见——包括本次评审运行的七个套件；只有经 test:ci/test:release 的 npm run test:scripts 才能捕获。请确认 test:scripts 在本 PR 的 CI 检查组中。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| it('copies only node_repl runtime mjs files into per-package dist', () => { | ||
| const rootDir = createFixtureRoot(); | ||
| writeFile( | ||
| rootDir, | ||
| 'packages/core/src/tools/node-repl/runtime/kernel.mjs', |
There was a problem hiding this comment.
[Suggestion] R1-48: The only test pinning copy_files.js's per-package .mjs copy fixtures and asserts just kernel.mjs; module-loader.mjs — the second runtime file, statically imported by kernel.mjs at spawn — is never pinned in the per-package dist/src layout (the exact-readdirSync triple pin covers only the bundle layout produced by copy_bundle_assets.js). A future edit replacing the extension-based isNodeReplRuntime rule with an explicit filename list that omits module-loader.mjs leaves every test green; resolveKernelPath() probes kernel.mjs only, so the kernel spawns and dies with ERR_MODULE_NOT_FOUND at kernel.mjs's import { createModuleLoader } from './module-loader.mjs'; the first time node_repl runs from a tsc-built layout. Also write module-loader.mjs in the fixture and assert the copied runtime directory exactly (['kernel.mjs', 'module-loader.mjs', 'tree-sitter-javascript.wasm']).
中文说明
唯一固定 copy_files.js 按包 .mjs 复制的测试只 fixture 并断言 kernel.mjs；module-loader.mjs——第二个运行时文件、kernel.mjs 启动时静态导入——在按包 dist/src 布局中从未被固定（精确 readdirSync 三元组固定只覆盖 copy_bundle_assets.js 产出的 bundle 布局）。未来若把基于扩展名的 isNodeReplRuntime 规则替换为显式文件名列表且遗漏 module-loader.mjs，所有测试仍全绿；resolveKernelPath() 只探测 kernel.mjs，因此内核照常启动，并在 tsc 构建布局首次运行 node_repl 时死于 kernel.mjs 的 import { createModuleLoader } from './module-loader.mjs';（ERR_MODULE_NOT_FOUND）。请在 fixture 中也写入 module-loader.mjs，并精确断言复制出的运行时目录（['kernel.mjs', 'module-loader.mjs', 'tree-sitter-javascript.wasm']）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) and the merge_group-only macOS/Windows test lanes were skipped in CI and the integration suite did not run locally; packages/desktop-shell suites (negated workspace) never ran.
Not explored to full depth (tool budget reached): chunk 11: executed the test file ( npx vitest run src/tools/node-repl/result-converter.test.ts ) — the review worktree has no node_modules , so this fell back to static ….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 3.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/tools/node-repl/cell-transform.ts:574 — [review] resetNodeReplCellParserForTesting exported but never…packages/core/src/tools/node-repl/kernel-manager.ts:346 — [review] Pre-write cancellation destroys a healthy warm kernel…packages/core/src/tools/node-repl/kernel-manager.ts:565 — [review] Raw child stdout/stderr path has zero test coverage…packages/core/src/tools/node-repl/kernel-manager.ts:716 — [review] Audit-frame version format policy bricks a trusted package…packages/core/src/tools/node-repl/kernel-manager.ts:855 — [review] Raw stream chunks decoded per-chunk: multi-byte splits…packages/core/src/tools/node-repl/runtime/kernel.mjs:292 — [review] data-URL grammar rejects RFC 2397 parameterized forms;…packages/core/src/tools/node-repl/runtime/module-loader.mjs:430 — [review] ~35 lines of unreachable code in resolveBare after the…packages/core/src/tools/node-repl/runtime/module-loader.mjs:466 — [review] Corrupt/missing trusted package.json masked as…packages/core/src/tools/node-repl/runtime/module-loader.mjs:819 — [review] Cross-realm dynamic-import bridge rebuilt per call;…packages/core/src/tools/node-repl/tool.ts:114 — [review] getDescription truncates by UTF-16 units, splitting astral…
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) and the merge_group-only macOS/Windows test lanes were skipped in CI and the integration suite did not run locally; packages/desktop-shell suites (negated workspace) never ran。
未探索到全部深度（达到工具调用预算）：chunk 11：executed the test file ( npx vitest run src/tools/node-repl/result-converter.test.ts ) — the review worktree has no node_modules , so this fell back to static …。
未审查：反向审计——在 3 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 10 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| NODE_REPL: 'NodeRepl', | ||
| NODE_REPL_RESET: 'NodeReplReset', | ||
| NODE_REPL_ADD_NODE_MODULE_DIR: 'NodeReplAddNodeModuleDir', |
There was a problem hiding this comment.
[Critical] R1-1 (round-2 re-check — still stands, re-measured this round): the three new tool display names have no zh translation entries and are not in the i18n test's KEEP_ENGLISH set, so packages/cli's has a zh translation for every core tool display name test fails — one of the two test failures currently reddening the ubuntu CI lane.
Failure path: npm test --workspace=packages/cli (and therefore CI's test:ci) exits 1 with untranslated = ['NodeRepl', 'NodeReplReset', 'NodeReplAddNodeModuleDir']; localizeToolDisplayName falls back to the English name.
Witness (this round's build-test, test-delta vs merge base):
cli i18n suite: green on base, failing only on the PR
expected [], received [ 'NodeRepl', 'NodeReplReset', 'NodeReplAddNodeModuleDir' ]
Fix: add toolDisplayName.NodeRepl / NodeReplReset / NodeReplAddNodeModuleDir entries to packages/cli/src/i18n/locales/zh.js (and other locales per house convention), or add the names to KEEP_ENGLISH if intentionally English.
中文说明
R1-1（第二轮复查——仍然存在，本轮重新测量确认）：三个新工具显示名没有 zh 翻译词条，也不在 i18n 测试的 KEEP_ENGLISH 集合中，packages/cli 的 i18n 完整性测试失败——这是当前 ubuntu CI 变红的两个测试失败之一。修复：在 zh.js 中补充对应 toolDisplayName 词条（或将其加入 KEEP_ENGLISH）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| NODE_REPL: 'node_repl', | ||
| NODE_REPL_RESET: 'node_repl_reset', | ||
| NODE_REPL_ADD_NODE_MODULE_DIR: 'node_repl_add_node_module_dir', |
There was a problem hiding this comment.
[Critical] R1-2 (round-2 re-check — still stands, re-measured this round): the three new wire tool names are missing from Web Shell's TOOL_DISPLAY_NAMES map (packages/web-shell/client/components/messages/toolFormatting.ts, not touched by this diff), so the web-shell drift test fails — the second test failure reddening the ubuntu CI lane.
Failure path: npm test --workspace=packages/web-shell exits 1: the drift test expects [], receives ['node_repl', 'node_repl_reset', 'node_repl_add_node_module_dir']; user-visible, formatToolDisplayName renders the raw internal wire names in web tool panels.
Witness (this round's build-test, test-delta vs merge base):
toolFormatting.drift.test.ts: exit 0 on the merge-base tree, exit 1 only on the PR
missing = [ 'node_repl', 'node_repl_reset', 'node_repl_add_node_module_dir' ]
Fix: add the three entries to TOOL_DISPLAY_NAMES in toolFormatting.ts.
中文说明
R1-2（第二轮复查——仍然存在，本轮重新测量确认）：三个新的 wire 工具名缺失于 Web Shell 的 TOOL_DISPLAY_NAMES 映射，drift 测试失败——ubuntu CI 变红的第二个测试失败；Web 面板还会直接显示原始内部工具名。修复：在 toolFormatting.ts 中补充三个条目。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| ].join('\n'); | ||
|
|
||
| const edits: Edit[] = []; | ||
| const activeBindings = new Map(previousBindingsByName); |
There was a problem hiding this comment.
[Critical] R1-3 (round-2 re-check — still stands): a cell that throws before a top-level var's declaring statement commits silently loses values already assigned to that hoisted var. activeBindings is seeded only from previous bindings, so statement-boundary snapshots before the declaration carry no y, and kernel.mjs's catch path wholesale-replaces bindings with the last snapshot.
Failure path: with no prior y, cell 1 y = 42; throw new Error('boom'); var y; — plain Node keeps y === 42 (var hoists, the assignment ran), but here the next cell's y; throws ReferenceError: silent REPL state loss diverging from Node semantics and from the tool's own "a failed cell keeps bindings committed before the failure" contract.
Witness (probe through the real kernel, this round):
bindings after failing cell: []
cell2 error: ReferenceError y is not defined
plain-node baseline: y = 42
flip (seed activeBindings with the cell-wide hoisted-var set): ["y"] / ["42"]
Fix: compute the cell-wide hoisted-var set before the item loop and initialize activeBindings with carriedNames ∪ hoistedVars so every snapshot — including the last one before a failure — carries hoisted-var bindings; add the regression test.
中文说明
R1-3（第二轮复查——仍然存在）：在顶层 var 的声明语句提交之前抛错的 cell 会静默丢失已赋给该 hoisted var 的值——快照只从上一 cell 绑定播种，kernel 的 catch 分支又用最后一个快照整体替换绑定。探针（真实内核）：失败 cell 后绑定为空，下一 cell y; 抛 ReferenceError；原生 Node 保留 42。修复：在 item 循环前计算整 cell 的 hoisted-var 集合并并入 activeBindings 初始集合。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| } | ||
|
|
||
| function capChars(text: string, limit: number): string { | ||
| return text.length <= limit ? text : `${text.slice(0, limit)}…`; |
There was a problem hiding this comment.
[Critical] R1-25 (round-2 re-check — partially fixed, residual confirmed): takeTextWithinTokenUnits now backs off split surrogate pairs (the model-budget path is fixed), but capChars — applied to error name (1024), message (16384) and stack (2048) — still slices raw UTF-16 units, and the kernel-side capText (runtime/kernel.mjs:82-89) does the same. A mid-pair cut there passes straight into llmContent because the capped fields fit the budget.
Failure path: a cell throws an error whose name/message has an astral character at the cap boundary (e.g. e.name = 'a'.repeat(1023) + '🙂'); the cut lands between the surrogates and the lone high surrogate is serialized as \ud83d in the next model request — strict protobuf-JSON provider frontends reject it (INVALID_ARGUMENT) while the result stays in history.
Witness (probe, this round):
message='a'*16383+'🚀': loneSurrogateFound: true at index 16390; JSON contains \ud83d: true
error-name path: llmLoneSurrogates=1
kernel capText: unguarded at HEAD (rawKernelEventLoneSurrogates=1)
flip (high-surrogate backoff in both slicers): loneSurrogateFound: false
Fix: back off one code unit when the cut lands on a high surrogate (0xD800–0xDBFF) in capChars and kernel.mjs capText (the display cap shares the class).
中文说明
R1-25（第二轮复查——部分修复，残留确认）：模型预算路径已修复（takeTextWithinTokenUnits 会回退拆半的代理对），但 capChars（错误名/消息/栈上限）与内核侧 capText 仍按 UTF-16 码元裸切，切点落在 emoji 中间时孤立高位代理项会进入 llmContent，被严格的 protobuf-JSON 供应商前端拒绝。修复：两处切割器遇到高位代理项时回退一个码元。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| let operationChain = Promise.resolve(); | ||
| let shuttingDown = false; | ||
|
|
||
| const timers = new Map(); |
There was a problem hiding this comment.
[Critical] R1-29 (round-2 re-check — still stands): sandbox timer creation is the only kernel resource with no cap — scheduleTimer has no size check while every other resource (text bytes/events, images, error chars) has an explicit cap — and clearAllTimers runs only in shutdown(), never per-exec, so timers accumulate for the whole session-persistent kernel lifetime.
Failure path: one cell running for (let i = 0; i < 200_000; i++) setInterval(() => {}, 0); — no malice needed, a botched polling loop from model-generated code does it — accumulates unbounded timers with permanent CPU/memory growth for the rest of the session, clearable only by a timeout/reset that discards all bindings.
Witness (probe, this round):
interval planted in cell 1 still ticking during cell 2: ticks>0:true, ticks=43
5000 intervals in one cell: status: ok (no cap exists to trip)
clearAllTimers sole call site: shutdown()
(Honest bound: 100k zero-delay empty intervals did not hard-lockup a follow-up exec; the established harm is unbounded accumulation, not instant saturation.)
Fix: cap the timers Map (matching the other resource caps) and/or clear a cell's timers at exec end, rejecting or dropping new timers beyond the cap with a surfaced notice.
中文说明
R1-29（第二轮复查——仍然存在）：定时器创建是唯一没有上限的内核资源，且 clearAllTimers 只在 shutdown 调用、从不逐 exec 清理——一个 cell 里失控的轮询循环即可让定时器在整个会话生命周期无限累积（CPU/内存持续增长），只能通过丢弃全部绑定的超时/重置清除。修复：为 timers Map 设上限，或在 exec 结束时清理该 cell 的定时器。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| payload = { kind: 'url', url: image }; | ||
| } else if (intrinsicArrayBufferIsView(image)) { | ||
| payload = { kind: 'bytes', bytes: image, mimeType: null }; | ||
| } else if (image instanceof intrinsicArrayBuffer) { |
There was a problem hiding this comment.
[Suggestion] R1-57 (round-2 re-check — still stands, concrete trigger confirmed this round): emitImage's input dispatch mixes realm-local and realm-agnostic brand checks — the ArrayBuffer.isView branch works cross-realm (internal-slot based), but this bare-ArrayBuffer branch uses realm-local instanceof, so host-realm ArrayBuffers — exactly what the kernel-provided fetch()/Response return — fall through to TypeError: unsupported emitImage input, violating the documented emitImage(string | Uint8Array | ArrayBuffer | {bytes}) contract.
Failure path: a cell runs await nodeRepl.emitImage(await (await fetch('https://…/chart.png')).arrayBuffer()) — undici builds the ArrayBuffer in the host realm; instanceof fails in the vm realm, 'bytes' in image is false, and the natural fetch→emitImage flow throws a misleading unsupported-input error.
Witness (probe, this round): a fetch().arrayBuffer() result fails instanceof ArrayBuffer inside a vm.createContext realm (vm: false, host: true, 'bytes' in ab: false), while ArrayBuffer.isView and new Uint8Array(hostAB) are realm-independent.
Fix: brand-check realm-independently — accept Object.prototype.toString.call(image) === '[object ArrayBuffer]' (or wrap new intrinsicUint8Array(image) in try/catch) for this branch.
中文说明
R1-57（第二轮复查——仍然存在，本轮确认具体触发路径）：emitImage 的裸 ArrayBuffer 分支使用 realm 局部 instanceof，而内核提供的 fetch() 返回宿主 realm 的 ArrayBuffer——await nodeRepl.emitImage(await (await fetch(url)).arrayBuffer()) 这一自然用法会抛 "unsupported emitImage input"。修复：改用与 realm 无关的品牌检查（Object.prototype.toString 或 try/catch 包装 Uint8Array 构造）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| relative === '' || | ||
| (!relative.startsWith('..') && !path.isAbsolute(relative)) |
There was a problem hiding this comment.
[Suggestion] R1-60 (round-2 re-check — still stands): isUnder treats any relative path textually starting with .. as an escape, so files whose names begin with two dots are misclassified — path.relative('/a/pkg', '/a/pkg/..foo') returns ..foo and startsWith('..') is true. The same defect is mirrored in runtime/module-loader.mjs's isUnder and in normalizeTrustedPackage.
Failure path: a host pinning a trusted additional file named ..helper.js (a legal POSIX name directly under the package dir) gets 'trusted package file escapes its package directory' and cannot enable trust pinning for that package. Direction is fail-safe (deny, never accept) — availability, not a bypass.
| relative === '' || | |
| (!relative.startsWith('..') && !path.isAbsolute(relative)) | |
| relative === '' || | |
| (relative !== '..' && !relative.startsWith('..' + path.sep) && !path.isAbsolute(relative)) |
中文说明
R1-60（第二轮复查——仍然存在）：isUnder 把任何以 .. 开头的相对路径都当作逃逸，文件名以两个点开头（如 ..foo）会被误拒——宿主无法为含此类文件名的可信包启用固定。方向是安全的（只拒不纵），属于可用性问题。建议按上面 suggestion 改为段感知比较（module-loader.mjs 中的镜像实现同步修改）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| Ordinary expression values are not returned automatically. Only explicit | ||
| console output, `nodeRepl.write(...)`, and `nodeRepl.emitImage(...)` enter the |
There was a problem hiding this comment.
[Suggestion] New this round: this Explicit-output claim is contradicted by the implementation — raw kernel-child stdout/stderr is also captured into model-facing content (collectRawStream → collectText → renderText's stdout/stderr branch), and because raw chunks carry no execId they are attributed unconditionally to whatever execution is in flight (kernel-manager.ts:855), bypassing the exec-sealing guarantee this same doc states for tagged output.
Failure path (probed end-to-end this round): a package entry schedules setTimeout(() => process.stdout.write('LATE-RAW-FROM-PKG'), 150); cell 1 imports it (texts []); cell 2 — a different execution — receives ['stdout:LATE-RAW-FROM-PKG', 'write:cell2-out']: cell N's import produced output that silently landed in cell N+1's model-facing result. Integrators and security auditors relying on this doc get the containment story wrong.
Fix: either correct the doc (state that raw stdout/stderr is additionally captured with [stdout]/[stderr] prefixes and attributed to the in-flight execution, and amend the sealing claim) or stop feeding collectRawStream output into inflight.events.
中文说明
本轮新发现：设计文档"只有显式输出进入工具结果"的声明与实现不符——内核子进程的原始 stdout/stderr 也会被捕获进模型内容，且原始块不带 execId，会被无条件归属给当前 in-flight 的 exec（端到端探针证实：cell N 导入的包延迟输出落进了 cell N+1 的结果）。修复：更正文档（并修正 exec 封存声明），或不再把原始流输出计入 inflight.events。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| if (node.type !== 'export_statement') return; | ||
| for (const child of node.namedChildren) { | ||
| collectDeclarationNames(child, bindings); | ||
| } |
There was a problem hiding this comment.
[Suggestion] New this round: export-exclusion is pinned only for the export <declaration> form — this collector walks only declaration children, so the equivalent export { x } clause form silently bypasses it: a clause-exported name is persisted into subsequent cells and skips the transform-time exportedCollision check.
Failure path (measured through the real transform this round): export const x = 1; → bindingExports = [] (excluded, per the contract "user-exported declarations stay local to their cell"), but const x = 1; export { x }; → bindingExports = ['x'] and the snapshot commits x; a later cell's export const x = 2; then fails at transform time with "Identifier 'x' has already been declared", while the same sequence starting from the declaration form succeeds — persistence becomes spelling-dependent, with the collision deferred from transform time to a later cell.
Fix: extend collectExportDeclarationBindings to also collect names from export_clause specifiers (so the clause form is excluded and collision-checked like the declaration form), or pin the asymmetry explicitly in a test and the contract.
中文说明
本轮新发现：导出排除只对 export <声明> 形式生效——const x = 1; export { x }; 子句形式会绕过排除与碰撞检查，x 被持久化进后续 cell，之后再用声明形式导出同名会报 "已声明"，持久化行为取决于拼写。修复：让收集器同时处理 export_clause 的说明符，或用测试明确固定这一不对称。
— qwen3.8-max via Qwen Code /review (v0.21.14)
| if (!Number.isSafeInteger(params.timeout_ms) || params.timeout_ms <= 0) { | ||
| return '"timeout_ms" must be a positive safe integer.'; | ||
| } |
There was a problem hiding this comment.
[Suggestion] New this round: timeout_ms is validated for a lower bound and integrality only; the effective ceiling is kernel-manager's silent Math.min(timeoutMs, MAX_TIMER_DELAY_MS) clamp (2³¹−1 ms ≈ 24.8 days), and the scheduler-level per-tool timeout (QWEN_CODE_TOOL_EXECUTION_TIMEOUT_MS) is disabled by default — so the runaway-cell guard is defeatable, and requested vs effective timeout silently diverge above the clamp.
Failure path: the model calls node_repl with timeout_ms: Number.MAX_SAFE_INTEGER anticipating a long computation; the cell hangs in an infinite loop — nothing bounds the run for up to ~24.8 days except manual user intervention, which is exactly the case the timeout exists to catch.
Fix: add a schema maximum (e.g. a product ceiling like 600_000) and reject larger values here, so the guard cannot be disabled by caller-supplied values and the cap is documented rather than silent.
中文说明
本轮新发现：timeout_ms 只校验下限与整数性，没有上限；实际生效的是 kernel-manager 的静默钳制（2³¹−1 ms ≈ 24.8 天），且调度器级工具超时默认关闭——模型传一个超大 timeout_ms 即可让失控 cell 保护形同虚设。修复：为 schema 增加 maximum 并在此拒绝超限值。
— qwen3.8-max via Qwen Code /review (v0.21.14)
… tools Replaces the three built-in `packages/core` node_repl tools with a self-contained MCP server package, `@qwen-code/node-repl-mcp`. Why --- Issue QwenLM#9333 triage accepted the runtime only "for exploration" and gated it on an open maintainer decision: built-in core tool vs MCP-server-first. Reversing OpenAI Codex 0.149.0 settled the shape — it ships code mode as a standalone host with an in-process fallback, and its real-Node `js_repl` (not the restricted V8 `exec`) is the analogue this roadmap needs, because stage 3 (QwenLM#9335) imports cua-driver's N-API addons, which only real Node can load. Delivering out-of-core also keeps a security-relevant subsystem out of the maintainer-gated `packages/core` until its value is proven. What changed ------------ - New `packages/node-repl`: the kernel, module loader, cell transform, protocol and kernel manager, ported and now dependency-free (local `debug-log`/`win-path`/`tokenizer` replace core utils; `output-adapter` emits MCP content blocks in place of `result-converter`). - Reverted every `packages/core` change, the ~11 packaging files that existed only to ship the runtime assets into six distribution layouts, and the two design/plan docs describing the core delivery. - Deleted the trusted-package/sha256 layer: it was empty and unreachable in production (`module-loader.mjs` 882 -> 483 lines). - Wired the package into `scripts/build.js` and the root `vitest.config.ts`. Net effect: `packages/core` is untouched relative to main; the tool is opt-in via `mcpServers` instead of registered unconditionally for every user. Correctness fixes made while porting ------------------------------------ - Stack line numbers were wrong and drifted with binding count (source line 4 reported as 87). The prelude is now one physical line and the cell compiles with `lineOffset: -1`. - Top-level `var` nested in blocks/try/switch/loops was silently dropped; collection now walks the statement subtree, pruning at function boundaries. Verified against real Node for 16 constructs. - A binding named `nodeRepl` permanently broke the output channel; such cells are now rejected. Ordinary globals stay shadowable, matching plain Node. - Errors thrown by imported modules lost their class, `code` and stack. - A throwing frame handler could discard buffered protocol frames. - A hoisted `var` assigned before a throw is now kept, as Node does. - Unhandled rejections settling after a cell no longer vanish. - Live sandbox timers are capped, so one runaway loop cannot saturate the session's event loop. - Image MIME types are matched case-insensitively on both input paths. - Binding sort no longer depends on host locale collation. - The published bin lacked a shebang and mis-detected its entry point, and the server plus its kernel leaked on every host disconnect. Tests: 146 across 14 files, including a compiled N-API addon fixture, 100 consecutive cells, 10 concurrent isolated kernels, stack-line fidelity, and hoisting semantics. Three smoke scripts cover the adapter, the MCP wire and process lifecycle; the packed tarball was installed into a clean project and driven end to end. Refs QwenLM#9333
LaZzyMan
changed the title
github-actions
Bot
removed
the
review/self-reported
Revision: reverted the core tools, delivered as a standalone MCP serverHeads-up for anyone picking up the earlier review: this PR changed shape. The two automated review rounds above were against the built-in Why#9333's triage accepted the runtime only "for exploration" and gated it on an open maintainer decision — built-in core tool vs MCP-server-first — observing that validating it outside the maintainer-gated Reversing Codex 0.149.0 supported the shape: it ships code mode as a standalone host with an in-process fallback, and its real-Node Disposition of the previous review findingsI re-probed every finding from the automated rounds against the ported code rather than assuming the move fixed them, since the kernel was copied. No longer applicable (11): R1-1 / R1-2 (the display-name and i18n drift — no core tool names to register now), R1-34 / R1-5 / R2-3 ( Probed and already correct in this package (3): R1-4 (a shared non-circular reference renders in full, not Confirmed present and now fixed, each with a regression test (4):
R1-37 ( Additional defects found and fixed during the portNot in the earlier rounds, all regression-tested:
Three tests in the ported suite had encoded the buggy behaviour as expected, which is why the nested- Verification146 tests across 14 files, plus three smoke scripts (adapter, MCP wire, process lifecycle). Coverage now includes the two #9333 acceptance criteria that previously had none: a compiled N-API addon fixture loaded through a cell (criterion 6), and 100 consecutive cells with 10 concurrent isolated kernels (criterion 10). The packed tarball was installed into a clean project and the installed Please noteThe runtime is not an OS security sandbox and is not presented as one. Only 中文说明PR 形态已变更上面两轮自动 review 针对的是内置于 原因:#9333 的 triage 只"接受探索",并卡在"内置 core 工具 vs 先做 MCP server"这一维护者决定上,且指出先在 maintainer-gated 的 对既有 review 结论的处理:因为内核是复制过来的,我没有假设"迁移即修复",而是把每条结论都在移植后的代码上重新实测。
移植中另外发现并修复的缺陷:报错行号错误且随绑定数漂移(第 4 行被报成第 87 行);嵌套在 block/ 验证:146 个测试 / 14 个文件,外加三个 smoke 脚本。补上了 #9333 此前完全没有覆盖的两条验收标准:真实编译的 N-API addon 经 cell 加载(标准 6),以及 100 个连续 cell 与 10 个并发隔离内核(标准 10)。打包后的 tarball 安装到干净项目,并由真实 agent CLI 驱动:返回了真实容器主机名,并在第二次工具调用中读回 请注意:该运行时不是操作系统级安全沙箱,也未如此声称。仅拒绝 |
…e-checks `packages/node-repl` declared `zod ^4.1.13`, so `npm ci` installed a nested zod 4.4.3 for it while `@modelcontextprotocol/sdk` resolved the hoisted zod 3.25.76. Two zod type identities in one compilation made every `registerTool` input schema unassignable (`Type 'ZodString' is not assignable to type 'AnySchema'`), failing `npm run build` for this workspace — and with it the install step of every CI job that builds workspaces. The SDK accepts `^3.25 || ^4.0`, so pin the hoisted 3.x line and drop the now-stale nested lockfile entry. One deduped zod, no behaviour change. This only reproduced with a lockfile-driven install; the local tree had no nested copy, which is why the build passed locally and failed in CI.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) lanes were skipped in CI; the packages/node-repl suite ran on Linux only.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) lanes were skipped in CI; the packages/node-repl suite ran on Linux only。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| } | ||
| } | ||
|
|
||
| console.log(`node-repl-mcp: runtime assets copied to ${distRuntime}`); |
There was a problem hiding this comment.
[Critical] R3-2: The four new .mjs files (build.mjs, scripts/smoke.mjs, mcp-smoke.mjs, lifecycle-smoke.mjs) fall outside every ESLint globals override in the root eslint.config.js — the scripts block grants Node globals to ./scripts/**/*.js, ./scripts/**/*.mjs and packages/*/scripts/**/*.js, but not packages/*/scripts/**/*.mjs or packages/*/build.mjs. npm run lint therefore exits 1 with 19 no-undef errors ('process'/'console'/'setTimeout'/'clearTimeout' is not defined), and CI's "Run ESLint" job (lint:ci --max-warnings 0, also in release.yml) fails on this PR as-is. Fix: add 'packages/*/scripts/**/*.mjs' and 'packages/*/build.mjs' to the scripts block (mirroring the existing packages/web-templates/*.mjs pattern).
npx eslint packages/node-repl --ext .ts,.tsx → exit 1, 19 no-undef errors
npm run lint → ✖ 19 problems (19 errors, 0 warnings), exit 1
中文说明
四个新增 .mjs 文件（build.mjs、scripts/ 下三个 smoke 脚本）不在根 eslint.config.js 的任何 Node globals 覆盖范围内（scripts 块只覆盖 ./scripts/** 与 packages/*/scripts/**/*.js），npm run lint 因此以 19 个 no-undef 错误退出 1；CI 的 "Run ESLint"（lint:ci --max-warnings 0，release.yml 亦执行）在本 PR 上必然失败。修复：在 scripts 块加入 'packages/*/scripts/**/*.mjs' 与 'packages/*/build.mjs'。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (commit) { | ||
| edits.push({ | ||
| start: item.endIndex, |
There was a problem hiding this comment.
[Critical] R3-1: Statement-boundary snapshot commits are spliced at item.endIndex with no statement terminator, so any top-level statement that relies on ASI (no trailing ;) is glued directly onto the generated snapshot identifier. Cell const a = 1 (no semicolon) transforms so the commit lands right after the declarator marker — the exec returns SyntaxError: Unexpected identifier '__qwen_repl_..._0__snapshot', an incomprehensible error for the most basic valid cell; cell const a = 1\nx2 = a produces x2 = a__qwen_repl_...__snapshot["a"] = a; — a syntactically valid but different program (user identifier fused with the snapshot name → ReferenceError at runtime). Every existing test uses semicolon-terminated statements, which is why the suite is green. Fix: make the inserted commit self-terminating (prefix ';'); add a semicolon-free end-to-end regression test.
KERNEL cell "const a = 1" -> status=error SyntaxError: Unexpected identifier '__qwen_repl_d9cd4310b45de4af64de994f_0__snapshot'
KERNEL cell "const a = 1;\nx2 = a" -> ReferenceError: a__qwen_repl_55124024a4012a149b01e820_0__snapshot is not defined
fix-flip (';' + commit): all probe cells node --check-valid, kernel status=ok
中文说明
语句边界快照提交被无终止符地拼接在 item.endIndex 处，任何依赖 ASI（无尾随 ;）的顶层语句都会与生成的快照标识符粘连。const a = 1（无分号）会因提交紧跟声明标记之后而报 SyntaxError: Unexpected identifier '__qwen_repl_..._0__snapshot'；const a = 1\nx2 = a 会生成 x2 = a__qwen_repl_...__snapshot["a"] = a;——语法合法但语义错误（用户标识符与快照名融合，运行时报 ReferenceError）。现有测试全部使用带分号语句，因此套件是绿的。修复：为插入的提交加前缀 ';' 使其自终止，并补充无分号的端到端回归测试。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (!ALLOWED_IMAGE_MIMES.has(image.mimeType)) { | ||
| return { ok: false, reason: `unsupported image MIME ${image.mimeType}` }; |
There was a problem hiding this comment.
[Critical] R3-6: Image-frame mimeType bypasses the raw image budget entirely: collectImage counts only data.length against MAX_RAW_IMAGE_CHARS (128 MiB) and validates only typeof mimeType === 'string'. Executed code can forge protocol frames on the kernel pipe (reachable via the documented createRequire pattern: read fs, extract the execId from import.meta.url, write image frames to fd 3): 15 frames with empty data and 10 MB mimeType each are all accepted (imagesDropped=0, 150 MB retained past the 128 MiB budget); by the constants the ceiling is ≈4 GB per exec — 32× this PR's own image ceiling. Both validateImage rejection branches then interpolate the full uncapped string into notice text, and the estimator/binary search runs over it (a 3×50 MB variant stalled frame processing past the 30 s exec timeout → status=timeout). The legitimate kernel path (bridge.emitImage) is safe; this is a forged-frame bypass of the host's protection. Fix: count mimeType.length in the budget and/or cap it at frame validation; build rejection reasons from a bounded slice.
BASE (unmodified): acceptedFrames=15/15 imagesDropped=0 totalMimeCharsRetained=150000000 (budget 134217728)
FIXED (collectImage counts data+mimeType): acceptedFrames=13/15 imagesDropped=2
中文说明
图像帧的 mimeType 完全绕过原始图像预算：collectImage 只按 data.length 计入 MAX_RAW_IMAGE_CHARS（128 MiB），且只校验 typeof mimeType === 'string'。被执行代码可经文档推荐的 createRequire 模式在内核管道上伪造协议帧（读 fs、从 import.meta.url 提取 execId、向 fd 3 写 image 帧）：15 个 data 为空、mimeType 各 10 MB 的帧全部被接受（imagesDropped=0，保留 150 MB 超出 128 MiB 预算）；按常量上限每次 exec 可达 ≈4 GB——是本 PR 自身图像上限的 32 倍。两个拒绝分支还会把完整未截断字符串拼进提示文本并在其上跑估算/二分（3×50 MB 变体使帧处理停滞超过 30s exec 超时 → status=timeout）。合法内核路径（bridge.emitImage）安全；这是伪造帧对宿主防护的绕过。修复：预算计入 mimeType.length 并/或在帧校验处设上限；拒绝原因使用有界切片。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| ...builtinModules, | ||
| ...builtinModules.map((name) => `node:${name}`), | ||
| ]); | ||
| const DENIED_BUILTINS = new Set(['process', 'node:process']); |
There was a problem hiding this comment.
[Critical] R3-4: The process denial is enforced only on the ESM import path and is bypassed by the allowed node:module builtin: createRequire(import.meta.url)('process') returns the live host process object, and require also bypasses the module-root containment / canonical-target revocation that the import path enforces (probed: after the symlink-swap scenario the sibling test pins for imports, createRequire(...)('replacement-fixture') still loads). Host-realm objects exposed in the context additionally reach host Function/process via .constructor despite the codeGeneration restrictions. The runtime is documented as not a security boundary — but the tool description recommends createRequire without stating that it voids the denial and the containment. Fix: enforce uniformly (deny node:module or shim createRequire through the same rules), or state in the description that the denial is advisory.
probe (zero module roots): require("node:child_process").execSync("echo ESCAPE-OK") → ESCAPE-OK; require("process") → typeof-process=object|pid=true
control: await import('process') → 'Importing module "process" is not allowed in node_repl'
中文说明
process 拒绝仅在 ESM import 路径生效，可用的 node:module 内置即可绕过：createRequire(import.meta.url)('process') 能拿到宿主真实 process 对象；require 还绕过了 import 路径强制的模块根包含/规范目标吊销（探针验证：兄弟测试所固定的符号链接替换场景下 createRequire 仍可加载）。上下文暴露的宿主对象也可经 .constructor 触达宿主 Function/process。运行时虽已声明不是安全边界，但工具描述推荐使用 createRequire 却未说明它会使该拒绝与包含约束失效。修复：统一强制（拒绝 node:module 或让 createRequire 走同一套规则），或在描述中声明该拒绝仅为提示。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (error instanceof realmErrorConstructor) throw error; | ||
| throw realmError(error); |
There was a problem hiding this comment.
[Critical] R3-3: importDynamicSafely classifies errors by realm (error instanceof realmErrorConstructor), but errors thrown by host builtins (fs, fetch, timers) through imported modules — and failures of native packages loaded via the host import() — are host-realm, so they are rewrapped into a message-only cell Error: class, code, cause, custom properties and the original stack are all dropped, although the adjacent comment promises they survive. A cell doing try { await import('./dep.mjs') } catch (e) {} where dep.mjs runs fs.readFileSync('./missing.json') sees e.code === undefined and a loader-pointing stack instead of ENOENT + the dep frame — any cell branching on e.code (ENOENT/EACCES fallbacks, a ubiquitous Node idiom) silently takes the wrong branch. Static imports preserve everything via describeThrown, so identical failures diagnose differently by import style. Fix: discriminate by provenance (tag loader-phase errors) instead of realm, or copy name/code/stack/cause/own-enumerables into the rewrap.
BASE(cell-realm throw): {"name":"Error","code":"E_CUSTOM","stackHasDep":true}
PR(host fs ENOENT through same catch): {"name":"Error","message":"ENOENT: … open './definitely-missing-probe.json'","stackHasDep":false} — code undefined
中文说明
importDynamicSafely 按 realm 分类错误（instanceof realmErrorConstructor），但宿主内置模块（fs、fetch、timers）经被 import 的模块抛出的错误、以及经宿主 import() 加载的原生包的失败都是宿主 realm 的，会被重包装成只有 message 的 cell Error——class、code、cause、自定义属性与原始 stack 全部丢失（与相邻注释的承诺相反）。依赖 e.code 分支（ENOENT/EACCES 回退，极常见的 Node 惯用法）的 cell 会静默走错分支；静态 import 经 describeThrown 保留一切，同样的失败因 import 方式不同而诊断信息不同。修复：按来源而非 realm 区分（给 loader 阶段错误打标记），或在重包装时复制 name/code/stack/cause/可枚举属性。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| context.atob = (data) => Buffer.from(data, 'base64').toString('binary'); | ||
| context.btoa = (data) => Buffer.from(data, 'binary').toString('base64'); |
There was a problem hiding this comment.
[Suggestion] R3-17: Hand-rolled atob/btoa shims diverge from WHATWG semantics: btoa silently corrupts non-Latin-1 input (Buffer 'binary' truncates each code unit mod 256) instead of throwing InvalidCharacterError, and atob accepts malformed base64 — even though spec-compliant globalThis.atob/btoa exist on Node ≥22 (the package's engines floor) and adjacent code passes host Web APIs through unchanged. Probed in-cell: btoa('中文') → "LYc=" (no throw), atob(btoa('中文')) → "-" — silent data corruption where the standard API errors; atob('!!!') → "" instead of throwing. Existing tests only feed valid base64. Fix: context.atob = globalThis.atob; context.btoa = globalThis.btoa;.
cell: btoa("中文") → "LYc=", atob(btoa("中文")) → "-", atob("!!!") → ""
host: globalThis.btoa('中文') → throws InvalidCharacterError; globalThis.atob('!!!') → throws InvalidCharacterError
中文说明
手写的 atob/btoa 垫片偏离 WHATWG 语义：btoa 对非 Latin-1 输入静默损坏（Buffer 'binary' 按码元模 256 截断）而不是抛 InvalidCharacterError，atob 接受非法 base64——尽管 Node ≥22（本包 engines 下限）自带符合规范的 globalThis.atob/btoa，且相邻代码原样透传宿主 Web API。cell 内实测：btoa('中文') → "LYc="（不抛），atob(btoa('中文')) → "-"——标准 API 会报错之处静默数据损坏；atob('!!!') → "" 而非抛错。现有测试只喂合法 base64。修复：context.atob = globalThis.atob; context.btoa = globalThis.btoa;。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| send({ type: 'fatal', message: described.message }); | ||
| hostSetTimeout(() => process.exit(1), 10).unref(); |
There was a problem hiding this comment.
[Suggestion] R3-27: fatal() exits on a fixed 10 ms timer immediately after queueing the fatal frame, with no flush wait — unlike shutdown() directly above (destroy input + protocolOutput.end() + 100 ms backup). One exec may legally queue up to ~16 MiB of text plus 64 images; a malformed frame / unknown message / previousBindings mismatch then appends the fatal frame behind that backlog — probed with an 8 MiB burst and a 150 ms-stalled host: only pipe-buffer frames arrived, fatal frame received: false, exit event: null. Worse: while the host holds the fd-4 pipe open, process.exit(1) never terminates the kernel at all (hangs in futex until EOF) — the host gets neither the frame nor an exit code. Fix: mirror shutdown() — destroy input + end(() => process.exit(1)) + 100 ms backup.
PR (150 ms stall): frames=["ready","output(2097152)"], fatal received=false, exit event=null | FIXED (60 ms stall): fatal received=true
中文说明
fatal() 在把 fatal 帧入队后立即以固定 10 ms 定时器退出、不等待冲刷——与紧邻上方的 shutdown()（销毁输入 + protocolOutput.end() + 100 ms 兜底）不同。一次 exec 可合法排队约 16 MiB 文本 + 64 张图像；此时畸形帧/未知消息/previousBindings 不匹配会把 fatal 帧追加在该积压之后——以 8 MiB 突发 + 宿主停滞 150 ms 探针：仅管道缓冲内的帧到达，fatal frame received: false、exit event: null。更糟：宿主保持 fd-4 管道打开时 process.exit(1) 根本不终止内核（futex 挂起直到 EOF）——宿主既收不到帧也收不到退出码。修复：仿照 shutdown()——销毁输入 + end(() => process.exit(1)) + 100 ms 兜底。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| process.on('unhandledRejection', (error) => { | ||
| const message = `Uncaught (in promise) ${describeThrown(error).message}`; |
There was a problem hiding this comment.
[Suggestion] R2-2: (Round-2 ledger R2-2, still stands.) The kernel registers an unhandledRejection handler but no uncaughtException counterpart, while setImmediate/clearImmediate/queueMicrotask are injected into the cell context as raw host functions with no callback error containment — unlike setTimeout/setInterval, which route through scheduleTimer's try/catch. Cell setImmediate(() => { throw new Error('boom'); }) evaluates fine (execResult ok); one tick later the kernel exits 1 with no fatal frame — the next exec reports 'crashed… bindings were lost' on a fresh kernel: the whole persistent session dies to a stray throw in a callback the tool itself injects. Fix: register process.on('uncaughtException', …) reporting through the same emitText-if-active / lateRejections path, or wrap the raw passthroughs like the timers.
real kernel: setImmediate(throw) → first exec status=ok; next exec status=crashed 'kernel exited (code=1); bindings were lost.'
raw-kernel drive: execResult ok → STDERR Error: boom → EXIT code=1, no 'fatal' frame emitted
中文说明
（第二轮台账 R2-2，仍然存在。）内核注册了 unhandledRejection 却没有 uncaughtException 对应项，而 setImmediate/clearImmediate/queueMicrotask 以裸宿主函数注入且回调无错误收容——不像 setTimeout/setInterval 走 scheduleTimer 的 try/catch。cell setImmediate(() => { throw new Error('boom'); }) 本身执行成功（execResult ok）；下一刻内核以退出码 1 退出且无 fatal 帧——下一次 exec 报 'crashed… bindings were lost' 并换新内核：整个持久会话因工具自己注入的回调中的一个随手抛出而死亡。修复：注册 process.on('uncaughtException', …) 走同一 emitText/lateRejections 路径，或像定时器一样包装这些裸透传。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| function assertEsm(filePath, localDefaultEsm) { | ||
| if (filePath.endsWith('.mjs')) return; | ||
| if (filePath.endsWith('.js')) { | ||
| if (localDefaultEsm || nearestPackageType(filePath) === 'module') { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R3-12: localDefaultEsm is hardcoded true in createCell and propagated unchanged through every chain, making nearestPackageType → readJson → allowedRoots (~55 lines, incl. MAX_PACKAGE_JSON_BYTES and the loader's entire readableRoots option) unreachable dead code, and the advertised 'local CommonJS file imports are not supported' rejection impossible to fire. A cell importing a local CommonJS .js file is silently accepted as ESM and fails at evaluate time with a confusing ReferenceError: module is not defined from inside the VM; meanwhile the API shape implies readableRoots constrains the loader (kernel.mjs passes it in) while kernel-side resolveReadableFile enforcement is real — the dead copy is actively misleading. Fix: pick one model and delete the other half.
unmodified: local CJS import → ReferenceError: module is not defined; mutant (localDefaultEsm:false) → 'local CommonJS file imports are not supported; … not in a type=module package'
中文说明
localDefaultEsm 在 createCell 中硬编码为 true 且沿调用链原样传播，使 nearestPackageType → readJson → allowedRoots（约 55 行，含 MAX_PACKAGE_JSON_BYTES 与 loader 的整个 readableRoots 选项）成为不可达死代码，宣称的 'local CommonJS file imports are not supported' 拒绝永远无法触发。导入本地 CommonJS .js 文件的 cell 被静默当作 ESM 接受，在求值期从 VM 内部抛出令人困惑的 ReferenceError: module is not defined；同时 API 形态暗示 readableRoots 约束 loader（kernel.mjs 确实传入），而内核侧 resolveReadableFile 强制是真实存在的——死代码副本具有误导性。修复：二选一，删掉另一半。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| try { | ||
| return path.join(fs.realpathSync(existingPrefix), ...missingSegments); | ||
| } catch (error) { |
There was a problem hiding this comment.
[Suggestion] R3-36: canonicalizeFuturePath's realpath of the EXISTING PREFIX (the only line keeping a not-yet-created root canonical under a symlinked prefix) has zero test coverage — the future-path cases use plain tmpdirs, where raw and real paths coincide on Linux CI. Mutant dropping fs.realpathSync survives every existing test; afterwards, registering a not-yet-existing root under a symlinked prefix stores a non-canonical canonicalPath, and once the directory is created, moduleSearchRoots() mismatches realpath vs canonicalPath and silently drops the root — imports fail with MODULE_NOT_FOUND with no indication the registration was voided (probed E2E). Fix: add a symlink-prefix future-path test asserting the result is rooted at the real target.
mutant (realpath removed): security-policy.test.ts still 4/4; probe: approved=…/link/future/node_modules expected=…/real/future/node_modules; E2E import=ERR:cannot resolve package 'futurepkg' from 0 module roots (unmodified: 42)
中文说明
canonicalizeFuturePath 对“已存在前缀”的 realpath（让尚不存在的根在符号链接前缀下保持规范化的唯一一行）零测试覆盖——未来路径用例都用普通 tmpdir，Linux CI 上原始与真实路径恰好一致。删除 fs.realpathSync 的突变体通过所有现有测试；此后在符号链接前缀下注册尚不存在的根会存储非规范的 canonicalPath，目录创建后 moduleSearchRoots() 比较 realpath 与 canonicalPath 不匹配并静默丢弃该根——import 报 MODULE_NOT_FOUND 而毫无注册已被作废的提示（已端到端探针验证）。修复：新增符号链接前缀的未来路径测试，断言结果以真实目标为根。
— qwen3.8-max via Qwen Code /review (v0.22.0)
The eslint "scripts we run with node" override matched
`packages/*/scripts/**/*.js` but not `.mjs`, nor a package-root `build.mjs`.
`packages/node-repl` is `type: module`, so its `build.mjs` and `scripts/*.mjs`
were linted as browser code and `eslint .` failed with `'process' is not
defined` / `'console' is not defined` / `'setTimeout' is not defined`.
The pre-commit hook only lints `*.{js,jsx,ts,tsx}`, so `.mjs` files are not
checked locally — this only surfaced in the root CI lint step.
Add `packages/*/scripts/**/*.mjs` and `packages/*/build.mjs` to the override,
matching the existing `.js` entries. Generic, additive, no behaviour change.
…entity, image bound)
Triaged the bot's round-3 critical findings against the ported code and fixed
the three that were genuine defects here; each has a regression test.
- R3-5 (resolution): a symlinked `<cwd>/node_modules` — the norm under pnpm,
monorepo hoisting and shared CI caches — was silently dropped, so the
documented zero-config `await import('pkg')` failed with "cannot resolve from
0 module roots" while plain Node resolved it. The implicit cwd root was
applying a re-link self-comparison that only makes sense for registered roots
(which carry a registration-time canonical baseline). Follow the symlink for
the implicit root, but skip it entirely when the cwd node_modules is itself a
registered root, so the registered root's revocation guard is not undermined.
- R3-3 (error identity): an error thrown by a host builtin inside imported code
(e.g. `fs.readFileSync` → ENOENT) was rewrapped message-only, dropping
`code`/`errno`/`syscall`/`cause`/`stack` — so `catch (e) { if (e.code ===
'ENOENT') }`, a ubiquitous Node idiom, silently took the wrong branch. Carry
those fields onto the realm-wrapped error.
- R3-6 (image bound): image-frame `mimeType` was unbounded — only `data.length`
counted against the raw image budget — so a malformed/forged frame could
retain a huge string that also got interpolated into a notice and tokenized.
Bound the MIME at frame ingestion (a real image MIME is a few dozen chars).
- R3-4 is by design (the runtime is not a security boundary), but the tool
description recommended `createRequire` without noting it is not subject to
the process denial or module-root containment the import path enforces; the
description now says so.
- R3-1 / R3-2 (the `.mjs` lint failure) were already fixed in an earlier commit.
Suite: 148 tests (added symlinked-cwd resolution and host-builtin error-code
regressions). The symlink fix initially defeated the registered-root revocation
test; the registered-root deferral above resolves both.
… fresh name
Round-3 review R3-12/R3-33: re-declaring an existing top-level function or class
in a later cell is a link-time SyntaxError (they persist as `let`, which the
prelude re-declares), whereas a plain Node REPL accepts it. A correct fix needs
declaration-kind tracking the manager does not carry today; until then the tool
description's rerun guidance ('prefer var') is corrected, since a function cannot
be made rerunnable via var.
Round-3 review dispositionI re-probed every round-3 critical against the current code (each with a runnable reproducer) rather than assume the earlier commits covered them. Fixes pushed in Criticals
Serious findings — probed, already correct
Serious finding — real, documented rather than rushed
The restMost remaining serious items are test-efficacy probes (mutation-style "deleting line X keeps the suite green") — legitimate coverage-hardening suggestions, not defects. They're noted for a follow-up test pass; none indicate incorrect behavior in the shipped code. Suite is 148 tests (added: symlinked-cwd resolution, host-builtin error-code identity). 中文说明逐条实测了 round-3 的全部 critical(每条都有可复现脚本),而非假设前面的提交已覆盖。修复在 Critical:R3-1/R3-2( 已实测本就正确的 serious:R3-11(块内 真实但选择先文档化的 serious:R3-12/R3-33(后续 cell 重定义顶层 function/class 会 link 期 SyntaxError,而 Node REPL 接受)—— 确认存在,但正确修复需要当前未跟踪的声明种类信息,半吊子修复有引入新分歧的风险; 其余:多数剩余 serious 是变异测试式的"删掉某行测试仍绿"的测试效能建议,是覆盖强化项而非缺陷,留待后续补测,不代表已发布代码有错误行为。 148 个测试(新增:符号链接 cwd 解析、宿主内建错误码身份); |
pomelo-nwu
left a comment
pomelo-nwu
left a comment
There was a problem hiding this comment.
LGTM, approving.
中文
看起来不错，批准。
|
Released in v0.22.2. |
6 participants
What this PR does
Delivers the session-persistent Node.js REPL of #9333 as a standalone MCP server — a new
packages/node-replpackage published as@qwen-code/node-repl-mcp— instead of three built-inpackages/coretools.packages/coreis untouched relative tomain. The net diff is the new package plus two wiring lines (scripts/build.jsbuild order, rootvitest.config.tsprojects). The tool is opt-in viamcpServersrather than registered unconditionally for every user.It exposes three tools over stdio MCP:
Each call evaluates a fresh
vm.SourceTextModule; previously committed bindings reach the next cell through an@prevSyntheticModule, so object identity, closures, declaration kinds and per-declarator partial commits survive. Cells use dynamicimport(); local.js/.mjsreload per execution while packages keep Node's singleton semantics, including CommonJS and N-API addons.Why this shape
Triage on #9333 accepted the runtime "for exploration" and gated it on an explicit maintainer decision — built-in core tool vs MCP-server-first — noting that validating it outside the maintainer-gated
packages/corewould prove its value first. This PR takes that path.Reversing OpenAI Codex 0.149.0 (its published
config-schema.jsonand thecodex-code-mode-hostbinary) showed the reference implementation ships code mode as a standalone host with an in-process fallback (code_mode_host.disable_in_process_fallback), and that it has two distinct mechanisms: a restricted V8execwith no Node, and a real-Nodejs_repl. The real-Node one is the analogue this roadmap needs, because stage 3 (#9335) imports cua-driver's N-API addons, which only real Node can load — not a restricted V8 isolate.An out-of-core package also means this security-relevant subsystem is not registered by default for every user, and can iterate without the two-tier core gate.
What changed relative to the previous revision
Everything the earlier revision added under
packages/corewas reverted, along with the ~11 packaging files that existed only to ship the runtime assets into six distribution layouts, and the two design/plan docs describing the core delivery. The package ships its own assets viabuild.mjs.The always-empty trusted-package / sha256 layer was deleted — it was unreachable in production (
NodeReplSecurityPolicy.default()returned an empty set with no way for a host to populate it), which stranded a large amount of security-critical code.module-loader.mjswent 882 → 483 lines.security-policy.tsnow only validates and canonicalizes model-suppliednode_modulesroots.Correctness fixes found while porting
Each is covered by a regression test:
lineOffset: -1. Imported modules keep their own correct line numbers.varnested in a block/try/switch/loop was silently dropped, soif (true) { var x = 1 }did not persist. Collection now walks the statement subtree, pruning at function-scope boundaries. Verified against real Node for 16 constructs (including class static blocks, IIFEs in object literals,for await, and correctly not hoistingconst/let).nodeReplpermanently broke the output channel with no recovery but a reset — while the tool description instructs the model to callnodeRepl.write. Such cells are now rejected with an actionable message. Ordinary globals (Buffer,URL,fetch, …) stay shadowable, matching plain Node.code, custom properties and stack; they are now rethrown untouched when already realm-owned, while loader failures still arrive as cell-realmErrors.varassigned before a throw was lost; plain Node keeps it, and the tool's own contract promises bindings committed before a failure survive.{bytes}anddata:URL paths.localeComparemade the host↔kernel handshake and the generated source vary byLANG.bincould not run: no shebang, and entry-point detection failed for npm's bin symlink and for paths containing spaces.oncloseon stdin EOF.Reviewer Test Plan
How to verify
Run vitest from inside the package. A bare
npx vitest runat the repo root executes every workspace project and can exhaust the default heap.The suite covers: binding persistence and partial commit, a compiled N-API addon fixture loaded through a cell via
createRequire(#9333 criterion 6), 100 consecutive mixed cells and 10 concurrent isolated kernels with no residual processes or temp dirs (criterion 10), stack-line fidelity, hoisting semantics vs real Node, module reload vs package singleton, timeout/cancel/crash revocation, MCP schema and annotations, and the output adapter's token budget, image caps and MIME validation.Evidence (Before & After)
N/A — non-UI runtime and packaging change. Before this revision the runtime was three built-in core tools; it is now an opt-in MCP server and
packages/coreis unchanged.Beyond the automated suite, the packed tarball was installed into a clean project and the installed
bindriven end to end over real MCP, and a real agent CLI (Qoder) drove the installed package: it returned the actual container hostname and read aDate.now()probe back in a second tool call, proving one persistent kernel, and carried a block-scopedvaracross two calls, confirming the hoisting fix in the shipped artifact.Tested on
Environment
Linux x64, Node.js v22.23.1 — against the source runtime, the built package, the packed tarball installed into a clean project, and a real model-driven MCP session.
Risk & Scope
process/node:processare denied; other builtins load with ordinary Node authority and the child inherits the parent environment. Grant this server only in trusted contexts. The design deliberately prioritises capability here, matching the reference implementation, which does not sandbox its code-mode host either.build.mjsresolvestscthroughrequire.resolverather thannpxso it should work on Windows, but that is untested.mcpServers. Nothing onmainchanges, so there is no user-facing migration.Linked Issues
Refs #9333
中文说明
本 PR 做了什么
把 #9333 的会话级持久 Node.js REPL 以独立 MCP server 形态交付 —— 新增
packages/node-repl包(@qwen-code/node-repl-mcp),不再是三个内置于packages/core的工具。packages/core相对main完全未改动。净变更是新增包 + 两行接线(scripts/build.js构建顺序、根vitest.config.tsprojects)。该能力通过mcpServers显式启用,而不是对所有用户无条件注册。为什么是这个形态
#9333 的 triage 只"接受探索",并明确卡在一个维护者决定上:内置 core 工具 vs 先做 MCP server,且指出在进入 maintainer-gated 的
packages/core之前先验证价值更合适。本 PR 走这条路。对 OpenAI Codex 0.149.0 的逆向(其公开的
config-schema.json与codex-code-mode-host二进制)表明:参考实现把 code mode 做成独立 host + 进程内 fallback(code_mode_host.disable_in_process_fallback),并且存在两套机制 —— 一个没有 Node 的受限 V8exec,和一个真实 Node 的js_repl。本路线图需要的是后者,因为第三阶段(#9335)要 import cua-driver 的 N-API addon,那只有真实 Node 能加载。相对上一版的变化
上一版在
packages/core下新增的内容全部回退,连同仅为把运行时资产塞进六种发行布局而存在的约 11 个打包文件,以及描述 core 交付形态的两份设计/计划文档。本包通过自己的build.mjs携带资产。始终为空的可信包 / sha256 层已删除 —— 它在生产中不可达(
NodeReplSecurityPolicy.default()返回空集合,且宿主无从填充),却搁置了大量安全敏感代码。module-loader.mjs从 882 行降到 483 行。移植过程中发现并修复的正确性问题
每一项都有回归测试:
lineOffset: -1编译。被 import 的模块保持自己正确的行号。try/switch/循环里的顶层var被静默丢弃,if (true) { var x = 1 }不会持久化。现在遍历语句子树、在函数作用域边界剪枝。对照真实 Node 验证了 16 种构造(含 class static block、对象字面量里的 IIFE、for await,以及正确地不提升const/let)。nodeRepl的绑定会永久破坏输出通道,除重置外无法恢复 —— 而工具描述恰恰教模型调用nodeRepl.write。此类 cell 现在被拒绝并给出可操作信息。普通全局(Buffer、URL、fetch等)仍可遮蔽,与原生 Node 一致。code、自定义属性与堆栈;现在若已属 cell realm 则原样重抛,而 loader 自身的失败仍包装为 cell realm 的Error。var会丢失;原生 Node 会保留,且工具自身承诺"失败前已提交的绑定会保留"。{bytes}与data:URL 两条路径上现已统一按大小写不敏感处理。localeCompare使 host↔kernel 握手与生成源码随LANG变化。bin无法运行:缺 shebang,且入口检测对 npm 的 bin 符号链接与含空格路径失效。onclose。风险与范围
process/node:process,其他内置模块以普通 Node 权限加载,子进程继承父环境。请仅在可信环境中启用。此处刻意以能力优先,参考实现同样不对其 code-mode host 做沙箱。build.mjs通过require.resolve解析tsc而非npx,理论上兼容 Windows,但未实测。mcpServers显式启用。main上没有任何变化,因此不存在面向用户的迁移。