feat: add /chat command for saving, listing, resuming, and deleting named sessions#3105
feat: add /chat command for saving, listing, resuming, and deleting named sessions#3105lnxsun wants to merge 13 commits into
Conversation
|
我已经在本地重新构建了cli.js并逐一测试了/chat的4个子命令（save、list、resume、delete）的功能，对于一开始存在的resume不能跳转到原save的会话的问题也进行了修正和复测通过，确认都能实现预期功能，请测试并拉取 |
| * 读取索引文件 | ||
| * @returns 索引对象，如果文件不存在则返回空对象 | ||
| */ | ||
| export async function readChatIndex(): Promise<ChatIndex> { |
There was a problem hiding this comment.
[Critical] readChatIndex() currently turns every read/parse failure into {}. That means permission errors and other I/O failures are silently reported to callers as "no saved sessions" / "session not found", which makes the new /chat commands lose data visibility without any actionable error.
| export async function readChatIndex(): Promise<ChatIndex> { | |
| export async function readChatIndex(): Promise<ChatIndex> { | |
| try { | |
| const content = await fs.readFile(getIndexPath(), 'utf-8'); | |
| return JSON.parse(content) as ChatIndex; | |
| } catch (error) { | |
| if (error instanceof SyntaxError) { | |
| return {}; | |
| } | |
| if ( | |
| typeof error === 'object' && | |
| error !== null && | |
| 'code' in error && | |
| error.code === 'ENOENT' | |
| ) { | |
| return {}; | |
| } | |
| throw error; | |
| } | |
| } |
— gpt-5.4 via Qwen Code /review
| /** | ||
| * 获取索引文件路径 | ||
| */ | ||
| function getIndexPath(): string { |
There was a problem hiding this comment.
[Critical] This index is written to a single global ~/.qwen/chat-index.json, but session lookup/removal is project-scoped through SessionService(cwd). In practice that means /chat list can show names saved in other repos that /chat resume cannot load here, and the index also ignores custom runtime roots such as QWEN_RUNTIME_DIR.
| function getIndexPath(): string { | |
| function getIndexPath(): string { | |
| return path.join(new Storage(process.cwd()).getProjectDir(), 'chat-index.json'); | |
| } |
The exact storage helper may differ, but the index needs to follow the same project/runtime scoping rules as session storage.
— gpt-5.4 via Qwen Code /review
| if (!deleted) { | ||
| return { | ||
| type: 'message', | ||
| messageType: 'error', |
There was a problem hiding this comment.
[Critical] /chat delete <name> only removes the alias from chat-index.json; it never deletes the underlying session file. That makes the command behave like "unlink alias" rather than "delete session", which is surprising for users and leaves conversation data on disk.
| messageType: 'error', | |
| const sessionId = await getSessionIdByName(name); | |
| if (!sessionId) { | |
| return { | |
| type: 'message', | |
| messageType: 'error', | |
| content: t('Session "{{name}}" not found.', { name }), | |
| }; | |
| } | |
| const config = context.services.config; | |
| if (!config) { | |
| return { | |
| type: 'message', | |
| messageType: 'error', | |
| content: t('Config not loaded.'), | |
| }; | |
| } | |
| const sessionService = new SessionService(config.getTargetDir()); | |
| const removed = await sessionService.removeSession(sessionId); | |
| if (!removed) { | |
| return { | |
| type: 'message', | |
| messageType: 'error', | |
| content: t('Session "{{name}}" not found.', { name }), | |
| }; | |
| } | |
| await deleteSessionFromIndex(name); |
— gpt-5.4 via Qwen Code /review
| 'test-session-1', | ||
| ); | ||
|
|
||
| expect(result).toEqual({ |
There was a problem hiding this comment.
[Critical] This test is still asserting the old resume behavior, but the implementation now returns a dialog: 'resume' action and requires a valid CommandContext. The focused test run is currently red because this case calls resumeCommand.action(undefined as any, ...) and then expects an info message containing Found session.
| expect(result).toEqual({ | |
| const mockContext: CommandContext = { | |
| services: { | |
| config: { | |
| getTargetDir: () => '/tmp/project', | |
| }, | |
| }, | |
| ui: {} as never, | |
| executionMode: 'non_interactive', | |
| }; | |
| const result = await resumeCommand?.action!(mockContext, 'test-session-1'); | |
| expect(result).toEqual({ | |
| type: 'dialog', | |
| dialog: 'resume', | |
| params: { sessionId: 'test-session-id-12345' }, | |
| }); |
— gpt-5.4 via Qwen Code /review
|
wenshao 已经针对您提出的4个问题重新完善并测试通过 |
wenshao
left a comment
There was a problem hiding this comment.
Note: packages/cli/src/ui/contexts/UIActionsContext.tsx:97 declares openResumeDialog: () => void but useResumeCommand.ts and slashCommandProcessor.ts both use (sessionId?: string) => void. Please update the interface to match: openResumeDialog: (sessionId?: string) => void;
|
|
||
| // Mock project directory | ||
| const mockProjectDir = vi.hoisted(() => ({ | ||
| path: path.join(process.env['TEMP'] || '/tmp', 'mock-project-test'), |
There was a problem hiding this comment.
[Critical] vi.hoisted() executes before module imports, so path.join(...) fails because path is not yet defined. This causes the entire test suite to fail at startup — zero tests actually run.
| path: path.join(process.env['TEMP'] || '/tmp', 'mock-project-test'), | |
| const mockProjectDir = vi.hoisted(() => ({ | |
| path: (process.env['TEMP'] || '/tmp') + '/mock-project-test', | |
| })); |
— qwen3.6-plus via Qwen Code /review
| const { SessionService } = await import('@qwen-code/qwen-code-core'); | ||
| vi.mocked(SessionService).mockImplementationOnce( | ||
| () => | ||
| ({ |
There was a problem hiding this comment.
[Critical] as unknown as typeof SessionService casts to the constructor type, but mockImplementationOnce should return an instance shape. This causes a TypeScript error (TS2740).
| ({ | |
| }) as unknown as SessionService, |
— qwen3.6-plus via Qwen Code /review
| return t('Save the current session with a name.'); | ||
| }, | ||
| kind: CommandKind.BUILT_IN, | ||
| action: async ( |
There was a problem hiding this comment.
[Critical] Session names from args.trim() are used directly with no validation. Names containing /, \, null bytes, or excessively long strings could corrupt the index JSON or cause unexpected filesystem behavior.
| action: async ( | |
| const name = args.trim(); | |
| if (!name) { | |
| return { | |
| type: 'message', | |
| messageType: 'error', | |
| content: t('Please provide a name. Usage: /chat save <name>'), | |
| }; | |
| } | |
| if (!/^[a-zA-Z0-9_\-\.]+$/.test(name) || name.length > 128) { | |
| return { | |
| type: 'message', | |
| messageType: 'error', | |
| content: t('Invalid session name. Use only letters, numbers, hyphens, underscores, and dots.'), | |
| }; | |
| } |
— qwen3.6-plus via Qwen Code /review
| } | ||
| }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
[Critical] The delete command immediately removes the session file and index entry without any user confirmation. A typo like /chat delete important-session causes irreversible data loss. Other destructive operations in the codebase use confirmation dialogs.
Consider returning a confirm_action result before proceeding with deletion, matching the pattern used by other destructive commands.
— qwen3.6-plus via Qwen Code /review
| import { QWEN_DIR } from '../config/storage.js'; | ||
|
|
||
| /** | ||
| * 会话索引数据结构 |
There was a problem hiding this comment.
[Suggestion] All JSDoc comments in this file are in Chinese (e.g., /** 会话索引数据结构 */), while the rest of the codebase uses English exclusively. Please translate to match project conventions.
— qwen3.6-plus via Qwen Code /review
| export async function readChatIndex(projectDir: string): Promise<ChatIndex> { | ||
| try { | ||
| const content = await fs.readFile(getIndexPath(projectDir), 'utf-8'); | ||
| return JSON.parse(content) as ChatIndex; |
There was a problem hiding this comment.
[Suggestion] JSON.parse(content) as ChatIndex assumes the result is Record<string, string>. If the file is corrupted (e.g., { "foo": 123 }), callers like listNamedSessions will crash on sessions[name].substring(0, 8).
| return JSON.parse(content) as ChatIndex; | |
| export async function readChatIndex(projectDir: string): Promise<ChatIndex> { | |
| try { | |
| const content = await fs.readFile(getIndexPath(projectDir), 'utf-8'); | |
| const parsed = JSON.parse(content); | |
| if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { | |
| throw new Error('Invalid chat index format'); | |
| } | |
| for (const [key, value] of Object.entries(parsed)) { | |
| if (typeof value !== 'string') { | |
| throw new Error(`Invalid entry in chat index: ${key}`); | |
| } | |
| } | |
| return parsed as ChatIndex; | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code === 'ENOENT') { | |
| return {}; | |
| } | |
| throw error; | |
| } | |
| } |
— qwen3.6-plus via Qwen Code /review
|
|
||
| const openResumeDialog = useCallback( | ||
| (sessionId?: string) => { | ||
| if (sessionId) { |
There was a problem hiding this comment.
[Suggestion] handleResume(sessionId) is async but called without await or .catch(). If loadSession throws (not returns undefined), it becomes an unhandled promise rejection.
| if (sessionId) { | |
| if (sessionId) { | |
| handleResume(sessionId).catch((err) => { | |
| // Log or handle error appropriately | |
| }); |
— qwen3.6-plus via Qwen Code /review
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
[Suggestion] saveSessionToIndex and deleteSessionFromIndex perform a read-modify-write pattern without any file locking or atomicity. Concurrent saves could lose data. At minimum, document the concurrency assumption. For robustness, write to a temp file and fs.rename atomically.
— qwen3.6-plus via Qwen Code /review
| expect(result.args).toBe('my-session'); | ||
| expect(result.canonicalPath).toEqual(['chat', 'resume']); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[Suggestion] Tests only check return messages but never verify that mocked functions (saveSessionToIndex, deleteSessionFromIndex, etc.) were called with correct arguments. Add expect(saveSessionToIndex).toHaveBeenCalledWith(...) assertions to ensure the command actually interacts with its dependencies.
— qwen3.6-plus via Qwen Code /review
|
已修复 `openResumeDialog` 类型签名不匹配的问题： |
|
已修复所有 review 提出的问题： |
…on, and code quality
第二轮修复 - 已完成并推送 ✅ |
第三轮改进 - 基于本地 Review ✅ |
第四轮改进 - 本地 Review 后续修复 ✅ |
第五轮改进 - 文档 + 防御性编程 ✅ |
| // Limit to 128 characters | ||
| if (name.length > 128) { | ||
| return 'chat.session_name_too_long'; | ||
| } |
There was a problem hiding this comment.
[Critical] __proto__ passes the validation regex /^[a-zA-Z0-9_.-]+$/ but silently corrupts the chat index. When saveSessionToIndex executes index['__proto__'] = sessionId, it sets the prototype instead of creating an own property — Object.keys() returns [] and JSON.stringify() produces {}, losing all saved session mappings.
| } | |
| // Only allow letters, numbers, hyphens, underscores, and dots | |
| // Reject prototype-polluting names | |
| if (!/^[a-zA-Z0-9_.-]+$/.test(name) || name === '__proto__' || name === 'constructor' || name === 'prototype') { | |
| return 'chat.invalid_session_name'; | |
| } |
Alternatively, use Object.create(null) for the index object in chatIndex.ts so that __proto__ is treated as a normal key.
— glm-5.1 via Qwen Code /review
| } | ||
|
|
||
| // User confirmed deletion - delete the actual session file | ||
| const sessionService = new SessionService(projectDir); |
There was a problem hiding this comment.
[Critical] /chat delete removes the underlying session file via sessionService.removeSession(sessionId) without checking if other saved names reference the same session ID. The same session can be saved under multiple names (e.g., /chat save draft then /chat save backup). Deleting one name removes the file on disk, orphaning all other names that still point to it.
| const sessionService = new SessionService(projectDir); | |
| // Only remove the session file if no other name references it | |
| const allSessions = await listNamedSessions(projectDir); | |
| const otherRefs = Object.entries(allSessions) | |
| .filter(([n, id]) => n !== name && id === sessionId); | |
| if (otherRefs.length === 0) { | |
| const sessionService = new SessionService(projectDir); | |
| await sessionService.removeSession(sessionId); | |
| } | |
| // Always remove the specific name from the index | |
| const indexDeleted = await deleteSessionFromIndex(projectDir, name); |
— glm-5.1 via Qwen Code /review
| * @param content File content | ||
| * @prerequisite The parent directory of filePath must exist | ||
| */ | ||
| async function atomicWrite(filePath: string, content: string): Promise<void> { |
There was a problem hiding this comment.
[Suggestion] The local atomicWrite duplicates the existing atomicWriteJSON utility from packages/core/src/utils/atomicFileWrite.ts. The existing utility also has exponential backoff retries for Windows EPERM/EACCES errors, which this implementation lacks.
| async function atomicWrite(filePath: string, content: string): Promise<void> { | |
| import { atomicWriteJSON } from '../utils/atomicFileWrite.js'; | |
| // Replace: | |
| // await atomicWrite(getIndexPath(projectDir), JSON.stringify(index, null, 2)); | |
| // With: | |
| // await atomicWriteJSON(getIndexPath(projectDir), index); | |
| // Then remove the local atomicWrite function and the crypto import. |
— glm-5.1 via Qwen Code /review
|
|
||
| let sessionData; | ||
| try { | ||
| sessionData = await sessionService.loadSession(sessionId); |
There was a problem hiding this comment.
[Suggestion] sessionService.loadSession(sessionId) reads and parses the entire JSONL file to verify existence, then handleResume in useResumeCommand.ts calls it again — doubling I/O cost. SessionService exposes a lighter sessionExists method that reads only the first line.
| sessionData = await sessionService.loadSession(sessionId); | |
| const sessionService = new SessionService(projectDir); | |
| const exists = await sessionService.sessionExists(sessionId); | |
| if (!exists) { | |
| return { | |
| type: 'message', | |
| messageType: 'error', | |
| content: t( | |
| 'Session data for "{{name}}" could not be loaded. The session file may have been deleted.', | |
| { name }, | |
| ), | |
| }; | |
| } |
— glm-5.1 via Qwen Code /review
第六轮改进 - 本地试用发现问题并修复 ✅ |
第七轮改进 - 修复 Critical 安全和性能问题 ✅ |
|
感谢@pomelo-nwu 指出这一点！我注意到 PR #1113 已经移除了 /chat 命令，转而采用 --continue/--resume 参数。
|
@tanzhenxin 感谢您的解释，加上 #3093 中的rename、delete相当于实现了/chat的功能 |
|
感谢你的贡献！我最近参考 Claude Code 的设计做了一些 session 方面的能力对齐。 你这个 PR 里有几个地方我也做了采纳：
|
目标：
我想给qwen code开发一个/chat命令，用其来保存、查看、恢复和删除会话。
实现的
/chat子命令：**/chat save [name]- 给当前会话命名保存/chat list- 列出已命名保存的会话/chat resume [name]- 按名字恢复会话/chat delete [name]- 删除已命名的会话Related to iflow cli的好功能接过来 #3025