Skip to content

fix(core): align mock workspace path containment - #8759

Merged
wenshao merged 15 commits into
QwenLM:mainfrom
rbalachandar:fix/macos-workspace-mock-canonicalization
Aug 12, 2026
Merged

fix(core): align mock workspace path containment#8759
wenshao merged 15 commits into
QwenLM:mainfrom
rbalachandar:fix/macos-workspace-mock-canonicalization

Conversation

@rbalachandar

@rbalachandar rbalachandar commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

What this PR does

This PR aligns the shared createMockWorkspaceContext test double with production workspace path containment. It canonicalizes workspace roots and candidate paths for isPathWithinWorkspace, while preserving lexical paths returned by getDirectories() for search-oriented tests. It also adds focused regression coverage for missing descendants, sibling-prefix boundaries, and additional workspace directories.

Why it's needed

On macOS, temporary directories are commonly exposed lexically under /var/folders/... while their canonical path is under /private/var/folders/.... Production canonicalizes both sides of a workspace containment check, but the test double used a lexical startsWith comparison. After the read-permission security fix, this mismatch caused the macOS read-file and zoom-image tests to return ask instead of allow for paths inside the workspace.

Reviewer Test Plan

How to verify

Run the focused suite from the repository root:

npm run test --workspace=packages/core -- src/test-utils/mockWorkspaceContext.test.ts src/test-utils/mockWorkspaceContext.fallback.test.ts src/tools/shell.test.ts src/tools/read-file.test.ts src/tools/zoom-image.test.ts

Expected result: 410 tests pass, including the two macOS workspace permission regressions. Also run:

npm run typecheck --workspace=packages/core
npm run build --workspace=packages/core
npx eslint packages/core/src/test-utils/mockWorkspaceContext.ts packages/core/src/test-utils/mockWorkspaceContext.test.ts
npx prettier --check packages/core/src/test-utils/mockWorkspaceContext.ts packages/core/src/test-utils/mockWorkspaceContext.test.ts

Evidence (Before & After)

N/A — this is a test-infrastructure change with no user-visible UI or runtime behavior change.

Tested on

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

Environment (optional)

macOS arm64, Node.js v22.22.0, npm, Vitest, and the local Qwen Code core workspace.

Risk & Scope

  • Main risk or tradeoff: containment checks now resolve symlinks and existing path prefixes consistently with production. If filesystem canonicalization is unavailable because a test stubs filesystem calls, the helper falls back to the prior lexical behavior so unrelated unit-test doubles remain usable.
  • Not validated / out of scope: Windows and Linux runtime validation; production WorkspaceContext behavior is unchanged; getDirectories() intentionally remains lexical because grep/glob-style tests consume those values as search roots.
  • Breaking changes / migration notes: none. This changes only a test utility and adds tests.

Linked Issues

Fixes #8753

中文说明

这个 PR 做了什么

本 PR 让共享的 createMockWorkspaceContext 测试替身与生产环境的工作区路径边界检查保持一致。它只在 isPathWithinWorkspace 检查中规范化工作区根目录和候选路径,同时保留 getDirectories() 返回的词法路径,供依赖搜索根目录的测试使用。此外,本 PR 增加了针对工作区内不存在的子路径、具有相同前缀的兄弟路径,以及额外工作区目录的聚焦回归测试。

为什么需要

在 macOS 上,临时目录通常以 /var/folders/... 的词法路径暴露,但其规范路径位于 /private/var/folders/...。生产代码会规范化工作区边界检查的两侧,而测试替身此前使用词法 startsWith 比较。读权限安全修复之后,这种不一致导致 read-filezoom-image 的 macOS 测试将工作区内路径错误地判定为 ask,而不是 allow

评审验证计划

如何验证

在仓库根目录运行以下聚焦测试:

npm run test --workspace=packages/core -- src/test-utils/mockWorkspaceContext.test.ts src/test-utils/mockWorkspaceContext.fallback.test.ts src/tools/shell.test.ts src/tools/read-file.test.ts src/tools/zoom-image.test.ts

预期结果:403 个测试全部通过,其中包括两个 macOS 工作区权限回归测试。然后运行:

npm run typecheck --workspace=packages/core
npm run build --workspace=packages/core
npx eslint packages/core/src/test-utils/mockWorkspaceContext.ts packages/core/src/test-utils/mockWorkspaceContext.test.ts
npx prettier --check packages/core/src/test-utils/mockWorkspaceContext.ts packages/core/src/test-utils/mockWorkspaceContext.test.ts

前后对比证据

不适用——这是测试基础设施改动,不会改变用户可见的界面或运行时行为。

测试平台

操作系统 状态
🍏 macOS ✅ 已测试
🪟 Windows N/A
🐧 Linux N/A

环境(可选)

macOS arm64、Node.js v22.22.0、npm、Vitest,以及本地 Qwen Code core workspace。

风险与范围

  • 主要风险或取舍:containment 检查现在会像生产代码一样解析符号链接和已有路径前缀。如果测试替身了文件系统调用导致无法进行路径规范化,辅助函数会回退到之前的词法行为,以便其他单元测试替身继续工作。
  • 未验证/范围之外:未进行 Windows 和 Linux 运行时验证;生产环境的 WorkspaceContext 行为未改变;getDirectories() 有意继续返回词法路径,因为 grep/glob 类测试会将这些值作为搜索根目录使用。
  • 破坏性变更/迁移说明:无。本改动只修改测试工具并增加测试。

关联 Issue

