Skip to content

docs(omni): 多模态实验架构设计(识别/Policy/Memory/存储) - #8110

Merged
yiliang114 merged 1 commit into
omni-experimentfrom
omni/design-docs
Aug 4, 2026
Merged

docs(omni): 多模态实验架构设计(识别/Policy/Memory/存储)#8110
yiliang114 merged 1 commit into
omni-experimentfrom
omni/design-docs

Conversation

@LaZzyMan

@LaZzyMan LaZzyMan commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Adds four interlinked architecture design documents for the omni multimodal experiment, serving as the design baseline for all subsequent implementation PRs on the omni-experiment branch:

  • File recognition & metadata (2026-07-29-multimodal-file-recognition-and-metadata.md): unified MediaRecognitionService with exactly two normalization trigger points (user input / tool results), URL localization with a 100 MiB approval threshold, sniff → probe → SHA-256 identity pipeline, raw-resource token estimation (audio 7 tok/s; visual w×h×frames/(32×32×2)), and a two-layer metadata model (internal extraction vs model-visible projection).
  • Policy orchestration (2026-07-29-omni-multimodal-policy-orchestration.md): MediaPolicyTool contract with two-part configuration (per-invocation argument schema + per-tool settings), a single ordered fixedPolicies set with a condition DSL (resource metadata / token estimate / session context), a mandatory lossy-output disclosure contract, transport guard measured on upload-channel limits, and delivery exclusively via the DashScope official temporary-upload channel (all-upload, no inline mode).
  • Memory (2026-07-29-omni-multimodal-memory.md): file-scoped memory graph with exactly two collection trigger points (FileRecognized / OmniPolicySucceeded), harness-exclusive writes (agent and side query are read-only), atomic policy-output commits, and active/side-query recall modes sharing one protocol.
  • Managed media storage (2026-07-30-omni-managed-media-storage.md): content-addressed object store under .qwen/omni/, staging/quarantine lifecycle aligned with policy transaction boundaries, upload cache (sha256+model → oss URL, 48h TTL), and mark-and-sweep GC rooted at active memory references.

Why it's needed

The omni experiment explores how different image/audio/video processing strategies affect multimodal model performance, with the goal of stabilizing strategies for long/short media handling and eventually producing training data. This work spans file ingestion, policy execution, memory, and storage — four coupled subsystems that need agreed contracts before implementation starts. These documents capture the reviewed design decisions (requirements from the Omni Harness design doc and stakeholder comments) so implementation PRs can be validated against a stable baseline.

Reviewer Test Plan

How to verify

Docs-only PR — no runtime behavior changes. Suggested review path: read the four documents in dependency order (recognition → policy orchestration → memory → storage) and check that cross-references between them are consistent (trigger points, PolicyArtifactBatch, managed storage areas, upload cache), and that stated source-code anchors (e.g. executeToolCall(recordToolResult: false), convertToFunctionResponse, DashScope converter media branches) match current origin/main.

Key claims were pre-verified: source assertions checked against origin/main, and the DashScope temporary-upload delivery path validated end-to-end with the real API (123KB / 21MB / 424MB video, image via image_url, audio via input_audio with oss URL; upload cap 1GB/file, URL valid 48h, credential reusable within 300s; missing X-DashScope-OssResourceResolve header deterministically returns HTTP 400).

Evidence (Before & After)

N/A (documentation only)

Tested on

OS Status
🍏 macOS
🪟 Windows N/A
🐧 Linux N/A

Environment (optional)

N/A — no code changes; documents formatted with Prettier.

Risk & Scope

  • Main risk or tradeoff: design-level only; the all-upload delivery decision depends on the DashScope temporary-upload channel (officially marked not-for-production), accepted for this experiment branch. No runtime impact until implementation PRs land.
  • Not validated / out of scope: per-modality policy tool catalogs, storage backend selection, and roadmap are follow-ups; whole-request media limits on DashScope still need E2E measurement before real model experiments.
  • Breaking changes / migration notes: none (docs only; experiment work stays on omni-experiment and is not intended for main).

Linked Issues

None — design baseline for the omni-experiment branch.

中文说明

本 PR 做了什么

