feat(config): add todosDirectory setting for project-local todo persistence#5928
feat(config): add todosDirectory setting for project-local todo persistence#5928mvanhorn wants to merge 2 commits into
Conversation
…stence Add a todosDirectory setting mirroring plansDirectory so todos written by the todo-write tool can be persisted inside the project (e.g. .qwen/todos) and synced via Git. When unset, todos continue to use the global runtime todos directory, so default behavior is unchanged. Closes QwenLM#5836
|
@qwen-code /triage |
|
Thanks for the PR, @mvanhorn! Template looks good ✓ — all required sections present and filled in. On direction: this is a clean fit. Issue #5836 explicitly requested On approach: the scope is tight and proportional. +203/-16 across 10 files, every change directly serves the stated goal. The Moving on to code review. 🔍 中文说明感谢贡献，@mvanhorn！ 模板完整 ✓ — 所有必填部分均已填写。 方向：完全契合。Issue #5836 明确要求按照 方案：范围精准。10 个文件 +203/-16 行，每处改动都服务于目标。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewClean implementation. No correctness bugs, security issues, or over-abstraction. Test coverage is proportional: 4 new Unit test results (PR code):
Real-Scenario TestingThe Before (installed qwen)After (this PR)All three behaviors verified:
中文说明代码审查实现干净。 无正确性 bug、安全问题或过度抽象。测试覆盖合理：4 个新的 真实场景测试CI 环境中 LLM 没有 已验证的三种行为：
— Qwen Code · qwen3.7-max |
|
This PR does exactly what it should, and nothing it shouldn't. The implementation is a clean mirror of the existing My independent proposal from the code review stage matched the PR's approach closely. One small thing I noted: the PR is actually slightly more elegant than Integration testing confirms the three key behaviors: default fallback to the runtime directory (no behavior change when unset), relative path resolution against the project root, and The one testing limitation is that full end-to-end CLI testing (actually triggering LGTM, approving. ✅ 中文说明这个 PR 做了该做的事，没有多余的东西。实现干净地镜像了现有的 我在代码审查阶段的独立方案与 PR 的方法高度吻合。一个细节值得注意：PR 实际上比 集成测试确认了三个关键行为：默认回退到运行时目录（未设置时无行为变化）、相对路径基于项目根目录解析、逃逸尝试时抛出 唯一的测试限制是 CI 环境中无法进行完整的端到端 CLI 测试（未配置 LGTM，批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
|
||
| // 4. Write new todos AFTER all validation passes | ||
| await writeTodosToFile(finalTodos, sessionId); | ||
| await writeTodosToFile(todoDir, finalTodos, sessionId); |
There was a problem hiding this comment.
[Critical] The configured todosDirectory is only realpath-contained when Config is constructed, but this write path later reuses the cached directory without revalidating it or the final file path. If .qwen/todos is replaced by a symlink after startup, fs.mkdir / atomicWriteFile will follow it and persist the todo JSON outside targetDir, which breaks the new project-root containment guarantee. The plan storage path handles the same risk by re-checking before I/O and checking the final file after write. Please mirror that for todos: track whether todosDirectory was configured, build todo file paths through a helper that can assert containment, re-run Storage.assertPathWithinDirectory before read/existence/write paths, and verify or roll back after writes.
— GPT-5 via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Re-affirming Request changes: the open [Critical] containment concern at todoWrite.ts:422 (configured todosDirectory validated only at construction, not re-checked or rolled back before I/O) is the merge blocker and is still unaddressed.
The PR describes itself as mirroring plansDirectory "exactly," but several safety/UX mechanisms plansDirectory carries were not brought across. Three are inline above (session-id sanitization, the legacy-location migration warning, the docs .gitignore note). Three more that don't anchor to a single changed line:
- SDK read/write split (latent) —
readTodosForSession/listTodoSessions(todoWrite.ts:537,545) callStorage.getTodosDir()with no args, so they always read the global runtime dir even when a project setstodosDirectory. You correctly scoped this out in Risk & Scope and there are no in-repo callers today, but the next caller wiring these into session-recovery/webui gets silently-empty results. A one-line code comment at the call sites would make the landmine visible in-file, not just in the PR description. - Test parity —
plansDirectoryhas aloadCliConfig plansDirectorytest inpackages/cli/src/config/config.test.tsand three symlink-escape tests forgetPlansDirinstorage.test.ts.todosDirectoryhas neither: no CLI-loader/Config-level test, and thegetTodosDirsuite stubsrealpathto the real fs, so only the lexical../escape is exercised — the symlink-containment limb (the one relevant to the :422 concern) is untested. - Does this actually deliver #5836? Todo files are named
${sessionId}.jsonwheresessionIdis a fresh per-process UUID. Committing.qwen/todosto git therefore does not give the cross-device "resume my task list" outcome the issue asks for: machine B starts a new session UUID and never reads the committed file. The storage-location half ships; the retrieval half (a stable, project-scoped key) does not. Worth confirming whether that's in scope or a follow-up — otherwiseFixes #5836overstates what lands.
Minor (non-blocking): the default is documented as ~/.qwen/todos but resolves to getRuntimeBaseDir()/todos, which differs when QWEN_RUNTIME_DIR is set; and the new todosDirectory? field in ConfigParameters lacks the JSDoc the adjacent plansDirectory? has.
Verified locally: storage.test.ts 67 tests pass and ESLint is clean on the changed sources. (The other two suites failed only on a pre-existing ajv version mismatch in the review sandbox, unrelated to this diff.)
中文版
维持 Request changes：todoWrite.ts:422 上那条仍未处理的 [Critical]（todosDirectory 只在构造时校验、I/O 前未重新校验/写后未回滚）才是合并阻塞项。
本 PR 自述"完全镜像 plansDirectory"，但 plansDirectory 带的若干安全/体验机制没有一并搬过来。三条在上方行内（session-id 净化、legacy 目录迁移告警、文档 .gitignore 提示）。另有三条无法锚定到单一改动行：
- SDK 读写错位（潜在） —
readTodosForSession/listTodoSessions（todoWrite.ts:537,545）无参调用Storage.getTodosDir()，即使项目配置了todosDirectory也永远读全局运行时目录。你已在 Risk & Scope 里说明、且当前无仓内调用方，但下一个把它接进 session 恢复/webui 的人会读到空结果。在调用处加一行代码注释，能让这个坑出现在文件里而不只在 PR 描述里。 - 测试对等 —
plansDirectory有loadCliConfig plansDirectory测试（packages/cli/src/config/config.test.ts）和getPlansDir的三个软链逃逸测试（storage.test.ts）。todosDirectory两者皆无：没有 CLI 装载/Config 层测试；getTodosDir套件把realpath打桩成真实 fs，只覆盖了字面../，软链包含校验那条分支（恰是 :422 关心的）未被测试。 - 真能解决 #5836 吗？ todo 文件名是
${sessionId}.json，而sessionId是每进程新生成的 UUID。把.qwen/todos提交进 git 并不能带来 issue 想要的"跨设备恢复任务清单"：机器 B 起的是新 session UUID，根本不会读那个提交过去的文件。落盘位置这一半实现了，检索这一半（稳定的、项目级的 key）没有。建议确认这是否在本 PR 范围内，否则Fixes #5836名不副实。
次要（不阻塞）：文档把默认写成 ~/.qwen/todos，实际解析为 getRuntimeBaseDir()/todos，设了 QWEN_RUNTIME_DIR 就不一样；ConfigParameters 里新增的 todosDirectory? 也缺了相邻 plansDirectory? 那样的 JSDoc。
本地验证：storage.test.ts 67 个用例通过，改动源文件 ESLint 干净。（另两个套件仅因评审沙箱里 ajv 版本不匹配而加载失败，与本 diff 无关。）
— claude-opus-4-8 via Claude Code /qreview
|
|
||
| function getTodoFilePath(todoDir: string, sessionId?: string): string { | ||
| // Use sessionId if provided, otherwise fall back to 'default' | ||
| const filename = `${sessionId || 'default'}.json`; |
There was a problem hiding this comment.
[Suggestion] This builds the todo file name from the raw sessionId. The plansDirectory pattern this PR mirrors routes the id through Storage.sanitizePlanSessionId() first (strips /, \, leading dots, Windows-invalid chars) precisely because the session id is untrusted SDK input — a ..-bearing id would otherwise resolve outside the now project-local todosDir and defeat the Storage.getTodosDir containment guard.
Not exploitable today: every sessionId ingress (--session-id, ACP/serve session/load/resume) validates against a UUID / SESSION_FILE_PATTERN regex before it reaches Config, so a traversal id can't arrive. So this is defense-in-depth that plans has and todos drops — suggest a Storage.sanitizeTodoSessionId() mirroring sanitizePlanSessionId, applied here. (Complements the open containment concern at line 422 — a post-write final-path re-check would also catch it.)
— claude-opus-4-8 via Claude Code /qreview
| this.targetDir = path.resolve(params.targetDir); | ||
| this.plansDirectoryConfigured = Boolean(params.plansDirectory?.trim()); | ||
| this.plansDir = Storage.getPlansDir(this.targetDir, params.plansDirectory); | ||
| this.todosDir = Storage.getTodosDir(this.targetDir, params.todosDirectory); |
There was a problem hiding this comment.
[Suggestion] When todosDirectory is set, the parallel plansDirectory path warns that files in the legacy location are now unreferenced — addLegacyPlanLocationWarning() (config.ts ~4137, called from the constructor ~1633, surfaced via getWarnings()). There is no addLegacyTodoLocationWarning equivalent, so a user who opts in silently stops seeing their existing ~/.qwen/todos/*.json: no warning, no log, the session just starts with an empty todo list. This contradicts the PR's "migration notes: none."
Suggest mirroring the plans warning — guard on a todosDirectoryConfigured flag, compare Storage.getRuntimeBaseDir()/todos (runtime base, not getGlobalQwenDir()) against the configured dir, and push a "move your existing todo files" warning.
— claude-opus-4-8 via Claude Code /qreview
| | `advanced.excludedEnvVars` | array of strings | Environment variables to exclude from project context. Specifies environment variables that should be excluded from being loaded from project `.env` files. This prevents project-specific environment variables (like `DEBUG=true`) from interfering with the CLI behavior. Variables from `.qwen/.env` files are never excluded. | `["DEBUG","DEBUG_MODE"]` | | ||
| | `advanced.bugCommand` | object | Configuration for the bug report command. Overrides the default URL for the `/bug` command. Properties: `urlTemplate` (string): A URL that can contain `{title}` and `{info}` placeholders. Example: `"bugCommand": { "urlTemplate": "https://bug.example.com/new?title={title}&info={info}" }` | `undefined` | | ||
| | `plansDirectory` | string | Custom directory for approved Plan Mode files. Relative paths are resolved from the project root, and the resolved path must stay within the project root. If unset, plan files are stored in `~/.qwen/plans`. **Requires restart.** If the directory is inside the project root, add it to `.gitignore` to avoid committing plan files. | `undefined` | | ||
| | `todosDirectory` | string | Custom directory for todo list files created by the todo-write tool. Relative paths are resolved from the project root, and the resolved path must stay within the project root. If unset, todo files are stored in the global runtime todos directory (`~/.qwen/todos`). **Requires restart.** | `undefined` | |
There was a problem hiding this comment.
[Suggestion] The parallel plansDirectory row (line 465) ends with "add it to .gitignore to avoid committing plan files." This row drops that guidance — yet the example at line 523 uses "todosDirectory": "./.qwen/todos" (inside the project root), which would commit per-session todo JSON (named by session UUID) into the repo.
| | `todosDirectory` | string | Custom directory for todo list files created by the todo-write tool. Relative paths are resolved from the project root, and the resolved path must stay within the project root. If unset, todo files are stored in the global runtime todos directory (`~/.qwen/todos`). **Requires restart.** | `undefined` | | |
| | `todosDirectory` | string | Custom directory for todo list files created by the todo-write tool. Relative paths are resolved from the project root, and the resolved path must stay within the project root. If unset, todo files are stored in the global runtime todos directory (`~/.qwen/todos`). **Requires restart.** If the directory is inside the project root, add it to `.gitignore` to avoid committing todo files. | `undefined` | |
— claude-opus-4-8 via Claude Code /qreview
| clearContextOnIdle: settings.context?.clearContextOnIdle, | ||
| fileFiltering: settings.context?.fileFiltering, | ||
| plansDirectory: settings.plansDirectory, | ||
| todosDirectory: settings.todosDirectory, |
There was a problem hiding this comment.
[Suggestion] This is the only place where the real CLI settings object is wired into Config, but the new todosDirectory pass-through has no integration test. The storage tests cover Storage.getTodosDir() directly, and the tool tests use a mocked getTodosDir(), so a future typo or omission here could make the documented setting silently ignored while those tests still pass. Please add loadCliConfig todosDirectory tests beside the existing plansDirectory block that verify a relative value resolves against cwd via config.getTodosDir() and that ../todos rejects with the containment error.
— GPT-5 via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
No additional findings beyond those already raised. The existing Critical from wenshao (missing containment re-check at the I/O site in todoWrite.ts after directory resolution, before the actual file write) and the coverage parity gaps vs plansDirectory (session-id sanitization, symlink-escape tests) are the primary blockers well-covered by the existing CHANGES_REQUESTED review. The known SDK limitation (readTodosForSession/listTodoSessions calling Storage.getTodosDir() with no args) is acknowledged in the PR description.
— claude-sonnet-4-6 via Qwen Code /review
Generated by Claude Code
| @@ -533,15 +534,15 @@ Todo list modification failed with error: ${errorMessage}. You may need to retry | |||
| export async function readTodosForSession( | |||
| sessionId?: string, | |||
| ): Promise<TodoItem[]> { | |||
There was a problem hiding this comment.
[Critical] readTodosForSession calls Storage.getTodosDir() with no arguments, which always returns the default runtime todos directory. It never consults the user-configured todosDirectory. Session recovery will silently read from the wrong location whenever a custom todosDirectory is set.
| ): Promise<TodoItem[]> { | |
| Add a `todoDir` parameter: `export async function readTodosForSession(sessionId?: string, todoDir?: string): Promise<TodoItem[]> { return readTodosFromFile(todoDir ?? Storage.getTodosDir(), sessionId); }` — callers that have a `Config` instance should pass `config.getTodosDir()`. |
— claude-sonnet-4-6 via Qwen Code /review
Generated by Claude Code
| /** | ||
| * Utility function to list all todo files in the todos directory | ||
| */ | ||
| export async function listTodoSessions(): Promise<string[]> { |
There was a problem hiding this comment.
[Critical] listTodoSessions calls Storage.getTodosDir() with no arguments, which always returns the default runtime todos directory and ignores any configured todosDirectory. A user who sets a custom todosDirectory would see an empty session list from this function.
| export async function listTodoSessions(): Promise<string[]> { | |
| Add a `todoDir` parameter: `export async function listTodoSessions(todoDir?: string): Promise<string[]>` and replace the `Storage.getTodosDir()` call with `todoDir ?? Storage.getTodosDir()`. Callers with a `Config` instance should pass `config.getTodosDir()`. |
— claude-sonnet-4-6 via Qwen Code /review
Generated by Claude Code
| this.targetDir = path.resolve(params.targetDir); | ||
| this.plansDirectoryConfigured = Boolean(params.plansDirectory?.trim()); | ||
| this.plansDir = Storage.getPlansDir(this.targetDir, params.plansDirectory); | ||
| this.todosDir = Storage.getTodosDir(this.targetDir, params.todosDirectory); |
There was a problem hiding this comment.
[Suggestion] Missing Config-level test coverage for todosDirectory wiring.
The parallel plansDirectory has a full Config-level test suite in config.test.ts (~lines 5015–5283) covering constructor path resolution, escape-path rejection, and getter behavior. No equivalent exists for todosDirectory — the Storage.getTodosDir unit tests in storage.test.ts cover the helper in isolation, but the Config constructor's threading of params.todosDirectory → Storage.getTodosDir(this.targetDir, ...) → this.todosDir → getTodosDir() is untested.
A regression in the constructor (e.g., passing params.todosDirectory in the wrong position, or using this.cwd instead of this.targetDir) would silently produce a wrong directory with no test catching it.
Consider adding two tests mirroring the plansDirectory ones:
- Construct
ConfigwithtodosDirectory: './.qwen/todos'and assertconfig.getTodosDir()equalspath.join(targetDir, '.qwen/todos') - Construct with
todosDirectory: '../todos'and assert the constructor throwsFatalConfigError
— qwen3.7-max via Qwen Code /review
4 participants
What this PR does
Adds a
todosDirectorysetting so todos created by the todo-write tool can be persisted inside the project (for example.qwen/todos) instead of only under the global runtime directory, letting task state be committed and synced via Git across machines and teammates. This mirrors the existingplansDirectorymechanism exactly, as proposed by maintainer @doudouOUC in #5836 ("addtodosDirectoryconfig param following the existingplansDirectorypattern"). When the setting is unset, todos continue to use the existing global runtimetodos/directory, so default behavior is unchanged. The newStorage.getTodosDir(projectRoot, todosDirectory)mirrorsgetPlansDir: it trims the value, resolves it against the project root, and asserts the result stays within the project root (throwingFatalConfigErrorotherwise). The value is threaded throughConfig(atodosDirectoryparam plus agetTodosDir()accessor) and the CLI config loader, andtodoWritenow resolves its read/write directory fromconfig.getTodosDir()instead of the hardcoded runtime path. The packaged VS Code settings JSON schema was regenerated to include the new key.Why it's needed
Todos currently live under
Storage.getRuntimeBaseDir()/todos, outside version control, so project task state is lost when switching machines and cannot be shared with teammates. The reporter asked for an option to persist todos inside the project so they can be synced via Git, matching whatplansalready supports. Opting into a project-local directory makes todos a first-class, syncable part of the repo, consistent with how plans are already handled.Reviewer Test Plan
How to verify
This is a non–user-visible config addition. With
todosDirectoryunset, todos still write to the global runtimetodos/directory (no behavior change). Setting"todosDirectory": ".qwen/todos"causes the todo-write tool to read and write todo JSON within the project root; a relative path is resolved from the project root, and a path escaping the project root throws aFatalConfigError(matching theplansDirectorycontainment guard).Commands run locally:
npx vitest run packages/core/src/config/storage.test.ts packages/core/src/tools/todoWrite.test.ts packages/cli/src/config/settingsSchema.test.ts— 126 tests pass.npm run generate:settings-schema— regenerated and committed the packaged VS Code schema.Added test coverage:
getTodosDirdefault fallback, configured relative-path resolution, out-of-root rejection, and the missing-projectRootguard; the schema key's type/category/default; and todo read/write round-trips honoring the configured directory.Evidence (Before & After)
N/A (non–user-visible config and persistence-path change; verified via unit tests above).
Tested on
Environment (optional)
Local unit tests via vitest; no runtime sandbox required.
Risk & Scope
getTodosDir()returns the same global runtimetodos/path when the setting is absent.readTodosForSession/listTodoSessionsSDK helpers (no Config in scope, no in-repo callers) keep using the default directory; mirrors theplansDirectoryconstructor-caching approach.Linked Issues
Fixes #5836