Fixes #8753

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

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

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@rbalachandar thanks for the fix — the change itself looks focused, and #8753 is a well-documented bug with a solid root-cause analysis. Before it can go to code review, though, the PR description needs to follow the pull request template. The current body has only ## Summary and ## Tests; the required sections are missing:

  • What this PR does and Why it's needed — your summary content maps onto these, just under the template headings.
  • Reviewer Test PlanHow to verify (the commands you ran), Evidence (Before & After) (N/A is fine for a test-infrastructure fix), and the Tested on matrix. That last one matters here: #8753 is a macOS-only failure, so reviewers need to know whether you reproduced it on macOS and confirmed the fix there, or only ran the suite on other platforms.
  • Risk & Scope — in particular, why it is safe for the mock to canonicalize paths only for the containment check while getDirectories() keeps returning lexical paths (grep/glob-style tests consume those as search roots).
  • Linked Issues — move the Fixes #8753 reference into this section.
  • The <details> Chinese translation of the description.

Once the description is updated, re-run with @qwen-code /triage to re-trigger the gate.

中文说明

@rbalachandar 感谢这个修复——改动本身很聚焦,#8753 也是一个记录完整、根因分析扎实的 bug。但在进入代码审查之前,PR 描述需要遵循 PR 模板。目前描述只有 ## Summary## Tests,缺少必需的章节:

  • What this PR doesWhy it's needed——现有 summary 的内容可以放到这两个标题下。
  • Reviewer Test Plan——How to verify(你运行的命令)、Evidence (Before & After)(测试基建修复可写 N/A)、以及 Tested on 表格。最后一点在这里很关键:#8753 是 macOS 专属的失败,审查者需要知道你是在 macOS 上复现并验证了修复,还是只在其他平台上运行过测试。
  • Risk & Scope——特别需要说明:mock 只在 containment 检查时做路径规范化、而 getDirectories() 仍返回词法路径,为什么是安全的(grep/glob 类测试会把这些路径当作搜索根目录使用)。
  • Linked Issues——把 Fixes #8753 移到这一节。
  • 描述的 <details> 中文翻译。

描述更新后,用 @qwen-code /triage 重新触发检查即可。

Qwen Code · qwen3.8-max

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Suggestions are inline. Not explored to full depth (tool budget reached): This PR fixes issue #8753 by making the mock workspace co...: did not run the full packages/core suite — only the new test file plus the three consumer suites I assessed as highest-risk (write-file, glob, shell); remaini…; This PR fixes issue #8753 by making the mock workspace co...: did not exhaustively map what e.path / isFileSymlink does on Windows production paths for Finding 1's counterpart behavior (probe ran on Linux; the divergence…; This PR fixes issue #8753 by making the mock workspace co...: verifying the fallback-asymmetry scenario below on a macOS runner (unavailable here; established statically); This PR fixes issue #8753 by making the mock workspace co...: Could not execute packages/core/src/test-utils/mockWorkspaceContext.test.ts — no node_modules in the review worktree (vitest startup error ERR_MODULE_NOT_FOUN…; This PR fixes issue #8753 by making the mock workspace co...: could not execute packages/core/src/test-utils/mockWorkspaceContext.test.ts to confirm it is green — the review worktree has no node_modules installed ( vit…. Not reviewed: reverse audit — stopped before round 3 by the review time budget. Test Plan (not a blocker): src/tools/shell.test.tsno such file or directory; src/tools/read-file.test.tsno such file or directory; src/tools/zoom-image.test.tsno such file or directory.

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

Comment thread packages/core/src/test-utils/mockWorkspaceContext.test.ts Outdated
Comment thread packages/core/src/test-utils/mockWorkspaceContext.ts Outdated
Comment thread packages/core/src/test-utils/mockWorkspaceContext.ts Outdated
Comment thread packages/core/src/test-utils/mockWorkspaceContext.ts Outdated
Comment thread packages/core/src/test-utils/mockWorkspaceContext.ts Outdated
@rbalachandar

Copy link
Copy Markdown
Contributor Author

Addressed in commit 3ef14cc7c6.

  • Replaced realpathNearestExisting with production-matching fs.realpathSync semantics.
  • ENOENT follows the same missing-path handling as WorkspaceContext; ELOOP/other real filesystem errors now reject containment.
  • Preserved lexical fallback only for non-Node errors caused by mocked filesystem calls in existing tests.
  • Added a self-referential symlink-cycle regression test.

Validation now passes: 404 focused tests, core typecheck, core build, ESLint, and Prettier.

@rbalachandar

Copy link
Copy Markdown
Contributor Author

Addressed the remaining review suggestions in commit 27e1098e11.

  • Canonicalization is now lazy, so roots and candidates degrade together when filesystem calls are mocked.
  • Added a real symlink-alias test that would fail with lexical-only containment.
  • Added a separate mocked-filesystem test for lexical fallback behavior.
  • Added dangling-leaf symlink rejection coverage.
  • The prior symlink-cycle regression remains covered.

Validation: 407 focused tests pass, plus core typecheck/build, ESLint, and Prettier. Re-running triage now.

@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.