新增 Omni 多模态实验的四篇互相关联的架构设计文档,作为 omni-experiment 分支后续所有实现 PR 的设计基线:

  • 文件识别与元数据:统一 MediaRecognitionService,仅两个归一化触发点(用户输入/工具结果),URL 本地化(100 MiB 许可门槛),sniff → probe → SHA-256 身份链路,原始资源口径 token 估算(音频 7 tok/s;视觉 w×h×frames/(32×32×2)),metadata 内部提取与模型可见投影双层结构。
  • Policy 编排MediaPolicyTool 双部分配置契约(单次调用参数 schema + 工具级 settings)、带条件 DSL 的统一有序 fixedPolicies 集合、有损输出强制披露契约、按上传通道口径度量的 transport guard、统一经 DashScope 官方临时上传投递(all-upload,无 inline 模式)。
  • Memory:文件级记忆图,仅两个收集触发点(FileRecognized / OmniPolicySucceeded),Harness 独占写入(Agent 与 side query 只读),policy 输出原子提交,active/sideQuery 双召回模式共用一套协议。
  • 受管媒体存储.qwen/omni/ 下内容寻址对象库、与 policy 事务边界对齐的 staging/quarantine 生命周期、上传缓存(sha256+model → oss URL,48h TTL)、以 active Memory 引用为根的 mark-and-sweep GC。

为什么需要

Omni 实验旨在探索不同图片/音频/视频处理策略对多模态模型表现的影响,固化长短媒体处理策略并最终用于训练数据生产。这项工作横跨文件接入、policy 执行、memory 和存储四个耦合子系统,实现开始前需要先确定契约。这些文档沉淀了经过评审的设计决策(需求来自《Omni Harness 设计方案》及需求方评论),使实现 PR 可以对照稳定基线验收。

Reviewer 测试计划

如何验证

纯文档 PR,无运行时行为变化。建议按依赖顺序阅读四篇文档(识别 → policy 编排 → memory → 存储),检查交叉引用一致性(触发点、PolicyArtifactBatch、受管存储分区、上传缓存),并核对文中源码锚点(如 executeToolCall(recordToolResult: false)convertToFunctionResponse、DashScope converter 媒体分支)与当前 origin/main 一致。

关键结论已预先验证:源码断言对照 origin/main 核验;DashScope 临时上传投递链路用真实 API 端到端验证(123KB / 21MB / 424MB 视频、image_url 图片、oss URL 走 input_audio 音频;单文件上限 1GB、URL 有效期 48h、凭证 300s 内可复用;漏加 X-DashScope-OssResourceResolve 请求头确定性返回 HTTP 400)。

证据(前后对比)

N/A(仅文档)

已测试平台

macOS ✅;Windows / Linux N/A。

环境(可选)

N/A —— 无代码变更;文档已通过 Prettier 格式化。

风险与范围

  • 主要风险/权衡:仅设计层面;all-upload 投递决策依赖 DashScope 临时上传通道(官方标注不建议用于生产),实验分支已接受该权衡。实现 PR 落地前无运行时影响。
  • 未验证/范围外:逐模态 policy Tool 清单、存储后端选型与 roadmap 为后续工作;真实模型实验前仍需实测 DashScope 整请求媒体总量限制。
  • 破坏性变更/迁移说明:无(仅文档;实验工作保持在 omni-experiment 分支,不面向 main)。

关联 Issue

无 —— omni-experiment 分支的设计基线。

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LaZzyMan Thanks for the design docs — the content looks thorough and well-structured.

One thing before we can proceed: the PR body doesn't follow the PR template. The template requires these headings:

  • ## What this PR does
  • ## Why it's needed
  • ## Reviewer Test Plan (with How to verify, Evidence, Tested on)
  • ## Risk & Scope
  • ## Linked Issues

Your current body uses different headings (概述 / 四篇文档与职责边界 / 关键已定决策 / 验证 / 后续). The information is largely there — it just needs to be reorganized under the template headings so reviewers and tooling can find it consistently. For a docs PR, the Evidence (Before & After) section can simply say N/A.

Could you update the PR body to match the template? Once that's done, re-run with @qwen-code /triage and we'll pick it right back up.

中文说明

@LaZzyMan 感谢提交设计文档——内容很扎实,结构也清晰。

在进入审查之前有一个问题:PR 描述没有按照 PR 模板 的格式填写。模板要求以下标题:

  • ## What this PR does
  • ## Why it's needed
  • ## Reviewer Test Plan(含 How to verifyEvidenceTested on
  • ## Risk & Scope
  • ## Linked Issues

当前描述使用的是自定义标题(概述 / 四篇文档与职责边界 / 关键已定决策 / 验证 / 后续)。信息基本都有,只需要按模板标题重新组织,方便 reviewer 和工具一致地定位内容。文档类 PR 的 Evidence (Before & After) 部分写 N/A 即可。

请更新 PR 描述后,使用 @qwen-code /triage 重新触发审查。

Qwen Code · qwen3.8-max-preview

Four interlinked design docs for the omni multimodal experiment:

