fix(core): sweep stale worktree project snapshots on startup - #7925
fix(core): sweep stale worktree project snapshots on startup#7925he-yufeng wants to merge 18 commits into
Conversation
|
Thanks for the PR! Template looks good ✓ (minor: "Risk & Scope" and "Linked Issues" headings are missing, but the content is covered in the body — not blocking) Problem: observed bug with strong evidence. Issue #7906 documents 98 of 111 entries in Direction: aligned. Cleaning up orphaned project snapshots is straightforward maintenance — no auth/sandbox/model/telemetry concerns. The startup-sweep approach is strictly stronger than shutdown deletion since it also covers crash/force-kill paths. Size: 82 production lines ( Approach: the design is conservative and well-reasoned — "prove-stale-or-keep" ensures normal project dirs are never touched. One sidecar judging the whole dir is justified by the shared-root invariant. Fire-and-forget with a once-guard avoids blocking startup. Scope feels right for the stated goal. Risk: no elevated risk signals (no high-risk paths matched). Moving on to code review. 🔍 中文说明感谢贡献！ 模板基本完整 ✓（小问题：缺少 "Risk & Scope" 和 "Linked Issues" 标题，但内容已在正文中覆盖——不阻塞） 问题：已观测到的 bug，有充分证据。Issue #7906 记录了 方向：对齐。清理孤儿项目快照是直接的维护工作——无 auth/sandbox/model/telemetry 顾虑。启动时清理比退出时删除更强，因为它也覆盖了 crash/force-kill 路径。 规模：82 行生产代码（ 方案：设计保守且合理——"证明过期才删除"确保正常项目目录永远不会被触碰。一个 sidecar 判断整个目录由共享根不变量证明合理。Fire-and-forget + once-guard 避免阻塞启动。范围与目标匹配。 风险：无升级风险信号（未匹配高风险路径）。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: given the issue (stale worktree project dirs accumulate because nothing removes them), I would add a startup sweep in Findings: The production code (
The test file change is +68 / −629 lines. The 629 deleted lines are all pre-existing tests for unrelated Storage functionality:
These tests are the only unit coverage for this functionality — there is no other test file that covers them. Replacing them with 5 sweep tests is a significant coverage regression. The new sweep tests should be added to the existing file, not replace it. The new sweep tests themselves look correct and cover the right cases (stale removed, live kept, no-sidecar kept, corrupted sidecar kept, missing projects dir, constructor once-guard). TestingThis is an unattended CI run — test evidence comes from the PR's own CI checks. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The ubuntu unit test suite is still running. macOS/Windows tests and integration tests were skipped (likely gated on ubuntu passing first). The author reports 5/5 pass for Not verified: the deleted tests' absence may cause the CI suite to report fewer total tests than 中文说明代码审查独立方案： 针对这个问题（过期 worktree 项目目录因无清理机制而累积），我会在 发现： 生产代码（
测试文件变更为 +68 / −629 行。被删除的 629 行是所有已有的 Storage 测试，覆盖无关功能：路径解析、运行时目录、QWEN_HOME 环境变量、plans 目录的安全相关路径遍历和符号链接逃逸检查、异步上下文隔离等。这些测试是该功能的唯一单元测试覆盖。用 5 个清理测试替换它们是严重的覆盖率回退。新的清理测试应该添加到现有文件中，而不是替换它。 新的清理测试本身正确，覆盖了合适的场景。 测试这是无人值守的 CI 运行——测试证据来自 PR 自身的 CI 检查。ubuntu 单元测试仍在运行中。作者报告 5/5 通过（ — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 2/5 — the production code is clean and well-designed, but the PR deletes the entire existing Storage test suite (629 lines of coverage for path resolution, security checks, env var handling, async context isolation) and replaces it with only the new sweep tests. That's a significant regression that blocks merge. The sweep itself is exactly the right fix for #7906 — conservative, fire-and-forget, startup-time, covers crash paths. I'd merge the production code as-is. But the test file needs to add the 5 new sweep tests to the existing suite, not replace it. The deleted tests include security-critical path traversal and symlink escape checks for To unblock: restore the existing tests in 中文说明置信度：2/5 —— 生产代码干净且设计良好，但 PR 删除了 Storage 的全部现有测试套件（629 行覆盖路径解析、安全检查、环境变量处理、异步上下文隔离），仅用新的清理测试替换。这是阻塞合并的严重回退。 清理功能本身是 #7906 的正确修复——保守、fire-and-forget、启动时执行、覆盖 crash 路径。生产代码可以直接合并。但测试文件需要添加 5 个新的清理测试到现有套件中，而不是替换。被删除的测试包括 解除阻塞方法：恢复 — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
The production code is clean and the right fix for #7906, but the PR deletes the entire existing Storage test suite (629 lines covering path resolution, security path-traversal checks, QWEN_HOME, async context isolation) and replaces it with only the 5 new sweep tests. Please restore the existing tests and append the new sweep tests. See my detailed notes above. 🙏
|
|
| ) as NodeJS.ErrnoException; | ||
| error.code = 'ENOENT'; | ||
| return error; | ||
| import { Storage, sweepStaleWorktreeProjects } from './storage.js'; |
There was a problem hiding this comment.
[Critical] The PR deletes the entire existing test suite for Storage (~629 lines, 8 describe blocks, ~59 test cases) and replaces it with only these 5 new tests for sweepStaleWorktreeProjects. The deleted tests were the sole dedicated unit coverage for security-critical getPlansDir tests (symlink escape prevention, path traversal rejection, Windows mixed-separator guards), getRuntimeBaseDir/setRuntimeBaseDir edge cases (tilde expansion, env-var priority, reset semantics), runtime path methods, config path stability, QWEN_HOME handling, and runWithRuntimeBaseDir async context isolation. The production code for all these methods is unchanged by this PR. No other test file in the codebase covers these edge cases — confirmed via cross-package grep.
Failure scenario: A future change to Storage.getPlansDir() or isResolvedPathWithinDirectory() could reintroduce a path traversal vulnerability (e.g., a plansDirectory config value of ../../etc or a symlink pointing outside the project root). The deleted tests specifically rejected these attacks. Without them, the regression would ship undetected until exploited.
Suggested fix: Restore the deleted test suites alongside the new sweepStaleWorktreeProjects tests. The new sweep tests are correctly written and should be kept, but they need to coexist with the existing suite rather than replace it.
— qwen3.7-max via Qwen Code /review
| it('the Storage constructor schedules the sweep once per base dir', async () => { | ||
| makeProjectSnapshot(base, '-tmp-qwen-exit-sess-ccc', { | ||
| worktreePath: path.join(base, 'missing'), | ||
| originalCwd: '/repo', | ||
| }); |
There was a problem hiding this comment.
[Suggestion] The test title claims "once per base dir" but the assertions only verify that the sweep eventually runs — not that the second new Storage('/tmp/y', base) was deduplicated. If the dedup guard in scheduleStaleWorktreeSweep were removed, both constructions would sweep independently, both would succeed (the second fsp.rm with force: true on an already-deleted path is a no-op), and this test would still pass.
Concrete cost: The dedup regression the test's title promises to catch would ship undetected.
Suggested fix: Spy on sweepStaleWorktreeProjects and assert it was called exactly once for the same base dir, or create two instances with different base dirs and verify both were swept (proving the Set is per-base, not global).
— qwen3.7-max via Qwen Code /review
|
Good catch, that was a careless overwrite on my end. Fixed in b12d194: the original 64-test suite is restored verbatim, and the 5 sweep tests moved into their own file (storage-sweep.test.ts), which also keeps them clear of the existing file's node:fs mock since the sweep tests want the real filesystem. 69/69 green locally. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
|
|
||
| new Storage('/tmp/x', base); | ||
| new Storage('/tmp/y', base); | ||
| await new Promise((resolve) => setTimeout(resolve, 300)); |
There was a problem hiding this comment.
[Suggestion] The fixed 300ms timeout for the fire-and-forget sweep is inherently flaky under CI load. — Failure scenario: on a heavily loaded CI runner, the async sweep (which does multiple fsp.readdir + fsp.readFile + fsp.rm calls) may not complete within 300ms, causing the existsSync assertion to fire before the deletion finishes and the test to fail intermittently.
Consider either increasing the timeout to a more generous value (e.g., 2000ms) to absorb CI variance, or making scheduleStaleWorktreeSweep return the promise (or store it on a testable surface) so the test can await it deterministically instead of racing a timer.
— qwen3.7-max via Qwen Code /review
| await fsp.rm(path.join(projectsDir, entry), { | ||
| recursive: true, | ||
| force: true, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] An fsp.rm failure for one entry (e.g., EACCES on a file with restricted permissions) propagates out of the for loop and aborts the entire sweep — all remaining stale entries after the failing one are skipped. — Failure scenario: a project directory contains a file created by a different user or on a read-only mount; fsp.rm throws, the outer .catch() logs a generic warning naming no specific entry, and all subsequent stale entries accumulate until the next process restart.
| await fsp.rm(path.join(projectsDir, entry), { | |
| recursive: true, | |
| force: true, | |
| }); | |
| try { | |
| await fsp.rm(path.join(projectsDir, entry), { | |
| recursive: true, | |
| force: true, | |
| }); | |
| removed.push(entry); | |
| logger.debug( | |
| `Removed stale worktree project snapshot ${entry} (worktree ${worktreePath} no longer exists)`, | |
| ); | |
| } catch (rmError: unknown) { | |
| logger.warn(`Failed to remove stale project snapshot ${entry}: ${rmError}`); | |
| } |
— qwen3.7-max via Qwen Code /review
|
All three inline notes addressed in 4bfaddb:
70/70 across the two storage test files, config suite 567/567. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| const chatsDir = path.join(projectsDir, entry, 'chats'); | ||
| let sidecarNames: string[]; | ||
| try { | ||
| sidecarNames = (await fsp.readdir(chatsDir)).filter((name) => | ||
| name.endsWith('.worktree.json'), | ||
| ); |
There was a problem hiding this comment.
[Suggestion] Sweep only scans chats/ top-level for .worktree.json sidecars — archived sessions' sidecars live in chats/archive/ and are missed.
— Failure scenario: a worktree session is archived (sessionService.archiveSessions() moves the sidecar to chats/archive/<sessionId>.worktree.json), the worktree is later removed, but the sweep finds no .worktree.json in chats/ top-level and skips the entry. The stale project snapshot accumulates — the exact bug this PR fixes, just through a different path.
| const chatsDir = path.join(projectsDir, entry, 'chats'); | |
| let sidecarNames: string[]; | |
| try { | |
| sidecarNames = (await fsp.readdir(chatsDir)).filter((name) => | |
| name.endsWith('.worktree.json'), | |
| ); | |
| const chatsDir = path.join(projectsDir, entry, 'chats'); | |
| let sidecarNames: string[] = []; | |
| for (const sub of ['', 'archive']) { | |
| try { | |
| const names = (await fsp.readdir(path.join(chatsDir, sub))).filter( | |
| (name) => name.endsWith('.worktree.json'), | |
| ); | |
| sidecarNames.push(...names); | |
| } catch { | |
| // subdirectory may not exist | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
| } catch { | ||
| continue; | ||
| } | ||
| if (worktreePath === undefined) continue; |
There was a problem hiding this comment.
[Suggestion] Only sidecarNames[0] is read — if it's corrupted, the entire project entry is skipped even when later sidecars are valid.
— Failure scenario: a project dir has two sessions; session-A.worktree.json is corrupted (partial write before crash, sorts first alphabetically), session-B.worktree.json is valid with worktreePath pointing at a deleted worktree. JSON.parse throws on session-A → catch fires continue → the outer loop moves on without ever reading session-B. The stale directory survives.
| } catch { | |
| continue; | |
| } | |
| if (worktreePath === undefined) continue; | |
| let worktreePath: string | undefined; | |
| for (const name of sidecarNames) { | |
| try { | |
| const parsed: unknown = JSON.parse( | |
| await fsp.readFile(path.join(chatsDir, name), 'utf-8'), | |
| ); | |
| if ( | |
| parsed !== null && | |
| typeof parsed === 'object' && | |
| typeof (parsed as Record<string, unknown>)['worktreePath'] === 'string' | |
| ) { | |
| worktreePath = (parsed as Record<string, string>)['worktreePath']; | |
| break; | |
| } | |
| } catch { | |
| // try next sidecar | |
| } | |
| } | |
| if (worktreePath === undefined) continue; |
— qwen3.7-max via Qwen Code /review
|
Both follow-ups landed in 39c31d4:
Three new tests pin the shapes: archived-only bucket gets swept, corrupted-first falls through to the valid second, one-live-one-gone bucket stays. Sweep file 9/9, config suite 570/570. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
| if (worktreePaths.some((worktreePath) => fs.existsSync(worktreePath))) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Critical] The sweep deletes any project bucket whose sidecars all point at gone worktrees, but a bucket does not have to be a worktree-specific snapshot to hold a .worktree.json sidecar. enter_worktree writes the sidecar into the normal project dir (projects/<sanitizeCwd(originalRepo)>/chats/) because the Config targetDir stays the original repo (it never calls relocateWorkingDirectory / process.chdir), and exit_worktree action='keep' deliberately preserves that sidecar. — Failure scenario: work in /repo → enter_worktree (sidecar lands in projects/-repo/chats/) → exit_worktree action='keep' → later remove the worktree manually (git worktree remove or rm -rf). On the next startup the sweep finds the sidecar in projects/-repo/, existsSync(worktreePath) is false, and fsp.rm(..., { recursive: true, force: true }) deletes the entire projects/-repo/ bucket — irreversibly destroying all chat history for the normal /repo project, not just worktree data. This contradicts the doc-comment invariant "this never touches normal project dirs". Confirmed end-to-end by probe: the normal bucket is deleted as-is, and kept once the guard below is added, while genuine worktree-specific buckets are still swept.
| if (worktreePaths.some((worktreePath) => fs.existsSync(worktreePath))) { | |
| continue; | |
| } | |
| if (worktreePaths.some((worktreePath) => fs.existsSync(worktreePath))) { | |
| continue; | |
| } | |
| // enter_worktree writes a sidecar into the NORMAL project dir (targetDir | |
| // stays the original repo), so only sweep buckets whose name matches a | |
| // sidecar's worktreePath — normal project dirs must never be removed. | |
| if (!worktreePaths.some((wp) => entry === sanitizeCwd(wp))) { | |
| continue; | |
| } |
— qwen3.8-max-preview via Qwen Code /review
| fs.chmodSync(stuck, 0o000); | ||
| try { | ||
| const removed = await sweepStaleWorktreeProjects(base); |
There was a problem hiding this comment.
[Suggestion] This test makes fsp.rm fail via DAC permission bits (chmod 0o000), but root (UID 0) bypasses DAC entirely, so on a root runner the removal succeeds and the test fails. — Failure scenario: in a Docker-based CI or devcontainer that runs as root (the project Dockerfile has no USER directive, so root is the default), fsp.rm(stuck, { recursive: true, force: true }) removes the mode-0o000 directory; the finally-block fs.chmodSync(stuck, 0o755) then throws ENOENT and the removed assertion fails. Confirmed by probe: 9/9 pass as UID 1000, this test fails as UID 0. It is green today only because the GitHub Actions runner is non-root.
| fs.chmodSync(stuck, 0o000); | |
| try { | |
| const removed = await sweepStaleWorktreeProjects(base); | |
| if (process.getuid?.() === 0) { | |
| // root bypasses DAC, so chmod 0o000 cannot force an rm failure here | |
| return; | |
| } | |
| fs.chmodSync(stuck, 0o000); | |
| try { | |
| const removed = await sweepStaleWorktreeProjects(base); |
(Alternatively, mock fsp.rm to reject for the stuck path so the failure path is exercised regardless of UID.)
— qwen3.8-max-preview via Qwen Code /review
|
Good point on the root case, that would have been a flaky red on the Docker runners. Reworked in 47da76b: the EACCES failure is now produced by a |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] storage.ts:73 — the previously-filed blocker (inline comment 3671452766) still stands and is NOT re-posted here as a duplicate. The sweep deletes the entire NORMAL project bucket: enter_worktree writes the .worktree.json sidecar into projects/<sanitizeCwd(originalRepo)>/chats/ (targetDir is never relocated), exit_worktree action='keep' preserves it, and once the worktree is removed (manually or via crash) fs.existsSync(worktreePath) is false so fsp.rm(..., {recursive,force}) destroys the whole normal bucket — all chat history — contradicting the doc-comment 'this never touches normal project dirs'. Probe-confirmed end-to-end; the suggested guard (only sweep buckets whose name matches sanitizeCwd(worktreePath)) flips the outcome to 'kept'.
— qwen3.8-max-preview via Qwen Code /review
| const worktreePaths = await readWorktreeSidecarPaths(chatsDir); | ||
| if (worktreePaths.length === 0) continue; |
There was a problem hiding this comment.
[Suggestion] The sweep only reclaims buckets that still hold a .worktree.json sidecar pointing at a gone worktree, but the normal worktree lifecycle deletes that sidecar before the sweep runs — so clean-exit snapshots are kept forever and the #7906 accumulation persists for the dominant path.
Failure scenario: a --worktree session stores its transcript + sidecar in projects/<sanitizeCwd(worktreePath)> (worktreeStartup chdirs before Config construction). exit_worktree action='remove' then runs removeUserWorktree + maybeClearWorktreeSession, which unlinks the sidecar but leaves the transcript and the bucket (nothing removes projects/<…>/ — Config.shutdown() doesn't, and worktreeCleanup only removes worktree dirs). restoreWorktreeContext does the same on a stale --resume. On the next startup readWorktreeSidecarPaths returns [], so worktreePaths.length === 0 → continue keeps the orphaned bucket forever. The sweep only reclaims the narrow crash/abandon-and-never-resume subset.
Identifying worktree buckets by name resolves this and the line-73 Critical together: a bucket whose name equals sanitizeCwd(p) for some p under */.qwen/worktrees/* is a worktree snapshot — delete it when that directory is gone, regardless of whether a sidecar survives.
— qwen3.8-max-preview via Qwen Code /review
| function scheduleStaleWorktreeSweep(runtimeBaseDir: string): void { | ||
| if (staleWorktreeSweepStarted.has(runtimeBaseDir)) return; | ||
| staleWorktreeSweepStarted.add(runtimeBaseDir); |
There was a problem hiding this comment.
[Suggestion] The sweep is one-shot per process per base dir — staleWorktreeSweepStarted is never cleared, and nothing else calls sweepStaleWorktreeProjects in production (its only caller is this constructor path). A long-lived qwen serve daemon (serve.ts blocks forever) sweeps once at startup and then never again, so worktree snapshots created after startup accumulate until the process is restarted — the same "accumulate forever" symptom #7906 targets, relocated to long-lived processes.
Failure scenario: a daemon runs for days; agents create/remove worktrees; each leaves a projects/<sanitizeCwd(worktreePath)> bucket; every later new Storage(…) hits the has(…) guard and returns; nothing is reclaimed until restart.
Consider re-arming instead of latching forever (e.g. drop the Set entry in a .finally(), optionally gated by a last-sweep timestamp file), or triggering a sweep from exit_worktree / a maintenance interval.
— qwen3.8-max-preview via Qwen Code /review
| ) { | ||
| this.targetDir = targetDir; | ||
| this.runtimeBaseDir = path.resolve(runtimeBaseDir); | ||
| scheduleStaleWorktreeSweep(this.runtimeBaseDir); |
There was a problem hiding this comment.
[Suggestion] The constructor now schedules an async recursive-rm sweep unconditionally on the resolved base dir. test-setup.ts does not redirect QWEN_HOME, so the default base dir resolves to the real os.homedir()/.qwen; roughly 105 single-arg new Storage(…) call sites in the test suite therefore fire the sweep against the real ~/.qwen/projects/ during ordinary unit-test runs — silently, and (per the line-73 Critical) capable of deleting normal buckets.
Failure scenario: a developer with stale worktree snapshots in ~/.qwen/projects/ runs any test that constructs new Storage(someDir); the first construction in that worker deletes real project dirs with no console output and no opt-out.
Consider gating the sweep behind an explicit opt-in from the CLI entry point, or skipping it when process.env['VITEST'] / NODE_ENV === 'test' is set.
| scheduleStaleWorktreeSweep(this.runtimeBaseDir); | |
| if (!process.env['VITEST']) { | |
| scheduleStaleWorktreeSweep(this.runtimeBaseDir); | |
| } |
— qwen3.8-max-preview via Qwen Code /review
| removed.push(entry); | ||
| logger.debug( |
There was a problem hiding this comment.
[Suggestion] A recursive delete of user data is recorded only through createDebugLogger, which writes nothing unless QWEN_DEBUG_LOG_FILE / a debug session is active (verified: writeLog returns early otherwise). So both this success line and the scheduler's logger.warn are dropped in a normal session — a wrongful deletion (see the line-73 Critical) leaves no default-level audit trail.
Failure scenario: a user reports "my chat history disappeared"; the oncall has no default log that the sweep ran or what it removed, and can only confirm it by reproducing with QWEN_DEBUG_LOG_FILE set — state the user has already lost.
For a destructive operation, emit the deletion record to a sink that is on by default (or at minimum raise it to a user-visible warning and document that diagnosing sweep behavior requires QWEN_DEBUG_LOG_FILE).
— qwen3.8-max-preview via Qwen Code /review
| try { | ||
| names = await fsp.readdir(dir); | ||
| } catch { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Suggestion] Two defensive branches are untested: a project bucket that lacks a chats/ subdir (every fixture creates one via makeProjectSnapshot), and the valid-JSON-but-wrong-shape sidecar (the tests only use invalid JSON '{not json', never e.g. {"worktreePath": 123}).
Failure scenario: a mutation deleting this inner readdir try/catch survives all 9 tests; in production a chats-less bucket (e.g. workflows/-only — getWorkflowRunsDir, subagents/, and the session-organization store all write under getProjectDir() without creating chats/) would then throw ENOENT out of readWorktreeSidecarPaths, reject the whole sweep (the constructor .catch swallows it), and silently skip every remaining stale entry.
Add a test with a chats-less project entry asserting it is kept while a sibling stale dir is removed, and a sidecar with valid JSON but a non-string worktreePath to pin the type-guard branch.
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Re-confirmed still standing (existing inline comment 3671452766, NOT re-posted as a duplicate): the sweep deletes the entire NORMAL project bucket. enter_worktree writes the .worktree.json sidecar into projects/<sanitizeCwd(originalRepo)>/chats/ because the Config targetDir is never relocated (enter-worktree.ts performs no relocateWorkingDirectory/process.chdir; the sidecar path comes from the current session service's getWorktreeSessionPath -> Storage(cwd).getProjectDir()); exit_worktree action='keep' deliberately preserves that sidecar; and once the worktree is later removed (manually or via crash) fs.existsSync(worktreePath) is false, so fsp.rm(..., {recursive:true, force:true}) at storage.ts:76-79 destroys the whole normal bucket — irreversibly deleting all chat history for the normal project, contradicting the doc-comment 'this never touches normal project dirs'. The round-2 reverse audit found a second planting trigger with the same root cause: /cd artifact migration (config.ts moveCurrentSessionArtifacts) moves the sidecar into the new normal bucket. The suggested guard (only sweep buckets whose name equals sanitizeCwd(worktreePath)) fixes all triggers and still removes genuine worktree buckets. Verified at the reviewed commit 2e5daba.
— qwen3.8-max-preview via Qwen Code /review
| const worktreePaths: string[] = []; | ||
| for (const sidecar of sidecars) { | ||
| try { | ||
| const parsed: unknown = JSON.parse(await fsp.readFile(sidecar, 'utf-8')); |
There was a problem hiding this comment.
[Suggestion] Duplicated sidecar parsing/validation — readWorktreeSession + isValidWorktreeSession in worktreeSessionService.ts already parse and validate this same .worktree.json format (six required string fields), while this inline check validates only worktreePath. Two parsers for one on-disk format diverge silently. — Concrete cost: if the sidecar format evolves (a version field gates interpretation, or worktreePath is renamed), a developer updating isValidWorktreeSession must separately discover and update this inline check; miss it and the sweep either stops cleaning up orphans or misparses sidecars (a sidecar valid here but invalid there can still drive a bucket deletion). Importing readWorktreeSession directly would create a circular dep (worktreeSessionService.ts already imports storage.ts), so extract the shape validation into a small shared module both files import.
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Re-confirmed still standing at the reviewed commit 0f9eeb0 (existing inline comment 3671452766, NOT re-posted as a duplicate): the sweep deletes the entire NORMAL project bucket. enter_worktree writes the .worktree.json sidecar into projects/<sanitizeCwd(originalRepo)>/chats/ because the SessionService's Storage is keyed on the original cwd (enter-worktree.ts performs no relocateWorkingDirectory/process.chdir; the sidecar path comes from getSessionService().getWorktreeSessionPath -> getChatsDir -> storage.getProjectDir()); exit_worktree action='keep' deliberately preserves that sidecar; and /cd artifact migration (config.ts moveCurrentSessionArtifacts) moves it into the new normal bucket. Once the worktree is later removed (manually or via crash), fs.existsSync(worktreePath) is false, so fsp.rm(..., {recursive:true, force:true}) at storage.ts:76-79 destroys the whole normal bucket — irreversibly deleting all chat history for the normal project, contradicting the doc-comment 'this never touches normal project dirs'. The suggested guard (only sweep buckets whose name equals sanitizeCwd(worktreePath)) is absent from the current diff and fixes all planting triggers while still removing genuine worktree buckets.
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] storage.ts:73 — the previously-filed blocker (inline comment 3671452766) still stands at this commit and is NOT re-posted inline as a duplicate. The sweep deletes the entire NORMAL project bucket: enter_worktree writes the .worktree.json sidecar into projects/<sanitizeCwd(originalRepo)>/chats/ (Config.getSessionService() is keyed on the original targetDir at config.ts:7333; enter-worktree.ts performs no relocateWorkingDirectory/process.chdir), exit_worktree action='keep' deliberately preserves that sidecar, and once the worktree is later removed (manually or via crash) fs.existsSync(worktreePath) is false, so fsp.rm(..., {recursive:true, force:true}) destroys the whole normal bucket — irreversibly deleting all chat history for the normal project, contradicting the doc-comment 'this never touches normal project dirs'. The suggested guard (only sweep buckets whose name equals sanitizeCwd(worktreePath)) is still absent from the current diff. Probe re-confirmed at the reviewed commit 34acd2d: the normal bucket is deleted as-is and kept once the guard is added, while genuine worktree buckets are still swept.
— qwen3.8-max-preview via Qwen Code /review
| it('falls through a corrupted sidecar to the next valid one', async () => { | ||
| const projectDir = makeProjectSnapshot(base, '-tmp-qwen-exit-sess-mix', { | ||
| worktreePath: path.join(base, 'gone'), |
There was a problem hiding this comment.
[Suggestion] This test is vacuous for the "fall through" behavior it names. Mutating the parse-error catch {} in readWorktreeSidecarPaths to catch { break; } (abort on the first corrupted sidecar) yields the same observable outcome — the project is kept — so the test passes either way: correct code keeps via some(existsSync) (session-2 is alive), the mutant keeps via length === 0 (empty list after the break). — Failure scenario: a future refactor replacing catch {} with catch { break; } ships, and a project whose first sidecar (in sort order) is corrupted while the rest are stale is kept forever instead of swept — the #7906 accumulation bug in a narrower form — with no test failing. Probe-confirmed: the break mutant passes this test as written. Make session-2 point at a gone path too, so correct code removes the project (all parseable sidecars stale) while the break mutant keeps it (empty list):
fs.writeFileSync(
path.join(projectDir, 'chats', 'session-2.worktree.json'),
JSON.stringify({ worktreePath: path.join(base, 'also-gone'), originalCwd: '/repo' }),
);
const removed = await sweepStaleWorktreeProjects(base);
expect(removed).toEqual(['-tmp-qwen-exit-sess-mix']);
expect(fs.existsSync(projectDir)).toBe(false);— qwen3.8-max-preview via Qwen Code /review
| } catch { | ||
| // corrupted sidecar: try the next one before judging the bucket | ||
| } |
There was a problem hiding this comment.
[Suggestion] This single catch conflates I/O errors from fsp.readFile(sidecar) with JSON.parse errors, silently discarding a sidecar that is unreadable (transient EACCES/EIO/ESTALE) rather than corrupt — and that sidecar may be the bucket's only "keep" vote. — Failure scenario: a bucket holds session-A (sidecar → gone worktree) and session-B (sidecar → live worktree, but the sidecar file is momentarily unreadable). readFile throws for session-B, the catch skips it, worktreePaths is only [gone], some(existsSync) is false, and fsp.rm deletes the whole bucket including session-B's live transcript. This contradicts the docstring "Anything that cannot prove itself stale … is kept" — an unreadable sidecar means staleness cannot be proven. The canonical readWorktreeSession in worktreeSessionService.ts already separates the two (it propagates non-ENOENT read errors and silences only parse errors). Split the read from the parse so a read error means "keep":
let raw: string;
try {
raw = await fsp.readFile(sidecar, 'utf-8');
} catch {
// Unreadable sidecar: cannot prove stale; keep the bucket.
return [sidecar];
}
try {
const parsed: unknown = JSON.parse(raw);
// …existing shape check / push worktreePath…
} catch {
// corrupted sidecar: try the next one
}— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] storage.ts:77 — the previously-filed blocker (inline comment 3671452766) still stands at this commit and is NOT re-posted inline as a duplicate. The sweep deletes the entire NORMAL project bucket: enter_worktree writes the .worktree.json sidecar into projects/<sanitizeCwd(originalRepo)>/chats/ (Config.getSessionService() is keyed on the original targetDir; enter-worktree.ts performs no relocateWorkingDirectory/process.chdir), exit_worktree action='keep' preserves that sidecar and maybeClearWorktreeSession clears only the current session's, so once the worktree is later removed (manual git worktree remove / rm -rf, another session's exit_worktree action='remove', or crash) fs.existsSync(worktreePath) is false and fsp.rm(..., {recursive:true, force:true}) destroys the whole normal bucket — irreversibly deleting all chat history for the normal project, contradicting the doc-comment 'this never touches normal project dirs'. Probe re-confirmed at the reviewed commit 263576d: a normal bucket holding a real transcript plus an enter_worktree sidecar pointing at a removed worktree is deleted as-is (removed=["-repo"], transcript gone), and is kept once the guard (only sweep buckets whose name equals sanitizeCwd(worktreePath)) is added, while genuine worktree buckets are still swept.
— qwen3.8-max-preview via Qwen Code /review
| import { AsyncLocalStorage } from 'node:async_hooks'; | ||
| import { getProjectHash, QWEN_DIR, sanitizeCwd } from '../utils/paths.js'; | ||
| import { FatalConfigError } from '../utils/errors.js'; | ||
| import { createDebugLogger } from '../utils/debugLogger.js'; |
There was a problem hiding this comment.
[Suggestion] This import introduces a circular module dependency: storage.ts → debugLogger.ts → storage.ts (debugLogger.ts imports Storage). It is safe today only because debugLogger.ts references Storage exclusively inside function bodies — but Storage is a class declaration, in the temporal dead zone until its definition is evaluated. — Concrete cost: a future top-level use of Storage in debugLogger.ts (e.g. caching Storage.getGlobalDebugDir() into a module-level const, which the existing ensuredDebugDirPath pattern already hints at) throws ReferenceError: Cannot access 'Storage' before initialization whenever storage.ts is the entry module that triggers the chain — a non-obvious invariant with no compile-time or lint-time enforcement. Consider a lazy/dynamic import for the logger, or document in debugLogger.ts that Storage must never be used at module scope.
— qwen3.8-max-preview via Qwen Code /review
| if ( | ||
| parsed !== null && | ||
| typeof parsed === 'object' && | ||
| typeof (parsed as Record<string, unknown>)['worktreePath'] === 'string' | ||
| ) { |
There was a problem hiding this comment.
[Suggestion] This shape-validation branch (valid JSON that is not an object, or an object whose worktreePath is not a string) has no test coverage. The only "corrupted" fixture in storage-sweep.test.ts is invalid JSON ('{not json'), which is caught by JSON.parse; no test feeds valid JSON that fails this check (e.g. {"worktreePath": 123} or "just a string"). — Failure scenario: a future refactor that drops the typeof === 'string' guard ships with all 9 tests still green, after which a non-string worktreePath is pushed into worktreePaths and fed to fs.existsSync. Add a case that writes a sidecar with valid-JSON-but-wrong-shape content and asserts the bucket is kept and removed is [].
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
[Critical] storage.ts:71-79 — the previously-filed blocker (inline comment 3671452766) still stands at the reviewed commit 125b952 and is NOT re-posted inline as a duplicate. The sweep deletes the entire NORMAL project bucket: enter_worktree writes the .worktree.json sidecar into projects/<sanitizeCwd(originalRepo)>/chats/ (enter-worktree.ts:197-198 calls getSessionService().getWorktreeSessionPath(); the SessionService's Storage is keyed on the original targetDir, and enter-worktree.ts performs no relocateWorkingDirectory/process.chdir), exit_worktree action='keep' preserves that sidecar, and once the worktree is later removed (manual git worktree remove, another session's exit_worktree action='remove', or crash) fs.existsSync(worktreePath) at storage.ts:71 is false, so fsp.rm(..., {recursive:true, force:true}) at storage.ts:77-79 destroys the whole normal bucket — irreversibly deleting all chat history for the normal project, contradicting the doc-comment 'this never touches normal project dirs'. The suggested guard (only sweep buckets whose name equals sanitizeCwd(worktreePath)) is absent from the current diff. Probe-confirmed at the reviewed commit 125b952: a normal bucket holding a transcript plus an enter_worktree sidecar pointing at a removed worktree is deleted as-is, and is kept once the guard is added, while genuine worktree buckets are still swept.
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
[Critical] storage.ts:71-79 — the previously-filed blocker (inline comment 3671452766) still stands at the reviewed commit ae6e6cb and is NOT re-posted inline as a duplicate. The sweep deletes the entire NORMAL project bucket: enter_worktree writes the .worktree.json sidecar into projects/<sanitizeCwd(originalRepo)>/chats/ (enter-worktree.ts:196 calls getSessionService().getWorktreeSessionPath(); the SessionService's Storage is keyed on the original targetDir at config.ts:7547, and enter-worktree.ts performs no relocateWorkingDirectory/process.chdir), exit_worktree action='keep' preserves that sidecar (exit-worktree.ts:206), and /cd artifact migration moves it into the new normal bucket (config.ts:4624). Once the worktree is later removed (manual git worktree remove, another session's exit_worktree action='remove', or crash) fs.existsSync(worktreePath) at storage.ts:71 is false, so fsp.rm(..., {recursive:true, force:true}) at storage.ts:77-79 destroys the whole normal bucket — irreversibly deleting all chat history for the normal project, contradicting the doc-comment 'this never touches normal project dirs'. The suggested guard (only sweep buckets whose name equals sanitizeCwd(worktreePath)) is absent from the current diff. Re-confirmed against the code at ae6e6cb by the issue-fidelity, security, and oncall agents plus an independent code trace.
— qwen3.8-max-preview via Qwen Code /review
| void sweepStaleWorktreeProjects(runtimeBaseDir).catch((error: unknown) => { | ||
| logger.warn(`stale worktree project sweep failed: ${error}`); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] The error stack trace is discarded before the logger can preserve it. logger.warn(...failed: ${error}) calls Error.toString() (message only) during template interpolation, before debugLogger's formatArgs — which special-cases Error instances to extract .stack — ever sees the object. The per-entry logger.debug(...${entry}: ${String(error)}) at line 82 has the same problem. — Concrete cost: when the sweep hits an unexpected failure (an EPERM/EIO that force: true does not suppress, or a future refactor bug), the debug log shows only Error: EACCES with no stack or call site, so the oncall cannot tell whether it came from readdir, readFile, rm, or JSON.parse without reproducing locally.
| void sweepStaleWorktreeProjects(runtimeBaseDir).catch((error: unknown) => { | |
| logger.warn(`stale worktree project sweep failed: ${error}`); | |
| }); | |
| void sweepStaleWorktreeProjects(runtimeBaseDir).catch((error: unknown) => { | |
| logger.warn('stale worktree project sweep failed:', error); | |
| }); |
— qwen3.8-max-preview via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
[Critical] storage.ts:71-79 — the previously-filed blocker (inline comment 3671452766) still stands at the reviewed commit b0d78d9 and is NOT re-posted inline as a duplicate. The sweep deletes the entire NORMAL project bucket: enter_worktree writes the .worktree.json sidecar into projects/<sanitizeCwd(originalRepo)>/chats/ (enter-worktree.ts:194-207 calls getSessionService().getWorktreeSessionPath(); the SessionService is keyed on the original targetDir via storage.getProjectRoot() at config.ts:7547-7549, and enter-worktree.ts performs no relocateWorkingDirectory/process.chdir), exit_worktree action='keep' deliberately preserves that sidecar (exit-worktree.ts:205-212), and /cd artifact migration moves it into the new normal bucket (config.ts:4624-4638). Once the worktree is later removed (manual git worktree remove, another session's exit_worktree action='remove', or crash) fs.existsSync(worktreePath) at storage.ts:71 is false, so fsp.rm(..., {recursive:true, force:true}) destroys the whole normal bucket — irreversibly deleting all chat history for the normal project, contradicting the doc-comment 'this never touches normal project dirs'. Probe-confirmed at the reviewed commit b0d78d9: a normal bucket holding a transcript plus an enter_worktree sidecar pointing at a removed worktree is deleted as-is, and is kept once a guard (only sweep buckets whose name matches sanitizeCwd(worktreePath)) is added, while genuine worktree buckets are still swept.
— qwen3.8-max-preview via Qwen Code /review
| ) { | ||
| worktreePaths.push((parsed as Record<string, string>)['worktreePath']); | ||
| } |
There was a problem hiding this comment.
[Suggestion] Sidecar validation accepts empty and relative worktreePath strings (only typeof === 'string'), so a valid-JSON sidecar like {"worktreePath": ""} counts as affirmative proof the worktree is gone instead of being skipped — violating this function's own invariant that a corrupted/ambiguous sidecar "proves nothing and is skipped". — Failure scenario: a manually-edited or exotically-corrupted sidecar with an empty worktreePath (fs.existsSync('') is false) or a relative one (resolved against an unrelated startup cwd) makes every sidecar in the bucket appear to point at a gone worktree, so fsp.rm deletes the bucket and its transcripts though no sidecar named a real worktree path that was verified absent. Probe-confirmed: a bucket holding a transcript plus a {"worktreePath": ""} sidecar is deleted as-is, and kept once the guard below is added. The trigger is narrow (the writer always writes absolute paths, and partial writes cannot match the .worktree.json filter), so this is hardening for a destructive auto-running path rather than a likely failure. The application's own isValidWorktreeSession path has a structural backstop (worktreePath must live under <originalCwd>/.qwen/worktrees/); the sweep has none.
| ) { | |
| worktreePaths.push((parsed as Record<string, string>)['worktreePath']); | |
| } | |
| typeof (parsed as Record<string, unknown>)['worktreePath'] === 'string' && | |
| (parsed as Record<string, string>)['worktreePath'].length > 0 && | |
| path.isAbsolute((parsed as Record<string, string>)['worktreePath']) | |
| ) { | |
| worktreePaths.push((parsed as Record<string, string>)['worktreePath']); |
— qwen3.8-max-preview via Qwen Code /review
b0d78d9 to
1ea3920
Compare
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): src/config/storage.test.ts — no such file or directory.
Deferred under the convergence posture (round 10, not a blocker) — recorded, not requested in this round:
packages/core/src/config/storage-sweep.test.ts:471 — [probe] 'once per base dir' test unfalsifiable — '/repo' fixture kept by arm-1's repo-existence conjunct regardless of the dedupepackages/core/src/config/storage.ts:226 — [review] JSDoc binds to hasLiveSiblingWorktree but describes readWorktreeSidecarRecords; false 'chats/archive/' claimpackages/cli/src/gemini.tsx:347 — [test] interactive-startup wiring (both hunks) untested — reverting either leaves all tests greenpackages/cli/src/cli.ts:384 — [test] serve-route arm hunk survives reversion — daemon sweep silently disabled with no red testpackages/cli/src/cli.ts:384 — [review] core barrel import before tryRunServeFastPath defeats the fast-path import boundary (577 ms cold load measured)packages/cli/src/gemini.tsx:347 — [review] QWEN_SANDBOX='0'/'false' disable idiom disarms the sweep on an unsandboxed host (truthiness conflict with sandboxConfig)packages/core/src/config/storage.ts:97 — [review] transient projects/ readdir failure silently disables the sweep for the process lifetime (dedupe Set already populated)packages/core/src/utils/paths.test.ts:1110 — [probe] symlink-loop test exits via ELOOP before the hop branch — the only iterative code in the moved helper is untestedpackages/cli/src/gemini.tsx:346 — [review] gate comment inverts the sandbox markers (QWEN_SANDBOX host-side; SANDBOX marks both container and seatbelt)packages/core/src/config/storage-sweep.test.ts:385 — [probe] live-runtime.json veto test vacuous — '/repo' fixture keeps before hasLiveRuntime; bypassing the veto leaves 25/25 greenpackages/core/src/config/storage-sweep-gate.test.ts:32 — [probe] default-off guardian uses a one-shot 300 ms sleep oracle — contention-model probe passed with an armed sweeppackages/core/src/config/storage-sweep.test.ts:243 — [probe] archive-veto test unfalsifiable — gate removal still keeps via zero-records rule; mixed case never exercised
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // The serve daemon is long-lived and never reaches gemini.tsx main(): | ||
| // arm the stale worktree sweep here so it fires once at startup. | ||
| const { enableStartupSweep } = await import('@qwen-code/qwen-code-core'); | ||
| enableStartupSweep(); |
There was a problem hiding this comment.
[Critical] R10-1: The serve route arms the destructive startup sweep unconditionally, bypassing the QWEN_SANDBOX/SANDBOX disarm gate this same PR applies in gemini.tsx main() — a sandboxed serve daemon can delete host session data through the writable-mounted state dir using the container's partial filesystem view. — Failure scenario: qwen serve inside a sandbox container is a product-recognized topology (root Dockerfile sets ENV SANDBOX; the shipped image has Env SANDBOX=qwen-code-sandbox; #7139's in-container serve wiring): the host runtime base dir is mounted writable at the same canonical path (sandbox.ts:513-520; getContainerPath is identity on Linux), and the container's view of host paths is partial — a host bucket whose worktree/launch path is invisible from the container while its originalCwd is mounted passes the sweep's existence gates (headless bucket, cold transcript, no runtime.json) and is rm -rf'd from inside the container even though it exists on the host — the exact outcome the gemini.tsx gate comment exists to prevent. Note the arm happens BEFORE tryRunServeFastPath, so the fast-path fallthrough into main() cannot substitute the gate either.
Witness (container A/B on this PR's own compiled sweep; node:22-bookworm, -e SANDBOX=qwen-code-sandbox, host scratch runtime base mounted rw at the same path, identical stale-shaped host bucket):
ARM=serve: host bucket exists after container: NO-DELETED
ARM=gated: host bucket exists after container: yes
| // The serve daemon is long-lived and never reaches gemini.tsx main(): | |
| // arm the stale worktree sweep here so it fires once at startup. | |
| const { enableStartupSweep } = await import('@qwen-code/qwen-code-core'); | |
| enableStartupSweep(); | |
| // The serve daemon is long-lived and never reaches gemini.tsx main(): | |
| // arm the stale worktree sweep here so it fires once at startup — but | |
| // never inside a sandbox re-launch, where the host state dir is mounted | |
| // writable and the container's path view is partial. SANDBOX marks both | |
| // container and seatbelt children; QWEN_SANDBOX additionally keeps the | |
| // pre-hop host process disarmed. | |
| if (!process.env['QWEN_SANDBOX'] && !process.env['SANDBOX']) { | |
| const { enableStartupSweep } = await import('@qwen-code/qwen-code-core'); | |
| enableStartupSweep(); | |
| } |
— qwen3.8-max via Qwen Code /review (v0.21.13)
| try { | ||
| if ((await fsp.readdir(path.join(chatsDir, 'archive'))).length > 0) | ||
| continue; | ||
| } catch { | ||
| // no archive dir: nothing retained | ||
| } |
There was a problem hiding this comment.
[Critical] R10-2: The archive-retention gate's catch classifies every readdir failure as "no archive dir", so a non-ENOENT error (EACCES/EIO/ESTALE/ELOOP) on an existing non-empty chats/archive/ proceeds to the stale verdict and deletion — contradicting the invariant three lines above ("any entry at all under chats/archive/ keeps the bucket, no parsing involved") and the keep-on-error bias of isDirectorySync/isPositivelyExistingDirectorySync in this same function. — Failure scenario: a bucket with a gone worktreePath, an existing originalCwd repo (arm 1 stale=true), and user-archived transcripts in chats/archive/: if readdir fails transiently (EIO/ESTALE on a downed or network mount, root_squash exports, DAC/ACL ownership mismatch between a root daemon and a user dir), the catch reads the archive as empty, archived transcripts never appear in the top-level transcript scan, no veto fires, and fsp.rm deletes explicitly retained archived data.
Witness (probe against the real sweep at this commit):
B(ELOOP archive) readdir=ELOOP removed=["…-eloop-gone-worktree"] bucketStillExists=false ← archived data deleted
A(EACCES archive) chmod-000 veto skipped, rm attempted (log: EACCES … scandir '…/chats/archive')
C(readable) identical contents → removed=[] bucketStillExists=true
| try { | |
| if ((await fsp.readdir(path.join(chatsDir, 'archive'))).length > 0) | |
| continue; | |
| } catch { | |
| // no archive dir: nothing retained | |
| } | |
| try { | |
| if ((await fsp.readdir(path.join(chatsDir, 'archive'))).length > 0) | |
| continue; | |
| } catch (error) { | |
| // ENOENT: no archive dir, nothing retained. Any other failure means the | |
| // archive exists but cannot be listed — it cannot be proven empty, and a | |
| // destructive sweep keeps on doubt. | |
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') continue; | |
| } |
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (stale && (await hasLiveSiblingWorktree(sidecars, entry))) { | ||
| // sanitizeCwd collapses fix.bug and fix-bug to one bucket name, so | ||
| // the gate cannot prove which worktree owns the bucket; a cold but | ||
| // on-disk co-owner worktree must keep it (cold data has no liveness | ||
| // signal for the vetoes below). | ||
| stale = false; | ||
| } |
There was a problem hiding this comment.
[Critical] R9-2: sanitizeCwd collision family — still stands at this commit. The round-9 fix (hasLiveSiblingWorktree) closes only the arm-1 same-repo shape; two probe-confirmed entrances remain open because sanitizeCwd collapses every non-alphanumeric to - while deletion is keyed on that lossy name: (1) arm 2 has no collision veto at all — this call site is the only one, inside arm 1 — so a live NON-worktree directory colliding with a gone tmpdir dir loses the shared bucket; (2) the sibling enumeration walks only <sidecar.originalCwd>/.qwen/worktrees/, so a live colliding worktree owned by a different repo with no sidecar in the shared bucket (e.g. after a /cd relocation) is invisible. Cold data has no liveness signal by definition, so the runtime.json/transcript vetoes cannot cover it. — Failure scenario (arm 2): /tmp/fix.bug (gone throwaway repo) and /tmp/fix-bug (live scratch dir) sanitize to the same bucket; the gone dir's sidecar passes arm 2 (key match, tmpdir-contained, gone); the live co-owner is headless and idle >10 min (the standing R7-1 gap), both vetoes are false, and fsp.rm deletes the live session's transcripts. — Failure scenario (cross-repo): repos /x/a.b/r and /x/a-b/r whose worktree paths sanitize identically; the dead repo's /cd-relocated sidecar sits in the shared bucket; the sibling check walks only the dead repo; the shared bucket is deleted while the other repo's worktree is alive.
Witness (two executed probes at this commit, each flipped by the missing veto):
arm 2: gone fix.bug + live colliding fix-bug, headless-idle co-owner
pristine: removed=[entry], live transcript deleted
+ sibling-collision veto: removed=[]
+ hasLiveSiblingWorktree forced true: STILL deleted (arm 2 never consults it)
cross-repo: x/a.b/r (dead) vs x/a-b/r (live), relocated sidecar in shared bucket
pristine: removed=[entry] with the live co-owner worktree on disk
+ hasLiveSiblingWorktree forced true: removed=[]
Fix direction (structural, on record since round 3): make bucket keys injective or pair deletion on the sidecar's own worktreePath; until then extend a collision veto to arm 2 and make the sibling check scan live directories/buckets whose sanitized name equals entry, not just the sidecar's own repo.
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if ( | ||
| (await hasLiveRuntime(chatsDir)) || | ||
| (await hasRecentTranscriptActivity(chatsDir)) | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Critical] R7-1: prior-round blocker still stands at this commit (open threads 3791013674 / 3793779988 / 3799487293; the author acknowledged this is real and asked for a maintainer decision). The liveness veto leaves headless/serve/ACP sessions unprotected whenever their transcript has been quiet longer than the 10-minute grace: they never write runtime.json (markRuntimeStatusEnabled is called only in startInteractiveUI.tsx; the config.ts refresh sites are gated on runtimeStatusEnabled), so transcript mtime is their only liveness signal — and any other qwen process starting up can sweep their bucket. — Failure scenario: a headless (qwen -p), qwen serve, ACP, or daemon session idle >10 minutes in a worktree deleted out from under it (or a colliding/tmpdir-keyed bucket per the R9-2 family): the arm gates pass, hasLiveRuntime finds no runtime.json, the transcript grace has lapsed, and fsp.rm deletes the live session's bucket — resumable history destroyed for a session that is still running. Witness: not run — mechanism unchanged at this commit and acknowledged by the author (round-9 reply, escalated together with R9-1 for a maintainer call); code trace at this commit confirms the sweep's own comment: "runtime.json is only written by interactive sessions". Fix direction on record: make liveness session-mode-agnostic (write runtime.json for headless/serve/ACP sessions too) or adopt the session-level lock the author flagged.
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (staleWorktreeSweepStarted.has(runtimeBaseDir)) return; | ||
| staleWorktreeSweepStarted.add(runtimeBaseDir); |
There was a problem hiding this comment.
[Critical] R9-1: prior-round blocker still stands at this commit (the author acknowledged it and asked for a maintainer call rather than stacking more heuristics). One-shot keepBucket protection: scheduleStaleWorktreeSweep dedupes on runtimeBaseDir alone and discards the keepBucket of every Storage constructed after the first, so the sweep can delete the bucket of a session the same process — now explicitly including the long-lived qwen serve daemon this PR arms — or another process is attaching to. — Failure scenario: the daemon's first Storage construction fixes keepBucket to one bucket and starts the async sweep; while it iterates, a second client resumes a session whose bucket qualifies as stale (gone worktree / gone tmpdir launch cwd, transcript older than the 10-minute grace, no runtime.json — serve sessions never write one); that session's keepBucket is dropped by this early return and its transcripts are rm -rf'd mid-resume. Witness: not run — mechanism unchanged at this commit and acknowledged by the author (round-9 reply); the round-6 probe executed the same race 40/40 (the sweep deleted the resumed session's bucket within ~300 ms after load). Fix direction on record: track keep-buckets as a per-base-dir set updated by every Storage construction (or re-derive the protected set inside the sweep loop); the session-level-lock direction is the open maintainer question.
— qwen3.8-max via Qwen Code /review (v0.21.13)
Worktree sessions register a project dir under .qwen/projects/<sanitizeCwd(worktreePath)>, but nothing ever removes it: Config.shutdown only drops the in-memory map entry, and crashes skip it entirely. Since temp worktree paths are single-use, the snapshots pile up as orphans whose transcripts all point at deleted paths (QwenLM#7906). On the first Storage construction per runtime base dir, sweep project dirs whose worktree sidecar points at a path that no longer exists, in the background so startup is not blocked. A single valid sidecar judges the whole dir because every session under it shares the same root. Anything that cannot prove itself stale (no sidecar, corrupted sidecar, live worktree path) is kept, so normal project dirs and renamed projects are never touched. A startup sweep also covers the crash path, which a shutdown-time delete cannot. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
A normal project bucket can hold worktree sidecars too: enter/exit run from the original repo never relocates session storage, so the sidecar lands in projects/<sanitizeCwd(originalRepo)>/chats/. Once the worktree is gone, the all-gone check matched and the sweep destroyed the whole normal bucket along with the repo's chat history. Skip any bucket whose name is not a sanitized worktree path, and cover the normal-bucket case with a test.
sanitizeCwd collapses distinct worktrees to one bucket name, so a stale sidecar can pass the name gate for a bucket that also holds a sidecar-less live session. A live runtime.json anywhere in the bucket now vetoes the sweep (hostname mismatch or a pid that answers kill(pid, 0) counts as live, mirroring getRuntimeStatusPathState). A plain file at the worktree path no longer counts as a live worktree either. The order-dependent sweep test now pins the stuck entry first in the readdir mock.
The name gate only matched buckets keyed by the worktree path itself, which structurally excluded QwenLM#7906's main orphan class: enter_worktree from a throwaway cwd inside the OS temp dir lands its sidecar in a bucket keyed by that launch cwd, with worktreePath nested under it, so the gate never matched and the bucket survived forever. Add a second arm: for a gate-mismatched bucket, sweep only when every parseable sidecar places its originalCwd inside os.tmpdir() and that cwd is gone too, and every worktree is gone. A launch cwd outside the temp dir, a missing originalCwd, or a still-present one always keeps the bucket, since an absent repo dir can mean an unplugged drive rather than garbage. Also sort the bucket entries so the sweep order is deterministic for tests and logs.
The second arm judged staleness from the sidecar fields alone, so a normal project bucket that merely held an ephemeral-launch sidecar after a /cd relocation would qualify once its launch cwd and worktree were gone, and the repo's history would be swept with it. Require the bucket entry to equal sanitizeCwd(originalCwd) as well, so only a bucket actually keyed by the gone ephemeral cwd is removed. New test covers the relocated-sidecar shape. From Kimi Code
Round ten's review, all four verified by repro before fixing: R2-2 was real: the static edges storage.ts -> debugLogger.ts (which imports Storage back) and storage.ts -> runtimeStatus.ts -> atomicFileWrite.ts -> debugLogger.ts crashed forked children whose module graph reaches debugLogger first (__name is not a function at createDebugLogger in atomicFileWrite.ts), killing 9 lease tests that pass on main. The sweep now lazy-imports both inside the functions that use them; the lease suite is back to 83/86 and the exact fork from the repro runs clean. R3-2: archiveSessions moves transcript and sidecar into chats/archive/ as an explicit retention action, so an archived sidecar is keep evidence, never deletion evidence. The archived fixture now includes its transcript and asserts the bucket survives. R4-2: arm 1 treated ENOENT on the worktree path as proof of deletion, but a downed or unmounted volume reads the same way. Deleting now also requires the owning repo (originalCwd) to still exist, matching the unplugged-drive guard the docstring already promised. R4-1: runtime.json is only written by interactive sessions, so headless/serve/ACP/daemon sessions had no liveness veto. A chats /*.jsonl touched within a 10-minute grace window vetoes too, which is the mode-agnostic signal a running session always produces. From Kimi Code
…kets Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
…gate Round 7 review found the sweep's resolveThroughExistingAncestor duplicating rule-parser's realpathNearestExisting. Move the helper into utils/paths (with its symlink-hop walk), reuse it from both, and pin the loop-gives-up branch in paths.test.ts. The sweep keeps its keep-bias by falling back to the raw candidate when the helper gives up, which then fails the tmpdir containment check.
Round 8 review found the constructor scheduled the sweep unconditionally, so the package's own unit tests (105+ bare Storage constructions against the real default dir) could sweep a developer's live ~/.qwen. Gate it behind enableStartupSweep(), called once from the CLI main(); bare constructions are inert, pinned by storage-sweep-gate.test.ts (red without the gate). Also stop the ephemeral-launch arm when the resolved tmpdir is the filesystem root (TMPDIR=/ would otherwise make every launch cwd 'inside tmp').
…orktree Round 9 review re-ran two families. The archive-retention veto keyed on a parseable archived sidecar, but the sidecar move is best-effort and a corrupted one reads as nothing: any entry under chats/archive/ now keeps the bucket, no parsing. And sanitizeCwd collapses fix.bug/fix-bug to one bucket name, so arm 1 could delete a shared bucket while the cold co-owner worktree still exists on disk: enumerate the owning repo's .qwen/worktrees/ and keep when any live sibling resolves to the bucket. Also arm the sweep on the qwen serve route, which bypasses the CLI main() where enableStartupSweep() is called.
|
Rebased onto current main. The conflict was real, not textual: #8636 kept a local copy of the realpath helper in rule-parser.ts and #8687 landed its own realpathNearestExisting in utils/paths.ts, so my R7 sharing commit would have exported a duplicate. Dropped the sharing commit entirely; rule-parser.ts and utils/paths.ts now have zero net diff, and the sweep gate imports main's never-throws helper (an unresolvable candidate degrades to lexical form, fails the tmpdir containment check, bucket kept; same keep-bias as before, one less code path). 236 tests green across the sweep and paths suites. |
90445b2 to
baa46fb
Compare
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI; the macOS-specific realpath containment path this PR relies on is not exercised on a macOS runner.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the Windows-specific sanitizeCwd casing path is not exercised on a Windows runner.
Test Plan (not a blocker): src/config/storage.test.ts — no such file or directory.
Deferred under the convergence posture (round 11, not a blocker) — recorded, not requested in this round:
packages/core/src/config/storage.ts:362 — [review] hasLiveRuntime's raw kill(pid, 0) probe counts zombies live; the shared isPidAlive helper excludes them, so an unreaped zombie pid vetoes the sweep for that bucket foreverpackages/core/src/config/storage.ts:113 — [probe] the tmpdirIsUsable guard (TMPDIR=/ disarm for arm 2) has no falsifying test; the deletion mutant survives 26/26packages/core/src/config/storage.ts:359 — [probe] a mismatched-hostname runtime.json vetoes the sweep permanently — no freshness bound, and the record is never deleted on exit
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const { enableStartupSweep } = await import('@qwen-code/qwen-code-core'); | ||
| enableStartupSweep(); |
There was a problem hiding this comment.
[Critical] R10-1: prior-round blocker still stands at this rebased commit — probe-reconfirmed. The serve route arms the destructive startup sweep unconditionally, bypassing the QWEN_SANDBOX/SANDBOX disarm gate this same PR applies in gemini.tsx main() — a sandboxed serve daemon can delete host session data through the writable-mounted state dir using the container's partial filesystem view. The arm happens BEFORE tryRunServeFastPath, so the fast-path fallthrough into main() cannot substitute the gate either (main()'s gate only arms, never disarms).
Failure scenario: qwen serve inside a sandbox container is a product-recognized topology (root Dockerfile sets ENV SANDBOX; the shipped image has Env SANDBOX=qwen-code-sandbox; #7139's in-container serve wiring; run-qwen-serve.sandbox.test.ts): the host runtime base dir is mounted writable at the same canonical path (sandbox.ts:519-524; getContainerPath is identity on Linux), and the container's view of host paths is partial — a host bucket whose worktree/launch path is invisible from the container while its originalCwd is mounted passes the sweep's existence gates (headless bucket, cold transcript, no runtime.json) and is rm -rf'd from inside the container even though it exists on the host — the exact outcome the gemini.tsx gate comment exists to prevent.
Witness (container A/B on this PR's own compiled sweep at this commit):
ARM A (serve-route arm): ARMED removed=[host bucket] — bucket gone while the worktree still exists on the host
ARM B (gated arm): DISARMED sandbox marker present — bucket intact
| const { enableStartupSweep } = await import('@qwen-code/qwen-code-core'); | |
| enableStartupSweep(); | |
| // The serve daemon is long-lived and never reaches gemini.tsx main(): | |
| // arm the stale worktree sweep here so it fires once at startup — but | |
| // never inside a sandbox re-launch, where the host state dir is mounted | |
| // writable and the container's path view is partial. | |
| if (!process.env['QWEN_SANDBOX'] && !process.env['SANDBOX']) { | |
| const { enableStartupSweep } = await import('@qwen-code/qwen-code-core'); | |
| enableStartupSweep(); | |
| } |
— qwen3.8-max via Qwen Code /review (v0.21.13)
| try { | ||
| if ((await fsp.readdir(path.join(chatsDir, 'archive'))).length > 0) | ||
| continue; | ||
| } catch { | ||
| // no archive dir: nothing retained | ||
| } |
There was a problem hiding this comment.
[Critical] R10-2: prior-round blocker still stands at this rebased commit — probe-reconfirmed. The archive-retention gate's catch classifies every readdir failure as "no archive dir", so a non-ENOENT error (EACCES/EIO/ESTALE/ELOOP) on an existing non-empty chats/archive/ proceeds to the stale verdict and deletion — contradicting the invariant comment three lines above ("any entry at all under chats/archive/ keeps the bucket, no parsing involved" — which even names "a failed readdir would silently drop them all") and the keep-on-error bias of isDirectorySync/isPositivelyExistingDirectorySync in this same function.
Failure scenario: a bucket with a gone worktreePath, an existing originalCwd repo (arm 1 stale=true), and user-archived transcripts in chats/archive/: if readdir fails transiently (EIO/ESTALE on a downed or network mount, root_squash exports, DAC/ACL ownership mismatch between a root daemon and a user dir), the catch reads the archive as empty, archived transcripts never appear in the top-level transcript scan, no veto fires, and fsp.rm deletes explicitly user-retained archived data.
Witness (probe against the real sweep at this commit; EACCES injected on the archive readdir only):
control (readable archive) -> kept
EACCES archive -> removed=[bucket], bucket deleted
flip (keep unless ENOENT) -> kept
| try { | |
| if ((await fsp.readdir(path.join(chatsDir, 'archive'))).length > 0) | |
| continue; | |
| } catch { | |
| // no archive dir: nothing retained | |
| } | |
| try { | |
| if ((await fsp.readdir(path.join(chatsDir, 'archive'))).length > 0) | |
| continue; | |
| } catch (error) { | |
| // ENOENT: no archive dir, nothing retained. Any other failure means the | |
| // archive exists but cannot be listed — it cannot be proven empty, and a | |
| // destructive sweep keeps on doubt. | |
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') continue; | |
| } |
— qwen3.8-max via Qwen Code /review (v0.21.13)
| stale = | ||
| allWorktreesGone && | ||
| // A tmpdir root that will not stat cleanly is a downed volume, not an | ||
| // ephemeral scratch space: keep every bucket until it comes back. | ||
| tmpdirIsUsable && |
There was a problem hiding this comment.
[Critical] R9-2: sanitizeCwd collision family — still stands at this rebased commit, probe-reconfirmed. The round-9 fix (hasLiveSiblingWorktree) closes only the arm-1 same-repo shape; two probe-confirmed entrances remain open because sanitizeCwd collapses every non-alphanumeric to - while deletion is keyed on that lossy name: (1) arm 2 has no collision veto at all — hasLiveSiblingWorktree is called only inside arm 1 — so a live NON-worktree directory colliding with a gone tmpdir dir loses the shared bucket; (2) the sibling enumeration walks only <sidecar.originalCwd>/.qwen/worktrees/, so a live colliding worktree owned by a different repo with no sidecar in the shared bucket (e.g. after a /cd relocation) is invisible. Cold data has no liveness signal by definition, so the runtime.json/transcript vetoes cannot cover it.
Failure scenario (arm 2): /tmp/fix.bug (gone throwaway repo) and /tmp/fix-bug (live scratch dir) sanitize to the same bucket; the gone dir's sidecar passes arm 2 (key match, tmpdir-contained, gone); the live co-owner is headless and idle >10 min (the standing R7-1 gap), both vetoes are false, and fsp.rm deletes the live session's transcripts. Failure scenario (cross-repo): repos /x/a.b/r and /x/a-b/r whose worktree paths sanitize identically; the dead repo's /cd-relocated sidecar sits in the shared bucket; the sibling check walks only the dead repo; the shared bucket is deleted while the other repo's worktree is alive.
Witness (two executed probes at this commit, each flipped by the missing veto):
arm 2: gone fix.bug + live colliding fix-bug, headless-idle co-owner
pristine: removed=[entry] — live transcript deleted
+ arm-2 collision veto: removed=[]
cross-repo: x/a.b (dead) vs x/a-b (live), relocated sidecar in shared bucket
pristine: removed=[entry] with the live co-owner worktree on disk
+ collision-aware sibling walk: removed=[]
Fix direction (structural, on record since round 3): make bucket keys injective or pair deletion on the sidecar's own worktreePath; until then extend a collision veto to arm 2 and make the sibling check scan live directories/buckets whose sanitized name equals entry, not just the sidecar's own repo.
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if ( | ||
| (await hasLiveRuntime(chatsDir)) || | ||
| (await hasRecentTranscriptActivity(chatsDir)) | ||
| ) { |
There was a problem hiding this comment.
[Critical] R7-1: prior-round blocker still stands at this rebased commit — probe-reconfirmed (open threads 3791013674 / 3793779988 / 3799487293; the author acknowledged this is real and asked for a maintainer decision). The liveness veto leaves headless/serve/ACP sessions unprotected whenever their transcript has been quiet longer than the 10-minute grace: they never write runtime.json (markRuntimeStatusEnabled is called only in startInteractiveUI.tsx; the config.ts refresh sites are gated on runtimeStatusEnabled — verified again at this commit: one production caller), so transcript mtime is their only liveness signal — and any other qwen process starting up can sweep their bucket.
Failure scenario: a headless (qwen -p), qwen serve, ACP, or daemon session idle >10 minutes in a worktree deleted out from under it (or a colliding/tmpdir-keyed bucket per the R9-2 family): the arm gates pass, hasLiveRuntime finds no runtime.json, the transcript grace has lapsed, and fsp.rm deletes the live session's bucket — resumable history destroyed for a session that is still running.
Witness (probe boundary measurement at this commit):
headless bucket, transcript age 1 min -> kept
headless bucket, transcript age 11 min -> removed=[bucket]
same 11-min fixture + live runtime.json -> kept
Protection exists exactly when the session writes the liveness signal headless/serve/ACP modes never write.
Fix direction on record: make liveness session-mode-agnostic (write runtime.json for headless/serve/ACP sessions too) or adopt the session-level lock the author flagged — this is the open maintainer question the author escalated together with R9-1.
— qwen3.8-max via Qwen Code /review (v0.21.13)
| function scheduleStaleWorktreeSweep( | ||
| runtimeBaseDir: string, | ||
| keepBucket?: string, | ||
| ): void { | ||
| if (staleWorktreeSweepStarted.has(runtimeBaseDir)) return; |
There was a problem hiding this comment.
[Critical] R9-1: prior-round blocker still stands at this rebased commit — probe-reconfirmed (the author acknowledged it and asked for a maintainer call rather than stacking more heuristics). One-shot keepBucket protection: scheduleStaleWorktreeSweep dedupes on runtimeBaseDir alone and discards the keepBucket of every Storage constructed after the first, so the sweep can delete the bucket of a session the same process — now explicitly including the long-lived qwen serve daemon this PR arms — or another process is attaching to.
Failure scenario: the daemon's first Storage construction fixes keepBucket to one bucket and starts the async sweep; while it iterates, a second client resumes a session whose bucket qualifies as stale (gone worktree / gone tmpdir launch cwd, transcript older than the 10-minute grace, no runtime.json — serve sessions never write one); that session's keepBucket is dropped by this early return and its transcripts are rm -rf'd mid-resume.
Witness (probe at this commit — no race needed): two synchronous constructions new Storage(targetA, base); new Storage(targetB, base), second one resuming a session whose worktree is gone (stale by every gate except keepBucket):
pristine: bucketGone=true — the resumed session's bucket rm -rf'd
flip (per-base-dir keepBucket Set consulted in the loop): kept
The round-6 probe executed the same race 40/40.
Fix direction on record: track keep-buckets as a per-base-dir Set updated by every Storage construction (or re-derive the protected set inside the sweep loop); the session-level-lock direction is the open maintainer question.
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (!process.env['QWEN_SANDBOX'] && !process.env['SANDBOX']) { | ||
| enableStartupSweep(); | ||
| } |
There was a problem hiding this comment.
[Critical] R11-1: the disarm gate assumes every restricted-view execution environment carries a product sandbox re-launch marker; an UNMARKED environment with a writable-mounted host state dir — most notably a user-launched container (docker run -v $HOME/.qwen:/root/.qwen <image with no SANDBOX/QWEN_SANDBOX env>) — arms the destructive sweep with a partial view of host paths. Distinct from R10-1 (serve route skips the gate entirely): here the gate itself runs and still arms, so fixing the serve route does not close this class. Product-native sandboxes are all marked (sandbox.ts sets QWEN_SANDBOX/SANDBOX on every re-launch path; the shipped image sets ENV SANDBOX); the hole is the unmarked-environment class.
Failure scenario: the host has a #7906-class bucket keyed by a launch cwd inside the host os.tmpdir() whose session is headless or idle past the 10-minute grace. The user runs qwen in their own container with persisted state: inside, no marker → main() arms; Storage resolves onto the mounted host state dir; arm 2 reads the container's own /tmp as realTmpdir and the host originalCwd as gone (host /tmp not mounted); runtime.json pid probes are blind across the container's fresh pid namespace (host pids probe ESRCH → dead) and the transcript grace has lapsed → fsp.rm deletes the host bucket through the writable mount while the launch cwd still exists on the host — resumable transcripts destroyed.
Witness (real-container A/B at this commit; unmarked node:22-bookworm, host state dir mounted rw, fixture bucket keyed by a host tmpdir launch cwd, aged 2h):
unmarked arm: GATE-ARMED true, BUCKETS-AFTER [] — host bucket deleted while the host launch cwd still exists
flip arm (-e SANDBOX=qwen-code-sandbox): GATE-ARMED false, bucket survives
Fix direction: env markers cannot detect arbitrary restricted-view environments, so close the class structurally — (1) document that containerized runs mounting host state must set SANDBOX/QWEN_SANDBOX, and (2) harden arm 2 against cross-device judgment: record the tmpdir realpath (or device) in the sidecar at write time and require it to match the sweeping process's os.tmpdir() before reading an originalCwd as "gone".
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
The ubuntu Test red on the last head was the serve fast-path bundle check: my serve-route arming imported the core barrel, and the extra edge let esbuild hoist the iconv-lite chain into a chunk that acpAgent now reaches statically, tripping the ACP closure guard. Switched the arming to a narrow subpath import (new startupSweep export on core, same convention as toolWriteOrigin/memoryScopes). Reproduced the failure locally, then verified this head: full build, bundle, and scripts/check-serve-fast-path-bundle.js all pass, plus the sweep suites (675) and cli serve suites (5165) stay green. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
8 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- monorepo-subdirectory launch-cwd keying miss in arm 2 — already reported (comment 3793780013)
- sweep deletion audit trail is default-off — already reported (comment 3675686974)
- liveness vetoes' hostname/EPERM branches have no test — already reported (comment 3708132061)
- serve-route arm hunk survives reversion (no gating test) — already recorded in the round-10 deferred list (review 4957012879)
- gemini.tsx interactive-startup wiring hunks untested — already recorded in the round-10 deferred list (review 4957012879)
- default-off gate test inert to whole-feature reversion — already recorded in the round-10 deferred list (review 4957012879)
- constructor keepBucket wiring untested — already recorded in the round-10 deferred list (review 4957012879)
- tmpdirIsUsable (TMPDIR=/) guard has no falsifying test — already recorded in the round-11 deferred list (review 4962258432)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI; the macOS-specific realpath containment path this PR relies on is not exercised on a macOS runner.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the Windows-specific sanitizeCwd casing path is not exercised on a Windows runner.
Test Plan (not a blocker): src/config/storage.test.ts — no such file or directory.
Deferred under the convergence posture (round 12, not a blocker) — recorded, not requested in this round:
packages/core/src/config/storage.ts:227 — [review] JSDoc attaches to the wrong function and falsely claims chats/archive/ collectionpackages/core/src/config/storage.ts:60 — [probe] sync FS probes in the fire-and-forget sweep block the shared event loop; async variants existpackages/core/src/config/storage.ts:362 — [probe] hasLiveRuntime hand-copies the PID probe — accepts pid<=0 (permanent veto) and counts zombies livepackages/core/package.json:36 — [test] the ./startupSweep exports entry is ungated — no test fails if it is removed or mispointedpackages/cli/src/gemini.tsx:351 — [review] the gate's QWEN_SANDBOX term is host-side — env-sandbox users never get the sweep anywhere
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // arm the stale worktree sweep here so it fires once at startup. Import | ||
| // the storage module directly: pulling the core barrel into this route | ||
| // reshuffles the serve/ACP bundle chunks and breaks the closure guard. | ||
| const { enableStartupSweep } = await import( |
There was a problem hiding this comment.
[Critical] R12-1: The new @qwen-code/qwen-code-core/startupSweep subpath import breaks the entire packages/cli/src/cli.test.ts suite (55 tests) because packages/cli/vitest.config.ts has no alias for it — every other core subpath export (goalWire, transcriptRecords, userPromptSubmitContext, memoryScopes, toolWriteOrigin) has an explicit source-file alias there, and the catch-all '@qwen-code/qwen-code-core' alias prefix-matches the subpath and rewrites it to packages/core/index.ts/startupSweep, which does not exist. — Failure scenario: any npm test --workspace=packages/cli (local or CI): vite's import-analysis fails while loading cli.test.ts — the suite covering the very file this PR modifies — so all 55 tests are uncollected and the workspace test exits 1. Runtime/bundle resolution is unaffected (the exports map resolves to the built dist/src/config/storage.js); only the test path is broken.
Witness (base-vs-PR rerun):
PR tree: Failed to resolve import "@qwen-code/qwen-code-core/startupSweep" from "src/cli.ts" — 55 tests uncollected
base tree: 55/55 pass (netNew by test-delta)
Fix — add the missing alias in packages/cli/vitest.config.ts, matching the five sibling entries:
'@qwen-code/qwen-code-core/startupSweep': path.resolve(__dirname, '../core/src/config/storage.ts'),— qwen3.8-max via Qwen Code /review (v0.21.13)
| // the storage module directly: pulling the core barrel into this route | ||
| // reshuffles the serve/ACP bundle chunks and breaks the closure guard. | ||
| const { enableStartupSweep } = await import( | ||
| '@qwen-code/qwen-code-core/startupSweep' |
There was a problem hiding this comment.
[Critical] R12-2: The dev-mode ESM loader does not rewrite this subpath — scripts/dev.js's resolve hook matches only the exact bare specifier ('@qwen-code/qwen-code-core' → packages/core/index.ts), so the subpath falls through to the exports map → ./dist/src/config/storage.js. This is the first subpath import whose named export is absent from every pre-PR dist, so it breaks the documented dev workflow where the sibling subpaths (present in any recent dist) do not. — Failure scenario: npm run dev -- serve (and dev:daemon, which scripts/dev.js explicitly covers) on the normal dev state — stale pre-PR dist, per AGENTS.md's "no build needed" contract: the old storage.js has no enableStartupSweep export, the destructure yields undefined, and the call throws TypeError: enableStartupSweep is not a function, killing serve startup. With a fresh dist, the flag is armed on the DIST module instance while every Storage a serve session constructs arrives via the bare specifier → source instance, whose startupSweepEnabled stays false — the sweep silently never fires in the environment used to develop it.
Witness (probe executing this import block through a verbatim copy of dev.js's loader):
RESOLVE @qwen-code/qwen-code-core/startupSweep => packages/core/dist/src/config/storage.js (dist, not source)
stale-dist arm: typeof enableStartupSweep: undefined -> "enableStartupSweep is not a function"
fresh-dist arm: sub.Storage === bare.Storage: false (split instance, sweep never fires)
fixed-loader arm: sub.Storage === bare.Storage: true
Fix — in scripts/dev.js's loader hook, also rewrite the subpath to source (same shape as the bare-specifier rewrite and the vitest alias for R12-1): map @qwen-code/qwen-code-core/startupSweep → packages/core/src/config/storage.ts.
— qwen3.8-max via Qwen Code /review (v0.21.13)
| } catch { | ||
| // no archive dir: nothing retained | ||
| } |
There was a problem hiding this comment.
[Critical] R10-2: prior-round blocker still stands — probe-reconfirmed at this commit. The archive-retention gate's catch classifies every readdir(chats/archive) failure as "no archive dir", so a non-ENOENT error (EACCES/EIO/ESTALE/ELOOP) on an existing non-empty chats/archive/ proceeds to the stale verdict and deletion — contradicting the invariant comment three lines above ("any entry at all under chats/archive/ keeps the bucket, no parsing involved") and the sweep's own keep-on-error bias elsewhere (isDirectorySync: non-ENOENT → cannot prove gone → keep). — Failure scenario: a bucket whose top-level sidecars all point at gone worktrees (arm-1 stale, cold transcript) but whose user-archived sessions live under chats/archive/: if that one readdir returns anything other than ENOENT (a permission bit, a downed/flaky mount serving ~/.qwen), the catch proceeds as if nothing were retained and fsp.rm destroys data the user explicitly archived.
Witness (probe at this commit, fault-injected readdir on a non-empty archive, arm-1 stale shape):
EIO arm: removed=["<entry>"] — archived transcripts destroyed
control (readable archive): removed=[]
with ENOENT-only fix: removed=[] (flip), then reverted
| } catch { | |
| // no archive dir: nothing retained | |
| } | |
| } catch (error) { | |
| // ENOENT: no archive dir — nothing retained. Any other readdir failure | |
| // cannot prove the archive is empty, and a destructive sweep keeps. | |
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') continue; | |
| } |
— qwen3.8-max via Qwen Code /review (v0.21.13)
| allWorktreesGone && | ||
| // A tmpdir root that will not stat cleanly is a downed volume, not an | ||
| // ephemeral scratch space: keep every bucket until it comes back. | ||
| tmpdirIsUsable && |
There was a problem hiding this comment.
[Critical] R9-2: sanitizeCwd collision family — still stands, probe-reconfirmed at this commit. sanitizeCwd collapses every non-alphanumeric to - while worktree slugs allow [a-zA-Z0-9._-]+, so distinct worktrees (fix.bug vs fix-bug) or distinct repos (/tmp/my.repo vs /tmp/my-repo) key the SAME bucket while deletion is keyed on that lossy name. The round-9 hasLiveSiblingWorktree fix closes only the arm-1 same-repo shape; two probe-confirmed entrances remain open: (1) arm 2 has no collision veto at all — this staleness gate is its only gate; (2) cross-REPO collisions defeat the sibling scan (it only scans .qwen/worktrees under the repos named by the sidecars' originalCwd), and a plain-cd non-interactive tenant in the shared bucket writes no sidecar and no runtime.json, so after a >10-minute transcript gap no veto applies. — Failure scenario: repo R1's gone worktree .../worktrees/fix-bug and repo R3's live worktree .../worktrees/fix.bug sanitize to the same bucket; arm 1 computes stale=true from R1's sidecars and fsp.rm destroys the shared bucket including R3's live session transcript. Arm-2 variant: a gone ephemeral launch cwd r92.launch collides with the live r92-launch; the sweep deletes the bucket while the live cwd still exists.
Witness (probe at this commit):
cross-repo, arm 1: removed=[shared bucket] — live tenant's transcript deleted (sibling scan never sees R3)
arm-2 variant: removed=["-tmp-r92-launch"], liveCwdStillThere: true
same-repo control: kept (sibling scan works there)
Fix direction: close the class structurally — make deletion require a collision-proof key (record the unsanitized path the bucket was created for and require an exact match), or veto whenever any colliding live candidate cannot be ruled out; no sibling-scan variant can cover cross-repo sidecar-less tenants.
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if ( | ||
| (await hasLiveRuntime(chatsDir)) || | ||
| (await hasRecentTranscriptActivity(chatsDir)) | ||
| ) { |
There was a problem hiding this comment.
[Critical] R7-1: prior-round blocker still stands — probe-reconfirmed at this commit (the author acknowledged it is real and asked for a maintainer decision). The liveness veto leaves headless/serve/ACP sessions unprotected whenever their transcript has been quiet longer than the 10-minute grace: they never write runtime.json (markRuntimeStatusEnabled is called only in startInteractiveUI.tsx; every writeRuntimeStatus site is gated on it), so transcript mtime is their only liveness signal, and a quiet-but-live session has none. The comment above ("a recently touched transcript file vetoes too, covering headless/serve/ACP sessions") holds only while a turn is active. — Failure scenario: a serve/headless/ACP session inside a worktree is quiet >10 min (one long tool step, idle between turns); the worktree dir is removed externally during that window (manual git worktree remove, tmp cleanup); any new qwen process starts, all four staleness conjuncts pass, and fsp.rm deletes the live session's entire bucket — its next append recreates an empty bucket, so the history is silently gone.
Witness (probe at this commit against the real sweep):
transcript-age boundary: {9.9: keep, 9.99: keep, 10: delete, 10.1: delete} at TRANSCRIPT_LIVE_GRACE_MS=600000
grep: markRuntimeStatusEnabled only in startInteractiveUI.tsx; every writeRuntimeStatus site gated on runtimeStatusEnabled
Fix direction: give every session mode a process-anchored liveness signal — have non-interactive sessions also write/refresh <sessionId>.runtime.json (the queueRuntimeStatusWrite plumbing already exists in Config), or veto buckets referenced by a live session-registry record.
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (staleWorktreeSweepStarted.has(runtimeBaseDir)) return; | ||
| staleWorktreeSweepStarted.add(runtimeBaseDir); |
There was a problem hiding this comment.
[Critical] R9-1: prior-round blocker still stands — probe-reconfirmed at this commit (the author acknowledged it and asked for a maintainer call). One-shot keepBucket protection: scheduleStaleWorktreeSweep dedupes on runtimeBaseDir alone and discards the keepBucket of every Storage constructed after the first, so the sweep can delete the bucket of a session the same process is resuming/attaching. The serve daemon is multi-session on one shared base (server.ts defaults sessionRuntimeBaseDir to Storage.getRuntimeBaseDir()), and the first Storage construction can be an incidental settings/session-list load, not the session being resumed. — Failure scenario: the first Storage construction in the daemon arms the sweep with only its own bucket protected; a user resuming a session through the daemon while the async sweep is still walking gets no keepBucket protection; if that bucket is arm-1-shaped (worktree gone, repo present, transcript older than the 10-minute grace, no runtime.json — serve/headless sessions never write one), fsp.rm deletes the attaching session's transcripts mid-startup — the exact failure keepBucket was added to prevent.
Witness (probe at this commit):
enableStartupSweep(); new Storage(targetX, base) scheduled the sweep with keepBucket=X
new Storage(targetY, base) — the attaching session — was deduped -> bucketYExists=false (deleted mid-startup)
control passing keepBucket directly to sweepStaleWorktreeProjects: kept
Fix direction: keep a module-level Set<string> of keep buckets that each Storage constructor adds to and the running sweep consults per entry, instead of a single keepBucket captured at schedule time.
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (!process.env['QWEN_SANDBOX'] && !process.env['SANDBOX']) { | ||
| enableStartupSweep(); | ||
| } |
There was a problem hiding this comment.
[Critical] R11-1: prior-round blocker still stands — fresh container A/B at this commit; no structural hardening landed in this diff (the sidecar writer is untouched, arm 2 still judges originalCwd against the sweeping process's own os.tmpdir(), and this gate is still env-marker-only). The disarm gate assumes every restricted-view execution environment carries a product sandbox re-launch marker; an UNMARKED environment with a writable-mounted host state dir — most notably a user-launched container (docker run -v $HOME/.qwen:/root/.qwen <image with no SANDBOX/QWEN_SANDBOX env>) — arms the destructive sweep with a partial view of host paths. Distinct from R10-1 (serve route skips the gate entirely): here the gate itself runs and still arms, so fixing the serve route does not close this class. Product-native sandboxes are all marked; the hole is the unmarked-environment class. — Failure scenario: the host has a bucket keyed by a launch cwd inside the host os.tmpdir() whose session is headless or idle past the grace; the user runs qwen in their own container with persisted state: no marker → main() arms; arm 2 reads the container's own /tmp as realTmpdir and the host originalCwd as gone; runtime.json pid probes are blind across the fresh pid namespace (host pids probe ESRCH → dead); fsp.rm deletes the host bucket through the writable mount while the launch cwd still exists on the host.
Witness (container A/B at this commit, node:22-bookworm driving the built CLI, host state dir mounted rw, fixture bucket keyed by a host-tmpdir launch cwd alive on the host, transcript cold):
ARM A (unmarked): projects/ empty — host bucket deleted through the mount
ARM B (-e SANDBOX=qwen-code-sandbox): bucket intact
Fix direction: close the class structurally — record the tmpdir realpath (or device) in the sidecar at write time and require it to match the sweeping process's os.tmpdir() before reading an originalCwd as gone; additionally document that containerized runs mounting host state must set SANDBOX/QWEN_SANDBOX.
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
The ubuntu red was the vitest side of the new subpath: package exports point at dist, which does not exist in the test step, so vite import-analysis could not resolve it. Added the vitest alias to source, following the existing goalWire/memoryScopes convention (5a5bbf9). src/cli.test.ts passes 55/55 locally. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
15 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- JSDoc misattached to hasLiveSiblingWorktree with a false chats/archive/ claim — already recorded in the round-10 and round-12 deferred lists
- monorepo-subdirectory launch-cwd keying miss in arm 2 — already reported (comment 3793780013)
- sidecar readFile I/O errors conflated with JSON corruption — already reported (comment 3681228267)
- vacuous 'once per base dir' dedupe test fixture — already recorded in the round-10 deferred list
- vacuous archive-veto test fixture — already recorded in the round-10 deferred list
- corrupted-sidecar fall-through test cannot discriminate — already reported (comment 3681228263)
- hasLiveRuntime hostname/EPERM branches untested — already reported (comment 3708132061)
- TMPDIR-root guard untested — already recorded in the round-11 deferred list
- hasLiveRuntime hand-copies the PID probe (pid<=0 accepted, zombies counted live) — already recorded in the round-12 deferred list
- second sidecar parser duplicates readWorktreeSession — already reported (comment 3710393507)
- QWEN_SANDBOX host-side gate term (env-sandbox users never get the sweep) — already recorded in the round-12 deferred list
- sweep deletion audit trail default-off — already reported (comment 3675686974)
- default-off gate test inert to whole-feature reversion — already recorded in the round-10 deferred list
- serve-route arm and interactive-startup wiring hunks survive reversion — already recorded in the round-10 deferred list
- ./startupSweep exports entry ungated — already recorded in the round-12 deferred list
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI; the macOS-specific realpath containment path this PR relies on is not exercised on a macOS runner.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the Windows-specific sanitizeCwd casing path is not exercised on a Windows runner.
Not reviewed: build-and-test — packages/webui suite did not run locally (whole-call budget exhausted before it started).
Not reviewed: build-and-test — packages/web-shell suite timed out under the shrunken call budget (infrastructure).
Test Plan (not a blocker): src/config/storage.test.ts — no such file or directory.
Deferred under the convergence posture (round 13, not a blocker) — recorded, not requested in this round:
packages/cli/src/gemini.tsx:353 — [probe] gemini.test.tsx deletes both sandbox markers and calls the real main() — every call arms startupSweepEnabled in that file's module registry; latent sweep of the real ~/.qwen if a future test there c…packages/cli/src/cli.ts:382 — [review] serve-route comment 'never reaches gemini.tsx main()' is false — the fast path falls back to main() for flags absent from its mirror, '--', and bootstrap failurepackages/cli/vitest.config.ts:34 — [test] the startupSweep alias hunk survives reversion — no test imports the subpath, so the R12-1 fix is itself ungated
— qwen3.8-max via Qwen Code /review (v0.21.13)
| const { enableStartupSweep } = await import( | ||
| '@qwen-code/qwen-code-core/startupSweep' | ||
| ); | ||
| enableStartupSweep(); |
There was a problem hiding this comment.
[Critical] R10-1: prior-round blocker still stands — re-checked against the code at this commit (unchanged since round 12 except the vitest alias commit). The serve route arms the destructive startup sweep unconditionally, bypassing the QWEN_SANDBOX/SANDBOX disarm gate this same PR applies in gemini.tsx main() — a sandboxed serve daemon can delete host session data through the writable-mounted state dir using the container's partial filesystem view. The arm happens BEFORE tryRunServeFastPath, so the fast-path fallthrough into main() cannot substitute the gate either (main()'s gate only arms, never disarms).
Failure scenario: qwen serve inside a sandbox container is a product-recognized topology (the root Dockerfile sets ENV SANDBOX; the shipped image has Env SANDBOX=qwen-code-sandbox; the in-container serve wiring is a supported path): the host runtime base dir is mounted writable at the same canonical path (sandbox.ts pushes --volume without :ro; getContainerPath is identity on Linux), and the container's view of host paths is partial — a host bucket whose worktree/launch path is invisible from the container while its originalCwd is mounted passes the sweep's existence gates (headless bucket, cold transcript, no runtime.json) and is rm -rf'd from inside the container even though it exists on the host — the exact outcome the gemini.tsx gate comment exists to prevent.
Witness (container A/B on this PR's own compiled sweep, on the PR as prior-round threads):
ARM A (serve-route arm): ARMED removed=[host bucket] — bucket gone while the worktree still exists on the host
ARM B (gated arm): DISARMED sandbox marker present — bucket intact
| const { enableStartupSweep } = await import( | |
| '@qwen-code/qwen-code-core/startupSweep' | |
| ); | |
| enableStartupSweep(); | |
| if (!process.env['QWEN_SANDBOX'] && !process.env['SANDBOX']) { | |
| const { enableStartupSweep } = await import( | |
| '@qwen-code/qwen-code-core/startupSweep' | |
| ); | |
| enableStartupSweep(); | |
| } |
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // the storage module directly: pulling the core barrel into this route | ||
| // reshuffles the serve/ACP bundle chunks and breaks the closure guard. | ||
| const { enableStartupSweep } = await import( | ||
| '@qwen-code/qwen-code-core/startupSweep' |
There was a problem hiding this comment.
[Critical] R12-2: prior-round blocker still stands — scripts/dev.js is untouched by this diff. The dev-mode ESM loader does not rewrite this subpath — scripts/dev.js's resolve hook matches only the exact bare specifier ('@qwen-code/qwen-code-core' → packages/core/index.ts), so the subpath falls through to the exports map → ./dist/src/config/storage.js. This is the first subpath import whose named export is absent from every pre-PR dist, so it breaks the documented dev workflow where the sibling subpaths (present in any recent dist) do not.
Failure scenario: npm run dev -- serve (and dev:daemon, which scripts/dev.js explicitly covers) on the normal dev state — stale pre-PR dist, per AGENTS.md's no-build-needed contract: the old storage.js has no enableStartupSweep export, the destructure yields undefined, and the call throws TypeError: enableStartupSweep is not a function, killing serve startup. With a fresh dist, the flag is armed on the DIST module instance while every Storage a serve session constructs arrives via the bare specifier → source instance, whose startupSweepEnabled stays false — the sweep silently never fires in the environment used to develop it.
Witness (probe executing this import block through a verbatim copy of dev.js's loader, prior round):
RESOLVE @qwen-code/qwen-code-core/startupSweep => packages/core/dist/src/config/storage.js (dist, not source)
stale-dist arm: typeof enableStartupSweep: undefined -> "enableStartupSweep is not a function"
fresh-dist arm: sub.Storage === bare.Storage: false (split instance, sweep never fires)
fixed-loader arm: sub.Storage === bare.Storage: true
Fix — in scripts/dev.js's loader hook, also rewrite the subpath to source (same shape as the bare-specifier rewrite and the vitest alias that fixed R12-1): map @qwen-code/qwen-code-core/startupSweep → packages/core/src/config/storage.ts.
— qwen3.8-max via Qwen Code /review (v0.21.13)
| } catch { | ||
| // no archive dir: nothing retained | ||
| } |
There was a problem hiding this comment.
[Critical] R10-2: prior-round blocker still stands — probe-reconfirmed at this commit (catch block unchanged). The archive-retention gate's catch classifies every readdir(chats/archive) failure as "no archive dir", so a non-ENOENT error (EACCES/EIO/ESTALE/ELOOP) on an existing non-empty chats/archive/ proceeds to the stale verdict and deletion — contradicting the invariant comment three lines above ("any entry at all under chats/archive/ keeps the bucket, no parsing involved") and the keep-on-error bias the sweep states elsewhere.
Failure scenario: a bucket whose chats/archive/ exists and is non-empty but whose readdir fails with a permissions or transient-filesystem error (EACCES on an NFS root-squash mount, ESTALE/EIO flutter) is read as having no archive → the retention veto never materializes → the bucket, including the user-archived transcripts retention was designed to protect, is rm -rf'd.
Witness (prior-round probes at this PR's code): injected non-ENOENT readdir failure on chats/archive/ flips the outcome from kept to deleted; ENOENT-only classification keeps it.
| } catch { | |
| // no archive dir: nothing retained | |
| } | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') continue; | |
| } |
— qwen3.8-max via Qwen Code /review (v0.21.13)
| sidecar.originalCwd !== undefined && | ||
| entry === sanitizeCwd(sidecar.originalCwd) && |
There was a problem hiding this comment.
[Critical] R9-2: sanitizeCwd collision family — still stands, probe-reconfirmed at this commit. sanitizeCwd collapses every non-alphanumeric to - while worktree slugs allow [a-zA-Z0-9._-]+, so distinct worktrees (fix.bug vs fix-bug) or distinct repos (/tmp/my.repo vs /tmp/my-repo) key the SAME bucket while deletion is keyed on that lossy name. The round-9 hasLiveSiblingWorktree closes only the arm-1 same-repo shape; arm 2 — gated here — has no collision veto at all, so colliding gone/alive tmpdir launch cwds keying the same bucket can delete live data (the alive dir's plain-cd session writes no sidecar, and a cold transcript gets no mtime veto).
New entrance folded this round (same class): sanitizeCwd collisions cross path-segment boundaries (a.b ≡ a/b) and the sibling scan enumerates only one directory level — a live NESTED worktree (parent/.qwen/worktrees/child) sanitizes to the same bucket name as a flat slug (parent-.qwen.worktrees.child) yet is invisible to hasLiveSiblingWorktree, so its cold transcripts are deleted with the shared bucket. Computationally verified: sanitizeCwd('/repo/.qwen/worktrees/parent-.qwen.worktrees.child') === sanitizeCwd('/repo/.qwen/worktrees/parent/.qwen/worktrees/child'), and the nested path shape is natively creatable (sessionService.ts handles it).
Witness (prior-round probes): the shared bucket is deleted while the colliding worktree exists on disk; arm 2 has no veto call (code re-read at this commit).
Fix direction on record: a collision veto for arm 2, and a depth-aware sibling scan (or sidecar-paired deletion / injective bucket keys) so the lossy key can never authorize deletion of a co-owned bucket.
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if ( | ||
| (await hasLiveRuntime(chatsDir)) || | ||
| (await hasRecentTranscriptActivity(chatsDir)) | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Critical] R7-1: prior-round blocker still stands — probe-reconfirmed at this commit (the author acknowledged it is real and asked for a maintainer decision). The liveness veto leaves headless/serve/ACP sessions unprotected whenever their transcript has been quiet longer than the 10-minute grace: they never write runtime.json (markRuntimeStatusEnabled is called only in startInteractiveUI.tsx; every writer site re-grepped this round), so transcript mtime is their only liveness signal.
Failure scenario: a qwen serve session (or headless/ACP) idle for more than 10 minutes between requests holds a cold transcript and no runtime.json; its bucket keyed by a gone worktree satisfies every arm-1 conjunct and no veto fires → the sweep deletes the live session's transcripts mid-run; the writer recreates the dir, so pre-idle persisted history is silently lost.
Witness (prior-round probes): a headless-shaped bucket with a transcript aged past the grace is swept while the session is live; code re-read at this commit confirms runtime.json writing remains interactive-only.
Fix direction on record: make liveness session-mode-agnostic (write runtime.json for headless/serve/ACP sessions too, or consult the session registry) rather than relying on transcript freshness alone.
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (staleWorktreeSweepStarted.has(runtimeBaseDir)) return; | ||
| staleWorktreeSweepStarted.add(runtimeBaseDir); |
There was a problem hiding this comment.
[Critical] R9-1: prior-round blocker still stands — probe-reconfirmed at this commit (the author acknowledged it and asked for a maintainer call). One-shot keepBucket protection: scheduleStaleWorktreeSweep dedupes on runtimeBaseDir alone and discards the keepBucket of every Storage constructed after the first, and the first keepBucket is keyed on the launch cwd — not on buckets the process is about to read/resume — so the sweep can delete the bucket of a session the same process is resuming.
Failure scenario (resume trace re-verified this round): a session ran in worktree W of repo R; W is later removed (normal cleanup), transcript older than the grace, original process dead. The user runs qwen --resume from R: main() arms the sweep; the first Storage construction schedules it with keepBucket = sanitizeCwd(R); the resumed bucket sanitizeCwd(W) (a gone worktree can never equal the launch cwd) satisfies every arm-1 condition → rm -rf races sessionService.loadSession in the same startup — either "No saved session found with ID …" for a session the picker just listed, or the bucket is deleted mid-startup, destroying the transcript history. Same shape in serve: the daemon's first workspace supplies the only keepBucket; sessions attaching to any other workspace are unprotected.
Witness (prior-round probes): the process's own sweep deletes the resumed session's bucket within ~300ms after load, 40/40 runs.
Fix direction on record: defer the sweep until the resume target is known and pass every bucket the process attaches to (a keep-set, not one keepBucket).
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (!process.env['QWEN_SANDBOX'] && !process.env['SANDBOX']) { | ||
| enableStartupSweep(); | ||
| } |
There was a problem hiding this comment.
[Critical] R11-1: prior-round blocker still stands — no structural hardening landed in this diff (the sidecar writer is untouched, arm 2 still judges originalCwd against the sweeping process's own os.tmpdir(), and this gate is still env-marker-only). The disarm gate assumes every restricted-view execution environment carries a product sandbox re-launch marker; an UNMARKED environment with a writable-mounted host state dir — most notably a user-launched container (docker run -v $HOME/.qwen:/root/.qwen <image with no SANDBOX/QWEN_SANDBOX env>) — arms the destructive sweep with a partial view of host paths. Distinct from R10-1 (serve route skips the gate entirely): here the gate itself runs and still arms, so fixing the serve route does not close this class.
New entrance folded this round (same class): any namespace sharing ~/.qwen but with a DIFFERENT tmp dir (NFS-mounted home, PrivateTmp=true serve units, containers not mounting host /tmp) — arm 2 reads the other namespace's alive launch cwd as gone and deletes through the writable mount.
Failure scenario: the host has a stale-shaped bucket keyed by a launch cwd inside the host os.tmpdir() whose session is headless or idle past the 10-minute grace. The user runs qwen in their own container with persisted state: inside, no marker → main() arms; Storage resolves onto the mounted host state dir; arm 2 reads the container's own /tmp as realTmpdir and the host originalCwd as gone (host /tmp not mounted); runtime.json pid probes are blind across the container's fresh pid namespace (host pids probe ESRCH → dead) and the transcript grace has lapsed → fsp.rm deletes the host bucket through the writable mount while the launch cwd still exists on the host — resumable transcripts destroyed.
Witness (real-container A/B at this PR's code, prior round):
unmarked arm: GATE-ARMED true, BUCKETS-AFTER [] — host bucket deleted while the host launch cwd still exists
flip arm (-e SANDBOX=qwen-code-sandbox): GATE-ARMED false, bucket survives
Fix direction on record: env markers cannot detect arbitrary restricted-view environments, so close the class structurally — (1) document that containerized runs mounting host state must set SANDBOX/QWEN_SANDBOX, and (2) harden arm 2 against cross-device/cross-namespace judgment: record the tmpdir realpath (or device/host identity) in the sidecar at write time and require it to match the sweeping process before reading an originalCwd as gone.
— qwen3.8-max via Qwen Code /review (v0.21.13)
| try { | ||
| const stat = await fsp.stat(path.join(chatsDir, name)); | ||
| if (stat.mtimeMs >= cutoff) return true; | ||
| } catch { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Critical] R13-1: the transcript-recency veto's per-file stat catch fails open. This catch skips a transcript on ANY stat failure, not just ENOENT — so a transient non-ENOENT error (ESTALE/EIO on an NFS-backed home) on the bucket's only fresh transcript drops the final liveness veto immediately before fsp.rm. This veto is the sole liveness gate for headless/serve/ACP sessions by this PR's own design (the comment above the gate: runtime.json "is only written by interactive sessions"); the sweep's own sibling helper isDirectorySync encodes the opposite rule ("anything other than ENOENT answers 'cannot prove gone', which for a destructive sweep means keep"). Distinct from the archive-gate catch (R10-2) and from the readdir catches disclosed under Needs Human Review: this one fires per-file after a successful readdir.
Failure scenario: a headless/serve session runs from a worktree bucket whose worktree is gone (deleted out from under it, or a sanitizeCwd-collided bucket); its only fresh .jsonl is actively appended. The sweep's readdir(chats) succeeds, but fsp.stat on that transcript transiently fails; the catch continues, no other transcript is fresh, the veto returns false → rm -rf deletes the running session's transcripts.
Witness (three-arm probe at this commit):
bug arm (EIO injected on the fresh transcript, PR code): removed=["…gone-worktree"] bucketExists=false
fix arm (same injection, ENOENT-only skip): removed=[] bucketExists=true
control (no injection, PR code): removed=[] bucketExists=true
| try { | |
| const stat = await fsp.stat(path.join(chatsDir, name)); | |
| if (stat.mtimeMs >= cutoff) return true; | |
| } catch { | |
| continue; | |
| } | |
| try { | |
| const stat = await fsp.stat(path.join(chatsDir, name)); | |
| if (stat.mtimeMs >= cutoff) return true; | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') return true; | |
| } |
— qwen3.8-max via Qwen Code /review (v0.21.13)
|
The ubuntu Test red is not from this change. The failing assertion is in scripts/tests/qwen-autofix-workflow.test.js (expected SURVIVED, got spawn status=141), which my diff never touches (this PR is all packages/cli + packages/core storage). The same test file also fails on current main locally (2 failures, 178 passed, macOS), and 141 is SIGPIPE, which smells like a timing flake in the spawned bash fixture rather than a deterministic break. Leaving it alone per infra-red policy; a rerun may just pass. The vitest alias red from earlier is fixed by 5a5bbf9 (cli.test.ts 55/55 green locally). |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 12 scenario(s). — Qwen Code · serve A/B |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
11 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- JSDoc misattached to hasLiveSiblingWorktree with a false chats/archive/ claim — already recorded in the round-10 and round-12 deferred lists
- hasLiveRuntime hand-copies the PID probe instead of isPidAlive (pid<=0 accepted, zombies counted live) — already recorded in the round-12 deferred list
- gate comment misattributes the sandbox markers (QWEN_SANDBOX host-side; SANDBOX marks both container and seatbelt) — already recorded in the round-10 deferred list
- serve-route comment 'never reaches gemini.tsx main()' is false (fast-path fallback does) — already recorded in the round-13 deferred list
- vacuous 'once per base dir' dedupe test fixture — already recorded in the round-10 deferred list
- interactive-startup and serve arming wiring hunks untested (survive reversion) — already recorded in the round-10 deferred list
- liveness vetoes' hostname/EPERM branches have no test — already reported (comment 3708132061)
- tmpdirIsUsable (TMPDIR=/) guard has no falsifying test — already recorded in the round-11 deferred list
- sweep deletion audit trail is default-off — already reported (comment 3675686974)
- monorepo-subdirectory launch-cwd keying miss in arm 2 — already reported (comment 3793780013)
- clean-exit worktree buckets kept forever because the lifecycle unlinks the sidecar before the sweep runs — already reported (comment 3675686912)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: build-and-test — Test (macos-latest, Node 22.x) was skipped in CI; the macOS-specific realpath containment path this PR relies on is not exercised on a macOS runner.
Not reviewed: build-and-test — Test (windows-latest, Node 22.x) was skipped in CI; the Windows-specific sanitizeCwd casing path is not exercised on a Windows runner.
Test Plan (not a blocker): src/config/storage.test.ts — no such file or directory.
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const { enableStartupSweep } = await import( | ||
| '@qwen-code/qwen-code-core/startupSweep' | ||
| ); | ||
| enableStartupSweep(); | ||
| const { tryRunServeFastPath } = await import('./serve/fast-path.js'); |
There was a problem hiding this comment.
[Critical] R10-1: prior-round blocker still stands at this commit — re-verified against the code (the sweep sources are unchanged since round 13). The serve route arms the destructive startup sweep unconditionally, bypassing the QWEN_SANDBOX/SANDBOX disarm gate this same PR applies in gemini.tsx main(). qwen serve inside a sandbox container is a product-recognized topology (the shipped image sets ENV SANDBOX; in-container serve wiring exists): the host runtime base dir is mounted writable at the same canonical path, and the container's view of host paths is partial — a host bucket whose worktree/launch path is invisible from the container while its originalCwd is mounted passes the sweep's existence gates (headless bucket, cold transcript, no runtime.json) and is rm -rf'd from inside the container even though it exists on the host — the exact outcome the gemini.tsx gate comment exists to prevent. The arm happens BEFORE tryRunServeFastPath, so the fast-path fallthrough into main() cannot substitute the gate either (main()'s gate only arms, never disarms).
Witness: at this commit the serve route calls enableStartupSweep() with no marker gate while gemini.tsx gates the same call; prior-round container A/B on this PR's own compiled sweep: serve-route arm ARMED, removed=[host bucket] while the worktree still exists on the host; gated arm DISARMED, bucket intact.
| const { enableStartupSweep } = await import( | |
| '@qwen-code/qwen-code-core/startupSweep' | |
| ); | |
| enableStartupSweep(); | |
| const { tryRunServeFastPath } = await import('./serve/fast-path.js'); | |
| // The serve daemon is long-lived: arm the stale worktree sweep here so it | |
| // fires once at startup — but never inside a sandbox re-launch, where the | |
| // host state dir is mounted writable and the container's path view is | |
| // partial. SANDBOX marks both container and seatbelt children; | |
| // QWEN_SANDBOX additionally keeps the pre-hop host process disarmed. | |
| if (!process.env['QWEN_SANDBOX'] && !process.env['SANDBOX']) { | |
| const { enableStartupSweep } = await import( | |
| '@qwen-code/qwen-code-core/startupSweep' | |
| ); | |
| enableStartupSweep(); | |
| } |
— qwen3.8-max via Qwen Code /review (v0.22.0)
| // the storage module directly: pulling the core barrel into this route | ||
| // reshuffles the serve/ACP bundle chunks and breaks the closure guard. | ||
| const { enableStartupSweep } = await import( | ||
| '@qwen-code/qwen-code-core/startupSweep' |
There was a problem hiding this comment.
[Critical] R12-2: prior-round blocker still stands — scripts/dev.js is untouched by this diff. The dev-mode ESM loader's resolve hook matches only the exact bare specifier ('@qwen-code/qwen-code-core' → packages/core/index.ts, scripts/dev.js:77), so this subpath falls through to the exports map → ./dist/src/config/storage.js. This is the first subpath import whose named export is absent from every pre-PR dist, so it breaks the documented dev workflow where the sibling subpaths (present in any recent dist) do not. npm run dev -- serve (and dev:daemon) with a stale pre-PR dist: the old storage.js has no enableStartupSweep export, the destructure yields undefined, and the call throws TypeError: enableStartupSweep is not a function, killing serve startup. With a fresh dist, the flag is armed on the DIST module instance while every Storage a serve session constructs arrives via the bare specifier → source instance, whose startupSweepEnabled stays false — the sweep silently never fires in the environment used to develop it.
Witness: probe executing this import block through a verbatim copy of dev.js's loader (prior round): RESOLVE @qwen-code/qwen-code-core/startupSweep => packages/core/dist/src/config/storage.js; stale-dist arm: typeof enableStartupSweep: undefined -> "enableStartupSweep is not a function"; fresh-dist arm: sub.Storage === bare.Storage: false (split instance); fixed-loader arm: true. Re-verified this round: scripts/dev.js:77 still matches only the exact bare specifier.
Fix — in scripts/dev.js's loader hook, also rewrite the subpath to source (same shape as the bare-specifier rewrite and the vitest alias that fixed R12-1): map @qwen-code/qwen-code-core/startupSweep → packages/core/src/config/storage.ts.
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (!process.env['QWEN_SANDBOX'] && !process.env['SANDBOX']) { | ||
| enableStartupSweep(); | ||
| } |
There was a problem hiding this comment.
[Critical] R11-1: prior-round blocker still stands — no structural hardening landed in this diff (the sidecar writer is untouched, arm 2 still judges originalCwd against the sweeping process's own os.tmpdir(), and this gate is still env-marker-only). The disarm gate assumes every restricted-view execution environment carries a product sandbox re-launch marker; an UNMARKED environment with a writable-mounted host state dir — most notably a user-launched container (docker run -v $HOME/.qwen:/root/.qwen <image with no SANDBOX/QWEN_SANDBOX env>) — arms the destructive sweep with a partial view of host paths: host buckets whose worktree paths are invisible in the partial mount view pass the existence gates and are deleted while they exist on the host (prior-round container A/B probe confirmed deletion through the partial view when no marker is present).
Fix direction: structural hardening instead of markers — prove deletability from positive evidence only (require the owning repo AND every transcript's recorded cwd to be positively existing inside the sweeping process's view), or refuse to arm when the state dir is a bind-mount of a foreign tree. Moving the marker check into enableStartupSweep() (this round's agents re-derived the same direction) closes the per-call-site asymmetry with R10-1 but does not by itself close the unmarked-environment case.
— qwen3.8-max via Qwen Code /review (v0.22.0)
| } catch { | ||
| // no archive dir: nothing retained | ||
| } |
There was a problem hiding this comment.
[Critical] R10-2: prior-round blocker still stands — probe-reconfirmed in prior rounds, catch block unchanged at this commit. The archive-retention gate's catch classifies every readdir failure as "no archive dir", so a non-ENOENT error (EACCES/EIO/ESTALE/ELOOP) on an existing non-empty chats/archive/ proceeds to the stale verdict and deletion — contradicting the invariant comment three lines above ("any entry at all under chats/archive/ keeps the bucket, no parsing involved") and the keep-on-error bias the sweep applies everywhere else (isDirectorySync keeps on any non-ENOENT; isPositivelyExistingDirectorySync keeps on any stat error). A bucket whose archived sessions live under chats/archive/ then loses explicitly user-retained transcripts to one transient readdir error.
| } catch { | |
| // no archive dir: nothing retained | |
| } | |
| } catch (error) { | |
| // ENOENT means no archive dir: nothing retained. Any other failure | |
| // cannot prove the archive is empty, and for a destructive sweep that | |
| // means keep. | |
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') continue; | |
| } |
— qwen3.8-max via Qwen Code /review (v0.22.0)
| sidecar.originalCwd !== undefined && | ||
| entry === sanitizeCwd(sidecar.originalCwd) && |
There was a problem hiding this comment.
[Critical] R9-2: sanitizeCwd collision family — still stands, probe-reconfirmed in prior rounds and re-derived independently this round by two agents. sanitizeCwd collapses every non-alphanumeric to - while worktree slugs allow [a-zA-Z0-9._-]+, so natively creatable worktrees (fix.bug vs fix-bug) or repos (/tmp/my.repo vs /tmp/my-repo) key the SAME bucket while deletion is keyed on that lossy name. The round-9 fix (hasLiveSiblingWorktree) closes only the arm-1 same-repo shape — its only call site is inside arm 1; arm 2 has no collision veto at all. Concrete entrance: session A's enter_worktree from throwaway /tmp/my_tmp leaves a sidecar {originalCwd: /tmp/my_tmp} in the shared bucket, and /tmp/my_tmp is later removed; session B ran plain cd /tmp/my-tmp && qwen (no sidecar), exited, transcript cold. Arm 2 passes every conjunct for A's sidecar (entry === sanitizeCwd(originalCwd) matches via the collision, containment and gone-checks pass for the dead dir), no veto fires, and fsp.rm deletes B's still-resumable transcripts even though /tmp/my-tmp exists — violating the sweep's own "cannot prove stale ⇒ keep" standard.
Fix direction on record: before removing, prove no live directory keys this bucket — enumerate existing dirs whose sanitizeCwd equals the entry, or read each chats/*.jsonl's first-record cwd and keep when it is a positively-existing directory distinct from the sidecars' originalCwd values; or move to sidecar-paired deletion / injective bucket keys.
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if ( | ||
| (await hasLiveRuntime(chatsDir)) || | ||
| (await hasRecentTranscriptActivity(chatsDir)) | ||
| ) { |
There was a problem hiding this comment.
[Critical] R7-1: prior-round blocker still stands at this commit (the author acknowledged it is real and asked for a maintainer decision). The liveness veto leaves headless/serve/ACP sessions unprotected whenever their transcript has been quiet longer than the 10-minute grace: they never write runtime.json (markRuntimeStatusEnabled is called only in startInteractiveUI.tsx; the config.ts refresh sites are gated on runtimeStatusEnabled — re-verified by grep at this commit), so transcript mtime is their only liveness signal. Reachability is stronger than it looks: a live session's own bucket can be keyed by a gone worktree (enter/exit lifecycle), so with sanitizeCwd collisions and sidecar-less plain-cd sessions a live non-interactive session's transcript can be deleted mid-run — the veto pair above is the only gate before fsp.rm.
Fix direction on record: make liveness session-mode-agnostic — write runtime.json for headless/serve/ACP sessions too, or give non-interactive sessions a freshness proof the sweep must honor before any deletion.
— qwen3.8-max via Qwen Code /review (v0.22.0)
| const stat = await fsp.stat(path.join(chatsDir, name)); | ||
| if (stat.mtimeMs >= cutoff) return true; | ||
| } catch { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Critical] R13-1: the transcript-recency veto's per-file stat catch fails open — still stands at this commit (catch unchanged). This catch skips a transcript on ANY stat failure, not just ENOENT — so a transient non-ENOENT error (ESTALE/EIO on an NFS-backed home) on the bucket's only fresh transcript drops the final liveness veto immediately before fsp.rm. This veto is the sole liveness gate for headless/serve/ACP sessions by this PR's own design (see R7-1), so one swallowed error converts directly into deletion of a live session's history.
| const stat = await fsp.stat(path.join(chatsDir, name)); | |
| if (stat.mtimeMs >= cutoff) return true; | |
| } catch { | |
| continue; | |
| } | |
| const stat = await fsp.stat(path.join(chatsDir, name)); | |
| if (stat.mtimeMs >= cutoff) return true; | |
| } catch (error) { | |
| // ENOENT means the transcript is gone; any other failure cannot prove | |
| // the bucket is cold, and for a destructive sweep that means keep. | |
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') return true; | |
| continue; | |
| } |
— qwen3.8-max via Qwen Code /review (v0.22.0)
| if (staleWorktreeSweepStarted.has(runtimeBaseDir)) return; | ||
| staleWorktreeSweepStarted.add(runtimeBaseDir); |
There was a problem hiding this comment.
[Critical] R9-1: prior-round blocker still stands — probe-reconfirmed at this commit (the author acknowledged it and asked for a maintainer call rather than stacking more heuristics). One-shot keepBucket protection: scheduleStaleWorktreeSweep dedupes on runtimeBaseDir alone and discards the keepBucket of every Storage constructed after the first, and the first keepBucket is keyed on the launch cwd — not on buckets the process is about to read. A session whose worktree bucket differs from the process's first-constructed bucket (resume/picker attach, serve workspace bind, /cd relocation) can have that bucket swept out from under it by the same process — prior-round probe: the process's own sweep deletes the resumed session's bucket within ~300ms after load (transcripts are append-only and the writer recreates the dir, so pre-resume persisted history is lost).
Fix direction on record: carry every constructed bucket into a keep-set (or defer the sweep until after session restore), so later constructions' keepBucket is honored.
— qwen3.8-max via Qwen Code /review (v0.22.0)
4 participants
What this PR does
Fixes #7906. Worktree sessions register a project snapshot under
.qwen/projects/<sanitizeCwd(worktreePath)>, but nothing ever removes it:Config.shutdown()only drops the in-memory map entry, and crash/force-kill paths skip cleanup entirely. Since temp worktree paths are single-use random strings, the snapshots pile up as orphans whose transcripts all point at deleted paths.The first
Storageconstruction per runtime base dir now sweeps those orphans in the background: for every project dir with a worktree sidecar, if the sidecar'sworktreePathno longer exists, the dir is removed. Startup is not blocked (fire-and-forget with a per-dir once-guard).Why it's needed
On a real install, 98 of 111 entries in
.qwen/projects/were stale temp-session snapshots. They are small now but grow unbounded, and every one of them is dead weight that--resumecannot use anyway (the cwd it names is gone).Design choices worth a look:
worktreePathinstead of reversingsanitizeCwd, which is lossy).Reviewer Test Plan
How to verify
npx vitest run src/config/storage.test.tsinpackages/core: 5/5 pass (stale removed, live kept, no-sidecar kept, corrupted sidecar kept, missing projects dir tolerated, constructor schedules once per base dir).npx vitest run src/configfor the neighborhood: 492/492 pass.tsc --noEmitand eslint clean on both touched files.Evidence (Before & After)
N/A (background cleanup, no UI surface change).
Tested on