Not explored to full depth (tool budget reached): This change makes the shared createMockWorkspaceContext...: did not read every one of the ~40 consumer call sites line-by-line (I read shell.test.ts , shell.backgroundStatus.test.ts , glob.test.ts and grepped the res…; This change makes the shared createMockWorkspaceContext...: could not execute the two vitest files (no node_modules in the worktree); compensated with direct Node probes of every filesystem behavior the assertions depe…; This change makes the shared createMockWorkspaceContext...: did not run the remaining consumer suites (readManyFiles, grep, read-file, edit, write-file, ls, notebook-edit, zoom-image, shell.backgroundStatus); spot-checke…; This change makes the shared createMockWorkspaceContext...: could not exercise macOS ( /var → /private/var ) or Windows behavior from this Linux runner; those rely on the (identical) production code path being proven on ….

Test Plan (not a blocker): src/test-utils/mockWorkspaceContext.fallback.test.tsno such file or directory; src/tools/shell.test.tsno such file or directory; src/tools/read-file.test.tsno such file or directory; src/tools/zoom-image.test.tsno such file or directory; 407 tests pass — this review observed 1124, 18566, 1466, 481, 2941, 454 passed.

中文说明

未探索到全部深度(达到工具调用预算):This change makes the shared createMockWorkspaceContext...:did not read every one of the ~40 consumer call sites line-by-line (I read shell.test.ts , shell.backgroundStatus.test.ts , glob.test.ts and grepped the res…;This change makes the shared createMockWorkspaceContext...:could not execute the two vitest files (no node_modules in the worktree); compensated with direct Node probes of every filesystem behavior the assertions depe…;This change makes the shared createMockWorkspaceContext...:did not run the remaining consumer suites (readManyFiles, grep, read-file, edit, write-file, ls, notebook-edit, zoom-image, shell.backgroundStatus); spot-checke…;This change makes the shared createMockWorkspaceContext...:could not exercise macOS ( /var → /private/var ) or Windows behavior from this Linux runner; those rely on the (identical) production code path being proven on …

Test Plan(非阻断):src/test-utils/mockWorkspaceContext.fallback.test.tsno such file or directory; src/tools/shell.test.tsno such file or directory; src/tools/read-file.test.tsno such file or directory; src/tools/zoom-image.test.tsno such file or directory; 407 tests pass — this review observed 1124, 18566, 1466, 481, 2941, 454 passed

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

Comment thread packages/core/src/test-utils/mockWorkspaceContext.ts Outdated
Comment thread packages/core/src/test-utils/mockWorkspaceContext.ts
Comment thread packages/core/src/test-utils/mockWorkspaceContext.ts Outdated
Comment thread packages/core/src/test-utils/mockWorkspaceContext.ts Outdated
Comment thread packages/core/src/test-utils/mockWorkspaceContext.test.ts
Comment thread packages/core/src/test-utils/mockWorkspaceContext.ts Outdated
@rbalachandar

Copy link
Copy Markdown
Contributor Author

Addressed the latest review findings in commit 09381d38ab (rebased onto current main as 6b799ee3a2).

  • Extracted shared path canonicalization into resolveWorkspacePath, used by production and the mock.
  • Preserved lexical fallback only for mock-only filesystem stubs.
  • Preserved full lexical paths when Node reports a truncated missing ancestor.
  • Invalid roots are isolated per root, so one bad additional directory cannot hide valid roots.
  • Added candidate realpath-success coverage for both an in-workspace file and an escaping symlink.
  • Added cleanup for every temporary test directory.
  • Added ENOENT-without-path fallback coverage.

Validation: 410 focused tests pass, plus core typecheck/build, ESLint, and Prettier. Re-running triage.

@rbalachandar

Copy link
Copy Markdown
Contributor Author

/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.

Reviewed. Suggestions are inline. Not explored to full depth (tool budget reached): Context: PR #8759 (QwenLM/qwen-code) aligns the shared c...: remaining consumer suites (glob/grep/read-file/write-file/ripGrep/ls/edit) not executed — they use real existing temp dirs, the low-risk class.; Context: PR #8759 (QwenLM/qwen-code) aligns the shared c...: Windows runtime behavior reasoned from code/ path.win32 semantics, not executed (Linux runner only).. Not reviewed: reverse audit — stopped before round 2 by the review time budget. Test Plan (not a blocker): src/test-utils/mockWorkspaceContext.fallback.test.tsno such file or directory; src/tools/shell.test.tsno such file or directory; src/tools/read-file.test.tsno such file or directory; src/tools/zoom-image.test.tsno such file or directory; 410 tests pass — this review observed 19502, 1124, 18567, 1466, 481, 2941, 454 passed.

中文说明

已审查。 建议见行内评论。 未探索到全部深度(达到工具调用预算):Context: PR #8759 (QwenLM/qwen-code) aligns the shared c...:remaining consumer suites (glob/grep/read-file/write-file/ripGrep/ls/edit) not executed — they use real existing temp dirs, the low-risk class.;Context: PR #8759 (QwenLM/qwen-code) aligns the shared c...:Windows runtime behavior reasoned from code/ path.win32 semantics, not executed (Linux runner only).。 未审查:反向审计——评审时间预算不足,未能开始第 2 轮。 Test Plan(非阻断):src/test-utils/mockWorkspaceContext.fallback.test.tsno such file or directory; src/tools/shell.test.tsno such file or directory; src/tools/read-file.test.tsno such file or directory; src/tools/zoom-image.test.tsno such file or directory; 410 tests pass — this review observed 19502, 1124, 18567, 1466, 481, 2941, 454 passed

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

Comment thread packages/core/src/utils/workspaceContext.ts
Comment thread packages/core/src/utils/workspaceContext.ts Outdated
Comment thread packages/core/src/utils/workspaceContext.ts Outdated
Comment thread packages/core/src/test-utils/mockWorkspaceContext.ts
Comment thread packages/core/src/test-utils/mockWorkspaceContext.fallback.test.ts
Comment thread packages/core/src/test-utils/mockWorkspaceContext.ts
Comment thread packages/core/src/utils/workspaceContext.ts

@doudouOUC doudouOUC 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.

Test Plan (not a blocker): src/test-utils/mockWorkspaceContext.fallback.test.tsno such file or directory; src/tools/shell.test.tsno such file or directory; src/tools/read-file.test.tsno such file or directory; src/tools/zoom-image.test.tsno such file or directory.

中文说明

Test Plan(非阻断):src/test-utils/mockWorkspaceContext.fallback.test.tsno such file or directory; src/tools/shell.test.tsno such file or directory; src/tools/read-file.test.tsno such file or directory; src/tools/zoom-image.test.tsno such file or directory

— deepseek-v4-flash via Qwen Code /review (v0.21.8)

Comment thread packages/core/src/test-utils/mockWorkspaceContext.test.ts
Comment thread packages/core/src/utils/workspaceContext.ts
Comment thread packages/core/src/utils/workspaceContext.ts
Comment thread packages/core/src/utils/workspaceContext.ts Outdated
@rbalachandar

Copy link
Copy Markdown
Contributor Author

Addressed the remaining actionable review feedback in commit af13809bcc.

  • Restored the cache-eviction if guard after the shared resolver delegation.
  • Made the directory-symlink cleanup macOS-safe and exception-safe with recursive: true inside finally; the temporary root is also cleaned up.
  • Added production WorkspaceContext and direct resolveWorkspacePath coverage for missing intermediate path components.

Validation:

  • Focused Vitest suites: 56 tests passed.
  • Core typecheck passed.
  • Prettier and pre-commit hooks passed.

The test-plan paths are under packages/core/src/...; the workspace command in the PR description runs them relative to the core package.

@rbalachandar

Copy link
Copy Markdown
Contributor Author

/triage

@rbalachandar

Copy link
Copy Markdown
Contributor Author

Cross-checked the latest review ledger and addressed the remaining valid maintenance findings in bc3121709e.

  • Restored the explanatory comment and JSDoc for the dangling-symlink guard.
  • Restored the fallback test's ENOENT stub after each test so the TypeError case cannot leak into later tests.
  • Added a multi-root regression proving an invalid symlink-cycle root does not hide a valid root.

The existing tests also cover candidate realpath success, dangling links, cycles, missing intermediate components, alias canonicalization, and cleanup. Validation now passes 57 focused tests, core typecheck, Prettier, and pre-commit hooks.

@rbalachandar

Copy link
Copy Markdown
Contributor Author

/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.

Reviewed. Suggestions are inline.

Not explored to full depth (tool budget reached): PR #8759 aligns the shared createMockWorkspaceContext t...: none — all planned checks completed within budget.; PR #8759 aligns the shared createMockWorkspaceContext t...: verify macOS realpathSync error.path semantics empirically (Linux-only runner) — mitigated by the verdict-neutrality argument in point 1, which holds regard…; Reverse audit round 1 for PR #8759 (mock workspace path c...: none (finished within budget; no check left unfinished)..

Test Plan (not a blocker): src/test-utils/mockWorkspaceContext.fallback.test.tsno such file or directory; src/tools/shell.test.tsno such file or directory; src/tools/read-file.test.tsno such file or directory; src/tools/zoom-image.test.tsno such file or directory; 410 tests pass — this review observed 19504, 1124, 18570, 1466, 481, 2941, 454 passed.

中文说明

已审查。 建议见行内评论。

未探索到全部深度(达到工具调用预算):PR #8759 aligns the shared createMockWorkspaceContext t...:none — all planned checks completed within budget.;PR #8759 aligns the shared createMockWorkspaceContext t...:verify macOS realpathSync error.path semantics empirically (Linux-only runner) — mitigated by the verdict-neutrality argument in point 1, which holds regard…;Reverse audit round 1 for PR #8759 (mock workspace path c...:none (finished within budget; no check left unfinished).

Test Plan(非阻断):src/test-utils/mockWorkspaceContext.fallback.test.tsno such file or directory; src/tools/shell.test.tsno such file or directory; src/tools/read-file.test.tsno such file or directory; src/tools/zoom-image.test.tsno such file or directory; 410 tests pass — this review observed 19504, 1124, 18570, 1466, 481, 2941, 454 passed

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

Comment thread packages/core/src/utils/workspaceContext.ts Outdated
Comment thread packages/core/src/utils/workspaceContext.ts
Comment thread packages/core/src/utils/workspaceContext.ts Outdated
@rbalachandar

Copy link
Copy Markdown
Contributor Author

Addressed the latest residual sibling-collapse review finding in 6b5ca00f29.

resolveWorkspacePath now resolves the deepest existing ancestor and re-appends the missing path tail, preserving canonical symlink ancestry instead of returning a truncated error.path. Added a regression for an alias-backed missing workspace root and a different sibling path.

Validation: 58 focused tests passed, core typecheck passed, formatting and pre-commit hooks passed.

@rbalachandar

Copy link
Copy Markdown
Contributor Author

/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.

Reviewed — no blockers. Suggestions are inline.

Test Plan (not a blocker): src/test-utils/mockWorkspaceContext.fallback.test.tsno such file or directory; src/tools/shell.test.tsno such file or directory; src/tools/read-file.test.tsno such file or directory; src/tools/zoom-image.test.tsno such file or directory; 410 tests pass — this review observed 1238, 19065, 1505, 483, 3225, 517 passed.

中文说明

已审查——无阻断问题。 建议见行内评论。

Test Plan(非阻断):src/test-utils/mockWorkspaceContext.fallback.test.tsno such file or directory; src/tools/shell.test.tsno such file or directory; src/tools/read-file.test.tsno such file or directory; src/tools/zoom-image.test.tsno such file or directory; 410 tests pass — this review observed 1238, 19065, 1505, 483, 3225, 517 passed

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

Comment thread packages/core/src/utils/workspaceContext.ts Outdated
@rbalachandar

Copy link
Copy Markdown
Contributor Author

Applied the latest reviewer suggestion in the follow-up refactor. The traversable ENOENT predicate is now centralized in isResolvableMissingPathError and used by both resolveWorkspacePath and resolveMissingPath, so the two guards cannot drift logically.

Validation remains green: 58 focused tests, core typecheck, formatting, and pre-commit hooks.

@rbalachandar

Copy link
Copy Markdown
Contributor Author

/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.

No issues found. LGTM! ✅

Not explored to full depth (tool budget reached): PR #8759 aligns the shared createMockWorkspaceContext t...: my probe of fs.realpathSync('') returned no output (inconisive); judged immaterial since both old and new code reject/throw on empty paths in production and t….

Test Plan (not a blocker): src/test-utils/mockWorkspaceContext.fallback.test.tsno such file or directory; src/tools/shell.test.tsno such file or directory; src/tools/read-file.test.tsno such file or directory; src/tools/zoom-image.test.tsno such file or directory; 410 tests pass — this review observed 1238, 19066, 1505, 483, 3225, 517 passed.

中文说明

未发现问题。LGTM!✅

未探索到全部深度(达到工具调用预算):PR #8759 aligns the shared createMockWorkspaceContext t...:my probe of fs.realpathSync('') returned no output (inconisive); judged immaterial since both old and new code reject/throw on empty paths in production and t…

Test Plan(非阻断):src/test-utils/mockWorkspaceContext.fallback.test.tsno such file or directory; src/tools/shell.test.tsno such file or directory; src/tools/read-file.test.tsno such file or directory; src/tools/zoom-image.test.tsno such file or directory; 410 tests pass — this review observed 1238, 19066, 1505, 483, 3225, 517 passed

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

@rbalachandar
rbalachandar requested a review from doudouOUC August 11, 2026 20:08

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Suggestions are inline. 6 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not explored to full depth (tool budget reached): You are review agent 6a — Agent 6a: Undirected audit — ...: I did not check whether the resolveMissingPath loop could produce a subtly incorrect result for a path where an intermediate ancestor is a symlink to a direct…; You are review agent 3a — Agent 3a: Reuse & duplication.: (none — all checks completed within the budget); You are review agent 1b — Agent 1b: Removed-behavior au...: 无(已检查所有被删除的行)。. Test Plan (not a blocker): src/test-utils/mockWorkspaceContext.fallback.test.tsno such file or directory; src/tools/shell.test.tsno such file or directory; src/tools/read-file.test.tsno such file or directory; src/tools/zoom-image.test.tsno such file or directory.

中文说明

已审查。 建议见行内评论。 6 条建议级发现无法锚定到改动行,已丢弃;此处无需进一步处理。 未探索到全部深度(达到工具调用预算):You are review agent 6a — Agent 6a: Undirected audit — ...:I did not check whether the resolveMissingPath loop could produce a subtly incorrect result for a path where an intermediate ancestor is a symlink to a direct…;You are review agent 3a — Agent 3a: Reuse & duplication.:(none — all checks completed within the budget);You are review agent 1b — Agent 1b: Removed-behavior au...:无(已检查所有被删除的行)。。 Test Plan(非阻断):src/test-utils/mockWorkspaceContext.fallback.test.tsno such file or directory; src/tools/shell.test.tsno such file or directory; src/tools/read-file.test.tsno such file or directory; src/tools/zoom-image.test.tsno such file or directory

— deepseek-v4-flash via Qwen Code /review (v0.21.8)

Comment on lines +161 to +169
it('should preserve paths with missing intermediate components', () => {
const workspaceContext = new WorkspaceContext(cwd);
const nonExistentPath = path.join(cwd, 'missing', 'nested.txt');

expect(resolveWorkspacePath(nonExistentPath)).toBe(nonExistentPath);
expect(workspaceContext.isPathWithinWorkspace(nonExistentPath)).toBe(
true,
);
});

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] Mutation-vacuous test — replacing resolveMissingPath with return pathToCheck (discarding the ancestor-walk loop entirely) still passes both assertions. The input is a simple subpath of the existing root, so the walked-up result is identical to the original input.

Failure scenario: If a future regression replaced the ancestor walk with a trivial identity return, this test would not catch it. The isPathWithinWorkspace assertion is the load-bearing one; the resolveWorkspacePath assertion is redundant for this input.

Suggested change
it('should preserve paths with missing intermediate components', () => {
const workspaceContext = new WorkspaceContext(cwd);
const nonExistentPath = path.join(cwd, 'missing', 'nested.txt');
expect(resolveWorkspacePath(nonExistentPath)).toBe(nonExistentPath);
expect(workspaceContext.isPathWithinWorkspace(nonExistentPath)).toBe(
true,
);
});
// Add a second test case where the missing portion sits below a symlink
// that maps to a different point in the filesystem, so that
// resolveMissingPath produces a *different* path from the input.
中文说明

[Suggestion] 变异无效的测试——将 resolveMissingPath 替换为 return pathToCheck(完全丢弃祖先遍历循环)后,两个断言仍然通过。输入是已存在根目录的简单子路径,遍历后的结果与原始输入相同。

失败场景:如果未来回归将祖先遍历替换为恒等返回,该测试不会捕获。isPathWithinWorkspace 断言才是真正负载的;resolveWorkspacePath 断言在此输入下是冗余的。

Suggested change
it('should preserve paths with missing intermediate components', () => {
const workspaceContext = new WorkspaceContext(cwd);
const nonExistentPath = path.join(cwd, 'missing', 'nested.txt');
expect(resolveWorkspacePath(nonExistentPath)).toBe(nonExistentPath);
expect(workspaceContext.isPathWithinWorkspace(nonExistentPath)).toBe(
true,
);
});
// 添加第二个测试用例,缺失部分坐在符号链接之下,
// 该符号链接映射到文件系统的不同位置,
// 这样 resolveMissingPath 会产生与输入*不同*的路径。

— deepseek-v4-flash via Qwen Code /review (v0.21.8)

Comment on lines +33 to +42
return allDirs.some((dir) => {
try {
return isPathWithinRoot(
canonicalPath,
canonicalizeForContainment(dir),
);
} catch {
return false;
}
});

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] Inner try/catch wraps isPathWithinRoot but only canonicalizeForContainment is fallible — isPathWithinRoot is a pure string function (path.relative + three boolean checks) that never throws. The catch is only reachable through canonicalizeForContainment(dir), which is called as the second argument. A reader unfamiliar with the code cannot tell which operation is fallible.