- file recognition & metadata: unified MediaRecognitionService, two
  normalization trigger points, URL localization, raw-resource token
  estimation, ffmpeg/ffprobe as hard dependency
- policy orchestration: MediaPolicyTool contract with per-invocation
  args + per-tool settings, fixedPolicies with condition DSL, transport
  guard on upload-channel limits, DashScope temporary-upload delivery
  (all-upload, no inline), lossy-output disclosure contract
- memory: two collection trigger points (FileRecognized /
  OmniPolicySucceeded), harness-exclusive writes, in-file graph,
  active/side-query recall sharing one protocol
- managed media storage: content-addressed object store, staging /
  quarantine lifecycle, upload cache (sha256+model -> oss URL, 48h),
  mark-and-sweep GC rooted at memory references

Verified against origin/main and validated end-to-end against the
DashScope uploads API (123KB / 21MB / 424MB video, image, audio).
@github-actions

Copy link
Copy Markdown
Contributor

Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration.

中文

请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。

@LaZzyMan

Copy link
Copy Markdown
Collaborator Author

@qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the design docs!

Template looks good ✓ — the previous template issue has been resolved.

Problem: this is a design-baseline PR, not a bug fix — the need is establishing agreed contracts for four coupled subsystems (recognition, policy, memory, storage) before implementation starts on the omni-experiment branch. That's a valid and important use of a docs PR.

