Skip to content

fix(cli): reject symlinked screenshot paths on win32 in capture_screen_context - #9847

Merged
wenshao merged 5 commits into
QwenLM:mainfrom
zhou2024NAU:fix/windows-lane-posix-test-guards
Aug 29, 2026
Merged

fix(cli): reject symlinked screenshot paths on win32 in capture_screen_context#9847
wenshao merged 5 commits into
QwenLM:mainfrom
zhou2024NAU:fix/windows-lane-posix-test-guards

Conversation

@zhou2024NAU

@zhou2024NAU zhou2024NAU commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

What this PR does

The capture_screen_context tool now rejects a screenshot path that is a symbolic link (including NTFS junctions) before opening it. The private-directory containment check stays unchanged; only the read itself is hardened. POSIX behavior is identical to before, with O_NOFOLLOW kept as a TOCTOU backstop.

Why it's needed

readPrivatePng relied on O_NOFOLLOW to refuse symlinked screenshot paths, but Windows silently ignores that flag. On win32 the guard was therefore a no-op: a Host returning a symbolic link could have its target read as a screenshot, bypassing the containment check this tool promises. This is part of #9481 (cluster 3, the CaptureScreenContextTool symlink-rejection failure).

Reviewer Test Plan

How to verify

Run npx vitest run src/acp-integration/live/capture-screen-context.test.ts inside packages/cli. On Windows, before this change, the test rejects a symlink and deletes only the Host-provided link failed with expected undefined to be truthy at line 91 — meaning the tool read through the link and returned success instead of rejecting it. After this change all 4 tests pass: the tool fails with an explicit error, only the link file is removed in cleanup, and the original PNG target stays intact.

Evidence (Before & After)

Before (Windows 11, Node v22.14.0): Tests 1 failed | 3 passed (4) with AssertionError: expected undefined to be truthy at capture-screen-context.test.ts:91. After the same command reports ✓ src/acp-integration/live/capture-screen-context.test.ts (4 tests) and Tests 4 passed (4). Non-UI change, so no screenshots are attached.

Tested on

OS Status
🍏 macOS ⚠️ not tested
🪟 Windows ✅ tested
🐧 Linux ⚠️ not tested

Environment (optional)

Local npx vitest run on Windows 11 with Node v22.14.0; Windows Developer Mode enabled so test fixtures can create real symlinks.

Note on the failing check (Test (ubuntu-latest, Node 22.x))

The single red check on this commit is an infrastructure-side failure, not a code regression:

  • Vitest summary in the failing step: Test Files 65 passed (65) / Tests 1713 passed | 11 skipped (1724) — no assertion failed anywhere in the run.
  • The suite touched by this PR passes: ✓ src/acp-integration/live/capture-screen-context.test.ts (5 tests) 60ms.
  • The only error vitest reports is internal: Error: [vitest-worker]: Timeout calling "onTaskUpdate", with the entire call stack inside node_modules/vitest/dist/chunks/rpc.*.js (worker→parent RPC timeout), matching the known flaky pattern previously worked on in fix(tests): avoid blocking the Vitest worker during directory E2E #8685.
  • Every other check on this commit is green (failing job log).

A re-run of this job should clear it; no code change is needed or planned for it in this PR.

Risk & Scope

  • Main risk or tradeoff: one extra lstat per capture — negligible next to the file read it guards.
  • Not validated / out of scope: other O_NOFOLLOW call sites elsewhere in the repository were not audited in this PR.
  • Breaking changes / migration notes: none.

Linked Issues

Part of #9481 (referenced without a closing keyword on purpose — the remaining clusters of that issue are being fixed separately).

中文说明

这个 PR 做了什么

capture_screen_context 工具在打开截图路径之前,现在会先拒绝符号链接路径(包括 NTFS junction)。私有目录约束检查保持不变,仅加固读取这一步;POSIX 行为与之前完全一致,并保留 O_NOFOLLOW 作为 TOCTOU 兜底。

为什么需要这个改动

readPrivatePng 原本依赖 O_NOFOLLOW 来拒绝符号链接截图路径,但 Windows 会静默忽略该标志。因此在 win32 上这道防线形同虚设:当 Host 返回一个符号链接时,其目标文件会被当作截图读进来,绕过了该工具承诺的目录约束。本 PR 属于 #9481 的簇 3(CaptureScreenContextTool 的符号链接拒绝失败项)。

审阅者测试计划

如何验证

packages/cli 内运行 npx vitest run src/acp-integration/live/capture-screen-context.test.ts。在 Windows 上、修改之前,用例 rejects a symlink and deletes only the Host-provided link 在第 91 行报 expected undefined to be truthy——即工具读穿了链接并返回成功,而不是拒绝。修改后 4 条测试全部通过:工具以明确的错误拒绝请求,清理阶段只删除链接文件本身,原始 PNG 目标保持完好。

证据(前后对比)

修改前(Windows 11,Node v22.14.0):Tests 1 failed | 3 passed (4),错误为 AssertionError: expected undefined to be truthy,位于 capture-screen-context.test.ts:91。修改后同一命令报告 ✓ src/acp-integration/live/capture-screen-context.test.ts (4 tests)Tests 4 passed (4)。本改动非 UI 变更,故未附截图。

测试平台

macOS 未测;Windows 已测;Linux 未测(CI 会覆盖)。

环境(可选)

Windows 11 本地 npx vitest run,Node v22.14.0;已开启 Windows 开发者模式以便测试夹具创建真实符号链接。

风险与范围

  • 主要风险或取舍:每次捕获多一次 lstat,相对于它保护的文件读取可忽略不计。
  • 未验证 / 超出范围:仓库中其他 O_NOFOLLOW 调用点不在本 PR 审查范围内。
  • 破坏性变更 / 迁移说明:无。

关联 issue

Part of #9481(有意不使用关闭关键词——该 issue 其余簇由其他改动分别修复)。

…n_context

Windows silently ignores O_NOFOLLOW, so the symlink guard in readPrivatePng was a no-op on win32: a Host returning a symbolic link could have its target read as a screenshot, bypassing the private-directory containment check. Probe the path with lstat and reject symlinks (including NTFS junctions) before opening; POSIX keeps O_NOFOLLOW as a TOCTOU backstop.

Before: 'rejects a symlink and deletes only the Host-provided link' failed on Windows (tool read through the link). After: rejected with an explicit error; only the link is removed and the target stays intact. Part of QwenLM#9481 (cluster 3).
@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

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

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

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR! Re-run at the current head — gate findings refreshed below.

  • Template: complete ✓
  • Problem: observed, not theoretical. Maintainer-filed ci: the Windows lane is red — 59 tests across 10 files, unobserved since the merge queue stopped running #9481 documents the red Windows lane and names this exact signature among the standing failures — CaptureScreenContextTool's symlink rejection. The root cause is documented and visible in-tree: win32 ignores O_NOFOLLOW, and the older symlink test carries a win32 skipIf whose comment says precisely that. This PR fixes the guard instead of the skip, and a new cross-platform test pins the rejection.
  • Direction: aligned. The tool explicitly promises private-directory containment against an untrusted Host; this closes a guard that is a no-op on exactly the platform where it fails. Referenced as part of ci: the Windows lane is red — 59 tests across 10 files, unobserved since the merge queue stopped running #9481 cluster 3 without a closing keyword, so no duplicate-fix linkage to act on.
  • Size: not a core-module path (packages/cli/src/acp-integration/) — 7 production lines (6+1) and 23 test lines. No Stage 0 tier triggered.
  • Approach: minimal — an lstat probe before open, keeping O_NOFOLLOW as the POSIX TOCTOU backstop. I don't see a smaller path (realpath comparison is more code with ENOENT edges; Node exposes no Windows equivalent of O_NOFOLLOW), and the diff carries no unrelated changes.
  • Risk: acp-integration matches this repo's high-risk path list from its revert-history analysis. Not a blocker, but it raises the review depth: Stage 2 runs at full depth and requires the PR's own CI evidence before approval.

