fix(cli): complete image routing across entry points - #7206
Conversation
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. |
tmux UI verificationTested the built CLI (
The local provider was used to inspect the exact outbound request shape without relying on external credentials. |
CI follow-upThe first repository-wide run exposed an incomplete
A fresh Qwen Code CI run was started from the updated head and remains running; it was not cancelled. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @yiliang114!
The PR body doesn't follow the pull request template. Several required sections are missing:
- What this PR does and Why it's needed — the body uses "Summary" and "Validation" instead
- Reviewer Test Plan (How to verify, Evidence before/after, Tested on OS table)
- Risk & Scope
- Linked Issues (with proper
References #NorFixes #Nkeyword — currently only mentioned as a note) - 中文说明
<details>section
Could you update the PR body to match the template? It helps reviewers understand the motivation, verify the change, and assess risk.
中文说明
感谢 PR，@yiliang114！
PR 描述没有按照 PR 模板 填写，缺少以下必填部分：
- What this PR does 和 Why it's needed — 目前用的是 "Summary" 和 "Validation"
- Reviewer Test Plan（验证方法、Before/After 证据、测试平台表格）
- Risk & Scope
- Linked Issues（用
References #N或Fixes #N关联 issue） - 中文说明
<details>部分
请按模板更新 PR 描述，方便 reviewer 理解动机、验证变更和评估风险。
— Qwen Code · qwen3.7-max
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
|
本轮按 review feedback 做了最小修复：ACP |
|
@qwen-code /resolve |
|
Qwen Code did not run conflict resolution for this request. PR #7206 is draft. |
resolve local image @ references consistently in TUI, ACP, and non-interactive entry points validate aliases and canonical targets against workspace, ignore, MIME, and file-identity boundaries route image turns through the full-turn agent or vision bridge while failing closed on unsupported/fallback paths integrate with tool-result vision bridge from #7484: @ image routing and tool-result image routing coexist with \0 suffix priority convention Refs #6988
4df535b to
070fc38
Compare
|
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）为单个提交。 |
|
Thanks for the PR — re-running on the current head Template looks good ✓ Problem: real and evidenced. The design doc ( Direction: aligned. Completing routing the design doc already planned and tightening file-read validation is squarely in scope. (Minor: the title says Size: touches core ( Approach: the scope holds together — the new non-interactive and ACP paths need the same validation the TUI already has, and the Risk: Code reads clean and the test coverage is substantial. Moving on to code review. 🔍 中文说明感谢贡献 —— 本次在当前 head 模板完整 ✓ 问题：真实且有据。设计文档明确在 Phase 1 推迟了非交互式路由，#6988 跟踪 full-turn 多模态交接。本 PR 补全该入口，并在三条路径上回溯加固 symlink/TOCTOU/MIME 校验。不是理论加固—— 方向：对齐。补全设计文档已规划的路由、收紧文件读取校验，完全在职责范围内。（小问题：标题是 规模：触及 core（ 方案：范围自洽——新的非交互与 ACP 路径需要与 TUI 相同的校验， 风险： 代码读起来干净、测试充分。进入代码审查。🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewIndependent proposal first (before reading the diff): given "non-interactive CLI and ACP don't route image turns like the TUI, and validation is missing," I would extract the TUI's existing vision-routing and file-validation into shared core helpers, call them from all three entry points, and close TOCTOU by canonicalizing with The PR matches that and goes a bit further. The non-interactive path reuses the same core helpers the TUI uses ( I walked the security path line by line and found it sound, with fail-closed behaviour at every branch:
No critical blockers and no convention violations. Two non-blocking nits:
sequenceDiagram
participant P1 as User prompt
participant P2 as Resolver (atCommandProcessor or Session)
participant P3 as readManyFiles
participant P4 as File handle (O_NOFOLLOW)
P1->>P2: at reference to image
P2->>P2: realpath then workspace and ignore check on canonical target
P2->>P2: revalidate and capture dev and ino identity
P2->>P3: canonical path plus identity plus display alias
P3->>P4: open with O_NOFOLLOW and verify dev and ino
P4-->>P3: positional read or snapshot copy then growth probe
P3-->>P2: accept or drop fail-closed on any mismatch
Test evidence (the PR's own CI at
|
| Check | Conclusion |
|---|---|
| Test (ubuntu-latest, Node 22.x) | ✅ success |
| web-shell E2E Smoke (ubuntu-latest, Node 22.x) | ✅ success |
| Post Coverage Comment (ubuntu-latest, 22.x) | ✅ success |
| Qwen Code CI (workflow) | ✅ success |
| Test (windows-latest, Node 22.x) | ⏭️ skipped (matrix) |
| Test (macos-latest, Node 22.x) | ⏭️ skipped (matrix) |
| Integration Tests (CLI, No Sandbox) | ⏭️ skipped |
Sandboxed verification would still pin the behavioural claim: @qwen-code /verify — that a non-interactive @/abs/image.png reference actually yields one outbound image payload and that a workspace-escaping symlink is dropped is not observable from the diff, and the suite would still pass with the routing removed. @qwen-code /tmux would cover the TUI/ACP surface the same way. The author has write access so both lanes are available; given the high-risk acp-integration path and the POSIX-only O_NOFOLLOW fallback, I'd like at least one run before merge — but for a maintainer-authored PR with this coverage I'm not treating it as a hard block.
中文说明
代码审查
先独立给出方案（读 diff 之前）：鉴于"非交互 CLI 与 ACP 不像 TUI 那样路由图片轮次、且缺少校验"，我会把 TUI 现有的视觉路由与文件校验抽成共享 core 辅助函数，在三个入口调用，并通过 realpath 规范化、对规范目标复查 workspace/ignore、固定 {dev, ino}、再经 O_NOFOLLOW 句柄读取来关闭 TOCTOU，使被替换的 symlink 无法进入模型上下文。
本 PR 与此一致并更进一步。非交互路径复用 TUI 使用的同一批 core 辅助函数（shouldRunVisionBridge、runVisionBridge、getFullTurnVisionModelSelector、clampInlineMediaPart、splitImageParts、hasImageParts），而非另起平行实现——正是我想要的复用。校验落在 core（readManyFiles），调用侧规范化分别在 atCommandProcessor.ts（TUI）与 Session.ts（ACP）。
我逐行走了安全路径，认为其健全，且每个分支都 fail-closed：
matchesValidatedPathIdentity在realpath(path) !== path（链路上任意 symlink）或 dev/ino 不符时拒绝。readManyFiles在提供身份映射时跳过映射中不存在的任何路径——未校验路径永远到不了读取器。- 读取以
O_NOFOLLOW打开，在已打开的 fd 上校验 dev/ino，再从该固定 fd 按位置读取（或快照拷贝），因此打开后的路径替换无法影响读到的字节。拷贝后的增长探针检测并发追加并丢弃该文件。 readTextFileFromHandle不关闭句柄，因此调用方唯一的finally { source.close() }正确（我查过双重关闭——不存在）。getRangeReadByteLimit由Infinity改为MAX_SAFE_INTEGER是必需而非顺手改动：句柄路径拒绝非有限边界。- ACP 增加
partsToSend过滤，把被拒绝的图片从出站 parts 中彻底移除，因此校验失败的文件无法作为原始fileData附件泄漏——这是该入口特有的旁路，已正确关闭。
无关键阻断，无规范违规。两个非阻断提示：
O_NOFOLLOW ?? 0在 Windows 回退为0，symlink 测试skipIf(win32)——保护实际仅限 POSIX。PR 已标注 Windows/Linux 未测；可接受，但值得后续跟进。fix(cli):低估了这项功能补全加安全加固的改动。
时序图展示校验主路径：别名经 realpath 规范化，对规范目标做 workspace 与 ignore 检查，复查并捕获 dev/ino 身份，读取时以 O_NOFOLLOW 打开并校验身份，按位置读取或快照拷贝后做增长探针，最终要么接受要么 fail-closed 丢弃。
测试证据（PR 自身在 cae7925 的 CI）
head SHA 上所有 check-run 完成且无失败——17 成功、51 跳过（矩阵与 bot 编排）。门禁项 Linux 单元套件绿；Windows/macOS/集成为矩阵跳过，属本仓库 PR 常态。我未自行运行 PR 代码——以上为通过 API 读取的 PR 自身 CI。
沙箱验证仍可固定行为性声明：@qwen-code /verify——非交互 @/abs/image.png 引用是否真产生一个出站图片 payload、逃逸工作区的 symlink 是否被丢弃，从 diff 看不出来，且即便移除路由该套件仍会通过。@qwen-code /tmux 可同样覆盖 TUI 与 ACP 面。作者有写权限，两条通道都可用；鉴于高风险 acp-integration 路径与仅 POSIX 的 O_NOFOLLOW 回退，我希望合并前至少跑一条——但对一个有如此覆盖度的维护者 PR，我不将其视为硬阻断。
— Qwen Code · qwen3.8-max-preview
Reviewed at cae7925efb4faa174cc02216dfbabe6437b4ffb1 · re-run with @qwen-code /triage
|
Confidence: 4/5 — clean review across every stage and green CI; the only reservations are non-blocking (POSIX-only symlink protection, and a behavioural claim best pinned by a sandboxed run). Stepping back: this is good work and I'd merge it. The motivation is real (the design doc explicitly deferred non-interactive routing; #6988 tracks it), the implementation matches my independent proposal and exceeds it, and the security handling — canonical-path validation on both alias and target, dev/ino identity rechecks, One correction to my previous pass: I had capped this at 3/5 and deferred, treating the ~855 core lines as a Stage 0 maintainer-awareness escalation. That gate applies to non-maintainer PRs only, and @yiliang114 has admin/write access on this repo — so the escalation (and the cap it drove) doesn't apply here. The high-risk So: approving. The two nits to carry forward — 中文说明置信度：4/5 —— 各阶段审查干净、CI 绿；唯一保留是非阻断性的（symlink 保护仅限 POSIX，以及一个最好用沙箱运行来固定的行为性声明）。 退一步看：这是好工作，我会合并。动机真实（设计文档明确推迟非交互路由，#6988 跟踪），实现与我的独立方案一致并更优，安全处理——对别名与目标都做规范路径校验、dev/ino 身份复查、 对上一次审查的一处更正：我之前把它限制在 3/5 并转交维护者，是把约 855 行 core 改动当作 Stage 0 维护者关注升级。该门禁仅适用于非维护者 PR，而 @yiliang114 在本仓库拥有 admin/write 权限——因此该升级（及其导致的上限）在此不适用。高风险 因此：批准。需带往前的两个提示—— — Qwen Code · qwen3.8-max-preview Reviewed at |
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 the local build failed (TS2352) before any test suite could run.
— qwen3.8-max-preview via Qwen Code /review
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
yiliang114
added
the
autofix/takeover
|
Resolved the current review round.
中文说明
|
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。移除 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Review round summary — no new changesAll actionable feedback from this round was already addressed by existing commits on the branch. No new code changes were made. Resolved in code (re-verified this round)
Informational (maintainer tracking notes, no action required)
Deferred (maintainer decision)
Verification
中文说明Review 轮次总结 — 无新改动本轮所有可操作的反馈已由分支上的现有提交解决。未做新的代码改动。 已在代码中解决（本轮重新验证）
信息性（维护者跟踪备注，无需操作）
推迟（维护者决定）
验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
Review —
|
base 9a4e924 |
head 6d9c074 |
|
|---|---|---|
toolDisplays |
[{Read File, Success}, {Read File, Error, "…EACCES: permission denied…"}] |
[] |
GOOD-CONTENT-VISIBLE in prompt |
yes | no |
Error during file search in prompt |
no | no |
filesRead.length |
2 | 2 |
The batch-level catch is pre-existing, but before this PR nothing ordinary reached it: processSingleFileContent catches its own I/O errors and returns a per-file error result, so one bad file never affected its siblings. The new unguarded open() is what routes a routine permission error into a whole-turn content wipe.
ACP has the same root cause with a different surface: Session.ts:8846 never inspects readResult.error, so the Error during file search: … string is pushed to the model as a text part and the sibling files' content is silently gone.
Suggested fix — three independent parts, any one of which stops the silent-loss:
- wrap
readValidatedTextFileContent(or just itsopen) in a per-filetry/catchthat degrades to the same per-file error resultreadFileContentproduces; - in
readManyFiles, keep already-collectedcontentParts/fileswhen the loop aborts instead of replacing them; - in
atCommandProcessor, surfaceresult.erroron a real channel (an errortoolDisplay+recording.status = 'error') rather thanonDebugMessage.
C2 — Critical: @-referencing a text file > 100 KB with lines longer than ~100 B now errors instead of attaching truncated content
Validated standard-FS text reads now go through readTextFileFromHandle, which requires a finite maxScanBytes, and readManyFiles passes the same value for both budgets:
// fileUtils.ts:1351
maxScanBytes: options.textFileMaxScanBytes ?? maxOutputBytes, // both = getRangeReadByteLimit(config)Default config → max(25_000, 25_000 * 4) = 100 000 bytes. readTextRangeFromHandle always streams (no buffered fast path), and readLargeUtf8Range throws TextScanBudgetExceededError when sourceSize > maxScanBytes && scannedBytes >= maxScanBytes and neither endLine nor the byte-truncation flag fired first. The old path-bound readTextFile never passed maxScanBytes at all (so it was Infinity) and took the ≤ 10 MB buffered fast path — it always returned truncated content.
Trigger: size > 100 000 and the first truncateToolOutputLines (1000) lines don't fit in 100 000 bytes, i.e. average line length > ~100 bytes. Minified bundles, single-line JSON, CSV, wide logs, base64 blobs.
A/B with production defaults (truncateToolOutputThreshold: 25000, truncateToolOutputLines: 1000), one @<file> per case:
| case | file | base 9a4e924 |
head 6d9c074 |
|---|---|---|---|
| A | 300 KB, single line | Success — Showing lines 1-1, ... [truncated] |
Error — Locating the requested line window would read more than 100000 bytes |
| B | 300 KB, 300-byte lines | Success — Showing lines 1-84 of 1025 |
Error |
| D | 150 KB minified JS, single line | Success | Error |
| E | 300 KB, 110-byte lines | Success | Error |
| F | 300 KB, 90-byte lines | Success | Success |
| G | 90 KB, single line | Success | Success |
Two things make this worth blocking on:
- The regression is encoded into the suite.
atCommandProcessor.test.ts'sshould attach a truncated text file larger than 10MBwas renamed toshould surface a byte-cap read error for a text file larger than 10MB, and its assertions flipped fromToolCallStatus.Success+... [truncated]toToolCallStatus.Error+Locating the requested line window would read more than 25000 bytes. The suite is green (70/70 locally) precisely because the only test that covered this now certifies the new behaviour. Neither the PR body nor any of the 45 threads mentions it. - It also hits ACP
resource_links, which now getvalidatedPathIdentitiesfor any file type — so the JetBrains "attach active editor" flow errors out on a large minified/JSON file that previously attached truncated.
Fix options, in order of preference:
- decouple the budgets:
maxOutputBytesis the truncation control;maxScanBytesshould be the file size (or a much larger ceiling) when the requested window starts at line 0, since locating line 0 is trivially free; - or catch
TextScanBudgetExceededErrorinreadValidatedTextFileContentand fall back to the byte-offset cursor reader / a bounded truncated result; - either way, restore the original test assertion as the regression guard.
Notes (non-blocking)
- N1 — wasted per-file I/O on the ordinary tool path.
readManyFiles.ts:190-193computesgetFileSystemService() instanceof …,await detectFileType(fullPath)andgetRangeReadByteLimit(config)unconditionally, but all three are consumed only whenvalidatedIdentityis set. On theread_many_filesglob path this is a seconddetectFileTypeper file (processSingleFileContentcalls it again), including a 4 KB content sniff for extensions the mime registry doesn't know. Guarding the block withif (validatedIdentity)removes it. - N2 — dead guard.
Number.isFinite(textHandleReadByteLimit)(readManyFiles.ts:198) can no longer be false now thatgetRangeReadByteLimitreturnsMAX_SAFE_INTEGER. Harmless today, but if theInfinityreturn ever comes back the silent fallback is the snapshot path plus the > 100 MB drop that thread3690997848just closed. Drop the check or assert the invariant. - N3 — global helper changed for one caller.
getRangeReadByteLimit'sInfinity → MAX_SAFE_INTEGERswitch also affects theread_filetool. Verified benign (normalizeMaxBytes/truncateUtf8only compare, never allocate), but the translation belongs at the handle call site rather than in a shared helper. - N4 —
readFileContentnow takes 9 positional parameters with four optional trailing ones, and the single call site has to passundefinedas the 8th. An options object would make this reviewable. - N5 — doc claim is broader than the code.
docs/design/full-turn-multimodal-routing.mdnow states "a symlink cannot disguise an ignored file or a non-image target".realpathdoesn't resolve hardlinks, so a hardlink to a git-ignored file still passes both the alias and the canonical ignore check. Either scope the sentence to symlinks or note the hardlink gap. (Raised in conversation on 07-29, never opened as a thread.) - N6 — sync I/O in the async ACP handler.
Session.tsrevalidation usesrealpathSync/statSyncwhileatCommandProcessorusesfs.promises.*. One workspace's slow mount stalls the shared ACP child's event loop for all of its sessions. - N7 —
matchesValidatedPathIdentityraces itself.Promise.all([realpath(p), stat(p)])lets the two syscalls observe different states. Sequentialrealpaththenstaton the resolved path (or reusing the open descriptor) closes the window.
What's good
The base-vs-head routing evidence is convincing, and several of the hardening changes are the right shape: the fail-closed validatedPathIdentities key-miss gate, replacing snapshot copies with handle-bound reads for text, partsToSend filtering rejected fileData parts on the early-return paths, the multi-root break → continue fix, and pruneSkippedPath keeping filesRead honest on revalidation failure.
Verdict: request changes — C1 and C2 both change behaviour users depend on today, and C2's only regression guard was rewritten to match the new behaviour.
中文摘要
对照 base 9a4e924 复核了完整 diff。三入口路由本身没问题,已解决的 45 条线程也确实修掉了不少真问题。下面两条是新发现,都在共享的 readManyFiles 读取路径上,现有线程均未覆盖,且都用生产默认配置做了 base/head A/B 实证。
C1(Critical):@ 引用中只要有一个文件读不了,整轮的全部附件内容被静默丢弃。
readValidatedTextFileContent(readManyFiles.ts:278)的 fs.promises.open 不在任何单文件错误边界内,EACCES / 删除竞态的 ENOENT / ELOOP / EMFILE 都会冒泡到 readManyFiles.ts:249 的函数级 catch,该 catch 丢弃已收集的全部 contentParts,返回 files: []。atCommandProcessor 随后:fileDisplays 为空(连错误卡片都没有)、atCommandProcessor.ts:932 只发默认关闭的 onDebugMessage、最后仍走成功返回(status: 'success'、filesRead 照报 2 个文件)。结果:模型收到零内容,界面无任何提示,会话记录声称读取成功。A/B 实证(capsh --drop=cap_dac_override,cap_dac_read_search 使 chmod 000 对 root 生效):base 给出 Success + Error 两张卡片且好文件内容正常送达;head 卡片为空、内容全无。批级 catch 是既有代码,但此前没有常规错误能到达它——processSingleFileContent 自己兜住 I/O 错误并返回单文件错误结果。ACP 侧同源:Session.ts:8846 从不检查 readResult.error。
C2(Critical):@ 引用 >100 KB 且平均行长 >~100 B 的文本文件,从"截断附加"变成"读取报错"。
校验过的标准 FS 文本读取改走 readTextFileFromHandle,它强制要求有限的 maxScanBytes,而 readManyFiles 把两个预算都设成 getRangeReadByteLimit(config)(默认 100 000 字节)。handle 路径永远走流式,readLargeUtf8Range 在 sourceSize > maxScanBytes && scannedBytes >= maxScanBytes 且未先命中 endLine/字节截断时抛 TextScanBudgetExceededError。而旧的 path 路径从不传 maxScanBytes(即 Infinity)并走 ≤10 MB 缓冲快路,总是返回截断内容。触发条件:文件 >100 KB 且前 1000 行装不进 100 000 字节。A/B:300 KB 单行、300 B 行、150 KB 压缩 JS、110 B 行全部由 Success 变 Error;90 B 行与 90 KB 单行不受影响。
两点使其应当阻塞合并:一是回归被写进了测试——should attach a truncated text file larger than 10MB 被改名为 should surface a byte-cap read error…,断言从 Success + ... [truncated] 翻成 Error,所以套件全绿(本地 70/70)恰恰是因为唯一覆盖它的测试改成了认可新行为;PR 描述与 45 条线程均未提及。二是ACP resource_link 同样受影响(现在任何类型的文件都会带上 validatedPathIdentities),JetBrains"附加当前编辑器"遇到大压缩文件会直接报错。
建议:解耦两个预算(窗口从第 0 行开始时定位成本为零,maxScanBytes 应取文件大小或大得多的上限);或在 readValidatedTextFileContent 捕获 TextScanBudgetExceededError 回退到字节游标读取;并恢复原测试断言作为回归护栏。
其余 N1–N7 为非阻塞项:ordinary read_many_files 路径上多余的 detectFileType/instanceof/getRangeReadByteLimit 计算;Number.isFinite 已成死分支;为单一调用方修改共享 helper 的 Infinity → MAX_SAFE_INTEGER;readFileContent 已达 9 个位置参数;设计文档"符号链接无法伪装被忽略文件"的说法未覆盖硬链接;Session.ts 在异步 ACP handler 里用同步 I/O;matchesValidatedPathIdentity 的 Promise.all 自竞态。
结论:request changes — C1、C2 都改变了用户当前依赖的行为,且 C2 唯一的回归护栏被改写成了认可新行为。
Review round 2 —
|
| case | base 9a4e924 |
head 6d9c074 |
|---|---|---|
@big.png, 101 MB |
Read File @big.png → Error: Image file exceeds the 100 MB source limit |
no card, no error, nothing in prompt |
@big.zip, 200 MB |
Read File @big.zip → Error: File size exceeds the 10MB limit (200.00MB) |
no card, no error, nothing in prompt |
@mid.zip, 50 MB |
Error: File size exceeds the 10MB limit |
Error: File size exceeds the 10MB limit (unchanged) |
filesRead / recording.status (all rows) |
honest | ["good.txt","big.png"] / success |
Note the 50 MB row: it is under the snapshot cap, so head copies the whole 50 MB to a temp file and then rejects it at the 10 MB binary limit. That is precisely the wasted I/O the constant claims to prevent — the cap is set to the image limit (100 MB) but applied to every non-text type, whose real limit is 10 MB.
Suggested fix: don't collapse "too large to snapshot" into the fail-closed continue. Either return a typed reason from snapshotValidatedFile and synthesise the same FileReadInfo error entry readFileContent would have produced, or check the per-type size cap before deciding to snapshot so processSingleFileContent still gets to emit its own error. More generally, every continue in that loop drops a path with no entry in files — atCommandProcessor has no way to tell "rejected" from "never asked for".
C1 and C2 — still open, re-verified at 6d9c074
Both re-run against base 9a4e924 on this head; details and suggested fixes are in the previous round — not repeating them here.
C1 — one unreadable @ file discards every attachment in the turn. readValidatedTextFileContent's unguarded fs.promises.open lets EACCES reach readManyFiles' function-level catch, which replaces all already-collected contentParts.
compare @good.txt and @noperm.txt (chmod 000, DAC caps dropped) |
base | head |
|---|---|---|
toolDisplays |
[{@good.txt, Success}, {@noperm.txt, Error, "EACCES: permission denied"}] |
[] |
| good file's content reached the model | yes | no |
shouldProceed / filesRead |
true / 2 files |
true / 2 files |
C2 — @-referencing text > 100 KB with lines longer than ~100 B now errors instead of attaching truncated content, because readValidatedTextFileContent passes maxScanBytes === maxOutputBytes === getRangeReadByteLimit(config).
summarize @wide.txt (300 KB, 300-byte lines) |
base | head |
|---|---|---|
| status | Success, ... [truncated] |
Error — Locating the requested line window would read more than 100000 bytes |
C2's only regression guard is still the atCommandProcessor.test.ts case that this PR renamed from should attach a truncated text file larger than 10MB to should surface a byte-cap read error… with the assertions flipped, so the green suite certifies the new behaviour rather than catching it.
Also checked this round (no findings)
- Non-interactive routing lifecycle.
fullTurnModelOverrideseeds the turn-scopedmodelOverride, and the notification/cron drain loop starts from a freshitemModelOverridewith noruntimeView— the drains genuinely don't inherit the selector, matching the new sentence indocs/design/full-turn-multimodal-routing.md. ThefullTurnModelOverrideActiveguard correctly refuses skill-tool overrides (including the clear-to-undefinedcase) without blocking a repeat of the same selector. resolveForModel(..., { failClosed: true })rejects into the outer turn handler, so an unresolvable vision route aborts the run instead of sending raw image bytes to the primary. Fail-closed as documented.runtimeViewplumbing throughexecuteToolCall→CoreToolScheduler.scheduleis a pass-through to existing machinery (that scheduler code is unchanged by this PR); the map entry is cleaned up on both the error and completion paths.- Multi-root resolution. The
break → continuefix plus deferring theignoredByReasonpush until after the loop behaves correctly when a path resolves from a later workspace directory.
Non-blocking, in addition to N1–N7 from the previous round
- N8 —
processSingleFileContent's catch block now reports a workspace-relative path in theerrorfield (Error reading file ${relativePathForDisplay}), where it previously reported the absolutefilePath. That field feeds logs/telemetry for every caller including theread_filetool, so absolute paths disappear from error records repo-wide for a change that only the@path needed. - N9 —
readTextFileFromHandleis optional on theFileSystemServiceinterface but invoked asreadTextFileFromHandle!.call(...). Safe today becauseshouldUseTextHandlerequiresStandardFileSystemService, butprocessSingleFileContentis exported, and any caller passingtextFileHandlewith a custom service gets aTypeErrorrather than a fallback.
Verdict: request changes — C3 is a new silent-data-loss path on a deterministic trigger (attach a large file), and C1/C2 are unchanged.
中文说明
第 2 轮评审 — 6d9c074(head 与上一轮完全一致)
head 与上轮所审提交逐字节相同,因此本轮集中在上轮覆盖较少的部分:非交互式入口、atCommandProcessor 的复校循环、以及 snapshot 路径。由此得到一条新的 Critical,并用同一套 base/head A/B 验证。上轮的 C1、C2 已在本 head 重跑,均仍可复现。
C3(Critical,新增):@ 引用 ≥100 MB 的文件会被静默丢弃,而会话仍报告该文件已读取。
snapshotValidatedFile(readManyFiles.ts:361)对超过 SNAPSHOT_MAX_SIZE_BYTES(100 MB)的文件返回 undefined,调用方(readManyFiles.ts:206)用 if (shouldSnapshot && !snapshot) continue; 处理,与"身份校验失败"的 fail-closed 情形完全无法区分。continue 不向 files 写入任何条目,于是 atCommandProcessor 连错误卡片都不会生成;而 contentLabelsForDisplay 仍保留该标签,filesRead 与 recording.status: 'success' 都声称文件已读取,提示词里 @big.png 记号也依然存在——模型被告知了一个它从未收到的文件。常量注释称这些文件"反正会被下游 processSingleFileContent 的上限拒绝,拷贝到临时文件是浪费 I/O",但那次拒绝正是用户可见的错误信息;省掉拷贝的同时也省掉了诊断。
A/B(生产默认配置,稀疏文件):@big.png(101 MB)base 给出 Error 卡片 Image file exceeds the 100 MB source limit,head 无任何卡片;@big.zip(200 MB)base 给出 File size exceeds the 10MB limit,head 同样无卡片;@mid.zip(50 MB)两侧一致仍报错。注意 50 MB 这行:它在 snapshot 上限之内,所以 head 会先把整整 50 MB 拷到临时文件,然后才在 10 MB 的二进制上限处被拒——这恰恰是该常量声称要避免的浪费 I/O:上限取的是图片的 100 MB,却应用到所有非文本类型,而后者的真实上限是 10 MB。
建议:不要把"过大无法 snapshot"并入 fail-closed 的 continue;或让 snapshotValidatedFile 返回带原因的结果并补出 readFileContent 本会产生的错误条目,或把按类型的大小上限前移到决定是否 snapshot 之前,让 processSingleFileContent 自己报错。更普遍地说,该循环里每一处 continue 丢弃路径时都不写 files,atCommandProcessor 无从区分"被拒绝"与"从未请求"。
C1、C2 仍未解决,已在 6d9c074 重新验证(细节与修复建议见上一轮评论,此处不再重复):
- C1:单个不可读的
@文件会丢弃整轮全部附件。compare @good.txt and @noperm.txt(chmod 000,已 drop DAC 能力):base 为 Success + Error 两张卡片且好文件内容正常送达;headtoolDisplays为空、内容全无,而shouldProceed: true、filesRead仍报两个文件。 - C2:
@引用 >100 KB 且行长 >~100 B 的文本文件由"截断附加"变为"报错"。summarize @wide.txt(300 KB、300 字节行):base 为 Success +... [truncated],head 为 ErrorLocating the requested line window would read more than 100000 bytes。其唯一的回归护栏仍是本 PR 把should attach a truncated text file larger than 10MB改名为should surface a byte-cap read error…并翻转断言的那条用例,所以套件全绿是在认可新行为而非拦截它。
本轮另行核查、未发现问题的部分: 非交互式路由生命周期(fullTurnModelOverride 只作用于本轮,通知/cron drain 从全新的 itemModelOverride 起步且不带 runtimeView,与设计文档新增的那句一致;fullTurnModelOverrideActive 守卫正确拒绝 skill-tool 覆写包括清空为 undefined 的情形);resolveForModel(..., { failClosed: true }) 失败会抛到外层 turn 处理器,确实是 fail-closed;runtimeView 经 executeToolCall → CoreToolScheduler.schedule 的透传属于既有机制(该调度器代码本 PR 未改动),map 条目在错误与完成两条路径上都会清理;多工作区根解析的 break → continue 修复与延后 push ignoredByReason 的处理正确。
非阻塞补充(在上轮 N1–N7 之外): N8 — processSingleFileContent 的 catch 分支现在在 error 字段里报相对路径而非绝对路径,该字段面向所有调用方(含 read_file 工具)的日志/遥测,为了 @ 路径的需要导致全仓错误记录都丢失绝对路径;N9 — readTextFileFromHandle 在 FileSystemService 接口上是可选的,却以 readTextFileFromHandle!.call(...) 调用,当前因 shouldUseTextHandle 要求 StandardFileSystemService 而安全,但 processSingleFileContent 是导出函数,自定义服务下传 textFileHandle 会得到 TypeError 而非回退。
结论:request changes — C3 是触发条件确定(附加一个大文件)的新静默数据丢失路径,C1、C2 亦未变化。
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
| // without a redundant read_file (issue #6289). Key by the canonical | ||
| // (resolved) path — not the display alias — because Edit / WriteFile | ||
| // looks up the cache by canonical path. | ||
| recordAttachedFileRead(config, canonicalPath ?? filePath, fileReadResult); |
There was a problem hiding this comment.
[Suggestion] No positive test asserts that a successful validated read is cached by its canonical path. The negative case (a dropped read leaves cache.size() === 0) and the non-validated positive case are both covered, but every positive cache test calls readManyFiles without validatedPathIdentities, where filePath === fullPath === canonicalPath — so a regression dropping the canonicalPath argument here would key the cache by the ephemeral snapshot temp path and no test would fail. — Failure scenario: a validated @-read succeeds via the snapshot path, then the user runs Edit/WriteFile on the same canonical path; checkPriorRead looks up the canonical path, misses (the entry is keyed by the now-deleted qwen-validated-read-* path), and forces a redundant re-read instead of passing prior-read enforcement. Concrete cost: the Edit-after-@-attach workflow silently breaks for validated reads only. Consider adding a test that performs a successful validated read and asserts checkPriorRead(cache, canonicalPath, 'editing').ok === true.
— qwen3.8-max-preview via Qwen Code /review
| const configuredProjectTempDir = Storage.getGlobalTempDir(); | ||
| const projectTempDir = await fs | ||
| .realpath(configuredProjectTempDir) | ||
| .catch(() => path.resolve(configuredProjectTempDir)); |
There was a problem hiding this comment.
[Suggestion] The realpath'd-temp-dir branch added here (isSubpath(projectTempDir, …), also consulted by the in-loop and revalidation checks) has no test that distinguishes it from the literal configuredProjectTempDir branch. Both existing temp-dir tests mock Storage.getGlobalTempDir to a directory that realpaths to itself, so on Linux CI acceptance is always decided by the literal branch — deleting the realpath computation and every projectTempDir check would pass the whole suite. — Failure scenario: on macOS the configured temp dir is non-canonical (/var/folders/…) while the in-loop check compares a canonicalized path (/private/var/folders/…); without this branch, isSubpath('/var/folders/X/tmp', '/private/var/folders/X/tmp/file.txt') is false and a legitimate project-temp @-reference is wrongly skipped. Consider a test that spies Storage.getGlobalTempDir to return a non-canonical (symlinked) path and asserts a file inside it is still read after canonicalization.
— qwen3.8-max-preview via Qwen Code /review
|
🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 1/100 轮）。改动内容与我反驳保留之处如下： Review feedback addressed —
|
|
🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 2/100 轮）。改动内容与我反驳保留之处如下： Review round 3 — feedback addressedCritical findings (from @wenshao's round-2 review)C1 — one unreadable C2 — C3 — Inline suggestions (automated reviewer)rc:3692400651 — no positive test for validated-read cache by canonical path — implemented. Added "records a validated text read by canonical path for prior-read enforcement": performs a validated text read via rc:3692400658 — realpath'd-temp-dir branch untested — implemented. Added "accepts a temp-dir file via the realpath branch when the configured path is a symlink": creates a real directory and a symlink to it, mocks Non-blocking notes (declined)N8 — catch block reports relative path in N9 — Verification
中文说明第 3 轮评审 — 反馈处理Critical 发现（来自 @wenshao 的第 2 轮评审）C1 — 单个不可读的 C2 — C3 — 内联建议（自动审查器）rc:3692400651 — 缺少验证读取按规范路径缓存的正向测试 — 已实现。新增 "records a validated text read by canonical path for prior-read enforcement"：通过 rc:3692400658 — realpath 临时目录分支未被测试覆盖 — 已实现。新增 "accepts a temp-dir file via the realpath branch when the configured path is a symlink"：创建真实目录和指向它的符号链接，将 非阻塞备注（已拒绝）N8 — catch 块在 N9 — 验证
Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
|
🔀 Base updated: red check(s) [Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： No action takenThe only feedback item this round is the failed check ubuntu-latest / Java 11 from the DiagnosisThis check is unrelated to the PR's changes:
No code changes are warranted. 中文说明未采取任何操作本轮唯一的反馈项是 诊断此检查与本 PR 的更改无关：
无需进行代码更改。 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round — no action takenNo code change was made this round. There is no actionable feedback requiring a change. Feedback triage
Still-red check:
|
Local verification report — built and run against a real binaryI built this PR from source and exercised it against an isolated local model provider that records every outbound request, so each claim below is backed by a captured HTTP request body rather than by log inspection. I ran the identical scenarios against Verdict: the headline fix is real and reproducible, and the hardening delivers one genuine type-confusion fix that main was vulnerable to. I recommend merging. Two statements in the PR description overstate the scope and are worth correcting before merge; one pre-existing gap is now reachable from a new entry point and deserves an explicit decision. Details below. Environment
Two models are registered on the same endpoint so bridge selection is deterministic:
1. The headline fix is confirmed
On 2. Full outcome matrix, every cell from a recorded request body
3. Correction: only the non-interactive CLI was brokenThe PR description says "the non-interactive CLI and ACP paths did not route image turns through the same capability-aware logic as the TUI." I drove a real 4. A real type-confusion fix the description undersellsThe most valuable thing I found is not in the test plan. Fixture: On With this PR the canonical target is resolved first, its MIME is not 5. Workspace escape and ignore rules: already safe on main
6. One gap to decide on before merge
The nuance that matters: because the non-interactive CLI now routes image turns, this pre-existing extension-trust behavior becomes reachable from 7. TOCTOUI did not reproduce the swap-during-read race live — the window is too tight to drive deterministically from outside the process. The mechanism reads correctly ( 8. Test suites
No failure is attributable to this PR. The failing files — SummaryThe routing fix does what it claims and I verified it end to end on the real binary at all three entry points. The canonical-path validation closes a genuine mislabeled-egress bug on the ACP path. My only requests before merge are wording corrections in §3 and §6, plus a decision on whether extension-only MIME trust warrants a follow-up now that 中文版本本地验证报告 —— 基于真实构建产物运行我从源码构建了本 PR，并针对一个会记录所有出站请求的隔离本地模型 provider 进行验证，因此下文每条结论都有捕获到的 HTTP 请求体作为依据，而非仅凭日志推断。同一批场景我也在 结论：核心修复真实可复现，安全加固中包含一处 main 确实存在的类型混淆漏洞的真修复。建议合入。 PR 描述中有两处表述夸大了范围，建议合入前修正；另有一处既有缺陷因为新入口打通而变得可达，值得显式决策。详见下文。 环境
在同一 endpoint 上注册两个模型，使 bridge 选择完全确定：
1. 核心修复已确认
在 2. 完整结果矩阵，每个单元格都来自真实请求体见上方矩阵截图。 3. 修正：只有非交互式 CLI 是坏的PR 描述称「非交互式 CLI 和 ACP 路径没有像 TUI 一样走能力感知路由」。我用 stdio 驱动了真实的 4. 一处被描述低估了的真实类型混淆修复我发现的最有价值的一点并不在测试计划里。Fixture： 在 本 PR 会先解析规范目标，其 MIME 不是 5. 工作区逃逸与 ignore 规则：main 上本就安全
6. 合入前需要拍板的一处缺口
需要注意的细节是：由于非交互式 CLI 现在会路由图片轮次，这个既有的扩展名信任行为第一次变得可以从 7. TOCTOU我没有在真实运行中复现「读取过程中被替换」的竞态 —— 该窗口太窄，无法从进程外确定性地驱动。相关机制（ 8. 测试套件
没有任何一个失败可归因于本 PR。 失败文件 —— 小结路由修复确实做到了它所声称的效果，我在真实产物上对三个入口都做了端到端验证。规范路径校验修掉了 ACP 路径上一个真实的「错误标注外发」缺陷。合入前我只希望修正第 3 节和第 6 节的措辞，并就「 |
|
@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: 18 passed · 0 failed · 18 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：18 通过 · 0 失败 · 18 总计 Verification report (report.md)Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
| expect(processedText).toContain( | ||
| 'Showing lines 1-1 of at least 1 total lines', | ||
| ); | ||
| expect(processedText).toContain('Showing lines 1-'); |
There was a problem hiding this comment.
[Suggestion] This truncation-header assertion was weakened from an exact format match ('Showing lines 1-1 of at least 1 total lines') to a bare prefix ('Showing lines 1-'), but the strict assertion still passes against the new code — the format string at readManyFiles.ts:537 is unchanged, so the weakening was unnecessary. — Failure scenario: a future regression that corrupts the line-range/total metadata (e.g. Showing lines 1-0 of 0 total lines) still satisfies toContain('Showing lines 1-') and ships undetected, while the sibling test at readManyFiles.test.ts:470 still pins the full format.
| expect(processedText).toContain('Showing lines 1-'); | |
| expect(processedText).toContain('Showing lines 1-1 of at least 1 total lines'); |
— qwen3.8-max-preview via Qwen Code /review
| const canonicalPath = resolveExistingFile(resolved); | ||
| if (!canonicalPath) continue; |
There was a problem hiding this comment.
[Suggestion] The isExistingFile → resolveExistingFile swap dropped the isFile() gate from the ACP text @ image path. resolveExistingFile accepts directories (isFile() || isDirectory?.()), and the MIME gate getSpecificMimeType is extension-only (mime.getType, no stat), so a directory named image.png now passes every gate and is routed to readDirectory, sending a directory listing to the model. — Failure scenario: pre-diff, isExistingFile (statSync(resolved).isFile()) returned false for a directory and skipped it; post-diff, mkdir image.png in the workspace plus a text @image.png mention sends a directory listing to the model (the revalidation loop has no isFile() gate either). Low practical risk — needs a directory with an image extension inside the workspace — hence Suggestion, not Critical.
| const canonicalPath = resolveExistingFile(resolved); | |
| if (!canonicalPath) continue; | |
| const canonicalPath = resolveExistingFile(resolved); | |
| if (!canonicalPath) continue; | |
| if (!statSync(canonicalPath).isFile()) continue; |
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Released in v0.21.3. |
What this PR does
Completes local image
@reference routing across all three entry points (TUI, ACP, and non-interactive CLI). Aliases and canonical targets are validated against workspace boundaries, ignore rules, MIME types, and file-identity checks before routing. Image turns then go through the full-turn agent or vision bridge, failing closed on unsupported or fallback paths.Why it's needed
Issue #6988 identified that the non-interactive CLI and ACP paths did not route image turns through the same capability-aware logic as the TUI. This left
@-referenced images silently dropped or mishandled outside the TUI, and skipped security validation (TOCTOU, symlink escape, MIME spoofing) on all paths.Reviewer Test Plan
How to verify
@/absolute/path/to/image.pngreference — confirm it produces a Read card and one outbound image payload (same as TUI behavior).@references to symlinked files escaping the workspace, non-image MIME types, and gitignored paths — confirm all are rejected.npm run test --workspace=packages/coreandnpm run test --workspace=packages/cli— all pass.Evidence (Before & After)
tmux UI verification: absolute
@image resolved to a Read card and one outbound image payload (full report in PR conversation). Deterministic visual routing verified with an isolated local provider and temporaryQWEN_HOME.Tested on
Environment (optional)
Unit + integration tests, tmux real-user session, isolated local provider with temporary
QWEN_HOME.Risk & Scope
Linked Issues
References #6988
中文说明
本 PR 做了什么
在 TUI、ACP 和非交互式 CLI 三个入口统一完成本地图片
@引用的路由。对别名和规范化目标执行工作区边界、ignore 规则、MIME 类型和文件身份校验后再路由。图片轮次通过 full-turn agent 或 vision bridge 处理，不支持或 fallback 路径一律 fail closed。为什么需要
Issue #6988 指出非交互式 CLI 和 ACP 路径没有像 TUI 一样走能力感知路由，导致
@引用的图片被静默丢弃或错误处理，且所有路径均缺少安全校验（TOCTOU、符号链接逃逸、MIME 伪造）。审阅测试计划
如何验证
@/absolute/path/to/image.png引用——确认产生 Read 卡片和一个出站图片 payload（与 TUI 行为一致）。@引用逃逸工作区的符号链接文件、非图片 MIME 类型、gitignore 路径——确认全部被拒绝。证据（前后对比）
tmux UI 验证：绝对路径
@图片解析为 Read 卡片和一个出站图片 payload（完整报告见 PR 对话）。使用隔离本地 provider 和临时QWEN_HOME验证确定性视觉路由。测试环境
环境（可选）
单元 + 集成测试、tmux 真实用户会话、隔离本地 provider + 临时
QWEN_HOME。风险与范围
关联 Issue
引用 #6988