feat(tools): add generic worktree support — EnterWorktree/ExitWorktree + Agent isolation#4073
Merged
Conversation
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Collaborator
Author
手动测试创建worktree
### 拒绝非法名称
### 退出worktree
### dirty状态拒绝直接删除worktree
### agent tool创建的worktree无变更自动清理
|
tanzhenxin
requested changes
May 12, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
Nice scoping — Arena's existing worktree code is genuinely untouched, slug validation is tight, and the design doc is refreshingly honest about the enter_worktree → returned-path → model-must-use-it contract. That said, the same trust-the-model contract is the source of every load-bearing issue below: the isolation guarantees the PR promises don't hold once the subagent uses any relative path or unqualified shell command, and the cleanup paths quietly destroy evidence (and unpushed commits) when they shouldn't.
I'd like fixes for items 1–3 before merge; 5 and 6 amplify them, and 4 is a one-line hygiene fix that brings the new tools in line with every other niche tool in this codebase.
1. isolation: 'worktree' does not actually isolate; cleanup silently destroys evidence (severity: high · confidence: very high)
When isolation: 'worktree' is set, the worktree is provisioned and a notice is prepended to the task prompt, but the subagent's Config (and therefore its Edit / Write / Read / Shell tool bindings) still resolves to the parent project root. Any edit via a relative path, or any shell command without an explicit working-directory argument (npm test, rg foo, etc.), runs against the parent checkout. The cleanup helper then inspects only the worktree's git status, sees it clean, and removes the worktree plus its branch. End result: silent edits in the main checkout, no [worktree preserved: …] suffix in the result, no signal to the user that isolation was bypassed. Worth either actually switching the subagent's targetDir to the worktree, or downgrading the feature's framing so users know the isolation is advisory.
2. exit_worktree has no permission override and can delete a worktree + branch without confirmation (severity: high · confidence: very high)
ExitWorktreeInvocation inherits the default allow permission used for read-only tools. In default, auto-edit, and yolo modes it bypasses the confirmation dialog entirely, so a model that hallucinates the wrong slug and calls it with action: 'remove' and discard_changes: true destroys the worktree and force-deletes its branch with no user prompt. This is asymmetric with edit / write_file / run_shell_command, all of which prompt by default — a destructive tool of this class should override getDefaultPermission() so it shows up in the normal confirmation flow.
3. Force-deleting the branch loses unpushed commits from a clean isolated run (severity: high · confidence: very high)
Both the agent-isolation cleanup and exit_worktree action=remove pass deleteBranch: true, which runs git branch -D worktree-<slug> — that's an unconditional force-delete that discards unmerged commits. The "are there changes?" guard checks only working-tree state via git status, so a subagent that commits its work and ends with a clean tree falls through the "no changes → safe to delete" branch and loses its commits along with the branch. The [worktree preserved] path is never reached for committed work. Either drop to -d (which refuses force-delete on unmerged branches) or check git rev-list worktree-<slug> ^<base> before removing.
4. The new tools aren't deferred to ToolSearch and ship in every API request (severity: medium · confidence: high)
Both new tools are constructed without shouldDefer: true, so their function declarations are included in the initial function-declaration list on every API request, every turn, regardless of whether the user is doing worktree work. registerLazy defers module import, not declaration inclusion — those are separate axes. Every other niche / explicit-trigger tool in this codebase is deferred behind ToolSearch: AskUserQuestion ("rarely needed"), ExitPlanMode ("only used when leaving plan mode"), Monitor, SendMessage, TaskStop, the Cron* family, LspTool, McpTool. The class doc on shouldDefer explicitly says it mirrors Claude Code's framework — where these same two tools are deferred. exit_worktree's own description tells the model "Only invoke this tool when the user explicitly asks to leave or clean up a worktree", which is textbook deferral criteria. One-line fix: pass shouldDefer: true for both, and add a searchHint so ToolSearch can match them by keyword.
5. The stale-worktree cleanup utility is dead code (severity: medium · confidence: very high)
cleanupStaleAgentWorktrees is exported and unit-tested but has no caller anywhere in packages/core or packages/cli — only its own test references it. The PR description's "stale ephemeral worktree cleanup utility included" implies it runs somewhere; in practice nothing invokes it, so leaked ephemeral worktrees accumulate under .qwen/worktrees/ indefinitely. Either wire it into CLI startup / shutdown, or remove it from the PR.
6. The foreground isolation path leaks the worktree on uncaught throw (severity: medium · confidence: high)
The background path correctly invokes the cleanup helper in both the success block and the catch block. The foreground path only calls it inside the try block — the matching finally covers event listeners but not the worktree, so any throw before the cleanup call leaves the worktree and branch behind. Combined with #5, those orphans never get reaped. Mirroring the background path's pattern (cleanup in both success and error paths) closes this.
Verdict
REQUEST_CHANGES — issues 1, 2, and 3 are data-loss class. 4 is a token / hygiene fix. 5 and 6 ensure mis-cleaned worktrees stay forever once they occur.
LaZzyMan
added a commit
that referenced
this pull request
May 12, 2026
Six issues raised on the initial review; each addressed with a verifiable guarantee. 1. Real isolation for `agent isolation: 'worktree'` Before: subagent's Config still resolved `getTargetDir()` to the parent project root, so Edit/Write/Read workspace checks and Shell's default cwd silently operated on the parent tree. The cleanup helper then saw a "clean" worktree and removed it — destroying the evidence. After: the worktree is provisioned BEFORE `createApprovalModeOverride`, and the resulting agent Config has `getTargetDir`/`getCwd`/`getWorkingDir` rebound to the worktree path. Relative paths, unqualified shell commands, and glob/grep roots all confine to the worktree. 2. `exit_worktree action='remove'` now prompts in default/auto-edit modes Added `getDefaultPermission()` on the invocation: `'ask'` when action is `remove`, `'allow'` when `keep`. Brings it in line with edit, write_file, and run_shell_command. 3. Force-delete no longer silently destroys unpushed commits `removeUserWorktree` now uses `git branch -d` (refuses unmerged) by default and surfaces `branchPreserved: true` when git refuses. Added `hasUnmergedWorktreeCommits` (checks if branch tip is reachable from any other local branch or remote ref). Both the agent isolation cleanup and `exit_worktree action='remove'` use this check: if the branch has work not covered elsewhere, the worktree+branch are preserved even when `discard_changes: true` is set (there is no `discard_commits` flag — committed work is rarely what `remove` means to discard). 4. Both new tools are now deferred behind ToolSearch `shouldDefer: true` + `searchHint` on both. Verified via openai-logging: `enter_worktree` and `exit_worktree` no longer appear in the function- declaration list sent on every API request. 5. Stale-worktree cleanup is wired in `Config.initialize()` fires `cleanupStaleAgentWorktrees(targetDir)` as a non-awaited startup sweep (skipped in bare mode). Picks up orphaned `agent-<7hex>` worktrees left by crashed runs. 6. Foreground isolation no longer leaks on uncaught throw The foreground try block tracks whether the cleanup helper ran on the success path; the finally block invokes it as a fallback when the try bailed early. Mirrors the background path's pattern. Verification: - Unit tests: 83 passed (16 worktree + 64 existing agent + 3 cleanup) — no regressions. - E2E #1: agent told to write `hello.txt` via RELATIVE path — file landed at `.qwen/worktrees/agent-XXXXXXX/hello.txt`, NOT at the parent root. - E2E #3: created worktree, committed work inside it, called exit_worktree with `discard_changes=true` — refused with clear message; worktree and branch both preserved. - E2E #4: openai-logging confirms worktree tools absent from API tool list (7 tools sent instead of 9). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
Author
|
Pushed 1. Real isolation — worktree is now provisioned before 2. Permission prompt — 3. Force-delete protection — 4. Deferred behind ToolSearch — both tools now pass 5. Stale-worktree sweep wired in — 6. Foreground leak on throw — the foreground try block now tracks a Unit tests: 83 passed (16 new worktree tests + 64 existing agent tests untouched + 3 cleanup tests). E2E sanity checks for #1, #3, #4 all pass against the local build. |
Collaborator
Author
|
CI failure on Ubuntu/macOS is the I didn't touch |
Adds first-class git worktree as a general-purpose capability: Phase A — User-facing tools - enter_worktree: creates `<projectRoot>/.qwen/worktrees/<slug>` on a `worktree-<slug>` branch and returns the absolute path. Slug auto-generated when omitted; validated against path traversal and disallowed characters. - exit_worktree: keeps or removes the worktree (and its branch). Refuses to remove a worktree with uncommitted tracked changes or untracked files unless `discard_changes: true` is set. Phase B — Agent isolation - Agent tool gains an `isolation: 'worktree'` parameter that provisions a temporary `agent-<7hex>` worktree, prepends a worktree notice to the task prompt, and on completion either removes the worktree (no changes) or preserves it and reports its path/branch in the result. Background and foreground execution paths both wired up; rejected for fork agents. - worktreeCleanup.cleanupStaleAgentWorktrees: fail-closed sweep for ephemeral `agent-<7hex>` worktrees older than 30 days with no tracked changes and no unpushed commits. User-named worktrees are never swept. - buildWorktreeNotice helper for fork subagents (parity with claude-code). Arena compatibility - The existing Arena worktree implementation (GitWorktreeService.setupWorktrees, ArenaManager, agents.arena.worktreeBaseDir) is untouched. Arena uses its own batch APIs and `~/.qwen/arena` base dir; the new general-purpose APIs live alongside under `<projectRoot>/.qwen/worktrees/`. Subagent safety - enter_worktree / exit_worktree are added to EXCLUDED_TOOLS_FOR_SUBAGENTS so a subagent cannot mutate the parent session's worktree state. Refs #4056 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… Windows The Windows CI run reported `enter-worktree.test.ts` failing because the expected string was hardcoded with `/` while `getUserWorktreesDir()` uses `path.join`, which returns `\\` on Windows. Build the expected path via `path.join` so the platform-correct separator is compared. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Some models pass `{ "name": "" }` when calling EnterWorktree, because the
schema marks `name` as optional and they emit an empty placeholder. The
previous validation rejected the empty string with "Worktree name must be
a non-empty string", which surprised users running the auto-slug path.
Now both `validateToolParams` and `execute` treat `name: ""` as equivalent
to `name: undefined` and fall back to the auto-generated `{adj}-{noun}-{4hex}`
slug. Explicit invalid slugs (`'../etc'`, `'a/b'`, etc.) are still rejected
as before.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Six issues raised on the initial review; each addressed with a verifiable guarantee. 1. Real isolation for `agent isolation: 'worktree'` Before: subagent's Config still resolved `getTargetDir()` to the parent project root, so Edit/Write/Read workspace checks and Shell's default cwd silently operated on the parent tree. The cleanup helper then saw a "clean" worktree and removed it — destroying the evidence. After: the worktree is provisioned BEFORE `createApprovalModeOverride`, and the resulting agent Config has `getTargetDir`/`getCwd`/`getWorkingDir` rebound to the worktree path. Relative paths, unqualified shell commands, and glob/grep roots all confine to the worktree. 2. `exit_worktree action='remove'` now prompts in default/auto-edit modes Added `getDefaultPermission()` on the invocation: `'ask'` when action is `remove`, `'allow'` when `keep`. Brings it in line with edit, write_file, and run_shell_command. 3. Force-delete no longer silently destroys unpushed commits `removeUserWorktree` now uses `git branch -d` (refuses unmerged) by default and surfaces `branchPreserved: true` when git refuses. Added `hasUnmergedWorktreeCommits` (checks if branch tip is reachable from any other local branch or remote ref). Both the agent isolation cleanup and `exit_worktree action='remove'` use this check: if the branch has work not covered elsewhere, the worktree+branch are preserved even when `discard_changes: true` is set (there is no `discard_commits` flag — committed work is rarely what `remove` means to discard). 4. Both new tools are now deferred behind ToolSearch `shouldDefer: true` + `searchHint` on both. Verified via openai-logging: `enter_worktree` and `exit_worktree` no longer appear in the function- declaration list sent on every API request. 5. Stale-worktree cleanup is wired in `Config.initialize()` fires `cleanupStaleAgentWorktrees(targetDir)` as a non-awaited startup sweep (skipped in bare mode). Picks up orphaned `agent-<7hex>` worktrees left by crashed runs. 6. Foreground isolation no longer leaks on uncaught throw The foreground try block tracks whether the cleanup helper ran on the success path; the finally block invokes it as a fallback when the try bailed early. Mirrors the background path's pattern. Verification: - Unit tests: 83 passed (16 worktree + 64 existing agent + 3 cleanup) — no regressions. - E2E #1: agent told to write `hello.txt` via RELATIVE path — file landed at `.qwen/worktrees/agent-XXXXXXX/hello.txt`, NOT at the parent root. - E2E #3: created worktree, committed work inside it, called exit_worktree with `discard_changes=true` — refused with clear message; worktree and branch both preserved. - E2E #4: openai-logging confirms worktree tools absent from API tool list (7 tools sent instead of 9). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
4358d60 to
5d07cbe
Compare
tanzhenxin
reviewed
May 12, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
Re-reviewing the fix commit against the prior REQUEST_CHANGES round. The three data-loss-class findings (illusory isolation, auto-approved exit_worktree action='remove' in default mode, and force-delete of unmerged commits) are all genuinely closed, along with the deferred-tools wiring and the foreground-throw leak. Nice work — the choice to provision the worktree before the agent-config override and rebind the working-dir getters as own properties threads cleanly through the existing tool-registry rebuild path. One material residual remains.
1. The newly-wired cleanup sweep is inert on common-case repos (severity: medium · confidence: very high)
The stale-worktree sweep is now invoked at startup, which closes the prior dead-code finding. But the unpushed-commits guard inside it runs git log --branches --not --remotes --oneline from the worktree directory, which lists unpushed commits across every local branch in the repo, not just the worktree's own branch. On a repo with no remote configured, or any repo with stray unpushed local branches anywhere, the command always returns non-empty and the sweep continues past every candidate. Net effect: leaked ephemeral worktrees still accumulate indefinitely on the common case the sweep was meant to address. Reproducer: in a fresh git init repo with two branches and no remote, the command prints every commit regardless of which worktree dir you cd into. Scoping it to the specific branch (git log <branch> --not --remotes --oneline), or reusing the for-each-ref --contains <tip> pattern already in hasUnmergedWorktreeCommits, would do the right thing.
Verdict
COMMENT — Fixes for the data-loss-class findings landed cleanly and the architecture is sound. The cleanup-sweep scope bug above is a one-line fix worth landing before merge. Smaller residual items (AUTO_EDIT still auto-approves exit_worktree action='remove' via the default info confirmation type, the override is asymmetric on getProjectRoot/getWorkspaceContext/getFileService, no unit-test coverage on the new isolation rebinding) are reasonable to address in a follow-up.
wenshao
requested changes
May 12, 2026
| `Failed to create isolation worktree: ${created.error ?? 'unknown error'}`, | ||
| ); | ||
| } | ||
| worktreeIsolation = { |
Collaborator
There was a problem hiding this comment.
[Critical] Worktree isolation 设置（L1220 worktreeIsolation = {...}）之后、try/finally 块（L1880）才注册 cleanupWorktreeIsolation。中间约 660 行代码（createApprovalModeOverride、子 agent 创建、fork 处理等）如果抛出异常，worktree 目录和分支成为孤儿——外层 catch 没有访问清理闭包的途径。每次此类异常泄露一个完整的 git worktree 副本，只有 30 天后的 cleanupStaleAgentWorktrees 才能回收。
Suggested change
| worktreeIsolation = { | |
| // 将 worktreeIsolation 提升到外层 try 之前，在外层 catch 中调用清理 | |
| let worktreeIsolation: {...} | null = null; | |
| try { | |
| // ... provision, createApprovalModeOverride, agent creation, runFramed ... | |
| } catch (e) { | |
| if (worktreeIsolation) { | |
| await cleanupWorktreeIsolation(); | |
| } | |
| throw e; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // Best-effort: preserve or remove the isolation worktree before | ||
| // the registry record reflects the failure. | ||
| try { | ||
| await cleanupWorktreeIsolation(); |
Collaborator
There was a problem hiding this comment.
[Critical] 后台 agent 错误捕获路径中 await cleanupWorktreeIsolation() 的返回值被丢弃。如果 agent 崩溃前在 worktree 中做了部分工作，worktree 被保留但 registry.fail() / finalizeCancelled() 的消息中不包含 worktree 路径和分支名——用户完全不知道有残留的 worktree 可恢复。
Suggested change
| await cleanupWorktreeIsolation(); | |
| const wtSuffix = formatWorktreeSuffix(await cleanupWorktreeIsolation()); | |
| // 将 wtSuffix 附加到 registry.fail() / finalizeCancelled() 的消息中 |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| path: string; | ||
| branch: string; | ||
| } | null = null; | ||
| const failWorktreeProvisioning = (reason: string): ToolResult => { |
Collaborator
There was a problem hiding this comment.
[Critical] failWorktreeProvisioning 和 enter-worktree.ts 中所有 worktree 创建失败路径（git 不可用、非 git 仓库、createUserWorktree 失败）均无日志输出。同样，cleanupWorktreeIsolation 返回 preserved path 时也无日志。生产环境排查 worktree 相关问题时，grep 日志找不到任何线索。
Suggested change
| const failWorktreeProvisioning = (reason: string): ToolResult => { | |
| // 在每个 return failWorktreeProvisioning(...) / return errorResult(...) 前添加 | |
| debugLogger.warn('Worktree creation failed:', reason); | |
| // 在 preserved return 前添加 | |
| debugLogger.info(`Worktree preserved: ${isolation.path} (branch=${isolation.branch})`); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // commits" flag because losing committed work is rarely what the | ||
| // user means by "remove worktree". | ||
| if (!this.params.discard_changes) { | ||
| const counts = await service.countWorktreeChanges(worktreePath); |
Collaborator
There was a problem hiding this comment.
[Suggestion] 当 countWorktreeChanges 返回 null（git 检查失败——可能是权限问题、索引损坏等），错误消息建议用户传 discard_changes: true 来绕过。但 null 未必意味着存在变更，可能是非内容错误。不应建议绕过安全检查而不了解根因。
Suggested change
| const counts = await service.countWorktreeChanges(worktreePath); | |
| // 不提示 discard_changes，只报告检查失败 | |
| return errorResult( | |
| `Cannot inspect worktree "${this.params.name}" — git status failed. ` + | |
| `Check permissions and repository integrity, then try again.`, | |
| ); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| .map((s) => s.trim()) | ||
| .filter((s) => s.length > 0 && s !== `refs/heads/${branchName}`); | ||
| return refs.length === 0; | ||
| } catch { |
Collaborator
There was a problem hiding this comment.
[Suggestion] hasUnmergedWorktreeCommits 的 catch 块吞没 git 错误——直接 return true（fail-closed 是正确的），但未记录实际的 git 错误。如果 git for-each-ref 因仓库损坏而失败，用户只看到 "has unmerged commits" 但不知道真正原因。
Suggested change
| } catch { | |
| } catch (error) { | |
| debugLogger.warn( | |
| `hasUnmergedWorktreeCommits failed for ${slug}: ${error}`, | |
| ); | |
| return true; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const worktreePath = this.getUserWorktreePath(slug); | ||
| const branchName = `worktree-${slug}`; | ||
|
|
||
| const removed = await this.removeWorktree(worktreePath); |
Collaborator
There was a problem hiding this comment.
[Suggestion] removeUserWorktree 先调用 removeWorktree（删除 worktree 目录），然后才尝试 git branch -d。虽有前置的 unmerged commits 检查，但存在 TOCTOU 竞态窗口——检查与删除之间如果分支被推送了新提交，目录已被删除但分支被保留。建议交换顺序：先检查分支可删除性，再删除目录。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| }; | ||
| } | ||
| try { | ||
| const result = await wtService.removeUserWorktree(isolation.slug, { |
Collaborator
There was a problem hiding this comment.
[Suggestion] cleanupWorktreeIsolation 只检查了 result.branchPreserved，未检查 result.success。当 removeUserWorktree 返回 { success: false }（目录删除失败），代码静默落到 return {}——worktree 泄露且无日志。
Suggested change
| const result = await wtService.removeUserWorktree(isolation.slug, { | |
| const result = await wtService.removeUserWorktree(isolation.slug, { | |
| deleteBranch: true, | |
| }); | |
| if (!result.success) { | |
| debugLogger.warn(`Failed to remove worktree ${isolation.path}: ${result.error}`); | |
| return { preservedPath: isolation.path, preservedBranch: isolation.branch }; | |
| } | |
| if (result.branchPreserved) { ... } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
reviewed
May 13, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
补充一轮深入审计后的 8 条问题（不与已有 7 条 deepseek 评论重复）。Critical 两条涉及 isolation 实际不成立，建议作为 blocking 项处理。
| const ov = agentConfig as any; | ||
| ov.getTargetDir = () => wtPath; | ||
| ov.getCwd = () => wtPath; | ||
| ov.getWorkingDir = () => wtPath; |
Collaborator
There was a problem hiding this comment.
[Critical] Override 不完整。Config.getProjectRoot()（config.ts:1630）与 Config.getFileService()（config.ts:2147）直接读字段 this.targetDir，不经过 getTargetDir() getter。在 Object.create(base) 的原型委托下：agentConfig.getTargetDir() 返回 worktree 路径，但 agentConfig.getProjectRoot() 仍走原型链回到父的 targetDir —— 两个 getter 返回值不一致。
实际影响：
edit.ts:360/write-file.ts:109用getProjectRoot()做 auto-memory 权限判断 → 用的是父项目根read-file.ts:107用getWorkspaceContext()做工作区成员检查（PR 同样未 override 此 getter）→ 用的是父 workspaceglob.ts:96在构造时缓存config.getFileService()（父的 FileDiscoveryService，绑定父的 targetDir）；glob.ts:159用被 override 的getTargetDir()算path.relative()—— 与 fileService 的项目根不一致 → ignore 规则按错误的根评估
最坏路径：subagent 写出绝对/上溯相对路径，文件落在父工作区；run 结束时 hasWorktreeChanges(worktreePath) 看到 worktree 干净，cleanup 删除 worktree，agent 真实改动散落在父树且无任何 worktree 痕迹。这与 PR 描述里的 "tools confined to the worktree even when the model emits relative paths or unqualified shell commands" 相矛盾。
修复建议（按完整性递增）：
- 同步 override
getProjectRoot/getFileService/getWorkspaceContext - 改写为
ov.targetDir = wtPath（own-property 字段 shadow），让所有读字段者一并生效 - 至少把 cleanup 改成同时
git status父工作区，检测到外溢就保留 worktree 并报告
| } | ||
|
|
||
| async execute(_signal: AbortSignal): Promise<ToolResult> { | ||
| const projectRoot = this.config.getTargetDir(); |
Collaborator
There was a problem hiding this comment.
[Critical] 设计文档明确写"解析到 git 仓库根（处理已在子目录的情况）"，实现直接用 getTargetDir()。在 monorepo 里从 monorepo/packages/cli/ 启动 qwen 时，worktree 落在 monorepo/packages/cli/.qwen/worktrees/<slug>，而不是 monorepo/.qwen/worktrees/<slug>。
后果：Config.initialize 里 cleanupStaleAgentWorktrees(this.targetDir) 永远扫不到从其他 cwd 创建的 worktree，过期 agent worktree 永久堆积在每个子目录里。agent.ts:1204 同样存在此问题。
应在 GitWorktreeService 构造前用 git rev-parse --show-toplevel 解析到仓库根。
| await this.git.raw([ | ||
| 'worktree', | ||
| 'add', | ||
| '-B', |
Collaborator
There was a problem hiding this comment.
[High] -B 无条件重置已存在分支。注释说"用于复用同名 slug 旧分支"，但若用户碰巧有一个无关的 worktree-foo 分支（之前手动 git checkout -b worktree-foo 留下，或队友推过），新建同 slug worktree 会静默把那个分支 reset 到当前 base —— commits 全部丢失。exit_worktree 的 unmerged check 只在删除时保护，创建路径无对称检查。
建议改为 -b（仅创建），分支已存在则报错并提示用户改名或先 git branch -d；或先检查分支是否存在再决定是否 -B。
| * repo: `<projectRoot>/.qwen/worktrees`. | ||
| */ | ||
| getUserWorktreesDir(): string { | ||
| return path.join(this.sourceRepoPath, '.qwen', WORKTREES_DIR); |
Collaborator
There was a problem hiding this comment.
[High] <projectRoot>/.qwen/worktrees/ 在项目树内部，但 PR 没有自动写入 gitignore。两个后果：
- 项目若没把
.qwen/整体 gitignore，每次enter_worktree都污染父工作区的git status - Glob/Grep/RipGrep 从父根递归搜索时会下钻到
.qwen/worktrees/，把 worktree 内文件混入父的搜索结果（与上面 C1 叠加更危险 —— subagent 看到的"父文件"里其实有自己 worktree 的副本）
建议首次创建时写入 .qwen/.gitignore 包含 worktrees/，与 claude-code 行为对齐。
| if (!/^[a-zA-Z0-9._-]+$/.test(slug)) { | ||
| return 'Worktree name may only contain letters, digits, dots, underscores, and hyphens.'; | ||
| } | ||
| if (slug.includes('..') || slug.startsWith('.') || slug.startsWith('-')) { |
Collaborator
There was a problem hiding this comment.
[Medium] 验证规则在 Windows 上有边界漏洞：
- 允许尾部
.（如foo.），Windows 会规范化为foo，导致创建/删除路径不一致 - 不拒绝 Windows 保留名（
conauxnulprncom1-com9lpt1-lpt9），创建会硬失败或行为异常
建议参考 claude-code 的等价 helper 补全这两类检查。
| const NOUNS = ['fox', 'owl', 'elm', 'oak', 'ray', 'sky', 'leaf', 'pine']; | ||
| const adj = ADJECTIVES[Math.floor(Math.random() * ADJECTIVES.length)]; | ||
| const noun = NOUNS[Math.floor(Math.random() * NOUNS.length)]; | ||
| const suffix = Math.random().toString(16).slice(2, 6).padEnd(4, '0'); |
Collaborator
There was a problem hiding this comment.
[Medium] generateAutoSlug 用 Math.random()，agent slug 用 randomBytes —— 同一文件里两套随机源。空间 8 adj × 8 noun × 65536 hex ≈ 4M，长期项目里几百次 unnamed worktree 后碰撞概率明显，createUserWorktree 会返回 "Worktree already exists" 让用户重试。
randomBytes 已经被 import（agent slug 用），统一用它，并扩大词表或后缀位数。Math.random() 不该用作任何标识符的来源。
| const result = await service.removeUserWorktree(entry.name, { | ||
| deleteBranch: true, | ||
| }); | ||
| if (result.success) { |
Collaborator
There was a problem hiding this comment.
[Medium] 仅检查 result.success，未检查 result.branchPreserved。当分支 tip 有未合并 commit 时 git branch -d 拒绝删除，removeUserWorktree 返回 {success: true, branchPreserved: true} —— cleanup 计数为成功，但 worktree-agent-<7hex> 分支永远残留（前置 hasUnpushedCommits 检查应该挡住，但浅克隆/分离 ref 等边界场景能漏过去）。
建议在 branchPreserved 时打 warn 日志，方便运维定期清理孤儿分支。
| // await this: it is a hygiene task that must never delay the | ||
| // first model turn. | ||
| if (!this.getBareMode()) { | ||
| void import('../services/worktreeCleanup.js') |
Collaborator
There was a problem hiding this comment.
[Medium] 每次 session 启动都跑 sweep（动态 import + readdir），即使 99% 的用户从未用过 worktree。绝对开销小，但完全不必要。
建议先 fs.access(<projectRoot>/.qwen/worktrees) 短路：目录不存在直接返回。另外 .then(...).catch(...) 链会丢失原始错误堆栈上下文，建议改成 try/await import/catch 形式。
…8 from wenshao)
The first round closed the data-loss-class issues. This round addresses
follow-ups from a deeper audit:
1. Stale-worktree sweep was inert on common-case repos
`cleanupStaleAgentWorktrees` previously ran `git log --branches --not
--remotes --oneline` from each worktree's directory — that lists
unpushed commits across EVERY local branch, not just the worktree's
own branch. On any repo with no remote configured (or with stray
unpushed branches), the sweep refused to remove every candidate.
Replaced with `service.hasUnmergedWorktreeCommits(slug)` which scopes
the check to the worktree branch via `for-each-ref --contains <tip>`.
Also added the `branchPreserved` warn log requested in M7 and an
`fs.access` shortcut for the empty-worktrees-dir case (M8).
2. `cleanupWorktreeIsolation` and `worktreeIsolation` were inside the
inner try (~660 lines from the outer catch). Hoisted both to the top
of `execute()` so the outer catch can reap or preserve the worktree
when anything between provisioning and the inner try throws (e.g.
`createApprovalModeOverride`, agent creation). Closure carries the
resolved `repoRoot` so cleanup never has to re-resolve.
3. Background error path discarded the cleanup result. Now captures
`formatWorktreeSuffix(...)` and appends it to the registry's failure
/cancel message, so users see the preserved path/branch even when
the agent crashed before reporting.
4. `cleanupWorktreeIsolation` now treats `result.success === false` as
"worktree still on disk" and surfaces it as preserved instead of
silently dropping it from the result.
5. Override was incomplete. Several Config methods read `this.targetDir`
directly (`getProjectRoot`, `getFileService`, etc.) — own-property
getter overrides did not redirect them. Now also shadows `targetDir`
and `cwd` as own properties on the agent's Config override, swaps in
a `FileDiscoveryService` rooted at the worktree, and rebuilds
`WorkspaceContext` to point at the worktree only. Verified
end-to-end: shell `pwd > pwd-record.txt` (no directory arg) lands at
`.qwen/worktrees/agent-<7hex>/pwd-record.txt`, not the parent root.
6. monorepo subdir issue. Both `enter_worktree` and the agent isolation
path now resolve `git rev-parse --show-toplevel` first and anchor
`.qwen/worktrees/<slug>` at the repo root. Worktrees created from
any subdirectory now end up where the startup sweep can find them.
7. Replaced `git worktree add -B` (silent force-reset of pre-existing
branches) with `git worktree add -b` plus an explicit existence
check via `git for-each-ref` (NOT `show-ref --quiet`, which
simple-git swallows). Pre-existing `worktree-<slug>` branches now
trigger a clear error instead of clobbering committed work.
8. First worktree creation in a repo writes `<projectRoot>/.qwen/.gitignore`
with `worktrees/` so worktree contents stay out of the parent's
`git status`, glob/grep results, and bundle tools. Idempotent: never
overwrites an existing file.
9. Logging across the failure paths (`enter_worktree` errors,
`agent.ts:failWorktreeProvisioning`, `cleanupWorktreeIsolation`,
`hasUnmergedWorktreeCommits` swallowed errors,
`cleanupStaleAgentWorktrees`'s `branchPreserved` race).
10. `exit_worktree` no longer suggests `discard_changes: true` when the
git status check itself fails — that would be advising the user to
bypass a safety check whose precondition is unknown. Now points at
the underlying repo problem.
11. `generateAutoSlug` switched from `Math.random()` (4 hex, weak RNG,
one-in-65k collision) to `randomBytes` (6 hex, ~16M combinations).
Two RNG sources in this file collapsed to one.
Pushed back: the TOCTOU swap in `removeUserWorktree` (S6 round 1) is
left as-is — `git branch -d` is the real safety, and reordering does
not eliminate the window. Windows reserved-name validation (M5 round 2)
deferred to a follow-up; the current allowlist already rejects path
separators, `..`, leading dot/dash, and the >64-char case.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
Author
|
Pushed `d777ff37f` addressing the round-2 findings (1 from @tanzhenxin, 7+8 from @wenshao via DeepSeek). Verified end-to-end against the failure cases described. tanzhenxin
wenshao — round 1 (deepseek inline)
wenshao — round 2 (deeper audit)
Verification
|
CodeQL's `js/biased-cryptographic-random` flagged
`randomBytes(4)[i] % ARRAY.length` in `generateAutoSlug`. The math is
actually exact for the current word-list lengths (256 % 8 == 0), but
the lint rule does not know that — and a future contributor changing
the list to a non-power-of-two length would silently introduce bias.
Switched the index lookups to `crypto.randomInt(0, length)`, which uses
rejection sampling and is uniform by construction. Suffix still uses
`randomBytes(3).toString('hex')` since hex encoding is unbiased.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
wenshao
requested changes
May 13, 2026
| } | ||
|
|
||
| async execute(_signal: AbortSignal): Promise<ToolResult> { | ||
| const projectRoot = this.config.getTargetDir(); |
Collaborator
There was a problem hiding this comment.
[Critical] ExitWorktree 使用 this.config.getTargetDir()（当前工作目录）构建 GitWorktreeService，而 EnterWorktree 已正确通过 getRepoTopLevel() 解析到仓库根目录。从 monorepo 子目录（如 packages/core/）启动时，worktree 创建在 <repoRoot>/.qwen/worktrees/<slug>，但 ExitWorktree 去 <subdir>/.qwen/worktrees/<slug> 查找，永远返回 "Worktree not found"。
Suggested change
| const projectRoot = this.config.getTargetDir(); | |
| const cwd = this.config.getTargetDir(); | |
| const probe = new GitWorktreeService(cwd); | |
| const projectRoot = (await probe.getRepoTopLevel()) ?? cwd; | |
| const service = new GitWorktreeService(projectRoot); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // the try block bailed before reaching it. | ||
| if (!worktreeCleanupRan) { | ||
| try { | ||
| await cleanupWorktreeIsolation(); |
Collaborator
There was a problem hiding this comment.
[Critical] 双重清理竞态：当 runFramed() 抛出异常时，finally 块调用 cleanupWorktreeIsolation() 清理 worktree（删除目录），但未将 worktreeIsolation 置为 null。外层 catch（line ~2048）检测到 worktreeIsolation 仍为 truthy，再次调用清理——此时目录已不存在，hasWorktreeChanges() 在已删除路径上失败（fail-closed 返回 true），生成虚假的 [worktree preserved: /nonexistent/path] 消息。
Suggested change
| await cleanupWorktreeIsolation(); | |
| if (!worktreeCleanupRan) { | |
| try { | |
| await cleanupWorktreeIsolation(); | |
| } catch { | |
| // Helper logs its own failures; never mask the original | |
| // error path with cleanup noise. | |
| } | |
| worktreeIsolation = null; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // first model turn. | ||
| if (!this.getBareMode()) { | ||
| void import('../services/worktreeCleanup.js') | ||
| .then(({ cleanupStaleAgentWorktrees }) => |
Collaborator
There was a problem hiding this comment.
[Critical] 启动清理扫描传入 this.targetDir（cwd，可能是子目录），但 agent worktree 创建在仓库根目录下（getRepoTopLevel()）。从子目录启动 qwen 时，清理扫描查找 <subdir>/.qwen/worktrees/——该目录不存在——始终返回 0，30 天过期清理完全失效。
Suggested change
| .then(({ cleanupStaleAgentWorktrees }) => | |
| if (!this.getBareMode()) { | |
| void import('../services/worktreeCleanup.js') | |
| .then(async ({ cleanupStaleAgentWorktrees }) => { | |
| const { GitWorktreeService } = await import('../services/gitWorktreeService.js'); | |
| const svc = new GitWorktreeService(this.targetDir); | |
| const root = (await svc.getRepoTopLevel()) ?? this.targetDir; | |
| return cleanupStaleAgentWorktrees(root); | |
| }) | |
| .catch((error: unknown) => { | |
| this.debugLogger.debug( | |
| `Stale worktree sweep failed (non-fatal): ${error}`, | |
| ); | |
| }); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // above guarantees correctness; the notice reduces user-visible | ||
| // surprises when the model summarises file paths. | ||
| if (worktreeIsolation) { | ||
| const notice = buildWorktreeNotice( |
Collaborator
There was a problem hiding this comment.
[Suggestion] buildWorktreeNotice() 使用 this.config.getTargetDir() 作为 parentCwd，该值可能是子目录而非仓库根目录。worktreeIsolation.repoRoot 在此处已可用，应使用它以确保路径转换指引在子目录场景下正确。
Suggested change
| const notice = buildWorktreeNotice( | |
| const notice = buildWorktreeNotice( | |
| worktreeIsolation.repoRoot, | |
| worktreeIsolation.path, | |
| ); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| /** | ||
| * Lists all user worktrees in this repo by reading the worktrees directory. | ||
| */ | ||
| async listUserWorktrees(): Promise<WorktreeInfo[]> { |
Collaborator
There was a problem hiding this comment.
[Suggestion] listUserWorktrees() 方法定义后从未被任何代码调用，是死代码。且使用同步 execSync 在循环中执行 git 命令，如果将来被调用会阻塞事件循环。建议删除此方法，或在有调用方时改为异步实现。
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| ]); | ||
| return out.trim().length > 0; | ||
| } catch { | ||
| return false; |
Collaborator
There was a problem hiding this comment.
[Suggestion] localBranchExists() 的 catch 块完全为空（} catch { return false; }）。如果 for-each-ref 因磁盘满/权限/仓库损坏等原因失败，调用方误以为分支不存在继续尝试创建，且日志中无任何线索——凌晨 3 点排障时完全看不到根因。
Suggested change
| return false; | |
| } catch (error) { | |
| debugLogger.warn( | |
| `localBranchExists failed for ${branchName}: ${error}`, | |
| ); | |
| return false; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
The previous round added `getRepoTopLevel` for `enter_worktree`'s provisioning, but missed three sibling call sites that still used the raw cwd. The double-cleanup race in the foreground path also leaked stale `[worktree preserved]` suffixes on rejected promises. All six findings from the deeper audit are addressed: 1. exit_worktree now resolves through `getRepoTopLevel()` before building its `GitWorktreeService`, mirroring `enter_worktree`. Without this, launching `qwen` from a monorepo subdirectory created the worktree under the repo root but exit_worktree looked under the subdir's `.qwen/worktrees/` and always returned "Worktree not found". Verified end-to-end: enter + exit from `packages/core/` works. 2. agent.ts cleanup helper now nulls `worktreeIsolation` immediately after capturing the closure value. The previous structure could reach the helper twice — once in the foreground try's success path and once in the foreground finally fallback (or once in the inner try and once in the outer catch on a thrown rejection). The second call would `hasWorktreeChanges()` against a directory the first call already removed, fail-closed, and emit a bogus `[worktree preserved: <missing path>]` suffix. 3. Config.initialize's startup sweep now resolves `getRepoTopLevel()` before invoking `cleanupStaleAgentWorktrees`. Without this, every subdir launch scanned a non-existent `<subdir>/.qwen/worktrees/` and the 30-day expiry sweep was permanently a no-op. 4. agent.ts's `buildWorktreeNotice` now passes `worktreeIsolation.repoRoot` as `parentCwd` instead of `this.config.getTargetDir()`. The notice's path-translation guidance (≈ "translate paths from <parent> to <worktree>") would otherwise misdirect the subagent in a monorepo subdir launch. 5. Removed dead method `GitWorktreeService.listUserWorktrees`. It had no callers anywhere in the codebase and used `execSync` in a loop (would have blocked the event loop if anyone wired it up). 6. `localBranchExists` no longer swallows git failures silently. The defensive `false` default is preserved (so `git worktree add -b` itself surfaces the conflict if the check missed an existing branch), but the catch now logs via `debugLogger.warn` so disk-full / permission / ref-store-corruption cases are visible in debug output instead of being invisible. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
Author
|
Pushed `ae5aa557e` addressing the round-3 findings. The previous round added `getRepoTopLevel` for `enter_worktree`'s provisioning but missed three sibling call sites — the audit caught all of them. Critical
Suggestions
84 unit tests pass, typecheck clean, monorepo-subdir E2E sanity checked against the local build. |
wenshao
reviewed
May 13, 2026
| return removed; | ||
| } | ||
|
|
||
| async function hasTrackedChanges(worktreePath: string): Promise<boolean> { |
Collaborator
Author
There was a problem hiding this comment.
Could you expand on this? The comment is just "test", and hasTrackedChanges at line 138 doesn't have an obvious issue I can act on without more context. If this was a draft / accidental save, I'll resolve the thread; if you intended specific feedback (e.g. about the helper's behaviour, missing test coverage, the status field choice, or fail-closed handling), please point me at it and I'll address it.
wenshao
reviewed
May 13, 2026
| * - Only `[a-zA-Z0-9._-]` characters; no path separators | ||
| * - No `..` or leading/trailing dots (would resolve outside the worktrees dir) | ||
| */ | ||
| static validateUserWorktreeSlug(slug: string): string | null { |
Collaborator
Author
There was a problem hiding this comment.
Same as the other 'test' comment — could you expand? The line touches createUserWorktree near where the worktree directory is created. If this is a draft, feel free to resolve; if you intended specific feedback, please point me at it.
Collaborator
Author
|
Pushed `3e2002588` addressing the round-4 findings. Critical
Suggestion
Two "test" commentsReplied on both discussion 3232474087 and discussion 3232485143 asking for clarification — both look like stray drafts (just the literal word "test"). Happy to act once the intent is clear. 86 unit tests pass; typecheck clean. |
wenshao
requested changes
May 13, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Test coverage gaps (no specific diff line):
[Critical] exit-worktree.ts (294 new lines, 3 safety guards + branch deletion coordination) has no test file. [Critical] Agent worktree isolation (~200 new lines in agent.ts: config override, cleanup coordination, foreground/background paths) has zero coverage in agent.test.ts. [Suggestion] gitWorktreeService.ts new methods (~400 lines, 8 methods) lack dedicated tests (pre-existing test file not updated). [Suggestion] Duplicate errorResult() function in enter-worktree.ts and exit-worktree.ts.
| const wtService = | ||
| projectRoot === cwd ? probe : new GitWorktreeService(projectRoot); | ||
| const slug = `agent-${randomBytes(4).toString('hex').slice(0, 7)}`; | ||
| const created = await wtService.createUserWorktree(slug); |
Collaborator
There was a problem hiding this comment.
[Critical] Agent isolation worktree is created from the wrong branch.
wtService.createUserWorktree(slug) is called without a baseBranch argument. createUserWorktree falls back to getCurrentBranch(), which runs git rev-parse --abbrev-ref HEAD on the main repo root — returning main (or whatever the primary working tree has checked out). If the user is on feature-x, inside a user worktree (worktree-my-feature), or any non-main branch, the isolated subagent starts from main and sees the wrong code.
Suggested change
| const created = await wtService.createUserWorktree(slug); | |
| const parentBranch = await wtService.getCurrentBranch(); | |
| const created = await wtService.createUserWorktree(slug, parentBranch); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| * Counts uncommitted file changes in a worktree. Returns null if the | ||
| * worktree can't be inspected (which the caller should treat as "dirty"). | ||
| */ | ||
| async countWorktreeChanges( |
Collaborator
There was a problem hiding this comment.
[Critical] countWorktreeChanges misses conflicted files — bypasses the dirty-state guard in exit-worktree.
countWorktreeChanges manually sums staged + modified + deleted + renamed + created but omits status.conflicted. In simple-git, conflicted files appear only in conflicted[] (mutually exclusive with other arrays). A worktree with only merge conflicts (no other modifications) returns {tracked: 0, untracked: 0} — the dirty guard passes, exit_worktree proceeds to delete without requiring discard_changes: true. Conflict resolution progress is lost.
Suggested change
| async countWorktreeChanges( | |
| const tracked = | |
| status.staged.length + | |
| status.modified.length + | |
| status.deleted.length + | |
| status.renamed.length + | |
| status.created.length + | |
| status.conflicted.length; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const service = | ||
| projectRoot === cwd ? probe : new GitWorktreeService(projectRoot); | ||
|
|
||
| const worktreePath = service.getUserWorktreePath(this.params.name); |
Collaborator
There was a problem hiding this comment.
[Critical] exit_worktree lacks session ownership validation — any session can delete any worktree.
The design doc guarantees: 仅操作本会话通过 EnterWorktree 创建的 worktree. However, execute() only checks that the worktree directory exists — it never verifies that the current session created it. In yolo mode, a malicious prompt injection can enumerate .qwen/worktrees/, then call exit_worktree with any discovered name to delete another session's worktree.
Phase A mitigation: write a .session metadata file into the worktree directory at creation time, check it here before allowing action='remove'.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| : 'Enter a new worktree'; | ||
| } | ||
|
|
||
| async execute(_signal: AbortSignal): Promise<ToolResult> { |
Collaborator
There was a problem hiding this comment.
[Critical] Missing nested worktree prevention.
The design doc requires: 验证当前未在 worktree 中（防止嵌套）. Neither enter_worktree nor agent isolation checks whether the current working directory is already inside a worktree. The model can call enter_worktree again from within a worktree, creating nested .qwen/worktrees/<slug>/.qwen/worktrees/<slug> — path resolution breaks, nested worktrees are orphaned on exit.
Suggested change
| async execute(_signal: AbortSignal): Promise<ToolResult> { | |
| // At the top of execute(), before any worktree creation: | |
| if (cwd.includes('/.qwen/worktrees/')) { | |
| return errorResult('Already inside a worktree. Exit the current worktree first.'); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // Reached here when the branch had unmerged commits and the caller | ||
| // did not opt into force-delete. Surface this so callers can leave | ||
| // a note for the user. | ||
| return { success: true, branchPreserved: true }; |
Collaborator
There was a problem hiding this comment.
[Suggestion] removeUserWorktree reports preservedPath for an already-deleted directory.
removeUserWorktree deletes the worktree directory first (removeWorktree), then attempts branch deletion. If branch deletion fails (unmerged commits), it returns { success: true, branchPreserved: true }. Callers (cleanupWorktreeIsolation, cleanupStaleAgentWorktrees) then surface a preservedPath pointing to a directory that no longer exists — misleading users into thinking work is recoverable at that path (the commits are safe on the branch, but the message is wrong).
When branchPreserved: true, the preservedPath should be null or accompanied by a note that the directory was removed but the branch retains the commits.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return removed; | ||
| } | ||
|
|
||
| async function hasTrackedChanges(worktreePath: string): Promise<boolean> { |
Collaborator
There was a problem hiding this comment.
[Suggestion] hasTrackedChanges misses conflicted files and unnecessarily scans untracked files.
Same conflicted[] gap as countWorktreeChanges: conflicted-only worktrees appear clean and get swept. Also, simpleGit(worktreePath).status() runs full git status --porcelain including untracked-file discovery — the slowest part of git status on large repos — but only staged/modified/deleted/renamed/created arrays are checked. The comment says Skip the untracked-files scan, but status() still does it.
Suggested change
| async function hasTrackedChanges(worktreePath: string): Promise<boolean> { | |
| // Add conflicted check and use --untracked-files=no: | |
| const result = await wtGit.raw(['diff', '--name-only', '--exit-code']); | |
| if (result.trim().length > 0) return true; | |
| // Also check for staged changes: | |
| const stagedCheck = await wtGit.raw(['diff', '--name-only', '--cached', '--exit-code']); | |
| return stagedCheck.trim().length > 0; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // guidance would tell the agent to map worktree paths back to a | ||
| // monorepo subdir that may not contain the parent file at all. | ||
| if (worktreeIsolation) { | ||
| const notice = buildWorktreeNotice( |
Collaborator
There was a problem hiding this comment.
[Suggestion] buildWorktreeNotice uses repoRoot instead of the parent agent's actual CWD as parentCwd.
const notice = buildWorktreeNotice(
worktreeIsolation.repoRoot, // repo top-level, not parent's actual cwd
worktreeIsolation.path,
);If the parent agent is running from a monorepo subdirectory (e.g. packages/core/) or is itself inside a user worktree, the notice tells the subagent to translate paths relative to the repo root — but inherited context references paths relative to the parent's actual working directory. This mismatch can cause path-translation errors.
Suggested change
| const notice = buildWorktreeNotice( | |
| const notice = buildWorktreeNotice( | |
| this.config.getTargetDir(), // parent agent's actual working directory | |
| worktreeIsolation.path, | |
| ); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| logStartSession(this, new StartSessionEvent(this)); | ||
| this.debugLogger.info('Config initialization completed'); | ||
|
|
||
| // Fire-and-forget sweep of stale ephemeral worktrees left behind by |
Collaborator
There was a problem hiding this comment.
[Suggestion] Startup sweep runs git rev-parse --show-toplevel before checking if the worktrees directory exists.
The sweep dynamically imports GitWorktreeService, constructs an instance, and calls getRepoTopLevel() — all before cleanupStaleAgentWorktrees does fs.access(worktreesDir) which fast-bails if the directory is absent. For 99% of users who never use worktrees, a git subprocess is spawned on every session startup unnecessarily.
Suggested change
| // Fire-and-forget sweep of stale ephemeral worktrees left behind by | |
| // Check directory existence first, skip git entirely if absent: | |
| const worktreesDir = path.join(this.targetDir, '.qwen', 'worktrees'); | |
| try { await fs.promises.access(worktreesDir); } catch { return; } | |
| // ... then proceed with existing logic |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Self-review caught a handful of issues across three categories: Reuse: - `pathExists` in the new code now uses the existing `fileExists` from `utils/fileUtils.ts` instead of duplicating an `fs.access` wrapper. - `worktree-` branch prefix was string-literalled in five places. Added `WORKTREE_BRANCH_PREFIX` and `worktreeBranchForSlug(slug)` exports in `gitWorktreeService.ts`; updated `gitWorktreeService.ts`, `worktreeCleanup.ts`, and `exit-worktree.ts` to use them. Future prefix changes are a single edit. Efficiency: - `Config.initialize` used two `await import(...)` calls inside the startup-sweep IIFE, paying that cost on every CLI start. Switched to static imports at the top of `config.ts` — the modules are tiny and the dynamic indirection bought nothing. - `cleanupWorktreeIsolation` in `agent.ts` ran `hasWorktreeChanges` and `hasUnmergedWorktreeCommits` sequentially. They have no data dependency on each other and each spawns its own `git` invocation; `Promise.all` halves the cleanup wall-clock on the common path. Same fix in `worktreeCleanup.ts`'s per-entry loop. - `ensureWorktreesGitignored` used `fs.access` then `fs.writeFile`, a TOCTOU race when two agent invocations created worktrees concurrently (both could pass the `access` check and the second would clobber the first's `.gitignore`). Now writes with `flag: 'wx'` and treats `EEXIST` as the no-op case — atomic in one syscall. Quality: - Dropped the `worktreeCleanupRan` boolean in the foreground execution path. `cleanupWorktreeIsolation` already nulls its closure variable at the top of every call (see the comment at its definition), so re-entries are no-ops. The boolean and its tracking were dead weight that obscured the real guard. - Trimmed the Phase-2 override comment block to drop the WHAT-stating enumerations (items 3 and 4 just narrated the lines below) and removed a navigation comment about hoisted helpers — the helpers are visible at the top of the same method. 84 unit tests pass; typecheck clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ectness
Five critical findings + four suggestions, all closed.
Critical:
1. Wrong base branch for agent isolation. `createUserWorktree(slug)` with
no `baseBranch` arg fell back to `getCurrentBranch()` on the **main**
working tree, returning `main` regardless of which branch the user
was actually on. A subagent invoked from `feature-x` would silently
start from `main` and produce diffs against the wrong baseline.
`enter_worktree` had the same bug. Both now resolve the parent's
current branch first and pass it explicitly. Verified end-to-end:
`git checkout feature-x` → `enter_worktree` → worktree HEAD includes
the feature-x commit.
2. `countWorktreeChanges` (used by `exit_worktree`'s dirty-state guard)
missed `status.conflicted[]`. In simple-git that array is mutually
exclusive with the staged/modified/etc. arrays, so a worktree
mid-merge with only conflicts looked `{tracked: 0, untracked: 0}`
to the guard and `action='remove'` would proceed without
`discard_changes: true`. Added `+ status.conflicted.length`.
3. `exit_worktree` had no session-ownership check, contradicting the
design doc's "only operates on worktrees created by THIS session".
In yolo mode a prompt injection could enumerate `.qwen/worktrees/`
and pass any name to drop another session's work. Now:
`enter_worktree` and agent isolation write a `.qwen-session`
marker into the worktree at provisioning time; `exit_worktree
action='remove'` reads it and refuses if it does not match the
current `Config.getSessionId()`. Worktrees from before this guard
(no marker file) are treated as "owner unknown" — allowed with a
warn log so the change is observable.
4. `enter_worktree` did not refuse nested invocations from inside an
existing worktree, contradicting the design doc. Now rejects any
cwd containing `.qwen/worktrees/` as a path component, with a
clear "Already inside a git worktree…" message. Verified: enter
from inside a worktree returns is_error with that text.
6. `hasTrackedChanges` (cleanup sweep) had the same `conflicted[]`
gap. Rewrote to use raw `git status --porcelain --untracked-files=no`
which lists every tracked change including `UU` conflict markers
in a single git call and explicitly skips the untracked walk
(the prior comment claimed to skip it, but `status()` always
does the scan).
Suggestion:
7. `buildWorktreeNotice` now receives the parent agent's actual
`getTargetDir()` again (was switched to `repoRoot` in round 3 on
a different reviewer's suggestion; round-5 caught that the model's
inherited paths reference the parent's cwd, not necessarily the
repo root, so the prior behaviour was correct).
8. Startup sweep now does `fs.access(<targetDir>/.qwen/worktrees)`
*before* importing GitWorktreeService and spawning `git
rev-parse --show-toplevel`. The git probe is reserved for users
who actually have a worktrees directory locally — 99% of users
pay only one syscall on startup.
9. Tests:
- New `exit-worktree.test.ts` covers metadata, validation,
`getDefaultPermission` (ask vs allow), and getDescription.
- `agent.test.ts` adds three `validateToolParams` cases for the
`isolation` parameter (accepted with subagent_type, rejected
without, rejected for non-"worktree" values).
- `enter-worktree.test.ts` adds round-trip tests for
`writeWorktreeSessionMarker` / `readWorktreeSessionMarker` plus
a `worktreeBranchForSlug` sanity check.
- Total: 101 tests pass (was 86 → +15).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
Author
|
Pushed `1d7da201d` addressing round-5 findings. Five critical + four suggestions, all closed. Critical
Suggestion
Typecheck clean. Bundle built. E2E sanity check covered #1, #4, and the session marker write. |
Empty string `''` is a valid `string` type, so the @ts-expect-error directive on `validateToolParams({ name: '', action: 'keep' })` did nothing — TypeScript correctly accepted the line, and `tsc --build` in CI reported TS2578 ("Unused '@ts-expect-error' directive"). The runtime assertion already covers the case; the directive was leftover from an earlier draft. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The Arena test mocks `gitWorktreeService.js` with a factory that
returns only `{ GitWorktreeService }`. PR #4073 added several other
exports to that module (`AGENT_WORKTREE_SLUG_PATTERN`,
`WORKTREE_BRANCH_PREFIX`, `worktreeBranchForSlug`,
`generateAgentWorktreeSlug`, `writeWorktreeSessionMarker`,
`readWorktreeSessionMarker`, `WORKTREE_SESSION_FILE`).
Other modules in the dep graph reach the mocked surface — most
notably `worktreeCleanup.ts` imports `AGENT_WORKTREE_SLUG_PATTERN`
and `worktreeBranchForSlug`, and now reaches the mock via the static
`config.ts` → `worktreeCleanup.ts` import chain added in the
self-review pass. The Arena test failed at module-load with:
Caused by: Error: [vitest] No "AGENT_WORKTREE_SLUG_PATTERN" export
is defined on the "../../services/gitWorktreeService.js" mock. Did
you forget to return it from "vi.mock"?
Use `importOriginal` to capture every real export, spread it into
the return object, and only replace `GitWorktreeService` (the class
the test actually needs to mock). The class-level mock keeps its
existing static-method shims.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
wenshao
previously requested changes
May 13, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Test coverage gaps:
[Critical] exit-worktree.test.ts — execute() has zero test coverage. The session ownership guard (lines 136-147), owner-null fallback, keep action, and all three remove guards are untested.
[Critical] enter-worktree.test.ts — execute() has zero test coverage. Nested prevention regex, git-unavailable path, getCurrentBranch() fallback, and createUserWorktree failure paths are untested.
[Critical] agent.test.ts — isolation execution path has zero test coverage. provisionWorktreeIsolation, cleanupWorktreeIsolation, formatWorktreeSuffix (preservedBranch-only path), and Config override layer are untested.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return 'Worktree name must not start with "." or "-" or contain "..".'; | ||
| } | ||
| if (slug.startsWith(`${AGENT_WORKTREE_PREFIX}-`)) { | ||
| return ( |
Collaborator
There was a problem hiding this comment.
[Critical] validateUserWorktreeSlug rejects all agent- prefixed slugs, but generateAgentWorktreeSlug() produces exactly agent-<7hex> slugs. createUserWorktree() calls validateUserWorktreeSlug unconditionally before creating the worktree, so every AgentTool isolation:'worktree' invocation will fail with "Worktree name must not start with 'agent-'...". The agent isolation feature is completely broken by its own validation.
Suggested change
| return ( | |
| if (slug.startsWith(`${AGENT_WORKTREE_PREFIX}-`)) { | |
| // Allow exact agent-<7hex> slugs generated by generateAgentWorktreeSlug(); | |
| // reject user-chosen names that would collide with the ephemeral cleanup pattern. | |
| if (!AGENT_WORKTREE_SLUG_PATTERN.test(slug)) { | |
| return ( | |
| `Worktree name must not start with "${AGENT_WORKTREE_PREFIX}-": that prefix ` + | |
| `is reserved for ephemeral agent worktrees and is subject to ` + | |
| `automatic cleanup after 30 days.` | |
| ); | |
| } | |
| } |
Also update enter-worktree.test.ts:71 — agent-1234567 (7 hex chars) should now pass validation instead of being rejected.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const wtGit = simpleGit(worktreePath); | ||
| const status = await wtGit.status(); | ||
| // `not_added` covers untracked; `staged`/`modified`/etc. cover the rest. | ||
| return !status.isClean(); |
Collaborator
There was a problem hiding this comment.
[Critical] hasWorktreeChanges uses status.isClean() which does not check status.conflicted. In simple-git, conflicted is mutually exclusive with staged/modified/etc. — a worktree with only merge conflicts appears clean and gets silently deleted by cleanupWorktreeIsolation in agent.ts. This same PR correctly fixed countWorktreeChanges (used by exit_worktree) and hasTrackedChanges (used by cleanupStaleAgentWorktrees) to include conflicted, but missed this function which is used by the agent cleanup path.
Suggested change
| return !status.isClean(); | |
| const status = await wtGit.status(); | |
| return !status.isClean() || status.conflicted.length > 0; |
Or switch to git status --porcelain like hasTrackedChanges does for consistency.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const trimmed = raw.trim(); | ||
| return trimmed.length > 0 ? trimmed : null; | ||
| } catch { | ||
| return null; |
Collaborator
There was a problem hiding this comment.
[Critical] readWorktreeSessionMarker — catch { return null; } silently treats every I/O error (disk failure, permission error, corrupt NFS) as "marker file not found". When storage is failing, exit_worktree's ownership guard is bypassed, allowing a different session to delete worktrees it shouldn't. The only log message produced is the misleading "no session marker; allowing removal" — not the actual I/O error.
Suggested change
| return null; | |
| } catch (error) { | |
| debugLogger.warn( | |
| `readWorktreeSessionMarker: cannot read ${path.join(worktreePath, WORKTREE_SESSION_FILE)}: ${error}`, | |
| ); | |
| return null; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| let exists = false; | ||
| try { | ||
| const stat = await fs.stat(worktreePath); | ||
| exists = stat.isDirectory(); |
Collaborator
There was a problem hiding this comment.
[Critical] fs.stat catch block — catch { exists = false; } maps EACCES, EIO, ENOTDIR and every other error to "Worktree not found", with zero logging. An operator sees "Worktree not found" in the tool output while ls shows the directory clearly exists — the root cause (permissions, disk error) is invisible.
Suggested change
| exists = stat.isDirectory(); | |
| } catch (error) { | |
| if (isNodeError(error) && error.code === 'ENOENT') { | |
| exists = false; | |
| } else { | |
| debugLogger.warn( | |
| `exit_worktree: cannot stat ${worktreePath}: ${error}`, | |
| ); | |
| return errorResult( | |
| `Cannot access worktree at ${worktreePath} (${error}).`, | |
| ); | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| let mtimeMs: number; | ||
| try { | ||
| const stats = await fs.stat(worktreePath); |
Collaborator
There was a problem hiding this comment.
[Critical] cleanupStaleAgentWorktrees — catch { continue; } on fs.stat silently skips any entry where stat fails (permission error, disk error, unmounted filesystem). Stale worktrees accumulate permanently with no log trail. Operators grep for "Removed stale" and find nothing, but the worktree directories keep consuming disk.
Suggested change
| const stats = await fs.stat(worktreePath); | |
| } catch (error) { | |
| debugLogger.warn( | |
| `Cannot inspect worktree ${worktreePath} — skipping cleanup: ${error}`, | |
| ); | |
| continue; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| this.targetDir, | ||
| '.qwen', | ||
| 'worktrees', | ||
| ); |
Collaborator
There was a problem hiding this comment.
[Suggestion] Fast-bail fs.access(targetDir/.qwen/worktrees) runs before resolving the repo root. When qwen is launched from a monorepo subdirectory (e.g. packages/core/), targetDir/.qwen/worktrees/ never exists — worktrees live at <repoRoot>/.qwen/worktrees/. The code returns early, never calling cleanupStaleAgentWorktrees. The comment says "cleanupStaleAgentWorktrees will fast-bail there too" but it never reaches that code. Stale agent worktrees accumulate indefinitely for monorepo-subdir users.
Suggested change
| ); | |
| // Resolve the repo root first, then fast-bail against the actual worktreesDir. | |
| const probe = new GitWorktreeService(this.targetDir); | |
| const root = (await probe.getRepoTopLevel()) ?? this.targetDir; | |
| const worktreesDir = path.join(root, '.qwen', 'worktrees'); | |
| try { | |
| await fsPromises.access(worktreesDir); | |
| } catch { | |
| return; | |
| } | |
| const removed = await cleanupStaleAgentWorktrees(root); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| * file lives outside the working tree (it is .gitignored as part of | ||
| * `.qwen/worktrees/.gitignore`) so it cannot leak into commits. | ||
| */ | ||
| export const WORKTREE_SESSION_FILE = '.qwen-session'; |
Collaborator
There was a problem hiding this comment.
[Suggestion] .qwen-session is written inside the worktree directory but nothing in the worktree's own .gitignore ignores it. The parent repo's .qwen/.gitignore only hides worktrees/ from the parent — inside the child worktree, .qwen-session is a visible untracked file. If a subagent runs git add -A or git add ., the session ID gets committed to the worktree branch.
Fix: write a per-worktree .gitignore containing .qwen-session during worktree creation, or store the marker outside the worktree directory.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| }; | ||
| }; | ||
|
|
||
| if (this.params.isolation === 'worktree') { |
Collaborator
There was a problem hiding this comment.
[Suggestion] enter_worktree has a nested worktree guard (regex /\\.qwen[\\\\/]worktrees[\\\\/]/ at enter-worktree.ts:82), but the agent isolation path at this line has no equivalent guard. If the parent Config's targetDir is already inside a worktree, creating another isolation worktree would produce two worktrees at the same level with the model's mental map still referencing the outer worktree — the same path-confusion rationale that motivated the enter_worktree guard.
Add the same check before getRepoTopLevel:
if (/\.qwen[\\/]worktrees[\\/]/.test(cwd)) {
return failWorktreeProvisioning(
'Already inside a git worktree — cannot create nested isolation.',
);
}— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
|
|
||
| let hasUnmerged = true; | ||
| try { |
Collaborator
There was a problem hiding this comment.
[Suggestion] hasUnmergedWorktreeCommits catch block has zero logging. A real git error (corrupt ref store, permission problem, disk full) looks identical to legitimate unmerged commits — the tool returns "Refusing to remove... its branch has commits that no other branch... points at". Operators manually inspect the branch, find no unmerged commits, and are confused. The actual git error that caused the refusal is invisible.
Suggested change
| try { | |
| } catch (error) { | |
| debugLogger.warn( | |
| `exit_worktree: hasUnmergedWorktreeCommits failed for ${branch}: ${error}`, | |
| ); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // Promote to `info` so operators chasing a worktree leak in | ||
| // production can grep the logs and see the sweep actually | ||
| // ran. The cleanup helper itself uses `debug`-level | ||
| // per-entry logs, which are silent at default verbosity. |
Collaborator
There was a problem hiding this comment.
[Suggestion] The startup sweep only logs when removed > 0. When the sweep runs and finds 0 stale worktrees, nothing is logged. Operators grepping for "Stale worktree" or "sweep" can't distinguish between "sweep was skipped entirely (fast-bail)" and "sweep ran but found nothing". Log something unconditionally so the sweep's execution (or non-execution) is always visible in logs.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
The biggest item — #1 — is a self-inflicted regression from round 5: the new agent- prefix reservation in `validateUserWorktreeSlug` rejected EVERY slug that `generateAgentWorktreeSlug` produces, since that helper emits exactly `agent-<7hex>`. Net effect: every `AgentTool isolation: 'worktree'` invocation failed at validation. The reservation now allows the canonical pattern through (everything the helper can produce) and only rejects user-chosen `agent-*` names that don't match it. Added a round-trip regression guard: 50 `generateAgentWorktreeSlug()` outputs are fed back through `validateUserWorktreeSlug` and must all pass. Other critical fixes: 2. `hasWorktreeChanges` (used by agent isolation cleanup) was the one remaining caller relying solely on `status.isClean()`. Defensive `|| status.conflicted.length > 0` so a future simple-git bookkeeping change can't let a mid-merge worktree appear clean and get auto-deleted. 3. `readWorktreeSessionMarker` swallowed every I/O error as "marker missing", which let a disk error / EACCES silently bypass the session-ownership guard. ENOENT is still treated as missing (legitimate); every other code now logs. 4. `exit_worktree` `fs.stat` catch was the same shape — every error collapsed to "Worktree not found". ENOENT → not found; everything else logs and returns a distinct "cannot access" error. 5. `cleanupStaleAgentWorktrees` `fs.stat` catch was again the same. ENOENT → silently skip (entry vanished between readdir and stat); everything else logs. Suggestions: 6. Startup sweep fast-bail was running BEFORE resolving the repo top-level. For monorepo subdir launches, `targetDir/.qwen/worktrees` never exists and the sweep early-returned — permanently a no-op. Now resolves the root first, then fast-bails against the resolved `<root>/.qwen/worktrees`. Also logs the skip case so operators can tell "skipped" from "ran, found nothing". 7. `.qwen-session` marker was visible to `git add -A` inside the worktree. Now writes a `.git/info/exclude` rule (resolved via `git rev-parse --git-dir`, since worktree `.git` is a file pointing at the parent repo's `.git/worktrees/<name>/`). Best-effort: failure to write the rule does not abort provisioning. 8. Agent isolation now refuses to provision when the parent's cwd is already inside a worktree — same regex guard as `enter_worktree`. 9. `exit_worktree`'s wrapper around `hasUnmergedWorktreeCommits` now logs at the call site so the chain (caller → reason it asked → underlying git error) is complete in operator logs. 10. Sweep now logs unconditionally at `info`. Three distinct messages: "skipped (no worktrees dir)", "ran, nothing to remove", "removed N". Tests: 11. New `execute()` coverage: • exit-worktree: session-ownership refusal, keep happy path, legacy/no-marker fallthrough with warn log, missing-worktree error, unmerged-commits guard with `discard_changes: true`, `writeWorktreeSessionMarker` round-trip. • enter-worktree: nested-guard rejection, non-git-repo error. These spin up real temp git repos (no filesystem mocking) and drive the actual tool invocation pipeline. Total: 135 tests pass (was 101 → +34). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
Author
|
Pushed `b374187fa`. Five critical + six suggestions, all closed. Critical
Suggestion
Test coverage (#11)Real-temp-git-repo `execute()` coverage:
135 tests pass (was 101 → +34). |
Self-review pass applying the round-6 review-triage framework (filter #5: "If a log only fires on the happy path, it's noise.") to my own round-6 changes: - "Stale worktree sweep skipped: <dir> does not exist" — fires on every CLI start for ~99% of users who never use worktrees. - "Stale worktree sweep ran under <root>: nothing to remove" — fires on every CLI start for users who have any worktrees but no stale ones at the moment. Both are happy-path noise at `info`. Demoted to `debug` so an operator can opt in via `--debug` when they want to confirm the sweep is wired up, but normal output stays clean. Only the actually-actionable case ("removed N worktrees") stays at `info` — that's the signal someone chasing a worktree leak would grep for. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
Author
|
Self-review pass on round 6: pushed `772082df3` demoting two of my own `info` logs to `debug`. The round-6 fix for #10 (sweep visibility) added `info` logs on every code path so operators could distinguish "didn't run" from "ran, found nothing". On reflection that's noise: those messages fire on every CLI start for ~99% of users and drown the actually-actionable "removed N worktrees" message that someone chasing a leak would grep for. Demoted both happy-path messages to `debug` (still available with `--debug` for confirming the sweep is wired up). The actionable case stays at `info`. Net behaviour change: removes recurring info noise without losing any signal an operator chasing a real problem would want. This is a partial walk-back of suggestion #10 from the round-6 review — the underlying observation was valid but the original `info`-everywhere implementation was over-defensive. Flagging explicitly so the change isn't mysterious. |
tanzhenxin
reviewed
May 14, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
Six rounds of follow-up commits land sweeping fixes for the prior round's findings — all data-loss-class issues are closed, and the round-by-round commit discipline (each commit naming the findings it addresses) makes verification straightforward. Two items remain worth flagging: one user-visible bug and one correctness gap with a clear failure mode. Several low-severity follow-ups (nested-worktree guard via --git-common-dir, untracked-only sweep tradeoff, agent-<7hex> slug collision, getProjectRoot rebinding scope, session-marker TOCTOU, detached-HEAD baseBranch propagation) are noted but not blocking.
1. exit_worktree action='remove' is still auto-approved in AUTO_EDIT mode (severity: high · confidence: very high)
The default-permission switch to ask only takes effect in DEFAULT mode. In AUTO_EDIT, the confirmation type falls through to the generic info form, which the auto-approval allow-list lets through silently. A model running in AUTO_EDIT can drop a worktree (with deleteBranch: true) without ever prompting. Overriding the confirmation-details factory to return a non-info type — anything in the exec/destructive bucket — closes it.
2. Agent isolation worktrees start from the parent's HEAD, not its working tree (severity: medium · confidence: high)
When isolation provisions a worktree, the new branch is created directly from the base ref. Any tracked or untracked changes already present in the parent checkout don't appear in the subagent's worktree. A common workflow — edit some code, then launch a review or test agent before committing — silently runs the subagent against the prior HEAD and returns results that look authoritative but reflect stale code. The Arena worktree-setup precedent overlays the parent's dirty state via git stash --include-untracked before launching; doing the same here, or refusing isolation when the parent is dirty, would close the gap.
Verdict
COMMENT — issue 1 is the only must-address before merge. Issue 2 is a correctness gap worth fixing soon. The low-severity items can be follow-ups.
Round-7 review caught two correctness gaps: 1. exit_worktree action='remove' was still auto-approved in AUTO_EDIT `getDefaultPermission` returning 'ask' is necessary but not sufficient. `permissionFlow.isAutoEditApproved` auto-approves any tool whose `confirmationDetails.type` is 'edit' OR 'info', and `BaseToolInvocation` returns 'info' by default. So a session in AUTO_EDIT could silently destroy a worktree (with branch deletion) without a confirmation prompt — the data-loss path the round-1 `'ask'` switch was meant to close. Now overrides `getConfirmationDetails` to return `type: 'exec'` for action=remove, which keeps the prompt in AUTO_EDIT. The `keep` action still falls through to the base info-type since it is non-destructive. Regression-guard test asserts the type is 'exec' (not 'info') for remove and that the command field describes both the worktree-remove and branch-delete operations. 2. Agent isolation worktrees ran against parent's HEAD, not its working tree `git worktree add -b <branch> <path> <base>` only checks out the base ref's tip — uncommitted edits in the parent's working tree do NOT propagate. The "edit code → ask review/test agent before committing" workflow silently ran the subagent against the pre-edit HEAD and returned results that looked authoritative but reflected stale code. Reviewer offered two options: overlay parent's dirty state à la Arena (~50 LOC, edge cases), or refuse isolation when parent is dirty (~10 LOC, clear UX). Chose the latter for Phase B scope — simpler, decisive, and matches the design-doc's explicit commitment that dirty-state overlay is Arena-specific. Users can commit/stash before re-invoking agent isolation; overlay can be a follow-up if users complain about the friction. Fail-closed on the dirty-check itself (assume dirty rather than silently launch on a possibly-stale tree). Test exercises both "dirty parent → guard fires" and "clean parent → guard passes" against real temp git repos. 139 unit tests pass (was 135, +4 regression guards). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
Author
|
Pushed `c91458792`. Both must-address findings closed; the six low-severity follow-ups triaged below. Issue 1 (must-address) — fixed`exit_worktree action='remove'` was indeed silently auto-approved in `AUTO_EDIT`. Verified the path: `permissionFlow.isAutoEditApproved(AUTO_EDIT, 'info')` returns true, and `BaseToolInvocation.getConfirmationDetails` defaults to `type: 'info'`. Now overrides `getConfirmationDetails` for `action='remove'` to return `type: 'exec'` (same bucket as `run_shell_command`), which is NOT in the AUTO_EDIT auto-approval list. `keep` still uses the base info-type since it's non-destructive. Added a regression-guard test asserting `type === 'exec'` for remove. Issue 2 (worth fixing soon) — fixed by refusingPicked the `refuse-when-dirty` option you offered. Reasoning: the design doc explicitly tagged the dirty-state overlay as Arena-specific (✅ Arena, not Phase-A/B generic worktrees), and overlay introduces edge cases (stash conflicts, untracked-file copy) that aren't worth chasing for Phase B scope. Refusal closes the silent stale-code hazard with ~10 LOC and gives the user a clear path: commit/stash, then re-invoke the agent. If users complain about the friction, an overlay-style upgrade is a straightforward follow-up. Fail-closed on the dirty-check itself so a flaky `git status` doesn't silently launch on a possibly-stale tree. Six low-severity follow-ups — triagedEach evaluated against the same five filters (legitimacy → design intent → user impact → reuse value → defensive-overdo); declined ones get reasoning, not silence.
Verification
|
tanzhenxin
approved these changes
May 14, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
Targeted re-review against round 7 (c9145879) — the two must-fix items from the prior round are genuinely closed. The AUTO_EDIT bypass on exit_worktree action='remove' no longer fires because the confirmation prompt is now classified as an exec action, which sits outside the AUTO_EDIT allow-list. The agent isolation path now refuses to provision a worktree when the parent checkout has uncommitted edits, so a subagent can't be silently run against stale code.
Verdict
APPROVE — must-fixes from the prior round are closed; remaining items are low-severity follow-ups.
wenshao
requested changes
May 14, 2026
| // at its definition), so this fallback fires once at most even | ||
| // when the success path already ran it. | ||
| try { | ||
| await cleanupWorktreeIsolation(); |
Collaborator
There was a problem hiding this comment.
[Critical] Background agent isolation worktree is destroyed before the background agent starts.
When run_in_background: true, execute() returns early with FORK_PLACEHOLDER_RESULT (line ~1951), but the outer finally block at this line unconditionally calls cleanupWorktreeIsolation(). This function nulls worktreeIsolation and — finding no changes in the freshly created worktree — removes the directory and branch.
Meanwhile, the background subagent (bgBody()) has not yet started executing (or has just begun), and finds itself operating without its isolated worktree — it either crashes or pollutes the parent working tree.
Impact: Every agent invocation with isolation='worktree' + run_in_background: true silently loses its sandbox. The background agent operates in the parent's working directory without any isolation.
Suggested change
| await cleanupWorktreeIsolation(); | |
| } finally { | |
| // Only reap the isolation worktree on the foreground path. | |
| // Background agents manage cleanup inside their own completion | |
| // and error handlers (bgBody). | |
| if (!shouldRunInBackground) { | |
| try { | |
| await cleanupWorktreeIsolation(); | |
| } catch { | |
| // Helper logs its own failures. | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
22 tasks
TaimoorSiddiquiOfficial
pushed a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 15, 2026
…e + Agent isolation (QwenLM#4073) * feat(tools): add generic worktree support (Phase A + B of QwenLM#4056) Adds first-class git worktree as a general-purpose capability: Phase A — User-facing tools - enter_worktree: creates `<projectRoot>/.qwen/worktrees/<slug>` on a `worktree-<slug>` branch and returns the absolute path. Slug auto-generated when omitted; validated against path traversal and disallowed characters. - exit_worktree: keeps or removes the worktree (and its branch). Refuses to remove a worktree with uncommitted tracked changes or untracked files unless `discard_changes: true` is set. Phase B — Agent isolation - Agent tool gains an `isolation: 'worktree'` parameter that provisions a temporary `agent-<7hex>` worktree, prepends a worktree notice to the task prompt, and on completion either removes the worktree (no changes) or preserves it and reports its path/branch in the result. Background and foreground execution paths both wired up; rejected for fork agents. - worktreeCleanup.cleanupStaleAgentWorktrees: fail-closed sweep for ephemeral `agent-<7hex>` worktrees older than 30 days with no tracked changes and no unpushed commits. User-named worktrees are never swept. - buildWorktreeNotice helper for fork subagents (parity with claude-code). Arena compatibility - The existing Arena worktree implementation (GitWorktreeService.setupWorktrees, ArenaManager, agents.arena.worktreeBaseDir) is untouched. Arena uses its own batch APIs and `~/.qwen/arena` base dir; the new general-purpose APIs live alongside under `<projectRoot>/.qwen/worktrees/`. Subagent safety - enter_worktree / exit_worktree are added to EXCLUDED_TOOLS_FOR_SUBAGENTS so a subagent cannot mutate the parent session's worktree state. Refs QwenLM#4056 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(worktree): use path.join in expected paths so the test passes on Windows The Windows CI run reported `enter-worktree.test.ts` failing because the expected string was hardcoded with `/` while `getUserWorktreesDir()` uses `path.join`, which returns `\\` on Windows. Build the expected path via `path.join` so the platform-correct separator is compared. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(enter-worktree): treat empty name as auto-generate Some models pass `{ "name": "" }` when calling EnterWorktree, because the schema marks `name` as optional and they emit an empty placeholder. The previous validation rejected the empty string with "Worktree name must be a non-empty string", which surprised users running the auto-slug path. Now both `validateToolParams` and `execute` treat `name: ""` as equivalent to `name: undefined` and fall back to the auto-generated `{adj}-{noun}-{4hex}` slug. Explicit invalid slugs (`'../etc'`, `'a/b'`, etc.) are still rejected as before. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address review findings 1-6 from PR QwenLM#4073 Six issues raised on the initial review; each addressed with a verifiable guarantee. 1. Real isolation for `agent isolation: 'worktree'` Before: subagent's Config still resolved `getTargetDir()` to the parent project root, so Edit/Write/Read workspace checks and Shell's default cwd silently operated on the parent tree. The cleanup helper then saw a "clean" worktree and removed it — destroying the evidence. After: the worktree is provisioned BEFORE `createApprovalModeOverride`, and the resulting agent Config has `getTargetDir`/`getCwd`/`getWorkingDir` rebound to the worktree path. Relative paths, unqualified shell commands, and glob/grep roots all confine to the worktree. 2. `exit_worktree action='remove'` now prompts in default/auto-edit modes Added `getDefaultPermission()` on the invocation: `'ask'` when action is `remove`, `'allow'` when `keep`. Brings it in line with edit, write_file, and run_shell_command. 3. Force-delete no longer silently destroys unpushed commits `removeUserWorktree` now uses `git branch -d` (refuses unmerged) by default and surfaces `branchPreserved: true` when git refuses. Added `hasUnmergedWorktreeCommits` (checks if branch tip is reachable from any other local branch or remote ref). Both the agent isolation cleanup and `exit_worktree action='remove'` use this check: if the branch has work not covered elsewhere, the worktree+branch are preserved even when `discard_changes: true` is set (there is no `discard_commits` flag — committed work is rarely what `remove` means to discard). 4. Both new tools are now deferred behind ToolSearch `shouldDefer: true` + `searchHint` on both. Verified via openai-logging: `enter_worktree` and `exit_worktree` no longer appear in the function- declaration list sent on every API request. 5. Stale-worktree cleanup is wired in `Config.initialize()` fires `cleanupStaleAgentWorktrees(targetDir)` as a non-awaited startup sweep (skipped in bare mode). Picks up orphaned `agent-<7hex>` worktrees left by crashed runs. 6. Foreground isolation no longer leaks on uncaught throw The foreground try block tracks whether the cleanup helper ran on the success path; the finally block invokes it as a fallback when the try bailed early. Mirrors the background path's pattern. Verification: - Unit tests: 83 passed (16 worktree + 64 existing agent + 3 cleanup) — no regressions. - E2E #1: agent told to write `hello.txt` via RELATIVE path — file landed at `.qwen/worktrees/agent-XXXXXXX/hello.txt`, NOT at the parent root. - E2E #3: created worktree, committed work inside it, called exit_worktree with `discard_changes=true` — refused with clear message; worktree and branch both preserved. - E2E #4: openai-logging confirms worktree tools absent from API tool list (7 tools sent instead of 9). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address review round 2 findings (1 from tanzhenxin, 7+8 from wenshao) The first round closed the data-loss-class issues. This round addresses follow-ups from a deeper audit: 1. Stale-worktree sweep was inert on common-case repos `cleanupStaleAgentWorktrees` previously ran `git log --branches --not --remotes --oneline` from each worktree's directory — that lists unpushed commits across EVERY local branch, not just the worktree's own branch. On any repo with no remote configured (or with stray unpushed branches), the sweep refused to remove every candidate. Replaced with `service.hasUnmergedWorktreeCommits(slug)` which scopes the check to the worktree branch via `for-each-ref --contains <tip>`. Also added the `branchPreserved` warn log requested in M7 and an `fs.access` shortcut for the empty-worktrees-dir case (M8). 2. `cleanupWorktreeIsolation` and `worktreeIsolation` were inside the inner try (~660 lines from the outer catch). Hoisted both to the top of `execute()` so the outer catch can reap or preserve the worktree when anything between provisioning and the inner try throws (e.g. `createApprovalModeOverride`, agent creation). Closure carries the resolved `repoRoot` so cleanup never has to re-resolve. 3. Background error path discarded the cleanup result. Now captures `formatWorktreeSuffix(...)` and appends it to the registry's failure /cancel message, so users see the preserved path/branch even when the agent crashed before reporting. 4. `cleanupWorktreeIsolation` now treats `result.success === false` as "worktree still on disk" and surfaces it as preserved instead of silently dropping it from the result. 5. Override was incomplete. Several Config methods read `this.targetDir` directly (`getProjectRoot`, `getFileService`, etc.) — own-property getter overrides did not redirect them. Now also shadows `targetDir` and `cwd` as own properties on the agent's Config override, swaps in a `FileDiscoveryService` rooted at the worktree, and rebuilds `WorkspaceContext` to point at the worktree only. Verified end-to-end: shell `pwd > pwd-record.txt` (no directory arg) lands at `.qwen/worktrees/agent-<7hex>/pwd-record.txt`, not the parent root. 6. monorepo subdir issue. Both `enter_worktree` and the agent isolation path now resolve `git rev-parse --show-toplevel` first and anchor `.qwen/worktrees/<slug>` at the repo root. Worktrees created from any subdirectory now end up where the startup sweep can find them. 7. Replaced `git worktree add -B` (silent force-reset of pre-existing branches) with `git worktree add -b` plus an explicit existence check via `git for-each-ref` (NOT `show-ref --quiet`, which simple-git swallows). Pre-existing `worktree-<slug>` branches now trigger a clear error instead of clobbering committed work. 8. First worktree creation in a repo writes `<projectRoot>/.qwen/.gitignore` with `worktrees/` so worktree contents stay out of the parent's `git status`, glob/grep results, and bundle tools. Idempotent: never overwrites an existing file. 9. Logging across the failure paths (`enter_worktree` errors, `agent.ts:failWorktreeProvisioning`, `cleanupWorktreeIsolation`, `hasUnmergedWorktreeCommits` swallowed errors, `cleanupStaleAgentWorktrees`'s `branchPreserved` race). 10. `exit_worktree` no longer suggests `discard_changes: true` when the git status check itself fails — that would be advising the user to bypass a safety check whose precondition is unknown. Now points at the underlying repo problem. 11. `generateAutoSlug` switched from `Math.random()` (4 hex, weak RNG, one-in-65k collision) to `randomBytes` (6 hex, ~16M combinations). Two RNG sources in this file collapsed to one. Pushed back: the TOCTOU swap in `removeUserWorktree` (S6 round 1) is left as-is — `git branch -d` is the real safety, and reordering does not eliminate the window. Windows reserved-name validation (M5 round 2) deferred to a follow-up; the current allowlist already rejects path separators, `..`, leading dot/dash, and the >64-char case. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): use randomInt to silence CodeQL biased-modulo finding CodeQL's `js/biased-cryptographic-random` flagged `randomBytes(4)[i] % ARRAY.length` in `generateAutoSlug`. The math is actually exact for the current word-list lengths (256 % 8 == 0), but the lint rule does not know that — and a future contributor changing the list to a non-power-of-two length would silently introduce bias. Switched the index lookups to `crypto.randomInt(0, length)`, which uses rejection sampling and is uniform by construction. Suffix still uses `randomBytes(3).toString('hex')` since hex encoding is unbiased. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address review round 3 findings 1-6 from PR QwenLM#4073 The previous round added `getRepoTopLevel` for `enter_worktree`'s provisioning, but missed three sibling call sites that still used the raw cwd. The double-cleanup race in the foreground path also leaked stale `[worktree preserved]` suffixes on rejected promises. All six findings from the deeper audit are addressed: 1. exit_worktree now resolves through `getRepoTopLevel()` before building its `GitWorktreeService`, mirroring `enter_worktree`. Without this, launching `qwen` from a monorepo subdirectory created the worktree under the repo root but exit_worktree looked under the subdir's `.qwen/worktrees/` and always returned "Worktree not found". Verified end-to-end: enter + exit from `packages/core/` works. 2. agent.ts cleanup helper now nulls `worktreeIsolation` immediately after capturing the closure value. The previous structure could reach the helper twice — once in the foreground try's success path and once in the foreground finally fallback (or once in the inner try and once in the outer catch on a thrown rejection). The second call would `hasWorktreeChanges()` against a directory the first call already removed, fail-closed, and emit a bogus `[worktree preserved: <missing path>]` suffix. 3. Config.initialize's startup sweep now resolves `getRepoTopLevel()` before invoking `cleanupStaleAgentWorktrees`. Without this, every subdir launch scanned a non-existent `<subdir>/.qwen/worktrees/` and the 30-day expiry sweep was permanently a no-op. 4. agent.ts's `buildWorktreeNotice` now passes `worktreeIsolation.repoRoot` as `parentCwd` instead of `this.config.getTargetDir()`. The notice's path-translation guidance (≈ "translate paths from <parent> to <worktree>") would otherwise misdirect the subagent in a monorepo subdir launch. 5. Removed dead method `GitWorktreeService.listUserWorktrees`. It had no callers anywhere in the codebase and used `execSync` in a loop (would have blocked the event loop if anyone wired it up). 6. `localBranchExists` no longer swallows git failures silently. The defensive `false` default is preserved (so `git worktree add -b` itself surfaces the conflict if the check missed an existing branch), but the catch now logs via `debugLogger.warn` so disk-full / permission / ref-store-corruption cases are visible in debug output instead of being invisible. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address review round 4 findings (data-loss + visibility) Seven actionable findings from a deeper audit, all closed: 1. User worktree slugs could collide with ephemeral-agent shape `validateUserWorktreeSlug` did not reject names starting with `agent-`, so a user-named `agent-1234567` matched the cleanup regex `/^agent-[0-9a-f]{7}$/` and would be silently swept after 30 days along with whatever work was in it. Now reserved — clear error message points users at the cause. 2. Slug producer and consumer were string-coupled across files `agent.ts` hardcoded `agent-${hex(7)}` and `worktreeCleanup.ts` independently hardcoded `/^agent-[0-9a-f]{7}$/`. Future change to hex length on one side would silently break the other. Lifted `AGENT_WORKTREE_PREFIX`, `AGENT_WORKTREE_HEX_LENGTH`, `AGENT_WORKTREE_SLUG_PATTERN`, and `generateAgentWorktreeSlug()` to `gitWorktreeService.ts`; both call sites import them. 3. Startup sweep was invisible at default log level Fire-and-forget sweep used `debug` for errors and discarded the success count. A leak-chasing operator had no log breadcrumb. Errors promoted to `warn`; successful removals (count > 0) logged at `info`. 4. `getRepoTopLevel()` silent catch Returned `null` on any git failure with no log. Combined with `?? cwd` fallback in callers, a flaky git would have made worktree creators and the startup sweep disagree silently about which dir to use. Now logs the underlying error. 5. `hasTrackedChanges()` silent catch Cleanup's fail-closed `return true` had no log. Couldn't tell "has real changes — leave alone" from "git index unreadable — repo may be corrupt". Now logs. 6. `cleanupWorktreeIsolation` claimed `preservedPath` for a removed dir When `removeUserWorktree` returns `{ success: true, branchPreserved: true }` it has already deleted the directory and failed only on `git branch -d`. The helper still reported the (now non-existent) path as preserved. Now returns only `preservedBranch` for that case; `formatWorktreeSuffix` emits a distinct message instructing recovery via `git worktree add <new-path> <branch>`. 7. `removeUserWorktree` swallowed branch-delete failures Both `-d` and `-D` catch blocks were empty. Locked refs, perms, disk full all looked identical to "unmerged commits". Both now `debugLogger.warn` with the underlying error. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(worktree): self-review pass — reuse, parallelism, dead code Self-review caught a handful of issues across three categories: Reuse: - `pathExists` in the new code now uses the existing `fileExists` from `utils/fileUtils.ts` instead of duplicating an `fs.access` wrapper. - `worktree-` branch prefix was string-literalled in five places. Added `WORKTREE_BRANCH_PREFIX` and `worktreeBranchForSlug(slug)` exports in `gitWorktreeService.ts`; updated `gitWorktreeService.ts`, `worktreeCleanup.ts`, and `exit-worktree.ts` to use them. Future prefix changes are a single edit. Efficiency: - `Config.initialize` used two `await import(...)` calls inside the startup-sweep IIFE, paying that cost on every CLI start. Switched to static imports at the top of `config.ts` — the modules are tiny and the dynamic indirection bought nothing. - `cleanupWorktreeIsolation` in `agent.ts` ran `hasWorktreeChanges` and `hasUnmergedWorktreeCommits` sequentially. They have no data dependency on each other and each spawns its own `git` invocation; `Promise.all` halves the cleanup wall-clock on the common path. Same fix in `worktreeCleanup.ts`'s per-entry loop. - `ensureWorktreesGitignored` used `fs.access` then `fs.writeFile`, a TOCTOU race when two agent invocations created worktrees concurrently (both could pass the `access` check and the second would clobber the first's `.gitignore`). Now writes with `flag: 'wx'` and treats `EEXIST` as the no-op case — atomic in one syscall. Quality: - Dropped the `worktreeCleanupRan` boolean in the foreground execution path. `cleanupWorktreeIsolation` already nulls its closure variable at the top of every call (see the comment at its definition), so re-entries are no-ops. The boolean and its tracking were dead weight that obscured the real guard. - Trimmed the Phase-2 override comment block to drop the WHAT-stating enumerations (items 3 and 4 just narrated the lines below) and removed a navigation comment about hoisted helpers — the helpers are visible at the top of the same method. 84 unit tests pass; typecheck clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address review round 5 — design-doc commitments + correctness Five critical findings + four suggestions, all closed. Critical: 1. Wrong base branch for agent isolation. `createUserWorktree(slug)` with no `baseBranch` arg fell back to `getCurrentBranch()` on the **main** working tree, returning `main` regardless of which branch the user was actually on. A subagent invoked from `feature-x` would silently start from `main` and produce diffs against the wrong baseline. `enter_worktree` had the same bug. Both now resolve the parent's current branch first and pass it explicitly. Verified end-to-end: `git checkout feature-x` → `enter_worktree` → worktree HEAD includes the feature-x commit. 2. `countWorktreeChanges` (used by `exit_worktree`'s dirty-state guard) missed `status.conflicted[]`. In simple-git that array is mutually exclusive with the staged/modified/etc. arrays, so a worktree mid-merge with only conflicts looked `{tracked: 0, untracked: 0}` to the guard and `action='remove'` would proceed without `discard_changes: true`. Added `+ status.conflicted.length`. 3. `exit_worktree` had no session-ownership check, contradicting the design doc's "only operates on worktrees created by THIS session". In yolo mode a prompt injection could enumerate `.qwen/worktrees/` and pass any name to drop another session's work. Now: `enter_worktree` and agent isolation write a `.qwen-session` marker into the worktree at provisioning time; `exit_worktree action='remove'` reads it and refuses if it does not match the current `Config.getSessionId()`. Worktrees from before this guard (no marker file) are treated as "owner unknown" — allowed with a warn log so the change is observable. 4. `enter_worktree` did not refuse nested invocations from inside an existing worktree, contradicting the design doc. Now rejects any cwd containing `.qwen/worktrees/` as a path component, with a clear "Already inside a git worktree…" message. Verified: enter from inside a worktree returns is_error with that text. 6. `hasTrackedChanges` (cleanup sweep) had the same `conflicted[]` gap. Rewrote to use raw `git status --porcelain --untracked-files=no` which lists every tracked change including `UU` conflict markers in a single git call and explicitly skips the untracked walk (the prior comment claimed to skip it, but `status()` always does the scan). Suggestion: 7. `buildWorktreeNotice` now receives the parent agent's actual `getTargetDir()` again (was switched to `repoRoot` in round 3 on a different reviewer's suggestion; round-5 caught that the model's inherited paths reference the parent's cwd, not necessarily the repo root, so the prior behaviour was correct). 8. Startup sweep now does `fs.access(<targetDir>/.qwen/worktrees)` *before* importing GitWorktreeService and spawning `git rev-parse --show-toplevel`. The git probe is reserved for users who actually have a worktrees directory locally — 99% of users pay only one syscall on startup. 9. Tests: - New `exit-worktree.test.ts` covers metadata, validation, `getDefaultPermission` (ask vs allow), and getDescription. - `agent.test.ts` adds three `validateToolParams` cases for the `isolation` parameter (accepted with subagent_type, rejected without, rejected for non-"worktree" values). - `enter-worktree.test.ts` adds round-trip tests for `writeWorktreeSessionMarker` / `readWorktreeSessionMarker` plus a `worktreeBranchForSlug` sanity check. - Total: 101 tests pass (was 86 → +15). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(test): drop unused @ts-expect-error in exit-worktree.test.ts Empty string `''` is a valid `string` type, so the @ts-expect-error directive on `validateToolParams({ name: '', action: 'keep' })` did nothing — TypeScript correctly accepted the line, and `tsc --build` in CI reported TS2578 ("Unused '@ts-expect-error' directive"). The runtime assertion already covers the case; the directive was leftover from an earlier draft. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(test): use importActual in ArenaManager mock to preserve new exports The Arena test mocks `gitWorktreeService.js` with a factory that returns only `{ GitWorktreeService }`. PR QwenLM#4073 added several other exports to that module (`AGENT_WORKTREE_SLUG_PATTERN`, `WORKTREE_BRANCH_PREFIX`, `worktreeBranchForSlug`, `generateAgentWorktreeSlug`, `writeWorktreeSessionMarker`, `readWorktreeSessionMarker`, `WORKTREE_SESSION_FILE`). Other modules in the dep graph reach the mocked surface — most notably `worktreeCleanup.ts` imports `AGENT_WORKTREE_SLUG_PATTERN` and `worktreeBranchForSlug`, and now reaches the mock via the static `config.ts` → `worktreeCleanup.ts` import chain added in the self-review pass. The Arena test failed at module-load with: Caused by: Error: [vitest] No "AGENT_WORKTREE_SLUG_PATTERN" export is defined on the "../../services/gitWorktreeService.js" mock. Did you forget to return it from "vi.mock"? Use `importOriginal` to capture every real export, spread it into the return object, and only replace `GitWorktreeService` (the class the test actually needs to mock). The class-level mock keeps its existing static-method shims. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address review round 6 (5 critical + 6 suggestions) The biggest item — #1 — is a self-inflicted regression from round 5: the new agent- prefix reservation in `validateUserWorktreeSlug` rejected EVERY slug that `generateAgentWorktreeSlug` produces, since that helper emits exactly `agent-<7hex>`. Net effect: every `AgentTool isolation: 'worktree'` invocation failed at validation. The reservation now allows the canonical pattern through (everything the helper can produce) and only rejects user-chosen `agent-*` names that don't match it. Added a round-trip regression guard: 50 `generateAgentWorktreeSlug()` outputs are fed back through `validateUserWorktreeSlug` and must all pass. Other critical fixes: 2. `hasWorktreeChanges` (used by agent isolation cleanup) was the one remaining caller relying solely on `status.isClean()`. Defensive `|| status.conflicted.length > 0` so a future simple-git bookkeeping change can't let a mid-merge worktree appear clean and get auto-deleted. 3. `readWorktreeSessionMarker` swallowed every I/O error as "marker missing", which let a disk error / EACCES silently bypass the session-ownership guard. ENOENT is still treated as missing (legitimate); every other code now logs. 4. `exit_worktree` `fs.stat` catch was the same shape — every error collapsed to "Worktree not found". ENOENT → not found; everything else logs and returns a distinct "cannot access" error. 5. `cleanupStaleAgentWorktrees` `fs.stat` catch was again the same. ENOENT → silently skip (entry vanished between readdir and stat); everything else logs. Suggestions: 6. Startup sweep fast-bail was running BEFORE resolving the repo top-level. For monorepo subdir launches, `targetDir/.qwen/worktrees` never exists and the sweep early-returned — permanently a no-op. Now resolves the root first, then fast-bails against the resolved `<root>/.qwen/worktrees`. Also logs the skip case so operators can tell "skipped" from "ran, found nothing". 7. `.qwen-session` marker was visible to `git add -A` inside the worktree. Now writes a `.git/info/exclude` rule (resolved via `git rev-parse --git-dir`, since worktree `.git` is a file pointing at the parent repo's `.git/worktrees/<name>/`). Best-effort: failure to write the rule does not abort provisioning. 8. Agent isolation now refuses to provision when the parent's cwd is already inside a worktree — same regex guard as `enter_worktree`. 9. `exit_worktree`'s wrapper around `hasUnmergedWorktreeCommits` now logs at the call site so the chain (caller → reason it asked → underlying git error) is complete in operator logs. 10. Sweep now logs unconditionally at `info`. Three distinct messages: "skipped (no worktrees dir)", "ran, nothing to remove", "removed N". Tests: 11. New `execute()` coverage: • exit-worktree: session-ownership refusal, keep happy path, legacy/no-marker fallthrough with warn log, missing-worktree error, unmerged-commits guard with `discard_changes: true`, `writeWorktreeSessionMarker` round-trip. • enter-worktree: nested-guard rejection, non-git-repo error. These spin up real temp git repos (no filesystem mocking) and drive the actual tool invocation pipeline. Total: 135 tests pass (was 101 → +34). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(worktree): demote noise startup-sweep logs to debug Self-review pass applying the round-6 review-triage framework (filter #5: "If a log only fires on the happy path, it's noise.") to my own round-6 changes: - "Stale worktree sweep skipped: <dir> does not exist" — fires on every CLI start for ~99% of users who never use worktrees. - "Stale worktree sweep ran under <root>: nothing to remove" — fires on every CLI start for users who have any worktrees but no stale ones at the moment. Both are happy-path noise at `info`. Demoted to `debug` so an operator can opt in via `--debug` when they want to confirm the sweep is wired up, but normal output stays clean. Only the actually-actionable case ("removed N worktrees") stays at `info` — that's the signal someone chasing a worktree leak would grep for. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): close AUTO_EDIT bypass + parent-dirty stale-code hazard Round-7 review caught two correctness gaps: 1. exit_worktree action='remove' was still auto-approved in AUTO_EDIT `getDefaultPermission` returning 'ask' is necessary but not sufficient. `permissionFlow.isAutoEditApproved` auto-approves any tool whose `confirmationDetails.type` is 'edit' OR 'info', and `BaseToolInvocation` returns 'info' by default. So a session in AUTO_EDIT could silently destroy a worktree (with branch deletion) without a confirmation prompt — the data-loss path the round-1 `'ask'` switch was meant to close. Now overrides `getConfirmationDetails` to return `type: 'exec'` for action=remove, which keeps the prompt in AUTO_EDIT. The `keep` action still falls through to the base info-type since it is non-destructive. Regression-guard test asserts the type is 'exec' (not 'info') for remove and that the command field describes both the worktree-remove and branch-delete operations. 2. Agent isolation worktrees ran against parent's HEAD, not its working tree `git worktree add -b <branch> <path> <base>` only checks out the base ref's tip — uncommitted edits in the parent's working tree do NOT propagate. The "edit code → ask review/test agent before committing" workflow silently ran the subagent against the pre-edit HEAD and returned results that looked authoritative but reflected stale code. Reviewer offered two options: overlay parent's dirty state à la Arena (~50 LOC, edge cases), or refuse isolation when parent is dirty (~10 LOC, clear UX). Chose the latter for Phase B scope — simpler, decisive, and matches the design-doc's explicit commitment that dirty-state overlay is Arena-specific. Users can commit/stash before re-invoking agent isolation; overlay can be a follow-up if users complain about the friction. Fail-closed on the dirty-check itself (assume dirty rather than silently launch on a possibly-stale tree). Test exercises both "dirty parent → guard fires" and "clean parent → guard passes" against real temp git repos. 139 unit tests pass (was 135, +4 regression guards). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> (cherry picked from commit 609e05b)
LaZzyMan
added a commit
that referenced
this pull request
May 21, 2026
…s + PR refs Three cross-cutting capabilities on top of the Phase A-C worktree foundation (PRs #4073, #4174). D-1: --worktree [name] CLI flag creates a worktree (or re-attaches to one that already exists) before any model turn runs. Supports bare, plain-slug, `=`, and PR-reference forms; --worktree + --acp rejected with a clear error; --worktree + --resume overrides the resumed session's saved sidecar and emits a stderr line. D-2: worktree.symlinkDirectories: string[] settings key opts into symlinking main-repo directories (e.g. node_modules) into every newly-created general-purpose worktree. Applies to all three creation paths: --worktree flag, EnterWorktreeTool, AgentTool isolation. Path traversal, absolute paths, and existing destinations all guarded; missing source dirs and EEXIST silently skipped (fail-open). D-3: --worktree=#<N> / --worktree <github-url> resolves a PR number, runs `git fetch origin pull/<N>/head` (30s timeout, no `gh` CLI dependency, LANG=C for stable error-taxonomy matching), and creates the worktree off FETCH_HEAD. URL regex tolerates /files, /commits, /checks sub-paths so users can paste any GitHub PR URL. Phase 6 verification fixes also included: - Re-attach to an existing worktree instead of failing with "Worktree already exists" — the common `qwen --resume <sid> --worktree foo` workflow now succeeds. The session ownership marker is preserved on re-attach so cross-session exit_worktree action="remove" still fails for non-owners. - Normalize path-taking argv fields (mcpConfig, jsonSchema @<path>, openaiLoggingDir, jsonFile, inputFile, telemetryOutfile, includeDirectories) to absolute paths against the launch cwd BEFORE the worktree chdir. Otherwise downstream fs.existsSync('./mcp.json') resolves into the worktree, where the file doesn't exist. Phase 7 code-review fixes: - buildStartupWorktreeNotice differentiates "Active worktree" (fresh create) from "Re-attached to worktree" (re-attach path). - Notice survives sidecar persist failure: set before the try block, refreshed inside with override addendum if persist succeeded. - getRegisteredWorktreeBranch verifies the candidate path's git common-dir matches the source repo's — rejects sibling `git init` directories that happen to be on a worktree-<slug> branch. Three-mode parity for the startup notice: TUI consumes via AppContainer effect, headless prepends a <system-reminder> + emits a worktree_started JSON event. ACP path is mutually exclusive with --worktree (ACP hosts supply per-session cwd separately). Tests (66 + 15 new): - 15 cli/src/startup/worktreeStartup.test.ts (slug forms, PR fetch against local fake remote, re-attach happy + wrong-branch guard) - 8 core/src/services/gitWorktreeService.test.ts (parsePRReference: #N, URLs, malformed, traversal, leading zeros, non-string) - 10 core/src/services/gitWorktreeService.symlinks.integ.test.ts (symlink loop + fetchPullRequestRef error taxonomy) Known limitations (documented in docs/users/features/worktree.md): - Cross-slug --resume <sid> --worktree <different-new-slug> is unsupported by design (sessions are bound to projectHash(cwd)); future Config refactor anchoring storage at repo root would lift this. - Mid-session enter_worktree still does NOT switch cwd/targetDir (Phase A's simplification); only the startup --worktree flag does. - yargs ambiguity: `qwen --worktree "say hi"` consumes the prompt as the slug. Quick Start shows the `=` form and reordering workarounds. Docs: - docs/users/features/worktree.md (new): Quick Start with --worktree flag, CLI Reference table for all four input forms + error codes, settings table, Limitations. - docs/design/worktree.md: Phase D section expanded into D-1/D-2/D-3 with open questions resolved; capability table updated. - docs/e2e-tests/worktree-phase-d.md (new): full E2E plan with Phase 4 dry-run baseline + Phase 6 post-impl reproduction tables. Refs #4056
LaZzyMan
added a commit
that referenced
this pull request
May 27, 2026
…s + PR refs (#4381) * feat(worktree): Phase D — startup --worktree flag + symlinkDirectories + PR refs Three cross-cutting capabilities on top of the Phase A-C worktree foundation (PRs #4073, #4174). D-1: --worktree [name] CLI flag creates a worktree (or re-attaches to one that already exists) before any model turn runs. Supports bare, plain-slug, `=`, and PR-reference forms; --worktree + --acp rejected with a clear error; --worktree + --resume overrides the resumed session's saved sidecar and emits a stderr line. D-2: worktree.symlinkDirectories: string[] settings key opts into symlinking main-repo directories (e.g. node_modules) into every newly-created general-purpose worktree. Applies to all three creation paths: --worktree flag, EnterWorktreeTool, AgentTool isolation. Path traversal, absolute paths, and existing destinations all guarded; missing source dirs and EEXIST silently skipped (fail-open). D-3: --worktree=#<N> / --worktree <github-url> resolves a PR number, runs `git fetch origin pull/<N>/head` (30s timeout, no `gh` CLI dependency, LANG=C for stable error-taxonomy matching), and creates the worktree off FETCH_HEAD. URL regex tolerates /files, /commits, /checks sub-paths so users can paste any GitHub PR URL. Phase 6 verification fixes also included: - Re-attach to an existing worktree instead of failing with "Worktree already exists" — the common `qwen --resume <sid> --worktree foo` workflow now succeeds. The session ownership marker is preserved on re-attach so cross-session exit_worktree action="remove" still fails for non-owners. - Normalize path-taking argv fields (mcpConfig, jsonSchema @<path>, openaiLoggingDir, jsonFile, inputFile, telemetryOutfile, includeDirectories) to absolute paths against the launch cwd BEFORE the worktree chdir. Otherwise downstream fs.existsSync('./mcp.json') resolves into the worktree, where the file doesn't exist. Phase 7 code-review fixes: - buildStartupWorktreeNotice differentiates "Active worktree" (fresh create) from "Re-attached to worktree" (re-attach path). - Notice survives sidecar persist failure: set before the try block, refreshed inside with override addendum if persist succeeded. - getRegisteredWorktreeBranch verifies the candidate path's git common-dir matches the source repo's — rejects sibling `git init` directories that happen to be on a worktree-<slug> branch. Three-mode parity for the startup notice: TUI consumes via AppContainer effect, headless prepends a <system-reminder> + emits a worktree_started JSON event. ACP path is mutually exclusive with --worktree (ACP hosts supply per-session cwd separately). Tests (66 + 15 new): - 15 cli/src/startup/worktreeStartup.test.ts (slug forms, PR fetch against local fake remote, re-attach happy + wrong-branch guard) - 8 core/src/services/gitWorktreeService.test.ts (parsePRReference: #N, URLs, malformed, traversal, leading zeros, non-string) - 10 core/src/services/gitWorktreeService.symlinks.integ.test.ts (symlink loop + fetchPullRequestRef error taxonomy) Known limitations (documented in docs/users/features/worktree.md): - Cross-slug --resume <sid> --worktree <different-new-slug> is unsupported by design (sessions are bound to projectHash(cwd)); future Config refactor anchoring storage at repo root would lift this. - Mid-session enter_worktree still does NOT switch cwd/targetDir (Phase A's simplification); only the startup --worktree flag does. - yargs ambiguity: `qwen --worktree "say hi"` consumes the prompt as the slug. Quick Start shows the `=` form and reordering workarounds. Docs: - docs/users/features/worktree.md (new): Quick Start with --worktree flag, CLI Reference table for all four input forms + error codes, settings table, Limitations. - docs/design/worktree.md: Phase D section expanded into D-1/D-2/D-3 with open questions resolved; capability table updated. - docs/e2e-tests/worktree-phase-d.md (new): full E2E plan with Phase 4 dry-run baseline + Phase 6 post-impl reproduction tables. Refs #4056 * refactor(worktree): apply self-review feedback on Phase D Self-review pass over the Phase D commit (2636f59) catching one real typecheck regression plus a batch of small quality + efficiency improvements. No user-visible behavior change beyond fixing the build. Build fix: - worktreeStartup.ts imports — pre-commit prettier had reorganized `writeWorktreeSession` and `readWorktreeSession` under an `import type { ... }` block, erasing them at compile time (verbatimModuleSyntax). `tsc --noEmit` was failing with TS1361. Bundle path still worked (esbuild is lenient) so this only surfaced when running typecheck. Startup-path efficiency (~10-25 ms saved per --worktree invocation on macOS; more on Windows): - Drop redundant `isGitRepository()` probe — `getRepoTopLevel()` returns null on non-git paths and covers both gates in one subprocess. - Run `getCurrentBranch()` + `getCurrentCommitHash()` in parallel via Promise.all (independent calls). - Combine the two `git rev-parse` probes inside `getRegisteredWorktreeBranch` into a single multi-arg call, and run it in parallel with the source-repo common-dir lookup. Saves one fork+exec on the re-attach path. Quality: - Extract `withReminder()` local helper in nonInteractiveCli.ts so the startup-notice and resume-restore branches share the system-reminder wrapping. - Log `readWorktreeSession` failures in `persistStartupWorktreeSidecar` with the sidecar path so operators can recover the previous slug from a backup. Silent swallow was making "where did my worktree binding go?" undebuggable. - Drop the dead `Config.getWorktreeSettings()` accessor (only `getWorktreeSymlinkDirectories()` has callers); keep the underlying `WorktreeSettings` interface for future fields. - Document the `pendingStartupWorktreeNotice` invariant: at most one consumer per process; ACP path is gated out earlier so only TUI XOR headless reads it. - Add a maintainer note in the gemini.tsx path-normalization block: the argv path-field allowlist is hand-maintained, register new path-bearing flags there or `--worktree` silently breaks for them. - Drop `Phase 6 fix (G1)/(G2)` parenthetical labels from inline comments — internal review-cycle identifiers that decay to noise post-merge. Substantive prose retained. Tests: cli 15/15 (unchanged) + core 66/66 (unchanged); bundle smoke verified fresh / re-attach / invalid slug / non-git cases. Findings deliberately left for follow-up: - Larger refactor extracting a shared `provisionUserWorktree` helper for the EnterWorktreeTool / startup overlap (~80% duplicate). - Splitting the re-attach branch out of `setupStartupWorktree` into its own function. - `isPathWithinRoot` / `isInsideManagedWorktree` shared utils. - `symlinkConfiguredDirectories` loop concurrency (saves 5-15 ms on a cold path that runs only when symlinkDirectories is configured). * docs(worktree): refresh stale docstring in worktreeStartup Top-of-file docstring still said `{adj}-{noun}-{4hex}` (actual format is 6 hex chars) and described the PR form as "detected and rejected with a clear 'coming in D-3' message" — but D-3 shipped in the same PR. Tighten to reflect what the code actually does. * fix(worktree): address findings from dual-reviewer self-check Two real bugs surfaced by an independent dual-reviewer pass (Claude + Codex) on the Phase D commits. Both correctness-affecting; both escaped the earlier internal reviews. P0 — re-attach captured the wrong baseline for the exit dialog (Codex): setupStartupWorktree captured `originalHeadCommit` from the launch cwd (main checkout) before any chdir. On the re-attach path the WorktreeExitDialog later runs `git rev-list <originalHeadCommit>..HEAD` inside the worktree to count "new commits this session". With the main-checkout baseline this counted every commit ever made in the kept worktree as new work from the current session — misleading the keep/remove prompt. Re-capture HEAD from inside the worktree after chdir so the count means what the dialog text says it means. P0 — getRegisteredWorktreeBranch mis-identified plain directories as registered worktrees (Claude): A plain directory at `<repo>/.qwen/worktrees/<slug>/` (e.g. a stale artifact from a previous tool) had no `.git` file of its own, so `git rev-parse --git-common-dir` walked up to the outer repo and returned the outer common-dir — matching the source repo's common-dir check and impersonating a registered worktree. If the outer repo happened to be on `worktree-<slug>`, setupStartupWorktree would silently chdir into the plain directory and treat it as attached; subsequent `exit_worktree action="remove"` would then delete a directory that was never registered. Fix: also probe `--show-toplevel` and require it to equal the candidate path (canonicalised via `realpath` so macOS /var → /private/var doesn't break the equality check). A plain dir under the main repo gets the outer repo's toplevel and is correctly rejected. Smaller polish from the same review: - Normalize the literal string `'HEAD'` returned by `getCurrentBranch` on detached HEAD to `undefined`, so the `baseRef` handed to `git worktree add -b … HEAD` does not implicitly anchor against the loose commit when the launch cwd is detached. - `symlinkConfiguredDirectories`: blocklist `.git` (any nested ancestor) and `.qwen/worktrees` (any nested ancestor). Linking `.git` would silently break commits inside the worktree; linking `.qwen/worktrees` would create a worktrees-inside-worktrees loop that confuses the startup sweep. - `WorktreeSettings.symlinkDirectories` typed `readonly string[]` to match the `createUserWorktree(options.symlinkDirectories)` contract and the immutable-config convention elsewhere. `Config.getWorktreeSymlinkDirectories()` return type updated to match. Docs: - design/worktree.md precedence table rewritten. The previous `--worktree` 赢 row was unreachable in practice (sessions are bound to `projectHash(cwd)`, and the chdir happens before session lookup). New table reflects what actually happens for each combination of `--resume` × `--worktree`, including the documented cross-projectHash limitation. The `persistStartupWorktreeSidecar` override branch is now annotated as dead-on-the-current-architecture but kept so a future Config refactor (anchor storage at repo root) picks it up for free. Tests: cli 15/15 + core 66/66 unchanged. Bundle smoke confirms both P0 fixes end-to-end (re-attach captures worktree HEAD = run-1 tip, plain-dir attempt errors out without clobbering existing content). * refactor(worktree): consolidate probe + name detached-HEAD sentinel Second /simplify pass on the dual-reviewer fixes. Three convergent findings; net effect is one fewer subprocess on the re-attach path and clearer intent on string handling / blocklist guards. Efficiency + quality: - Fold the worktree HEAD SHA into `getRegisteredWorktreeBranch`'s combined rev-parse. The probe already requests common-dir, toplevel, and abbrev-ref HEAD in a single subprocess; adding a leading `HEAD` positional (which must come BEFORE `--abbrev-ref` so the flag doesn't apply to it) returns the SHA on its own line. Return type widened to `{ branch, headCommit } | null`. Removes the second `GitWorktreeService` instantiation and `getCurrentCommitHash` call that `setupStartupWorktree`'s re-attach branch used to do. Quality: - Hoist `'HEAD'` to a module-level `DETACHED_HEAD` constant in `worktreeStartup.ts`. Three uses, two meanings (input filter when normalizing `getCurrentBranch` output, fallback metadata for the sidecar's `originalBranch` field on detached state). Naming the sentinel makes intent self-documenting and pre-empts the "why is the value we just stripped re-appearing as a fallback?" reader stall flagged by the round-3 quality review. Reuse + quality: - `symlinkConfiguredDirectories`: replace two hand-rolled containment checks (`startsWith(prefix + sep)` for `.qwen/worktrees`; `path.relative(...).split(sep)[0]` for `.git`) with `isWithinRoot` from `utils/fileUtils.ts`, which is already imported in this file. Replace the hardcoded `path.join(repoRootAbs, '.qwen', 'worktrees')` with `this.getUserWorktreesDir()` so the layout lives in one place (the exported `WORKTREES_DIR` constant). Split the misleading `sourceAbs === repoRootAbs` clause out of the `.git` branch into its own dedicated "empty / repo-root path" rejection with a clearer warn message. Tests: cli 15/15 + core 66/66 unchanged. Bundle smoke verified the folded probe still captures the worktree's HEAD on re-attach (not the launch-cwd HEAD). Skipped from this review pass: - Moving `'HEAD'` normalization into `GitWorktreeService.getCurrentBranch()` itself — would ripple through `enter-worktree.ts` and `agent.ts` callers that hand the result verbatim to `git worktree add -b ...`. Out of scope for a polish pass; the local const is enough. * fix(worktree): broaden symlink blocklist from .qwen/worktrees to all of .qwen Caught by a second pr-tracker dual-reviewer pass (Codex). The previous guard at `symlinkConfiguredDirectories` only refused paths inside `<repoRoot>/.qwen/worktrees/` — `.qwen` itself (the parent) sailed through because `isWithinRoot` is a strict descendant check. A user setting `symlinkDirectories: ['.qwen']` would therefore symlink the entire CLI metadata tree into the new worktree, recursively pulling in `.qwen/worktrees` and recreating the loop the guard was meant to prevent. Other `.qwen/*` subtrees (`projects`, `tmp`, …) are CLI state with no legitimate cross-worktree sharing use case either. Fix: broaden the guard to reject the whole `<repoRoot>/.qwen` tree. Both `.qwen` itself and any descendant fail closed. Also synced the user-facing settings schema description (the in-IDE help text and the published JSON schema) so it mentions the `.git` and `.qwen` rejection rules. The `WorktreeSettings` interface JSDoc already mentioned them; the schema description had not been updated. Tests: cli 15/15 + core 66/66 unchanged. Smoke confirms `--worktree foo` with `symlinkDirectories: ['.qwen']` configured leaves the worktree free of any `.qwen` symlink (only the legitimate per-worktree `.qwen-session` marker file appears). * fix(worktree): guard fetchPullRequestRef against CodeQL command-injection alert CodeQL flagged a "Second order command injection" finding (rule 235) on the `git fetch origin pull/<N>/head` call in `fetchPullRequestRef`. The taint analyzer doesn't see the type-narrowing at the function entry (`Number.isSafeInteger(prNumber) && prNumber > 0 && prNumber <= 1e9`), so it considers `prNumber` library input that could in principle reach a `--upload-pack=…`-shaped flag and thereby execute an arbitrary program. In practice the entry guard already prevents that, but the alert blocks the CodeQL CI check. Add `--end-of-options` between `origin` and the refspec — git's canonical "stop parsing flags" marker (git ≥ 2.24). Tells git definitively that every subsequent argv element is a positional, not a flag, which (a) satisfies the analyzer, (b) adds defense-in-depth against a future regression that might relax the entry guard, and (c) has zero behavior change for any well-formed PR number. Verified locally: `git fetch --end-of-options origin pull/<N>/head` against a local bare-remote with a seeded `refs/pull/42/head` still fetches the ref correctly; the `--worktree=#42` smoke test reads back the PR content from the materialized worktree. Tests: cli 15/15 + core 66/66 unchanged. * fix(worktree): lexical sanitizer for CodeQL + missing test mock entry Two fixes from the third CI round on PR #4381: 1. CodeQL re-fires (round 2 of the same finding). `--end-of-options` is a git-runtime defense, not a lexical sanitizer that CodeQL's `js/second-order-command-line-injection` taint tracker recognises. The alert re-fired against the same call after the previous fix. Switch to a CodeQL-recognised sanitizer: validate the numeric component against `/^[1-9][0-9]*$/` immediately at the sink. The regex digit-only check is one of the documented sanitizer patterns the rule looks for, and proves at the analyzer level that the resulting argv element cannot resemble a flag (`--foo`). The entry guard at the top of the function still establishes the same fact at runtime; this layer makes the proof visible to static analysis. Keep `--end-of-options` as a runtime fallback against any future regression that loosens the entry guard. 2. `nonInteractiveCli.test.ts` mock was missing the new `consumePendingStartupWorktreeNotice` Config method. Phase D-1 added the method on `Config` and `nonInteractiveCli` calls it on every prompt to pick up the one-shot startup-worktree notice. The test file's `mockConfig` literal was not updated, so all 19 `runNonInteractive` tests threw `TypeError: config.consumePendingStartupWorktreeNotice is not a function` on Ubuntu / macOS CI. Add a stub returning `null` so the helper short-circuits, matching the equivalent Phase C stub for `getResumedSessionData`. Local: cli (worktreeStartup + nonInteractiveCli) 60 passed + 1 skipped; core (gitWorktreeService + symlinks + hooks + enter-worktree) 66 passed. * test(worktree): mock getWorktreeSymlinkDirectories in three more test files Round 4 of the same Phase D-2 mock-drift class. CI surfaced 9 test failures across three files whose `Config` mocks construct `EnterWorktreeTool` for setup but lack the new `getWorktreeSymlinkDirectories` method `createUserWorktree` now calls: - enter-worktree.session.integ.test.ts (2 tests) - exit-worktree.session.integ.test.ts (3 tests) — provisions worktrees via EnterWorktreeTool before exercising exit paths - exit-worktree.test.ts (4 tests) — same provisioning pattern via `provisionWorktree()` and the `makeMockConfig` helper Add a `getWorktreeSymlinkDirectories: () => []` stub to each so the symlink loop is a no-op in tests. `enter-worktree.test.ts` and `agent/agent.test.ts` intentionally skipped — they mock `GitWorktreeService.createUserWorktree` outright, so the method call never fires in their code paths. Adding the stub there would be defensive speculation. If a future test exercises the real path, it'll surface there too and we'll add it then. Local: core tools tests now 123 passed (was 9 failed / 114 passed on CI run 26213122427 against commit 000c9f6). * fix(worktree): normalize repoRoot path separators + disable autocrlf in tests Round 5 of CI: Windows-only test failures on the latest HEAD. Two unrelated Windows-specific bugs, both in / around worktreeStartup. 1. `setupStartupWorktree` stored the raw `getRepoTopLevel()` output in `context.repoRoot`. git always emits POSIX paths via `--show-toplevel` (`C:/Users/...`), so on Windows the value was forward-slash where `fs.realpath` and `path.join` produce backslash. The sidecar's `originalCwd` field got the inconsistent format and a downstream `expect(...).toBe(tempRepo)` in the round-trip test compared `C:/Users/.../tmp/...` against `C:\Users\.../tmp/...`. Wrap the value in `path.resolve()` to normalize to the platform-native separator before storing. Downstream consumers (`path.join(session.originalCwd, '.qwen', 'worktrees')` in `restoreWorktreeContext`, `new GitWorktreeService(originalCwd)` in `AppContainer`) already handle either format, so no migration concern for older sidecars. 2. `makeTempRepo` in worktreeStartup.test.ts didn't configure `core.autocrlf=false`. On Windows runners the default is `true`, so files committed and pushed to the test's fake-remote `pull/<N>/head` ref get CRLF-converted on the worktree's checkout. The PR-content assertion `expect(prFile).toBe('from PR 42\n')` then failed with `'from PR 42\r\n'`. Add `core.autocrlf=false` + `core.eol=lf` to the temp-repo setup so test files round-trip byte-for-byte regardless of host platform. Local mac: cli worktreeStartup 15/15 still pass. Windows verification deferred to CI. * fix(worktree): reject '..' segments + use junction on Windows Two Copilot findings on symlinkConfiguredDirectories (PR #4381 round 3): 1. The settingsSchema description, docs/users/features/worktree.md, and WorktreeSettings JSDoc all promise that entries containing `..` are rejected — but the post-resolve isWithinRoot check accepted `foo/../bar` (resolves to `bar`, inside the repo). Add a literal `..` segment check before path.resolve so the code matches the contract. 2. On Windows, fs.symlink(..., 'dir') requires SeCreateSymbolicLinkPrivilege (admin / Developer Mode) and EPERMs on default consumer installs. Use 'junction' for directory entries on win32 — junctions are reparse points that achieve the same semantics without elevation. Keep 'dir' on POSIX and 'file' for non-directory sources (no junction-equivalent for files; rare path). Adds an integration test exercising `foo/../bar` to lock in the syntactic guard; existing absolute-path and traversal tests already covered the other rejection forms. * fix(worktree): PR-worktree HEAD-SHA capture + symlink guard tests Three findings from wenshao round 4 (PR #4381): 1. For --worktree=#42 (PR worktrees), originalHeadCommit was captured from the parent repo's HEAD via getCurrentCommitHash() — but the worktree branches off FETCH_HEAD (the PR tip), not main. Downstream, WorktreeExitDialog's `rev-list <originalHeadCommit>..HEAD` would count every commit in the fetched PR as "new work this session" alongside the user's actual commits. Same root cause covers the FETCH_HEAD TOCTOU window: between `git fetch origin pull/<N>/head` and `git worktree add ... FETCH_HEAD`, a concurrent `git fetch` from any other process sharing this repo could overwrite .git/FETCH_HEAD, causing the worktree to branch off an unrelated commit. Fix: add GitWorktreeService.resolveRef(ref) that returns a 40-char SHA (or null). In setupStartupWorktree, immediately after fetchPullRequestRef succeeds, resolve FETCH_HEAD to an immutable SHA; pass that SHA both as the baseRef to createUserWorktree (closes the TOCTOU) AND as originalHeadCommit in the returned context (closes the exit-dialog miscount). Fail-close on null resolve. 2. Orphaned JSDoc block at gitWorktreeService.ts:1035-1048 — originally wrote validateUserWorktreeSlug's docs, stranded above parsePRReference after that function was inserted between them. Move the block down to sit immediately above validateUserWorktreeSlug at its current line. 3. `.git` / `.qwen` symlink rejection guards (~20 lines of security- critical code at gitWorktreeService.ts:1640-1655) had no regression tests — only absolute paths, `..` traversal, isWithinRoot escapes, and missing sources were covered. Add two integ tests in gitWorktreeService.symlinks.integ.test.ts: one asserts `.git/hooks` is refused, one asserts `.qwen/projects` is refused. Also extends the existing PR-worktree integration test in worktreeStartup.test.ts to assert originalHeadCommit equals the resolved FETCH_HEAD SHA AND does NOT equal the parent repo's main HEAD — the assertion would fail loudly if the new SHA-capture path were reverted. * fix(worktree): realpath check on symlinkDirectories source + dest paths Security fix from PR #4381 round 7 (wenshao/qwen3.7-max). The lexical isWithinRoot + .git/.qwen blocklist checks in symlinkConfiguredDirectories all operated on path.resolve(repoRoot, raw) — a STRING operation that doesn't follow symlinks. A committed (or out-of-band) symlink at <repo>/node_modules pointing into .git would pass every gate: 1. path.resolve gives `<repo>/node_modules` (lexical, passes isWithinRoot against repo root). 2. The .git/.qwen blocklists also see the lexical path — they don't detect that the realpath chains into .git. 3. fs.stat() follows the symlink and succeeds against .git/. 4. fs.symlink writes `<worktree>/node_modules → <repo>/node_modules`, which OS-side resolves through to <repo>/.git. Any tool inside the worktree that writes to node_modules/hooks/post-merge then has RCE on the next hook-firing git operation. Fix: after fs.stat succeeds, fs.realpath the source and RE-RUN the three containment checks against the realpath. Refuse on any escape. Use the realpath (not the lexical sourceAbs) as the symlink target so the new link is one-hop canonical rather than preserving the chain. Also closes the dest-side variant of the same root cause — flagged in round 4 thread #5 (declined then as overthinking) but now in scope per the skill's iteration rule (two consecutive rounds raising the same root-cause class). path.join(worktreePath, raw) is also lexical: if git worktree add materialized a committed worktree-level symlink (e.g. HEAD ships tools → /etc), then fs.mkdir / fs.symlink for a nested entry like "tools/cache" writes OUTSIDE the worktree. Realpath the dest parent before mkdir and refuse if it escapes the worktree. New integ test covers both source-side variants (escape-to-git via out-of-band symlink + escape-to-outside-dir) in one block. Was RED against the pre-fix code: <wt>/escape-to-git was created as a symlink that chained into the source repo's .git. GREEN after the fix. * fix(worktree): canonicalise repo root before symlinkDirectories checks Round-7's source-side realpath fix introduced a canonical-vs-lexical mismatch: `repoRootAbs = path.resolve(this.sourceRepoPath)` is purely lexical, while `realSource = await fs.realpath(sourceAbs)` is canonical. On macOS where `/tmp → /private/tmp` and `/var → /private/var` are ubiquitous, and on any Linux/Windows setup where the user's checkout sits behind a symlink, the prefixes diverge at the symlink boundary and `isWithinRoot(realSource, repoRootAbs)` silently rejects every configured entry. Production callers (worktreeStartup.ts, EnterWorktreeTool, agent isolation) all pass the lexical path returned by `git rev-parse --show-toplevel`. The integ tests masked the bug because the shared `beforeEach` did `repoRoot = await fs.realpath(dir)` upfront. Round 8 fix: - Hoist `repoRootAbs`, `gitDirAbs`, `qwenDirAbs`, and `realWorktreePath` outside the for-loop — they're loop invariants and were being recomputed once per entry. - `await fs.realpath(this.sourceRepoPath)` for `repoRootAbs` so every containment check below is canonical-vs-canonical. The derived `gitDirAbs` / `qwenDirAbs` blocklist paths inherit the canonical prefix automatically. `sourceAbs = path.resolve(repoRootAbs, raw)` inherits it too, so the early lexical reject paths (absolute, `..`, repo-root equality, isWithinRoot) stay self-consistent. - Fail-close: if the repo root itself doesn't realpath (deleted / inaccessible), bail out of the entire symlink loop rather than continuing with comparisons we can't trust. Non-destructive — the worktree was created earlier by `git worktree add`. New integ test provisions the production shape: a symlink path used as `sourceRepoPath`, distinct from its canonical realpath. RED on the pre-fix code (assertion fired with "symlinkDirectories entry was silently rejected — canonical vs lexical isWithinRoot mismatch"), GREEN after.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
enter_worktree,exit_worktree) plus anisolation: 'worktree'parameter on theagenttool. Stale ephemeral worktree cleanup utility included.Validation
Commands run:
```bash
unit tests
npx vitest run src/tools/enter-worktree.test.ts \
src/services/worktreeCleanup.test.ts \
src/tools/agent/agent.test.ts
→ 80 passed (16 new + 64 existing agent tests, no regressions)
npm run typecheck # → passes across all workspaces
npm run build # → passes
npm run bundle # → produces dist/cli.js
E2E in a temp git repo against the local build
TEST_DIR=$(mktemp -d) && cd "$TEST_DIR" && git init -q && \
git config user.email t@example.com && git config user.name t && \
echo hi > README.md && git add . && git commit -qm initial
QWEN=/path/to/dist/cli.js
node $QWEN "…tool prompt…" --approval-mode yolo --output-format json
```
Prompts / inputs used: see `docs/e2e-tests/worktree.md`
Expected vs Observed: see "Reproduction report" table in `docs/e2e-tests/worktree.md`
Quickest reviewer verification path: `npx vitest run src/tools/enter-worktree.test.ts src/services/worktreeCleanup.test.ts` from `packages/core`, then start a session in any git repo and ask the model to "create a worktree named test, then exit it with action='remove'".
Evidence:
Scope / Risk
Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Refs #4056 (this PR delivers Phase A + B; Phases C and D will follow in separate PRs)
🤖 Generated with Claude Code