fix(core): honor runtime output dir for auto memory - #4715
Conversation
| return process.env['QWEN_CODE_MEMORY_BASE_DIR']; | ||
| } | ||
| return Storage.getGlobalQwenDir(); | ||
| return Storage.getRuntimeBaseDir(); |
wenshao
left a comment
There was a problem hiding this comment.
[Critical] _autoMemoryRootCache cross-session contamination in daemon mode — paths.ts:99
_autoMemoryRootCache is keyed only by projectRoot, but the cached value depends on getMemoryBaseDir() → Storage.getRuntimeBaseDir(), which varies per AsyncLocalStorage context (set by runWithRuntimeBaseDir per session in daemon mode). clearAutoMemoryRootCache() is never called in production code.
In daemon mode, when two concurrent sessions have different runtimeOutputDir for the same project root, the first session to call getAutoMemoryRoot() caches its resolved path. The second session silently reads/writes memory in the wrong directory. This also affects isAutoMemPath permission checks in write/edit/read tools.
const _autoMemoryRootCache = new Map<string, string>();
export function getAutoMemoryRoot(projectRoot: string): string {
const cacheKey = `${getMemoryBaseDir()}\0${projectRoot}`;
const cached = _autoMemoryRootCache.get(cacheKey);
if (cached !== undefined) return cached;
[Suggestion] QWEN_CODE_MEMORY_BASE_DIR doesn't resolve paths — paths.ts:89
Returns the raw env var string without path.resolve(). Compare with Storage.getRuntimeBaseDir() which resolves paths via resolvePath(). A tilde-prefixed value (~/my-memory) would be used literally.
return path.resolve(process.env['QWEN_CODE_MEMORY_BASE_DIR']);
[Suggestion] Test coverage gaps — store.test.ts
- No test verifies that
QWEN_RUNTIME_DIRenv var (highest-priority path insidegetRuntimeBaseDir()) drives memory paths correctly throughgetAutoMemoryRoot. - No test checks what happens when
runtimeBaseDirchanges between twogetAutoMemoryRootcalls without clearing the cache — the exact production scenario behind the Critical cache bug above.
— qwen3.7-max via Qwen Code /review
| return process.env['QWEN_CODE_MEMORY_BASE_DIR']; | ||
| } | ||
| return Storage.getGlobalQwenDir(); | ||
| return Storage.getRuntimeBaseDir(); |
There was a problem hiding this comment.
[Critical] Changing from getGlobalQwenDir() to getRuntimeBaseDir() silently relocates auto-memory from ~/.qwen/projects/... to <runtimeOutputDir>/projects/.... There is no migration logic, no symlink, and no fallback read from the old path. Users who have advanced.runtimeOutputDir set in settings.json will experience unexplained loss of all accumulated auto-memory (user preferences, project knowledge, feedback).
Suggested fix: Add a one-time migration — when the new path doesn't exist but the old getGlobalQwenDir()-based path does, either copy/symlink the old tree or fall back to reading from the old path with a deprecation log.
— qwen3.7-max via Qwen Code /review
|
Pushed 389a389 for the review feedback. Changes:
Validation:
|
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
Re-reviewed against the latest commit. The earlier concern — the auto-memory root cache being keyed only by project root, which could serve a stale root once the memory base became runtime-dependent — is resolved: the cache key now includes the runtime base dir (and the local-memory flag), with added test coverage for the multi-runtime case.
Remaining items are low-severity and fine as follow-ups: a release-notes line for users who already set a runtime dir (their existing ~/.qwen memory tree becomes orphaned, not lost), and a docs refresh for the now-relative memory path.
Verdict
APPROVE — correct, runtime-consistent fix for the split-root bug; the one substantive concern from the prior pass is fixed with tests, and only doc/migration polish remains.
Summary
runtimeOutputDirconfiguration #4709.Storage.getRuntimeBaseDir()soruntimeOutputDir/QWEN_RUNTIME_DIRmove the per-project memory tree with other runtime output.QWEN_CODE_MEMORY_BASE_DIRas the explicit override and cover both path priorities in regression tests.Verified
npm run test --workspace=packages/core -- src/memory/store.test.tsnpm run typecheck --workspace=packages/corenpx eslint packages/core/src/memory/paths.ts packages/core/src/memory/store.test.tsgit diff --check