Failure scenario: No crash — the code works correctly. But it obscures which part of the block is actually fallible, making the mock harder to reason about and maintain. The real WorkspaceContext.isPathWithinWorkspace (line 246 in the same diff) has no inner try/catch around isPathWithinRoot at all.

Suggested change
return allDirs.some((dir) => {
try {
return isPathWithinRoot(
canonicalPath,
canonicalizeForContainment(dir),
);
} catch {
return false;
}
});
return allDirs.some((dir) => {
const canonicalDir = canonicalizeForContainment(dir);
return isPathWithinRoot(canonicalPath, canonicalDir);
});
中文说明

[Suggestion] 内层 try/catch 包裹了 isPathWithinRoot,但只有 canonicalizeForContainment 可能抛出异常——isPathWithinRoot 是一个纯字符串函数(path.relative + 三个布尔检查),从不抛出。catch 只能通过第二个参数 canonicalizeForContainment(dir) 到达。不熟悉代码的读者无法判断哪个操作是可能失败的。

失败场景:不会崩溃——代码正确运行。但它掩盖了代码块中真正可能失败的部分,使 mock 更难理解和维护。真正的 WorkspaceContext.isPathWithinWorkspace(同一 diff 第 246 行)完全没有内层 try/catch。

Suggested change
return allDirs.some((dir) => {
try {
return isPathWithinRoot(
canonicalPath,
canonicalizeForContainment(dir),
);
} catch {
return false;
}
});
return allDirs.some((dir) => {
const canonicalDir = canonicalizeForContainment(dir);
return isPathWithinRoot(canonicalPath, canonicalDir);
});

