feat(cli): enable /remember, /forget, /dream in ACP mode#4819
Conversation
These three memory-related slash commands return `submit_prompt` or `message` action types which are fully supported by the ACP session handler. Adding `acp` to their `supportedModes` allows web-shell clients to invoke them via `POST /session/:id/prompt` passthrough. Changes: - /remember: add supportedModes (zero handler changes needed) - /forget: add supportedModes + wrap memory manager calls in try-catch so filesystem/model errors surface as user-friendly messages instead of raw JSON-RPC errors - /dream: add supportedModes + document that onComplete callback (dream metadata tracking) is not invoked in ACP mode Known limitation: /dream's onComplete (writeDreamManualRun) is silently skipped in ACP — the auto-dream scheduler may not know a manual dream already ran. Accepted because eagerly calling it would record completion before consolidation actually finishes. Refs #4514
…ertions - Call writeDreamManualRun eagerly before returning submit_prompt so auto-dream dedup works correctly in ACP mode (timestamp is slightly early but acceptable for scheduler min_hours check) - Switch supportedModes test assertions from toContain to toEqual per codebase convention (catches accidental mode additions) Refs #4514
- dreamCommand: add try-catch for error resilience in ACP; make eager writeDreamManualRun conditional on executionMode === 'acp' to avoid double-write in interactive mode and cancel-semantics regression - rememberCommand: add explicit if (!config) guard (consistency with dream/forget; avoids silent fallthrough in ACP) - Add config:null test for rememberCommand - Split dream test into interactive (no eager write) vs ACP (eager write) Refs #4514
|
@/tmp/stage-1.md |
📋 Review SummaryThis PR enables 🔍 General Feedback
🎯 Specific Feedback🟢 Medium
🔵 Low
✅ Highlights
|
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
PR description doesn't follow the pull request template — see my comment above for details. Please update the description to match the template structure.
There was a problem hiding this comment.
Pull request overview
This PR enables the /remember, /forget, and /dream slash commands to run in ACP (web-shell) mode by explicitly declaring ACP support, and improves UX in ACP by converting thrown errors into clearer, user-facing command error messages.
Changes:
- Allow
/remember,/forget,/dreamto be discoverable/executable in ACP mode viasupportedModes: ['interactive', 'acp']. - Add defensive error handling in
/forgetand/dreamso failures are surfaced as readable command errors. - Adjust
/dreambookkeeping to record manual dream runs eagerly in ACP (whereonCompleteis not invoked), and add targeted unit tests for these behaviors.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/ui/commands/rememberCommand.ts | Adds ACP support and a consistent !config guard; keeps managed/non-managed memory flows. |
| packages/cli/src/ui/commands/rememberCommand.test.ts | Adds coverage for argument validation, missing config, managed vs non-managed behavior, and ACP support declaration. |
| packages/cli/src/ui/commands/forgetCommand.ts | Adds ACP support and wraps memory-manager operations in try/catch to return user-friendly failures. |
| packages/cli/src/ui/commands/forgetCommand.test.ts | Adds coverage for argument validation, missing config, success/fallback, error handling, and ACP support declaration. |
| packages/cli/src/ui/commands/dreamCommand.ts | Adds ACP support, wraps prompt construction in try/catch, and records dream metadata eagerly in ACP. |
| packages/cli/src/ui/commands/dreamCommand.test.ts | Extends coverage for ACP support declaration, missing config, interactive vs ACP metadata write behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Without argumentHint, ACP clients advertise these commands as taking no input, so users can't provide the required text argument. Refs #4514
wenshao
left a comment
There was a problem hiding this comment.
Incremental review at 843b82d — new commit adds argumentHint to /remember and /forget for the ACP command palette. Clean, safe additions. ✅
Previous round suggestions (fire-and-forget observability, test coverage) at 7940f31 remain unresolved and visible in the thread above.
— qwen3.7-max via Qwen Code /review
…n test - ACP mode returns without onComplete (eliminates double-execution risk if someone later propagates onComplete in handleCommandResult) - Add test for writeDreamManualRun rejection (verifies .catch prevents unhandled promise rejection) - Add return value + no-onComplete assertions to ACP test Refs #4514
Review response round 3 — 6221642
|
✅ Local Verification Report — PR #4819feat(cli): enable /remember, /forget, /dream in ACP mode Environment
Build Verification
Test Results
Pre-existing Failures (NOT introduced by this PR)
None of the failing tests are in files modified by this PR. Review Issues — All Addressed
Code Review Summary
Verdict✅ PASS — All 16 command tests pass, no regressions introduced, review feedback addressed across 3 fix commits. Verified by wenshao |
| content: prompt, | ||
| onComplete: recordDream, | ||
| }; | ||
| } catch (error) { |
There was a problem hiding this comment.
[Suggestion] The catch branch (lines 57–65) added by this PR has no test coverage. The existing ACP error test (dreamCommand.test.ts:91) mocks writeDreamManualRun to reject, but that promise runs outside this try/catch via the detached .catch(() => {}) — so it never exercises this catch block. A synchronous throw from buildConsolidationPrompt (e.g., corrupted memory index) or any of the Storage/getAutoMemoryRoot calls would hit this path with zero test verification.
| } catch (error) { | |
| } catch (error) { | |
| return { | |
| type: 'message', | |
| messageType: 'error', | |
| content: t('Failed to process /dream: {{message}}', { | |
| message: error instanceof Error ? error.message : String(error), | |
| }), | |
| }; | |
| } | |
| }, | |
| }; |
Suggested test:
it('returns error when buildConsolidationPrompt throws', async () => {
const context = createMockCommandContext({
services: {
config: {
getProjectRoot: vi.fn().mockReturnValue(path.join('tmp', 'p')),
getMemoryManager: vi.fn().mockReturnValue({
buildConsolidationPrompt: vi.fn().mockImplementation(() => {
throw new Error('corrupted memory index');
}),
}),
getSessionId: vi.fn().mockReturnValue('session-1'),
},
},
});
const result = await dreamCommand.action?.(context, '');
expect(result).toEqual({
type: 'message',
messageType: 'error',
content: expect.stringContaining('corrupted memory index'),
});
});— qwen3.7-max via Qwen Code /review
| const result = rememberCommand.action?.(context, 'user prefers dark mode'); | ||
| expect(result).toMatchObject({ | ||
| type: 'submit_prompt', | ||
| content: expect.stringContaining('user prefers dark mode'), |
There was a problem hiding this comment.
[Suggestion] The managed-memory test only verifies the user fact ('user prefers dark mode') appears in the prompt, but does not verify the dirHint — the memory directory path from getAutoMemoryRoot() that is the key behavioral difference between the managed-memory and QWEN.md-fallback paths. A bug that drops dirHint from the prompt would pass this test.
| content: expect.stringContaining('user prefers dark mode'), | |
| const result = rememberCommand.action?.(context, 'user prefers dark mode'); | |
| expect(result).toMatchObject({ | |
| type: 'submit_prompt', | |
| content: expect.stringMatching(/user prefers dark mode.*\.qwen\/memory|\.qwen\/memory.*user prefers dark mode/s), | |
| }); |
Or split into two assertions:
expect(result).toMatchObject({
type: 'submit_prompt',
content: expect.stringContaining('user prefers dark mode'),
});
expect((result as any).content).toContain('.qwen/memory');— qwen3.7-max via Qwen Code /review
Summary
/remember,/forget,/dreamslash commands in ACP mode by addingsupportedModes: ['interactive', 'acp']declarations, plus error handling and config guards.filterCommandsForMode('acp')excluded them (defaulted to['interactive']only).dreamCommand.tsconditional eagerrecordDreamlogic (ACP fire-and-forget vs interactiveonComplete).Validation
filterCommandsForMode('acp')resultssupportedModesincludes'acp'in each command fileScope / Risk
/dream'swriteDreamManualRunis fire-and-forget in ACP mode — if it fails silently, auto-dream scheduler may not know a manual dream ran (acceptable for dedup timing)Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Refs #4514
Re-targeting of #4811 (incorrectly merged to
mainand reverted). Same code, correct base branch (daemon_mode_b_main).🤖 Generated with Qwen Code