Moving on to code review. 🔍

中文说明

感谢贡献!以下是在当前 head 上刷新的门禁结论。

  • 模板:完整 ✓
  • 问题:已观测到,不是理论问题。维护者提交的 ci: the Windows lane is red — 59 tests across 10 files, unobserved since the merge queue stopped running #9481 记录了变红的 Windows lane,并点名了完全相同的失败签名——CaptureScreenContextTool 的符号链接拒绝。根因有据可查且就写在树里:win32 会忽略 O_NOFOLLOW,旧的符号链接用例带着 win32 skipIf,其注释正是这么说的。本 PR 修的是守卫本身而不是跳过逻辑,并新增了一个跨平台用例钉住拒绝行为。
  • 方向:对齐。该工具对不可信 Host 明确承诺私有目录约束;本改动修补的防线恰好在它失效的那个平台上形同虚设。作者以非关闭关键词引用 ci: the Windows lane is red — 59 tests across 10 files, unobserved since the merge queue stopped running #9481 的簇 3,因此没有需要处理的重复修复关联。
  • 规模:非核心模块路径(packages/cli/src/acp-integration/)——7 行生产代码(6+1)、23 行测试。未触发 Stage 0 分层。
  • 方案:最小化——在 open 前加 lstat 探测,保留 O_NOFOLLOW 作为 POSIX 的 TOCTOU 兜底。没有更小的路径(realpath 对比代码更多且有 ENOENT 边界;Node 没有暴露 Windows 上的 O_NOFOLLOW 等价物),diff 中也没有无关改动。
  • 风险acp-integration 命中本仓库 revert 历史分析的高风险路径清单。不构成阻断,但会提高审查深度:Stage 2 全程深审,且批准前要求 PR 自身的 CI 证据。

进入代码审查 🔍

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Code review

Independent baseline first: for "the symlink guard is a no-op on win32 because Windows ignores O_NOFOLLOW", the minimal fix is to probe the path with lstat before opening, keep O_NOFOLLOW as the POSIX TOCTOU backstop, and pin the rejection with a test that asserts the exact error message — the message assertion matters because it is the only way a Linux lane can distinguish fix from base. That is precisely this PR's shape, so nothing simpler was missed.

Findings:

  • No blockers. The guard sits at the top of readPrivatePng, so the rejection flows through the invocation's existing catch into the same failure(...) shape as every other refusal, and the finally cleanup still unlinks only the Host-provided path — on a symlink that removes the link, never its target. lstat probes the link itself (NTFS junctions report as symlinks on Windows), and the error is a fixed string with no path leakage into model-visible content. One extra lstat per capture is noise next to the read it guards.
  • Convention-clean. ESM, colocated test, and the comment explains the non-obvious why (Windows ignoring O_NOFOLLOW). lstat-then-reject is this repo's established symlink-guard idiom; there is no shared helper to reuse.
  • Non-blocking. On win32 a TOCTOU window remains between the probe and open — there is no O_NOFOLLOW backstop there — but the base guard was a complete no-op on that platform, so this is strictly stronger, and closing the race fully would need Windows APIs Node does not expose. Also, the older win32-skipIf test could in principle be un-skipped now; with no Windows PR lane to run it (below), the new cross-platform test carries the coverage instead, so no action needed.