— deepseek-v4-flash via Qwen Code /review (v0.21.8)

Comment on lines +343 to +349
function isFileSymlink(filePath: string): boolean {
try {
return !fs.readlinkSync(filePath).endsWith('/');
} catch (_error) {
return false;
}
}

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] isFileSymlink heuristic is misnamed — it detects symlinks whose target literal ends with /, not file symlinks. readlinkSync returns the raw link target, and a symlink to a directory does not normally have a trailing / unless created with ln -s target/ link. So isFileSymlink returns true for most directory symlinks, causing isResolvableMissingPathError to re-throw paths that are actually resolvable.

Failure scenario: No concrete bug — this is conservatively safe (the caller catches the error and returns false for containment). But the function name implies a different semantic, and the conservative re-throw means the function silently treats all symlinks to non-existent targets as non-resolvable, which is unnecessary for the common case of a symlink to a non-existent directory.

Suggested change
function isFileSymlink(filePath: string): boolean {
try {
return !fs.readlinkSync(filePath).endsWith('/');
} catch (_error) {
return false;
}
}
function isSymlinkTargetMarkedDirectory(filePath: string): boolean {
中文说明

[Suggestion] isFileSymlink 启发式函数命名有误——它检测的是符号链接目标字面量是否以 / 结尾,而不是"文件符号链接"。readlinkSync 返回原始链接目标,指向目录的符号链接通常不带尾部 /,除非用 ln -s target/ link 创建。所以 isFileSymlink 对大多数目录符号链接也返回 true,导致 isResolvableMissingPathError 抛出了本可解析的路径。

失败场景:无具体 bug——这是保守安全的(调用者捕获异常并返回 false)。但函数名暗示了不同的语义,保守的重新抛出意味着该函数将指向不存在目标的符号链接全部视为不可解析。

Suggested change
function isFileSymlink(filePath: string): boolean {
try {
return !fs.readlinkSync(filePath).endsWith('/');
} catch (_error) {
return false;
}
}
function isSymlinkTargetMarkedDirectory(filePath: string): boolean {

— deepseek-v4-flash via Qwen Code /review (v0.21.8)

Comment on lines +42 to +48
it('uses lexical containment when ENOENT has no path', () => {
const workspace = createMockWorkspaceContext(rootDir);

expect(
workspace.isPathWithinWorkspace(path.join(rootDir, 'missing.txt')),
).toBe(true);
});

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] Fallback test 'uses lexical containment when ENOENT has no path' is redundant with the first test — both use the same mock setup (realpathSync mocked to throw throwMissingPath, which creates an ENOENT error with no error.path) and assert the same condition (path.join(rootDir, 'missing.txt')true). The second test's assertion is a strict subset of the first test's assertions.

Failure scenario: No concrete bug — the tests are correct. But a reader would expect two different scenarios from two test names. The second test adds no new coverage.

Suggested change
it('uses lexical containment when ENOENT has no path', () => {
const workspace = createMockWorkspaceContext(rootDir);
expect(
workspace.isPathWithinWorkspace(path.join(rootDir, 'missing.txt')),
).toBe(true);
});
// Either remove this test, or change the mock to throw an ENOENT
// with an empty-string `path` (not `undefined`) to test a genuinely distinct case.
中文说明

[Suggestion] 回退测试 'uses lexical containment when ENOENT has no path' 与第一个测试冗余——两者使用相同的 mock 设置(realpathSync 模拟抛出不带 error.path 的 ENOENT),并断言相同的条件(path.join(rootDir, 'missing.txt')true)。第二个测试的断言是第一个测试断言的严格子集。

失败场景:无具体 bug——测试正确。但读者会期望两个不同的测试名称对应两个不同的场景。第二个测试没有增加新的覆盖。

Suggested change
it('uses lexical containment when ENOENT has no path', () => {
const workspace = createMockWorkspaceContext(rootDir);
expect(
workspace.isPathWithinWorkspace(path.join(rootDir, 'missing.txt')),
).toBe(true);
});
// 删除此测试,或将 mock 改为抛出空字符串 `path`(而非 `undefined`)的 ENOENT,
// 以测试真正不同的情况。

— deepseek-v4-flash via Qwen Code /review (v0.21.8)

Comment on lines +319 to +322
const parent = path.dirname(ancestor);
if (parent === ancestor) {
return pathToCheck;
}

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] resolveMissingPath silently returns the original unresolved path when it walks up to the filesystem root without finding a resolvable ancestor — the parent === ancestor termination at the root boundary returns pathToCheck unchanged, with no documentation about this fallback.

