fix(core): parse the workflow meta literal instead of evaluating it - #9325
Conversation
`extractAndStripMeta` evaluated the model-authored `export const meta = {...}`
block in a vm and then walked the result. Both halves are unbounded, and each
hangs the host on its own:
{ name: (function () { while (true) {} })() } // spins during evaluation
{ get phases() { while (true) {} } } // spins during the walk
The loops are synchronous, so the event loop is blocked outright — no timer
fires and nothing in-process can cancel it. Bounding them turned out to be a
moving target: a vm timeout does not reach a getter invoked on the host, and
moving the walk into the vm still leaves promise reactions, proxy traps and
runaway allocation. Each fix invited the next.
Meta is a declaration, not a computation. Every contract field is a string, and
upstream states the rule outright: the meta object must be a pure literal, with
no variables, calls, spreads or interpolation. Given that contract, evaluating
it was the wrong mechanism. Parse it instead.
A parser has no execution semantics, so none of those failures are bounded —
they are unrepresentable. There is nothing to time out, sandbox or isolate. The
vm context, the timeout and the thenable walker all go away.
The grammar is JSON's value grammar plus the spellings a model actually writes:
unquoted keys, single-quoted and substitution-free template strings, trailing
commas, and comments. Anything meaning "evaluate something" is rejected by name
so the diagnostic tells the author which rule they hit.
This narrows the contract. A meta block that computed a value used to work if
the computed field was outside the contract surface, because validateMeta
dropped it silently; now the whole literal is refused. Verified against every
meta literal in the existing suite: of the ten the parser refuses and the vm
accepted, nine are the attacks above, and the tenth is a regex literal in a
non-contract field. Zero cases where both accept and disagree on the value.
No workflow scripts ship in the repo, so nothing in tree changes behaviour.
Parsing is also ~70x faster than the vm path it replaces, which matters because
this call is on the path to the confirmation dialog and saved-workflow
enumeration.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks for the PR — this is the third iteration of the workflow-meta hazard thread (#9097 → #9136 → this), and it reads like the terminal answer to it. Template looks good ✓ — all sections present, bilingual body, evidence correctly marked N/A for a non-user-visible change. Problem: observed, not theoretical. The vm-eval path in Direction: aligned. Meta is a declaration — every contract field is a string, and the pure-literal rule is upstream's own contract. "Stop evaluating, start parsing" is the direction the review rounds on #9097/#9136 were already pushing. Size: core paths touched ( Approach: the scope feels right. The simpler alternatives were already tried and found wanting in the two predecessor PRs, and there's no drive-by change here — the diff is exactly the swap plus its tests. The deliberate contract narrowing is disclosed and measured: of the 42 existing meta fixtures, 19 parse identically, 13 were rejected by both paths, 10 are now refused (nine hostile fixtures plus one regex in a non-contract field that used to be silently dropped), and — the row that matters — zero cases where both paths accept and disagree on the value. Risk: no elevated revert-risk path matches. One flag for the final stage though: this does restructure part of Moving on to code review. 🔍 中文说明感谢贡献!这是 workflow-meta 隐患线程的第三次迭代(#9097 → #9136 → 本 PR),读起来是该问题的最终答案。 模板完整 ✓ —— 各部分齐全,中英双语,非用户可见改动的证据正确标注为 N/A。 问题:已观测到,而非理论性的。 方向:对齐。Meta 是声明——契约的每个字段都是字符串,纯字面量规则本身就是上游契约。"不再求值,改为解析"正是 #9097/#9136 评审过程一直在推动的方向。 规模:触及核心路径( 方案:范围合理。更简单的替代方案已在前两个 PR 中尝试过并被否定;diff 中也没有夹带无关改动——恰好是替换本身加测试。有意的契约收窄已披露并经过度量:现有 42 个 meta fixture 中,19 个解析结果完全一致,13 个两条路径都拒绝,10 个现在被拒绝(九个恶意 fixture 加一个位于非契约字段的 regex——过去会被静默丢弃),而最关键的一行是:零 例两条路径都接受但取值不同的情况。 风险:未命中高回滚风险路径。但为最终结论标记一点:本 PR 重构了 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI read the diff against my own baseline proposal for this problem (which was the same terminal move — a pure literal is parsed, not evaluated), and the PR matches or exceeds it. No correctness or security blockers found. What I verified:
Test evidence — the PR's own CI (unattended run; PR code never executed here)CI on Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Not verified: the author's 42-fixture old-vs-new comparison table (19 identical / 13 both reject / 10 parser-refuses / 0 value differences) is the author's measurement, not independently re-run here — the shared-acceptance slice of it is what the 141 untouched tests pin, so the running ubuntu suite is the load-bearing signal for it. The "revert and watch it hang" reproduction is from the PR description (and the predecessor threads), also not re-run — CI runs never execute PR code. Sandboxed verification would settle the rest: 中文说明代码审查:我将 diff 与自己对该问题的独立方案(同一个最终思路——纯字面量应被解析而非求值)对照,本 PR 达到或超过了该方案。未发现正确性或安全性阻塞项。已验证:解析器所有路径都是有界扫描(每个循环要么推进下标要么抛错,递归深度上限 32 且两侧都有测试,输出规模与源码线性相关)——循环、getter、proxy 陷阱、promise 反应、失控内存分配在这里不是"被限制",而是"不可表示"。转义解码器(作者自己也承认是最易出错的部分)与 JS 语义在全部接受形式上一致,并对八进制、截断、越界形式拒绝,每种形式都在测试中对照等价 JS 字面量钉死;唯一的非阻塞偏差:U+2028/U+2029 前的反斜杠会保留该字符而非按行续处理,输入概率极低且无安全影响。所有可执行构造均按名称拒绝并给出规则说明; 测试证据(无人值守运行,此处绝不执行 PR 代码): — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review across every stage; the score reflects policy, not doubt. Stepping back: my independent proposal for this problem was the same move the PR makes — a pure literal should be parsed, never evaluated — and the PR executes it better than my baseline would have (null-prototype construction, pinned escape tests, named diagnostics). The two predecessor PRs already proved the simpler paths dead ends, so this isn't overengineering; it's where the thread terminates. The diff carries nothing but the swap and its tests, the ~140 lines of deleted thenable-walking machinery are a net simplification, and the problem it removes — a synchronous loop in model-authored meta hanging the host with no timer able to fire — is a real one with concrete shapes, not a theoretical hardening. The reason I'm not approving is categorical, not substantive. This is a fork PR into core infrastructure that restructures part of the workflow sandbox's security model and deliberately narrows an extension-author-facing contract (computed meta fields now hard-error instead of being silently dropped). The narrowing is disclosed and measured — zero cases where both old and new paths accept a literal and disagree on the value — and it matches upstream's own pure-literal rule, but a contract change of that shape should have a human maintainer's name on it, not a bot's. That is the whole reservation; the code itself I would merge. Two practical notes for the maintainer: the ubuntu unit suite was still running at review time (see the CI table above — the finalize job will update it when CI lands), and the equivalence claim's load-bearing slice is pinned by the 141 untouched sandbox tests. If you want A/B proof that the old path genuinely hangs on the getter fixture while this one rejects it, ⏸️ Deferring to @wenshao — fork PR into core touching the workflow sandbox's security model plus a deliberate meta-contract narrowing; needs a human sign-off on the contract change. No blocking issues found in review; the call here is yours, not the gate's. Needs a human call on this one. 中文说明置信度:3/5 —— 每个阶段都干净;这个分数反映的是流程策略,而非疑虑。 退一步看:我对该问题的独立方案与 PR 的做法一致——纯字面量应该被解析而不是被求值——而 PR 的执行比我的基线方案更好(空原型构造、钉死转义语义的测试、点名规则的报错)。前两个 PR 已经证明更简单的路径走不通,所以这不是过度设计,而是这条线程的终点。diff 中除替换本身与测试外别无他物,删除的约 140 行 thenable 遍历机制是净简化,它消除的问题是真实的(模型生成的 meta 中的同步循环会挂死宿主进程且任何定时器都无法触发),且有具体形态佐证,不是理论性加固。 不批准的原因是类别性的,而非实质性的。这是一个进入核心基础设施的 fork PR,重构了 workflow 沙箱安全模型的一部分,并有意收窄了面向扩展作者的契约(计算型 meta 字段从静默丢弃变为硬性报错)。该收窄已披露且经过度量——新旧两条路径都接受且取值不同的情况为零——并与上游自身的纯字面量规则一致,但这种形状的契约变更应当由人类 maintainer 署名,而不是机器人。这是唯一的保留意见;就代码本身而言我会合并。 给 maintainer 的两条实务提示:审查时 ubuntu 单测套件仍在运行(见上方 CI 表格——CI 落定后 finalize 任务会更新它),等价性声明的承重部分由 141 个未改动的沙箱测试钉住。如果你想要 A/B 证据证明旧路径确实会在 getter fixture 上挂死而新路径会立即拒绝, ⏸️ 转交 @wenshao —— 进入核心模块的 fork PR,触及 workflow 沙箱安全模型,且有意收窄 meta 契约;契约变更需要人类签核。审查未发现阻塞问题;这里的决定权在你,不在门禁。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): 212 passed — this review observed 1471, 20936, 1571, 494 passed; 698 passed — this review observed 1471, 20936, 1571, 494 passed.
中文说明
Test Plan(非阻断):212 passed — this review observed 1471, 20936, 1571, 494 passed; 698 passed — this review observed 1471, 20936, 1571, 494 passed。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| this.index < this.src.length && | ||
| WHITESPACE.has(this.src[this.index]!) |
There was a problem hiding this comment.
[Suggestion] The trivia set diverges from ECMAScript WhiteSpace + LineTerminator: WHITESPACE omits the Space_Separator characters U+1680, U+2000–U+200A, U+202F, U+205F, U+3000, and skipTrivia never skips U+2028/U+2029 — Concrete cost: meta blocks using them as separators fail workflow load with a diagnostic pointing at legal syntax. Probe: {\u3000name: 'x'\u3000} throws expected a property name and { name: 'x'\u2028} throws expected "," or "}" at position 11, while native eval accepts both. U+3000 (ideographic full-width space) is commonly emitted by CJK input methods and by models writing CJK text — plausible in exactly this product's workflow scripts. Fail-closed, but the grammar claims the JS spellings models actually write, and trivia is not a spelling choice. (/\s/ matches exactly the JS WhiteSpace + LineTerminator set; parseString keeps its own stricter rules, so string contents are unaffected.)
| this.index < this.src.length && | |
| WHITESPACE.has(this.src[this.index]!) | |
| this.index < this.src.length && | |
| (WHITESPACE.has(this.src[this.index]!) || /\s/.test(this.src[this.index]!)) |
中文说明
空白(trivia)集合与 ECMAScript 的 WhiteSpace + LineTerminator 不一致:WHITESPACE 缺少 Space_Separator 字符 U+1680、U+2000–U+200A、U+202F、U+205F、U+3000,且 skipTrivia 从不跳过 U+2028/U+2029。具体代价:以它们作为分隔符的 meta 块会导致 workflow 加载失败,而诊断指向完全合法的语法。Probe:{\u3000name: 'x'\u3000} 抛 expected a property name,{ name: 'x'\u2028} 抛 expected "," or "}" at position 11,而原生 eval 两者都接受。U+3000(全角空格)常被 CJK 输入法和书写 CJK 文本的模型输出——在本产品的 workflow 脚本里完全可能出现。虽是失败即拒绝(fail-closed),但文法声称接受模型实际会写的 JS 写法,而空白处理不属于写法选择。(/\s/ 恰好匹配 JS 的 WhiteSpace + LineTerminator 集合;parseString 保留自己更严格的规则,字符串内容不受影响。)上方 suggestion 即该修复。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| it('treats a backslash-newline as a line continuation', () => { | ||
| const { name } = parseWorkflowMetaLiteral( | ||
| '{ name: "a\\\nb", description: "d" }', |
There was a problem hiding this comment.
[Suggestion] The CRLF backslash line-continuation branch (case '\r': in parseEscape(), workflow-meta-literal.ts:327) has no test — only this \n continuation is pinned, despite the file's stated policy of pinning every escape form because escape handling is the fiddliest part of a hand-rolled parser — Concrete cost: for a model-authored meta string on CRLF line endings that wraps a long line with a backslash, a future edit deleting or breaking the \r arm ships green: backslash+CRLF would then either spuriously reject with unterminated string (the following \n trips the non-template check) or decode a stray \r via the identity-escape default — no current test turns red. Verified: deleting the arm leaves the 64-test suite green.
Add beside this case (a backslash followed by CR LF in the source, expecting 'ab'):
it('treats a backslash-CRLF as a line continuation', () => {
const src = '{ name: "a\\' + '\r\n' + 'b", description: "d" }';
const { name } = parseWorkflowMetaLiteral(src) as { name: string };
expect(name).toBe('ab');
});中文说明
CRLF 反斜杠行续接分支(parseEscape() 中的 case '\r':,workflow-meta-literal.ts:327)没有测试——尽管本文件声明的方针是「每种转义形式都对照固定」(因为转义处理是手写解析器最容易出错的部分),这里却只固定了 \n 续接。具体代价:模型编写的 CRLF 换行 meta 字符串用反斜杠折行时,未来任何删除或破坏 \r 分支的改动都会绿灯通过:反斜杠+CRLF 要么被误拒为 unterminated string(随后的 \n 触发非模板换行检查),要么经恒等转义默认分支解出多余的 \r——现有测试没有一个会变红。已验证:删除该分支后 64 项测试仍全绿。建议在此用例旁补一个 CRLF 用例(源码中反斜杠后跟 CR LF,期望 'ab')。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (c === '{') return this.parseObject(depth); | ||
| if (c === '[') return this.parseArray(depth); | ||
| if (c === '"' || c === "'" || c === '`') return this.parseString(); | ||
| if (c === '-' || (c >= '0' && c <= '9')) return this.parseNumber(); |
There was a problem hiding this comment.
[Suggestion] Value dispatch only enters parseNumber on - or a digit, so the legal JS numeric spellings .5 and +5 are rejected with the generic unsupported value — while the parser internally accepts -.5 and 1., making the numeric grammar inconsistent with itself as well as with native JS; the rejection is a dispatch accident, not a principled subset boundary (the diagnostic's boilerplate even lists "numbers" as allowed) — Failure scenario: probe-verified — { name: 'w', description: 'd', threshold: .5 } throws unsupported value while native strict eval returns 0.5 and the deleted vm path loaded it; -.5 in the same parser is ACCEPTED (-0.5), proving the scanner already intends leading-dot decimals. extractAndStripMeta rethrows, so the whole workflow fails to load over legal JS. Nothing pins any of these spellings.
| if (c === '-' || (c >= '0' && c <= '9')) return this.parseNumber(); | |
| if (c === '-' || c === '.' || (c >= '0' && c <= '9')) | |
| return this.parseNumber(); |
A bare . still fails via the Number.isFinite backstop with the named numeric diagnostic. To also accept +5, additionally consume a leading + in parseNumber's sign arm (if (ch === '-' || ch === '+')).
中文说明
值分发只在 - 或数字处进入 parseNumber,因此合法 JS 数字写法 .5 与 +5 会被以笼统的 unsupported value 拒绝——而解析器内部却接受 -.5 和 1.,数字文法既与原生 JS 不一致、也自相矛盾;该拒绝是分发的意外,而非有原则的子集边界(诊断模板甚至把 "numbers" 列为允许项)。失败场景(probe 验证):{ name: 'w', description: 'd', threshold: .5 } 抛 unsupported value,而原生严格 eval 返回 0.5、被删除的 vm 路径也能加载;同一解析器对 -.5 却接受(-0.5),证明扫描器本就支持前导点小数。extractAndStripMeta 会重新抛出异常,于是整个 workflow 因合法 JS 而加载失败。这些写法没有任何测试固定。上方 suggestion 把 . 加入分发;孤立的 . 仍会经 Number.isFinite 兜底以具名数字诊断拒绝。若还想接受 +5,需在 parseNumber 的符号分支同时消费前导 +(if (ch === '-' || ch === '+'))。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const ID_START = /[A-Za-z_$]/; | ||
| const ID_PART = /[A-Za-z0-9_$]/; |
There was a problem hiding this comment.
[Suggestion] The unquoted-key grammar is ASCII-identifier-only, narrower than ECMAScript PropertyName: numeric keys and Unicode-identifier keys are legal JS that the deleted vm path accepted but this parser rejects — Concrete cost: probe-verified — native eval accepts {1:'x'}, {名前:'x'}, {café:'x'}; the parser rejects { 1: 'x', ... } and { 名前: 'x', ... } with expected a property name, and { café: 'x', ... } with expected ":" after key — having silently consumed caf as the key, so the diagnostic points at the wrong place. Numeric keys are among the most common JS object spellings, and CJK keys mirror the CJK-input realistic already accepted for the whitespace finding; the whole workflow fails to load over a key validateMeta would have dropped under the old path. Distinct from the ID_START-guard mutant finding (that one is about the rejection path being unpinned; this is over-rejection of legal JS). Quoted forms are accepted, so a recovery path exists — but the author must infer the quoting rule, and the accented-key diagnostic actively misdirects.
Either widen to Unicode identifiers (and accept numeric-literal keys in parseKey, delegating to the number scan):
const ID_START = /[\p{ID_Start}]/u;
const ID_PART = /[\p{ID_Continue}]/u;or explicitly document and test the ASCII-only restriction as intentional; pin { 1: 'x' } in the suite either way.
中文说明
不带引号的键的文法仅限 ASCII 标识符,比 ECMAScript 的 PropertyName 更窄:数字键与 Unicode 标识符键都是合法 JS,被删除的 vm 路径接受它们,而本解析器拒绝。具体代价(probe 验证):原生 eval 接受 {1:'x'}、{名前:'x'}、{café:'x'};解析器对 { 1: 'x', ... } 与 { 名前: 'x', ... } 报 expected a property name,对 { café: 'x', ... } 报 expected ":" after key——它已静默把 caf 消费为键,诊断指向错误的位置。数字键是最常见的 JS 对象写法之一,CJK 键则与空白字符发现中已接受的 CJK 输入场景同源;整个 workflow 会因一个在旧路径下 validateMeta 本会直接丢弃的键而加载失败。此项与 ID_START 守卫变异体发现不同(那一项是「拒绝路径未被测试固定」,这一项是「对合法 JS 的过度拒绝」)。带引号的形式可被接受,存在恢复路径——但作者必须自行悟出加引号的规则,且带重音键的诊断会主动误导。要么扩展为 Unicode 标识符(并让 parseKey 接受数字字面量键,委托给数字扫描),要么把「仅限 ASCII」作为有意限制写进文档并加测试;无论哪种,都应在套件中固定 { 1: 'x' }。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| it('accepts nesting up to the depth cap', () => { | ||
| const src = `{ a: ${'['.repeat(30)}${']'.repeat(30)} }`; |
There was a problem hiding this comment.
[Suggestion] The depth-cap tests do not pin the cap's boundary despite the test name: 30-vs-200 nesting constrains MAX_DEPTH only to [30, 199], while the real cap is 32 (probe: 32 nested arrays accepted, 33 rejected with meta literal is nested too deeply) — Concrete cost: a mutant sweep against the real test file shows MAX_DEPTH = 30, 31, 33 all leave both tests green (29 and 200 kill it), so an edit moving the cap anywhere inside [30, 199] ships unpinned — including one that changes how deep hostile model-authored input can recurse before the named WorkflowMetaSyntaxError fires. Same surviving-mutant class as the CRLF-continuation, ID_START-guard and exponent-branch gaps. (A claimed MAX_DEPTH=1000 survivor was checked and refuted: the 200-bracket test kills every mutant ≥ 200.)
Use the true boundary, e.g.:
it('accepts nesting at the depth cap and rejects just past it', () => {
const ok = `{ a: ${'['.repeat(32)}${']'.repeat(32)} }`;
const bad = `{ a: ${'['.repeat(33)}${']'.repeat(33)} }`;
expect(() => parseWorkflowMetaLiteral(ok)).not.toThrow();
expect(() => parseWorkflowMetaLiteral(bad)).toThrow(/nested too deeply/);
});(or export MAX_DEPTH and build both cases from it).
中文说明
尽管测试名为「接受直至深度上限的嵌套」,深度上限测试并未固定边界:30 对 200 的嵌套只把 MAX_DEPTH 约束到 [30, 199],而真实上限是 32(probe:32 层数组被接受,33 层被拒并报 meta literal is nested too deeply)。具体代价:对真实测试文件做变异扫描,MAX_DEPTH = 30、31、33 均使两个测试保持全绿(29 和 200 能杀死变异体),因此把上限改到 [30, 199] 内任何位置的改动都能不被固定地溜走——包括改变敌对的模型输入在触发具名 WorkflowMetaSyntaxError 之前能递归多深。与 CRLF 续接、ID_START 守卫、指数分支属于同一类「变异体存活」缺口。(曾被声称的 MAX_DEPTH=1000 存活变异体已核查并被证伪:200 层括号测试能杀死所有 ≥ 200 的变异体。)建议使用真实边界(或导出 MAX_DEPTH 并据此构造两个用例)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (this.src[this.index] === '-') this.index++; | ||
| while ( | ||
| this.index < this.src.length && | ||
| /[0-9]/.test(this.src[this.index]!) |
There was a problem hiding this comment.
[Suggestion] parseNumber silently accepts legacy-octal numeric spellings (010, 01, 007) and gives them a decimal value that no JavaScript reading of the source produces — strict mode (ESM, which workflow scripts are) is a SyntaxError, and sloppy mode (the realm of the deleted vm path) evaluates 010 as 8 — Failure scenario: probe-verified — parseWorkflowMetaLiteral('{ a: 010, description: "d" }') returns {"a":10}; native strict throws Octal literals are not allowed in strict mode.; sloppy/BASE-VM yields {"a":8}. A model-authored meta containing retries: 010 loads with value 10 where the replaced vm path read 8 and the script's own module semantics refuse to load at all. Latent end-to-end today because contract fields are strings and validateMeta drops non-contract fields — but the parser assigns a value no semantics of the source supports, contradicting its own case '0' rule, whose comment rejects \01 for exactly this reason.
After the scan, reject legacy octal shapes alongside the existing finiteness check, e.g.:
if (/^0\d/.test(text.replace(/^-/, ''))) {
throw this.fail('octal literals are not allowed in meta');
}(keeps 0, 0.5, 0e1, -0 legal).
中文说明
parseNumber 静默接受传统八进制数字写法(010、01、007),并赋予它们一个在任何一种 JS 语义下都不存在的十进制值——严格模式(workflow 脚本所处的 ESM)下是语法错误,非严格模式(被删除的 vm 路径所在的 realm)把 010 求值为 8。失败场景(probe 验证):parseWorkflowMetaLiteral('{ a: 010, description: "d" }') 返回 {"a":10};原生严格模式抛 Octal literals are not allowed in strict mode.;非严格/BASE-VM 得到 {"a":8}。模型写的 meta 若含 retries: 010,会以值 10 加载,而被替换的 vm 路径读作 8、脚本自身的模块语义则根本拒绝加载。目前端到端上是潜伏的(契约字段均为字符串,validateMeta 丢弃非契约字段)——但解析器赋予了一个任何源语义都不支持的值,且与自身 case '0' 规则相矛盾:那里的注释正是以同样理由拒绝 \01 的。建议在扫描后、现有有限性检查旁拒绝传统八进制形态(保持 0、0.5、0e1、-0 合法)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // Rejects `1n`, `0x10`, `1abc` — anything the scan above did not consume. | ||
| const after = this.src[this.index]; | ||
| if (after !== undefined && ID_PART.test(after)) { |
There was a problem hiding this comment.
[Suggestion] The trailing-garbage check that rejects 1n/1abc (ID_PART.test(after)) also catches _, so ES2021 numeric separators — legal JS — are rejected as unsupported numeric literal; the grammar comment lists no separator exclusion, so the rejection is collateral, not a named subset boundary — Failure scenario: probe-verified — { name: 'w', description: 'd', tokens: 1_000 } throws unsupported numeric literal pointing at the _, failing the workflow load; native strict eval returns {"tokens":1000} and the deleted vm path accepted it; 1_0.5e1_0 also threw while native accepts it. Numeric separators are a common model spelling for large counts/limits, and the module header's mandate covers "the JS spellings a model actually writes".
Either consume _ between digits during each digit scan (rejecting leading/trailing/double separators to match JS), e.g.:
while (this.index < this.src.length) {
const ch = this.src[this.index]!;
if (/[0-9]/.test(ch)) {
this.index++;
continue;
}
if (ch === '_' && /[0-9]/.test(this.src[this.index + 1] ?? '')) {
this.index++;
continue;
}
break;
}or, if separators are deliberately out of the documented subset, name the exclusion in the grammar comment next to the hex/bigint rejections so the refusal is principled rather than an ID_PART accident.
中文说明
用于拒绝 1n/1abc 的尾部垃圾检查(ID_PART.test(after))也会命中 _,于是 ES2021 数字分隔符——合法 JS——被当作 unsupported numeric literal 拒绝;文法注释并未列出任何分隔符排除项,因此该拒绝是连带伤害,而非有原则的具名子集边界。失败场景(probe 验证):{ name: 'w', description: 'd', tokens: 1_000 } 抛出指向 _ 的 unsupported numeric literal,workflow 加载失败;原生严格 eval 返回 {"tokens":1000},被删除的 vm 路径也接受它;1_0.5e1_0 同样被拒而原生接受。数字分隔符是模型书写大数值/限额时的常见写法,而模块头部的承诺覆盖「模型实际会写的 JS 写法」。要么在每段数字扫描中消费数字之间的 _(按 JS 规则拒绝前导/尾随/连续分隔符),要么如果分隔符确实不在文档化的子集内,就在文法注释中与 hex/bigint 的拒绝并列写明该排除,让拒绝有据可依,而不是 ID_PART 的意外。
— qwen3.8-max via Qwen Code /review (v0.21.13)
已被后续 commit 取代,当前 head 需重新 review
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Test Plan (not a blocker): 212 passed — this review observed 20162, 1471, 20936, 1571, 494, 3655, 504 passed; 698 passed — this review observed 20162, 1471, 20936, 1571, 494, 3655, 504 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/agents/runtime/workflow-meta-literal.test.ts:55 (+10 locations) — [probe] 12 unpinned parser behaviors in the new suite (surviving mutants)packages/core/src/agents/runtime/workflow-meta-literal.ts:2 — [review] New files carry a Google LLC copyright header; sibling convention is Qwenpackages/core/src/agents/runtime/workflow-sandbox.test.ts:223 — [review] Half-updated comment still asserts the deleted meta-eval vm contextpackages/core/src/agents/runtime/workflow-meta-literal.ts:139 — [probe] Bare undefined values now hard-error; undocumented narrowingpackages/core/src/agents/runtime/workflow-meta-literal.ts:325 — [probe] \u{...} capped at 6 hex digits; zero-padded legal escapes rejectedpackages/core/src/agents/runtime/workflow-meta-literal.ts:359 — [probe] Whitespace between unary minus and operand rejected with misleading diagnosticpackages/core/src/agents/runtime/workflow-meta-literal.ts:141 — [probe] Generic unsupported value where the docstring promises rejection by namepackages/core/src/agents/runtime/workflow-sandbox.ts:158 (+1 locations) — [review] Docstring claims every contract field is a string; phases is an arraypackages/core/src/agents/runtime/workflow-meta-literal.test.ts:18 — [probe] plain() test helper routes __proto__ through the prototype setterpackages/core/src/agents/runtime/workflow-meta-literal.test.ts:365 — [probe] Exported entry accepts non-object roots despite its {...} contractpackages/core/src/agents/runtime/workflow-meta-literal.ts:100 — [review] R1-4 trivia set diverges from ECMAScript WhiteSpace + LineTerminatorpackages/core/src/agents/runtime/workflow-meta-literal.ts:188 — [review] R1-6 test-efficacy mutant survived: ID_START guard unpinnedpackages/core/src/agents/runtime/workflow-meta-literal.ts:206 — [review] R1-7 get/set/async modifier lookahead misses backtick/digit startspackages/core/src/agents/runtime/workflow-meta-literal.ts:335 — [review] R1-8 identity-escape arm accepts \1–\9 (strict-mode SyntaxError natively)packages/core/src/agents/runtime/workflow-meta-literal.ts:187 — [review] R1-9 parseKey accepts template-literal keyspackages/core/src/agents/runtime/workflow-meta-literal.ts:359 — [review] R1-10 parseNumber exponent branches unpinned by any testpackages/core/src/agents/runtime/workflow-meta-literal.ts:127 — [review] R1-11 value dispatch rejects .5/+5 while accepting -.5/1.packages/core/src/agents/runtime/workflow-meta-literal.ts:74 — [review] R1-12 unquoted-key grammar ASCII-only (numeric/Unicode keys rejected)packages/core/src/agents/runtime/workflow-meta-literal.test.ts:304 — [review] R1-13 depth-cap tests do not pin the cap boundary (real cap 32)packages/core/src/agents/runtime/workflow-meta-literal.ts:344 — [review] R1-14 parseNumber accepts legacy-octal spellings with decimal values- …and 1 more (see the run report)
中文说明
仅完成部分审查,审查缺口已披露。
未审查:反向审计——在 5 轮的反审轮数上限内未收敛。
Test Plan(非阻断):212 passed — this review observed 20162, 1471, 20936, 1571, 494, 3655, 504 passed; 698 passed — this review observed 20162, 1471, 20936, 1571, 494, 3655, 504 passed。
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 21 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is queued in workflow run. |
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No blocking issues. LGTM! ✅
Test Plan (not a blocker): 212 passed — this review observed 1471, 20936, 1571, 494, 3655, 504 passed; 698 passed — this review observed 1471, 20936, 1571, 494, 3655, 504 passed.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/agents/runtime/workflow-meta-literal.test.ts:189 — [probe] raw U+2028/U+2029 preservation in template strings is unpinned — a cooking mutation would pass the whole suitepackages/core/src/agents/runtime/workflow-meta-literal.ts:72 — [probe] WHITESPACE embeds U+00A0/U+FEFF as raw invisible characters — silent Set-dedupe riskpackages/core/src/agents/runtime/workflow-meta-literal.test.ts:281 — [probe] escaped \${ template form parses correctly but is unpinned, one character from the rejected ${ casepackages/core/src/agents/runtime/workflow-meta-literal.ts:392 — [probe] 1e309 overflow rejected by the Number.isFinite guard; native strict JS accepts Infinitypackages/core/src/agents/runtime/workflow-meta-literal.ts:176 — [probe] duplicate __proto__ keys accepted (last wins); native strict mode throws SyntaxError
中文说明
无阻断问题。LGTM!✅
Test Plan(非阻断):212 passed — this review observed 1471, 20936, 1571, 494, 3655, 504 passed; 698 passed — this review observed 1471, 20936, 1571, 494, 3655, 504 passed。
收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 5 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Resolves the branch's only conflict, in packages/core/src/agents/runtime/workflow-sandbox.ts: both sides added a different import at the same position — this branch added `parseWorkflowMetaLiteral` from './workflow-meta-literal.js', main added `stripAnsiAndControl` from '../../utils/textUtils.js'. Both symbols are used in the merged file, so both imports are kept; there is no semantic overlap between the two changes. The PR was MERGEABLE=CONFLICTING/DIRTY and has carried an APPROVED verdict (round 3, 0 findings) since 2026-08-18 without being mergeable. Verified on the merged tree: - packages/core `tsc --noEmit`: clean (0 errors) - packages/core `npm run build`: success - `npx vitest run src/agents/runtime/workflow-sandbox.test.ts src/agents/runtime/workflow-meta-literal.test.ts`: 231 passed (2 files)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Test Plan (not a blocker): 212 passed — this review observed 1614, 22275, 1616, 494, 3849, 553 passed; 698 passed — this review observed 1614, 22275, 1616, 494, 3849, 553 passed.
中文说明
未发现问题。LGTM!✅
Test Plan(非阻断):212 passed — this review observed 1614, 22275, 1616, 494, 3849, 553 passed; 698 passed — this review observed 1614, 22275, 1616, 494, 3849, 553 passed。
— qwen3.8-max via Qwen Code /review (v0.21.14)
…ion (QwenLM#9340) * feat(review): say when the approach, not the patch, is the open question Every finding /review emits is anchored to a `file:line` in the current diff. That is what a finding is — and it means a review can report where an approach leaks, but never that a different approach would retire all of the leaks at once. Measured: one change to `extractAndStripMeta` took three attempts across two PRs. QwenLM#9097 (3 rounds, 18 findings) added a timeout to the vm call; QwenLM#9136 (6 rounds, 56 findings) moved the walk inside the vm and ended up spawning a child process per call, growing 228 -> 920 source diff lines. QwenLM#9325 landed it in one commit by not evaluating the literal at all. All 74 findings were individually correct, and every one of them went away with the mechanism. The signal was already there and filed as the wrong kind of thing: `did not converge within the reverse-audit round cap` appeared four times across the two PRs, as a coverage gap — "we did not finish looking" — rather than as a conclusion about the change. Nothing was responsible for reading it as "stop patching". Add one advisory paragraph, and one clause on the terminal verdict line, when a non-Approve round is past the round threshold AND its source diff has grown at least 3x since the review first measured it. This round's round-cap stop rides along as corroborating text when present; it is never a trigger on its own. It is deliberately not a finding. Findings are what the autofix loop consumes, and that loop patching each finding in turn is the pattern being interrupted — a finding here would be fixed rather than read. It addresses the human deciding what happens next, so it is a body paragraph and a verdict-line clause, it adds no cap, and it never moves the event. The baseline is a baseline, not the previous round's size: 228 -> 920 across six rounds is ~1.3x per round, which no per-round delta would notice, but 4.0x cumulatively. `Ledger.src0` records the first measurement and is carried forward unchanged, so a diff that later shrinks cannot rewrite its own baseline. It is the one marker field that survives truncation — the ruling that withholds an anchor from a partial finding list does not extend to a measurement of the diff. Known limits, documented rather than papered over: it cannot see across pull requests, so the three-attempt shape that motivated it would have fired only on a second forgeable persisted counter; and it is retroactively blank, staying silent until a PR has posted two rounds after this ships. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(review): suppress approach signal for downgraded approvals * fix(review): measure approach growth over full diff * fix(review): validate approach signal evidence * fix(review): pin approach-signal boundaries and validator coverage Round-5 review findings: boundary tests for the round threshold, growth factor, and source-diff floor; the round-cap corroborating clause and its zh rendering; src0 survival through the pr-context persist seam and the incremental marker carry-forward; artifact validator refusal/absence tests for approachSignal; design doc firing list names the pre-cap verdict. * fix(review): clamp the approach signal's round at the ledger cap (R9-1) The signal computed its displayed round with an unclamped `prevRound + 1` while the ledger marker stamp and the deferred-suggestions clause both clamp with `Math.min(prevRound + 1, LEDGER_MAX_ROUND)`. `parseLedger` accepts `round == LEDGER_MAX_ROUND`, so a side file at the cap is representable and carries forward: one composed body announced "⚠️ Round 10001" beside a marker stamping `"round":10000`, and the terminal verdict line printed 10001 too — the doc comment in this same diff claims all three consumers cannot disagree "at the cap included". The new test pins the cap for the third consumer, mirroring the existing deferred-clause cap test; mutation-verified that reverting the clamp turns it red with `round: 10001`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: qqqys <266654365+qqqys@users.noreply.github.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
|
Released in v0.22.2. |
What this PR does
Replaces the vm evaluation of a workflow script's
export const meta = {...}block with a static parser. A meta literal that would hang, allocate without bound, or schedule a rejection is now refused on syntax, because none of those things can be written in the grammar a parser accepts.Supersedes #9136 (and #9097 before it) — see "How this got here".
Why it's needed
extractAndStripMetaevaluated the model-authored meta literal in a vm realm and then walked the resulting value on the host. Both halves are unbounded, and each one hangs the host process on its own:The loops are synchronous, so the event loop is blocked outright: no timer fires, no signal handler runs, and nothing left in the process can cancel it. Running the second form under vitest does not even produce a test timeout, because the worker's own timeout timer cannot fire.
Bounding this turned out to be a moving target. A
timeouton the vm call does not reach a getter that runs later on the host. Moving the walk into the vm fixes the getter but leaves promise reactions, proxy traps and runaway allocation. Each fix invited the next finding.Meta is a declaration, not a computation. Every field in the contract —
name,description,whenToUse,phases[].title/detail/model— is a string. Upstream states the rule outright: the meta object must be a pure literal, with no variables, function calls, spreads or template interpolation. Given that contract, evaluating it was the wrong mechanism from the start.So this parses it. A parser has no execution semantics, so a loop, a getter, a proxy trap, a promise reaction and a large allocation are not bounded — they are unrepresentable. There is nothing to time out, sandbox or isolate. The vm context, the timeout and the thenable walker are all deleted.
The grammar is JSON's value grammar plus the spellings a model actually writes: unquoted keys, single-quoted and substitution-free template strings, trailing commas, and comments. Anything that would mean "evaluate something" — an identifier, a call, a spread, a computed key, an accessor, a method, a template substitution, a regex — is rejected by name, so the diagnostic tells the author which rule they hit rather than emitting a generic syntax error.
What this narrows
This is a deliberate contract change, not a pure refactor. A meta block that computed a value used to work if the computed field sat outside the contract surface, because
validateMetadropped unknown fields silently. Now the whole literal is refused.The size of that surface was measured rather than assumed. Every
export const metaliteral in the existing test suite (42 of them) was run through both the old vm path and the parser:Of the ten the parser refuses, nine are the hostile fixtures above (getter loops, IIFEs,
Promise.resolve,import(), spreads, cycles, a 200 KB string, a scope probe). The tenth is{ name: 'x', pattern: /\{[a-z]+\}/g }— a regex in a non-contract field, which used to be dropped silently and is now an error naming the rule. No workflow scripts ship in this repo, so nothing in tree changes behaviour.The zero row is the important one: there is no case where both paths accept a literal and produce different values.
Cost
Parsing the literal is roughly 70× cheaper than the vm path it replaces (~6µs vs ~450µs on a representative meta block). That matters beyond microbenchmarks, because this call sits on the path to two surfaces that do not exist yet but are the reason the hang mattered: rendering a workflow's name and phases in the tool-confirmation dialog, and reading each saved workflow's description when the slash-command palette is built at startup.
How this got here
#9097 proposed the minimal fix — a
timeouton the existingrunInContext. Review correctly found it bounds only the literal's own evaluation, and the getter case above walks straight past it. #9136 then ran both halves inside the vm as two separate programs, which fixes the getter and the scope-sharing problem, but six review rounds kept finding further evaluation hazards (looping promise reactions undermicrotaskMode, allocation) and the branch ended up spawning a child process per extraction — ~31ms each, and ~920 lines for a{name, description}contract object.At that point the question stopped being "how do we bound this evaluation" and became "why are we evaluating it". This PR answers the second question. Both predecessors are closed.
Reviewer Test Plan
How to verify
cd packages/core && npx vitest run src/agents/runtime/workflow-meta-literal.test.ts src/agents/runtime/workflow-sandbox.test.ts— 212 passed. Whole workflow runtime plus the Workflow tool:npx vitest run src/agents/runtime/ src/tools/workflow/— 698 passed, 6 skipped.workflow-meta-literal.test.tsis new (64 cases): the contract shape, the accepted JS spellings, every string escape form pinned against the equivalent JS literal (this is the fiddliest part of a hand-rolled parser, so\n \t \r \b \f \v \0 \\ \/ \xNN \uNNNN \u{...}and identity escapes each get a case, plus rejections for octal, truncated and out-of-range forms), one rejection case per executable construct, the depth cap, and prototype safety for__proto__/constructor.In
workflow-sandbox.test.ts, 141 of 148 tests are untouched. Seven changed, and the diff is worth reading:process/require) still reject; only the message changed, and the comments now say why — there is no scope to escape from, because no identifier is a value.To see the failure being removed, revert
workflow-sandbox.tsand run the getter fixture: it does not fail, it hangs, and--testTimeoutcannot rescue it because the worker's event loop is blocked.Evidence (Before & After)
N/A — no user-visible or TUI change.
Tested on
Environment (optional)
Unit tests only, Node 22.23.0 on Linux. Unlike the child-process approach in #9136, nothing here is platform-dependent — there is no spawn, no signal handling and no timeout.
Risk & Scope
findMetaBlockBounds(which locates the literal),validateMeta(which checks the contract fields), the script body's own execution, or any other vm use in the sandbox. It does not document the pure-literal rule in the model-facing tool description — that belongs with the authoring-contract work, and the parser's own error messages state the rule at the point of failure in the meantime.Linked Issues
Supersedes #9136 and #9097.
中文说明
这个 PR 做了什么
把 workflow 脚本
export const meta = {...}块的 vm 求值换成静态解析。会挂死、会无界分配、会遗留 rejection 的 meta 字面量,现在在语法层面就被拒绝——因为这些东西在解析器接受的文法里根本写不出来。取代 #9136(以及更早的 #9097)——见下方"事情是怎么走到这一步的"。
为什么需要
extractAndStripMeta会在 vm realm 里对模型编写的 meta 字面量求值,然后在宿主侧遍历结果。两个阶段都没有边界,而且各自都能单独卡死宿主进程:这些循环是同步的,事件循环被直接阻塞:定时器不触发,信号处理器不运行,进程内没有任何东西能取消它。第二种形式在 vitest 下甚至不会产生测试超时,因为 worker 自己的超时定时器也无法触发。
给它加边界后来被证明是个移动靶。vm 调用上的
timeout管不到之后在宿主上运行的 getter;把遍历挪进 vm 解决了 getter,却仍留下 promise reaction、proxy trap 和失控分配。每修一处,就招来下一处发现。meta 是声明,不是计算。 契约里的每个字段——
name、description、whenToUse、phases[].title/detail/model——都是字符串。上游把规则写得很明白:meta 对象必须是纯字面量,不允许变量、函数调用、spread 或模板插值。既然契约如此,一开始就不该用求值这个机制。所以改成解析。解析器没有执行语义,于是死循环、getter、proxy trap、promise reaction、大块分配都不是"被框住了"——而是无法表达。没有东西需要超时、沙箱或隔离。vm context、超时、thenable 遍历器全部删除。
文法是 JSON 的值文法,加上模型实际会写的那些写法:不带引号的键、单引号、无替换的模板字符串、尾逗号、注释。任何意味着"执行点什么"的东西——标识符、调用、spread、计算键、访问器、方法、模板替换、正则——都会被具名拒绝,让诊断信息直接告诉作者撞了哪条规则,而不是抛一个笼统的语法错误。
这收窄了什么
这是有意的契约变更,不是纯重构。过去,如果计算出来的字段落在契约之外,带计算的 meta 是能跑的,因为
validateMeta会静默丢弃未知字段。现在整个字面量都会被拒。这个变更面是实测出来的,不是估计的。现有测试套件里全部 42 个
export const meta字面量,同时走旧的 vm 路径和新解析器:被解析器拒绝的 10 个里,9 个正是上面那些敌对样本(getter 死循环、IIFE、
Promise.resolve、import()、spread、循环引用、200 KB 字符串、作用域探测)。第 10 个是{ name: 'x', pattern: /\{[a-z]+\}/g }——一个非契约字段里的正则,过去被静默丢弃,现在会报出具名错误。仓库里没有任何随附的 workflow 脚本,因此树内没有行为变化。那个 0 才是关键:不存在"两条路径都接受、但产出不同值"的情况。
开销
解析比它替换掉的 vm 路径快约 70 倍(代表性 meta 块上 ~6µs vs ~450µs)。这不只是微基准的意义,因为这个调用正处在两个尚未存在、但恰恰是"挂死为何要紧"之原因的路径上:在工具确认对话框里渲染 workflow 的名称与阶段,以及在启动构建斜杠命令面板时读取每个已保存 workflow 的描述。
事情是怎么走到这一步的
#9097 提的是最小修复——给已有的
runInContext加timeout。评审正确指出它只框住了字面量自身的求值,上面那个 getter 案例会直接穿过去。#9136 于是把两个阶段都放进 vm,作为两个独立程序,解决了 getter 和作用域共享问题;但六轮评审不断找出新的求值风险(microtaskMode下循环的 promise reaction、分配),分支最终演变成每次提取都 spawn 一个子进程——每次约 31ms,为一个{name, description}契约对象写了约 920 行。到这一步,问题就不再是"怎么给这次求值加边界",而是"我们为什么要求值"。本 PR 回答的是第二个问题。两个前身 PR 都已关闭。
审阅者验证方案
如何验证
cd packages/core && npx vitest run src/agents/runtime/workflow-meta-literal.test.ts src/agents/runtime/workflow-sandbox.test.ts——212 项通过。完整 workflow 运行时加 Workflow 工具:npx vitest run src/agents/runtime/ src/tools/workflow/——698 项通过、6 项跳过。workflow-meta-literal.test.ts是新增的(64 个用例):契约形状、被接受的 JS 写法、每一种字符串转义形式都对照等价的 JS 字面量固定(这是手写解析器最容易出错的地方,因此\n \t \r \b \f \v \0 \\ \/ \xNN \uNNNN \u{...}与恒等转义各有一例,另加八进制、截断、越界形式的拒绝用例)、每一种可执行构造各一个拒绝用例、深度上限,以及__proto__/constructor的原型安全。workflow-sandbox.test.ts里 148 个测试有 141 个未改动。改动的七个值得一读:process/require)仍然拒绝,只是错误信息变了;注释现在说明了为什么——没有作用域可逃逸,因为标识符根本不是一种值。想看被消除的故障:回退
workflow-sandbox.ts后跑 getter 那个样本——它不会失败,它会挂起,而且--testTimeout救不了,因为 worker 的事件循环被阻塞了。证据(前后对比)
N/A——没有用户可见或 TUI 层面的变化。
测试环境
环境(可选)
仅单元测试,Linux 上的 Node 22.23.0。与 #9136 的子进程方案不同,这里没有任何平台相关的部分——没有 spawn、没有信号处理、没有超时。
风险与范围
findMetaBlockBounds(定位字面量)、validateMeta(校验契约字段)、脚本体自身的执行,或 sandbox 中任何其他 vm 用法。它也没有把纯字面量规则写进面向模型的工具描述——那属于「编写契约」那项工作;在此之前,解析器自己的错误信息会在失败点直接陈述该规则。关联 Issue
取代 #9136 与 #9097。