fix(core): keep no-follow reads protected where O_NOFOLLOW is missing - #10007
fix(core): keep no-follow reads protected where O_NOFOLLOW is missing#10007yiliang114 wants to merge 20 commits into
Conversation
O_NOFOLLOW does not exist on Windows: fs.constants.O_NOFOLLOW is undefined, so the `(O_RDONLY | (O_NOFOLLOW ?? 0))` flag expressions silently collapse into a plain open that follows symlinks, dropping the symlink/TOCTOU hardening added for @-referenced file reads (QwenLM#7206). Add a cross-platform open helper that uses the kernel flag where present and otherwise compensates with an lstat -> open -> fstat identity check, refusing symlinked paths, identity races, and zero-inode filesystems (fail-closed, matching QwenLM#8290/QwenLM#9857). Route the confirmed no-follow read call sites through it: validated @-file reads, session metadata reads, background-shell output tails, untracked diff line counts, the workspace registration store, and session-artifact workspace status. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR — re-run after the merge of Template looks good ✓ Problem: this is an observed defect, not theoretical hardening. Issue #8227 documents that on Windows Direction: aligned. This is the claim-1 compensating control for the hardening already accepted in #7206, and it reuses the fail-closed posture on Size: core paths touched (core + cli + acp-bridge) — 376 production lines vs. 938 test lines, plus 27 lines of subpath wiring (tsconfigs, vitest aliases, package.json exports) at the current head. Well under any advisory threshold. Author is a maintainer. Approach: the scope still feels right after the iteration. Six confirmed read call sites converge on one shared helper instead of duplicating the lstat → open → fstat fallback; the redundant per-site flag helpers are deleted in the same pass; write paths and platform-guarded sites stay deferred per the issue thread. The merge of Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明感谢贡献！这是在合入 模板完整 ✓ 问题：这是已观测到的缺陷，不是理论性加固。Issue #8227 记录了 Windows 上 方向：对齐。这是 #7206 已接受加固的 claim 1 补偿控制，并复用了 #8290/#9857 已确立的 规模：触及核心路径（core + cli + acp-bridge）——当前 head 上 376 行生产代码、938 行测试代码，另有 27 行子路径接线（tsconfig、vitest 别名、package.json exports）。远低于任何提醒阈值。作者是维护者。 方案：迭代之后范围依然合理。六个已确认的读取调用点收敛到一个共享助手，而不是复制 lstat → open → fstat 回退逻辑；各调用点多余的标志位助手在同一改动中删除；写路径和平台守卫调用点按 issue 线程留给后续。合入 风险：无升级风险信号——改动文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Re-run on Merge of main — conflict resolution reviewThe only real conflict was
Code review (carried over — code unchanged since the last pass)The implementation matches my independent proposal for this problem — one shared helper, kernel flag where available, lstat → open → fstat identity fallback otherwise — and the iteration added exactly what the review rounds asked for:
No blockers found. Standing non-blocking nits: on Windows, Files changed (22 of 22 shown)
Testing evidence (the PR's own CI via API — no PR code was executed in this review)CI has now run on the reviewed commit and is fully green: all four
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification is now unblocked — with the conflict resolved, Real-scenario testing: N/A — no user-visible behavior change (defense-in-depth on internal read paths), and this is an unattended run. Not verified here: the author's local test-run numbers (770 core tests etc.) — attributed to the author, not re-run; Windows/macOS runtime behavior — those lanes are skipped in CI (issue claim 3, follow-up scope). 中文说明合入 main —— 冲突解决审查本轮在 唯一的真实冲突在
代码审查（承接上一轮——代码未变）实现与我对这个问题的独立方案一致——一个共享助手，平台支持时走内核标志，否则用 lstat → open → fstat 身份回退——迭代恰好补齐了评审轮次要求的内容：
未发现阻塞项。遗留的非阻塞小问题不变：Windows 上 测试证据（通过 API 读取本 PR 自己的 CI——本审查未执行任何 PR 代码）被审查的提交上现在已跑过 CI 且全绿： CI 表格见上方标记区域。 沙箱验证现在已解除阻塞——冲突解决后 真实场景测试：N/A——无用户可见行为变化（内部读取路径的纵深防御），且本次为无人值守运行。 此处未验证：作者本地测试数字（770 个 core 测试等）——转述作者说法，未重跑；Windows/macOS 运行时行为——这些通道在 CI 中被跳过（issue claim 3，后续范围）。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — approving. Both preconditions that held this PR at 4/5 last pass are now cleared: the conflict with Stepping back: this is what a good compensating-control PR looks like after iteration converges. The problem is documented in #8227, observable in the base code's own The merge of Housekeeping: my approval below supersedes this bot's own two stale change-request reviews from the earlier automated rounds — same account, and GitHub counts the latest review per reviewer; every finding they carried was resolved as a thread and the code has been fully re-reviewed at this head. The remaining merge gate is the second human approval Optional, not gating: 中文说明置信度：4/5 —— 批准合入。上一轮让本 PR 停在 4/5 的两个前置条件现已全部解除：与 退一步看：这是一个优秀的补偿控制 PR 在迭代收敛之后该有的样子。问题记录在 #8227，在 base 代码自己的 合入 事务性说明：下方的批准取代本机器人在更早自动评审轮次中的两个过期 request-changes 评审——同一账号，GitHub 只计每个评审者的最新评审；其中的每条发现都已作为线程解决，且代码已在当前 head 上被完整重新审查。剩余的合入门槛是 可选项（不作为门槛）： — Qwen Code · qwen3.8-max Reviewed at |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": none — no check was cut short..
Test Plan (not a blocker): src/utils/sessionStorageUtils.test.ts — no such file or directory; src/services/backgroundShellRegistry.test.ts — no such file or directory; src/utils/no-follow-open.test.ts — no such file or directory; 26 passed — this review observed 1702, 23964, 21483, 1659, 601, 4226, 627 passed.
中文说明
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 1)"：none — no check was cut short.。
Test Plan（非阻断）：src/utils/sessionStorageUtils.test.ts — no such file or directory; src/services/backgroundShellRegistry.test.ts — no such file or directory; src/utils/no-follow-open.test.ts — no such file or directory; 26 passed — this review observed 1702, 23964, 21483, 1659, 601, 4226, 627 passed。
— qwen3.8-max via Qwen Code /review (v0.22.0)
openSyncNoFollow bound node:fs through a namespace import, which vitest resolves to its own copy of the externalized CJS module. Suites that spy the fs object — sessionService.rename.test.ts stubs openSync/readSync for fabricated session paths — never intercept that copy, so the open threw on the mocked paths and the catch-all reported "no title" (10 of 19 tests red, the CI Test failure). Take the fs binding through the default import the way the callers' suites spy it, teach the doMock factories to carry the stub on the default binding too, and stub lstatSync/fstatSync in the rename suite so the Windows lstat -> open -> fstat fallback accepts the fabricated paths as well. Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
The fallback's inode-0 fail-closed refusal carried code 'ELOOP' like a genuine symlink refusal, so consumers with ELOOP-specific handling misfired on LEGITIMATE files on inode-0 volumes (Windows FAT/exFAT/SMB, where Node reports ino 0): session-artifact workspace status flagged a contained file as an escape, the workspace registration store reported a regular store as "must be a regular file", and untracked text files rendered as binary with dropped hunks. Give the inode-unverifiable refusal its own code (EUNVERIFIABLE) plus an isUnverifiableIdentityError guard, keep ELOOP for genuine symlink refusals and identity races, and adjust the three consumers: the artifact status degrades to plain 'missing', the store surfaces an identity-unverifiable error, and the untracked diff read falls back to the pre-QwenLM#8227 plain read (its lstat gate already rejected symlinks and non-regular files). Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
…opens No call site and no test ever passes flags or mode — repo-wide, every invocation opens by path alone — yet the fs.open-shaped signature was exported through the core index and invited write/create flags through a helper whose docs, lstat -> open -> fstat semantics, and tests cover only the read-only case (O_WRONLY | O_CREAT would create on POSIX while the Windows fallback's pre-open lstat throws ENOENT for the same input). Hardcode the read-only base flags and delete resolveBaseFlags; a PR that first needs more can re-add them with a caller and tests. Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
readLastJsonStringFieldsSync was rerouted through the no-follow helper in the same pass as the single-field variant, but only the latter got a symlink-refusal test — reverting the plural open to a plain fs.openSync kept the whole suite green. Mirror the Windows-flag-set symlinked-session test for the plural variant, asserting the all-undefined empty result; the test is red on that mutant (leaked-secret surfaces) and green on the restored code. Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
|
Closeout — cap-4 round on the 12:31Z review (single non-force push
Verification: rename suite 19/19, no-follow-open 10/10, sessionStorageUtils 55/55, 411 passed across 6 directly-hit files, cli workspace-registration-store 26 passed; typecheck clean core/cli/acp-bridge; eslint+prettier clean. Not run locally: gitDiff fixture tests (host git 2.19 lacks |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review of PR #10007 — fix(core): keep no-follow reads protected where O_NOFOLLOW is missing
Reviewed at: 47d0757 (HEAD), 13 files, +748/−81
Verdict: no blocking issues found.
Summary
This PR adds a cross-platform openNoFollow/openSyncNoFollow helper to replace the collapsing idiom (O_RDONLY ?? 0) | (O_NOFOLLOW ?? 0) used across six read call sites. On POSIX the helper ORs the same kernel flag — byte-for-byte unchanged. On Windows, where O_NOFOLLOW is undefined, the helper compensates with an lstat → open → fstat identity check that refuses to follow symlinks. Well-scoped, well-tested, and correct.
What I verified
-
The fallback is correct and fail-closed.
lstatrefuses a symlinked final component; the post-openfstatmust match dev/ino;ino: 0volumes are refused throughhasVerifiableInode. Each refusal closes the handle and carries eitherELOOP(symlink/race) orEUNVERIFIABLE(inode-0) so callers can distinguish. -
Caller error semantics survive.
workspace-registration-storekeeps its ENOENT → empty path;gitDiffandbackgroundShellRegistrykeep their fail-safe catches;readManyFilesandsessionArtifactskeep their validated-identity re-checks. -
POSIX is untouched. Where
O_NOFOLLOWexists the helper ORs the same flag — no behavioral change. The lazyfs.constants?.O_NOFOLLOWaccess also keeps strict vitest mocks loadable. -
Scope is disciplined. Six confirmed read sites converge; four redundant per-site flag helpers are deleted; write paths and platform-guarded sites are explicitly deferred per the issue thread.
-
Test coverage is thorough. Two reproduction tests (session metadata leak, background-shell output leak) that fail on
mainand pass here. The newno-follow-open.test.tscovers the native path (open, ELOOP, ENOENT), the fallback path (symlink, identity mismatch, inode-0), and the sync/async variants.
Minor observations (none blocking)
- The
openUntrackedForDiffReadtwo-phase fallback (tryopenNoFollow→ if EUNVERIFIABLE → plainopen()) ingitDiff.tshas no dedicated test for the inode-0 degradation path. The behavior is correct and the existinggitDiff.test.tscovers the normal path, but the fallback is untested. - The
readManyFiles.tschanges don't explicitly handleEUNVERIFIABLEfromopenNoFollow— on inode-0 volumes the error propagates through existing error handling with a technical error message. This is consistent with theino: 0posture from #8290/#9857. - The
sessionStorageUtils.test.tssingle-field and multi-field reproduction tests have nearly identical setup code. Minor style concern.
Conclusion
Clean, focused fix for a real Windows security gap. No regressions on POSIX. The two reproduction tests are real evidence that the fix works. Ready to merge once CI on the latest commit (47d0757) lands green.
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Two-phase code review summary (round 1 only)
PR: #10007 — fix(core): keep no-follow reads protected where O_NOFOLLOW is missing
Head reviewed: 47d0757fb2e0ed2d9bec9b485d74e603142f2ec8
Round 1 model: deepseek-v4-flash
Round 2: skipped because round 1 reported findings
Verdict
No blocking issues, but 3 minor observations were raised:
- Untested inode-0 degradation path in
gitDiff.ts— theEUNVERIFIABLEbranch whenhasVerifiableInodeis false lacks direct test coverage. EUNVERIFIABLEpropagation inreadManyFiles.ts— confirm that callers downstream of the text-handle read correctly distinguishEUNVERIFIABLEfromELOOPso legitimate files on FAT/exFAT/SMB volumes are not misclassified as symlink attacks.- Test setup duplication — the symlink-stubbing pattern is repeated across new tests; consider a shared helper.
Notes
- The
openNoFollow/openSyncNoFollowhelper design looks correct: fail-closed on Windows, byte-for-byte unchanged on POSIX. - All six converged call sites route through the helper, and four redundant per-site flag helpers are removed.
- Caller error semantics (
ENOENT→ empty,ELOOP→ binary-row/<error>) are preserved. - The full review pipeline could not run due to a network failure (
getaddrinfo() thread failed to starton git operations), so this pass was performed from the downloaded diff and relevant source files.
The async fallback's TOCTOU identity re-check in openNoFollow (assertSameIdentity plus close-on-rejection) was pinned by no test: deleting the whole try/catch block kept the suite green, because the async symlink tests reject at the earlier isSymbolicLink() check and only the sync variant's re-check was driven. A refactor dropping that block would ship green while a path swapped for a symlink between lstat and open gets read through on Windows and the rejection-path handle leaks unclosed. Add the async identity-change test using the same prototype trick as the sync one, applied to fs.promises.lstat (the real opened FileHandle's stat() cannot be intercepted through fs mocks): the doctored before-stats carry ino + 1, so the real handle's stat mismatches and the open must reject with code ELOOP after closing the handle (asserted through a close spy). The test is red on the mutant (block deleted: 1 failed | 10 passed) and green on restored code (11 passed). Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
|
Closeout round: cleared the last unresolved finding (deferred S2).
|
Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
The Ubuntu failure was PR-caused, not an infra flake: Fixed in |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): src/utils/sessionStorageUtils.test.ts — no such file or directory; src/services/backgroundShellRegistry.test.ts — no such file or directory; src/utils/no-follow-open.test.ts — no such file or directory; 26 passed — this review observed 1702, 20501, 21495, 1659, 601, 4226, 627 passed.
中文说明
Test Plan（非阻断）：src/utils/sessionStorageUtils.test.ts — no such file or directory; src/services/backgroundShellRegistry.test.ts — no such file or directory; src/utils/no-follow-open.test.ts — no such file or directory; 26 passed — this review observed 1702, 20501, 21495, 1659, 601, 4226, 627 passed。
— qwen3.8-max via Qwen Code /review (v0.22.0)
Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
Both integration-tests and acp-bridge import the new @qwen-code/qwen-code-core/noFollowOpen subpath but had no paths entry for it, so typecheck resolved it through core's pre-built dist and failed with TS2307 on dist-less trees. Add the source entries next to the sibling subpaths, following the documented 'keep in sync with the exports maps' invariant. Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
Three pins for the no-follow fallback's guarantee surface: - spy fs.closeSync in both sync rejection tests (identity change, inode 0) so a dropped close in openSyncNoFollow's catch turns red, mirroring the async closeSpy pin - add a dev-mismatch identity variant so deleting the dev comparison from assertSameIdentity turns red (ino alone is unique per device) - add sync+async tests that perturb every lstat after the first call and expect the open to SUCCEED, pinning that the identity re-check compares against the pre-open lstat snapshot rather than a fresh post-open lstat Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Review of PR #10007 — fix(core): keep no-follow reads protected where O_NOFOLLOW is missing
Reviewed at: e2b59655 (HEAD), 19 files, +1029/−81
Previous rounds: 2 rounds (CHANGES_REQUESTED twice), 15 findings posted (3 Criticals, 12 Suggestions); this re-review at commit e2b59655
Previous findings — status
3 Criticals — all resolved:
- R1-1 (sessionStorageUtils.ts rerouting breaks sessionService.rename.test.ts): fixed in
bdecd4c8— lstatSync/fstatSync spies added to the rename suite; default-import binding ensures vitest spies intercept the helper. ✓ - R1-2 (inode-0 carries ELOOP, misclassifying legitimate files): fixed in
84df828c—UNVERIFIABLE_IDENTITY_CODE(EUNVERIFIABLE) replacesELOOPfor inode-0 refusals; all three consumers (sessionArtifacts, workspace-registration-store, gitDiff) distinguish the two codes. ✓ - R2-1 (cli vitest config missing subpath alias, breaking 63 tests): fixed in
d6e4a63a— alias entries in both cli/vitest.config.ts and acp-bridge/vitest.config.ts. ✓
12 Suggestions — 10 resolved, 2 standing (see below):
- R1-3 (flags/mode params): doc comment clarifies read-only-only. ✓
- R1-4 (async TOCTOU re-check): tested in no-follow-open.test.ts. ✓
- R1-5 (multi-field symlink refusal): tested in sessionStorageUtils.test.ts. ✓
- R2-2 (default-import mockability): test factories set
default: modified. ✓ - R2-3 (sessionArtifacts EUNVERIFIABLE branch untested): still standing — no test exercises the isUnverifiableIdentityError branch in sessionArtifacts.ts.
- R2-4 (cli workspace-registration-store EUNVERIFIABLE branch untested): now tested (
"reports an unverifiable store identity as a store error"). ✓ - R2-5 (gitDiff inode-0 degradation path untested): still standing — the openUntrackedForDiffRead fallback to plain open() on EUNVERIFIABLE has no test in gitDiff.test.ts.
- R2-6 (sync rejection-path fd close unpinned): now tested (closeSpy). ✓
- R2-7/R2-8 (subpath export config): package.json export entry + tsconfig/vitest aliases all present. ✓
- R2-9 (dev identity check untested): now tested. ✓
- R2-10 (pre-open snapshot comparison untested): now tested. ✓
My review — no new issues found
I reviewed the full diff (1421 lines across 19 files) and the prior review threads. The implementation is correct and well-tested. Key observations:
1. Helper design is correct. The openNoFollow/openSyncNoFollow functions are cleanly split between the POSIX fast path (kernel O_NOFOLLOW — byte-for-byte unchanged) and the fallback path (lstat → open → fstat identity check). The fallback correctly:
- Refuses symlinked final components via pre-open lstat
- Refuses dev/ino identity mismatches (TOCTOU swap race)
- Refuses inode-0 volumes via hasVerifiableInode with a distinct error code (EUNVERIFIABLE)
2. Error code separation is consistent. EUNVERIFIABLE is used ONLY for the inode-0 case; all genuine symlink/race refusals carry ELOOP. All three consumers that distinguish them (sessionArtifacts.ts, workspace-registration-store.ts, gitDiff.ts) use the correct code checks. The remaining three consumers (sessionStorageUtils.ts, backgroundShellRegistry.ts, readManyFiles.ts) do not need to distinguish — their catch-all handling is correct for either code.
3. Cross-package integration is complete. The four subpath export entries (package.json, both vitest.config.ts files, acp-bridge tsconfig.json, integration-tests tsconfig.json) are all present. The serve-fast-path-bundle-check.test.js confirms the leaf import does not pull the core barrel.
4. Test coverage is thorough. The no-follow-open.test.ts (468 lines) covers both the native and fallback paths with all error variants. The reproduction tests in sessionStorageUtils.test.ts and backgroundShellRegistry.test.ts prove the fix works by failing on main and passing here. The sessionService.rename.test.ts compatibility test confirms the helper does not break existing mock patterns.
5. Two minor Suggestions remain standing (from previous rounds, not new):
- R2-3 (Suggestion): sessionArtifacts.ts isUnverifiableIdentityError branch is untested. The sessionArtifacts.test.ts file is not in this PR's diff. Adding a test there would require setting up O_NOFOLLOW=undefined + ino: 0 in the acp-bridge test suite.
- R2-5 (Suggestion): gitDiff.ts openUntrackedForDiffRead inode-0 degradation path is untested in gitDiff.test.ts. The fallback behavior (plain open() on EUNVERIFIABLE) is correct but has no dedicated test.
Verdict
No blocking issues. The 3 Criticals from previous rounds are all verified fixed at this HEAD. The 2 remaining Suggestions are minor and non-blocking. The implementation is correct, well-tested, and consistent with the established #8290/#9857 posture. Ready to merge (pending CI green).
— Independent review (no worktree: git fetch blocked on Windows; reviewed from full diff, 15 prior finding threads, and PR context)
…gitDiff Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/utils/sessionStorageUtils.test.ts — no such file or directory; src/services/backgroundShellRegistry.test.ts — no such file or directory; src/utils/no-follow-open.test.ts — no such file or directory; 26 passed — this review observed 1704, 24732, 21627, 1659, 601, 4235, 630 passed.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/utils/gitDiff.ts:92 — [review] openUntrackedForDiffRead doc comment overclaims the symlink guarantee (code unchanged since the previous round)packages/core/src/tools/readManyFiles.ts:303 — [probe] readManyFiles open-side guard has no paired test (code unchanged since the previous round)packages/core/src/utils/no-follow-open.test.ts:285 — [probe] async identity-change test does not pin the fd-based re-check (code unchanged since the previous round)
Convergence: round 3 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 10 (10 new). Findings keep coming back to the same files: packages/core/src/utils/no-follow-open.test.ts (findings in round 2; 2 more now); packages/cli/src/serve/workspace-registration-store.ts (findings in round 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/utils/sessionStorageUtils.test.ts — no such file or directory; src/services/backgroundShellRegistry.test.ts — no such file or directory; src/utils/no-follow-open.test.ts — no such file or directory; 26 passed — this review observed 1704, 24732, 21627, 1659, 601, 4235, 630 passed。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 3 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 3 轮发布了 3 条行内评论，其中 3 条是首次提出；上一轮发布了 10 条（其中 10 条首次提出）。发现反复回到同一批文件：packages/core/src/utils/no-follow-open.test.ts（第 2 轮已出过发现，本轮又有 2 条）；packages/cli/src/serve/workspace-registration-store.ts（第 2 轮已出过发现，本轮又有 1 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.0)
The ino-mismatch and dev-mismatch identity-change tests never plant a symlink: they write a plain file and perturb fstatSync via vi.doMock. The itNoSymlink guard therefore only skipped them on win32, the one platform where the lstat/open/fstat fallback is the production path. Switch both to plain it, matching the async twin. Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
packages/cli imports @qwen-code/qwen-code-core/noFollowOpen in workspace-registration-store.ts but had no paths entry for the leaf, so cli typecheck resolved it through core's compiled dist and breaks in deep-cleaned worktrees. Mirror the entry already added for acp-bridge and integration-tests. Co-authored-by: Qwen-Coder <qwen-coder @alibabacloud.com>
|
CI note —
|
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI; the fallback tests un-skipped this round ran only on Linux.
Test Plan (not a blocker): src/utils/sessionStorageUtils.test.ts — no such file or directory; src/services/backgroundShellRegistry.test.ts — no such file or directory; src/utils/no-follow-open.test.ts — no such file or directory; 26 passed — this review observed 24739, 21627, 1704, 1659, 601, 4235, 630 passed.
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/core/src/utils/no-follow-open.test.ts:67 — [probe] helper's read-only open flags unpinned by any testpackages/core/src/utils/no-follow-open.test.ts:445 — [probe] isUnverifiableIdentityError tested only on its true branch
Convergence: round 4 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 3 (3 new). Findings keep coming back to the same files: packages/core/src/utils/no-follow-open.test.ts (findings in round 3; 2 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI; the fallback tests un-skipped this round ran only on Linux。
Test Plan（非阻断）：src/utils/sessionStorageUtils.test.ts — no such file or directory; src/services/backgroundShellRegistry.test.ts — no such file or directory; src/utils/no-follow-open.test.ts — no such file or directory; 26 passed — this review observed 24739, 21627, 1704, 1659, 601, 4235, 630 passed。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 2 条（原文未翻译，列表见上方英文部分）。
收敛情况：第 4 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 3 条（其中 3 条首次提出）。发现反复回到同一批文件：packages/core/src/utils/no-follow-open.test.ts（第 3 轮已出过发现，本轮又有 2 条）。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
Extract the repeated O_NOFOLLOW-less node:fs mock skeleton into a shared mockNoFollowFs factory (the load-bearing `default` member stays) plus a perturbedStats helper, and drop the six per-test doUnmock/resetModules blocks already covered by the describe-level afterEach. Add sync, async, and inode-0 variants asserting a failing rejection-path close never masks the pinned ELOOP / EUNVERIFIABLE refusal codes. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
@qwen-code /triage |
|
Sandboxed verification: Skipped because the PR has merge conflicts, so refs/pull/10007/merge is unavailable — resolve conflicts and re-run. 中文 — 判定：
|
|
@qwen-code /triage |
|
Sandboxed verification: Skipped because the PR has merge conflicts, so refs/pull/10007/merge is unavailable — resolve conflicts and re-run. 中文 — 判定：
|
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
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 explored to full depth (tool budget reached): "agent 6c": could not execute src/utils/no-follow-open.test.ts — neither the review worktree nor the parent checkout has node_modules (vitest fails at config load with …; "agent 3b": executing packages/core/src/utils/no-follow-open.test.ts — the review worktree has no node_modules , and npm ci + npm run build to enable it exceeds the …; "agent 6a": executed npx vitest run src/utils/no-follow-open.test.ts at HEAD — worktree has no node_modules and a full monorepo install in the shared review tree was not ….
Test Plan (not a blocker): src/utils/sessionStorageUtils.test.ts — no such file or directory; src/services/backgroundShellRegistry.test.ts — no such file or directory; src/utils/no-follow-open.test.ts — no such file or directory; 26 passed — this review observed 21630, 1704, 24740, 1659, 601, 4235, 630 passed.
Convergence: round 5 posted 2 inline comment(s), 2 of them reported for the first time; the previous round posted 2 (2 new). Findings keep coming back to the same files: packages/core/src/utils/no-follow-open.test.ts (findings in round 4; 2 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. No Critical finding is open on this round, so merging and moving the remaining Suggestion threads to a follow-up issue is available as an ending — a merged pull request cannot diverge further. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 6c"：could not execute src/utils/no-follow-open.test.ts — neither the review worktree nor the parent checkout has node_modules (vitest fails at config load with …；"agent 3b"：executing packages/core/src/utils/no-follow-open.test.ts — the review worktree has no node_modules , and npm ci + npm run build to enable it exceeds the …；"agent 6a"：executed npx vitest run src/utils/no-follow-open.test.ts at HEAD — worktree has no node_modules and a full monorepo install in the shared review tree was not …。
Test Plan（非阻断）：src/utils/sessionStorageUtils.test.ts — no such file or directory; src/services/backgroundShellRegistry.test.ts — no such file or directory; src/utils/no-follow-open.test.ts — no such file or directory; 26 passed — this review observed 21630, 1704, 24740, 1659, 601, 4235, 630 passed。
收敛情况：第 5 轮发布了 2 条行内评论，其中 2 条是首次提出；上一轮发布了 2 条（其中 2 条首次提出）。发现反复回到同一批文件：packages/core/src/utils/no-follow-open.test.ts（第 4 轮已出过发现，本轮又有 2 条）。新发现的产出速度没有下降。一个不断再生兄弟发现的簇，通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因，或把独立的簇拆成单独的 PR，通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送，或将本 PR 的评审降到 --severity-floor critical，可以避免循环反复推导同一组发现。本轮没有未决的 Critical，因此"合入后把剩余 Suggestion 线程转到后续 issue"是一个可选的结束方式——已合入的 PR 不会继续发散。（仅为观察——本轮评审未因此扣留任何内容。）
— qwen3.8-max via Qwen Code /review (v0.22.2)
|
@qwen-code /triage |
|
Sandboxed verification: Skipped because the PR has merge conflicts, so refs/pull/10007/merge is unavailable — resolve conflicts and re-run. 中文 — 判定：
|
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
Merge conflict with Conflict: Verification at the merge commit:
Mergeable: now Note: |
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. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI; the fallback path is exercised on Linux only via stubbed constants (the PR's documented simulation).
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/utils/sessionStorageUtils.test.ts — no such file or directory; src/services/backgroundShellRegistry.test.ts — no such file or directory; src/utils/no-follow-open.test.ts — no such file or directory.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/core/src/utils/gitDiff.ts:14 — [review] Deferral inventory mislabels gitUtils.ts readFirstLineNoFollow — a read site with the collapsing idiom — as a write pathpackages/core/src/tools/readManyFiles.ts:303 — [probe] readManyFiles open reroute has no test pinning the openNoFollow open (plain-open regression ships green)integration-tests/tsconfig.json:48 — [test] noFollowOpen paths mapping imported by no integration test — dead configpackages/core/src/utils/no-follow-open.test.ts:352 — [probe] async inode-0 fail-closed refusal unpinned (async openNoFollow has no EUNVERIFIABLE witness)packages/core/src/index.ts:154 — [review] barrel re-exports of the four no-follow symbols have zero read sites — dead API surfacepackages/core/src/utils/no-follow-open.test.ts:188 — [probe] fallback's lstat-before-open ordering unpinned (open-first mutant passes all 12 tests)packages/core/src/utils/no-follow-open.test.ts:73 — [review] R5-1 still stands — O_NOFOLLOW-stripped fs mock copied five times across two packages (deferred; main-repo follow-up acknowledged)packages/core/src/utils/no-follow-open.test.ts:475 — [review] R4-2 still stands — best-effort-close tests re-paste the deduplicated perturbation blocks (deferred; main-repo follow-up acknowledged)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Test (windows-latest, Node 22.x) and Test (macos-latest, Node 22.x) were skipped in CI; the fallback path is exercised on Linux only via stubbed constants (the PR's documented simulation)。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan（非阻断）：src/utils/sessionStorageUtils.test.ts — no such file or directory; src/services/backgroundShellRegistry.test.ts — no such file or directory; src/utils/no-follow-open.test.ts — no such file or directory。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 8 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.22.2)
4 participants
What this PR does
Adds a cross-platform "open without following symlinks" helper (
openNoFollow/openSyncNoFollowinpackages/core/src/utils/no-follow-open.ts) and routes the confirmedO_NOFOLLOWread call sites through it. On platforms that exposeO_NOFOLLOWthe helper simply ORs the kernel flag into the open flags — behavior is byte-for-byte unchanged. Where the constant does not exist (Windows:fs.constants.O_NOFOLLOWisundefined), the previous(O_RDONLY | (O_NOFOLLOW ?? 0))expressions silently collapsed into a plain open that follows symlinks; the helper instead compensates with anlstat→open→fstatidentity check that refuses symlinked final components, refuses opens whose dev/ino no longer matches the pre-openlstat(the swap race), and refuses filesystems that reportino: 0because identity cannot be proven there. Every refusal carriescode: 'ELOOP'so existing caller error handling applies unchanged.Converged call sites: the validated
@-file read path (readManyFiles.ts, both the text-handle read and the snapshot), session metadata reads (sessionStorageUtils.ts), background-shell output tails (backgroundShellRegistry.ts), untracked diff line counting and hunk synthesis (gitDiff.ts), the workspace registration store (cli/serve/workspace-registration-store.ts), and session-artifact workspace status (acp-bridge/sessionArtifacts.ts).Why it's needed
PR #7206 hardened
@-referenced file reads with symlink/TOCTOU protection, but on Windows that protection materially disappears becauseO_NOFOLLOWisundefinedand the?? 0fallback drops the guarantee (issue #8227, claim 1). With the constant stubbed away (the Windows flag set), a symlink planted over a session file redirected the metadata read to the link target, and a symlink planted over a background-shell output file leaked its content into model context — both reproduced as red tests in this PR before the fix. Claim 2 of the issue (vacuous dev/ino checks) was already closed by #8290 and #9857; the fail-closed posture onino: 0established there is reused here viahasVerifiableInode.Reviewer Test Plan
How to verify
Two new reproduction tests stub
fs.constants.O_NOFOLLOWtoundefined(the same seamsession-start-profiler.test.tsalready uses) and plant a symlink at the read path; they fail onmainand pass with this PR:sessionStorageUtils ... expected undefined, received 'leaked-secret'andBackgroundShellRegistry ... expected '<task-notification>...' not to contain 'secret credentials'.no-follow-open.test.tsadditionally pins the native path (regular file opens, symlink →ELOOP, ENOENT passthrough) and the fallback path (symlink refusal, identity-mismatch refusal,ino: 0refusal).Broader targeted suites, all green:
gitDiff.test.ts(124),readManyFiles.test.ts,sessionService.test.ts,session-transcript-reader.test.ts,gitDirect.test.ts,session-start-profiler.test.ts,fileReadCache.test.ts— 770 tests across 10 core files;cliworkspace-registration-store.test.ts(26 passed / 1 skipped);acp-bridgesessionArtifacts.test.ts(126).tsc --noEmitclean forcore,cli, andacp-bridge; ESLint clean on all changed files.Evidence (Before & After)
N/A — no user-visible UI change (defense-in-depth on internal read paths).
Tested on
Environment (optional)
Unit tests only (
vitest), Node v24.19.0.Risk & Scope
O_NOFOLLOWthe fallback adds onelstat+ onefstatper open (low-frequency read paths only), and reads onino: 0volumes (FAT/exFAT, some SMB) now fail closed instead of opening without any no-follow guarantee — consistent with the posture decided in fix(core): fail closed on zero inode file cache #8290/fix(core): reject unverifiable validated read inodes #9857. POSIX behavior is unchanged (same kernel flag as before).skipIf(process.platform === 'win32')tests) remains separate follow-up scope, as in fix(core): reject unverifiable validated read inodes #9857. OtherO_NOFOLLOWcall sites not confirmed in the issue thread (write paths such asskill-args-file.ts,gitUtils.ts,skill-curator.ts,session-writer-lease.ts; platform-guarded sites insession-start-profiler.ts/sessionService.ts; typeof-guardedvoice-keyterms.ts/customBanner.ts) are intentionally left for a follow-up.Linked Issues
Refs #8227 (claim 1 compensating control; claim 3 remains follow-up). Claim 2 was closed by #8290 and #9857. Builds on the hardening introduced by #7206.
中文说明
本 PR 做了什么
新增跨平台"打开但不跟随符号链接"助手函数（
packages/core/src/utils/no-follow-open.ts中的openNoFollow/openSyncNoFollow），并把已确认的O_NOFOLLOW读取调用点统一收敛过去。在提供O_NOFOLLOW的平台上，助手只是把内核标志按位或进打开标志——行为与之前完全一致。在该常量不存在的平台（Windows 上fs.constants.O_NOFOLLOW为undefined），之前(O_RDONLY | (O_NOFOLLOW ?? 0))的写法会静默塌缩成跟随符号链接的普通 open；助手改用lstat→open→fstat身份校验来补偿：拒绝末位成分是符号链接的路径，拒绝打开后 dev/ino 与打开前lstat不一致的情况（即替换竞态），并拒绝报告ino: 0的文件系统（因为无法证明文件身份）。所有拒绝都带code: 'ELOOP'，调用方现有的错误处理无需改动即可生效。收敛的调用点：
@文件校验读路径（readManyFiles.ts的文本句柄读取与快照两处）、会话元数据读取（sessionStorageUtils.ts）、后台 shell 输出尾部（backgroundShellRegistry.ts）、untracked diff 行数统计与 hunk 合成（gitDiff.ts）、工作区注册存储（cli/serve/workspace-registration-store.ts）、会话产物工作区状态（acp-bridge/sessionArtifacts.ts）。为什么需要
PR #7206 为
@引用文件读取加了符号链接/TOCTOU 防护，但在 Windows 上这层防护实质失效：O_NOFOLLOW为undefined，?? 0回退把保证丢掉了（issue #8227 的 claim 1）。把该常量 stub 掉（即 Windows 的标志集）后可以复现：在会话文件上植入符号链接，元数据读取会被重定向到链接目标；在后台 shell 输出文件上植入符号链接，其内容会泄漏进模型上下文——两者都作为红测试包含在本 PR 中，修复前失败、修复后通过。issue 的 claim 2（dev/ino 空检）已由 #8290 和 #9857 关闭；本 PR 复用它们确立的ino: 0fail-closed 立场（hasVerifiableInode）。审阅者测试计划
如何验证
两个新的复现测试把
fs.constants.O_NOFOLLOWstub 成undefined（与session-start-profiler.test.ts已有的测试接缝相同），并在读取路径上植入符号链接；它们在main上失败，在本 PR 上通过：sessionStorageUtils ... expected undefined, received 'leaked-secret'以及BackgroundShellRegistry ... expected '<task-notification>...' not to contain 'secret credentials'。no-follow-open.test.ts还固化了原生路径（正常文件可打开、符号链接 →ELOOP、ENOENT 透传）和回退路径（符号链接拒绝、身份不一致拒绝、ino: 0拒绝）。更大范围的目标测试套件全部通过：
gitDiff.test.ts（124）、readManyFiles.test.ts、sessionService.test.ts、session-transcript-reader.test.ts、gitDirect.test.ts、session-start-profiler.test.ts、fileReadCache.test.ts——core 共 10 个文件 770 个测试；cli的workspace-registration-store.test.ts（26 通过 / 1 跳过）；acp-bridge的sessionArtifacts.test.ts（126）。tsc --noEmit在core、cli、acp-bridge三个包均通过；所有改动文件 ESLint 通过。证据（前后对比）
N/A——无用户可见 UI 变化（内部读取路径的纵深防御）。
测试环境
运行环境（可选）
仅单元测试（
vitest），Node v24.19.0。风险与范围
O_NOFOLLOW的平台上，回退路径每次 open 多一次lstat+ 一次fstat（仅低频读取路径）；在ino: 0的卷（FAT/exFAT、部分 SMB）上，这些读取现在会 fail closed，而不是在毫无 no-follow 保证的情况下打开——与 fix(core): fail closed on zero inode file cache #8290/fix(core): reject unverifiable validated read inodes #9857 确定的立场一致。POSIX 行为不变（与之前相同的内核标志）。skipIf(process.platform === 'win32')的测试）照 fix(core): reject unverifiable validated read inodes #9857 的做法留作后续单独跟进。未在 issue 线程中确认的其他O_NOFOLLOW调用点（写路径如skill-args-file.ts、gitUtils.ts、skill-curator.ts、session-writer-lease.ts；平台守卫的session-start-profiler.ts/sessionService.ts；typeof 守卫的voice-keyterms.ts/customBanner.ts）有意留给后续 PR。关联 Issue
Refs #8227（claim 1 的补偿控制；claim 3 留作后续）。claim 2 已由 #8290 和 #9857 关闭。建立在 #7206 引入的加固之上。