Direction: aligned. Multimodal processing is an established area — the CHANGELOG shows progressive work from multi-modal input support (#1564) through multimodal routing (#7045) to payload optimization (#6045). An experiment branch with design docs as the baseline is a sound approach. The PR correctly targets omni-experiment rather than main.

Size: not applicable — no core module paths touched. All four files are in docs/design/.

Approach: the four-document structure follows the dependency chain (recognition → policy → memory → storage), and each doc scopes itself clearly with explicit non-goals. The ~4000 lines total is substantial but reasonable for four interlinked architecture docs that need to be reviewed together for cross-reference consistency. Source-code anchors (executeToolCall(recordToolResult: false), convertToFunctionResponse, DashScope converter media branches) verified against current origin/main ✓.

Risk: no elevated risk signals — docs-only change, no high-risk paths matched.

Moving on to code review. 🔍

中文说明

感谢贡献设计文档!

模板完整 ✓ —— 之前的模板问题已修复。

问题:这是一个设计基线 PR,不是 bug 修复——需求是在 omni-experiment 分支实现开始前,为四个耦合子系统(识别、policy、memory、存储)确定契约。这是文档 PR 的合理且重要的用途。

方向:对齐。多模态处理是已有方向——CHANGELOG 显示从多模态输入支持(#1564)到多模态路由(#7045)再到 payload 优化(#6045)的持续演进。实验分支加设计文档作为基线是合理的方法。PR 正确地以 omni-experiment 而非 main 为目标分支。

规模:不适用——未触及核心模块路径。四个文件均在 docs/design/ 下。

方案:四篇文档按依赖链组织(识别 → policy → memory → 存储),每篇都有明确的范围和非目标。总计约 4000 行对于四篇需要一起审查交叉引用一致性的架构文档来说是合理的。源码锚点(executeToolCall(recordToolResult: false)convertToFunctionResponse、DashScope converter 媒体分支)已对照当前 origin/main 验证 ✓。

风险:无升级风险信号——纯文档变更,未匹配高风险路径。

进入代码审查 🔍

Qwen Code · qwen3.8-max-preview

Reviewed at 154b6ab4b778893063b1fa2c744fe1e132cfd69c · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Read all four documents in dependency order (recognition → policy → memory → storage). These are unusually thorough design docs — each one has clear goals/non-goals, core decisions with rationale, typed data contracts, failure semantics tables, security constraints, and concrete acceptance criteria.

Cross-document consistency checks out. The two collection trigger points (FileRecognized / OmniPolicySucceeded) are referenced consistently across memory, policy, and storage docs. PolicyArtifactBatch is defined in the policy doc and consumed identically by the memory doc's OmniPolicySucceeded atomic commit. The managed storage areas (objects/, staging/, quarantine/, downloads/) align with the lifecycle described in policy §9 and memory §13. The upload cache (sha256+model → oss URL, 47h TTL) is defined in the storage doc and referenced by the policy doc's upload service (§10.3.2).

Source-code anchors verified against current origin/main:

  • executeToolCall(recordToolResult: false) — exists in nonInteractiveToolExecutor.ts and coreToolScheduler.ts
  • convertToFunctionResponse — exported from coreToolScheduler.ts
  • DashScope converter media branches — converter.ts has DashScope-specific handling ✓

Design quality observations (non-blocking):

  1. The omni.ingestion configuration example appears in both the recognition doc (§3.5) and the policy doc (§12.2). The docs acknowledge this and propose snapshot tests to prevent drift — reasonable, but worth tracking as a maintenance risk.

  2. The policy orchestration doc is 1862 lines. The content earns its length (condition DSL, delivery semantics, transport guard, model access, configuration, failure tables, acceptance criteria), but a future split into core architecture vs. configuration reference might help reviewers.

  3. The storage doc is comparatively concise (270 lines). The GC mark-and-sweep section defers implementation details appropriately, but the "artifact lifetime must not be shorter than active Memory references" constraint from memory §13 could benefit from a cross-reference in the GC section itself — it's mentioned in §5 but only implicitly.

No correctness bugs, no security concerns, no AGENTS.md violations. The docs stay within their stated scope and don't pre-build abstractions for follow-up work.

Testing

N/A — docs-only PR, no runtime behavior changes.

Check Conclusion
Test (ubuntu-latest, Node 22.x) ✅ success
Test (macos-latest, Node 22.x) ⏭️ skipped
Test (windows-latest, Node 22.x) ⏭️ skipped
Integration Tests (CLI, No Sandbox) ⏭️ skipped
web-shell E2E Smoke ⏭️ skipped
Post Coverage Comment ⏭️ skipped
Classify PR ✅ success
Remind on force-push ✅ success

CI is clean — ubuntu test passed, other platforms and integration tests skipped as expected for a docs-only change. No pending PR CI workflow runs.

中文说明

代码审查

按依赖顺序阅读了四篇文档(识别 → policy → memory → 存储)。这些设计文档非常详尽——每篇都有明确的目标/非目标、带理由的核心决策、类型化数据契约、失败语义表、安全约束和具体验收标准。

跨文档一致性验证通过。两个收集触发点(FileRecognized / OmniPolicySucceeded)在 memory、policy 和 storage 文档中引用一致。PolicyArtifactBatch 在 policy 文档中定义,被 memory 文档的 OmniPolicySucceeded 原子提交完全一致地消费。受管存储分区(objects/staging/quarantine/downloads/)与 policy §9 和 memory §13 描述的生命周期对齐。上传缓存(sha256+model → oss URL,47h TTL)在 storage 文档中定义,被 policy 文档的上传服务(§10.3.2)引用。

源码锚点已对照当前 origin/main 验证:

  • executeToolCall(recordToolResult: false) — 存在于 nonInteractiveToolExecutor.tscoreToolScheduler.ts
  • convertToFunctionResponse — 从 coreToolScheduler.ts 导出 ✓
  • DashScope converter 媒体分支 — converter.ts 有 DashScope 特定处理 ✓

设计质量观察(非阻塞):

  1. omni.ingestion 配置示例同时出现在识别文档(§3.5)和 policy 文档(§12.2)。文档已说明并提出用 snapshot 测试防止漂移——合理,但值得作为维护风险跟踪。

  2. Policy 编排文档 1862 行。内容对得起这个长度(条件 DSL、投递语义、transport guard、模型访问、配置、失败表、验收标准),但未来拆分为核心架构 vs. 配置参考可能有助于审查。

  3. 存储文档相对简洁(270 行)。GC mark-and-sweep 部分适当地推迟了实现细节,但 memory §13 的"artifact 生命周期不得早于 active Memory 引用"约束可以在 GC 部分增加一个显式交叉引用。

无正确性问题、无安全隐患、无 AGENTS.md 违规。文档保持在声明范围内,未为后续工作预建抽象。

测试

N/A —— 纯文档 PR,无运行时行为变化。

CI 干净——ubuntu 测试通过,其他平台和集成测试按预期跳过(纯文档变更)。无待处理的 PR CI 工作流运行。

Qwen Code · qwen3.8-max-preview

Reviewed at 154b6ab4b778893063b1fa2c744fe1e132cfd69c · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — thorough, well-structured design docs with verified source anchors and real API validation; only non-blocking nits around config example duplication and doc length.

These four documents do what design docs should do: they pin down contracts before implementation starts. The "two trigger points" constraint, the content-evidence-first principle, and the harness-exclusive write model are the kind of decisions that prevent divergent implementations later. The acceptance criteria sections are concrete enough to write tests from.

What impressed me most is the DashScope upload validation — the author didn't just read the docs, they ran 123KB / 21MB / 424MB videos through the real API, confirmed the X-DashScope-OssResourceResolve header behavior, and documented the exact limits. That's the kind of evidence that makes a design baseline trustworthy.

The non-blocking nits (config example duplication, policy doc length, storage GC cross-reference) are minor and don't affect the design's correctness. The docs correctly target omni-experiment and don't touch main.

Approving. ✅

中文说明

置信度:4/5 —— 详尽、结构良好的设计文档,源码锚点已验证,真实 API 已验证;仅有配置示例重复和文档长度等非阻塞小问题。

这四篇文档做到了设计文档应该做的事:在实现开始前确定契约。"两个触发点"约束、内容证据优先原则和 Harness 独占写入模型,是防止后续实现分叉的关键决策。验收标准部分足够具体,可以直接据此编写测试。

最令人印象深刻的是 DashScope 上传验证——作者不仅阅读了文档,还用真实 API 跑了 123KB / 21MB / 424MB 视频,确认了 X-DashScope-OssResourceResolve 请求头行为,并记录了精确限制。这种证据使设计基线值得信赖。

非阻塞小问题(配置示例重复、policy 文档长度、存储 GC 交叉引用)是次要的,不影响设计正确性。文档正确地以 omni-experiment 为目标,不触及 main

批准。✅

Qwen Code · qwen3.8-max-preview

Reviewed at 154b6ab4b778893063b1fa2c744fe1e132cfd69c · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline. Not reviewed: reverse audit — stopped after 3 consecutive productive rounds (9 → 4 → 8 new findings) without reaching two dry rounds; rounds 4-5 not run, to bound review volume on this docs-only PR.

中文说明

已审查。 建议见行内评论。 未审查:reverse audit — stopped after 3 consecutive productive rounds (9 → 4 → 8 new findings) without reaching two dry rounds; rounds 4-5 not run, to bound review volume on this docs-only PR。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +570 to +574
gaps: Array<{
scope: MediaScope;
channels: MediaChannel[];
reason: 'not_processed' | 'partial_coverage' | 'artifact_unavailable';
}>;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The gaps[].reason enum has no value for text-budget trimming, but the maxTextChars config comment (§10) requires reporting a gap when text is trimmed. An implementer wiring up maxTextChars finds only not_processed / partial_coverage / artifact_unavailable here — none means "trimmed by recall budget" — so they must misuse partial_coverage (conflating policy-coverage with recall-budget semantics and misattributing telemetry), invent an unlisted reason, or silently drop the gap report the config mandates. Suggested fix: add a fourth reason (e.g. 'text_budget_trimmed') and reference it from the maxTextChars comment.

中文说明

gaps[].reason 枚举没有"文本预算裁剪"对应的取值,但 maxTextChars 配置注释(§10)要求文本被裁剪时上报 gap。实现者接到 maxTextChars 时在这里只看到 not_processed / partial_coverage / artifact_unavailable,没有一个表示"按召回预算裁剪",只能误用 partial_coverage(把 policy 覆盖语义和召回预算语义混淆,导致 telemetry 归因错误)、自造未列出的 reason,或干脆丢弃配置所要求的 gap 上报。建议:新增第四个 reason(如 'text_budget_trimmed'),并在 maxTextChars 注释中引用。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +549 to +554
entries: Array<{
entryId: MediaMemoryEntryId;
kind: 'metadata' | 'derived_media' | 'policy_result' | 'execution';
role?: string;
content?: string;
resourceId?: string;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Policy doc §3.2 mandates the disclosure text enters Memory via NormalizedPolicyOutput.disclosure "供后续召回时复述", but this recall-result entry protocol has no disclosure field — content carries text-result bodies (transcript/OCR) and provenance carries IDs, so the stored disclosure is unreachable at recall time. A lossy policy (e.g. downsample) persists its disclosure on commit, but when the derived image is later recalled the Harness can only materialize the media Part without the adjacent disclosure Part §3.2 requires — the model sees the downsampled image without knowing what was lost. Suggested fix: add disclosure?: string to this entry type and state in §9.2/§9.3 that a recalled derived_media entry with a non-empty disclosure emits it as an adjacent text Part.

中文说明

Policy 设计 §3.2 要求披露文本经 NormalizedPolicyOutput.disclosure 进入 Memory,"供后续召回时复述",但这里的召回结果 entry 协议没有 disclosure 字段——content 承载文本结果体(transcript/OCR),provenance 承载 ID,因此已存储的披露在召回时不可达。有损 policy(如降采样)在提交时持久化了披露,但当该衍生图片后续被召回时,Harness 只能物化媒体 Part,而无法附上 §3.2 要求的相邻披露 Part——模型看到降采样后的图片却不知道丢失了什么。建议:在该 entry 类型上新增 disclosure?: string,并在 §9.2/§9.3 说明召回带有非空 disclosure 的 derived_media entry 时,将其作为相邻文本 Part 发出。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +641 to +644
"active": {
// 一次 ToolCall 最多查询多少个当前 session 资源,防止无界批量扫描。
"maxFilesPerCall": 8,
},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] maxFilesPerCall is declared with the DoS rationale "防止无界批量扫描", but nothing binds the model-controlled MediaMemoryRecallRequest.resourceIds (§9.2) to it — not §9.2, not the §10.1 validation list, not the §17.4 acceptance criteria. Compare maxSelectedEntries, which is declared, validated (maxSelectedEntries <= maxEntries), enforced (§9.3 rejects the whole result), and acceptance-tested. In active mode the model supplies resourceIds freely, so passing e.g. 200 session-authorized IDs forces 200 root-graph traversals in one call — exactly the unbounded scan this field exists to prevent — while satisfying every documented constraint. Suggested fix: state in §9.2 that resourceIds whose count exceeds maxFilesPerCall is rejected with a structured error, and add a matching §10.1 rule and §17.4 bullet.

中文说明

maxFilesPerCall 声明时带有 DoS 理由"防止无界批量扫描",但没有任何地方把模型可控的 MediaMemoryRecallRequest.resourceIds(§9.2)绑定到它——§9.2、§10.1 校验清单、§17.4 验收标准都没有。对比 maxSelectedEntries:它有声明、有校验(maxSelectedEntries <= maxEntries)、有强制(§9.3 整体拒绝)、有验收测试。active 模式下模型可自由提供 resourceIds,因此一次传入例如 200 个已授权资源 ID 会在单次调用中强制遍历 200 个根图——正是该字段要防止的无界扫描——同时却满足所有已写明的约束。建议:在 §9.2 说明 resourceIds 数量超过 maxFilesPerCall 时返回结构化错误,并补充对应的 §10.1 规则与 §17.4 验收项。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +329 to +333
disclosure?: string;
scope: MediaScope;
channels: MediaChannel[];
coverage: MediaCoverage;
parentVersionId: MediaFileVersionId;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] NormalizedPolicyOutput declares scope, channels, and coverage as required, but neither upstream contract the bridge normalizes from carries them — PolicyArtifactBatch (policy §6.3: toolName/invocationId/executionOrigin/artifacts) and MediaPolicyToolDescriptor.outputs (kind/role?/mimeTypes/required/lossy?) have no scope/channels/coverage — yet §7.2 gates the commit on these being normalized. For the well-known roles (transcript/keyframe/clip) a hardcoded mapping is guessable, but §5.6 explicitly allows custom roles, which have no guessable scope/channels/coverage, so the bridge cannot normalize them without a contract extension the doc never specifies. Suggested fix: extend MediaPolicyToolDescriptor.outputs[] with per-output scope/channels/coverage defaults (or a coverage template), or specify that ToolArtifact.metadata must carry them and add that to the §7.2 gate.

中文说明

NormalizedPolicyOutputscopechannelscoverage 声明为必需,但 bridge 归一化所依据的两个上游契约都不含这些字段——PolicyArtifactBatch(policy §6.3:toolName/invocationId/executionOrigin/artifacts)和 MediaPolicyToolDescriptor.outputs(kind/role?/mimeTypes/required/lossy?)都没有 scope/channels/coverage——而 §7.2 却以这些字段已归一化作为提交门槛。对内置 role(transcript/keyframe/clip)尚可猜测硬编码映射,但 §5.6 明确允许自定义 role,其 scope/channels/coverage 无从猜测,bridge 在没有文档未规定的契约扩展时无法归一化。建议:在 MediaPolicyToolDescriptor.outputs[] 上为每个 output 增加 scope/channels/coverage 默认值(或 coverage 模板),或规定 ToolArtifact.metadata 必须携带这些字段并加入 §7.2 门槛。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +835 to +837
2. **请求头注入**:请求包含 oss:// 媒体 Part 时,为该请求附加
`X-DashScope-OssResourceResolve: enable`(缺失时服务端确定性 400,属于
final clamp 应拦截的编程错误)。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This attributes the missing X-DashScope-OssResourceResolve header to "final clamp 应拦截的编程错误", but the final clamp's definition (§10.2) enumerates exactly two checks — every Omni media Part is a managed oss:// URL, and size/duration within guard limits — and is positioned "进入 DashScope OpenAI converter 之前", upstream of the converter that injects this header. A pre-converter clamp cannot validate a header that does not exist yet at clamp time. The same attribution is repeated in the risk table (~line 1774) and §18.3 (~line 1825). An implementer building the clamp per §10.2 would forward a request that passes both checks yet receives the deterministic HTTP 400 the doc claims is prevented. Suggested fix: either add a header-presence check to §10.2 and clarify it runs at/after the converter, or reassign header validation to the converter layer in §10.3.3, the risk table, and §18.3.

中文说明

这里把漏加 X-DashScope-OssResourceResolve 请求头归为"final clamp 应拦截的编程错误",但 final clamp 的定义(§10.2)只列举两项检查——所有 Omni 媒体 Part 必须是受管 oss:// URL、文件大小/时长在 guard 上限内——并且位置在"进入 DashScope OpenAI converter 之前",即注入该请求头的 converter 的上游。一个 converter 之前的 clamp 无法校验在 clamp 时尚不存在的请求头。同样的归因在风险表(约 1774 行)和 §18.3(约 1825 行)重复出现。按 §10.2 实现 clamp 的人会放行一个通过两项检查、却收到文档声称已被阻止的确定性 HTTP 400 的请求。建议:要么在 §10.2 增加请求头存在性检查并说明它在 converter 处/之后执行,要么在 §10.3.3、风险表和 §18.3 把请求头校验改归 converter 层。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +567 to +569
任何字段无法获得时,该 comparison 结果为 `unavailable`,不能静默当作 `false`。
运行记录必须说明缺失字段和来源状态;policy 按 `onConditionUnavailable` 的默认
`skip` 行为跳过,后续如有需要再开放 `abortTurn`。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] §8.3 defines all/any combinators and three-valued comparisons (true/false/unavailable) but never specifies how unavailable propagates through all/any. For any: [{ resource.estimatedTokenCount > session.availableContextTokens }, { session.contextWindowTokens >= 131072 }] with the first field unavailable and the second definitely true, policy-level short-circuit (any unavailable leaf → skip the whole policy) skips despite a true any branch, while three-valued propagation (any(true, unavailable) → true) executes — both readings are defensible, so two implementers produce different behavior for every any condition with a missing field. Suggested fix: state the propagation rule explicitly, e.g. "当 all/any 的任一子条件为 unavailable 时,整个 when 视为 unavailable,触发 onConditionUnavailable(不做短路求值)" — or specify short-circuit if that is intended.