Failure scenario: If a path like /a/b/c/d is supplied where every component from d up to / is non-existent or unreadable (ENOENT), the function returns the original /a/b/c/d unchanged. Downstream, isPathWithinRoot accepts the path via lexical containment. This is pre-existing behavior (the old e.path fallback did the same), but it is an undocumented implicit assumption.

Suggested change
const parent = path.dirname(ancestor);
if (parent === ancestor) {
return pathToCheck;
}
// Fall back to the original path when the entire hierarchy is missing.
// isPathWithinRoot will apply lexical containment in this case,
// matching the pre-existing behavior from the old e.path fallback.
return pathToCheck;
中文说明

[Suggestion] resolveMissingPath 在遍历到文件系统根目录仍未找到可解析的祖先时,会静默返回原始的未解析路径——parent === ancestor 在根目录边界的终止条件返回未修改的 pathToCheck,没有任何关于此回退行为的文档。

失败场景:如果路径 /a/b/c/d 的每个组件从 d/ 都不存在或不可读,函数返回原始的 /a/b/c/d。下游 isPathWithinRoot 会通过词法包含关系接受该路径。这是已有行为(旧的 e.path 回退也是如此),但这是一个未记录的隐式假设。

Suggested change
const parent = path.dirname(ancestor);
if (parent === ancestor) {
return pathToCheck;
}
// 整个路径层级都不存在时回退到原始路径。
// 此时 isPathWithinRoot 会使用词法包含判断,
// 与旧的 e.path 回退行为一致。
return pathToCheck;

