feat(cli): enable /directory command in ACP mode by doudouOUC · Pull Request #4826 · QwenLM/qwen-code

doudouOUC · 2026-06-06T17:39:00Z

Summary

What changed: Refactor /directory (show/add) from addItem-based output to returning MessageActionReturn, enabling ACP mode (web-shell) support.
Why it changed: Web-shell users couldn't manage workspace directories because /directory was supportedModes: ['interactive'] only and used TUI-only addItem for output.
Reviewer focus: The add subcommand refactoring — multiple addItem calls collapsed into a single MessageActionReturn with collected messages.

Validation

Commands run:

npx vitest run packages/cli/src/ui/commands/directoryCommand.test.ts packages/cli/src/ui/commands/directoryCommand.test.tsx packages/cli/src/services/commandUtils.test.ts

Expected result: All tests pass
Observed result: 41/41 tests pass (28 directory + 14 commandUtils - 1 overlap)
Quickest reviewer verification path: Run the test command above; verify supportedModes includes 'acp' in the command file
Evidence: CI coverage report on PR

Scope / Risk

Main risk or tradeoff: Mixed success+error results use messageType: 'error' for the whole message — single MessageActionReturn can't express mixed severity. In interactive mode, successful text renders with error styling when errors coexist (minor visual regression, same tradeoff as other message-returning commands).
Not covered / not validated: E2E integration test with running daemon + web-shell
Breaking changes / migration notes: None — message return type is handled identically by slashCommandProcessor in interactive mode

Testing Matrix

	🍏	🪟	🐧
npm run	✅	⚠️	⚠️
npx	N/A	N/A	N/A
Docker	N/A	N/A	N/A
Podman	N/A	N/A	N/A
Seatbelt	N/A	N/A	N/A

Testing matrix notes:

Only unit tests involved; no platform-specific behavior

Linked Issues / Bugs

Refs #4514 (T3.10)

🤖 Generated with Qwen Code