中文说明

§8.3 定义了 all/any 组合子和三值比较(true/false/unavailable),但从未说明 unavailable 如何经由 all/any 传播。对于 any: [{ resource.estimatedTokenCount > session.availableContextTokens }, { session.contextWindowTokens >= 131072 }],当第一个字段不可用、第二个明确为真时:policy 级短路(任一叶子 unavailable → 跳过整个 policy)会在存在为真的 any 分支时仍跳过;而三值传播(any(true, unavailable) → true)会执行——两种解读都站得住脚,因此两个实现者会对每个含缺失字段的 any 条件产生不同行为。建议:明确传播规则,例如"当 all/any 的任一子条件为 unavailable 时,整个 when 视为 unavailable,触发 onConditionUnavailable(不做短路求值)"——若本意是短路,也请明确写出。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +152 to +153
4. 校验 `objects/` 中随机抽样对象的文件名与实际 hash 一致(廉价的损坏探测,
不做全库校验)。

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] §4.1 writes .tmp files inside objects/sha256/ab/ (same directory as the target), and §9 plus §11 both promise ".tmp 启动时清理", but this four-step startup recovery never deletes .tmp from objects/ — step 4 only sample-verifies hash consistency of existing objects. An implementer following §6.1 as the authoritative procedure leaves stale .tmp files accumulating in the object store indefinitely, contradicting §9's guarantee. Suggested fix: add a step (e.g. between 3 and 4): "删除 objects/ 下所有 .tmp 残留——它们属于中断的晋升写入,原子 rename 未完成,不可见但占空间".

