feat(loader): support constructed values and nested calls#3067
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReflection parsing now supports nested call targets, ordered mutation execution, recursive ChangesReflection and typed value resolution
Virtual resource loading
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant SceneFormatV2Test
participant ReflectionParser
participant CallOrderComponent
SceneFormatV2Test->>ReflectionParser: parse mutation block
ReflectionParser->>CallOrderComponent: apply resolved props
ReflectionParser->>CallOrderComponent: invoke target method
CallOrderComponent-->>SceneFormatV2Test: expose updated state
sequenceDiagram
participant ResourceManager
participant MainAssetPromise
participant GLTFLoader
ResourceManager->>GLTFLoader: load main asset
GLTFLoader-->>MainAssetPromise: complete main asset
ResourceManager->>MainAssetPromise: resolve sub-asset path
MainAssetPromise-->>ResourceManager: return sub-asset
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3067 +/- ##
===========================================
+ Coverage 79.61% 79.64% +0.03%
===========================================
Files 904 904
Lines 101208 101220 +12
Branches 11404 11428 +24
===========================================
+ Hits 80579 80619 +40
+ Misses 20445 20417 -28
Partials 184 184
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts`:
- Around line 165-167: Move the standalone "$args" validation immediately after
obj is created in ReflectionParser, before branches handling "$ref", "$class",
"$entity", "$component", or "$signal", so any object containing "$args" without
"$type" is rejected. Add a test covering a mixed discriminator such as "$class"
with "$args" and assert it fails.
- Around line 43-57: Update the call traversal and invocation logic in
ReflectionParser to reject the property names "constructor", "prototype", and
"__proto__" in every call.target segment and in call.method before accessing or
invoking them. Return a rejected Promise using the existing validation-error
pattern, while preserving valid nested method resolution and invocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d5ce96ab-e890-4807-80ee-e87475a9bfd1
📒 Files selected for processing (3)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.tspackages/loader/src/schema/CommonSchema.tstests/src/loader/SceneFormatV2.test.ts
| let target = instance; | ||
| if (call.target !== undefined) { | ||
| if (!Array.isArray(call.target) || call.target.some((key) => typeof key !== "string" || key.length === 0)) { | ||
| return Promise.reject(new Error(`Call "${call.method}" target must be an array of non-empty strings`)); | ||
| } | ||
| for (const key of call.target) target = target?.[key]; | ||
| } | ||
| const method = target?.[call.method]; | ||
| if (typeof method !== "function") { | ||
| return Promise.reject(new Error(`Call target does not have method "${call.method}"`)); | ||
| const path = call.target?.length ? `${call.target.join(".")}.` : ""; | ||
| return Promise.reject(new Error(`Call target does not have method "${path}${call.method}"`)); | ||
| } | ||
|
|
||
| return Promise.all((call.args ?? []).map((arg) => this._resolveValue(arg))) | ||
| .then((resolvedArgs) => Promise.resolve(method.apply(instance, resolvedArgs))) | ||
| .then((resolvedArgs) => Promise.resolve(method.apply(target, resolvedArgs))) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Block prototype and constructor traversal before invoking nested calls.
Loader-controlled paths can traverse function properties and reach Function, for example target: ["nested", "setValue"] with method: "constructor". Combined with call.result, this permits constructing and invoking arbitrary JavaScript.
Reject constructor, prototype, and __proto__ in both target segments and method names.
Proposed guard
+ const forbiddenKeys = new Set(["constructor", "prototype", "__proto__"]);
let target = instance;
if (call.target !== undefined) {
if (!Array.isArray(call.target) || call.target.some((key) => typeof key !== "string" || key.length === 0)) {
return Promise.reject(new Error(`Call "${call.method}" target must be an array of non-empty strings`));
}
+ if (call.target.some((key) => forbiddenKeys.has(key)) || forbiddenKeys.has(call.method)) {
+ return Promise.reject(new Error(`Call "${call.method}" contains a forbidden target key`));
+ }
for (const key of call.target) target = target?.[key];
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts`
around lines 43 - 57, Update the call traversal and invocation logic in
ReflectionParser to reject the property names "constructor", "prototype", and
"__proto__" in every call.target segment and in call.method before accessing or
invoking them. Return a rejected Promise using the existing validation-error
pattern, while preserving valid nested method resolution and invocation.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
审查 @ ceba5a347（首轮）— feat(loader): support constructed values and nested calls
总结
给 v2 场景 ReflectionParser 补三块运行时反序列化能力：① $type 支持递归解析的 $args 构造参数；② call.target 支持把方法调用打到嵌套属性路径的 owner 上；③ $number: "Infinity" 的 JSON-safe 正无穷哨兵；④ 把 parseMutationBlock 从「props/calls 并发无序」改成「props 先、calls 后」的确定性顺序。方向完全正确，是 #3061 那版错 base 的 d59c6392d 收敛到 dev/2.0 纯 Loader 改动后的干净版本。逐链核对无 P0/P1/P2，可合并。
逐链核对（非信 commit message / PR 描述）
- props-before-calls 顺序是收紧而非回退：原
Promise.all([props, calls])的 JSDoc 明确写「without imposing ordering」（来自 #2959284cce8e1），但那只是描述行为、未给出「必须无序」的理由。改成 props→calls 是确定性收紧：老行为并发交错，没有任何合法数据能依赖「calls 先于 props」，所以本改动严格更安全，且符合语义直觉（props 配置字段状态，calls 在其上动作，如addBurst需先配好emission）。走 Git 历史透镜确认无矛盾——不是还原任何fix:。 - 顺序测试反向可证伪：
should finish resolving props before executing calls构造精巧——$ref被 mock 成延迟 promise，await Promise.resolve()放过一个微任务后才 resolve。新序下 call 捕获"ready"；revert 回Promise.all则 call 在 ref 解析前就跑、捕获""，测试 fail。真守回归。 - particle 恢复测试是端到端真链路：
Burst($args:[0.5, curve, 2, 0.1])/ParticleCurve($args:[CurveKey,CurveKey])/CurveKey($args:[0,1])/ParticleCompositeCurve($args:[8])的构造签名逐一核对全部对齐真实构造函数，且经call.target:["generator","emission"] + addBurst/clearBurst走公开 API。这正是本特性的动机——editor 需要 round-trip 需构造参数 + 经方法（非纯 props）设置的粒子曲线/burst。reverse-falsifiable。 call.target绑定正确：method.apply(target, ...)打到遍历后的 owner；call.target === undefined时target = instance、target?.[call.method]退化为原instance?.[call.method]，行为等价。错误消息 path 拼接在 target 校验之后，安全。- schema 新增类型非死代码：
TypeValue/SpecialNumberValue在CommonSchema.ts无 parser 消费者，但这与既有ClassRef（同样零消费者）/Vec3Tuple/EntityRef完全一致——该文件的职责就是声明 v2 格式的规范词汇表，不是必须被 import 才算活。按项目惯例非冗余。 - 注释合规：更新后的
_resolveValue优先级 JSDoc 已同步补入$number(#4)/$type,$args?(#5) 重编号，与实际分支顺序一致；parseMutationBlockJSDoc 由「without imposing ordering」改为「Apply props before executing calls」——非 aspirational，与代码一致。CI 全绿（build×3 / codecov patch 88.37% + project 79.61% / prettier）。
已评估的 CodeRabbit 两条 finding（均非阻塞，据实校准严重度）
-
CR「Critical: prototype/constructor 遍历可达 RCE」→ 实为 P3 defensive、非新洞。核心事实：
call.target:["fn"], method:"constructor"能拿到Function进而call.result链式 RCE——但这条可达性早于本 PR：parseCalls从 #2959 起就有instance[call.method]+call.result → parseMutationBlock递归，{method:"constructor", result:{calls:[{method:"constructor", result:{calls:[{method:"call", args:[...]}]}}]}}早已可达Function并执行任意代码。本 PR 的target遍历只是便利路径，未引入新能力类。且 v2 场景数据是 editor 产出（与 glTF/scene 资产同信任级、等同 app 代码），非系统边界的不可信输入。CR 的 Critical 定级在此信任模型下过高。鉴于本 PR 恰好动到这段遍历代码，加一个constructor/prototype/__proto__黑名单守卫是合理的 defense-in-depth，但属可选加固，不阻塞。const forbidden = new Set(["constructor", "prototype", "__proto__"]); if (call.target?.some((k) => forbidden.has(k)) || forbidden.has(call.method)) { return Promise.reject(new Error(`Call "${call.method}" contains a forbidden key`)); }
问题
-
[P3] ReflectionParser.ts:165-167 — 独立
$args校验位置偏后（CR Minor，据实保留为 P3）。if ("$args" in obj) reject排在$ref/$class/$entity/$component/$signal分支之后，故{ $class:"Foo", $args:[] }会先命中$class分支、静默丢弃$args。editor 不会产出这种混合 discriminator，属畸形输入容错的小缺口。若要收紧，把该校验挪到obj创建后、所有 discriminator 分支之前，并补一条$class+$args的混合用例。不阻塞。 -
[P3] ReflectionParser.ts —
$number仅覆盖正无穷。当前只支持$number:"Infinity"（对应 Burst 无限循环 cycles），显式 rejectNaN。-Infinity暂无字段需要故未支持——YAGNI 合理。仅记录：若将来出现需持久化-Infinity的字段（某些下界），此哨兵需扩展。不阻塞。
简化建议
无。代码干净，抽象克制（$args 复用 _resolveValue 递归、call.target 与既有 parseCalls 无缝衔接），无过度设计。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
审查 @ 84b614111（第二轮，增量）— fix(loader): decode infinite burst cycles
总结
自上轮 ceba5a347 起线性追加一个 commit（parent 就是 ceba5a347，非 force-push）。改动只做一件事：删掉上轮刚引入的通用 $number: "Infinity" 哨兵，改成在 _resolveValue 的 $type 构造分支里对 $type === "Burst" 且 args[2] 做位置化、字符串化的特判——"Infinity" → Infinity，其余非 null 的非正整数 → reject。逻辑正确、反向证伪测试到位、CI 全绿（build×3 / codecov patch+project / e2e 4/4 / lint），可合并。但方向上把一个通用能力换成了单类硬编码特判，值得一议——记 [P2]，不阻塞。
逐链核对（增量）
Burst.$args[2]确是cycles：Burst构造签名(time, count, cycles?, repeatInterval?)（Burst.ts:51），args[2]位置对齐。cycles === Infinity是EmissionModule.ts:312的真运行时哨兵（maxCycles = cycles===Infinity ? ceil((duration-burstTime)/repeatInterval) : cycles），JSON 无法表达Infinity（JSON.stringify(Infinity)==="null"）故需 sentinel——需求真实。- 守卫逻辑穷举无洞：
"Infinity"→Infinity✓ /2→过 ✓ /"NaN"→非"Infinity"非null且非number→throw ✓（测试覆盖）/null|undefined→!=null短路跳过→构造函数Math.max(cycles??1,1)=1✓ /1.5/0/-Infinity→非正整数→throw ✓。 - 反向证伪成立：测试改用
"Infinity"串断言bursts[0].cycles === Infinity，并新增"NaN"用例断言rejects.toThrow(...)；revert 特判即红。 - Git 历史透镜无矛盾：
$number是本 PR 上一 commit（ceba5a347）自己引入的，全仓无第二处引用、从未发版/被消费——删它不是还原任何fix:，是作者重做自己刚加的机制。 - 注释合规：
_resolveValue优先级 JSDoc 从 10 条重编号回 9 条（去掉$number），与实际分支顺序（$ref3/$type4/$class5/$entity6/$component7/$signal8/plain 9）逐条一致，无残留。SpecialNumberValueinterface 同步删除。
问题
-
[P2] ReflectionParser.ts:130-137 — 通用
Infinity编码退化成Burst单类位置特判，是「类型枚举代替特征」+ 抽象泄漏。上轮的$number: "Infinity"虽然只覆盖正无穷（我上轮记为 P3-YAGNI），但它是格式层的通用 JSON-safe 数值编码——任何 prop / ctor arg 都能用。本轮把它换成if ($type === "Burst" && args.length > 2)对args[2]的字符串特判，埋进了本该类型无关的_resolveValue分发器里，带来两点结构代价:Infinity本质是通用序列化问题，不是 Burst 问题。同仓Joint.breakForce/breakTorque默认就是Infinity（Joint.ts:27-28，对应 Unity「不可断裂关节」惯例）且是 public 可序列化属性（Joint.ts:157/171）；ResourceManager.timeout、request.defaultTimeout同样默认Infinity。这些字段将来要经 v2 round-trip 时，通用$number本可覆盖，现在每个都得往 parser 里再加一条特判。新类型要 infinity → 必须改 parser——正是「加白名单，漏补即 bug」的信号。- 泛型分发器嵌入了单个引擎类的构造参数语义（硬编码「
Burst的第 3 个位置参是cycles」）。Burst构造顺序一旦调整，这里静默失效，且无编译期护栏。
另外，
"Infinity"这个魔法串契约在 schema 里零文档——TypeValue.$args?: unknown[]完全没提Burst的第三参接受字符串"Infinity"，producer（editor companion PR #3793）与 reader 全靠隐式约定。格式契约与 editor 共有、且处于 alpha 无兼容包袱——正是选对编码的最佳时机。建议保留通用 sentinel 方向（哪怕仍只实现正无穷），让「JSON-safe 特殊数值」成为格式层一等公民、与具体类解耦；Burst 侧只需在
$args里放该 sentinel 即可，parser 不必认识Burst。若坚持只服务 Burst，也至少把"Infinity"串契约写进CommonSchema.ts的TypeValue/ Burst 相关注释，别让它只活在 parser 实现里。不阻塞。
已在上轮闭环、本轮不重提
- CodeRabbit 在本 commit 重贴的两条（
constructor/prototype遍历可达 RCE = Critical；独立$args校验位置偏后 = Minor）我上轮均已评估：前者是早于本 PR 的既有可达性 + editor 信任模型下的可选加固（本轮 traversal 代码未变），后者我上轮已记为 P3。二者状态未变，不重复。
简化建议
若采纳上面 P2 的「恢复通用 sentinel」方向，_resolveValue 里那段 8 行 Burst 特判可整段删除，换成一条与类无关的 $number（或等价）分支，净复杂度更低且覆盖面更广——这本身就是更简的解。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
审查 @ 1307f43d（第三轮，增量）— fix(asset): make virtual sub-asset loading reliable
总结
自上轮 84b614111 起线性追加一个 commit（parent 就是 84b614111，非 force-push）。这一 commit 换了完全不同的子系统——不再动 ReflectionParser，而是修 ResourceManager 的 sub-asset 加载竞态 + 把 editor-only 的 initVirtualResources 重命名成 public registerVirtualResources。竞态修复本身方向正确、是根因层修复、反向证伪测试到位。但本 commit 顺手做的一个 Object→object 类型收窄改动破坏了 tsc，CI 全红（build×3 / codecov / e2e 4/4 全挂在同一条 TS2322），必须先修才能合。记 [P0]。
逐链核对（增量）
- 竞态修复是根因层、非输出端擦屁股：eager 回调（
_onSubAssetSuccesspush）是 fast path，但_loadMainAsset完成时会_releaseSubAssetPromiseCallback(baseURL)整表删除；一个在这个 cleanup 边界之后才注册的 sub-asset 查询会永久 pending（notes 里meshes[0][1]症状）。新修法给每个 sub-promise 额外挂一条mainPromise.then(r => resolve(_getResolveResource(r, _parseQueryPath(subPath))))——以权威主资源为兜底，解析不再依赖脆弱的 push 时序/cleanup 边界。这是对的落点：eager 是优化，main promise 是 source-of-truth。 - eager 值 vs 兜底值一致：对
meshes[0][1]，eager 侧GLTFParserContext:194emit 的是item[i]（ModelMesh），同一.then里:187又把glTFResource.meshes[index]=item写回；兜底侧mainResource.meshes[0][1]===item[1]=== 同一 mesh。二者 resolve-once 竞争但产出等值，无分叉。 - 兜底还顺带修了进度：
mainPromise.onProgress(setTaskCompleteProgress, setTaskDetailProgress)把主资源进度转发进 sub-promise（onProgress返回this已核）——旧 eager-only 路径下 pending 的 sub-promise 是不报进度的，这里一并接上了。 - 反向证伪成立：新测 mock loader
AssetPromise.resolve(mainAsset)不 emit 任何 eager 回调，load(".glb?q=materials[0]")唯一能 resolve 的路径就是新兜底；revert 掉mainPromise.then(...)块则永久 pending → timeout fail。真守回归。 - API rename 无破坏：
initVirtualResources保留为委托 shim（转调registerVirtualResources），in-engine 唯一调用点ProjectLoader.ts:22不受影响。旧实现里那段if (element.dependentAssetMap) this._map[vp].dependentAssetMap = element.dependentAssetMap是 no-op（_map[vp]赋值后就是element本身，等于自赋值），删掉行为不变——作者删对了。 - Git 历史透镜：
Object[]返回类型来自既有代码、非任何fix:引入，本轮改它不是还原历史 bug；但改错了（见下）。
问题
-
[P0] ResourceManager.ts:91 —
loadoverload 返回类型从Object[]收窄成object[]破坏tsc，全 CI 红。本 commit 把AssetPromise<T | Object[]>改成AssetPromise<T | object[]>（同时:47改_assetUrlPool的Object→object，那一处无害）。但:97-98的多资源分支AssetPromise.all(promises)产出AssetPromise<T[]>：T是无约束泛型可为原始类型，string[]不可赋给object[]（object排除原始类型），旧的Object[]（含装箱原始类型）则可赋。于是tsc报：src/asset/ResourceManager.ts(98,5): error TS2322: Type 'AssetPromise<T[]>' is not assignable to type 'AssetPromise<T | object[]>'.这条确定性错误让 build (mac/ubuntu/win) + codecov + e2e 1~4/4 全部失败（同一根因，
packages/core b:types: tscexit 2）。上一 commit84b614111用Object[]时 CI 全绿，故是本 commit 引入的纯类型回归。PR 描述里"full Loader type build is blocked by stale workspace deps ... none point to touched files"——说明作者本地根本没跑出干净的 coretsc，把这条漏了过去。修法：
:91返回类型改回Object[]（最小改动），或若坚持用object则改成T | object[]无法涵盖T[]，需要把签名写成能容纳数组分支的形态（如AssetPromise<T | T[]>配合两条 overload）。最省事就是回退:91这一处为Object[]，:47的object可保留。
已在前两轮闭环、本轮不重提
- CodeRabbit 在本 tip 重贴的两条（
constructor/prototype遍历可达 = Critical；独立$args校验位置偏后 = Minor）我前两轮均已评估并闭环（前者=早于本 PR 的既有可达性+editor 信任模型下的可选加固、traversal 代码本轮未变；后者=我已记 P3）。状态未变，不重复。 - 上轮 [P2]（
Burst"Infinity"单类特判 vs 通用 sentinel）本 commit 未触碰 ReflectionParser，状态不变，不重提。
简化建议 / 观察（不阻塞）
- [P3]
notes/loader/2026-07-15-*.md：basedev/2.0上无notes/目录，本 PR 新引入的开发者工作笔记。同 #3039 惯例，这类 notes 不属于提交树、建议移除（不阻塞，仅记录，勿反复纠缠）。 - [P3] 兜底路径对真不存在的 sub-path（
_getResolveResource无边界检查）会resolve(undefined)而非 reject。旧 eager-only 路径下这种 query 本就永久 stranded，所以谈不上回归、反而不再 hang；但静默返回undefined对调用方不友好，若要收紧可在 walk 到undefined时 reject 一个"sub-asset not found: {path}"。不阻塞。 - PR 粒度：本 PR 已从"loader
$type/$args/nested calls"扩到"ResourceManager sub-asset 竞态 + virtual resource public API"两个正交变更单元。回滚粒度变粗、注意力稀释。提一次即可，不强求拆分。
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts (1)
43-57: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winBlock prototype and constructor traversal before invoking nested calls.
Loader-controlled paths can traverse function properties and reach
FunctionorObject.prototype, for example viatarget: ["nested", "setValue"]ormethod: "constructor". Combined withcall.result, this permits constructing and invoking arbitrary JavaScript or polluting prototypes.Reject
constructor,prototype, and__proto__in both target segments and method names. Note that the validation forcall.methodmust be positioned outside thecall.targetcondition to ensure it is always evaluated.🔒 Proposed guard
chain = chain.then(() => { + const forbiddenKeys = new Set(["constructor", "prototype", "__proto__"]); + if (forbiddenKeys.has(call.method)) { + return Promise.reject(new Error(`Call method "${call.method}" is forbidden`)); + } let target = instance; if (call.target !== undefined) { if (!Array.isArray(call.target) || call.target.some((key) => typeof key !== "string" || key.length === 0)) { return Promise.reject(new Error(`Call "${call.method}" target must be an array of non-empty strings`)); } + if (call.target.some((key) => forbiddenKeys.has(key))) { + return Promise.reject(new Error(`Call target contains a forbidden key`)); + } for (const key of call.target) target = target?.[key]; } const method = target?.[call.method];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts` around lines 43 - 57, Update the call handling in ReflectionParser around target traversal and method lookup to reject “constructor”, “prototype”, and “__proto__” in every call.target segment and in call.method. Place method-name validation before the call.target conditional so it always runs, while preserving existing target array validation and invocation behavior for safe names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@packages/loader/src/resource-deserialize/resources/parser/ReflectionParser.ts`:
- Around line 43-57: Update the call handling in ReflectionParser around target
traversal and method lookup to reject “constructor”, “prototype”, and
“__proto__” in every call.target segment and in call.method. Place method-name
validation before the call.target conditional so it always runs, while
preserving existing target array validation and invocation behavior for safe
names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7ac69fe8-e090-4ace-b70a-6dd145c7c380
📒 Files selected for processing (5)
notes/loader/2026-07-15-runtime-special-number-contract.mdpackages/core/src/asset/ResourceManager.tspackages/loader/src/resource-deserialize/resources/parser/ReflectionParser.tspackages/loader/src/schema/CommonSchema.tstests/src/loader/SceneFormatV2.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/loader/src/schema/CommonSchema.ts
- tests/src/loader/SceneFormatV2.test.ts
- packages/core/src/asset/ResourceManager.ts
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
审查 @ be65ff7e（第四轮，增量）— refactor(loader): make special number decoding generic — ✅ 三条历史 finding 全闭环，解除门控
总结
自上轮 1307f43d 起线性追加一个 commit（parent 就是 1307f43d，非 force-push）。这一 commit 精准地一次性关闭了我此前三轮遗留的三条 finding，且改法与我给的方向逐字一致：① 修掉上轮 [P0] 的 tsc 回归；② 采纳上轮 [P2]，把 Burst 单类特判换回通用 $number sentinel；③ 采纳首轮 [P3]，把 $args 独立校验前移到所有 discriminator 分支之前。逐链核对无新 P0/P1/P2，build×3 已转绿（上轮红的根因已消除），可合并。解除我上轮的 --request-changes 门控。
逐链核对（增量）
-
[P0 上轮] tsc 回归已修：
ResourceManager.ts:91AssetPromise<T | object[]>→AssetPromise<T | T[]>。多资源分支AssetPromise.all(promises)产出AssetPromise<T[]>现在可赋给T | T[]（旧object[]因object排除原始类型故不可赋）；单资源分支_loadSingleItem返回AssetPromise<T>同样可赋。两分支均 type-check，build (mac/ubuntu/win) 三平台已 pass，实证根因消除。注：T | T[]是实现签名（对外由两条 overloadload<T extends EngineObject>/load<T extends EngineObject[]>决定），实现签名对调用方不可见，故无 API 破坏。 -
[P2 上轮] Burst 单类特判 → 通用 sentinel，逐字采纳我的建议：删掉
_resolveValue里if ($type === "Burst" && args.length > 2)那 8 行位置化特判，恢复为类型无关的$number分支（_resolveValue#4）。运行时结果等价：particle 测试$args:[0.5, curve, {$number:"Infinity"}, 0.1]→ 第 3 arg 经_resolveValue解出Infinity→new Burst(...,Infinity,...)→bursts[0].cycles===Infinity（测试 :444 反向证伪）。泛型分发器不再认识Burst的构造参数语义——正是上轮 P2 要消除的抽象泄漏。删掉的'Burst $args[2] must be...'校验丢失不是新问题：它是本 PR 自己上一 commit84b614111引入的、从未发版；且 source-v2 数值合法性由 builder/compiler 上游负责（PR 描述明载「Builder owns source-v2 validation」），runtime reader 本就不该做单类校验——这正是采纳 P2 的必然结果，与我上轮立场一致，不再提。 -
[P3 首轮]
$args独立校验前移已修：if ("$args" in obj && !("$type" in obj)) reject提到所有 discriminator 分支之前（:104）。现在{$class, $args}会先命中该校验而 reject（测试 :947 覆盖），不再静默丢$args。前移到$ref之前也顺带收紧了{$ref, $args}这类畸形输入（editor 不产出，无回归）。!("$type" in obj)正确放过合法的{$type, $args}。 -
$number校验穷举无洞：Object.keys(obj).length !== 1 || obj.$number !== "Infinity"→ reject。{$number:"Infinity"}→Infinity✓ /{$number:"NaN"}→reject ✓（测试 :950）/{$number:"Infinity", x:1}→keys=2→reject ✓ /{$number:-Infinity}（真数值）→!=="Infinity"→reject ✓。 -
[P3 round-2] sentinel 契约已文档化：
SpecialNumberValue { $number: "Infinity" }以带 JSDoc 的 interface 回到CommonSchema.ts，与同文件ClassRef/TypeValue的单行/** */风格一致；零消费者与ClassRef同属该「格式词汇表」文件的既有惯例，非死代码。notes文件亦把 rejected-alternative 逐条记录。 -
注释合规：
parsePropsJSDoc(:16) 与_resolveValue优先级 JSDoc(:86-96) 均同步补入$number并重编号至 10 条，与实际分支顺序逐条一致；$args-guard 作为前置校验门未编号（合理，它非解析分支）。CI lint 绿。
已在前几轮闭环、本轮不重提
- CodeRabbit 的
constructor/prototype遍历可达（Critical）+ 独立$args校验（Minor，已被本 commit 修）——前三轮均已评估闭环，traversal 代码本轮未变，状态不变。 $number仅覆盖正无穷（YAGNI）——notes 文件已显式记录边界决策，非回归。
观察（不阻塞，勿反复纠缠）
- [P3]
notes/loader/2026-07-15-runtime-special-number-contract.md仍是不属于提交树的开发者笔记（同 #3039 惯例）。上轮已记，仅一次性提示，不再重复。
无新阻塞项。此前 --request-changes 门控随本 approve 解除。
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
审查 @ b8ce4dfa（第六轮，同 HEAD 架构/冗余/性能专项深扫）— 无 P0/P1，1×P2 + 6×P3，均不阻塞
总结
HEAD 未变（b8ce4dfa3），本轮不是增量审查，而是对整个 PR 做的架构、上下游冗余、性能专项深扫：ResourceManager 异步生命周期做了 eager 注册表 × mainPromise fallback 的全交错穷举（每种时序下用户 promise 恰好 settle 一次、注册表清理不变量成立、无新增 unhandled rejection、fallback 的 _parseQueryPath 重解析与被 shift() 消耗前的 paths 等价）；ReflectionParser 对 $args/$number/call.target/props 顺序做了全输入形态复核。结论：无新 P0/P1，可合并状态不变。 分层判定正确：builder 管 source-v2 语义校验/lowering，runtime 只做格式边界 grammar 校验，不构成双源冗余；eager + authoritative fallback 双路径合理（eager 提供主资源完成前的早解析，fallback 保 liveness）。唯一的真冗余见下 [P2]，删三行白赚。
问题
-
[P2] ResourceManager.ts:419-421 —
mainPromise.catch((e) => this._onSubAssetFail(...))已被本 PR 的 authoritative fallback 彻底架空，唯一残余效果是写入毒化重试的 stale stash，建议直接删除这 3 行。逐链证据：- handler 按注册顺序执行：
_loadMainAsset的 cleanup（load 创建时注册，最早）→ 此 catch（每个 sub 请求注册）→ fallback（:493，最晚）。主资源失败时 cleanup 总是先_releaseSubAssetPromiseCallback删掉整个 baseURL 注册表； - 因此此 catch 里的
_onSubAssetFail永远查不到 pending callback，只会走 else 分支写入一条永不清理的{rejectedValue}stash（cleanup 已跑过，不会再删它）； - 后果：主资源失败后重试同一
?q=URL，_createSubAssetPromiseCallback:473命中 stale stash 瞬间拒绝——哪怕网络已恢复、fresh main load 已作为副作用启动且会成功（fresh load 完成后其 cleanup 才清掉 stash，之后的请求才正常）； - pending sub promise 的失败传播已由 fallback 的 reject（
:493第二参）正确完成，错误对象同源（同一 main rejection），删 catch 无任何行为损失；loader 主动上报的 eager_onSubAssetFail（如 glb 整体成功但单个子资源失败）是另一条独立路径，不受影响。
据实标注：stash 毒化重试是 pre-existing（merge-base 上这段逐字节相同），且 pre-PR 此 catch 也从未成功 reject 过 pending entry——reject 先被 cleanup 删掉、promise 挂死，正是本 PR 所修 strand bug 的失败侧。故这不是本 PR 引入的回归，而是本 PR 让这条路由的存在价值归零：删除 = 纯简化 + 顺带修复既有的重试毒化，一举两得。不阻塞。
- handler 按注册顺序执行：
-
[P3] ReflectionParser.ts:48-53 —
call.target中间段为 undefined 时错误消息误导。for (const key of call.target) target = target?.[key]无逐段校验，中间段断裂（如 schema 版本漂移致generator不存在）与"方法真不存在于合法 owner"产出同一条Call target does not have method "generator.emission.addBurst"——实际死在generator，方法本身在emission上存在。失败仍是响亮 reject，仅调试误导。若收紧：逐段 walk 时报Call target path segment "X" is unresolved。 -
[P3] ReflectionParser.ts:140-142 —
$type无$args时白付Promise.all([])+ 一跳 microtask。旧代码在既有.then内同步new Class()；现在 constructorArgs 为空也绕道Promise.all(空数组.map())。千级$type值的大场景（粒子模块/曲线/渐变/key）合计毫秒级纯浪费。加if (constructorArgs.length === 0)直构快路径即可，$args路径保持不变。 -
[P3] ResourceManager.ts:493-500 — fallback 在 eager 已 settle 后仍无条件重跑解析。主资源完成时，每个已被 eager 提前 resolve 的 sub promise 的 fallback continuation 仍会执行
_parseQueryPath（regex.replace）+_getResolveResource遍历，再对已 settle 的 promise 调 resolve（no-op）。数百 sub-asset 的 glb 加载完成瞬间低个位 ms 纯浪费。查注册表项是否已清或 promise 是否已 settle 即可短路。 -
[P3] ReflectionParser.ts:78 — props→calls 串行化把「解析」与「应用」耦合，损失 block 内取资源并行度（bounded，仅记录）。顺序保证本身正确且必要（本轮维持首轮判定），但确定性只要求应用有序，不要求解析有序：现在 call args 里的
$ref网络加载排在全部 props resolve 之后，props/calls 各引用一个慢资产的 block 加载延迟近似翻倍。跨组件并行仍保留（HierarchyParser:160 整体Promise.all），故影响有界。将来若加载延迟敏感，标准解法是两阶段「全量并行解析 → 顺序应用」。现在不必动。 -
[P3] ResourceManager.ts:590 —
registerVirtualResources转公开 API 但无 unregister/clear 对偶。@remarks 自己宣传的正是 object-URL 资源包场景：包卸载 +URL.revokeObjectURL后，_virtualPathResourceMap留 stale 映射（全 tree 无任何 delete/clear 写点），后续load("virtual/path")撞 revoked blob URL 报晦涩网络错而非清晰的 "not registered"；反复切包时 map 单调增长。转公开的时刻正是定义完整生命周期的时刻，建议补unregisterVirtualResources(paths)或在 TSDoc 明示注册为会话级不可卸载。 -
[P3] ResourceManager.ts:628 —
initVirtualResourcesshim 无@deprecated标记，ProjectLoader.ts:22 仍调旧名。双入口对同一操作永久共存且无退场信号。一行修复：@deprecated Use {@link registerVirtualResources}+ ProjectLoader 迁移到新名。
简化建议
删 :419-421 三行（见 P2）——这是本轮唯一的结构性收益点，其余均为微优化。性能面已验证全部改动均在加载/解析路径，零每帧影响。
无阻塞项。前四轮 approve 立场不变，保持可合并状态。
|
对第六轮 review 中 [P3]「 消费者普查（engine tip 树 + editor 仓 + builder 导出模板 + 相关一方仓库）支持直接删除：
删除依据： （可选项，与第六轮 P3 的 unregister 议题相关：单入口落定后，若资源包运行时消费者尚未落地， |
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
审查 @ 7f2bcbaa（第七轮，增量）— fix(asset): allow sub-asset retries after failure — 一次性关闭上轮两条遗留 finding，无新问题
总结
自上轮 b8ce4dfa 起线性追加一个 commit（gh api commits/7f2bcbaa 报 parent = b8ce4dfa，非 force-push；merge-base(dev/2.0)=b7bf6f7c，真实 PR delta 仍是 7 文件的 loader/ResourceManager/schema 单元）。这一 commit 精准关闭我上轮记录的两条 [P3]/[P2]，且落点比我给的建议更彻底（根因层删除而非最小止血）。逐链核对无新 P0/P1/P2/P3，CI 全绿（build×3 / codecov / e2e 4/4 / lint 全 pass），保持可合并状态。ReflectionParser.ts/CommonSchema.ts 自 b8ce4dfa 起零改动，无需复审。
逐链核对（增量）
-
[P2 上轮] 毒化重试 stash → 根因层删除（比我建议的「删 3 行 catch」更彻底）。上轮我只提议删
:419-421的mainPromise.catch，作者直接把整套rejectedValuestash 机制端掉：删_onSubAssetFail整个方法（-15行）、删mainPromise.catch(e => _onSubAssetFail(...))、删_createSubAssetPromiseCallback里读rejectedValue的分支（现在只判resolvedValue，命中走AssetPromise.resolve(resolvedValue)）、删SubAssetPromiseCallbacks类型里的rejectedValue?: Error字段。净 -26 行。移除完整性核实：_onSubAssetFail全 tip 树仅一个调用点（就是被删的那个 catch）——两个 loader（RenderTargetLoader.ts:51/GLTFParserContext.ts:194/198/206）在 tip 上只调_onSubAssetSuccess，零_onSubAssetFail。故上轮我说的「loader 主动上报的 eager_onSubAssetFail是另一条独立路径」据实修正为不成立：根本没有这条 loader 侧路径，_onSubAssetFail是纯内部单点死码，删除无任何行为损失。 -
失败传播完整保留：pending sub-promise 的主资源失败仍由
_createSubAssetPromiseCallback:474的mainPromise.onProgress(...).then(resolve-fn, reject)第二参reject正确完成。无 unhandled rejection：mainPromise的两个消费者（_loadMainAsset:426的promise.then(_, fail)+ 此处.then(_, reject)）各自 handled，被删的mainPromise.catch是唯一 orphan 分支——删掉反而更干净。 -
retry-after-failure 生命周期逐帧核对（旧序 vs 新序）：
- 旧序毒化根因：主失败时三 handler 按注册序 fire——①
_loadMainAsset的 release handler（最早，在_loadMainAsset内注册）_releaseSubAssetPromiseCallback删整表 → ②mainPromise.catch → _onSubAssetFail查不到（表已删）走 else 重建{rejectedValue}stash（cleanup 已跑过、永不再删）→ ③ sub-promise reject。重试同 URL 时_createSubAssetPromiseCallback先命中 stalerejectedValue瞬间拒绝，哪怕 loader 已被重新调用且会成功。 - 新序：stash 机制整体消失 → 主失败后
_releaseSubAssetPromiseCallback删表即终结，无残留 → 重试时loadingPromises[baseURL]已删 →_loadMainAsset重新调 loader（第二个 mock 成功）→_subAssetPromiseCallbacks[baseURL]为 undefined（已释放）→ 无resolvedValue→ fresh 注册 →mainPromise.then解出mainAsset.materials[0]=material。✅
- 旧序毒化根因：主失败时三 handler 按注册序 fire——①
-
反向证伪成立：新测
retries a sub-asset after its main asset initially fails用mockReturnValueOnce(reject).mockReturnValueOnce(resolve)——首loadawait rejects.toThrow()，次load(url)断言equal(material)+loaderSpy恰好 2 次。revert 修复（恢复rejectedValuestash + catch）后：旧序 stash 在 cleanup 之后重建，次load命中 stale stash 瞬拒 →equal(material)断言 fail。loaderSpy2 次那条不区分（旧序 loader 也会被重调），但equal(material)那条真守回归。测试构造精确。 -
[P3 上轮]
initVirtualResourcesshim@deprecated+ ProjectLoader 迁移已修。:601补@deprecated Use {@link registerVirtualResources}（多行 JSDoc、末尾句号合规）；ProjectLoader.ts:22从initVirtualResources(data.files)迁到registerVirtualResources(data.files)，且删掉了那行上方冗余的// @ts-ignore（public API 不再需要）。engine 仓内initVirtualResources现零调用（唯一调用点已迁移），shim 变纯委托保留给仓外一方工具。
关于我 round-6 后 follow-up 评论（@deprecated vs 整段删除）
我在上轮 review 后追加的评论建议「不必补 @deprecated，直接删掉 shim 只留 registerVirtualResources」。作者选择了保守中间路线（补 @deprecated + 迁移调用点，保留委托 shim）。这是可辩护的设计取舍、非缺陷：@deprecated 给出清晰退场信号，同时为「消费已发布版本的仓外一方工具」保留一个迁移窗口。既然我已发声、作者的选择合理，此点视为由作者取舍闭环，不再以任何变体重提。
已在前几轮闭环、本轮不重提
- 上轮 4 条纯微优化 [P3]（
call.target错误消息逐段校验 /$type无$args空Promise.all快路径 / fallback eager-settle 后短路 / props→calls 解析并行度 /registerVirtualResourcesunregister 对偶）——ReflectionParser/ResourceManager 相关行本轮未改这些点，状态不变、均已各记录一次，按「提一次即可」不再纠缠。 - CodeRabbit
constructor/prototype遍历可达（Critical）+ 独立$args（Minor）——前几轮已评估闭环，traversal 代码本轮未变。
无新阻塞项。此前 --request-changes 门控已于 round-4 approve 时解除，保持可合并状态。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
审查 @ aa623272（第八轮，增量）— refactor(asset): remove legacy virtual resource API — 删除死 shim，收敛到我 round-6 建议的终点，无新问题
总结
自上轮 7f2bcbaa 起线性追加一个 commit（gh api commits/aa623272 报 parent = 7f2bcbaa，非 force-push；merge-base(dev/2.0)=2f88c2fab）。这一 commit 只做一件事：删掉 initVirtualResources 的 @internal @deprecated 委托 shim（compare 报 +0 -8，仅动 ResourceManager.ts 一个文件）。这正是我 round-6 follow-up 评论提的方向——上轮作者选了保守中间路线（补 @deprecated + 迁移调用点、保留委托 shim），我当时判为「作者取舍闭环」；本轮作者进一步收敛到更干净的终点。逐链核对无新 P0/P1/P2/P3，CI 全绿（build×3 / lint / e2e pass，codecov 进行中），保持可合并状态。
逐链核对（增量）
-
删除完整性核实——shim 是纯死码，删除无行为损失：
- 引擎仓内
initVirtualResources现零调用——在 tipaa623272上，唯一的 in-engine 调用点ProjectLoader.ts:21已在上轮迁移为engine.resourceManager.registerVirtualResources(data.files)，本轮git grep全 tree 仅registerVirtualResources命中、initVirtualResources零命中。 VirtualResource类型未被孤立：删 shim 后ResourceManager.ts仍在 4 处使用VirtualResource（:327_resolveLoadItemOptions参数、:558_virtualPathResourceMap字段、:565registerVirtualResources参数、:633interface 定义），导出面core/src/index.ts:30仍export { ResourceManager, resourceLoader, type VirtualResource }。故删除不产生死导入。- 非破坏性变更：被删方法带
@internal标记（round-7 diff 已核），从未是发布 API 表面——core/src/index.ts从来只 named-exportResourceManager/VirtualResource，从不 exportinitVirtualResources。仓外一方工具若消费的是已发布版本，走的仍是那些历史版本里的 shim；本 PR 引入的registerVirtualResources本就是新 public API，删掉本 PR 自己刚加的过渡 shim 不影响任何已发布契约。
- 引擎仓内
-
收敛判定（非社交护面子，据实修正）：round-7 我把「保留
@deprecatedshim」记为「可辩护的设计取舍、非缺陷」，并声明「不再以任何变体重提」。作者本轮主动删到底，与我 round-6 的原始建议一致。这不是我重提——是作者自行收敛，我据实记录终点更干净即可。整个 PR 现回到「新增一个 public API + 唯一 in-engine 调用点直接用新名、零遗留 shim」的最简形态。
已在前几轮闭环、本轮不重提
- 上轮全部 finding（毒化重试 stash 根因层删除 /
@deprecated+ ProjectLoader 迁移 / tsc 回归 / Burst 单类特判→通用 sentinel /$args前移 / 缺失 sub-path reject / notes 删除）——ResourceManager相关行本轮只删了这 8 行 shim，其余零改动，状态不变。 - 4 条纯微优化 [P3]（
call.target逐段校验 / 空$args快路径 / fallback eager-settle 短路 / props→calls 解析并行度）——ReflectionParser.ts/CommonSchema.ts自b8ce4dfa起零改动，均已各记录一次，不再纠缠。 - CodeRabbit
constructor/prototype遍历可达（Critical）+ 独立$args（Minor）——前几轮已评估闭环，traversal 代码本轮未变。
无新阻塞项。此前 --request-changes 门控已于 round-4 approve 时解除，保持可合并状态。
2 participants
Summary
$typevalues{ "$number": "Infinity" }value instead of aBurstconstructor-position special caseResourceManager.registerVirtualResourcesandVirtualResourcefor stable package paths backed by generated URLsArchitecture boundary
Editor resource packages remain source-v2. Builder/compiler code owns source-v2 validation and lowering to runtime-v2, including converting Editor's JSON-safe
"Infinity"source value into{ "$number": "Infinity" }.Engine consumes runtime-v2 only. The reflection parser does not know Editor
class/constructParamsstructures, particle classes, or constructor parameter positions.For sub-assets, loader notifications remain the eager fast path. The completed main resource is the authoritative fallback, so callback lifecycle ordering cannot leave a valid query pending forever. Missing query paths and main-asset failures reject without leaving stale failure state behind.
Verification
pnpm vitest run tests/src/core/resource/ResourceManager.test.ts tests/src/loader/SceneFormatV2.test.ts: 72 passedpnpm build: passed, including all package declaration buildsgit diff --check: passed