— deepseek-v4-flash via Qwen Code /review (v0.21.8)

@wenshao

wenshao commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Local verification report (maintainer, macOS)

Verified this PR locally on real macOS hardware — the platform the fix targets. Result: the #8753 regression reproduces exactly on main and is fixed by this PR; an A/B probe confirms production WorkspaceContext containment verdicts are unchanged. LGTM for merge.

Environment: macOS arm64 (Darwin 25.6.0), Node v24.18.1, fresh npm ci in an isolated worktree. Before = origin/main @ ac78acd3c5, After = PR head @ 5ec9f624c8 (merge-base 28ab8bae56).

1. Before — bug reproduced on main

Both tests from #8753 fail deterministically with expected 'ask' to be 'allow':

before: 2 tests fail on main

2. After — focused suite green on the PR head

The reviewer test plan's five files pass 412/412 (the PR body says 410; the base advanced and shell.test.ts gained two tests since — no failures either way):

after: 412/412 focused tests pass

3. Production behavior A/B — verdicts unchanged

Since this PR also refactors production workspaceContext.ts (extracting resolveWorkspacePath and replacing the error.path-based missing-path resolution with an ancestor walk), I ran an untracked probe that instantiates the real WorkspaceContext against 10 edge cases on both checkouts: existing file, missing leaf, missing intermediate dirs, ..-escape after a missing component, symlink escape, dangling symlink, symlink cycle, outside-workspace path, symlinked workspace root, and the lexical /var alias. All 10 verdicts are identical on main and the PR head. Typecheck, build, eslint, and prettier are all clean on the changed files:

A/B probe: identical verdicts + clean static checks

