-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(core): bound the workflow meta evaluation #9136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 2 commits
4d249cc
8a4b227
aec7704
e49a3b5
908ea66
ab0ba69
c09ce9b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -266,6 +266,210 @@ describe('extractAndStripMeta', () => { | |||||||
| expect(() => extractAndStripMeta(src)).toThrow(/unbalanced/i); | ||||||||
| }); | ||||||||
|
|
||||||||
| // The meta literal is model-authored source, and every caller reaches it on | ||||||||
| // a path where a wedged thread is unrecoverable: the run path (before the | ||||||||
| // sandbox's own 30s body timeout is armed) and, in follow-up work, the tool | ||||||||
| // confirmation dialog and the saved-workflow palette. A field value that | ||||||||
| // never returns must surface as an ordinary malformed-meta error. | ||||||||
| // | ||||||||
| // Each case asserts a generous wall-clock bound rather than a precise one, | ||||||||
| // so the assertion stays stable on a loaded CI runner. Without the bound | ||||||||
| // these hang the worker until vitest's own timeout kills it. | ||||||||
| describe('bounded evaluation', () => { | ||||||||
| const BOUND_MS = 5_000; | ||||||||
|
|
||||||||
| function timed(fn: () => unknown): number { | ||||||||
| const startedAt = Date.now(); | ||||||||
| try { | ||||||||
| fn(); | ||||||||
| } catch { | ||||||||
|
Comment on lines
+297
to
+301
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R6-1: witness: unmodified tree: 10 timed tests 563-576ms each; inline-measurement fix on one test: 563ms → 285ms, still passes. const startedAt = Date.now();
expect(() => extractAndStripMeta(src)).toThrow(/failed to evaluate meta object literal/);
expect(Date.now() - startedAt).toBeLessThan(BOUND_MS);中文说明
— qwen3.8-max via Qwen Code /review (v0.21.12) |
||||||||
| /* the throw is asserted separately */ | ||||||||
| } | ||||||||
| return Date.now() - startedAt; | ||||||||
| } | ||||||||
|
|
||||||||
| it('bounds a field value that loops on evaluation', () => { | ||||||||
| const src = `export const meta = { name: (function () { while (true) {} })(), description: 'd' }\nreturn 1`; | ||||||||
| expect(() => extractAndStripMeta(src)).toThrow( | ||||||||
| /failed to evaluate meta object literal/, | ||||||||
| ); | ||||||||
| expect(timed(() => extractAndStripMeta(src))).toBeLessThan(BOUND_MS); | ||||||||
| }); | ||||||||
|
|
||||||||
| // The case a timeout on the literal's own evaluation does NOT catch: a | ||||||||
| // getter defers its work to property-read time, so the literal itself | ||||||||
| // evaluates instantly and only spins when the value is walked. Walking on | ||||||||
| // the host would run it on the host thread, unbounded. | ||||||||
| it('bounds a getter that loops when the value is walked', () => { | ||||||||
| const src = `export const meta = { name: 'x', description: 'd', get phases() { while (true) {} } }\nreturn 1`; | ||||||||
| expect(() => extractAndStripMeta(src)).toThrow( | ||||||||
| /failed to evaluate meta object literal/, | ||||||||
| ); | ||||||||
| expect(timed(() => extractAndStripMeta(src))).toBeLessThan(BOUND_MS); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('bounds a getter nested inside phases', () => { | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Two wedge shapes named in superseded PR 9097's Critical review have no regression test, though this PR positions itself as the converged replacement of that feedback. Probe-verified both are bounded today: a Fix: add two cases to the 中文说明被取代的 PR 9097 的 Critical 评审中点名的两种卡死形态没有回归测试,尽管本 PR 将自己定位为针对该反馈的收敛替代方案。已探测验证两者目前都有界: 修复:在 — qwen3.8-max via Qwen Code /review (v0.21.11)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 已修复:新增无限循环 then getter 的超时回归,以及 phases 自定义 Symbol.iterator 不被调用的回归。验证:workflow-sandbox.test.ts 167/167 通过。 |
||||||||
| const src = `export const meta = { name: 'x', description: 'd', phases: [{ get title() { while (true) {} } }] }\nreturn 1`; | ||||||||
| expect(() => extractAndStripMeta(src)).toThrow( | ||||||||
| /failed to evaluate meta object literal/, | ||||||||
| ); | ||||||||
| expect(timed(() => extractAndStripMeta(src))).toBeLessThan(BOUND_MS); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('refuses a meta literal that exceeds the serialized size cap', () => { | ||||||||
| const src = `export const meta = { name: 'x'.repeat(200000), description: 'd' }\nreturn 1`; | ||||||||
| expect(() => extractAndStripMeta(src)).toThrow( | ||||||||
| /failed to evaluate meta object literal/, | ||||||||
| ); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('enforces the cap after JSON escaping', () => { | ||||||||
| const src = `export const meta = { name: '\\0'.repeat(20000), description: 'd' }\nreturn 1`; | ||||||||
| expect(() => extractAndStripMeta(src)).toThrow( | ||||||||
| /failed to evaluate meta object literal/, | ||||||||
| ); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('enforces the cap for string-free containers', () => { | ||||||||
| const src = `export const meta = { name: 'x', description: 'd', phases: Array.from({ length: 40000 }, () => ({})) }\nreturn 1`; | ||||||||
| expect(() => extractAndStripMeta(src)).toThrow( | ||||||||
| /failed to evaluate meta object literal/, | ||||||||
| ); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('bounds a looping then getter', () => { | ||||||||
| const src = `export const meta = { name: 'x', description: 'd', extra: { get then() { while (true) {} } } }\nreturn 1`; | ||||||||
| expect(() => extractAndStripMeta(src)).toThrow( | ||||||||
| /failed to evaluate meta object literal/, | ||||||||
| ); | ||||||||
| expect(timed(() => extractAndStripMeta(src))).toBeLessThan(BOUND_MS); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('does not invoke a phases iterator', () => { | ||||||||
| const src = `export const meta = { | ||||||||
| name: 'x', | ||||||||
| description: 'd', | ||||||||
| phases: Object.assign([{ title: 'one' }], { | ||||||||
| [Symbol.iterator]: function () { while (true) {} }, | ||||||||
| }), | ||||||||
| }\nreturn 1`; | ||||||||
| expect(extractAndStripMeta(src).meta?.phases).toEqual([{ title: 'one' }]); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('leaves a well-formed meta literal unaffected', () => { | ||||||||
| const src = `export const meta = { name: 'w', description: 'd', phases: [{ title: 'One' }] }\nreturn 1`; | ||||||||
| const { meta } = extractAndStripMeta(src); | ||||||||
| expect(meta).toEqual({ | ||||||||
| name: 'w', | ||||||||
| description: 'd', | ||||||||
| phases: [{ title: 'One' }], | ||||||||
| }); | ||||||||
| }); | ||||||||
| }); | ||||||||
|
|
||||||||
| // The literal is evaluated as its own program, so it never shares a lexical | ||||||||
| // scope with the serializer that walks it. Interpolating it into the | ||||||||
| // serializer's scope would let it read the helpers and overwrite the flag | ||||||||
| // that decides whether a thenable was found — i.e. disarm the check that | ||||||||
| // keeps a stray rejected Promise from killing the host process. | ||||||||
| it('meta source cannot observe or mutate the serializer scope', () => { | ||||||||
| const src = `export const meta = { name: String(typeof copy) + ':' + String(typeof hasThenable), description: 'd' }\nreturn 1`; | ||||||||
| const { meta } = extractAndStripMeta(src); | ||||||||
| expect(meta?.name).toBe('undefined:undefined'); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('a thenable stays rejected even when the literal predefines the flag name', () => { | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The test title promises a scenario its fixture never sets up. Fix: put the claimed scenario in the fixture, still asserting const src = `export const meta = { name: 'x', description: 'd', hasThenable: false, phases: Promise.resolve(1) }\nreturn 1`;中文说明测试标题承诺了其 fixture 并未构造的场景。 修复:把标题声称的场景放进 fixture,仍然断言 — qwen3.8-max via Qwen Code /review (v0.21.11)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 已修复:测试 fixture 现显式设置 hasThenable: false,覆盖预定义标志仍不能隐藏 Promise 的场景。验证:workflow-sandbox.test.ts 167/167 通过。 |
||||||||
| const src = `export const meta = { name: 'x', description: 'd', hasThenable: false, phases: Promise.resolve(1) }\nreturn 1`; | ||||||||
|
Comment on lines
+589
to
+590
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R5-13: Because the fixture's hostile value is a real Promise ( 中文说明由于 fixture 的敌意值是真实 Promise( — qwen3.8-max via Qwen Code /review (v0.21.12) |
||||||||
| expect(() => extractAndStripMeta(src)).toThrow( | ||||||||
| /meta values must not be Promises/, | ||||||||
| ); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('isolates serializer intrinsics from the meta literal', () => { | ||||||||
| const src = `export const meta = { | ||||||||
| name: (JSON.stringify = () => ({ toString() { return '@'; } }), 'x'), | ||||||||
| description: 'd', | ||||||||
| }\nreturn 1`; | ||||||||
| expect(extractAndStripMeta(src).meta).toEqual({ | ||||||||
| name: 'x', | ||||||||
| description: 'd', | ||||||||
| }); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('rejects Promises after the literal mutates serializer helpers', () => { | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R5-13: Because the fixture's hostile value is a real Promise ( Fix: make the hostile value a non-Promise thenable so the hook never fires — 中文说明由于 fixture 的恶意值是真实 Promise( 修复:把恶意值改为非 Promise 的 thenable,使钩子永不触发—— — qwen3.8-max via Qwen Code /review (v0.21.12) |
||||||||
| const src = `export const meta = { | ||||||||
| name: ( | ||||||||
| Object.keys = () => ['name', 'description'], | ||||||||
| Promise.prototype.then = () => undefined, | ||||||||
| 'x' | ||||||||
| ), | ||||||||
| description: 'd', | ||||||||
| extra: Promise.resolve(1), | ||||||||
| }\nreturn 1`; | ||||||||
| expect(() => extractAndStripMeta(src)).toThrow( | ||||||||
| /meta values must not be Promises/, | ||||||||
| ); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('does not expose host helpers to serializer getters', () => { | ||||||||
| const src = `export const meta = { | ||||||||
| name: 'x', | ||||||||
| description: 'd', | ||||||||
| extra: Object.defineProperty({}, 'value', { enumerable: true, get: function () { | ||||||||
| const serializerGlobal = arguments.callee.caller.constructor('return globalThis')(); | ||||||||
| if (serializerGlobal.__qwenWorkflowMetaIsPromise) throw new Error('host helper exposed'); | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R2-4: This isolation probe checks
Suggested change
(or delete the stale probe if the adjacent scope-isolation tests are deemed sufficient.) 中文说明[Suggestion] R2-4:这个隔离探测检查的是 — qwen3.8-max via Qwen Code /review (v0.21.11)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 已修复。验证证据:workflow-sandbox 183/183 通过;隔离探测现检查 serializer realm 的 copy helper。 |
||||||||
| return 'safe'; | ||||||||
| } }), | ||||||||
| }\nreturn 1`; | ||||||||
| expect(extractAndStripMeta(src).meta).toEqual({ | ||||||||
| name: 'x', | ||||||||
| description: 'd', | ||||||||
| }); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('prefers a Promise error after the size budget is exceeded', () => { | ||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R5-11: The only test meant to pin the parent-side Fix: change the fixture's Promise to a plain thenable so the envelope path is exercised — 中文说明唯一意在固定宿主侧"hasThenable 先于 tooLarge"信封优先级的测试走的是子进程 async-hooks 路径:fixture 创建了真实 Promise( 修复:把 fixture 的 Promise 改为普通 thenable,使信封路径真正被执行—— — qwen3.8-max via Qwen Code /review (v0.21.12) |
||||||||
| const src = `export const meta = { | ||||||||
| name: 'x'.repeat(200000), | ||||||||
| description: 'd', | ||||||||
| extra: Promise.resolve(1), | ||||||||
|
Comment on lines
+687
to
+691
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] R5-11: The only test meant to pin the parent-side 中文说明唯一旨在固定宿主侧 — qwen3.8-max via Qwen Code /review (v0.21.12) |
||||||||
| }\nreturn 1`; | ||||||||
| expect(() => extractAndStripMeta(src)).toThrow( | ||||||||
| /meta values must not be Promises/, | ||||||||
| ); | ||||||||
| }); | ||||||||
|
|
||||||||
| it('copies shared phase objects at each array position', () => { | ||||||||
| const src = `export const meta = { | ||||||||
| name: 'x', | ||||||||
| description: 'd', | ||||||||
| phases: (function () { | ||||||||
| const phase = { title: 'one' }; | ||||||||
| return [phase, phase]; | ||||||||
| })(), | ||||||||
| }\nreturn 1`; | ||||||||
| expect(extractAndStripMeta(src).meta?.phases).toEqual([ | ||||||||
| { title: 'one' }, | ||||||||
| { title: 'one' }, | ||||||||
| ]); | ||||||||
| }); | ||||||||
|
|
||||||||
| it.each([ | ||||||||
| [ | ||||||||
| `export const meta = { name: 'x', description: 'd', whenToUse: () => {} }\nreturn 1`, | ||||||||
| /meta.whenToUse must be a string/, | ||||||||
| ], | ||||||||
| [ | ||||||||
| `export const meta = { name: 'x', description: 'd', phases: [{ title: 'one', detail: Symbol('x') }] }\nreturn 1`, | ||||||||
| /meta.phases\[\].detail must be a string/, | ||||||||
| ], | ||||||||
| [ | ||||||||
| `export const meta = { name: 'x', description: 'd', phases: [{ title: 'one', model: 1n }] }\nreturn 1`, | ||||||||
| /meta.phases\[\].model must be a string/, | ||||||||
| ], | ||||||||
| ])('preserves invalid optional field types for validation', (src, error) => { | ||||||||
| expect(() => extractAndStripMeta(src)).toThrow(error); | ||||||||
| }); | ||||||||
|
|
||||||||
| // P4a adversarial review (HIGH × 3 lenses): the docstring at | ||||||||
| // workflow-sandbox.ts:283-294 promises the returned meta is HOST-realm — | ||||||||
| // a per-field copy that defends against T1/T8/T14-style vm-realm escape | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] R5-1:
BOUND_MS = 5_000is looser than the child outer kill (META_CHILD_TIMEOUT_MS = 2_000), so no test pins the 250ms per-script vm timeout (META_EVAL_TIMEOUT_MS) — verified by mutation: withMETA_EVAL_TIMEOUT_MS = 1900all 17 bounded-evaluation tests still pass, per-case durations swelling from ~565ms to 3,866–3,881ms. — Failure scenario: a future refactor drops or inflatestimeout: META_EVAL_TIMEOUT_MS; every malformed meta literal then blocks the calling thread ~2–3.9s (outer SIGKILL + error-path drain) instead of ~0.25–0.6s on the run path, and CI stays green.Fix: for the cases resolved by the vm timeout (all except the native-builtin case, which intentionally exercises the outer kill), assert a bound between the two timeouts, e.g.
expect(timed(...)).toBeLessThan(1_500)— generous vs the production worst case (~2×250ms + spawn overhead ≈ 0.6s) but belowMETA_CHILD_TIMEOUT_MS, so losing the per-script timeout turns the suite red.中文说明
BOUND_MS = 5_000比子进程外层强杀(META_CHILD_TIMEOUT_MS = 2_000)更宽松,因此没有任何测试固定 250ms 的单脚本 vm 超时(META_EVAL_TIMEOUT_MS)——变异验证:把META_EVAL_TIMEOUT_MS改为 1900 后,17 个 bounded-evaluation 测试全部通过,单例耗时从约 565ms 膨胀到 3,866–3,881ms。— 故障场景:未来重构删除或调大timeout: META_EVAL_TIMEOUT_MS后,每个恶意/畸形 meta 字面量都会在运行路径上阻塞调用线程约 2–3.9 秒(外层 SIGKILL + 错误路径排空),而不是约 0.25–0.6 秒,且 CI 保持绿色。修复:对由 vm 超时收口的用例(除刻意验证外层强杀的 native builtin 用例外)断言一个介于两层超时之间的上界,例如
expect(timed(...)).toBeLessThan(1_500)——相对生产最坏情况(约 2×250ms + 启动开销 ≈ 0.6s)留有余量,又低于META_CHILD_TIMEOUT_MS,这样单脚本超时一旦丢失套件即红。— qwen3.8-max via Qwen Code /review (v0.21.12)