Since the previous pass reviewed d44f873, two things landed: the message-pinning test (4c890fdc) and main-branch merges. The pinning test changes what the Linux lane proves: reverting just the six production lines now makes the ubuntu suite fail (open's generic ELOOP message ≠ the pinned string), whereas before it passed identically either way. The change is load-bearing in CI as of this head.

Testing evidence — the PR's own CI, read via API (no PR code executed here)

All checks on this head are completed and nothing is red. The ubuntu unit lane — the one that runs the new cross-platform test — is green; the earlier vitest-worker RPC-timeout flake reported on this PR cleared on re-run at this commit.

Two structural facts bound what this CI can prove:

  • The Windows and macOS test lanes report skipped on this PR by design: since fix(ci): give the macOS and Windows lanes a trigger again #9370 merged they trigger only on merge_group, schedule (the nightly is load-bearing) and workflow_dispatch — the pull_request trigger is deliberately off, per the standing comment in ci.yml. This PR's CI therefore never executes on Windows.
  • The win32 read-through itself — the actual bypass — is thus substantiated only by the author's local Windows 11 before/after (their claim, not independently re-run here) plus the platform contract of lstat/isSymbolicLink. Not verified: win32 behaviour in CI — no lane exists for it on this PR.

A sponsored @qwen-code /verify run is already in flight on this PR (run 33259005212, in_progress at the time of writing); its A/B report posts into this thread when it completes.

Sandboxed verification is the lane that would settle the remainder: @qwen-code /verify — sponsored run, as the author has read-only access. The win32 read-through is not observable from the diff, and no lane this PR triggers runs on Windows; a maintainer's /verify approves the head it is written against and carries a pre-execution risk screen plus a full workspace wipe. Read its report with the same skepticism as the fork's own CI logs — the code under verification is adversarial input, and a crafted PR can shape what the report says even though the sandbox bounds what it can do.

CI results for f684fdb (table region auto-updated by the triage finalize job once CI settles):

Check Conclusion
Classify PR ✅ success
Dependency CVE audit ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Real daemon E2E / Java 11 ✅ success
Secret scan (TruffleHog) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
macos-latest / Java 21 ✅ success
ubuntu-latest / Java 11 ✅ success
ubuntu-latest / Java 17 ✅ success
ubuntu-latest / Java 21 ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
windows-latest / Java 21 ✅ success
Test (windows-latest, Node 22.x) ⏭️ skipped — by design, no PR trigger
Test (macos-latest, Node 22.x) ⏭️ skipped — by design, no PR trigger
Integration Tests (CLI, No Sandbox) ⏭️ skipped — merge_group-only check name
中文说明

代码审查:先给独立基线——对"符号链接守卫因 Windows 忽略 O_NOFOLLOW 而在 win32 上形同虚设"这一问题,最小修法就是 open 前用 lstat 探测、保留 O_NOFOLLOW 作为 POSIX 的 TOCTOU 兜底,并用断言精确错误信息的用例钉住拒绝——消息断言之所以重要,是因为它是 Linux lane 唯一能区分"修与未修"的方式。本 PR 的形态与此完全一致,没有遗漏更简路径。

结论:无阻断项。守卫位于 readPrivatePng 顶部,拒绝经由调用处既有的 catch 进入与其他拒绝一致的 failure(...) 形态;finally 清理只删除 Host 提供的路径——对符号链接而言删的是链接本身而非目标。lstat 探测链接本体(Windows 上 NTFS junction 会被报告为符号链接),错误是固定字符串,不会向模型可见内容泄漏路径。符合仓库约定(ESM、同目录测试、注释解释了 Windows 忽略 O_NOFOLLOW 这一非显而易见的理由)。非阻断备注:win32 上探测与 open 之间仍有 TOCTOU 窗口(该平台无 O_NOFOLLOW 兜底),但基线守卫在该平台本就是空操作,本改动严格更强;另旧的 win32 skipIf 用例理论上可取消跳过,但当前没有会运行的 Windows PR lane,由新的跨平台用例承担覆盖即可。

与上一次审查的 d44f873 相比新增了两项:钉住错误信息的用例(4c890fdc)与两次 main 合并。钉住用例改变了 Linux lane 的证明力:仅回退 6 行生产代码现在就会让 ubuntu 套件失败(open 的通用 ELOOP 信息 ≠ 被钉住的字符串),此前两个状态在 Linux 上同样通过。截至当前 head,该改动在 CI 中是承载性的。

测试证据(通过 API 读取 PR 自身 CI,未执行任何 PR 代码):该 head 上所有检查已完成且无红。ubuntu 单元 lane(运行新跨平台用例的那条)为绿;此前提到的 vitest-worker RPC 超时 flake 在本提交的重跑中已消除。两点结构性事实限定 CI 的证明范围:Windows 与 macOS 测试 lane 在本 PR 上按设计显示 skipped——#9370 合并后它们仅由 merge_groupschedule(nightly 是承重点)与 workflow_dispatch 触发,pull_request 触发被有意关闭(见 ci.yml 中的长注释);因此本 PR 的 CI 从不在 Windows 上执行。win32 读穿链接这一实际绕过本身,目前仅有作者本地 Windows 11 的前后对比(作者声明,未在此独立复跑)与 lstat/isSymbolicLink 的平台契约支撑。未验证:win32 行为的 CI 证据——本 PR 没有对应 lane。针对本 PR 的赞助 @qwen-code /verify 运行已在途(run 33259005212,撰写时为 in_progress),完成后报告会发布在本帖。沙箱验证正是补齐其余缺口的通道:@qwen-code /verify——赞助运行(作者只有读权限);win32 读穿行为无法从 diff 观察,且本 PR 触发的所有 lane 都不在 Windows 上运行;维护者的 /verify 会对其撰写时的 head 生效并带有执行前风险筛查与完整工作区清理。请以阅读 fork 自身 CI 日志的同等怀疑态度阅读其报告——被验证的代码是对抗性输入,精心构造的 PR 可以影响报告"说什么",尽管沙箱限定了它"能做什么"。

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — a clean, minimal fix for an observed, maintainer-documented failure; the standing reservation is infrastructure-shaped, not code-shaped: no lane this PR can trigger ever executes on Windows, so the win32 behaviour itself rests on the author's local evidence plus platform contract.

Stepping back: the approach is exactly the one I'd have written — lstat probe before open, O_NOFOLLOW kept as the POSIX TOCTOU backstop, and since my last pass a cross-platform test pinning the dedicated rejection message, which also makes the change load-bearing in the ubuntu lane: revert the six production lines and the suite goes red. The problem is not a hypothesis: #9481 is maintainer-filed, names this exact failing signature on the Windows lane, and the fix replaces a win32 skipIf fig leaf with a real guard. The change is monotonically safe — POSIX behaviour is unchanged (regular files pass the probe exactly as before; symlinks were already rejected there via ELOOP), Windows is strictly stronger, and a misbehaving probe degrades to a refusal, never to reading a file it shouldn't.

The honest reservation, kept on the record: since #9370 merged, the Windows and macOS lanes trigger only via merge_group/nightly/dispatch — deliberately, per the standing ci.yml comment — so this PR's CI proves no Windows execution. The sponsored @qwen-code /verify run already in flight on this PR targets exactly that gap; its report will land in this thread. Non-blocking nits: the PR body's Evidence section still describes the pre-pinning-test state ("4 tests", the old test name and line) — cosmetic staleness only — and the older win32-skipIf test could one day be un-skipped once a Windows PR lane exists again.

Six months from now: six lines, one comment, one test, no new abstractions — thank them, not curse them.

CI: every pull_request workflow run on this head completed green and nothing is pending, so no deferred approval — approving now, pinned to the reviewed commit. Merging still needs the second human approval main requires; the /verify report above is there for whoever wants win32 evidence before that.

中文说明

置信度:4/5 —— 对一个已被维护者记录在案的观测到的失败,这是干净、最小的修复;唯一的保留意见属于基础设施层面而非代码层面:本 PR 能触发的所有 lane 都不会在 Windows 上执行,因此 win32 行为本身依托作者的本地证据与平台契约。

退一步看:方案与我会写的完全一致——open 前用 lstat 探测,保留 O_NOFOLLOW 作为 POSIX 的 TOCTOU 兜底;在上次审查之后又新增了一个跨平台用例钉住专用拒绝信息,这同时使改动在 ubuntu lane 中变得承载性:仅回退那 6 行生产代码套件就会变红。问题不是假设:#9481 由维护者提交,点名了 Windows lane 上完全相同的失败签名;本修复用真正的守卫替换了 win32 skipIf 这块遮羞布。改动单调更安全——POSIX 行为不变(常规文件通过探测的方式与之前完全一致;符号链接在那里本就经由 ELOOP 被拒绝),Windows 上严格更强,探测即使意外失效也只是退化为拒绝,而绝不会读进不该读的文件。

如实保留的意见:#9370 合并后,Windows 与 macOS lane 仅由 merge_group/nightly/dispatch 触发——按 ci.yml 长注释这是有意为之——因此本 PR 的 CI 证明不了任何 Windows 执行。已在途的赞助 @qwen-code /verify 运行正对该缺口而来,报告会发布在本帖。非阻断的小问题:PR 正文的证据部分仍在描述钉住用例加入之前的状态("4 tests"、旧用例名与行号)——仅是表述滞后;旧的 win32 skipIf 用例将来可在 Windows PR lane 恢复后再取消跳过。

六个月后维护这段代码:六行、一条注释、一个用例、没有新抽象——会感谢作者。

CI:该 head 上所有 pull_request workflow 运行均已绿且无挂起,因此不做延迟批准——现在即批准,并绑定到被审查的提交。合并仍需 main 所要求的第二个人类批准;上方 /verify 报告可供需要在合并前看到 win32 证据者参考。

Qwen Code · qwen3.8-max

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, looks ready to ship — CI landed green after the review. ✅

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed. Suggestions are inline.

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.

Not reviewed: reverse audit — stopped before round 3 by the review time budget.

Test Plan (not a blocker): src/acp-integration/live/capture-screen-context.test.tsno such file or directory; 4 tests pass — this review observed 23597 passed; Tests 4 passed — this review observed 23597 passed; 3 passed — this review observed 23597 passed.

中文说明

仅完成部分审查,审查缺口已披露。 建议见行内评论。

未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。

未审查:反向审计——评审时间预算不足,未能开始第 3 轮。

Test Plan(非阻断):src/acp-integration/live/capture-screen-context.test.tsno such file or directory; 4 tests pass — this review observed 23597 passed; Tests 4 passed — this review observed 23597 passed; 3 passed — this review observed 23597 passed

— qwen3.8-max via Qwen Code /review (v0.22.0)

// Windows silently ignores O_NOFOLLOW, so a symlinked screenshot path
// would be followed and read on win32. Probe the link itself first.
if ((await lstat(path)).isSymbolicLink()) {
throw new Error('Host returned a symbolic link screenshot path.');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The new win32 symlink guard is not gated by any test: the pre-existing symlink test asserts only expect(result.error?.message).toBeTruthy(), and on the Linux PR-gate runners open(..., O_NOFOLLOW) throws ELOOP for a symlinked path anyway, so deleting the four added probe lines keeps the whole suite green. A future change that removes or breaks the win32 probe would pass PR CI silently and surface only in the post-approval Windows merge-queue job, re-opening the issue 9481 failure cluster this PR fixes.

Probe evidence (Linux scratch tree, this review):

  • probe deleted → symlink test still green via the ELOOP fallback (Test Files 3 passed (3)), error='ELOOP: too many symbolic links encountered', truthy assertion passes
  • strengthened assertion → red on the mutant: expected 'ELOOP: too many symbolic links encoun…' to be 'Host returned a symbolic link screens…'

Strengthen the assertion in capture-screen-context.test.ts so removing the probe fails on every platform:

expect(result.error?.message).toBe(
  'Host returned a symbolic link screenshot path.',
);
中文说明

新的 win32 符号链接守卫没有任何测试把关:现有的符号链接测试只断言 expect(result.error?.message).toBeTruthy(),而在 Linux PR 门禁的 runner 上,无论是否有新增的探测代码,open(..., O_NOFOLLOW) 都会对符号链接路径抛出 ELOOP,因此删除新增的 4 行探测代码后整个测试套件依然是绿的。未来若有改动移除或破坏了 win32 探测,PR CI 会静默放行,只有批准后的 Windows 合并队列任务才会发现,等于重新打开本 PR 修复的 issue 9481 失败簇。

探测证据(Linux 草稿树,本次审查):

  • 删除探测代码 → 符号链接测试经 ELOOP 兜底仍然通过(Test Files 3 passed (3)),truthy 断言通过;
  • 加强断言后 → 突变体变红:expected 'ELOOP: too many symbolic links encoun…' to be 'Host returned a symbolic link screens…'

capture-screen-context.test.ts 中加强断言,使删除探测代码在所有平台上都会让测试失败:

expect(result.error?.message).toBe(
  'Host returned a symbolic link screenshot path.',
);

— qwen3.8-max via Qwen Code /review (v0.22.0)

…context

The lstat guard added for win32 had no test that fails when it is removed: on Windows dropping the guard makes the tool read through the link, but on POSIX CI O_NOFOLLOW still rejects the link with a generic ELOOP error, so the regression would be invisible there. Assert the exact 'Host returned a symbolic link screenshot path.' message so both removal paths turn the suite red. Addresses review finding R1-1 (Suggestion).

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ Downgraded from Approve to Comment: CI still running. Partially reviewed — gaps disclosed.

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.

Test Plan (not a blocker): src/acp-integration/live/capture-screen-context.test.tsno such file or directory.

中文说明

⚠️ 已从批准降级为评论:CI still running。 仅完成部分审查,审查缺口已披露。

未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。

Test Plan(非阻断):src/acp-integration/live/capture-screen-context.test.tsno such file or directory

— qwen3.8-max via Qwen Code /review (v0.22.0)

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x). Partially reviewed — gaps disclosed.

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): "agent 1a": did not execute capture-screen-context.test.ts — the review worktree has no node_modules and no built workspace dist/ packages, so running the spec would ….