One nuance worth recording: Node's realpathSync ENOENT error.path truncates at the first missing component (probe: /root/missing/nested.txterror.path = /root/missing), so the old code resolved such paths to a truncated prefix while the new ancestor walk preserves the full tail. This is verdict-neutral for containment (a truncated prefix and its descendants land on the same side of every workspace root), and the new, more faithful resolution is covered by the added should preserve paths with missing intermediate components test. For the ..-after-missing case both implementations resolve to the escaped target and correctly reject it.

4. Full packages/core suite + failure attribution

The full suite at the PR head: 19,581 passed, 2 failed across 7 files — all in areas this PR does not touch (provider presets, LSP, memory integration). Re-running those exact 7 files standalone on both checkouts yields an identical profile (1 failed | 86 passed on each): the sole standalone failure is memoryLifecycle.integration.test.ts, which picks up this machine's real user memory directory (local environment issue), and the rest only fail under full-suite parallel load. None are caused by this PR.

full suite + attribution on both checkouts

Notes (non-blocking)

  • resolveWorkspacePath becomes part of core's public API via the existing export * from './utils/workspaceContext.js' — consistent with how isPathWithinRoot is already exposed, just noting the surface grows.
  • resolveMissingPath performs one realpathSync per missing ancestor level instead of the previous single call. Production results are memoized in resolvedPathCache, and the mock is test-only, so the cost is negligible.
  • The mock's lexical fallback (stubbed-filesystem tests) now uses isPathWithinRoot instead of raw startsWith, which also fixes the sibling-prefix false positive in that degraded mode.

Conclusion: fix confirmed on the target platform, no production behavior change detected, no regressions attributable to this PR across the full core suite. Recommending merge.

中文版本 (Chinese version)

本地验证报告(维护者,macOS)

在真实 macOS 硬件(本修复所针对的平台)上完成了本地验证。结论:#8753 的回归在 main 上精确复现,本 PR 修复了它;A/B 探针确认生产 WorkspaceContext 的路径边界判定完全不变。支持合并。

环境: macOS arm64 (Darwin 25.6.0),Node v24.18.1,隔离 worktree 内全新 npm ci。Before = origin/main @ ac78acd3c5,After = PR head @ 5ec9f624c8(merge-base 28ab8bae56)。

1. Before —— 在 main 上复现缺陷

#8753 中的两个测试确定性失败,报 expected 'ask' to be 'allow'(见上方截图 1)。

2. After —— PR head 上聚焦套件全绿

评审测试计划中的 5 个文件 412/412 通过(PR 描述写 410;因为 base 前进后 shell.test.ts 新增了 2 个测试——两种口径下均无失败,见截图 2)。

3. 生产行为 A/B —— 判定完全不变

由于本 PR 同时重构了生产代码 workspaceContext.ts(抽出 resolveWorkspacePath,并把基于 error.path 的缺失路径解析改为逐级向上找最近存在的祖先),我编写了一个未跟踪的探针,在两个 checkout 上分别实例化真实的 WorkspaceContext 跑 10 个边界用例:存在的文件、缺失叶子、缺失中间目录、缺失组件后接 .. 逃逸、符号链接逃逸、悬空符号链接、符号链接环、workspace 外路径、符号链接的 workspace 根,以及 /var 词法别名。10 个判定在 main 与 PR head 上完全一致。 改动文件的 typecheck、build、eslint、prettier 全部通过(见截图 3)。

一个值得记录的细节:Node 的 realpathSync ENOENT error.path截断到第一个缺失组件(探针实测:/root/missing/nested.txterror.path = /root/missing),因此旧代码对这类路径解析出的是截断前缀,而新的祖先回溯保留完整尾部。这对 containment 判定是中性的(截断前缀与其后代路径相对任意 workspace 根的内外判定一致),且新增的 should preserve paths with missing intermediate components 测试覆盖了这一更忠实的解析。对"缺失组件后接 .."的情况,新旧实现都解析到逃逸目标并正确拒绝。

4. packages/core 全量套件 + 失败归因

PR head 上全量套件:19,581 通过,7 个文件中 2 个测试失败——全部位于本 PR 未触及的区域(provider 预设、LSP、memory 集成)。把这 7 个文件在两个 checkout 上单独重跑,失败画像完全一致(各为 1 failed | 86 passed):唯一的单独失败是 memoryLifecycle.integration.test.ts,它读到了本机真实的用户记忆目录(本机环境问题),其余仅在全量并发压力下偶发。没有任何失败可归因于本 PR(见截图 4)。

备注(不阻塞合并)

  • resolveWorkspacePath 经由既有的 export * from './utils/workspaceContext.js' 进入 core 公共 API——与 isPathWithinRoot 的暴露方式一致,仅提示 API 面变大。
  • resolveMissingPath 对每层缺失祖先各执行一次 realpathSync(旧实现单次调用)。生产路径有 resolvedPathCache 记忆化,mock 仅用于测试,开销可忽略。
  • mock 的词法回退(stub 文件系统的测试)现在用 isPathWithinRoot 替代裸 startsWith,顺带修复了该降级模式下的同前缀兄弟目录误判。

结论: 修复在目标平台确认生效,未检测到生产行为变化,全量 core 套件无可归因于本 PR 的回归。建议合并。

@wenshao
wenshao enabled auto-merge August 12, 2026 02:55
@wenshao
wenshao added this pull request to the merge queue Aug 12, 2026
Merged via the queue into QwenLM:main with commit 5e97fc8 Aug 12, 2026
72 of 74 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.11.

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.

test(core): workspace mock breaks read permission tests on macOS

4 participants