Refactor /directory (show/add) from addItem-based output to returning MessageActionReturn so it works in ACP mode (web-shell). Changes: - Add supportedModes: ['interactive', 'acp'] to parent + both subcommands - Add argumentHint to add subcommand for command palette - Add parent action returning usage hint for bare /directory invocation - Refactor show: return message instead of addItem - Refactor add: collect all outputs into messages array, return single message (messageType: 'error' if any errors, 'info' otherwise) - Add try-catch outer wrapper for unexpected errors - Simplify pathsToAdd parsing (remove no-op split-join) - Update existing .tsx tests to assert on return values instead of addItem - Add new .ts test file with 11 tests covering ACP paths Known limitations: - Mixed success+error returns use messageType: 'error' for the whole message (single MessageActionReturn can't express mixed severity) - Cross-session: directory add is session-scoped, other sessions see the change after restart (pre-existing architectural property) Refs #4514

github-actions · 2026-06-06T17:40:26Z

📋 Review Summary

This PR refactors the /directory command (show/add subcommands) from TUI-only addItem-based output to returning MessageActionReturn, enabling ACP mode (web-shell) support. The implementation is well-structured with comprehensive test coverage. The key change consolidates multiple addItem calls into a single message return with collected content.

🔍 General Feedback

The refactoring successfully enables ACP mode support by adding 'acp' to supportedModes for the parent command and both subcommands
Good consolidation of multiple addItem calls into message collection arrays, then joining at the end
Test coverage is solid with 41 tests passing (28 directory + 14 commandUtils - 1 overlap)
The PR description clearly documents the tradeoff about mixed success/error results using single messageType
Added argumentHint to the add subcommand for better UX hints

🎯 Specific Feedback

🟡 High

File: packages/cli/src/ui/commands/directoryCommand.tsx:147-152 - The path parsing logic changed from args.split(' ') then .join(' ') then .split(',') to just args.split(','). This could be a breaking change if users were passing paths with spaces (e.g., /path/with spaces). The original code preserved spaces within paths, but the new code will split on spaces too early. Consider: args.split(',').map(p => p.trim()).filter(p => p) to handle "path1, path2" format while preserving spaces within individual paths.

🟢 Medium

File: packages/cli/src/ui/commands/directoryCommand.tsx:139 - The parent command now has a default action that returns a usage hint. However, this action is also marked with supportedModes: ['interactive', 'acp']. Consider if the parent command should have an argumentHint as well to show expected subcommands, similar to how the add subcommand has one.
File: packages/cli/src/ui/commands/directoryCommand.tsx:258-267 - The memory loading block inside if (added.length > 0) has a nested try-catch that pushes to errors array, but then the outer block continues to the success message. This means if memory loading fails, the error is recorded but users still see "Successfully added directories". Consider making the error more prominent or restructuring so memory failures don't get buried.

🔵 Low

File: packages/cli/src/ui/commands/directoryCommand.tsx:283 - The fallback message t('No directories added.') when messages.join('\n') is empty seems unlikely to occur given the logic flow (if added.length > 0, messages will have content; if added.length === 0 but alreadyAdded.length > 0, that message is added). Consider if this fallback is actually reachable or can be removed.
File: packages/cli/src/ui/commands/directoryCommand.test.tsx - The existing tests still use the old pattern with mockContext.ui.addItem expectations. These tests will need to be updated to expect the new MessageActionReturn format instead of checking addItem calls. The diff shows new test file directoryCommand.test.ts was created with the correct assertions, but the old .test.tsx file still has the old test patterns that will fail.

✅ Highlights

Excellent test coverage in the new directoryCommand.test.ts file with clear assertions for the new return format
Good error handling with try-catch wrapping the entire operation to catch unexpected failures
The mixed success/error handling is well-implemented - collecting all messages and returning error type when any errors occur
Proper i18n usage with t() throughout all user-facing messages
The show subcommand refactoring is clean and straightforward
Good use of as const type assertions for type safety on the return objects

qwen-code-ci-bot · 2026-06-06T17:41:29Z

Thanks for the PR!

Template: the PR body covers all the key areas (what/why/validation/risk/linked issues) but uses custom headings instead of the template's structure. Not blocking, but matching the template headings makes it easier for maintainers to scan. Minor.

On direction: clearly aligned. Enabling /directory in ACP/web-shell mode is part of the daemon mode initiative (#4514 T3.10), and converting addItem → MessageActionReturn is the established pattern — branchCommand, hooksCommand, languageCommand, rewindCommand, and others all use it. No question this is the right direction.

On approach: scope is tight and focused. Three files — implementation, new unit test, and migrated existing test. The addition of a top-level action for the usage hint and argumentHint on the add subcommand are nice touches for ACP usability. The outer try/catch wrapping the entire add action body is a new behavioral addition — in the original code, unexpected errors would propagate up; now they're caught and returned as user-facing error messages. This is reasonable for ACP robustness, but worth being aware of as a semantic change.

One observation on path parsing: the old code did args.split(' ') → join(' ') → split(',') which is equivalent to just split(',') but also broke paths with spaces. The new args.split(',').filter(p => p.trim()) is cleaner and handles space-containing paths better. This is a quiet fix — not called out in the description but it's an improvement.

Moving on to code review. 🔍

中文说明

感谢贡献！

模板：PR 正文覆盖了所有关键信息（变更内容/原因/验证/风险/关联 issue），但使用了自定义标题而非模板结构。不阻塞，但匹配模板标题更方便维护者快速浏览。小问题。

方向：明确对齐。在 ACP/web-shell 模式下启用 /directory 是 daemon mode 计划（#4514 T3.10）的一部分，将 addItem 转换为 MessageActionReturn 是已有模式——branchCommand、hooksCommand、languageCommand、rewindCommand 等都采用这种方式。方向没有问题。

方案：范围紧凑聚焦。三个文件——实现、新单元测试、迁移后的已有测试。顶层 action 提供用法提示和 add 子命令的 argumentHint 对 ACP 可用性很有帮助。外层 try/catch 包裹整个 add action 主体是新增的行为变更——原代码中未预期的错误会向上抛出，现在会被捕获并作为用户可见的错误消息返回。对于 ACP 健壮性来说合理，但值得注意这是一个语义变化。

路径解析的观察：旧代码做了 args.split(' ') → join(' ') → split(',')，等价于直接 split(',') 但还会打断包含空格的路径。新的 args.split(',').filter(p => p.trim()) 更简洁，也能更好地处理含空格的路径。这是一个静默修复——描述中没提到，但确实是改进。

进入代码审查 🔍

— Qwen Code · qwen3.7-max

Copilot

Pull request overview

Refactors the /directory slash command to return MessageActionReturn results (instead of using TUI-only addItem output), and enables the command in ACP mode so web-shell clients can add/show workspace directories.

Changes:

Enable ACP execution for /directory (parent + add/show) via supportedModes: ['interactive', 'acp'].
Refactor add and show actions to return type: 'message' results and aggregate multi-step output into a single message.
Update and add unit tests to validate ACP support and the new message-returning behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
packages/cli/src/ui/commands/directoryCommand.tsx	Enable ACP mode and refactor `/directory` actions to return `MessageActionReturn` instead of `addItem` UI output.
packages/cli/src/ui/commands/directoryCommand.test.tsx	Update existing tests to assert returned message objects rather than `addItem` calls.
packages/cli/src/ui/commands/directoryCommand.test.ts	Add focused tests for ACP `supportedModes`, `argumentHint`, and basic message-returning behavior.

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

…md message 1. Usage hint now shows comma-separated format: `<path>[,<path>,...]` 2. QWEN.md files success message only emitted when memory refresh actually runs

doudouOUC · 2026-06-06T18:13:44Z

Review fix summary — `0dea510`

Thread	Action	Detail
Copilot: usage hint format (line 114)	Fixed	Usage hint now shows `<path>[,<path>,...]`
Copilot: conditional QWEN.md message (line 249)	Fixed	Message moved inside `shouldLoadMemoryFromIncludeDirectories()` block

CI: Reran triage workflow (run 27069285830) — failed with "Reached max session turns" (triage bot limitation, not PR-related).

…ch, test consolidation 1. Use messageType 'warning' for partial success (some paths added, some failed) instead of 'error' which throws in ACP mode via Session.ts 2. Wrap gemini.addDirectoryContext() in its own try-catch to prevent losing accumulated success messages on failure 3. Consolidate duplicate .test.ts into .test.tsx, add space-in-path test, add settings.setValue assertion for mixed-result scenario 4. Delete redundant directoryCommand.test.ts

doudouOUC · 2026-06-06T20:14:39Z

Review fix summary — `4b3721f`

#	Thread	Action	Detail
1	[Critical] `messageType: 'error'` throws in ACP (line 273)	Fixed	Partial success now uses `'warning'`; `'error'` only when all paths fail
2	[Critical] `gemini.addDirectoryContext()` no try-catch (line 255)	Fixed	Wrapped in own try-catch; errors push to array instead of hitting outer catch
3	[Suggestion] Outer catch doesn't log (line 283)	Pushed back	Follows dreamCommand/forgetCommand precedent; cross-command concern
4	[Suggestion] No test for space-in-path (line 143)	Fixed	Added `should handle paths with spaces without splitting on space`
5	[Suggestion] Mixed-result test missing `setValue` (line 302)	Fixed	Added `settings.setValue` assertion
6	[Suggestion] Duplicate test files (line 1)	Fixed	Consolidated into `.test.tsx`, deleted `.test.ts`

All 22 tests pass.

…uards 1. Add test for addDirectoryContext() rejection → messageType 'warning' + error message 2. Restore null-config tests for both show and add subcommands (lost during consolidation)

doudouOUC · 2026-06-07T02:32:21Z

Review fix summary — `e87b1c0` (round 3)

#	Thread	Action	Detail
1	[Critical] gemini try-catch zero test coverage (line 266)	Fixed	Added `should warn when gemini.addDirectoryContext throws` test
2	[Suggestion] Null-config tests lost during consolidation (line 32)	Fixed	Restored null-config tests for both `show` and `add` subcommands

All 25 tests pass.

wenshao · 2026-06-07T04:03:03Z

+    it('should return error when config is null', async () => {
      if (!showCommand?.action) throw new Error('No action');
-      showCommand.action(mockContext, '');
+      const nullConfigContext = {


[Suggestion] Both null-config test stubs (show at line 103, add at line 131) create { services: { config: null } } without a settings property, relying on as unknown as CommandContext to bypass type checking. The add action destructures const { config, settings } = context.services, setting settings to undefined. This works because the if (!config) guard returns early — but if someone inserts settings-accessing code between the destructuring and the null guard, the test will crash with a confusing TypeError.

Suggested change

const nullConfigContext = {

const nullConfigContext = {

services: { config: null, settings: { merged: {}, workspace: {}, setValue: vi.fn() } },

} as unknown as CommandContext;

— DeepSeek/deepseek-v4-pro via Qwen Code /review

wenshao · 2026-06-07T07:32:37Z

-          if (gemini) {
-            await gemini.addDirectoryContext();
+          if (added.length > 0) {
+            try {


[Suggestion] The entire shouldLoadMemoryFromIncludeDirectories() branch (lines 222–258, ~36 statements) has zero test coverage — all tests mock it as false. This covers loadServerHierarchicalMemory, setUserMemory, setGeminiMdFileCount, setConditionalRulesRegistry, and the associated error catch. A regression here could silently leave stale memory/rules after adding directories.

Consider adding a test that mocks shouldLoadMemoryFromIncludeDirectories as true and verifies the config setters are called, plus a test where loadServerHierarchicalMemory throws to exercise the error catch.

— qwen3.7-max via Qwen Code /review

wenshao · 2026-06-07T07:32:37Z

+                includeDirectories,
+              );
+            } catch (error) {
+              errors.push(


[Suggestion] The settings.setValue error catch has no test — setValue never throws in any test. This means the degradation behavior when settings persistence fails (e.g., read-only workspace) is unverified. The error is pushed to the errors array and the return becomes warning or error, but this is unconfirmed.

Consider adding a test that mocks settings.setValue to throw and asserts the error message and messageType.

— qwen3.7-max via Qwen Code /review

wenshao · 2026-06-07T07:32:37Z

      },
      kind: CommandKind.BUILT_IN,
-      supportedModes: ['interactive'] as const,
+      supportedModes: ['interactive', 'acp'] as const,


[Suggestion] With ACP mode enabled, remote clients can add arbitrary readable filesystem paths (e.g., /etc, /root/.ssh) to workspace context. The only guard is isRestrictiveSandbox(), which defaults to false. Since workspace context feeds the file crawler, this creates a potential path for directory enumeration via the LLM.

Consider adding ACP-specific path confinement (e.g., restrict to project root or an allowlist) when context.executionMode === 'acp', or document this as an accepted risk for authenticated ACP sessions.

— qwen3.7-max via Qwen Code /review

wenshao · 2026-06-07T07:32:43Z

Local Verification Report

Branch: feat/directory-acp-mode → daemon_mode_b_main
Commit: e87b1c0f8
Environment: macOS Darwin 25.4.0 / Node.js / Vitest 3.2.4

Unit Tests

Package	Test Files	Tests	Result
`packages/cli` (directoryCommand.test.tsx)	1 passed	25/25 passed	✅

Test breakdown:

directoryCommand > show: 2 tests (null config, list directories)
directoryCommand > add: 14 tests (null config, no path, sandbox, spaces, single/multi path, persist, dedupe, skip, error, partial success, gemini throw)
directoryCommand > misc: 5 tests (subcommand hints, usage, Windows path expansion)
getDirPathCompletions: 4 tests (isDirectory flag, prefix filter, comma-separated, nested)

ESLint

directoryCommand.tsx: 0 warnings, 0 errors ✅

TypeScript Type Check

Package	Errors	Status
`packages/cli`	43 (all pre-existing)	✅ No new errors

No typecheck errors in directoryCommand.tsx or directoryCommand.test.tsx. All 43 errors are pre-existing (@google/genai dual-package resolution, ink/dom, httpAcpBridge, etc.).

Whitespace Check

git diff --check: clean ✅

Summary

Check	Status
CLI unit tests (25)	✅ Pass
ESLint	✅ Clean
CLI typecheck (delta)	✅ No new errors
Whitespace	✅ Clean

All verification checks pass. The PR is ready for merge.

Verified locally by wenshao

yiliang114

LGTM

doudouOUC requested review from Copilot and wenshao June 6, 2026 17:39

Copilot started reviewing on behalf of doudouOUC June 6, 2026 17:39 View session

Copilot AI reviewed Jun 6, 2026

View reviewed changes

Comment thread packages/cli/src/ui/commands/directoryCommand.tsx Outdated

Comment thread packages/cli/src/ui/commands/directoryCommand.tsx Outdated

fix: address Copilot review — usage hint format and conditional QWEN.…

0dea510

…md message 1. Usage hint now shows comma-separated format: `<path>[,<path>,...]` 2. QWEN.md files success message only emitted when memory refresh actually runs

wenshao requested changes Jun 6, 2026

View reviewed changes

doudouOUC requested a review from wenshao June 6, 2026 23:38

wenshao requested changes Jun 7, 2026

View reviewed changes

Comment thread packages/cli/src/ui/commands/directoryCommand.tsx

Comment thread packages/cli/src/ui/commands/directoryCommand.test.tsx

fix: add missing test coverage for gemini try-catch and null-config g…

e87b1c0

…uards 1. Add test for addDirectoryContext() rejection → messageType 'warning' + error message 2. Restore null-config tests for both show and add subcommands (lost during consolidation)

github-actions Bot mentioned this pull request Jun 7, 2026

📊 AI CLI 工具社区动态日报 2026-06-07 jasonalang/big_model_radar#51

Open

wenshao approved these changes Jun 7, 2026

View reviewed changes

doudouOUC requested review from chiga0, yiliang114 and ytahdn June 7, 2026 04:10

wenshao reviewed Jun 7, 2026

View reviewed changes

yiliang114 approved these changes Jun 7, 2026

View reviewed changes

doudouOUC merged commit 8749ecf into daemon_mode_b_main Jun 7, 2026
11 checks passed

doudouOUC deleted the feat/directory-acp-mode branch June 7, 2026 12:24

doudouOUC mentioned this pull request Jun 7, 2026

tracking(serve): daemon capability gaps & prioritized backlog (post v0.16-alpha) #4514

Open

Conversation

doudouOUC commented Jun 6, 2026

Summary

Validation

Scope / Risk

Testing Matrix

Linked Issues / Bugs

Uh oh!

github-actions Bot commented Jun 6, 2026

📋 Review Summary

🔍 General Feedback

🎯 Specific Feedback

🟡 High

🟢 Medium

🔵 Low

✅ Highlights

Uh oh!

qwen-code-ci-bot commented Jun 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

doudouOUC commented Jun 6, 2026

Review fix summary — 0dea510

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

doudouOUC commented Jun 6, 2026

Review fix summary — 4b3721f

Uh oh!

Uh oh!

Uh oh!

doudouOUC commented Jun 7, 2026

Review fix summary — e87b1c0 (round 3)

Uh oh!

wenshao Jun 7, 2026

Choose a reason for hiding this comment

Uh oh!

wenshao Jun 7, 2026

Choose a reason for hiding this comment

Uh oh!

wenshao Jun 7, 2026

Choose a reason for hiding this comment

Uh oh!

wenshao Jun 7, 2026

Choose a reason for hiding this comment

Uh oh!

wenshao commented Jun 7, 2026

Local Verification Report

Unit Tests

ESLint

TypeScript Type Check

Whitespace Check

Summary

Uh oh!

yiliang114 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

Review fix summary — `0dea510`

Review fix summary — `4b3721f`

Review fix summary — `e87b1c0` (round 3)