Test Plan (not a blocker): src/acp-integration/live/capture-screen-context.test.tsno such file or directory; 4 tests pass — this review observed 23601 passed; Tests 4 passed — this review observed 23601 passed; 3 passed — this review observed 23601 passed.

Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/acp-integration/live/capture-screen-context.test.ts:96 — [review] New symlink test duplicates the adjacent test's setup; assertions can fold into one
中文说明

⚠️ 已从批准降级为评论:CI failing: Test (ubuntu-latest, Node 22.x)。 仅完成部分审查,审查缺口已披露。

未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。

未探索到全部深度(达到工具调用预算):"agent 1a"did not execute capture-screen-context.test.ts — the review worktree has no node_modules and no built workspace dist/ packages, so running the spec would …

Test Plan(非阻断):src/acp-integration/live/capture-screen-context.test.tsno such file or directory; 4 tests pass — this review observed 23601 passed; Tests 4 passed — this review observed 23601 passed; 3 passed — this review observed 23601 passed

收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.22.0)

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

⚠️ Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x). Partially reviewed — gaps disclosed.

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.

Test Plan (not a blocker): src/acp-integration/live/capture-screen-context.test.tsno such file or directory.

Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:

  • packages/cli/src/acp-integration/live/capture-screen-context.test.ts:96 — [review] New symlink test duplicates the adjacent test's setup; assertions can fold into one
中文说明

⚠️ 已从批准降级为评论:CI failing: Test (ubuntu-latest, Node 22.x)。 仅完成部分审查,审查缺口已披露。

未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。

Test Plan(非阻断):src/acp-integration/live/capture-screen-context.test.tsno such file or directory

收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。

— qwen3.8-max via Qwen Code /review (v0.22.0)

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed. Suggestions are inline.

Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally.

Test Plan (not a blocker): src/acp-integration/live/capture-screen-context.test.tsno such file or directory; 4 tests pass — this review observed 24761 passed; Tests 4 passed — this review observed 24761 passed; 3 passed — this review observed 24761 passed.

中文说明

仅完成部分审查,审查缺口已披露。 建议见行内评论。

未审查:build-and-test — Test (windows-latest, Node 22.x) was skipped in CI and its suite did not run locally。

Test Plan(非阻断):src/acp-integration/live/capture-screen-context.test.tsno such file or directory; 4 tests pass — this review observed 24761 passed; Tests 4 passed — this review observed 24761 passed; 3 passed — this review observed 24761 passed

— qwen3.8-max via Qwen Code /review (v0.22.0)

