feat(omni): S5 multimodal media memory — two-trigger collection, root-bounded recall, active/sideQuery surfaces - #8949
Merged
Merged
Conversation
…-success points Add the media-memory service (packages/core/src/services/media-memory): a persistent, file-scoped lineage graph over .qwen/omni/memory.json that records recognized files/versions and policy executions with their derived media and text outputs. Collection side (Stage A of the multimodal memory design): - store.ts: single-JSON v1 backend with atomic writes, corrupt-document backup/rebuild, and read/transact serialization shared with the omni JSON cache chain. - service.ts: recordFileRecognized (idempotent file+version upsert, CURRENT_VERSION follows disk both ways) and commitPolicySucceeded (one atomic commit per succeeded policy execution: execution record, derived versions with DERIVED_FROM/PRODUCED_BY lineage, and derived_media/policy_result entries with conservative channel and coverage derivations). executionId is a content-identity key (source sha256 + policy fingerprint), so replays and degradation-cache hits converge on the same execution node. findBindingBySha256 gives the reactive ladder a read-only identity lookup. - config.ts: omni.memory normalization with fail-fast validation (unknown keys, budgets, kinds, cross-field ordering). Wire both trigger points: processMediaForOmniDelivery records the source file after recognition (hashing the source upfront and threading sha256 through the policy pipeline), and executePolicy commits after object promotion on both the success and degradation-cache-hit paths. Persistence failures are logged and never block delivery; a policy commit itself stays all-or-nothing.
omni_extract_keyframes and omni_clip_video emitted artifacts without metadata.omniRole, so role consumers could not tell an excerpt from a complete derivative: output routing selectors (role:keyframe / role:clip) never matched, and media-memory coverage derivation fell through to 'complete' for sampled keyframes and temporal clips. Label them 'keyframe' and 'clip', matching the transcript labeling convention.
Stage B read side of multimodal media memory (design M §9): - MediaResourceRegistry: session-scoped opaque handle binder (fileVersionId <-> resourceId, M §5.2) so recall payloads never carry real paths or stable identifiers. - MediaMemoryRecallService: current-version-first recall (§9.5) over the rootFileId-bounded derivation graph (§8), returning the minimal protocol shape (§9.4) with synthesized metadata/execution entries, honest gaps (not_processed / partial_coverage / artifact_unavailable, D5), and advisor-driven nextPolicyActions. Unknown or empty resource requests reject as a whole (§9.2); an unreadable store degrades to a plain miss.
Hang a lazy MediaResourceRegistry off Config and bind every memory-known resource the delivery pipeline puts in front of the model — the source at recognition, each preprocessing deliverable, and the transport-guard replacement — so recall can rebind opaque session handles back to persistent memory identities without ever exposing a path.
…le disclosure Stage B recall surface #1 (memory design M §9, D10 mutual exclusion): - omni_recall_media_memory tool, registered only when omni is enabled AND omni.memory.recall.mode === 'active'. Read-only (D11): consults the persistent store through MediaMemoryRecallService and binds session handles for returned derived artifacts; whole-request rejection maps to invalid_tool_params so the model can correct and retry. - Session resource handles are now disclosed to the model: delivery carries the SOURCE binding's resourceId, and both consumers (file reads and the tool-result funnel) lead the part group with a 【媒体资源】 annotation — the handle stands in for the path the model never sees (M §5.2), while disclosures keep their D8 adjacency to the media part. - Advisor wiring: recall gaps suggest nextPolicyActions only for tools that are both registered and opened via modelAccess.enabled — recall never steers the model into calls the media-policy gate would reject. - omni.memory normalization moved to first use (ensureOmniMemoryConfig): createToolRegistry needs recall.mode before initialize()'s omni block runs; invalid settings stay startup-fatal on both paths.
…gate Stage B evidence-gathering path (memory design M §5.2): a gated model/client call of a media-policy tool may now name its source by the opaque session resourceId announced at delivery or returned by recall, instead of a filesystem path the model must never see. - evaluateMediaPolicyToolCall resolves the handle through the session registry BEFORE the lockedArguments check (a resolved inputPath cannot sidestep an operator-pinned input); unknown/fabricated handles and inputPath+resourceId together reject as invalid_params; fixed_policy origins remain untouched (RESERVED_ARGUMENT_KEYS already bans the key in policy arguments). - Shared io schema advertises resourceId as the alternative to inputPath; required drops inputPath (presence is enforced in validateMediaPolicyIoParams AFTER gate resolution, with an actionable either-or message). - assertMediaPolicyIo input errors now name only the file's basename: those messages reach the model, and a handle-resolved call must not leak the locator the handle stands in for — basename matches the displayName the model already saw at delivery (D5: a deleted source errors without disclosing its path).
…uest Stage B recall surface #2 (memory design M §9.3, D10 sideQuery mode): - MediaMemoryRecallService grows the two sideQuery faces on the same root-bounded walk recall() uses: candidateSummaries() builds the bounded selector manifest (structure + capped preview, never raw media/full text/paths; maxCandidateEntries cap, deterministic order), and recallSelection() materializes a selection through the unified protocol — an unknown/over-budget entryId rejects the WHOLE selection (invalid_selection), never a partial fulfilment. - runOmniMemorySideQuery orchestrates one passive pass: handles are parsed from the 【媒体资源】 annotations the request itself carries (never a project-wide scan), the selector runs on the runSideQuery JSON face (entryIds only, validate enforces manifest membership and maxSelectedEntries), and every failure — timeout, selector error, rejected selection — degrades to an empty recall with a recorded reason while the main request proceeds. - client.ts injects the materialized recall as a system reminder on UserQuery/Cron turns strictly BEFORE the main request is sent (M §9.3: never retrofitted into a later turn); latency is bounded by sideQuery.timeoutMs. - disclosure.ts gains parseResourceHandleText, keying on the harness-minted handle grammar so displayNames containing the separator cannot confuse extraction.
LaZzyMan
requested review from
tanzhenxin,
wenshao and
yiliang114
as code owners
August 11, 2026 15:31
Collaborator
Collaborator
|
Thanks for the PR! Template looks good ✓ (all sections are present in substance — the Problem: this is a planned experiment milestone, not a hypothetical — it closes #8188 (S5a minimal recall) and #8189 (S5b full graph), both filed and labeled Direction: aligned. This is the S5 slice of the omni experiment track — S1–S4 (#8422, #8512, #8632, #8815) already landed on this same Size: 6,052 changed lines in 45 files — 3,815 production / 2,123 test / 114 generated (IDE settings schema). Author is a maintainer, so the two-tier core gate is exempt per AGENTS.md; for awareness only: production volume is well past the 1000-line advisory. The S5a+S5b coupling is justified in the description — collection triggers, store, recall service, and registry form one dependency closure, and the two surfaces are mutually exclusive by design (D10), so a split would have landed two near-identical mega-diffs. Approach: the scope coheres — exactly two write triggers on existing pipeline events, one recall service with two mutually-exclusive surfaces, and opaque session handles as the only model-visible identity. Existing-file edits are wiring only (config node, tool names, orchestrator commit point, client.ts injection point, schema regen); I see no drive-by refactors or unrelated churn. The questions worth pressure-testing are at the seams: the Risk: no elevated risk signals — no match against the revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓（各部分实质内容齐全—— 问题：这是计划中的实验里程碑，而非假设性问题——关闭 #8188（S5a 最小召回）与 #8189（S5b 完整图），两个 issue 均由作者提出并标记 方向：对齐。这是 omni 实验路线的 S5 切片——S1–S4（#8422、#8512、#8632、#8815）已先后合入同一条 规模：45 个文件共 6,052 行变更——3,815 行生产代码 / 2,123 行测试 / 114 行生成文件（IDE settings schema）。作者为维护者，按 AGENTS.md 豁免两层核心门禁；仅作知会：生产代码量远超 1000 行大 PR 建议线。S5a+S5b 合并在描述中已论证——收集触发点、存储、召回服务与注册表构成一个依赖闭包，两个界面按设计互斥（D10），拆分会产生两个近乎相同的巨型 diff。 方案：范围自洽——恰好两个写入触发点挂在既有管线事件上，一个召回服务带两个互斥界面，不透明会话句柄是模型可见的唯一身份。对既有文件的改动仅为接线（config 节点、工具名、orchestrator 提交点、client.ts 注入点、schema 重新生成）；未见顺手重构或无关变更。值得重点审视的是接缝处： 风险：无升级风险信号——未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewReviewed the full diff at the commit below. Independent baseline first: for "persist what omni already learned about media and let later sessions recall it", I'd reach for exactly what this PR does — one content-keyed JSON store with atomic writes, collection on the two existing pipeline events, one recall service with an opaque-handle privacy boundary. No simpler shape comes to mind that keeps the path-privacy and root-bounded-traversal guarantees, so the review focused on whether the implementation actually holds those invariants. It does:
Non-blocking observations (follow-ups, not merge-blockers):
sequenceDiagram
participant P1 as Delivery pipeline
participant P2 as MediaMemoryService
participant P3 as memory.json store
participant P4 as Recall service
participant P5 as Session registry
participant P6 as Selector model
Note over P1,P3: Collection - two triggers only
P1->>P2: FileRecognized - source sha256 plus identity
P2->>P3: atomic upsert, temp plus rename
P1->>P1: fixed policies run, objects promoted
P1->>P2: OmniPolicySucceeded - execution plus outputs
P2->>P3: one all-or-nothing commit
Note over P4,P6: Recall - sideQuery mode, before the main request
P4->>P5: resolve handles the request carries
P4->>P3: read snapshot, root-bounded walk
P4->>P6: bounded manifest, entryIds only
P6-->>P4: selection, validated against manifest
P4->>P5: rebind returned derivatives to fresh handles
P4-->>P1: system reminder injected before send
Files changed (30 of 45 shown)
Test evidenceThis is an unattended CI run — the PR's code was NOT built or executed here; the evidence below is the PR's own CI on the reviewed commit, fetched via API. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The unit suite for this PR's scope (author-reported: 72 new media-memory tests + the 803-test omni suite) is what the ubuntu job is running right now; it had not concluded at review time, so treat "tests pass" as NOT yet established here. The macOS/Windows/integration checks are skipped for this run — nothing red, but also no cross-platform signal. The author's E1–E5 table (real DashScope API, 986MB movie closed loop) is the author's claim from a local run, not independently re-run in this review. Sandboxed verification would settle the remaining behavioural claim: 中文说明代码审查按上述提交完整审阅了 diff。独立基线：对于"持久化 omni 已学到的媒体知识并让后续会话召回"，我能想到的最简形态正是本 PR 的做法——一个以内容哈希为键、原子写入的 JSON 存储，在既有管线事件上收集，一个带不透明句柄隐私边界的召回服务。评审重点因此放在实现是否真正守住这些不变量。结论：守住了。
非阻塞观察（后续跟进）：每次投递新建 MediaMemoryService 会重读 memory.json（实验规模可接受，S6 再优化）；store 的"不可读文档→跳过"分支缺专门测试；downscale-video.ts 有一处纯格式化折行。 测试证据本次为无人值守 CI 运行——未构建或执行 PR 代码，以上证据为通过 API 获取的 PR 自身 CI。ubuntu 单测作业审阅时仍在运行，"测试通过"尚未成立；macOS/Windows/集成检查本轮被跳过。作者的 E1–E5 真实 API 结果为作者本地声明，未独立复跑。沙箱验证可了结剩余行为性论断： — Qwen Code · qwen3.8-max Reviewed at |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
|
Confidence: 4/5 — clean static review with no blockers found; the remaining point of uncertainty is the still-running unit suite and the real-model behaviour only Stepping back: my independent proposal for "make omni's processing knowledge survive the turn" was exactly this shape — one content-keyed atomic JSON store, collection on the two pipeline events that already exist, one recall service behind an opaque-handle privacy boundary — and I didn't find a materially simpler path this PR missed. The hard questions all answer well: the problem is real and quantified (re-referencing the same media re-pays recognition + ASR; the PR's own 986MB case shows recall answering alone), the direction is the planned S5 slice of an experiment track whose S1–S4 already landed on this branch, and the scope coheres because the two surfaces share one store and one service by design. What impressed me in the code is the discipline at the seams: handle resolution ordered before the lockedArguments check, commits landing only after object promotion, gaps computed from the unfiltered subgraph so request filters can't rewrite processing truth, dead artifacts degrading to gaps instead of dead handles, and every failure path on the collection side proven unable to block delivery. The test suite pins these invariants rather than just exercising happy paths. What keeps this at 4 rather than 5: the unit suite had not concluded on the reviewed commit at decision time, the macOS/Windows/integration checks are skipped for this run, and the cross-session-recall-with-a-real-model claim rests on the author's local E2E — substantiated only if a maintainer triggers the sandboxed lane named in the review comment. The non-blocking nits (per-delivery service construction re-reading the store; no store-level test for the unreadable-document no-op) are named in Stage 2 and belong to follow-ups, not this merge. Approving — but CI is still running on the reviewed commit, so approval is deferred until CI lands green on 中文说明置信度：4/5 —— 静态审查干净、未发现阻塞项；剩余不确定性在于仍在运行的单测套件，以及只有 退一步看：我对"让 omni 的处理知识活过回合"的独立设想与本次实现形态完全一致——一个以内容哈希为键的原子 JSON 存储、在两个既有管线事件上收集、一个位于不透明句柄隐私边界之后的召回服务——没有发现被本 PR 遗漏的更简路径。关键问题都站得住：问题真实且可量化（重复引用同一媒体要重新付出识别 + ASR 代价，PR 自身的 986MB 案例展示仅靠召回作答）；方向是实验路线计划中的 S5 切片，S1–S4 已合入本分支；范围自洽，因为两个界面按设计共用一个存储与一个服务。 代码中令人印象深刻的是接缝处的纪律：句柄解析排在 lockedArguments 检查之前、提交只发生在对象提升之后、gap 由未过滤子图计算使请求过滤无法改写处理真相、失效工件降级为 gap 而非死句柄、收集侧每条失败路径都被证明不会阻塞投递。测试套件钉住的是这些不变量，而不只是跑通 happy path。之所以是 4 而不是 5：审阅提交上的单测在决策时仍未出结果，macOS/Windows/集成检查本轮被跳过，跨会话真实模型召回的论断仍基于作者本地 E2E——只有维护者触发上面点名的沙箱验证通道才能 substantiate。非阻塞的小问题（每次投递新建 service 会重读存储；store 的不可读文档分支缺专门测试）已在 Stage 2 点名，属于后续跟进，不影响本次合入。 予以批准——但 CI 在审阅提交上仍在运行，批准推迟到 CI 在该提交上转绿；本次运行不直接提交批准。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
added
the
omni-experiment
qwen-code-ci-bot
previously requested changes
Aug 11, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): You are review agent reverse-audit — Reverse audit agen...: none — all checks above were completed within budget.; chunk 7: couldn't run typecheck/tests (the review worktree has no node_modules, and installing the monorepo's dependencies exceeded the remaining budget); type correctne…; chunk 7: could not run typecheck/tests — the review worktree has no node_modules and installing monorepo dependencies exceeded the remaining budget; type correctness w…; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget., and 5 more.
Test Plan (not a blocker): src/core/client.test.ts — no such file or directory.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：You are review agent reverse-audit — Reverse audit agen...：none — all checks above were completed within budget.；chunk 7：couldn't run typecheck/tests (the review worktree has no node_modules, and installing the monorepo's dependencies exceeded the remaining budget); type correctne…；chunk 7：could not run typecheck/tests — the review worktree has no node_modules and installing monorepo dependencies exceeded the remaining budget; type correctness w…；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.，另有 5 条。
Test Plan（非阻断）：src/core/client.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| OMNI_CLIP_VIDEO: 'ClipVideo', | ||
| OMNI_CONVERT_IMAGE: 'ConvertImage', | ||
| OMNI_TRANSCRIBE_AUDIO: 'TranscribeAudio', | ||
| OMNI_RECALL_MEDIA_MEMORY: 'RecallMediaMemory', |
Collaborator
There was a problem hiding this comment.
[Critical] R1-1: The new tool name/display name is registered in core without propagating to the two drift-guarded consumer surfaces — web-shell TOOL_DISPLAY_NAMES and cli i18n zh.js. — Failure scenario: npm test fails in packages/web-shell (toolFormatting.drift.test.ts: expected [ 'omni_recall_media_memory' ] to deeply equal []) and packages/cli (i18n/index.test.ts: expected [ 'RecallMediaMemory' ] to deeply equal []). Measured net-new against the merge base (base web-shell run 151/151 green) — CI on this PR is red because of this.
Suggested fix: add omni_recall_media_memory: 'RecallMediaMemory' to web-shell toolFormatting.ts and toolDisplayName.RecallMediaMemory to packages/cli/src/i18n/locales/zh.js (see also the zh-TW comment on the tool constructor).
中文说明
新工具名/显示名只在 core 注册，未同步到两个有漂移守卫的消费面：web-shell 的 TOOL_DISPLAY_NAMES 与 cli 的 i18n zh.js。失败场景：packages/web-shell 与 packages/cli 的 npm test 失败（已相对合并基线实测为本 PR 新增失败），本 PR 的 CI 因此变红。修复：在上述两处补齐条目。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| }); | ||
| } | ||
| } | ||
| // Memory collection (S5, design M §6.4): commit AFTER promotion — the |
Collaborator
There was a problem hiding this comment.
[Critical] R1-2: The OmniPolicySucceeded memory commit fires only for fixed-policy orchestrator invocations (the only commitPolicySucceeded callers are orchestrator.ts:762/:964, both hardcoding fixed_policy origin). Successful model/client-origin media-policy tool calls — the exact resourceId-resolved calls this PR builds the gate for — never commit to memory. — Failure scenario: recall gap speech_text: not_processed → advisor suggests evidence gathering → model calls the tool with the handle → the tool succeeds, artifacts land on disk — but nothing commits, so every later recall returns the identical gap and suggestion, re-paying the work every session forever. Design M §3.2/§7.1/§17.1 mandate both fixed and model policy successes enter the same OmniPolicySucceeded boundary; corroborating: MediaExecutionOrigin model/client kinds are produced only by test fixtures.
Suggested fix: add the commit point on the model/client success path (scheduler/ACP, after the gated invocation succeeds and artifacts validate), reusing commitPolicySucceeded with executionOrigin: { kind: 'model' | 'client' }.
中文说明
OmniPolicySucceeded 入库只在固定 policy 的 orchestrator 调用点触发；模型/客户端发起的媒体工具调用（本 PR 专门为其建设了 resourceId 解析门）成功后从不入库。失败场景：召回缺口 → 建议取证 → 模型调用成功但不入库 → 之后每次召回都返回相同缺口与建议，永久重复处理。设计文档 M §3.2/§7.1/§17.1 要求固定与模型 policy 成功后都经同一 OmniPolicySucceeded 边界入库。修复：在模型/客户端成功路径上补上提交点。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // effective arguments, and descriptor version at the S4 boundary. | ||
| const sourceVersion = snapshot.versions[input.source.fileVersionId]; | ||
| const sourceSha = sourceVersion?.sha256 ?? input.source.fileVersionId; | ||
| const executionId = hashId('x', `${sourceSha}|${input.omniConfigHash}`); |
Collaborator
There was a problem hiding this comment.
[Critical] R1-3: Content-identity replay collision. executionId is content-keyed and omits file/version identity, so when a SECOND file with identical bytes hits the replay branch, nothing is written for it (probe-observed: zero executions/entries/lineage for file B), mediaBindings are rebuilt with B's rootFileId over A's version records (mixed root), and the declared reusedExecutionId field (types.ts:177) is assigned nowhere. — Failure scenario: movie.mkv fully processed; byte-identical copy.mkv delivered later → replay commit changed: false → recall on B's handle returns false not_processed gaps on every channel (probe-observed) while A's derivatives get rebound under B's root. Violates M §11.2/§11.3 (每个 File 仍写入自己的 PolicyExecution 与 provenance，不共享图节点); the existing "two files with identical bytes" test only asserts recognition-time node separation, never recalls the second file.
Suggested fix: in the replay branch, when existing.sourceVersionId !== input.source.fileVersionId, write the caller's own lightweight execution record with reusedExecutionId and entries parented on the caller's version; build mediaBindings from the recorded version's own rootFileId.
中文说明
内容身份重放冲突：executionId 仅以内容哈希为键、不含文件/版本身份，第二个同字节文件走重放分支时不写入任何记录（实测：B 文件零执行/零条目/零血缘），且 mediaBindings 以 B 的根覆盖 A 的版本记录（跨根混用），声明的 reusedExecutionId 字段无任何赋值点。失败场景：对 B 的召回返回全通道假 not_processed 缺口。违反 M §11.2/§11.3。修复：重放分支在源版本不同时为调用方写入轻量执行记录（含 reusedExecutionId），并用记录自身的 rootFileId 构建绑定。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| disclosure, | ||
| // Marks the artifact as a temporal excerpt for downstream role | ||
| // consumers (output routing selectors, memory coverage). | ||
| role: 'clip', |
Collaborator
There was a problem hiding this comment.
[Critical] R1-21 (pattern, location 1 of 2 — twin at extract-keyframes.ts:273): role: 'clip' is introduced but the tool descriptor version stays '1', so pre-PR degradation-cache entries and recorded fingerprints converge onto the same executionId post-PR. The cache-hit recommit passes role: hit.role = undefined → coverageFor(undefined) = complete; the replay branch never upgrades recorded entries. — Failure scenario: after upgrading, a cache hit or replay for a pre-PR-processed clip commits/reports complete coverage for a temporal excerpt → recall overclaims completeness, the model answers about footage spans it never saw, and the advisor never proposes closing the gap. Violates the service.ts "Honesty over precision — never overclaim" invariant (probe-verified: roleless output → complete; with role → partial).
Suggested fix: bump the descriptor version ('1' → '2') here and in extract-keyframes so pre-PR cache/fingerprints no longer converge, or treat an absent role on cache-hit recommit as non-committable.
中文说明
（模式问题，2 处之 1，另一处在 extract-keyframes.ts:273）新增 role: 'clip' 但工具描述符版本仍为 '1'：PR 前的降级缓存条目与指纹在升级后仍命中同一 executionId，缓存命中重提交携带 role: undefined → 覆盖度被记为 complete；重放分支也从不升级既有记录。失败场景：升级后旧缓存命中会把片段剪辑报成完整覆盖 → 召回过度声明、模型回答从未看过的片段。违反 "never overclaim" 不变量（已探针验证）。修复：两处工具一起升级描述符版本，或缓存命中重提交时视缺失 role 为不可提交。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| omniDisclosure: `原视频 ${originalDuration}${originalResolution} → 关键帧 ${index + 1}/${frames.length}${atTime}，${samplingNote}，时间连续性丢失`, | ||
| // Marks the artifact as a sampled excerpt for downstream role | ||
| // consumers (output routing selectors, memory coverage). | ||
| omniRole: 'keyframe', |
Collaborator
There was a problem hiding this comment.
[Critical] R1-21 (pattern, location 2 of 2 — twin at clip-video.ts:204): omniRole: 'keyframe' is introduced but the descriptor version stays '1', so pre-PR state converges onto the same fingerprints post-PR. Two concrete triggers: (1) replay-discard — a pre-PR recorded execution (role-less entries, coverageFor(undefined) = complete) takes the replay branch on re-delivery, which rebuilds bindings and returns changed: false WITHOUT upgrading recorded entries, so keyframes keep reporting complete visual coverage instead of sampled; (2) a stale single-frame cache entry (maxFrames: 1) recommits with role: hit.role = undefined. — Failure scenario: recall overclaims complete visual coverage for sampled keyframes — the model answers about footage spans it never saw; the advisor never proposes closing the gap. Same "never overclaim" invariant violation as the clip-video twin.
Suggested fix: bump this tool's descriptor version together with clip-video's.
中文说明
（模式问题，2 处之 2，另一处在 clip-video.ts:204）新增 omniRole: 'keyframe' 但描述符版本仍为 '1'。两个触发路径：重放丢弃（PR 前已记录的执行走重放分支，条目覆盖度保持 complete 而不升级为 sampled）；单帧旧缓存条目以 role: undefined 重提交。失败场景：关键帧被报成完整视觉覆盖 → 召回过度声明。修复：与 clip-video 一起升级描述符版本。
— qwen3.8-max via Qwen Code /review (v0.21.9)
Comment on lines
+72
to
+73
| sampleCount?: number; | ||
| sampleRate?: number; |
Collaborator
There was a problem hiding this comment.
[Suggestion] R1-51 (dead persistent-schema pattern, location 3 of 3): three more declared-but-unwritten-and-unread fields — MediaCoverage.sampleCount, MediaCoverage.sampleRate (here) and MediaVersionRecognition.probeBackend (types.ts:116). Repo-wide greps: probeBackend has exactly one match (its declaration); sampleCount/sampleRate have zero producers/consumers in the media-memory code. Both interfaces persist — coverage on every entry, recognition on every version record. — Concrete cost: the schema advertises capabilities that do not exist: a tool emitting mode: 'sampled' coverage gets no sampling-statistics semantics (recall reads only mode/channels), and the MediaVersionRecognition doc claim ("enough to decide whether a past recognition is still trustworthy") cannot be honored — nothing can consult probeBackend to invalidate stale recognitions.
Suggested fix: delete the three fields (per simplicity-first), or document them as explicitly reserved with the planned consumer.
中文说明
（死持久化字段模式，3 处之 3）另有三个只声明、不写入、不读取的字段：MediaCoverage.sampleCount/sampleRate 与 MediaVersionRecognition.probeBackend。全仓 grep 证实无生产者/消费者，而两个接口分别持久化在每条条目与每个版本记录上。代价：schema 宣称不存在的能力（采样统计、探测后端可信度判断）。修复：删除三个字段，或明确标注为预留并写明计划消费方。
— qwen3.8-max via Qwen Code /review (v0.21.9)
Comment on lines
+3454
to
+3455
| "maxEntries": { | ||
| "type": "number", |
Collaborator
There was a problem hiding this comment.
[Suggestion] R1-69: All eight numeric omni.memory fields emit "type": "number", but the runtime validator positiveInteger requires Number.isInteger and is startup-fatal — the schema blesses fractional values that abort CLI startup. The file's established convention for integer-only settings is "type": "integer" (11 prior uses; the generator supports it via case 'integer'). — Concrete cost: a user editing settings.json in VS Code sets omni.memory.recall.maxEntries: 12.5 — the schema validates green (number, ≥ 1), the IDE gives no warning, and the next CLI launch aborts with "must be a positive integer (got 12.5)". This section is where pre-validation matters most: its own description says "Invalid values abort startup".
Suggested fix: in settingsSchema.ts change the eight jsonSchemaOverride fragments from type: 'number' to type: 'integer' (optionally add uniqueItems: true to the kinds override), then regenerate via npx tsx scripts/generate-settings-schema.ts.
中文说明
新的八个 omni.memory 数值字段都生成 "type": "number"，但运行时校验器 positiveInteger 要求整数且启动即失败——schema 放行了会让 CLI 启动中止的小数值。本文件对整数设置的既有约定是 "type": "integer"（已有 11 处）。代价：用户在 VS Code 中设置 maxEntries: 12.5 时编辑器零警告，下次启动直接中止。修复：settingsSchema.ts 中八处 override 改为 integer 并重新生成。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| mediaType: 'audio', | ||
| channels: ['speech_text'], | ||
| toolName: ToolNames.OMNI_TRANSCRIBE_AUDIO, | ||
| reason: 'no transcript collected yet: transcribe the audio', |
Collaborator
There was a problem hiding this comment.
[Suggestion] R1-70: Sibling of R1-46 for images: expectedChannels('image') demands 'visual', but no flow ever covers it for an image no fixed policy processed — the common case ("There are NO system-default fixedPolicies"; guard policies run only over transport limits) — and GAP_STEP has no image row. — Concrete cost: every recall of an untouched image reports an uncloseable not_processed visual gap with no suggested action; combined with R1-2 (model-origin calls never commit) the model cannot close it either — permanently partial. The onscreen_text precedent documented one line above ("its absence is not reported as a gap") argues visual on an image whose delivered bytes ARE the visual evidence should not be expected either.
Suggested fix: drop 'visual' from expectedChannels('image') (mirroring the onscreen_text stance), or add an image-appropriate coverage rule recognizing the delivered/recognized image as visual evidence.
中文说明
R1-46 的图片版孪生：expectedChannels('image') 要求 'visual'，但未经固定 policy 处理的图片（常见情形）没有任何流程能覆盖它，GAP_STEP 也没有 image 行。代价：每次召回未处理图片都报告无法闭合的 visual 缺口且无建议动作；叠加 R1-2 后模型也无法闭合——永远 partial。上方 onscreen_text 先例（无生产者即不期待）同样适用。修复：从图片期望通道中移除 'visual'，或增加承认已投递图片即视觉证据的覆盖规则。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| // The manifest the selector saw is the ordered, capped view — | ||
| // validate against exactly that, so an id beyond the cap (which | ||
| // the selector was never shown) rejects like any unknown id. | ||
| const manifest = new Map( |
Collaborator
There was a problem hiding this comment.
[Suggestion] R1-72: The sideQuery flow judges one selection against TWO snapshots taken seconds apart: candidateSummaries (store read #1) → selector LLM window (≤ timeoutMs, default 30s) → recallSelection (store read #2) re-derives orderCandidates + slice(maxCandidateEntries) for validation — despite the comment claiming validation against exactly the manifest the selector saw. Probe: a cap-boundary pick valid under manifest #1 is rejected wholesale (invalid_selection) after one interleaved commit. — Concrete cost: cross-session recall is the feature's stated purpose — a concurrent commit during the selector window (shared .qwen/omni/memory.json) shifts the newest-first cap and pushes a legitimately-shown entryId below it → the WHOLE selection rejects → passive recall degrades to empty for that turn despite every pick having been valid when shown. Since entries are append-only, the mismatch can only false-reject.
Suggested fix: validate against the manifest the selector actually saw — have candidateSummaries return (or the service cache) the manifest/snapshot it produced, and have recallSelection validate and materialize against that one read instead of re-reading the store.
中文说明
sideQuery 流程用相隔数秒的两次快照判定同一选择：candidateSummaries（读 #1）→ selector 窗口 → recallSelection（读 #2）重新派生截断 manifest 做校验——与注释声称的"按 selector 实际所见 manifest 校验"不符。实测：一次穿插提交即可让原本合法的选择被整体拒绝。代价：selector 窗口内的并发提交（跨会话共享存储是该特性的既定目的）会把合法条目挤出上限 → 整个选择被拒 → 被动召回降级为空。修复：按 selector 实际所见的 manifest 校验（传递/缓存读 #1 的结果）。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| mimeType: output.mimeType, | ||
| sizeBytes: output.sizeBytes, | ||
| }, | ||
| ...(output.kind === 'text' |
Collaborator
There was a problem hiding this comment.
[Suggestion] R1-74: Oversized transcript text is truncated silently (truncateUtf8 at commit, truncateChars at recall) with NO truncation marker while the entry keeps coverage: {mode:'complete'} for speech_text; the doc's escape hatch ("the full content stays reachable through the artifactRef") does not exist in any shipped read path — recall exposes no artifactRef, and text entries carry no derived version/handle. The trigger is cheap at defaults: maxTextChars (24000 chars) < maxInlineTextBytes (65536 bytes), so any transcript over ~24k chars is cut at recall. — Concrete cost: the passive reminder dumps the truncated entry framed as complete — the model answers questions about late-audio content from a prefix it is told is complete; speech_text registers as covered, so no gap is ever reported and the advisor never suggests re-transcription. Contradicts the "never overclaim" stance one function above.
Suggested fix: when truncation actually happens, record it — persist a flag (inlineTextTruncated: true) or append a visible …[truncated] marker to content — and surface it in the recalled entry so coverage and payload don't contradict each other.
中文说明
超长转录文本被静默截断（提交时 truncateUtf8、召回时 truncateChars），无任何截断标记，而条目仍保持 speech_text 的 complete 覆盖；文档声称的逃生通道（经 artifactRef 访问完整内容）在任何已发布的读取路径中都不存在。默认配置下触发成本很低：超过约 24k 字符的转录必然被截。代价：被动注入把截断后的内容当作完整呈现——模型基于前缀回答后段音频问题；无缺口、无建议。修复：截断发生时记录标记（如 inlineTextTruncated 或可见的 …[truncated]）并在召回条目中呈现。
— qwen3.8-max via Qwen Code /review (v0.21.9)
…ded surface A new core tool name must land in four places, not one: core ToolNames/ToolDisplayNames, the cli i18n dictionaries, and the web-shell display-name map plus its own zh dictionary. Two drift tests guard this (cli i18n/index.test.ts and web-shell toolFormatting) and both were red.
…eted sources, keep the selector's view of the question
Three independent correctness fixes in the memory slice:
- Config normalization rejected unknown keys at every nested level but
not at the root, so `{recalll: {...}}` silently discarded the entire
configuration and ran default active mode while the operator believed
sideQuery was configured — the exact silent fallback this module's
startup-fatal stance exists to prevent.
- gapsForVersion emitted `artifact_unavailable` for a deleted source and
then CONTINUED the channel scan, adding `not_processed` siblings. The
advisor only filters `artifact_unavailable`, so recall steered the
model into evidence-gathering calls on a file that no longer exists —
the gate resolves the handle and the io assertion then fails. Nothing
can be gathered from a deleted file: report the loss and stop.
- The sideQuery selector dropped any text part starting with
`<system-reminder>`, but IDE context is PREPENDED INTO the user's own
part before the passive pass runs — so in IDE mode the selector never
saw the question and picked relevance-blind. Strip reminder blocks
instead of discarding the part.
…e annotations The role labels these tools gained are what memory maps to honest coverage (`clip` → partial, `keyframe` → sampled); role-less outputs derive `complete`. The descriptor version feeds the policy fingerprint, which keys both the degradation cache and the content-identity execution id — so leaving it at '1' let pre-role cache entries and recorded executions converge onto the post-change fingerprint two ways: a cache hit recommits with the persisted `role: undefined`, and the replay branch returns without upgrading recorded entries. Either path makes recall report a temporal excerpt or a sampled frame set as complete coverage: the model then answers about footage it never saw and the advisor never proposes closing the gap. Bumping the version partitions pre- and post-annotation state instead.
…ut every read `load()` validated the document envelope but not the record values inside it. A single non-object value — one hand edit, bad merge, or truncated sync — made every read path throw (`indexSnapshot` dereferences `version.parentVersionId`, `findBindingBySha256` reads `version.sha256`), and those throws are caught into miss/empty by the never-fatal read wrappers. The result was a PERMANENT global recall blackout for the whole project, with no `.corrupt-*` backup and nothing named in the debug log, because the envelope itself stayed valid. Prune the bad values individually and name them, matching the sibling `OmniJsonCacheFile.load()` defense. Dangling references to pruned records already degrade gracefully — a missing version reads as an `artifact_unavailable` gap.
Two Files with identical bytes shared one execution node: the key was `sha256 ⊕ configHash`, so the second file took the replay branch and got ZERO records of its own, while the first file's derivative versions were handed back stamped with the second file's rootFileId. Recall on the second file then reported false `not_processed` gaps across every channel and re-paid the work, and the derivative's lineage pointed into a different root's tree. M §11.2/§11.3 prescribe the opposite: identical bytes never merge Files, each File writes its own PolicyExecution and provenance, and only the computation and the stored bytes are reused. Three layers, cleanly separated: - execution identity is now per-File (source VERSION ⊕ config), so a replay is a genuine same-file replay and stays idempotent; - the content-identity reuse key (`sha256 ⊕ config`) is derived separately and spans Files — that is what makes cross-file reuse possible while provenance stays apart. A match records `reusedExecutionId` (declared since the schema landed, never assigned until now) pointing at the original; - derivative File nodes are keyed by (root, object path) instead of the object path alone, so each root owns cheap metadata rows over the SAME content-addressed object rather than sharing a node whose root belongs to whoever derived it first — bounded traversal (M §8) holds for both.
…ing file Collection recorded the source's persistent identity as the path it was handed — correct for a user file, whose bytes stay in place (S §4), but wrong for tool-result media: that path is a staging `.part` the funnel deletes in its `finally` the same turn, while this same delivery promotes the identical bytes into the content-addressed object store. So the disclosed handle resolved to a file that no longer existed: any media-policy tool the model pointed at it failed with ENOENT, and recall reported `artifact_unavailable` across every channel for an artifact that actually persists. Tool-origin sources now record (and bind) the content-addressed object location, which is derivable from the hash before promotion runs. If promotion never happens — the transport guard omitted the media — the ref dangles and recall says `artifact_unavailable`: honest, because the bytes were not retained.
… memory boundary Memory design M §7.1 requires fixed calls, model tool calls and direct client calls to enter ONE `OmniPolicySucceeded` boundary, and §17 names both capture sites — the core scheduler and ACP's own executor. Only the fixed-policy orchestrator ever committed: the scheduler built the `PolicyArtifactBatch` (with the real origin) and nothing consumed it, and the "model-call artifact bridge" its comment referred to did not exist. So evidence gathering never accumulated. Recall reported a gap, the advisor suggested a tool, the model called it with a session handle, the tool succeeded and wrote real artifacts — and the next recall reported the identical gap, re-paying the work every session, forever. That is exactly the loop the resourceId path in this PR was built to close. The bridge walks the same gates in the same order as the orchestrator: validate each artifact against the descriptor (contained in the output directory the call declared), require every declared output, promote to the content-addressed store, THEN commit (M §6.4 / S §5 — no record may reference bytes that are not yet in `objects/`). A handle-driven call recovers its source identity from the session registry instead of re-hashing the input; a raw-path call falls back to an idempotent recognition upsert. Fixed-policy batches are skipped: the orchestrator already owns them. `resourceId` also joins the fingerprint's excluded keys. Like `inputPath`, it is per-invocation plumbing — and it is minted fresh every session, so leaving it in would make each session re-derive identical work.
The degradation cache maps one input to ONE media derivative, so its own comment admits that multi-output tools and text products are "simply not cached — re-run instead of guessing". That made #8189's «同文件同 settings 二次触发同一 policy：直接复用，无重复执行» unmet in the cases that cost the most: re-delivering an 81-minute movie re-ran the whole ASR pass every time, because a transcript is a text product. Memory already records every output of every execution, keyed by content identity — so it can answer the reuse question the cache cannot. The orchestrator now consults it BEFORE the cache and, on a hit, rebuilds the deliverables from the recorded objects and skips execution entirely, covering media, text and multi-output alike. Reuse is verified exactly like a cache hit, because memory.json is project-local and hand-editable: every object must still be a regular file, must still hash to its recorded identity, and a media object must re-recognize as a type the tool DECLARES producing — otherwise a crafted record could route arbitrary store content through a policy that never made it. Text is re-read from the promoted object rather than from the entry's `inlineText`, which is truncated to `collection.maxInlineTextBytes` and must never be delivered as the whole transcript. Any doubt falls through to a fresh derivation. The reusing file still commits its own execution, stamped with `reusedExecutionId` (M §11.3): the computation is shared, the provenance is not.
…wording Two acceptance criteria in #8188 read as direct contradictions of this document, and one of them contradicts #8189 as well. Both are resolved by naming the layer each statement belongs to, rather than by changing the model. - §11.2.1 (new): «内容相同的两个文件不重复建节点» and «不合并不同文件» live on different layers. Content-keyed reuse covers the physical object and the derived computation; the File node stays locator-keyed because identity MUST be stable across content changes — key it on content and §11.1's version chain and #8189's «修改内容后旧衍生物不默认召回» both become inexpressible (an edit yields an unrelated node, and editing back makes history a cycle). Also records why derivatives are keyed by (root, object path). - §9.2.1 (new): «按路径/内容 hash 查询» and «返回中永不暴露真实本地路径» are reconciled per caller — the model gets session handles only (unforgeable capability; a path parameter would let injected media steer it into probing other files, whose transcripts recall returns), while path/hash lookup stays a harness capability that already exists. The known cost is written down too: a deleted file's memory is currently unreachable, and the fix belongs in the authorization layer (@-reference re-anchoring), not in widening what the model may pass. The issue bodies were updated to match.
Collaborator
Author
Review response — all 10 Critical addressed, plus two design conflicts resolvedNine commits pushed ( The 10 Critical findings
R1-3 in detail: three layers, not a different identity keyThe probe was right about the symptom and about its cause being the content-keyed
Beyond the review: two acceptance criteria that contradicted the design
One suggested change declinedSeparating the tool-settings hash from the final arguments in the reuse key. The The 55 Suggestion-level comments are being triaged next. |
… sections Both found by running the real pipeline rather than the unit suites. Text-product reuse never fired. `findReusableOutputs` only resolves an object path for outputs that have a derived VERSION node, which text products do not have — so `rebuildReusedOutputs` bailed and the tool re-ran. The comment there even said the caller reconstructs the path from the content hash; that half was never written. A real run made it obvious: extract-audio and keyframes logged reuse hits while the transcript re-ran the entire ASR pass — the single most expensive thing #8189 exists to avoid, and the headline claim of the reuse commit. The object store is content-addressed, so the path is derivable; text outputs now reuse like media. The existing test passed because it used a media artifact, so a transcript-shaped case was added alongside it. Unknown sections under `omni` were silently ignored. The settings loader scans TOP-LEVEL keys only, and only to a debug line, so a nested typo was caught by nothing: `omni.memoryy.recall.mode = sideQuery` started cleanly and registered the active-mode recall tool — the operator's whole configuration discarded without a word. The same hole swallowed `omni.processingg`, which would silently disable every degradation policy. `omni`'s own normalizers are startup-fatal by design, so the namespace now rejects unknown sections with the allowed set named. The permitted keys are derived from the settings schema, not hardcoded, so they cannot drift.
…al execution windows Three defects a real multi-session run surfaced. The first destroyed work. **Fixed output filenames collided.** Every policy tool wrote a constant name (`clip.mp4`, `downsampled.jpg`, `transcript.txt`). Under fixed-policy orchestration that is safe — each invocation gets its own staging directory — but `modelAccess` lets a caller pick a PERSISTENT `outputDir`, and there the second call silently destroyed the first artifact. In the run: a clip cut on day one was overwritten by a different clip on day three, and the commentary written against the first clip began describing the wrong footage. Worse, the model noticed something was off, could not diagnose it, and skipped re-watching — so the defect surfaced as degraded output quality rather than an error. Names now carry the axes that distinguish artifacts: the source stem, plus a natural variant where the operation has one (a clip's span, a frame's index). Same source and variant still resolve to one name, so re-running an operation replaces its own output — idempotent, not destructive. The keyframe lister derives its matcher from the same template, so it can never pick up a sibling video's frames out of a shared directory. **The transport guard had no duration dimension.** A 98-minute film downscaled to 474 MB cleared the 1 GiB byte ceiling, uploaded, and was then refused by the provider for being too long: the guard reported success, the caller got an opaque 400, and a 97-second transcode was paid for nothing. Duration is a transport limit like the others, so `maxDurationSeconds` now sits beside the byte and token checks with the same semantics (unset disables it; missing metadata never rejects) and a message that names duration and says downscaling cannot fix it. **Model-origin executions recorded the collection window, not the work.** `startedAt` was taken after the tool had already run, so extracting audio from a 98-minute film was recorded as 0.6s. The scheduler already tracks `executionStartTime` (approval wait excluded); it is now threaded through.
A derived artifact of a clip re-uses the clip's filename as its source stem, and the clip's own variant contains '+' (`123s+40s`). Stripping it made the derived stem drift from its parent (`clip-123s-40s-keyframe-…`). '+' is portable on every major filesystem, so keep it: names now round-trip through a derivation chain.
…ated Two friction points the character-analysis run surfaced. **A remembered file's memory was unreachable once its bytes were gone.** A handle is minted only at delivery, and delivery needs the bytes — so a transcript or keyframe set outlived the file it described with no way back into a session. The same wall hits an audit that does not want to re-deliver a 2.4 GB film just to ask what work was recorded: with no handle, recall rejects everything, and the natural next move (passing the filename as a resourceId) is correctly refused. Observed exactly that: the model reached for `resourceIds: ["robot-dreams.mkv"]` and got `not issued in this session`. An `@`-reference to a missing-but-remembered path now mints a handle from the recorded identity — the user's reference is the same authorization a delivery carries, so the fix belongs at the authorization layer rather than by widening what the model may pass (design M §9.2.1). The model gets the handle plus an explicit 【媒体缺失】 note, so it recalls instead of trying to read. Path/locator lookup stays a harness capability. **A truncated recall page was indistinguishable from an exhaustive one.** An audit read 6 clips under `limit: 12` and reported "no keyframes were ever extracted" while the store held 72 of them. The reader was honest about what it saw — it simply had no way to know it was looking at a page. `matchedEntries` now states the full match count, and only when the budget actually cut something, so an exhaustive page stays as small as before (§9.4 minimal return).
…e reprocessing The handle annotation ships with every memory-known delivery, but nothing in the model's context explained it: the media-guidance section listed only the three disclosure markers, and the tool that consumes handles is deferred, so its description — which does explain them — is not in context until ToolSearch surfaces it. Meanwhile the same section actively tells the model to gather missing evidence with the policy tools, i.e. to reprocess from scratch exactly what memory already holds. The active-recall surface's whole purpose could silently never fire. The section now names the handle marker, states that it is the only identity the model will ever get for that file, and says to consult recall BEFORE reprocessing. Gated on active recall mode: under sideQuery the harness injects recalled memory itself and the tool is not registered, so naming it would invite a guaranteed unknown-tool error (D10). The tool-display-name drift test now runs over every translating locale rather than zh alone. `t()` has no cross-locale fallback, so a tool added to one locale renders a raw English badge beside translated siblings in the other.
Fourteen findings from the PR review, verified one at a time. The ones that could mislead a caller: - a truncated recall entry now says so. maxTextChars (24k chars) is smaller than the collection bound (64 KiB), so any long transcript is cut at READ time while coverage still legitimately reports `complete` — the model saw a prefix, no gap, and answered about audio it never read. `contentTruncated` marks the prefix. - a resourceId handle whose modality the tool does not accept is refused as a parameter error instead of becoming a spawned ffmpeg that burns the tool timeout and returns an opaque stderr tail. - the advisor no longer suggests audio extraction against a video's speech_text gap: that gap is still open in the very payload that RETURNS the extracted track, so the suggestion repeated itself. Nor does it suggest anything against `partial_coverage` — sampled evidence stays sampled by design, so that advice could never close the gap. - a derived version keeps the execution that actually produced it. A second execution landing on byte-identical output was rewriting the pointer, leaving the version naming an execution whose outputs it is not. - `maxAttempts` governs the client's retry loop only; a selection that parses but names entries outside the manifest is refused without a retry. The comment and the setting description said otherwise. - `sideQuery.model: null` means the side-query default (the configured fast model, falling back to the session model), not "the session's active model" as three places claimed. And the hygiene: - duplicate resourceIds and repeated selector picks collapse instead of spending the budget twice - an empty materialization records `materialized_nothing` rather than injecting a reminder that says nothing - `candidateSummaries` lets a defect propagate (the caller already degrades with a recorded reason) instead of swallowing the stack - `kinds`/`roles` reject `[]`, which read as "restrict to nothing" - count and millisecond settings are `integer` in the generated schema - dropped three never-written fields; corrected the `managedId` doc, which described an extension the key does not carry
The @-command/URL funnel records the file in memory and mints a registry binding, then never tells the model the handle. Active recall rejects the handle as never-issued, and the passive selector finds no handles to consult — so a session could pay for a download, an upload, and a policy run, and be structurally unable to recall any of it. The withheld case is where it matters most: the model cannot see the media, so the handle is its only remaining route to the content.
`contentTruncated` and `matchedEntries` both exist because a reader that cannot tell a prefix from the whole, or a page from the set, draws confident wrong conclusions — and both were shipped in code without the design doc naming them.
Mutation probes showed both were free to break silently: - inverting the selector's manifest-membership check kept the whole suite green while, with the default single attempt, every passive recall degrades to `selector_failed` — the feature dead in production. The mock now forwards responses through `validate` the way production does, so the happy path exercises the gate too; three tests fail on the inversion. - dropping `...handleParts` at any of the five replacement sites left tool-produced or withheld media with no recall handle, and no test cared. Each site now fails exactly one case. Also pinned: the 4000-char selector request cap, the abort/timeout signal composition (a Ctrl-C in the selector window must cancel it, not leave the main request waiting out the full timeout), promptId attribution, and the basename-only input error — whose leak-prevention was satisfied by the old full-path message too. The manifest no-path assertion moved out of the mock: a leak used to fail as "result was null" instead of naming the path.
…ough Sixteen behaviors the design doc states and the code implements, none of which any test defended. Each was verified by breaking the source, seeing the new test fail, and restoring it. The ones that matter most: - gaps must speak for the CURRENT version's processing state, always, and must be computed from the FULL subgraph. Two mutants — looping gaps over every consulted version, and threading the request's kinds/roles filter into the coverage scan — would have made recall report missing work that exists, purely because the caller narrowed what it asked to see. The existing history test could not catch either; the new fixtures invert its direction (unprocessed history, processed current) and kill both. - the selector-visible manifest: the 200-char description cap was never binding (longest fixture was 184 chars), so removing it would have sent full transcripts to the selector; and validation against the CAPPED manifest let an entry past maxCandidateEntries be materialized though the selector was never shown it. - registry rebinding now pins first-binding-wins explicitly. Updating in place would repoint a handle the model already holds; minting a second handle for one version would break cross-call correlation. - the store's rename-based write and newest-wins backup retention, pinned black-box (inode replacement, no orphan .tmp) rather than by mocking. - multi-output executions: every call site in the suite committed exactly one output, so truncating the output loop was invisible. - the config normalizer's real contract: it throws rather than clamps, its bounds are non-strict, and it validates the MERGED values — lowering only maxEntries to 6 is startup-fatal against the default 12.
…call degraded Three findings that each end the same way — the session holds a memory it cannot get back to, and nothing says so: - A transport-guard rejection threw away the handle it had already minted. Every throw site sits downstream of the bind, and the omission branch with the identical "over-limit, withheld" verdict does disclose its handle — so the rejection was the one path that stranded a recorded resource: the model can neither see the bytes nor ask memory about them. The verdict now carries the handle, and the tool-result funnel emits it. - Passive recall degrades silently by design (the turn proceeds without the memory it was meant to carry). With a pinned-but-unavailable selector model that is a permanent outage with nothing to see, so the reason is now recorded at the injection seam, and the manifest-failure branch — which had no log at all — gets one. - Chinese queries never ranked. Splitting on separators yielded one token per phrase, scored by whole-substring containment: unless an entry repeated the caller's exact phrasing, everything scored zero and ordering collapsed to newest-first. Unsegmented scripts now index as overlapping character bigrams, split at script boundaries so `480p字幕` does not produce bigrams that straddle them. Also: `PolicySucceededCommit.created` had no consumer outside tests. It distinguishes a content-identity replay from a fresh record, which is exactly what a reader of the debug log cannot otherwise tell — "no new execution appeared" reads identically to "collection did nothing".
Each was free to disappear without a red test: - the orchestrator's post-promotion commit, its degradation-cache-hit commit, and the memoryBinding threaded onto each WorkItem. Disabling the commit, or always threading the source binding instead of the derived one, left the suite green while every fixed-policy execution recorded wrong or no lineage. Covered with a real MediaMemoryService on a tmp store, reading memory.json back — including a two-stage chain, where stage two must be EXECUTED_ON stage one's version, not the root. - the reactive-degradation ladder's memory wiring, which had no coverage at all beyond its pure helpers. One test pins that a rung's re-derivation lands on the binding found by content hash; the other that an unknown hash degrades normally and records NO execution, rather than inventing provenance on someone else's lineage. - the client injection seam: moving it after the systemReminders splice — the exact regression that kills passive recall — now fails. - the D10 exposure gate at registry-construction level: the recall tool appears in active mode, is absent in sideQuery mode, and an invalid omni.memory.recall.mode aborts instead of falling back to defaults.
Collaborator
Author
Suggestion triage — all 55 reviewed, 20 fixed, 33 covered by new tests, 2 declined with evidencePushed as 8 commits on top of the Critical batch. Grouped by what the finding actually cost. Fixed — findings that could mislead a caller
Fixed — hygiene
Also fixed the CI red: Covered by new tests — 33 findings, each verified by mutationEvery test below was checked by breaking the source, watching it fail, and restoring the source. Highlights where the probe found something worse than "missing coverage":
Declined, with evidenceR1-40 — display-name localization. No change needed: R1-72 — one selection judged against two snapshots. Keeping it. The second read is the atomicity boundary that makes validation meaningful; the alternative (trusting a manifest the caller captured earlier) would validate against a snapshot that no longer exists. If the store genuinely changed in between, the entry moved, and a whole-selection rejection degrading to an empty recall is the correct outcome — not a bug to engineer around. R1-46 / R1-70 — |
…o ACP `Check serve fast-path bundle closure` went red: the ACP agent's static import closure had acquired iconv-lite's encoding tables (~553 KB). The mechanism is worth writing down, because the obvious readings are all wrong. Nothing imported iconv-lite. What S5 did was add five re-exports to the ROOT barrel, which is statically imported by ~150 modules in the ACP closure. esbuild assigns modules to chunks by which entry points reach them, so every module added to the barrel's graph regroups the split — and that regrouping moved `sync-file-encoding` (and through it `iconvHelper`, whose top-level `import 'iconv-lite'` has side effects and cannot be shaken out) into a chunk the ACP chunk statically imports. So the fix is not "move the export somewhere else": my first two attempts relocated it to the `omni` barrel, which is itself statically imported by the root barrel and by tool-result-media, and stayed red. The barrel surface itself has to come back to baseline. None of the five needed to be there. Four had no consumer outside their own module and tests; the rest are reached by dynamic import, which does not care where the export lives: - ACP's model-call collection gets a dedicated `./omniPolicyCollection` subpath, the pattern `./goalWire` and `./transcriptRecords` already establish for "the CLI needs one deep function, not a barrel". - `reanchorRememberedMedia` moves to the `omni` door the `@`-reference funnel already loads for every other omni call it makes. - `createMediaMemoryRecallService`, `buildMediaMemoryRecallAdvisor`, `extractRequestResourceIds`, `runOmniMemorySideQuery`, `formatOmniMemorySideQueryReminder`, `OmniRecallMediaMemoryTool` and `formatResourceHandleText` simply leave the root barrel. Located by bisecting the 28 commits with the check itself, then diffing the ACP closure module sets between a green and a red metafile: the delta was exactly three modules, which is what named the mechanism.
Collaborator
Author
One more red, and it was a real one
What it was: the ACP agent's static import closure had acquired iconv-lite's encoding tables (~553 KB). Why, which is the interesting part: nothing imported iconv-lite. S5 had added five re-exports to the root barrel, which ~150 modules in the ACP closure import statically. esbuild assigns modules to chunks by which entry points reach them, so each module added to the barrel's graph regroups the split — and that regrouping moved So "move the export elsewhere" does not fix it: relocating it to the None of the five needed to be on it. Four had no consumer outside their own module and tests; the rest are reached by dynamic import, which does not care where the export lives:
How it was located, since static reasoning kept pointing at the wrong file: bisected the 28 commits with the check itself (green at 13, red at 14), then diffed the ACP closure module sets between a green and a red metafile. The delta was exactly three modules — Worth noting for future omni work: this check does not guard "don't import package X". It guards "don't hang new modules off a barrel that half the CLI imports statically." |
…s as model calls
Two findings from the re-review, both real:
- The URL funnel hands its staging download to the delivery pipeline with
no lifetime marker, so memory recorded the `.part` path as the file's
persistent identity — the same defect C9 fixed for tool-result media,
missed on this third funnel. The handle already shown to the model
resolves to ENOENT the moment the funnel's `finally` runs: every later
policy call on it fails, and cross-session recall reports
`artifact_unavailable` for bytes the object store still holds. URL media
now anchors to the content-addressed object path, and the version's
source records the URL itself (`protocol: 'url'` existed in the schema,
unused until now) — the durable identity of downloaded media is where it
came from, not where it briefly landed.
- ACP committed its policy successes with `executionOrigin: 'client'`
while the modelAccess gate forty screens up pins the very same call as
`{ kind: 'model' }` ("Every ACP-originated call is a model call").
Recall provenance contradicted the gate that admitted the call.
The URL-anchor test is mutation-verified: reverting the lifetime marker
fails it.
Collaborator
Author
|
Both re-review findings confirmed and fixed in P1 — URL media handle pointed at a deleted staging file. Verified: the funnel passed no lifetime marker, so the delivery recorded the P2 — ACP provenance said |
This was referenced Aug 13, 2026
Closed
…ake out GC Section 6.1 described the recovery scan as drafted; the implementation learned things the draft could not know, and the doc should carry them: "delete all of staging/" became a grace window because a second CLI process's recovery must not delete a live invocation's transcode out from under it (config validation caps tool timeouts below the window for the same reason), and corrupt-object deletion cascades into the degradation cache so a cache hit can never point at bytes that are gone. Section 6.2 is S6's landing point, sharpened by what S5 settled: the GC root set has TWO sources in the memory snapshot (artifactRef.managedId AND managed-protocol source locators — tool/URL media anchor their identity there), the snapshot read is fail-closed (no roots readable, no deletion at all), and object deletion reuses the recovery scan's existing cache-cascade rather than growing a second one.
The bucket loop's budget guard calls remainingTimeoutMs() right before the ffmpeg call reads it again; on a slow runner a millisecond elapses between the two reads and the exact-equality assertions on the first call's timeoutMs turn flaky (observed on ubuntu CI: 599999 != 600000). Freeze Date.now in the full-duration spread test (ffmpeg is mocked, nothing there needs real time) and tolerate the guard-to-use drift in the shared-budget test, which does need the real clock for its 50ms burn. Restore spies in afterEach so the frozen clock never leaks into neighbouring tests.
wangyuenlp
approved these changes
Aug 13, 2026
This was referenced Aug 13, 2026
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Implements the S5 memory slice of the omni experiment — S5a (minimal cross-session recall) and S5b (full graph: atomic policy registration, lineage, reuse, sideQuery) — as one persistent multimodal memory: collect at exactly two trigger points, recall through one root-bounded service with two mutually-exclusive surfaces.
Collection side (Stage A)
services/media-memory/): single-JSON v1 backend (.qwen/omni/memory.json, temp+rename atomic writes,schemaVersion: 1) holdingMediaFileRecord/MediaFileVersionRecord/MediaPolicyExecutionRecord/ normalized policy outputs, withHAS_VERSION/DERIVED_FROM/PRODUCED_BYexpressed as record references bounded byrootFileId.FileRecognized—processMediaForOmniDelivery(and the reactive-degrade ladder) records the source identity (localPath + sha256, user originals never copied intoobjects/) idempotently;OmniPolicySucceeded— the orchestrator's success point commits execution + derived versions + text outputs in one all-or-nothing transaction, AFTERobjects/promotion (S §5 ordering invariant: no record may reference a missing object). Failure/quarantine/abort paths commit nothing. Collection failures never block delivery (D12).executionId = sha256(sourceSha | policyFingerprint)— degradation-cache hits and replays converge onto the same execution node; the graph does not grow on re-delivery.FileVersionand movesCURRENT_VERSION.Recall side (Stage B)
recall.ts): request/result protocol per M §9 — current-version-first (§9.5, stale bound versions surface as an explicitcurrent: falsehistory hint), root-bounded traversal (§8), honest gaps (not_processed/partial_coverage/artifact_unavailable, D5: deleted user file degrades to a gap, never an error), whole-request rejection for unknown/fabricated handles (§9.2), and advisor-producednextPolicyActionslimited to tools that are registered ANDmodelAccess-enabled.fileVersionId ↔opaque sessionresourceIdbinder hung offConfig; every delivery mounts the source (and derivatives) and the delivery consumers lead the part group with a【媒体资源】displayName：resourceIdannotation — the handle is the ONLY identity the model ever sees. Recall results rebind returned derivatives to fresh session handles.omni_recall_media_memory): registered only whenomni.memory.recall.mode === "active". Read-only (D11);maxFilesPerCallenforced; rejections map to retryableinvalid_tool_params.runSideQueryJSON face) reads a candidate manifest (maxCandidateEntriescap, summaries only — no raw media/full text/paths), returns entryIds only; unknown/cross-root/over-maxSelectedEntriesselections reject wholesale; timeout or selector failure degrades to an empty recall with a recorded reason. The materialized result is injected as a system reminder strictly BEFORE the main request (client.ts UserQuery/Cron turns). The two modes are mutually exclusive (D10) — active never runs the selector, sideQuery never registers the tool.resourceIdin place ofinputPath, resolved at the single call gate (evaluateMediaPolicyToolCall, shared by CoreToolScheduler and ACP) before the lockedArguments check; input io errors report only the file's basename so a handle-resolved call cannot leak the locator it stands in for.omni.memory.collection/recallsettings node, normalized once at startup (getOmniMemoryConfig()), startup-fatal on invalid budgets/mode, cross-field orderingmaxSelectedEntries ≤ maxEntries ≤ maxCandidateEntriesenforced.Design mapping:
docs/design/2026-07-29-omni-multimodal-memory.md(M) +docs/design/2026-07-30-omni-managed-media-storage.md(S §4–§6/§8).Why it's needed
Closes #8188
Closes #8189
After S4, every policy execution's knowledge dies with the turn: re-referencing the same movie re-pays recognition, keyframe extraction, and a full ASR pass; the model has no way to ask "what do we already know about this file?". S5 makes processing knowledge persistent and addressable — a transcript produced last week answers today's question in one tool call (or one passive injection), across sessions, without ever exposing a real path to the model.
Reviewer Test Plan
How to verify
Unit suites (all green):
E2E acceptance ran against the real DashScope API with
qwen3.5-omni-plus(npm run build && npm run bundle,node dist/cli.js), five cases:partial: 10 entries (transcript + 4 keyframes with per-frame disclosures + wav + metadata + 3 executions); derivatives rebound to fresh session handles; zero path leakage in every model-visible payload; store unchangednextPolicyActionssuggestion → model callsomni_extract_keyframeswithresourceId(no path) → gate resolves → 8 real frames on disknot issued in this session,invalid_tool_params)【媒体记忆】reminder injected with the unified protocol JSON; session 2 answers transcript/keyframe questions from injected memory without reprocessinghit: 12 entries, 0 gapsrecall aloneSpot-checks worth doing in review:
recordFileRecognized/commitPolicySucceededare called exclusively from the omni pipeline; the tool surface is read-only.memory.jsonis either the old or the new snapshot (temp+rename), never a half-written one; a quarantined invocation leaves zero records.includeHistoricalVersionsopt-in).omni.memory.recall.mode→ tool registration and selector behavior swap; both never active at once.Tested on
Environment (optional)
Local vitest + tsc; E2E against DashScope with
qwen3.5-omni-plus(real uploads, real ASR, real 986MB movie).Risk & Scope
timeoutMs-capped) before media-bearing requests.memory.json+ orphaned objects (S6); historical oss URL rewriting; sideQuery candidate pre-ranking token budgets (M §18 open item — conservative truncation for now); persistent schema migration (v1 writesschemaVersion: 1).omni.memorysettings node with full defaults; memory is inert unless omni is enabled.