中文说明

§4.1 把 .tmp 文件写在 objects/sha256/ab/ 内(与目标同目录),§9 和 §11 都承诺".tmp 启动时清理",但这里的四步启动恢复从未删除 objects/ 下的 .tmp——第 4 步只是抽样校验现有对象的 hash 一致性。按 §6.1 作为权威流程的实现者会让陈旧的 .tmp 文件在对象库中无限累积,与 §9 的保证矛盾。建议:新增一步(例如放在第 3、4 步之间):"删除 objects/ 下所有 .tmp 残留——它们属于中断的晋升写入,原子 rename 未完成,不可见但占空间"。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +157 to +159
- **根集合**:Memory store 中全部 active 记录引用的 managedId(含历史版本仍
被 provenance 引用的对象)+ 当前运行 session 的 MediaResourceRegistry 正在
使用的对象;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The root set includes only "当前运行 session 的 MediaResourceRegistry" (process-local), but §12 explicitly contemplates multi-process ("同一 project 开多个 Qwen Code 实例") and defers only the lock strategy. A directory lock serializes GC execution but does not make process A aware of process B's in-use objects: if object X leaves the shared Memory root set (record deleted, past retention) while process B begins using it, process A can acquire the lock, see X unreferenced in both the shared Memory and its own registry, and delete it — leaving B with a dangling managedId and violating "有引用对象永不删除". The grace period only narrows the window. Suggested fix: qualify the root set with the single-process assumption and note in §12 that multi-process GC needs cross-process visibility of in-use objects (e.g. a shared in-use manifest), not just a lock.