Comment on lines +101 to +103
it('reports the dedicated symlink error on every platform', async () => {
const target = await captureFile();
const link = join(target.directory, 'linked.png');

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The sibling test above — rejects a symlink and deletes only the Host-provided link — is still gated it.skipIf(process.platform === 'win32') with the comment "The rejection relies on O_NOFOLLOW, which libuv ignores on win32". That rationale no longer holds: rejection now goes through the platform-agnostic lstat guard this PR adds, and this new test beside it runs the identical setup on every platform. The skip entered this branch through the merge with main (#9728, which silenced the then-failing test); with the guard in place the sibling test passes on win32 as written.

This matters because the sibling test is the only test asserting the cleanup semantics — the symlink's target survives and only the Host-provided link is unlinked. While it stays win32-skipped, nothing verifies those assertions on the very platform this PR hardens: a future change that regresses the finally-block unlink on win32 (resolving/realpath-ing the path before unlink and deleting the target instead of the link, or failing to remove the link) ships green on the Windows lane. The stale comment also invites a future maintainer to conclude win32 symlink rejection is untestable and re-introduce a skip or remove the guard.

Suggested fix: drop .skipIf(process.platform === 'win32') from the sibling test and rewrite its comment — e.g. "Rejection goes through the explicit lstat guard on every platform; O_NOFOLLOW is the POSIX TOCTOU backstop." Symlink creation is smoke-validated on this repo's self-hosted Windows pool (windows-runner-smoke.yml, "Verify symbolic links" step), and this PR's own new test already calls symlink() ungated. Alternatively, fold the sibling's two filesystem assertions into this test and delete the duplicate.

中文说明

上方相邻的用例 rejects a symlink and deletes only the Host-provided link 仍然被 it.skipIf(process.platform === 'win32') 跳过,且注释写着 "The rejection relies on O_NOFOLLOW, which libuv ignores on win32"。这个理由已经不再成立:拒绝逻辑现在走的是本 PR 新增的、与平台无关的 lstat 守卫,而旁边这个新用例已经在所有平台上运行完全相同的准备步骤。这个跳过是经由与 main 的合并(#9728,当时为了 silenced 失败的测试)进入本分支的;在守卫就位后,相邻用例在 win32 上按原样即可通过。

这一点很重要,因为相邻用例是唯一断言清理语义的测试——符号链接的目标文件保持完好、只有 Host 提供的链接本身被删除。只要它在 win32 上仍被跳过,在本 PR 所加固的这个平台上就没有任何测试验证这些断言:未来某个改动若在 win32 上破坏了 finally 块中的 unlink(例如先 resolve/realpath 再删除、从而删掉了目标文件而不是链接,或根本没有删掉链接),会在 Windows 流水线上绿灯通过。过时的注释还会误导后来的维护者,使其以为 win32 上无法测试符号链接拒绝,从而重新引入跳过或删除守卫。

建议修复:去掉相邻用例上的 .skipIf(process.platform === 'win32'),并改写其注释——例如 "Rejection goes through the explicit lstat guard on every platform; O_NOFOLLOW is the POSIX TOCTOU backstop."。本仓库自托管 Windows 资源池已通过冒烟流程验证可以创建符号链接(windows-runner-smoke.yml 的 "Verify symbolic links" 步骤),且本 PR 自己的新用例已经在不加门控的情况下调用 symlink()。也可以把相邻用例的两个文件系统断言合并进本用例,然后删除重复的用例。

— qwen3.8-max via Qwen Code /review (v0.22.0)

@wenshao

wenshao commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

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: 37 passed · 0 failed · 37 total

Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:37 通过 · 0 失败 · 37 总计

抖动门:✅ 1 changed test file(s) x 5 identical rounds, no divergence

Verification report

Verification report — PR #9847

Verdict: merge-ready — 37/37 scripted assertions passed, 0 unexpected failures. Verified head: f684fdb5eda7bca70c9a213ebd15bb48aa4971a5 (merge base 03b03c9da04a7ca6407175ea94cc3c4b53603751).

中文摘要
  • 结论merge-ready。37 项脚本化断言全部通过,0 项意外失败。
  • A/B 结论:同一套真实文件系统夹具(真符号链接、真 PNG、真 unlink)在 base(03b03c9)与 head(f684fdb)上各跑 14 项断言,均按各自预期通过。base 上符号链接被 open(O_NOFOLLOW) 以通用 ELOOP 拒绝(POSIX),head 上在 open 之前由 lstat 守卫以专用消息 Host returned a symbolic link screenshot path. 拒绝——含悬空符号链接;普通 PNG、目录约束、清理语义(只删链接、目标完好)两臂一致。win32 上 O_NOFOLLOW 被忽略的绕过本身无法在 Linux 容器内复现,但关闭它的平台无关守卫已被证明承担作用(见下表与 01-ab-symlink-base-vs-head.png)。
  • 测试钉住性:变异矩阵(02-mutation-matrix-guard-and-message.png)——未变异 5/5 绿;删除 lstat 守卫后恰好新增用例以行为性不匹配变红(旧符号链接用例在 POSIX 上仍绿,印证 commit 2 的"不可见回归"说法);改消息字符串亦变红。守卫与消息均被钉住。
  • Findings:1 条 Suggestion(预存、非本 PR 引入):Host 在私有目录内放置 FIFO 会使工具无限阻塞,两臂 A/A 均挂起(03-fifo-aa-both-arms-hang.png);同一 lstat 结果上加 !isFile() 的候选补丁已实测(FIFO 27ms 内拒绝,14/14 与 5/5 无副作用),但现有套件两侧皆绿、未钉住该轴。其余为 nit/信息级(缺失文件错误文本由 open 变 lstat;旧用例 skip 注释过期;win32 残留 TOCTOU;其他 O_NOFOLLOW 调用点同族弱点,作者已声明超出范围)。
  • 未覆盖:win32 实际行为与 Windows CI 符号链接创建能力(Linux 容器无法执行);PR 所述 ubuntu CI flake(无 GitHub token,本环境未复现);逐 commit 归因(浅克隆,仅聚合 diff);仓库级全量测试(已跑定向门:live/ 11/11、Session.test.ts 722/722、cli typecheck 含活性验证)。

Central claim and A/B

Central claim: capture_screen_context rejects a symlinked screenshot path with a dedicated error before opening it, on every platform — closing the win32 hole where O_NOFOLLOW is silently ignored and a Host-returned symlink would have its target read as the screenshot, bypassing the private-directory containment promise.

Secondary claims: (1) the new exact-message test pins the guard on POSIX CI, where the old test alone would stay green; (2) non-symlink behavior (success path, containment, cleanup) is unchanged.

Harness: harness/symlink-ab.mjs — mock-free, drives the real CaptureScreenContextTool.build({}).execute() over real mkdtemp dirs, real symlink(), real PNG bytes; base arm loads the module from a scratch worktree at 03b03c9 (realpath asserted into the base tree; git diff --stat HEAD^1..HEAD -- packages/core is empty, so the shared @qwen-code/qwen-code-core dependency resolving into the head tree is identical code). Witness: 01-ab-symlink-base-vs-head.png; raw logs logs/ab-base.log, logs/ab-head.log.

cell fixture base (03b03c9) head (f684fdb)
C1 symlink → valid PNG, same dir rejected, ELOOP … open '…' rejected, dedicated message
C2 symlink → target outside capture dir (containment-bypass shape) rejected, ELOOP rejected, dedicated message
C3 regular PNG (positive control, both arms) success, base64 roundtrip, file consumed identical
C4 path outside capture dir outside its private directory, target intact identical
C5 dangling symlink rejected, ELOOP rejected, dedicated message
C6 nonexistent path ENOENT … open ENOENT … lstat (see Corrections)
per-arm scripted assertions 14/14 as-expected 14/14 as-expected

The C2 cell is the security-relevant shape: the textual containment check passes (the link is inside the private dir), so on a platform where open follows the link the outside target would have been read; on head the guard rejects before open on every platform. The win32 read-through itself is OS semantics not reproducible on this Linux container — what is proven here is the platform-independent guard that closes it, plus the POSIX control behavior (see Not covered).

Reviewer Test Plan walkthrough

  1. "Run npx vitest run src/acp-integration/live/capture-screen-context.test.ts inside packages/cli" — ran at head: 5 passed (5) (Tests 5 passed (5)), matching the PR's CI note of "(5 tests)". ✔
  2. "Before this change, the test failed on Windows with expected undefined to be truthy at line 91" — the Windows before-behavior (tool reads through the link, error undefined) is not reproducible on Linux; the closest local evidence: on the base arm the old test's toBeTruthy() assertion would pass via ELOOP (it did in M1 below), and the win32 read-through is OS-level. Not verifiable here; the claim is consistent with the old test's line-91 assertion shape.
  3. "After this change all tests pass; only the link is removed and the target stays intact" — verified on Linux for both symlink cells (C1/C2: target-intact and link-removed PASS on head). ✔
  4. "POSIX behavior identical to before" — holds for outcomes (C1–C5) with one error-text delta on the missing-file path (Corrections below).

Corrections

  • "POSIX behavior is identical to before" is not byte-exact for the missing-file error path. Cell C6: base reports ENOENT: no such file or directory, open '…', head reports ENOENT: …, lstat '…' — the probe now originates from lstat instead of open. Functionally identical (still an ENOENT failure, still cleaned up); inherent to adding the probe. Labelled as a correction to the description's "identical" wording, not a code defect.

Findings

F1 (Suggestion, pre-existing — not introduced by this PR): a FIFO planted as the screenshot hangs the tool on every platform. readPrivatePng's lstat probe rejects only symlinks; open(O_RDONLY) on a FIFO blocks until a writer appears. A/A probe (harness/fifo-probe.mjs under timeout 5, witness 03-fifo-aa-both-arms-hang.png): base exit 124, head exit 124 — identical hang, so this is the pre-existing shape, unchanged by the guard. The repo's own hardened sibling (packages/cli/src/serve/live/discovery.ts, same trust boundary) already rejects non-regular files via !stat.isFile() on the lstat result; this PR adopted the symlink half of that pattern but not the regular-file half. Measured candidate hardening (scratch, then restored): add if (!hostPathStat.isFile()) throw new Error('Host returned an invalid screenshot file.') after the symlink check on the same lstat result — FIFO rejected in 27 ms, harness 14/14 and suite 5/5 unchanged (zero collateral). The suite is green with and without the patch, i.e. nothing pins the FIFO axis; the fixture that would pin it is the FIFO probe itself. Severity is bounded: the attacker is a Host that can already write into its own private directory, so this is availability hardening, not a containment bypass — hence Suggestion, not blocker.

F2 (Nit): stale skip comment on the old symlink test. capture-screen-context.test.ts:76 says "The rejection relies on O_NOFOLLOW, which libuv ignores on win32" — at head the rejection no longer relies on O_NOFOLLOW (the lstat guard is platform-uniform, and the new test runs unskipped on win32). The skipIf(win32) on the old test is now conservative rather than necessary; it could be unskipped in a follow-up. Harmless.

F3 (Informational): residual win32 TOCTOU between lstat and open. A Host racing a regular file into a symlink after the probe is backstopped by O_NOFOLLOW on POSIX but by nothing on win32. The PR acknowledges this design ("O_NOFOLLOW kept as a TOCTOU backstop" for POSIX); closing the win32 window requires reparse-point-aware open semantics, out of this PR's scope.

F4 (Informational, author-declared out of scope): sibling O_NOFOLLOW call sites share the win32 no-op weakness. The PR fixes cluster 3 of #9481 and explicitly does not audit the rest. Sites passing O_NOFOLLOW unconditionally (silently dropped on win32, same class as the fixed bug): packages/channels/wecom/src/WeComAdapter.ts:1563, packages/acp-bridge/src/sessionArtifacts.ts:3307, packages/cli/src/commands/channel/pidfile.ts:244, packages/cli/src/serve/conversations/conversation-runtime-ownership.ts:226. Sites already following the repo's win32 ? 0 : O_NOFOLLOW + extra-guard convention (e.g. sessionService.ts, discovery.ts, skill-args-file.ts) accept the gap deliberately. Listed for the maintainer; not a condition on this PR.

Mutation / vacuity matrix

Witness: 02-mutation-matrix-guard-and-message.png. Control (unmutated head): 5/5 green.

mutant suite result classification
M1: lstat guard removed (revert of the key hunk) 1 failed | 4 passed — exactly the new test, behavioral mismatch (expected 'ELOOP: …' to be 'Host returned a symbolic link screenshot path.') killed; the old symlink test stayed green under M1, confirming commit 2's claim that on POSIX CI the old test alone cannot see the regression
M2: guard kept, message string changed 1 failed | 4 passed — exact-equality mismatch on the new test killed (message pinned)

Both mutations landed in the mutated file's own suite and failed the intended assertion with expected-vs-actual values — the positive control for the harness is the M1/M2 red itself. No survivors: the PR introduces exactly one guard and one pinning assertion, and each kills its mutant.

Targeted gates (all at head)

  • npx vitest run src/acp-integration/live/capture-screen-context.test.ts — 5/5.
  • npx vitest run src/acp-integration/live/ — 4 files, 11/11.
  • npx vitest run src/acp-integration/session/Session.test.ts (the tool's consumer) — 722/722.
  • npm run typecheck -w packages/cli (tsc --noEmit) — clean; liveness proven by planting a type error (tsc failed) and restoring (clean).

Not covered

  • win32 behavior itself (O_NOFOLLOW ignored; the read-through repro; symlink creation on the Windows lane). This container is Linux; the test_windows lane runs only for merge_group/schedule/dispatch, and there is no GitHub access here. Mitigating precedent: skill-args-file.test.ts already creates symlinks in unskipped tests, so the lane tolerates symlink creation; the new test's unskipped symlink() is consistent with that precedent.
  • The PR's claim that the red Test (ubuntu-latest, Node 22.x) check was a vitest worker-RPC flake — no token to inspect the CI log; none of my five vitest runs exhibited the RPC timeout.
  • Per-commit attribution — checkout is depth 2; only the merge-from-main commit is reachable locally while the metadata lists five commits. The aggregate HEAD^1..HEAD diff (2 files) is what was verified.
  • Repo-wide test suite — only the targeted gates above were run.

Methodology

Environment: CI verify container, node v22.23.2, Linux; npm ci/npm run build pre-completed at head. The A/B harness imports the real module per arm via tsx (base from git worktree at 03b03c9, head from the merge tree), asserts each arm's module realpath, and confirms the shared core dependency is untouched by the diff. All harnesses are mock-free: real filesystem fixtures, real tool execution; the only mutation runs were applied in place and restored (final git status clean, base worktree removed). Raw logs in logs/ (ab-base.log, ab-head.log, fifo-base.log, fifo-head.log, capture-screen-context.ts.orig); harnesses in harness/. Assertion count: 28 harness (14/arm) + 3 mutation + 2 FIFO A/A + 2 typecheck liveness + 2 isolation = 37.

Flakiness gate log

rounds=5 files=1 skipped=0
file packages/cli/src/acp-integration/live/capture-screen-context.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/live/capture-screen-context.test.ts


per-file results (P=pass F=fail I=infra-exit, one letter per run):
  packages/cli/src/acp-integration/live/capture-screen-context.test.ts: PPPPP

verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence

--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/acp-integration/live/capture-screen-context.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/live/capture-screen-context.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/live/capture-screen-context.test.ts: P (exit 0)
round 4 · packages/cli/src/acp-integration/live/capture-screen-context.test.ts: P (exit 0)
round 5 · packages/cli/src/acp-integration/live/capture-screen-context.test.ts: P (exit 0)

Evidence images

01-ab-symlink-base-vs-head

02-mutation-matrix-guard-and-message

03-fifo-aa-both-arms-hang

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, looks ready to ship. ✅

@wenshao

wenshao commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator

Local verification on a real build — maintainer review

I rebuilt this PR merged into origin/main in a standalone clone and drove the real CaptureScreenContextTool against real files on disk, rather than trusting mocks. The bug only exists on a platform I don't have, so I reproduced Windows' open(2) semantics on macOS at the syscall boundary.

Setup

Tree standalone clone; PR head f684fdb merged into origin/main (7503a3b) → 695ba51; git diff --stat upstream-main = 2 files changed, 29 insertions(+), 1 deletion(-) — exactly this PR
Tool under test the actual module compiled from repo TS by esbuild, importing the real @qwen-code/qwen-code-core build; two artifacts (main / this PR) driven by one harness
Windows lane a DYLD_INSERT_LIBRARIES interpose that strips O_NOFOLLOW from open(2)/openat(2) for paths under the fixture root — production code untouched
Host macOS 26.6.2 (Darwin 25.6.0, arm64), Node v24.18.1

The emulation is faithful, not a guess: libuv defines UV_FS_O_NOFOLLOW as 0 on win32 (include/uv/win.h), so the real platform drops the flag exactly the way the interpose does.

1. The bug is real, and this PR closes it

symlink escape A/B

The Host hands back a symlink whose target lives outside its private directory. On the Windows lane, main answers ok: true with returnedIsSecret: true — the tool read a file it had promised it would never reach, and handed those bytes to the model as inlineData. With this PR, the same input on the same lane is rejected with Host returned a symbolic link screenshot path.

On stock macOS both versions reject; only the error text changes (ELOOP → the dedicated message). In every case cleanup removed the Host-provided link and left the target intact (linkStillOnDisk: false, targetStillOnDisk: true).

2. The repo's own suite reproduces the author's Windows failure

vitest on the emulated Windows lane

Reverting only the production hunk (keeping the PR's test file) and running capture-screen-context.test.ts on the Windows lane fails with expected undefined to be truthy on rejects a symlink and deletes only the Host-provided link — the exact assertion and message the PR description quotes from Windows 11. Restore the hunk, same command, same lane: 5 passed.

Context worth recording: on today's main that older test is it.skipIf(process.platform === 'win32') — I added that skip myself in #9728, after this PR was opened, so the "before" evidence in the description was accurate when it was written and is no longer reproducible against current main. The PR's new always-run test reports the dedicated symlink error on every platform is what actually restores win32 coverage here, and it also kills the mutant on macOS (ELOOP… ≠ the pinned string), so it is not Windows-only ballast.

3. Full scenario matrix — no behaviour regressions

scenario matrix

8 Host-supplied path shapes × 2 platform lanes. Everything that used to succeed still succeeds (real PNG, hard link); everything that used to be rejected is still rejected, including the containment check. Exactly two rows change: the win32 symlink read (fixed) and the ENOENT text (openlstat; nothing in the repo asserts on it).

4. Provenance, lint, types, bundle, regression sweep

regression sweep

eslint clean on both touched files, tsc --noEmit clean for packages/cli, the guard string survives into the shipped bundle (dist/chunks/acpAgent-*.js, i.e. not just the test build), and all 39 files / 1818 tests under packages/cli/src/acp-integration/ pass.

Notes for whoever merges this

  1. Scope, stated honestly. The Live Host is macOS-only today — live-host-installer.ts returns "Qwen Live Host is available only on macOS.", packages/live-host/electron-builder.yml defines only a mac: target, and the Host channel is loopback-only. No user is exposed on Windows right now, so this is hardening plus restored coverage plus a clearer error, not an incident fix. Still worth taking: the cost is one lstat, and the design is the same one core already uses in gitDiff.ts countUntrackedLines (lstat first, O_NOFOLLOW as the TOCTOU backstop, with a comment noting Windows omits the flag).
  2. The NTFS junction claim checks out. libuv's fs__stat_assign_statbuf sets S_IFLNK for any reparse point under lstat, so junctions are covered. Side effect: so is every other reparse point (OneDrive placeholders and friends) — harmless for a private temp directory, but it is a rejection rather than a follow.
  3. Residual gap, pre-existing and unchanged by this PR. If the private directory itself is a symlink/junction, containment is still bypassed on both platforms (the two private DIR itself is a symlink rows): resolvePrivatePngPath compares lexically and lstat only probes the final path component. Follow-up material, not a blocker — acpAgent.ts's isOwnerOnlyDirectory path already has the shape to copy (lstat + realpath round trip + dev/ino identity).
  4. Residual gap on win32 only. lstatopen is check-then-use. I forced a swap between the two calls: on macOS O_NOFOLLOW rejects it with ELOOP (so the PR's "TOCTOU backstop" wording is accurate), while on the Windows lane the read goes through. Closing it would mean comparing dev/ino between the lstat and the opened handle's fstat. Also follow-up.
  5. One stale section in the description. The "Note on the failing check" block no longer applies: that ubuntu job has since passed (37m52s) and every non-skipped check on f684fdb is green. Note Test (windows-latest, Node 22.x) reports skipping — since ci: take the macOS and Windows lanes off pull requests #10059 the Windows lane does not run on PRs, so CI structurally cannot validate the behaviour this PR fixes. That is precisely why I ran the lane locally.

Verdict: LGTM — recommend merge. No blocking findings. Items 3 and 4 are pre-existing and belong in their own issue under #9481 rather than as scope creep here.

How to reproduce the Windows lane locally (macOS)
// win32_open_emu.c — strip O_NOFOLLOW the way libuv does on win32
#include <fcntl.h>
#include <stdarg.h>
#include <stdlib.h>
#include <string.h>

typedef struct interpose_s { void *new_func; void *orig_func; } interpose_t;

static int in_scope(const char *path) {
  const char *prefix = getenv("EMU_PREFIX");
  return prefix && path && strncmp(path, prefix, strlen(prefix)) == 0;
}

int emu_open(const char *path, int flags, ...) {
  mode_t mode = 0;
  if (flags & O_CREAT) {
    va_list ap; va_start(ap, flags); mode = (mode_t)va_arg(ap, int); va_end(ap);
  }
  if (in_scope(path)) flags &= ~O_NOFOLLOW;
  return open(path, flags, mode);
}

__attribute__((used)) static const interpose_t interposers[]
    __attribute__((section("__DATA,__interpose"))) = {
        {(void *)emu_open, (void *)open},
};
clang -O2 -dynamiclib -o win32_open_emu.dylib win32_open_emu.c

# Smoke test: this must FAIL with ELOOP without the dylib and SUCCEED with it.
ln -s target.txt link.txt
node -e "fs=require('fs');fs.openSync('link.txt',fs.constants.O_RDONLY|fs.constants.O_NOFOLLOW)"

# Then run the suite on the emulated lane. Launch vitest through node directly:
# /bin/sh is SIP-protected and would strip DYLD_INSERT_LIBRARIES from the child.
cd packages/cli
EMU_PREFIX="$(node -p 'require("os").tmpdir()')" \
DYLD_INSERT_LIBRARIES=/abs/path/win32_open_emu.dylib \
node ../../node_modules/vitest/vitest.mjs run \
  src/acp-integration/live/capture-screen-context.test.ts --reporter=verbose

With main's readPrivatePng this reports Tests 2 failed | 3 passed (5); with this PR, Tests 5 passed (5).

中文说明

本地真实环境验证 — 维护者复核

我在一个独立 clone 里把本 PR 合入 origin/main 重新构建,并用真实CaptureScreenContextTool 对磁盘上真实的文件跑验证,而不是依赖 mock。这个缺陷只在我手上没有的平台上存在,所以我在 macOS 上从系统调用层复现了 Windows 的 open(2) 语义。

环境

代码树 独立 clone;PR head f684fdb 合入 origin/main7503a3b)得到 695ba51git diff --stat upstream-main2 files changed, 29 insertions(+), 1 deletion(-),即本 PR 本身
被测对象 由仓库 TS 源经 esbuild 编译出的真实模块,导入真实的 @qwen-code/qwen-code-core 构建产物;main 与本 PR 两份产物,由同一个 harness 驱动
Windows 泳道 一个 DYLD_INSERT_LIBRARIES interpose,对 fixture 根目录下的路径从 open(2)/openat(2) 里剥掉 O_NOFOLLOW;生产代码零改动
宿主 macOS 26.6.2(Darwin 25.6.0, arm64),Node v24.18.1

这个模拟不是猜的:libuv 在 win32 上把 UV_FS_O_NOFOLLOW 定义为 0include/uv/win.h),真实平台丢弃该标志的方式与 interpose 完全一致。

1. 缺陷真实存在,本 PR 确实堵住了它(图 1)

Host 返回一个指向私有目录之外的符号链接。在 Windows 泳道上,main 返回 ok: truereturnedIsSecret: true——工具读到了它承诺永不触碰的文件,并把这些字节以 inlineData 交给了模型。打上本 PR 后,同一输入在同一泳道被拒绝,错误为 Host returned a symbolic link screenshot path.

在原生 macOS 上两个版本都拒绝,只是错误文案不同(ELOOP → 专用文案)。所有场景下清理都只删掉了 Host 给的链接、原目标文件完好(linkStillOnDisk: falsetargetStillOnDisk: true)。

2. 仓库自带的测试复现了作者报告的 Windows 失败(图 2)

回退生产代码那一段(保留本 PR 的测试文件),在 Windows 泳道跑 capture-screen-context.test.ts,失败于 rejects a symlink and deletes only the Host-provided linkexpected undefined to be truthy——与 PR 描述中引用的 Windows 11 断言与文案逐字一致。恢复该段后,同一命令同一泳道:5 passed。

一点值得记录的上下文:当前 main 上那条旧用例是 it.skipIf(process.platform === 'win32'),这个 skip 是我自己在 #9728 里加的,时间晚于本 PR 开出,所以描述里的「before」证据在写下时是准确的,只是已无法在今天的 main 上复现。本 PR 新增的、全平台都跑的 reports the dedicated symlink error on every platform 才是真正把 win32 覆盖补回来的那条;而且它在 macOS 上同样能杀死变异体(ELOOP… ≠ 被钉住的字符串),并不是只对 Windows 有意义的摆设。

3. 完整场景矩阵——无行为回归(图 3)

8 种 Host 给出的路径形态 × 2 个平台泳道。原本成功的仍然成功(真实 PNG、硬链接);原本被拒的仍然被拒,包含目录约束检查。只有两行发生变化:win32 上的符号链接读取(已修复)与 ENOENT 文案(openlstat,仓库里没有任何地方断言它)。

4. 溯源、lint、类型、打包与回归扫描(图 4)

两个改动文件 eslint 干净,packages/clitsc --noEmit 干净,护栏字符串进入了发布 bundle(dist/chunks/acpAgent-*.js,即不只存在于测试构建),packages/cli/src/acp-integration/39 个文件 / 1818 条用例全绿。

给合并者的几点说明

  1. 范围要说实话。 Live Host 目前仅支持 macOS——live-host-installer.ts 会返回 "Qwen Live Host is available only on macOS."packages/live-host/electron-builder.yml 只定义了 mac: 目标,且 Host 通道仅走 loopback。也就是说当下没有 Windows 用户暴露在该问题下;本 PR 属于加固 + 补回覆盖 + 更清晰的错误,而不是止血。但仍然值得合:代价只是一次 lstat,而且这个设计与 core 里 gitDiff.tscountUntrackedLines 完全一致(先 lstat,O_NOFOLLOW 作为 TOCTOU 兜底,注释里也写明了 Windows 没有该标志)。
  2. NTFS junction 的说法成立。 libuv 的 fs__stat_assign_statbuf 在 lstat 下对任意 reparse point 都置 S_IFLNK,所以 junction 会被覆盖。副作用是其它 reparse point(OneDrive 占位文件之类)也会被一并拒绝——对一个私有临时目录无害,但确实是拒绝而非跟随。
  3. 遗留缺口,早已存在且本 PR 未改变。 如果私有目录本身是符号链接/junction,两个平台上目录约束依然会被绕过(矩阵中两行 private DIR itself is a symlink):resolvePrivatePngPath 是词法比较,而 lstat 只探测最后一段路径。属于后续工作,不构成阻塞——acpAgent.tsisOwnerOnlyDirectory 那条路径已经有可以照抄的形态(lstat + realpath 往返 + dev/ino 同一性)。
  4. 仅限 win32 的遗留缺口。 lstatopen 是 check-then-use。我在两次调用之间强制做了替换:macOS 上 O_NOFOLLOWELOOP 拒绝(所以 PR 里「TOCTOU 兜底」的措辞是准确的),而 Windows 泳道上读取会穿过去。要堵住它需要比较 lstat 与已打开句柄 fstatdev/ino。同样归入后续。
  5. 描述里有一处已过时。 「Note on the failing check」那一段不再适用:那个 ubuntu job 后来已经通过(37m52s),f684fdb 上所有非 skip 的检查均为绿。注意 Test (windows-latest, Node 22.x) 显示为 skipping——自 ci: take the macOS and Windows lanes off pull requests #10059 起 Windows 泳道不在 PR 上运行,因此 CI 在结构上就无法验证本 PR 所修复的行为。这也正是我在本地把这条泳道跑起来的原因。

结论:LGTM,建议合并。 没有阻塞项;第 3、4 条属于既有问题,应在 #9481 下另开 issue,而不是在本 PR 里扩范围。

@wenshao
wenshao added this pull request to the merge queue Aug 29, 2026
Merged via the queue into QwenLM:main with commit 8cfa895 Aug 29, 2026
91 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants