Skip to content

fix(cli): Allow ACP local fallback reads from /tmp - #6370

Merged
wenshao merged 1 commit into
QwenLM:mainfrom
doudouOUC:codex/acp-tmp-local-read-fallback
Jul 6, 2026
Merged

fix(cli): Allow ACP local fallback reads from /tmp#6370
wenshao merged 1 commit into
QwenLM:mainfrom
doudouOUC:codex/acp-tmp-local-read-fallback

Conversation

@doudouOUC

Copy link
Copy Markdown
Collaborator

What this PR does

This PR expands only ACP local read fallback roots so ACP sessions can recover from client-side workspace-boundary rejections for POSIX /tmp paths. It keeps the existing read-file default permission policy unchanged, adds an append-only QWEN_ACP_LOCAL_READ_ROOTS escape hatch for extra absolute fallback roots, and keeps fallback path safety delegated to the existing realpath/subpath checks.

Why it's needed

ACP clients can reject generated local files under paths such as /tmp/datastudio_cli_extract/... as outside the workspace, even though the daemon process can read them locally. The change lets ACP fallback handle that narrow case without turning /tmp into a general auto-allowed read_file location for every mode.

Reviewer Test Plan

Reviewers should confirm that ACP sessions include /tmp in local fallback roots on POSIX platforms, that QWEN_ACP_LOCAL_READ_ROOTS appends only absolute entries after defaults, and that regular read_file default permissions still ask for external /tmp reads.

How to verify

Run cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts src/acp-integration/service/filesystem.test.ts and expect both ACP agent root configuration tests and filesystem fallback safety tests to pass. Run npx eslint packages/cli/src/acp-integration/acpAgent.ts packages/cli/src/acp-integration/acpAgent.test.ts packages/core/src/tools/read-file.ts and expect no lint findings. Run npm run build && npm run typecheck from the repository root and expect build plus workspace typecheck to complete successfully.

Evidence (Before & After)

N/A — non-UI ACP fallback behavior.

Tested on

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

Environment (optional)

macOS local workspace with Node.js v22.22.3, npm 10.9.8, and qwen 0.19.6 available.

Risk & Scope

  • Main risk or tradeoff: POSIX ACP sessions can now locally fallback-read files under /tmp after specific ACP boundary errors, so this intentionally broadens only that fallback surface while preserving realpath/subpath validation.
  • Not validated / out of scope: Windows runtime behavior was not tested locally; Windows also does not receive a default /tmp root and must use explicit absolute env roots.
  • Breaking changes / migration notes: No breaking changes or migration required; QWEN_ACP_LOCAL_READ_ROOTS is additive and optional.

Linked Issues

N/A

中文说明

What this PR does

本 PR 只扩大 ACP 本地读取 fallback roots,让 ACP 会话在客户端因 workspace 边界拒绝 POSIX /tmp 路径时,可以由 daemon 本地 fallback 读取。它保持现有 read_file 默认权限策略不变,新增追加式 QWEN_ACP_LOCAL_READ_ROOTS 作为额外绝对 fallback root 的低层开关,并继续复用现有 realpath/subpath 校验保证路径安全。

Why it's needed

ACP 客户端可能会把 /tmp/datastudio_cli_extract/... 这类生成文件判定为 workspace 外路径并拒绝读取,即使 daemon 进程本地有能力读取。该改动让 ACP fallback 覆盖这个窄场景,同时避免把 /tmp 变成所有模式下通用自动允许的 read_file 位置。

Reviewer Test Plan

评审者应确认 POSIX 平台的 ACP 会话会把 /tmp 放入本地 fallback roots,QWEN_ACP_LOCAL_READ_ROOTS 只会在默认 roots 后追加绝对路径,并且普通 read_file 对外部 /tmp 路径的默认权限仍然是 ask。

How to verify

运行 cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts src/acp-integration/service/filesystem.test.ts,应看到 ACP agent roots 配置测试和 filesystem fallback 安全测试都通过。运行 npx eslint packages/cli/src/acp-integration/acpAgent.ts packages/cli/src/acp-integration/acpAgent.test.ts packages/core/src/tools/read-file.ts,应无 lint 问题。最后在仓库根目录运行 npm run build && npm run typecheck,应成功完成构建和 workspace typecheck。

Evidence (Before & After)

N/A — 非 UI 的 ACP fallback 行为改动。

Tested on

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

Environment (optional)

macOS 本地工作区,Node.js v22.22.3、npm 10.9.8、qwen 0.19.6 可用。

Risk & Scope

  • Main risk or tradeoff: POSIX ACP 会话在特定 ACP 边界错误后,现在可以本地 fallback 读取 /tmp 下文件;这有意只扩大 fallback 面,同时保留 realpath/subpath 校验。
  • Not validated / out of scope: 未在本地验证 Windows 运行时行为;Windows 默认也不会获得 /tmp root,需要通过环境变量显式配置绝对路径。
  • Breaking changes / migration notes: 无破坏性变更,也不需要迁移;QWEN_ACP_LOCAL_READ_ROOTS 是可选的追加配置。

Linked Issues

N/A

Add POSIX /tmp to ACP local read fallback roots without changing read_file's default permission behavior. Also add QWEN_ACP_LOCAL_READ_ROOTS as an append-only absolute-path override for ACP fallback reads.

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@doudouOUC
doudouOUC marked this pull request as ready for review July 6, 2026 06:43
Copilot AI review requested due to automatic review settings July 6, 2026 06:43
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: No linked issue, but the scenario is concrete — ACP clients with strict workspace boundaries reject /tmp-based paths that the daemon process can read locally. This is a real operational gap, not theoretical hardening. The fix is narrow (one default root + an env escape hatch) and verifiable through unit tests.

Direction: Aligned. ACP fallback read behavior is core to the product, and broadening it to cover /tmp (a standard POSIX temp location) makes sense. The read_file default permission policy is untouched — this only affects the ACP-specific fallback path. No CHANGELOG reference needed for this scope.

Approach: The scope feels right — extract inline roots into buildAcpLocalReadRoots(), add /tmp on POSIX, add QWEN_ACP_LOCAL_READ_ROOTS as an additive-only env override with proper filtering (absolute paths only, trimmed, empty entries dropped). Test refactoring reduces duplication nicely. The SYNC comment in read-file.ts is updated to reflect that ACP may append fallback-only roots beyond the mirrored group. Clean separation.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:没有关联 issue,但场景是具体的——ACP 客户端因 workspace 边界限制拒绝了 daemon 本地可读的 /tmp 路径。这是实际运维中的缺口,不是理论性加固。修复范围很窄(一个默认 root + 一个环境变量逃生口),可以通过单元测试验证。

方向:对齐。ACP fallback 读取行为是产品核心功能,扩展覆盖 /tmp(标准 POSIX 临时目录位置)是合理的。read_file 默认权限策略未受影响——只改变 ACP 专属的 fallback 路径。

方案:范围合理——将内联 roots 提取为 buildAcpLocalReadRoots(),在 POSIX 上添加 /tmp,添加 QWEN_ACP_LOCAL_READ_ROOTS 作为仅追加的环境变量覆盖(过滤绝对路径、trim、去空条目)。测试重构减少了重复。read-file.ts 中的 SYNC 注释已更新,说明 ACP 可以在镜像组之后追加 fallback-only roots。分离清晰。

进入代码审查 🔍

Qwen Code · qwen3.7-max

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Expands ACP file-system local read fallback roots on POSIX to include /tmp, enabling ACP sessions to recover from client-side workspace-boundary rejections for temporary files while keeping the read_file tool’s default permission policy unchanged.

Changes:

  • Centralized ACP local fallback root construction in buildAcpLocalReadRoots, mirroring read_file’s default local roots and then appending ACP-only roots (/tmp on POSIX) plus QWEN_ACP_LOCAL_READ_ROOTS.
  • Updated ACP agent wiring to use the new builder for AcpFileSystemService options.
  • Refactored/expanded tests to validate default /tmp inclusion (POSIX-only) and env-root appending behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
packages/core/src/tools/read-file.ts Updates cross-component sync comment for local roots vs ACP fallback roots.
packages/cli/src/acp-integration/acpAgent.ts Adds /tmp (POSIX) + QWEN_ACP_LOCAL_READ_ROOTS support via buildAcpLocalReadRoots and uses it when creating AcpFileSystemService.
packages/cli/src/acp-integration/acpAgent.test.ts Adds assertions for /tmp default (POSIX) and env var appends; refactors setup into helpers for reuse.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +109 to +111
// SYNC: Keep these base roots and the auto-memory check below aligned with
// AcpAgent.buildAcpLocalReadRoots' mirrored ReadFileTool group. ACP may
// append fallback-only roots after that group.
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