中文说明

根集合只包含"当前运行 session 的 MediaResourceRegistry"(进程本地),但 §12 明确考虑了多进程("同一 project 开多个 Qwen Code 实例"),且只推迟了锁策略。目录锁能使 GC 执行串行化,却无法让进程 A 感知进程 B 正在使用的对象:若对象 X 离开共享 Memory 根集合(记录被删、超过保留期)而进程 B 开始使用它,进程 A 可能持锁后在共享 Memory 和自身 registry 中都看不到 X 的引用并将其删除——使 B 持有悬空 managedId,违反"有引用对象永不删除"。宽限期只是缩小窗口。建议:在根集合处注明单进程假设,并在 §12 说明多进程 GC 需要对在用对象的跨进程可见性(如共享的在用清单),而不仅仅是一把锁。

— qwen3.8-max-preview via Qwen Code /review

Comment on lines +373 to +378
code:
| 'invalid_source'
| 'access_denied'
| 'download_failed'
| 'sniff_failed'
| 'resource_limit';

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] §5.2 defines download_failed as a failed code and §7.2 routes download timeouts / redirect exhaustion to it ("超时按下载失败返回"), but the §9 "失败语义" table — the document's canonical scenario→result catalogue — has no row for it (it maps not_media, generic failed, sniff_failed, approval_required, resource_limit, and probe degradation only). An implementer treating §9 as authoritative omits the most common real-world URL failure (a transfer that starts but times out or drops), and the .part cleanup for that path is unspecified (the table only specifies cleanup for user rejection). Suggested fix: add a §9 row, e.g. "URL 完整下载超时、网络失败或超过 maxRedirects → failed/download_failed,清理 .part".

