feat(omni): S2 input expansion — image/audio/URL sources and token-dimension transport guard - #8512
Conversation
… guard Extends the S1 video-only upload delivery to the full S2 input surface: - recognition: generalized to image/audio/video with content sniffing (magic bytes for png/jpeg/webp/gif, mp3/wav/flac/ogg/m4a, plus the S1 video set) and per-modality ffprobe metadata; modality mismatches between sniff and reference fail closed - delivery: all modalities upload ORIGINAL bytes — no resizing or transcoding on the default path (lossy transforms are the job of omni policies, which must disclose); images skip renderImageOverview when omni is active and carry a dimensions+zoom-hint text part instead - converter: new fileData→input_audio branch passing the bare oss:// URL (previously audio fileData silently textified as 'Unsupported file media type' after a successful upload); getAudioFormat widened to flac/ogg/m4a in lockstep with the recognizer - URL inputs: @https://… is intercepted ahead of filesystem resolution (previously silently dropped via the ENOENT skip), streamed to downloads/ with SSRF/redirect policy mirroring fetchWithPolicy, hashed while writing, re-recognized from local bytes, and delivered through the same pipeline; per-URL failure isolation keeps the turn alive - token estimation: versioned raw-resource estimator (method raw-resource-v1) attached to read results as the single tokenEstimate location; formula is a swappable slot pending provider confirmation - transport guard: byte ceiling (S1) plus omni.transport.maxEstimatedTokens (default 0 = disabled — observability only until the formula is confirmed; positive values fail closed before any copy/upload) - second normalization trigger point: processToolResultOmniMedia converts inline tool-result media to oss:// fileData at both physical funnels (CoreToolScheduler terminal sites and ACP Session.runTool), as a sibling of the vision bridge — converted parts are invisible to isImagePart so bridge behavior is untouched Part of the omni-experiment S2 slice (#8184).
The no-file-paths early return in resolveAtCommandQuery did not count urlMediaRefs, so a prompt whose only @-reference was a URL ran the full download → recognize → store → upload pipeline and then discarded both the delivered fileData parts and their display cards. Found by S2 E2E verification (the model improvised around the missing image via its own tools, masking the drop). Part of the omni-experiment S2 slice (#8184).
Security lens: - tool-result media funnel: per-result upload budget (8 parts / 128 MiB aggregate — a malicious tool result must not fan out unbounded uploads from the user's account); excess parts stay inline - modality gate on tool-result parts now keyed to the SNIFFED modality, not the declared MIME type (a part declared audio whose bytes are a video container no longer bypasses a video-disabled config) - fs error messages are path-scrubbed before reaching model-visible error text (basenames survive, absolute paths do not) Correctness lens: - non-sniffable media formats (bmp/tiff/heic images, exotic containers) fall back to the baseline inline path via a cheap content pre-sniff instead of failing closed — fail-closed remains for genuine pipeline failures, not for formats the recognizer simply does not support - user cancellation during URL media download ends the turn gracefully instead of rejecting through resolveAtCommandQuery (which has no throw contract) into an unhandled-rejection banner Part of the omni-experiment S2 slice (#8184).
LaZzyMan
added
the
omni-experiment
|
🔄 Qwen Triage is running — watch live progress. Stage results will post in this thread as they complete. 🔄 Qwen Triage 正在运行 —— 查看实时进度。各阶段结果完成后会更新在本线程。 |
The serve fast-path bundle-closure CI check failed: iconv-lite encoding
tables (553KB) became statically reachable from the acpAgent chunk.
Root cause: the CLI-side dynamic imports of the FULL core barrel
(`import('@qwen-code/qwen-code-core')` in atCommandProcessor) request
the whole namespace object, defeating tree-shaking of the barrel — which
statically re-exports sync-file-encoding → iconvHelper → iconv-lite
tables — and esbuild folds a dynamic import of an already-statically-
imported module into a static chunk edge.
Fixes:
- add a `./omni` subpath export to @qwen-code/qwen-code-core (pattern:
existing ./transcriptRecords) and point every CLI-side dynamic import
at it, so only the omni module graph is requested;
- re-export processToolResultOmniMedia from omni/index (circular-safe:
both modules only bind functions) so the subpath serves the
tool-result funnel;
- coreToolScheduler and ACP Session load the funnel via dynamic import
(mirroring fileUtils' existing pattern for the omni module).
Verified: `npm run check:serve-fast-path-bundle` passes; bisected
against base in a clean worktree to isolate the offending edge.
Part of the omni-experiment S2 slice (#8184).
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. |
The previous fix added a `./omni` subpath export to @qwen-code/qwen-code-core, but vitest/tsc in this monorepo resolve the core package through explicit aliases to TS sources rather than the package exports map — so every suite loading atCommandProcessor or the ACP Session failed with "Failed to resolve import '@qwen-code/qwen-code-core/omni'". Add the subpath alias alongside the existing transcriptRecords/goalWire precedents in cli/acp-bridge/sdk-typescript vitest configs and the cli/acp-bridge tsconfig paths. Part of the omni-experiment S2 slice (#8184).
The self-hosted CI runners export QWEN_RUNTIME_DIR for their own qwen job tooling. Storage.getRuntimeBaseDir gives that env var top priority, so logger.test.ts (HOME-derived path expectations) and the ACP Session runtime-pinning tests (Storage.setRuntimeBaseDir-based expectations) fail whenever a PR lands on such a runner — independent of the change under test. Reproduced locally with QWEN_RUNTIME_DIR=/tmp/fake-spool. Clear the variable inside the affected suites (restored after each test), making them hermetic on any runner.
|
Thanks for the S2 slice! Template looks good ✓ Problem: observed, not theoretical. This is the tracked roadmap slice (#8184, roadmap #8197) building on S1 (#8422, merged into Direction: aligned. Second vertical slice of an established experiment with S1 already merged; everything stays behind the S1 gate and targets the Size: core paths touched heavily — 1496 production lines vs 667 test lines (plus 24 schema and 18 build-config lines). The author is a maintainer (admin), so the two-tier core gate doesn't apply; numbers recorded for transparency. Large but coherent: S2 is the "full input surface behind one gate" vertical slice, and splitting it up would fragment that design. Approach: the scope feels right. The structure mirrors S1 cleanly (recognize → guard → store → upload), reuses the existing Risk: two changed files match the revert-history high-risk paths — Moving on to code review. 🔍 中文说明感谢 S2 切片！ 模板完整 ✓ 问题： 已观测到、非理论性问题。这是 roadmap 跟踪的切片（#8184，总纲 #8197），建立在 S1（#8422，2026-08-03 合入 方向： 对齐。这是既定实验的第二个垂直切片，S1 已合入；所有行为都在 S1 门控之后、目标是 规模： 大量触及核心路径——生产逻辑 1496 行 vs 测试 667 行（另有 24 行 schema、18 行构建配置）。作者是 maintainer（admin 权限），双层核心门禁不适用，数字仅作透明记录。体量大但内聚：S2 是"一个门控后的完整输入面"垂直切片，拆分会打碎该设计。 方案： 范围合理。结构清晰复用 S1（识别 → 守卫 → 存储 → 上传），SSRF/重定向策略复用现有 风险： 两个变更文件命中 revert 历史高风险路径—— 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI wrote my independent proposal before reading the diff (extend the S1 recognizer to three modalities, add per-modality converter branches, intercept What I verified by reading the code, not just the description:
Non-blocking observations (author's call, none of these gate merge):
The main new runtime flow, for orientation: sequenceDiagram
participant P1 as User prompt
participant P2 as at-command processor
participant P3 as tool-result funnel
participant P4 as download module
participant P5 as omni pipeline
participant P6 as DashScope upload
participant P7 as Model request
P1->>P2: mentions media by path or URL
P2->>P4: stream URL to downloads (SSRF policy, hashing)
P4-->>P2: temp file with sha256
P2->>P5: sniff, probe, byte and token guards, store
P3->>P5: inline tool-result media (budgeted)
P5->>P6: upload original bytes
P6-->>P5: oss URL
P5-->>P7: fileData part (image_url, input_audio or video_url)
Files changed (30 of 31 shown)
Test evidence (the PR's own CI)Unattended run — PR code is never built or executed here; the evidence below is the PR's own CI read through the API at the reviewed commit. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The skipped jobs are this repo's normal CI shape — macOS/Windows and the integration suite run only in the merge queue; the ubuntu suite is the PR signal. It was still running at review time (started 11:27 UTC, and this repo's full suite takes ~30 minutes), so there is no green attestation yet — the finalize job rewrites the table above once CI settles. Not verified: real-API behavior (19/19 E2E assertions on qwen3.5-omni-plus, byte-exact object invariant) — that is the author's claim from the PR description, not independently re-run here. Sandboxed verification would settle the remaining gap: 中文说明代码审查： 在读 diff 之前我先写了独立方案（把 S1 识别器扩到三模态、converter 按模态分支、在文件系统解析前拦截 已逐行核实（而非只看描述）：vision bridge 互斥真实成立（bridge 仅在 image 模态关闭时运行，omni 要求 modalities.image，二者不可能吞掉同一文件）；SSRF/重定向策略复用现有 非阻塞建议：错误信息清洗目前只覆盖 POSIX 路径（Windows 路径仍可能进入模型可见内容，较 S1 已是改进）；atCommandProcessor 中 omni 动态导入发生在门控检查之前（影响可忽略）；六个 S1 弃用别名建议在实验毕业时清理。 测试证据（本 PR 自己的 CI）： 无人值守运行——此处不构建、不执行 PR 代码。ubuntu 套件在审查时仍在运行（约需 30 分钟），macOS/Windows/集成测试按仓库设计仅在合并队列运行。上表由 finalize 任务在 CI 落定后更新。真实 API 行为（19/19 E2E 断言等）为作者在 PR 描述中的声明，未在此独立复跑。 沙箱验证可补齐剩余缺口： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, well-scoped vertical slice with no blockers; the holdouts are the still-running ubuntu suite and real-API behavior that rests on the author's evidence. Honest reflection: the PR matches my independent proposal everywhere I had one, and beats it where I didn't — I hadn't thought through the pre-sniff fallback for formats the recognizer can't handle, and the sniffed-modality gate on tool results is a stricter call than I would have made. The scope question ("cut 80%?") doesn't bite here: S2's whole point is the full input surface behind one gate, and a single-modality remnant would defeat the slice. Nothing in the diff feels like it's trying too hard — the fail-closed discipline, the versioned estimator slot, and the observe-only default are proportional to a media pipeline whose failure mode is "the model silently didn't see what the user sent". Every change earns its place except the two self-hosted-runner test fixes, and those are justified environment repairs. The property I cared about most — gate-off paths staying byte-identical — holds by construction: the omni branch in The 1/5 point I'm withholding: CI on the reviewed commit was still in flight when this review finished, and the real-API claims (19/19 E2E assertions, byte-exact object invariant on qwen3.5-omni-plus) are the author's, not independently re-run. So approval is deferred until CI lands green on 中文说明置信度：4/5 —— 干净、范围得当的垂直切片，无阻塞项；剩余的是仍在运行的 ubuntu 套件，以及基于作者证据的真实 API 行为。 反思：PR 与我独立写出的方案处处一致，并在我没想到的地方更好——我没有考虑到不可嗅探格式的预嗅探回落，工具结果上以嗅探模态作门也比我自己的选择更严格。"砍掉 80% 会怎样"在这里不成立：S2 的意义就是"一个门控后的完整输入面"，只留单一模态会摧毁这个切片。diff 中没有过度设计——fail-closed 纪律、版本化估算器槽位、默认仅观测，与"模型静默没看到用户发送内容"这种失败模式的媒体管线是相称的。除两处自托管 runner 测试环境修复外每个改动都有必要，而这两处是合理的环境修复。 我最看重的性质——门控关闭路径逐字节一致——在构造上成立：fileUtils 的 omni 分支三重门控（投递激活、模态开启、预嗅探匹配），converter 新分支只对先前会被文本化的音频 fileData 生效。六个月后回看，这更像一个有边界的模块（omni/ 拥有管线，漏斗只调用它），而不是涂抹在代码树各处的功能。 扣掉的 1 分：审查结束时该 commit 的 CI 仍在运行；真实 API 证据（19/19 E2E 断言、qwen3.5-omni-plus 上的字节不变式）来自作者声明，未独立复跑。因此批准推迟到 CI 在该 commit 全绿之后——若有检查变红，推迟的批准将被扣留。 — Qwen Code · qwen3.8-max Reviewed at |
The `await import()` of the omni graph inside processSingleFileContent and resolveAtCommandQuery performs a real filesystem read under vitest (the SSR transform writes and reads the module under /tmp/RP_*/ssr/), so mock-fs based suites — pathReader's image cases — failed with ENOENT on a path they never mocked. Gate both call sites on the cheap synchronous config.isOmniEnabled() before reaching for the import. Sessions without omni configured (every existing test suite, and every user who has not opted in) now never load the module, which also avoids the wasted module load on the common path. Delivery behavior when omni IS enabled is unchanged: isOmniDeliveryActive still performs the full five-condition gate check afterwards.
|
@qwen-code /verify |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 170 passed · 0 failed · 170 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：170 通过 · 0 失败 · 170 总计 Verification reportPR 8512 verification —
|
| cell (gate on) | head (S2) | base (S1 control) |
|---|---|---|
photo.png (image) |
text+oss — 1 getPolicy + 1 upload, uploaded sha256 == source sha256; dimensions+zoom-hint text part; raw-resource-v1 estimate attached |
text+inline, 0 uploads (A/B control) |
tone2s.wav (audio) |
oss — 1 upload, byte-exact |
inline, 0 uploads (A/B control) |
clip2s.mp4 (video) |
oss — 1 upload, byte-exact |
oss — 1 upload, byte-exact (S1 preserved) |
| gate-off image/video | inline, 0 uploads | inline, 0 uploads — llmContent deep-equal head vs base |
gate-on image.bmp (unsniffable) |
inline, 0 uploads (pre-sniff fallback) | inline — llmContent deep-equal head vs base |
Flip count: 2/3 modalities (image, audio) move inline→oss; video and all gate-off/fallback paths unchanged. Every getPolicy carried Bearer sk-fake-static-key and the model param; OSS PostObject field order (policy fields before file) observed on the wire. Assertions: head 18/18, base 8/8, cross-build identity 3/3.
Secondary claim 1 — token-dimension transport guard
| cell | result |
|---|---|
| default (unset threshold), video | delivers; estimate == ceil(w·h·ceil(dur·fps)/2048) recomputed from recognized.metadata (self-consistent oracle), method raw-resource-v1, status ok |
| audio default | estimate == ceil(seconds·7); image == ceil(px/2048) |
maxEstimatedTokens=1 |
OmniTransportGuardError naming estimate/method/omni.transport.maxEstimatedTokens; 0 getPolicy, 0 uploads, 0 object-store files (rejected before copy/upload) |
maxEstimatedTokens=100000 |
delivers |
| unavailable estimate + positive threshold | returns {status:'unavailable', estimatedTokenCount:0}, never rejects |
byte dimension (maxFileBytes=1000) |
throws before upload on head and base (S1 control, 5/5) |
Assertions: guard head 22/22, guard base 5/5.
Secondary claim 2 — URL source policy (downloadMediaUrl)
53/53. Highlights: happy path streams to .part with hash-while-write (sha256(body) == reported == on-disk); 404 → isolated OmniDownloadError + no .part leftover; SSRF refusals for 127.0.0.1, localhost, 10.x, 192.168.x, 172.16.x, 169.254.169.254 (cloud metadata), CGNAT 100.64.x, [::1], single-label and .internal hosts — fetch spy never invoked; same-host (www-strip) redirect followed, cross-origin and redirect-to-loopback refused; redirect loop capped; Content-Length ceiling enforced pre-body and actual-bytes ceiling mid-stream with .part cleanup; user abort rejects and removes .part. Witness: evidence/04-wire-oracle-harnesses.png.
Tool-result funnel (processToolResultOmniMedia)
25/25. Inline image part → oss:// fileData with byte-exact upload; 9 parts → exactly 8 uploads (9th inline); single >128 MiB part never uploads and array identity preserved; part declared audio/mp4 whose bytes sniff as video stays inline when video is disabled (sniffed-modality gate) while the video-declared control converts; non-media bytes → same array identity; gate off → same identity; nested functionResponse.parts converted in place; injected OSS 500 on first upload keeps that part inline while the second still converts.
Converter wire shape (head vs base)
Head 21/21, base 15/15. Head converts oss:// audio fileData to input_audio with the bare URL (no data: wrapper) for mpeg/wav/flac/ogg/m4a; base has no such branch (textified placeholder) — the A/B control. audio/amr stays a placeholder on both; image→image_url and video→video_url parity intact; inline audio keeps its data: URI on both.
Mutation matrix (vacuity)
| mutant | suite | outcome |
|---|---|---|
| M1 guard.ts: token throw disabled | guard.test.ts | killed — token guard > enabled: rejects above threshold… (1F/6P) |
| M2 download.ts: SSRF gate disabled | download.test.ts | killed — refuses private/loopback hosts (SSRF) (1F/6P) |
| M3 tool-result-media.ts: declared-MIME gate | tool-result-media.test.ts | killed — gates on the SNIFFED modality… (1F/4P) |
| M3b tool-result-media.ts: budget 8→9 | tool-result-media.test.ts | killed — enforces the per-result upload-count budget (expected "spy" to be called 8 times, but got 9 times) |
| M4 converter.ts: new audio-fileData branch disabled | converter.test.ts | killed — exactly the two new tests (2F/215P); clean attribution |
| control (unmutated) | omni + converter | 289/289 green |
Witnesses: evidence/02-mutation-matrix-m1-m3.png, evidence/03-mutation-matrix-m3b-m4-control.png. No survivors among the guards this PR introduced. (M4's first attempt mutated the wrong — inline — occurrence and killed two pre-existing inline tests; the corrected second-occurrence mutation is the one reported, which also demonstrates the inline and fileData branches are independently pinned.)
Findings (non-blocking)
- Suggestion — no in-repo regression test for the URL media path, including the URL-only-prompt fix.
atCommandProcessor.tsgained ~120 lines (URL interception, localization loop, and theurlMediaRefs.length === 0early-return fix from commit14409cb), but neitheratCommandProcessor.test.tsnoratCommandProcessor.session.test.tsmentions URLs. Proof: reverting the early-return condition leaves both suites fully green (69/69). The PR's own E2E round covers this externally, but an in-repo test would pin a defect that already shipped once within this PR's development. House rule classifies missing tests as a Suggestion. - Nit — test count drift. The PR text says
npx vitest run src/omni/is "77 tests"; measured at the verified head: 72 tests in 9 files (all passing). - Note (pre-existing semantics, mirrored by design) — SSRF gate is URL-string based.
isPrivateHostnever resolves DNS, so a public hostname A-recording to a private IP would bypass the gate.download.tsdeliberately mirrorsfetchWithPolicy(same property), so this is a pre-existing platform limitation, not a regression; worth a platform-level look eventually (DNS pinning / post-resolution re-check). - Note (environmental) —
logger.test.tsfails identically in every arm here. 31F/21P across head/base × {no env,QWEN_RUNTIME_DIRmissing dir, existing dir} — six identical cells, so the failure is this container's environment (A/A parity), not the PR and not the hermeticity fix of commitf048f0d. The fix is neither credited nor blamed by this round; its target scenario (a runner exportingQWEN_RUNTIME_DIR) could not be distinguished here because the suite fails for another environmental reason regardless.
Not covered
- Real-model E2E (no DashScope credential in this sandbox), TUI download cards / Esc-during-download UX, ACP
Session.runToolend-to-end, Qwen OAuth behavior beyond the gate predicate, download consent UX (deferred by design). - Per-commit attribution: the checkout is depth-2 (
git rev-list HEAD^1..HEAD^2returns 1 while the metadata lists 8 commits — the shallow-boundary trap); all intermediate commits are unreachable, so the aggregateHEAD^1..HEADdiff was verified instead. - Repo-wide test suite and repo-wide typecheck; targeted gates only: core
src/omni/(72/72) + converter (217/217 incl. new), cliatCommandProcessor*+settingsSchema(105/105),Session.test.ts(460/460),config.test.ts(315/315), typecheck core + cli (exit 0),check:serve-fast-path-bundle(pass; liveness proven — mutating the ACP dynamic import to a full-barrel import fails the gate withiconv-lite … chunk-… (553070 bytes) … static path: acpAgent → chunk, witnessevidence/05-closure-gate-pass-and-liveness.png). - The verify container ships no ffmpeg/ffprobe; I installed static builds (ffmpeg 7.0.2 / ffprobe 4.0.2) into a scratch dir on PATH — the omni pipeline's hard prerequisite.
- URL success-path cells use an injected
fetchFnencoding real HTTP semantics, because loopback hosts are — correctly — refused by the SSRF gate and the container has no public egress; the SSRF/refusal cells run against the real URL parser with a fetch spy asserting zero calls.
Methodology
Environment: CI merge-ref checkout (HEAD = merge, HEAD^1 = base tip, HEAD^2 = verified head), npm ci + npm run build pre-run at head; base control built in a scratch worktree at HEAD^1 (tsc --build of packages/core only, reusing the root node_modules — the PR leaves the lockfile untouched; harnesses import base/head dist by absolute path and assert realpath provenance before running). The gate was made active against the loopback fake origin via the real DASHSCOPE_PROXY_BASE_URL proxy-match seam plus a static-key stub Config (configuration seam only — the pipeline, uploader, recognizer, store, and converter are the real compiled dist code). The fake DashScope origin emulates getPolicy + OSS PostObject and records both sides of the wire; byte-exactness asserted by sha256 of the multipart file part vs the source. Assertions: 170 pass / 0 fail / 170 total (per-suite counts in logs/*.log [COUNTS] lines). Raw logs, harness .mjs files, cell-result JSONs, and the five evidence PNGs live in this artifact directory; mutation and gate liveness runs restore every mutated file (verified by empty git status afterwards).
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
中文说明
— claude-opus-4-6 via Qwen Code /review (v0.21.5)
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
中文说明
— claude-opus-4-6 via Qwen Code /review (v0.21.5)
|
Qwen Code review timed out. Qwen review timed out after 21600 seconds (of the 360-minute budget). This run already used the maximum 360 minute timeout. See workflow logs. |
There was a problem hiding this comment.
[Critical] The new URL-media path's SSRF gate at packages/core/src/omni/download.ts:79 only calls isPrivateHost(url), which classifies the hostname text and literal IPs but never resolves a hostname. A dotted public-looking hostname is therefore accepted even when its A/AAAA record resolves to 127.0.0.1, 169.254.169.254, or a private RFC1918 address (including DNS-rebinding and split-horizon cases); the global fetch at line 109 then connects to that address. With a media-producing internal endpoint, the response is also promoted and uploaded, so this defeats the new path's stated private and loopback SSRF protection and can disclose local or cloud-internal data.
Resolve and validate every target hostname to globally routable addresses immediately before each outbound fetch, including accepted redirect hops, rejecting the request if any candidate is private, loopback, link-local, or otherwise non-public. Please add a regression test using a hostname that appears public syntactically but resolves to a private address.
SSRF (review-blocking): downloadMediaUrl's gate called only isPrivateHost,
which classifies hostname text and IP literals but never resolves. A
syntactically public name whose A/AAAA record points at 127.0.0.1,
169.254.169.254 or an RFC1918 address was therefore accepted, and because
this path uploads the fetched bytes to DashScope, an internal endpoint's
response would be exfiltrated.
Add findNonPublicAddress() to utils/fetch.ts: it resolves the hostname via
dns.lookup({all: true}) and rejects if ANY returned address is non-public,
since the connect may pick any of them. IP literals short-circuit without a
lookup. Extract isPrivateAddress() as the shared bare-address classifier so
isPrivateIp keeps one implementation.
download.ts calls it inside the redirect loop, immediately before each
connect, so every accepted hop is re-validated — a permitted same-host
redirect can still re-resolve to a private address between hops.
Two deliberate choices, both recorded in the doc comment: the helper is NOT
wired into fetchWithPolicy, because WebFetch intentionally supports intranet
FQDNs resolving to private addresses, so applying it there would be a silent
breaking change; and DNS failure returns null rather than failing closed,
because the connect that follows reports the real error and failing closed
would turn a resolver blip into something the user cannot act on. The
residual check-then-connect race is documented too: fetch resolves again, so
closing it fully needs an agent that pins the connect to a vetted address,
which undici does not expose portably.
Also fixed: sniffMediaType classified any file starting with 0xFF 0xFE (the
UTF-16 LE BOM) as audio/mpeg, because 0xFE passes the frame-sync mask. Real
MPEG frames never use 0xFE — the layer bits would be 11, reserved — so
excluding it costs no genuine detection and keeps the "null for non-media"
contract honest for callers without a secondary modality gate.
Tests for the previously uncovered branches reviewers flagged:
- download: public-looking host resolving to 127.0.0.1/169.254.169.254/
10.1.2.3; mixed A records where only one is private; per-hop re-check
asserting the redirect is refused before the second fetch; DNS failure;
the ?? fetch fallback via a globalThis spy; and the production DNS path
asserting dns.lookup receives {all: true, verbatim: true}
- readMediaViaOmniDelivery: image gets the resolution + zoom_image hint
parts, audio gets a bare fileData, failure yields an error result and
never inline base64
- probeMediaMetadata: the audio branch reads the audio stream (proven with a
file carrying both h264 and aac), the image branch reports no duration
- sniffFileModality: recognized headers, non-media, and an absent path
- atCommandProcessor URL pipeline: a URL with omni active becomes a fileData
part, the same URL with omni disabled falls through as text without
downloading, and a URL-only prompt does not hit the no-content early
return
|
Warning Superseded. The approach described below (preflight Thanks — all findings are addressed in the latest push. Critical (SSRF, @qqqys)You're right that
Two deliberate choices worth flagging for review:
The residual check-then-connect race is documented in the helper's doc comment: Regression tests ( Suggestions (@doudouOUC)
On the
|
|
@qwen-code /verify |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 188 passed · 0 failed · 188 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：188 通过 · 0 失败 · 188 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) - follow-up round at new head Ran the PR in an isolated, token-free container: A/B against the base build and the previous verified head, mock-free harness assertions (including an LD_PRELOAD Scripted assertions: 188 passed · 0 failed · 188 total 中文 — 判定：✅ 通过 · 可合入（agent 判定，跟进轮）本轮在隔离、无凭证容器中对新头
Verification reportPR 8512 verification (follow-up round) —
|
| # | finding (prev round) | severity | status at fa35283 |
|---|---|---|---|
| 1 | no in-repo regression test for the URL media path incl. the URL-only-prompt fix | Suggestion | fixed — fa35283 adds 3 URL tests to atCommandProcessor.test.ts; reverting the urlMediaRefs.length === 0 early-return fix (mutant M7) now kills 2 of them (localizes a media URL into a fileData part…, does not hit the no-content early return for a URL-only prompt). Agree. |
| 2 | test-count drift: PR text says 77 omni tests | Nit | stands (nit) — measured at new head: 89 tests in 9 files (all passing; the delta added tests). PR body still says 77. |
| 3 | SSRF gate URL-string based, no DNS resolution | Note | fixed by the delta — see "Delta claim 1" below: 5 hole cells flip between prev-head and head; per-hop re-check physically observed. Residual check-then-connect race and the deliberate non-wiring into fetchWithPolicy are documented in the code; agree with the documented rationale. |
| 4 | logger.test.ts fails identically in every arm (environmental) |
Note | stands (environmental) — re-measured: 31F/21P at new head; base tree A/A cell identical 31F/21P (same failure names). Not the PR, not the f048f0d hermeticity fix. |
Central claim + A/B (re-measured at new head)
Central claim (unchanged): with the omni gate active, image/audio/video inputs are delivered as oss:// fileData via the DashScope upload channel with original bytes as-is; base (S1) had this path only for video. Re-driven against the real compiled fileUtils.processSingleFileContent (head dist vs base worktree rebuilt from HEAD^1; provenance asserted via realpath, zero internal @qwen-code imports in the loaded dists). Witness: evidence/01-ab-delivery-base-vs-head.png (live re-run).
| cell (gate on) | head (S2) | base (S1 control) |
|---|---|---|
photo.png (image) |
text+oss — 1 getPolicy + 1 upload, uploaded sha256 == source; 8x8 dimensions + zoom-hint text part; estimate 1 (raw-resource-v1) |
text+inline, 0 uploads |
tone2s.wav (audio) |
oss — 1 upload byte-exact; estimate 14 == ceil(2.0s×7) |
inline, 0 uploads |
clip2s.mp4 (video) |
oss — 1 upload byte-exact; estimate 40 == ceil(64·64·ceil(2·10)/2048) |
oss — 1 upload byte-exact (S1 preserved) |
| gate-off image/video | inline, 0 uploads | inline — llmContent deep-equal head vs base |
gate-on image.bmp |
inline, 0 uploads (pre-sniff fallback) | inline — llmContent deep-equal |
Flip: 2/3 modalities (image, audio) inline→oss; video + all gate-off/fallback paths unchanged. Object store holds byte-exact objects for all three modalities; OSS multipart field order policy-fields-before-file; every getPolicy carries Bearer sk-fake-static-key + model. Counts: head 34/34, base 8/8, cross-build identity 3/3.
Token guard (head): maxEstimatedTokens=1 rejects with estimate/method/setting in the message and 0 getPolicy / 0 uploads / 0 objects (fail closed before copy/upload); 100000 delivers; unavailable estimate never rejects. Byte dimension rejects pre-upload on head and base.
Delta claim 1 — DNS-before-fetch SSRF gate (the load-bearing fix)
Method: an LD_PRELOAD getaddrinfo shim (compiled with gcc, harness/dns_shim.c) answers controlled A records for *.verify-ssrf.test names and delegates everything else — the production resolver path (findNonPublicAddress → dns.lookup({all:true,verbatim:true}) → libc) runs unmodified; the injected fetchFn is a recording spy so no real connect ever happens. Witness: evidence/02-dns-ssrf-ab-head-vs-prevhead.png.
| cell | head fa35283 |
prev-head 88d7e34 (control) |
|---|---|---|
| public-looking name → 127.0.0.1 | refused pre-fetch, 0 fetches, "resolves to 127.0.0.1", no .part |
accepted, 1 fetch (hole) |
| name → 169.254.169.254 (metadata) | refused | accepted (hole) |
| name → 10.1.2.3 (RFC1918) | refused | accepted (hole) |
| mixed A records (public+private) | refused (ANY-private) | accepted (hole) |
| same-host redirect re-resolving to 127.0.0.1 | refused at hop 2 after exactly 1 fetch | follows rebind, 2 fetches (hole) |
| name → all-public | downloads, sha256-match (symmetric control) | downloads (control) |
| public IP literal | accepted with zero resolver lookups (short-circuit) | n/a |
private IP literal / text-gate set (18 hosts incl. CGNAT, [::1], .internal, single-label) |
refused by pre-existing text gate, 0 fetches | same (unchanged surface) |
| DNS failure | gate passes (fail-open by design), fetch proceeds | same |
5/5 hole cells flip. The shim's lookup log is the physical witness: head resolves each refused name once (twice for the rebind: 93.184.216.34 then 127.0.0.1); prev-head's log is empty — it never resolves. Counts: head 22/22, prev-head 9/9.
Delta claim 2 — UTF-16 BOM no longer mis-sniffed as MPEG
[FF FE …] → null at head vs audio/mpeg at prev-head (byte-level and file-level sniffFileModality); five genuine sync headers (FF FB/E0/E2/FF/FA) + ID3 still detect on both arms; png/wav/mp4 fixtures still classify. 12/12 per arm.
Delta claim 3 — new regression tests are load-bearing
Mutant M7 (revert urlMediaRefs.length === 0 in the early return) kills the two new atCommandProcessor tests; M8 (image zoom-hint part removed) kills adds a resolution + zoom_image hint part for images; M9 (fail-closed catch mutated to emit inline base64) kills fails closed with an error result (never inline base64) on failure; M5 (per-hop DNS gate disabled) kills exactly the four new download tests, with behavioral mismatches (missing resolves to error, dns.lookup spy never called with the host, rebind loop instead of refusal) — not import/compile breakage.
Secondary claims (re-measured)
- Download policy 38/38: hash-while-write, 404 isolation +
.partcleanup, 18 SSRF text-gate refusals with zero fetches, www-strip same-host redirect followed, cross-origin / loopback / credentialed redirects refused, loop capped at 6 fetches, Content-Length ceiling pre-body, lying-CL mid-stream ceiling with.partcleanup, user abort rejects and removes.part. - Tool-result funnel (head 12/12): byte-exact conversion; 9 parts → exactly 8 uploads; 64MiB converts then 65MiB > remaining stays inline (byte budget); declared-audio/video-bytes stays inline under video-disabled while video-declared control converts (sniffed gate); identity preserved for non-media and gate-off; nested
functionResponse.partsconverted; injected OSS 500 isolates one part. - Converter head 10/10 vs base 8/8:
oss://audio fileData →input_audiowith bare URL for mpeg/wav/flac/ogg/m4a (base: placeholder, A/B control);audio/amrplaceholder on both; audio-disabled placeholder; image/video parity; inline audio keepsdata:URI on both.
Mutation matrix (10 mutants, 0 survivors)
| mutant | suite | outcome |
|---|---|---|
| M1 guard token-throw disabled | guard.test.ts | killed (1F/6P) |
| M2 download SSRF text gate off | download.test.ts | killed (1F/13P) |
| M3 funnel sniffed→declared gate | tool-result-media.test.ts | killed (1F/4P) |
| M3b funnel budget 8→9 | tool-result-media.test.ts | killed (1F/4P) |
| M4 converter audio-fileData branch off | converter.test.ts | killed (2F/215P, the two new tests) |
| M5 download per-hop DNS gate off | download.test.ts | killed (4F/10P, the four new DNS tests) |
| M6 BOM exclusion removed | recognition.test.ts | killed (1F/14P) |
| M7 URL-only early-return reverted | atCommandProcessor.test.ts | killed (2F/62P, the two new URL tests) |
| M8 image zoom-hint part removed | index.test.ts | killed (1F/11P) |
| M9 fail-closed catch → inline fallback | index.test.ts | killed (1F/11P) |
Witness: evidence/03-mutation-matrix-summary-and-m5-live.png (summary + live M5 re-run). Unmutated controls green: core omni+converter 306/306, cli atCommandProcessor 72/72. All mutated files restored (empty git status afterwards).
Findings (non-blocking)
- Nit (carried) — test-count drift. PR body says
src/omni/is "77 tests"; measured atfa35283: 89 tests in 9 files, all passing. - Note (carried, environmental) —
logger.test.ts31F/21P in every arm here. Head and base A/A parity re-confirmed with identical failing-test names; unrelated to the PR. - Note (reviewed, not a defect) — documented residual DNS risk.
findNonPublicAddressreturns null on resolver failure (fail-open; the connect then reports the real error) and a check-then-connect TOCTOU window remains; both are documented in the code with rationale, and the helper is deliberately NOT wired intofetchWithPolicy(WebFetch supports intranet FQDNs). The per-hop re-check shrinks the practical rebinding window as claimed.
Not covered
- Real-model E2E (no DashScope credential), TUI download cards / Esc-during-download UX, ACP
Session.runToolend-to-end over the wire (suite-level only: 460/460), Qwen OAuth beyond the gate predicate, download consent UX (deferred by design). - Per-commit attribution: checkout is depth-2 (shallow-boundary trap:
rev-list HEAD^1..HEAD^2= 1 vs 9 commits in metadata); the aggregate diff was verified; the delta commit was additionally isolated because the previous verified head is reachable locally. - Repo-wide test suite/typecheck; targeted gates only: core omni (89/89) + converter (217/217) + config (474/474), cli atCommandProcessor (72/72) + settingsSchema (36/36) + Session (460/460), typecheck core+cli exit 0,
check:serve-fast-path-bundlepass. - Download header/idle timeouts (30s/60s) not exercised in real time.
- The verify container ships no ffmpeg/ffprobe; static builds (ffmpeg/ffprobe 7.0.2) were installed to a scratch PATH dir — the omni pipeline's hard prerequisite.
Methodology
Environment: CI merge-ref checkout (HEAD merge, HEAD^1 base tip, HEAD^2 verified head; previous verified head reachable as a commit object), npm ci + npm run build pre-run at head. Base control rebuilt from HEAD^1 and delta control from 88d7e34 in scratch worktrees (tsc --build of packages/core, root node_modules symlinked — lockfile untouched; the eight nested packages/core/node_modules deps symlinked too after the first build attempt proved they are package-local installs); provenance asserted by realpath and by grepping loaded dists for internal @qwen-code imports (none). Harnesses import compiled dist by absolute path and drive the real pipeline against a loopback fake DashScope origin recording both sides of the wire (getPolicy + OSS PostObject multipart parsed for field order and file-part sha256); the DNS seam is an LD_PRELOAD getaddrinfo shim, not a code mock. Gate activated via the DASHSCOPE_PROXY_BASE_URL proxy-match seam + static-key stub Config. Bundle-gate and typecheck liveness proven by planted violations (full-barrel import → gate fails with iconv-lite … 553070 bytes … static path: acpAgent → chunk; planted type error → tsc fails). Raw logs, harness .mjs/.c sources, cell JSONs, and evidence PNGs live in this artifact directory; witness evidence/04-wire-oracle-harnesses.png shows the policy/funnel/converter harnesses' live re-run.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 155 passed · 0 failed · 155 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：155 通过 · 0 失败 · 155 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed — merge-ready (agent verdict) - follow-up round at head Ran the PR in an isolated, token-free container: A/B against the base build and the previous verified head, mock-free harness assertions (including an LD_PRELOAD Scripted assertions: 155 passed · 0 failed · 155 total 中文 — 判定：✅ 通过 · 可合入（agent 判定，跟进轮）本轮在隔离、无凭证容器中重新执行全部测量。新头
Verification reportPR 8512 verification (follow-up round) —
|
| # | finding (prev round) | severity | status at fa35283 (re-measured) |
|---|---|---|---|
| 1 | no in-repo regression test for the URL media path incl. the URL-only-prompt fix | Suggestion | fixed — re-verified: mutant M7 (revert urlMediaRefs.length === 0 early-return clause) kills exactly the two new URL tests in atCommandProcessor.test.ts (2F/62P) with behavioral mismatches. Agree. |
| 2 | test-count drift: PR body says 77 omni tests | Nit | stands (nit) — re-measured at this head: 89 tests in 9 files under src/omni/, all passing (7+4+10+13+15+9+5+14+12). PR body still says 77. |
| 3 | SSRF gate URL-string based, no DNS resolution | Note | fixed — re-measured: 5/5 hole cells flip between prev-head and head; per-hop re-check physically observed via the shim's lookup log (public then private on the rebind cell). Residual check-then-connect race and the deliberate non-wiring into fetchWithPolicy are documented in the code; agree with the documented rationale. |
| 4 | logger.test.ts fails identically in every arm (environmental) |
Note | stands (environmental) — re-measured: 31F/21P at head; base-tree A/A cell identical 31F/21P with a byte-identical failing-test-name set (25 unique names; diffs only in timing suffixes). Not the PR. |
Central claim + A/B (re-run)
Central claim: with the omni gate active, image/audio/video inputs are delivered as oss:// fileData via the DashScope upload channel with original bytes as-is; base (S1) had this path only for video. Re-driven against the real compiled fileUtils.processSingleFileContent (head dist vs base worktree rebuilt from HEAD^1; provenance asserted by realpath, by zero internal @qwen-code imports in the loaded dists, and by differential markers — base dist lacks sniffFileModality, prev-head dist lacks findNonPublicAddress). Witness: evidence/01-ab-delivery-base-vs-head.png (live re-run this round).
| cell (gate on) | head (S2) | base (S1 control) |
|---|---|---|
photo.png (image) |
text+oss — 1 getPolicy + 1 upload, uploaded sha256 == source; 8x8 dimensions + zoom-hint text part; estimate 1 (raw-resource-v1) |
text+inline legacy overview, 0 uploads |
tone2s.wav (audio) |
oss — 1 upload byte-exact; estimate 14 == ceil(2.0s×7) |
inline, 0 uploads |
clip2s.mp4 (video) |
oss — 1 upload byte-exact; estimate 40 == ceil(64·64·ceil(2·10)/2048) |
oss — 1 upload byte-exact (S1 preserved) |
| gate-off image / video | inline, 0 uploads — llmContent deep-equal head vs base (3/3) | inline |
gate-on image.bmp |
inline, 0 uploads (pre-sniff fallback) — deep-equal | inline |
Flip: 2/3 modalities (image, audio) inline→oss; video and all gate-off/fallback paths unchanged. Every getPolicy carries Bearer sk-fake-static-key + model; OSS multipart field order is policy-fields-before-file. Counts: head 25/25, base 9/9 (control reds encoded as assertions expecting red), cross-build identity 3/3.
Token guard (head): maxEstimatedTokens=1 rejects with estimate/method/threshold/setting in the message and 0 getPolicy / 0 uploads / 0 objects (fail closed before copy/upload); 100000 delivers; byte dimension rejects pre-upload on head and base.
Delta claims (re-run; delta control = prev-head 88d7e34)
DNS-before-fetch SSRF gate. LD_PRELOAD getaddrinfo shim (harness/dns_shim.c, compiled with gcc) answers controlled A records for *.verify-ssrf.test and delegates everything else — the production resolver path runs unmodified; the fetchFn seam is a recording spy so no real connect happens. Witness: evidence/02-dns-ssrf-ab-head-vs-prevhead.png.
| cell | head | prev-head (control) |
|---|---|---|
| public-looking name → 127.0.0.1 | refused pre-fetch, 0 fetches, "resolves to 127.0.0.1", no .part |
accepted, 1 fetch (hole) |
| name → 169.254.169.254 (metadata) | refused | accepted (hole) |
| name → 10.1.2.3 (RFC1918) | refused | accepted (hole) |
| mixed A records (public+private) | refused (ANY-private) | accepted (hole) |
| same-host redirect re-resolving to 127.0.0.1 | refused at hop 2 after exactly 1 fetch | follows rebind, 2 fetches (hole) |
| name → all-public | downloads, sha256-match (symmetric control) | downloads (control) |
| public IP literal | accepted with zero resolver lookups (short-circuit) | n/a |
18 text-gate hosts (loopback, CGNAT, [::1], ULA, .internal, single-label, …) |
refused by pre-existing text gate, 0 fetches, no .part |
same (unchanged surface) |
| DNS failure | gate passes (fail-open by design), fetch proceeds | same |
5/5 hole cells flip. The shim's lookup log is the physical witness: head resolves each refused name once (twice for the rebind: 93.184.216.34 then 127.0.0.1); prev-head's log is empty — it never resolves. Counts: 23/23 assertions.
UTF-16 BOM no longer mis-sniffed as MPEG. [FF FE …] → null at head vs audio/mpeg at prev-head (byte-level and file-level sniffFileModality); five genuine sync headers (FF FB/E0/E2/FF/FA) + ID3 still detect on both arms; png/wav/mp4 fixtures still classify. 12/12 per arm.
Secondary claims (re-run)
- Download policy 22/22: hash-while-write, 404 isolation +
.partcleanup, 18 SSRF text-gate refusals with zero fetches, www-strip same-host redirect followed, cross-origin / loopback / credentialed redirects refused, loop capped at exactly 6 fetches, Content-Length ceiling pre-body, lying-CL mid-stream ceiling with.partcleanup, user abort rejects and removes.part. (The SSRF policy structurally excludes loopback origins from the production path — that is the feature — so the socket layer is the injectedfetchFnseam with realResponseobjects; everything above it runs in production code. SSRF cells run fully in production.) - Tool-result funnel 13/13 + 3/3: byte-exact conversion; 9 parts → exactly 8 uploads; 64MiB converts then 65MiB (> remaining of 128MiB) stays inline; declared-audio with video BYTES stays inline under video-disabled while the video-declared control converts (sniffed gate); identity preserved for non-media and gate-off; nested
functionResponse.partsconverted; injected OSS 500 isolates one part (sibling converts). - Converter head 10/10 vs base 10/10 (control reds encoded as assertions):
oss://audio fileData →input_audiowith bare URL for mpeg/wav/flac/ogg/m4a (base: no input_audio — the textify-after-upload bug);audio/amrplaceholder on both; audio-disabled placeholder; image/video parity; inline audio keepsdata:URI on both.
Mutation matrix (10 mutants, 0 survivors)
| mutant | suite | outcome |
|---|---|---|
| M1 guard token-throw disabled | guard.test.ts | killed (1F/6P) — "expected function to throw an error, but it didn't" |
| M2 download SSRF text gate off | download.test.ts | killed (1F/13P) — intended SSRF assertion fails |
| M3 funnel sniffed→declared gate | tool-result-media.test.ts | killed (1F/4P) |
| M3b funnel budget 8→9 | tool-result-media.test.ts | killed (1F/4P) — "expected spy to be called 8 times, but got 9" |
| M4 converter audio-fileData branch off | converter.test.ts | killed (2F/215P, the two oss-audio tests) |
| M5 download per-hop DNS gate off | download.test.ts | killed (4F/10P, the four DNS tests) |
| M6 BOM exclusion removed | recognition.test.ts | killed (1F/14P) — "expected { audio/mpeg } to be null" |
| M7 URL-only early-return reverted | atCommandProcessor.test.ts | killed (2F/62P, the two new URL tests) |
| M8 image zoom-hint part removed | index.test.ts | killed (1F/11P) |
| M9 fail-closed catch loses error fields | index.test.ts | killed (1F/11P) |
Witness: evidence/03-mutation-matrix-summary-and-m5-live.png (summary + live M5 re-run). Every kill is a behavioral assertion mismatch at the intended test — no import/compile breakage. Unmutated controls green (core omni+converter 306/306; cli atCommandProcessor 64/64). All mutated files restored (git status empty afterwards).
Findings (non-blocking)
- Nit (carried) — test-count drift. PR body says
src/omni/is "77 tests"; measured atfa35283: 89 tests in 9 files, all passing. - Note (carried, environmental) —
logger.test.ts31F/21P in every arm here. Head and base A/A parity re-confirmed with byte-identical failing-test names; unrelated to the PR. - Note (reviewed, not a defect) — documented residual DNS risk.
findNonPublicAddressreturns null on resolver failure (fail-open; the connect then reports the real error) and a check-then-connect TOCTOU window remains; both are documented in the code with rationale, and the helper is deliberately NOT wired intofetchWithPolicy(WebFetch supports intranet FQDNs). The per-hop re-check shrinks the practical rebinding window as claimed.
Not covered
- Real-model E2E (no DashScope credential), TUI download cards / Esc-during-download UX, ACP
Session.runToolend-to-end over the wire (suite-level only: 460/460), Qwen OAuth beyond the gate predicate, download consent UX (deferred by design). - Per-commit attribution: checkout is depth-2 (shallow-boundary trap:
rev-list HEAD^1..HEAD^2= 1 vs 9 commits in metadata); the aggregate diff was verified; the delta commit was additionally isolated because the previous verified head is reachable locally. - Repo-wide test suite/typecheck; targeted gates only: core omni (89/89) + converter (217/217) + config (config.test.ts 474/474, storage 64/64, safe-mode 24/24, session-env 9/9, workflows 4/4, workflow-registration 5/5), cli atCommandProcessor (64/64) + settingsSchema (36/36) + Session (460/460), typecheck core+cli exit 0,
check:serve-fast-path-bundlepass. - Download header/idle timeouts (30s/60s) not exercised in real time.
- The verify container ships no ffmpeg/ffprobe; static builds (7.0.2) were installed to a scratch PATH dir — the omni pipeline's hard prerequisite (same as the previous round).
Methodology
Environment: CI merge-ref checkout (HEAD merge, HEAD^1 base tip, HEAD^2 verified head; previous verified head reachable as a commit object), npm ci + npm run build pre-run at head. Base control rebuilt from HEAD^1 and delta control from 88d7e34 in scratch worktrees (tsc --build of packages/core, root node_modules symlinked — lockfile untouched; the eight nested packages/core/node_modules deps symlinked too). Provenance asserted three ways: realpath of the internal workspace links (they point at the head tree, so the harnesses never import through them — they load each control's own dist by absolute file URL), a grep proving the loaded dists contain zero internal @qwen-code imports, and differential markers (base dist lacks sniffFileModality; prev-head dist lacks findNonPublicAddress). Harnesses drive the real compiled pipeline against a loopback fake DashScope origin recording both sides of the wire (getPolicy auth/model + OSS PostObject multipart parsed for field order and file-part sha256); the DNS seam is an LD_PRELOAD getaddrinfo shim, not a code mock; the gate is activated via the DASHSCOPE_PROXY_BASE_URL proxy-match seam + static-key stub Config (env captured at module load, so multi-origin cells run in separate processes). Bundle-gate and typecheck liveness proven by planted violations (full-barrel import → gate fails with iconv-lite encoding tables … static path: acpAgent; planted type error → tsc TS2322, exit 2), then restored and re-run green. Raw logs, harness .mjs/.c sources, cell JSONs, and evidence PNGs live in this artifact directory; witnesses 01-ab-delivery-base-vs-head.png, 02-dns-ssrf-ab-head-vs-prevhead.png, 03-mutation-matrix-summary-and-m5-live.png, 04-wire-oracle-harnesses.png are live re-runs captured this round via terminal-capture.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
[Critical] The DNS preflight is still not bound to the connection. downloadMediaUrl resolves currentUrl at packages/core/src/omni/download.ts:109, then the global fetchFn(currentUrl) at line 126 independently resolves the hostname again; packages/core/src/utils/fetch.ts:177 explicitly confirms this race. An attacker controlling DNS for a public-looking media hostname can answer with a public address during the validation lookup and with loopback, link-local, or RFC1918 space during the fetch lookup. The request then passes the new gate, reads the internal response, and the omni path promotes/uploads those bytes to DashScope — the SSRF/data-exfiltration boundary remains bypassable.\n\nPlease bind the outbound connection lookup to the address set that was validated (and fail closed when that validation cannot resolve), for example with an HTTP dispatcher/agent whose lookup returns only vetted public addresses. If the client cannot pin the connection, this URL-media path must reject hostnames that cannot be safely bound rather than claiming DNS-rebinding protection.
Validating the resolved address and then letting fetch re-resolve is
check-then-connect: a low-TTL record answering public once and private
the second time would still be connected to, and the fetched bytes are
uploaded to a third party. Each hop now resolves via
resolveNetworkTarget('public') — which vets every returned address and
returns a lookup pinned to the vetted one — and installs that lookup as
the request dispatcher, so the name is never resolved a second time.
- fail closed: unresolvable/unvettable hosts and targets without a
pinned lookup are refused instead of fetched unpinned
- re-validated and re-pinned at every redirect hop; the previous hop's
agent is replaced and closed
- fetch defaults to undici's own fetch so dispatcher and fetch come
from one undici version (mixing majors rejects the agent)
- Node-only: Bun accepts dispatcher and silently ignores it (verified
on 1.3.11), so non-Node runtimes are refused with a clear message
- https and credential-free URLs enforced with named errors
- utils/fetch.ts: remove findNonPublicAddress/HostResolver (a weaker
duplicate of resolveNetworkTarget) and the doc paragraph wrongly
claiming undici cannot pin connections
The @-path terminator set treats '?', '&', ',' and ';' as sentence punctuation, which is right for filesystem paths but wrong for URL refs: a presigned OSS/S3 link carries its signature in the query string, so '@https://…/a.mkv?x-oss-signature=…' was silently cut at the '?' and the download failed with HTTP 403 naming only the host. URL refs now run to the first character RFC 3986 does not permit unencoded (covering whitespace and unspaced CJK prose), with trailing sentence punctuation trimmed afterwards so it can never cut a URL short mid-query. Filesystem path parsing is unchanged.
|
You're right, and my previous reply was wrong on two counts. Thanks for pushing back instead of accepting it. Correcting my previous replyI wrote that pinning the connect to a vetted address "needs a custom agent … which undici doesn't expose portably," and treated narrowing the race as an acceptable endpoint. Both parts were wrong:
The fixThe preflight is now bound to the connection, and it reuses existing infrastructure rather than the parallel helper I'd added:
Net effect on the diff: Tests (
|
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Not reviewed: drift commits f63a764/ba9ff593 (855 delta lines, SSRF pinning fix + @-path parser fix) verified by orchestrator delta-read only — no agent-level re-review of the delta.
Not reviewed: build-and-test — cross-repo lightweight mode: no local checkout of QwenLM/qwen-code exists, so no build or test suite was executed.
中文说明
未审查：drift commits f63a764/ba9ff593 (855 delta lines, SSRF pinning fix + @-path parser fix) verified by orchestrator delta-read only — no agent-level re-review of the delta。
未审查：build-and-test — cross-repo lightweight mode: no local checkout of QwenLM/qwen-code exists, so no build or test suite was executed。
— qwen3.8-max via Qwen Code /review (v0.21.5)
The input-box highlighter had its own @-ref charset that excluded '%', '?', '&' and '=', so a presigned URL was painted file-colored only up to the first percent-escape and looked truncated even after the parser was fixed to consume it whole. URL refs now use the same RFC 3986 charset and trailing-punctuation trim as parseAllAtCommands, keeping the painted span identical to what the parser consumes.
Criticals: - tool-result-media: staging-dir setup moved inside convertPart's try — a mkdir failure (ENOSPC, EACCES, ~/.qwen/omni as a regular file) now degrades that part to inline instead of rejecting the whole tool result and reporting a succeeded tool as failed - sanitizeErrorMessage: exact split/join replacement of the known filePath before the pattern pass (immune to CJK segments, ~-prefixed and special-character basenames), with separator-based — not ASCII-word-based — fallback classes and a Windows-path pass - the fail-closed delivery result now sanitizes BOTH llmContent and the error field (the scheduler sends 'error', not llmContent, to the model on READ_CONTENT_FAILURE) and names the file by displayName - the 100 MB image-source cap only applies when the overview decoder will actually run: it protects the decoder, and the omni path uploads original bytes under its own 1 GiB ceiling without decoding Suggestions: - malformed redirect Location surfaces as a named OmniDownloadError instead of a raw TypeError (header value kept out of the message) - header/idle watchdog timeouts are now identifiable in the surfaced error instead of a generic 'This operation was aborted' - effectiveMaxDownloadFileBytes is clamped to the upload cap even when omni.download.maxFileBytes is configured higher - estimation rejects degenerate present values (0/negative/non-finite) as 'unavailable' instead of emitting a 0-token 'ok' estimate - corrected the 0xFE sniff comment: 0xFF 0xFE is a valid MPEG-1 Layer I header (reserved encodings are version 01 / layer 00); the exclusion is a deliberate trade against UTF-16 LE false positives - SSRF tests now cover private IPv6 (loopback, link-local, ULA, IPv4-mapped) plus a public-IPv6 acceptance case Regression tests are mutation-verified: reverting the mkdir placement or the error-field sanitization makes exactly the new tests fail.
|
Round-2 findings triaged: all 16 verified against HEAD before acting — every one was real. 10 fixed in Fixed (4 Critical + 6 Suggestions)R1-1 (Critical, tool-result-media.ts) — staging-dir setup moved inside R1-2 (Critical, sanitizeErrorMessage) — you were right on all three mechanisms, and the fix follows your suggestion: the known R1-3 (Critical, fail-closed error field) — applied, and one step further than the suggestion: R1-4 (Critical, 100 MB image cap) — the cap now applies only when R1-5 — malformed R1-7 — comment corrected per your spec reading (version bits 11 = MPEG-1, layer 11 = Layer I; reserved are version 01 / layer 00). The exclusion is kept and now documented as a deliberate trade: a CRC-protected Layer I frame is rare, UTF-16 LE text files are not. R1-11 — R1-14 — both watchdogs are now identifiable: the catch blocks check R1-15 — added a R1-16 — SSRF corpus now probes Deferred (6), with reasons
Also in this push (found dogfooding the URL path end-to-end)
Verified end-to-end after the parser fix: presigned OSS URL (986 MB) → full-signature download → re-recognition → store → DashScope upload → model inference. The full-length film was rejected server-side with Core omni + fetch + network-policy + fileUtils: 353/353. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
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 reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.21.5)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
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 reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.21.6)
- sanitizeErrorMessage: a path segment containing a space defeats the pattern pass (segment classes break at whitespace), so 'john doe' survived scrubbing. Call sites now pass every path they know about: the store wrap adds the store root (covers putFile throwing before objectPath is assigned), and the upload wrap - previously passing err.message through verbatim - sanitizes with the object path and store root. Function exported for direct shape tests. - estimation: animated images (GIF/APNG/animated WebP) no longer estimate as a single frame. The image probe branch retains ffprobe's nb_frames as frameCount, and the estimator applies the design 6.4 visual formula with the real count - a 480x480 300-frame GIF now estimates ~33,750 tokens instead of 113, so an enabled token guard actually engages. Static images and containers that do not report a frame count keep frameCount 1. - tool-result media: a transport-guard rejection is a policy verdict, not a transfer failure - the part is now withheld with a text placeholder instead of being delivered inline as base64, which silently bypassed an enabled guard at greater request cost than the upload it was rejecting. Transfer failures keep the inline degradation; the batch never fails for one part. - mid-turn URL media: resolveAtCommandQuery runs under a fixed 10s mid-turn budget that structurally killed every mid-turn @https reference (the download alone can exceed it, and inside the resolver the timeout abort is indistinguishable from a user cancel). URL-media turns are now exempt from that timer - the download path carries its own 30s header and 60s idle watchdogs - while filesystem resolution keeps the 10s cap unchanged. - input-box highlighter: match the URL scheme case-insensitively, like the parser it mirrors; an uppercase @https ref no longer half-paints.
- download: resolve-phase watchdog, malformed-URL and proxy refusal, resolver-error translation, path scrubbing, cause-chain surfacing, DownloadedMedia slimmed to partPath; mutation-proven tests for redirect cap, both watchdogs, byte-cap streaming, abort wiring - recognition: direct recognizeMediaFile tests, GIF 6-byte signature, guarded probe-handle close; sha256 moved out of RecognizedMedia - pipeline: hash after guards (stat -> byte guard -> probe -> token guard -> hash); displayName override so URL-funnel errors name the remote file, not the staging path - fileUtils: omni gating suite incl. 100 MB image-cap bypass pin - dead S1 shims deleted (video-only probe/delivery/read wrappers) - URL funnel: post-download modality gate mirroring the local-file gate
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed at experimental standard (base omni-experiment, not main). The new download.ts is unusually well-hardened for any bar: https-only, SSRF refused by hostname AND resolved address with DNS pinning (connect.lookup returns only the vetted address) re-applied at every redirect hop, refusal on Bun and behind proxies where the pin is unenforceable, same-host bounded redirects, maxBytes enforced against both Content-Length and written bytes, three-timer watchdogs, .part cleanup on failure, and error messages scrubbed of local paths/credentials. Identity is re-derived from the file on disk (hash/sniff/size), never from server Content-Type/final URL. No P0/P1. Light nits for later (non-blocking, experimental): consider a whole-body content-type sniff before promotion, and document the proxy refusal in user docs since it changes behavior for proxied users.
|
Round-4 review — passing. Re-reviewed the full diff at The round-1–3 threads hold up against the code as it stands, not just against the anchored lines: the dead probe/sniff fields ( Five things I still found. None of them should block this PR — all are contained by the S1 gate and none changes gate-off behavior — so I'd suggest they land as a follow-up issue against the S3 slice rather than churning this branch. 1. HEIF/AVIF containers sniff as Failure scenario: The same coarse brand table also delivers an 2. Verified: 3. Neither tool-result funnel pre-gates the dynamic import. 4. The mid-turn 5. Two S1-compat shims are still dead, with a justification comment that no longer holds. |
7 participants
What this PR does
Implements the S2 slice of the omni experiment: extends the S1 video-only upload delivery to the full input surface — image and audio modalities, URL media sources, tool-result media (the second normalization trigger point), raw-resource token estimation, and a token dimension on the transport guard.
fileData → input_audiobranch passing the bareoss://URL (previously audio fileData silently textified after a successful upload);getAudioFormatwidened to flac/ogg/m4a in lockstep with the recognizer.@https://…(previously silently dropped) is intercepted ahead of filesystem resolution, streamed todownloads/with SSRF/redirect policy, hashed while writing, re-recognized from local bytes, and delivered through the same pipeline. Per-URL failure isolation; user cancellation ends the turn gracefully.raw-resource-v1) attached to read results as the singletokenEstimatelocation;omni.transport.maxEstimatedTokens(default 0 = observability only — the formula is pending provider confirmation; positive values fail closed before any copy/upload).processToolResultOmniMediaconverts inline tool-result media tooss://fileData at both physical funnels (CoreToolScheduler terminal sites + ACPSession.runTool), as a sibling of the vision bridge; per-result upload budget (8 parts / 128 MiB) bounds malicious tool output; the sniffed modality — not the declared MIME type — gates conversion.Why it's needed
Second vertical slice of the omni roadmap (#8197), building on S1 (#8422). Real-model testing during S1 showed the token dimension binds far earlier than the byte dimension (a 36MB/8.4min video exceeds qwen3.5-omni-plus's 196608 input ceiling while 424MB/60s passes), so S2 lands the guard structure with the formula as a swappable slot.
Reviewer Test Plan
How to verify:
npm run build && npm run bundle; envQWEN_CODE_ENABLE_OMNI=1, DashScope endpoint + static key, modelqwen3.5-omni-plus.node dist/cli.js "@photo.png @clip.mp3 @video.mp4 分别描述" --approval-mode yolo --output-format json --openai-logging --openai-logging-dir ./logs→ all three parts in the request log areoss://(image_url / input_audio / video_url); objects under.qwen/omni/objects/hash-match the sources byte-exactly.node dist/cli.js "@https://upload.wikimedia.org/wikipedia/commons/3/3a/Cat03.jpg 描述"→ download card, oss:// delivery, no.partleftovers. A 404 URL shows an error card and the turn continues.@http://localhost/...is refused (SSRF).{"omni":{"transport":{"maxEstimatedTokens":196608}}}+ a long video → rejected before upload with estimate/method/threshold/setting in the error; without the setting the estimate is attached but nothing is blocked..bmpunder omni → delivered inline exactly as baseline; omni disabled → all modalities inline as baseline.cd packages/core && npx vitest run src/omni/(77 tests).Evidence:
Tested on:
Risk & Scope
Linked Issues
Closes #8184. Roadmap: #8197. S1: #8422. Design docs: #8110.
中文说明
本 PR 做了什么
实现 omni 实验 S2 切片：把 S1 的视频上传投递扩展到完整输入面——图片/音频模态、URL 媒体来源、工具结果媒体（第二归一化触发点）、原始资源 token 估算、transport guard 的 token 维度。
fileData → input_audio分支（bare oss:// URL；此前音频 fileData 上传成功后会被静默文本化）；getAudioFormat扩展到 flac/ogg/m4a。@https://…（此前被静默忽略）在文件系统解析前拦截，流式下载（SSRF/重定向策略、边下边 hash）、本地重新识别、同管线投递；单 URL 失败隔离；下载中取消优雅结束。omni.transport.maxEstimatedTokens默认 0（仅观测——公式待模型方确认；设正值后在拷贝/上传前 fail closed）。processToolResultOmniMedia在 core scheduler 与 ACP 双漏斗把工具结果的 inline 媒体转为 oss:// fileData；单结果上传预算（8 个/128MiB）约束恶意工具输出；以嗅探模态（非声明 MIME）作为门。验证
见英文 Test Plan。证据：19/19 真实 API E2E 断言（对照钉死的 S1 期基线）；qwen3.5-omni-plus 真实内容校验（2 分钟解说音频与 Whisper 转写一致；对象字节不变式三模态成立）；两轮对抗审查（安全+正确性）落地，含 3 个 E2E 阶段隐藏缺陷的修复与回归测试。
风险与范围
全部行为在 S1 门控之后；门控关闭路径与基线逐字节一致（E2E 验证）。已知边界（按设计，均有跟踪）：上传缓存待 S3；token guard 默认关闭待公式确认；下载 consent UX 推迟（结构已预留）；Qwen OAuth 暂不支持上传通道。
关联
Closes #8184；Roadmap #8197；S1 #8422；设计文档 #8110。