The implementation is clean and correct:

  • buildAcpLocalReadRoots() cleanly separates the mirrored ReadFileTool group from ACP-only additions (/tmp + env var). The SYNC comment contract is preserved — the first group mirrors read-file.ts exactly, and ACP-specific roots are appended after.
  • parseAcpLocalReadRootsEnv() properly filters: trims whitespace, drops empty entries, requires absolute paths. Platform-aware delimiter (path.delimiter) handles both POSIX (:) and Windows (;).
  • defaultAcpOnlyLocalReadRoots() correctly gates /tmp on non-Windows platforms.
  • The read-file.ts change is comment-only — updates the SYNC note to reflect that ACP may append fallback-only roots. No behavioral change to read_file's default permission policy.
  • Test refactoring extracts expectAcpLocalReadRoots() helper, reducing duplication while adding a new test case for QWEN_ACP_LOCAL_READ_ROOTS env var filtering (relative paths rejected, whitespace trimmed, absolute entries appended).

No blockers, no reuse issues, no security concerns. Existing realpath/subpath validation in read_file provides symlink protection for the new /tmp root.

Testing

Non-UI internal configuration change — tmux real-scenario testing not applicable (ACP fallback behavior is only exercised within ACP sessions, not through direct CLI invocations). Unit test results below serve as verification.

Unit Tests

$ cd packages/cli && npx vitest run src/acp-integration/acpAgent.test.ts

 ✓ src/acp-integration/acpAgent.test.ts (191 tests) 8881ms

 Test Files  1 passed (1)
      Tests  191 passed (191)
   Duration  13.95s
$ cd packages/cli && npx vitest run src/acp-integration/service/filesystem.test.ts

 ✓ src/acp-integration/service/filesystem.test.ts (30 tests) 37ms

 Test Files  1 passed (1)
      Tests  30 passed (30)
   Duration  8.81s

Lint

$ npx eslint packages/cli/src/acp-integration/acpAgent.ts packages/cli/src/acp-integration/acpAgent.test.ts packages/core/src/tools/read-file.ts
(no output — clean)

Build + Typecheck

$ npm run build
(success — all packages built)

$ npm run typecheck
> @qwen-code/chrome-bridge typecheck — tsc --noEmit ✓
> @qwen-code/qwen-code typecheck — tsc --noEmit ✓
> @qwen-code/qwen-code-core typecheck — tsc --noEmit ✓
> @qwen-code/sdk typecheck — tsc --noEmit ✓
> @qwen-code/webui typecheck — tsc --noEmit ✓

All green. 221 tests passing, lint clean, build and typecheck clean across all 5 workspace packages.

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

This is a clean, well-scoped fix. The problem is real — ACP clients enforcing workspace boundaries will naturally reject /tmp paths, and the daemon should be able to fall back to reading those locally. The solution does exactly what's needed: one new default root on POSIX, one env var escape hatch for edge cases, and a test refactor that reduces duplication while adding coverage for the new behavior.

The extraction of buildAcpLocalReadRoots() is the right call — having a single named function with an explicit SYNC comment makes future drift between read-file.ts and the ACP fallback easier to catch. The env var filtering is defensive (absolute paths only, trimmed, platform-aware delimiter) without being over-engineered.

221 tests passing, lint clean, build and typecheck clean across all workspace packages. No correctness, security, or convention concerns.

Approving. ✅

中文说明

这是一个干净、范围合理的修复。问题是真实存在的——ACP 客户端执行 workspace 边界限制时会自然地拒绝 /tmp 路径,daemon 应该能够本地 fallback 读取。解决方案恰好做到了需要的:POSIX 上一个新的默认 root,一个用于边缘情况的环境变量逃生口,以及一个测试重构——在减少重复的同时增加了新行为的覆盖。

buildAcpLocalReadRoots() 提取为独立函数是正确的——有一个带显式 SYNC 注释的命名函数,使 read-file.ts 和 ACP fallback 之间的未来漂移更容易发现。环境变量过滤是防御性的(仅绝对路径、trim、平台感知分隔符),没有过度工程化。

221 个测试通过,lint 干净,所有 workspace 包的 build 和 typecheck 干净。无正确性、安全性或规范问题。

批准 ✅

Qwen Code · qwen3.7-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.

LGTM, looks ready to ship. ✅

@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! ✅

Downgraded from Approve to Comment: CI still running (30 checks pending). Will approve once CI passes.

— qwen3.7-max via Qwen Code /review

@wenshao
wenshao added this pull request to the merge queue Jul 6, 2026
Merged via the queue into QwenLM:main with commit 5c8af1a Jul 6, 2026
65 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.

4 participants