中文说明

§5.2 把 download_failed 定义为 failed code,§7.2 把下载超时/重定向耗尽路由到它("超时按下载失败返回"),但 §9 的"失败语义"表——本文档权威的场景→结果目录——没有对应行(它只映射 not_media、通用 failedsniff_failedapproval_requiredresource_limit 和 probe 降级)。把 §9 当作权威的实现者会漏掉最常见的真实 URL 失败(传输已开始但超时或中断),且该路径的 .part 清理也未规定(表中只规定了用户拒绝时的清理)。建议:在 §9 增加一行,例如"URL 完整下载超时、网络失败或超过 maxRedirects → failed/download_failed,清理 .part"。

— qwen3.8-max-preview via Qwen Code /review

@wenshao

wenshao commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /takeover

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🚫 Takeover not engaged: the loop only manages PRs that target main, and this one targets omni-experiment. A stacked PR moves whenever its base branch does, so "new feedback since the last round" and base-conflict resolution are not well defined until the base lands. Retarget this PR to main once the base PR merges and re-run @qwen-code /takeover — or take over the base PR instead.

中文说明

🚫 未接管:循环只管理以 main 为 base 的 PR,而本 PR 的 base 是 omni-experiment。堆叠 PR 会随 base 分支移动,因此“自上一轮以来的新反馈”与 base 冲突处理都无法良定义。待 base 的 PR 合入后将本 PR 改为面向 main 并重新执行 @qwen-code /takeover;或改为接管 base 那个 PR。

@wenshao

wenshao commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /takeover

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🚫 Takeover not engaged: the loop only manages PRs that target main, and this one targets omni-experiment. A stacked PR moves whenever its base branch does, so "new feedback since the last round" and base-conflict resolution are not well defined until the base lands. Retarget this PR to main once the base PR merges and re-run @qwen-code /takeover — or take over the base PR instead.

中文说明

🚫 未接管:循环只管理以 main 为 base 的 PR,而本 PR 的 base 是 omni-experiment。堆叠 PR 会随 base 分支移动,因此“自上一轮以来的新反馈”与 base 冲突处理都无法良定义。待 base 的 PR 合入后将本 PR 改为面向 main 并重新执行 @qwen-code /takeover;或改为接管 base 那个 PR。

@tanzhenxin tanzhenxin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@yiliang114
yiliang114 merged commit dd188b0 into omni-experiment Aug 4, 2026
135 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants