From 609e05baeeb40408f0e8fe7ea5d44dab5965e71f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=A1=BE=E7=9B=BC?= Date: Thu, 14 May 2026 18:00:30 +0800 Subject: [PATCH] =?UTF-8?q?feat(tools):=20add=20generic=20worktree=20suppo?= =?UTF-8?q?rt=20=E2=80=94=20EnterWorktree/ExitWorktree=20+=20Agent=20isola?= =?UTF-8?q?tion=20(#4073)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(tools): add generic worktree support (Phase A + B of #4056) Adds first-class git worktree as a general-purpose capability: Phase A — User-facing tools - enter_worktree: creates `/.qwen/worktrees/` on a `worktree-` 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 `/.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 * 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 * 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 * fix(worktree): address review findings 1-6 from PR #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 * 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 `. 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/` 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-` branches now trigger a clear error instead of clobbering committed work. 8. First worktree creation in a repo writes `/.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 * 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 * fix(worktree): address review round 3 findings 1-6 from PR #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: ]` suffix. 3. Config.initialize's startup sweep now resolves `getRepoTopLevel()` before invoking `cleanupStaleAgentWorktrees`. Without this, every subdir launch scanned a non-existent `/.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 to ") 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 * 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 `. 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 * 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 * 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(/.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 * 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 * 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 #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 * 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 `/.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//`). 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 * 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: does not exist" — fires on every CLI start for ~99% of users who never use worktrees. - "Stale worktree sweep ran under : 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 * 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 ` 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 --------- Co-authored-by: Claude Opus 4.7 --- docs/design/worktree.md | 213 +++++++ docs/e2e-tests/worktree.md | 300 +++++++++ .../src/agents/arena/ArenaManager.test.ts | 16 +- .../core/src/agents/runtime/agent-core.ts | 4 + packages/core/src/config/config.ts | 79 +++ .../core/src/services/gitWorktreeService.ts | 569 ++++++++++++++++++ .../core/src/services/worktreeCleanup.test.ts | 33 + packages/core/src/services/worktreeCleanup.ts | 187 ++++++ packages/core/src/tools/agent/agent.test.ts | 111 ++++ packages/core/src/tools/agent/agent.ts | 421 ++++++++++++- .../core/src/tools/agent/fork-subagent.ts | 23 + .../core/src/tools/enter-worktree.test.ts | 346 +++++++++++ packages/core/src/tools/enter-worktree.ts | 253 ++++++++ packages/core/src/tools/exit-worktree.test.ts | 302 ++++++++++ packages/core/src/tools/exit-worktree.ts | 396 ++++++++++++ packages/core/src/tools/tool-names.ts | 4 + 16 files changed, 3248 insertions(+), 9 deletions(-) create mode 100644 docs/design/worktree.md create mode 100644 docs/e2e-tests/worktree.md create mode 100644 packages/core/src/services/worktreeCleanup.test.ts create mode 100644 packages/core/src/services/worktreeCleanup.ts create mode 100644 packages/core/src/tools/enter-worktree.test.ts create mode 100644 packages/core/src/tools/enter-worktree.ts create mode 100644 packages/core/src/tools/exit-worktree.test.ts create mode 100644 packages/core/src/tools/exit-worktree.ts diff --git a/docs/design/worktree.md b/docs/design/worktree.md new file mode 100644 index 0000000000..4de9968b7a --- /dev/null +++ b/docs/design/worktree.md @@ -0,0 +1,213 @@ +# Worktree 通用能力设计 + +## 问题陈述 + +qwen-code 目前仅有面向 Arena 多模型对比场景的内部 worktree 实现（`GitWorktreeService`），用户无法在普通会话中使用 worktree 隔离工作，AgentTool 也不支持为 subagent 创建隔离的 worktree 环境。 + +目标是将 worktree 做成通用能力，支持用户会话级隔离和 Agent 级隔离，同时保证现有 Arena 功能体验完全不变。 + +## 现状对比 + +| 功能 | qwen-code | claude-code | +| --------------------------------- | --------------- | ----------- | +| `EnterWorktree` 工具 | ❌ | ✅ | +| `ExitWorktree` 工具 | ❌ | ✅ | +| AgentTool `isolation: 'worktree'` | ❌ | ✅ | +| worktree 会话状态持久化与恢复 | ❌ | ✅ | +| 过期 worktree 自动清理 | ❌ | ✅ | +| Post-creation setup（hooks 配置） | ❌ | ✅ | +| StatusLine worktree 状态展示 | ❌ | ✅ | +| WorktreeExitDialog（退出提示） | ❌ | ✅ | +| 符号链接目录（node_modules 等） | ❌ | ✅ | +| sparse checkout | ❌ | ✅ | +| `--worktree` CLI 启动标志 | ❌ | ✅ | +| tmux 集成 | ❌ | ✅ | +| Arena 多模型 worktree 隔离 | ✅（qwen 独有） | ❌ | +| 脏状态覆盖（stash + copy） | ✅ | ✅ | +| Baseline commit 追踪 | ✅（qwen 独有） | ❌ | + +## 设计原则 + +**worktree 是通用能力，Arena 是其上层应用。** + +- 通用 worktree 层：`EnterWorktree`/`ExitWorktree` 工具、AgentTool `isolation` 参数、会话状态管理、自动清理 +- Arena 层：多模型并行调度、`worktreeBaseDir` 自定义路径、批量创建与 diff 对比，继续使用 `GitWorktreeService.setupWorktrees()` 的现有逻辑，不受通用层改动影响 + +AgentTool 的 `isolation: 'worktree'` 只走通用路径，Arena 内部不经过这个参数创建 worktree，两者路径独立。 + +## 路径与配置 + +### 通用 worktree 路径 + +由 `EnterWorktree` 工具或 AgentTool `isolation: 'worktree'` 创建的 worktree 固定存放在： + +``` +{git 仓库根}/.qwen/worktrees/{slug} +``` + +路径不可配置。slug 命名规则： + +- 用户会话 worktree：用户指定名称，或自动生成（格式：`{形容词}-{名词}-{4位随机}`） +- Agent worktree：`agent-{7位随机 hex}` + +### Arena worktree 路径（已有，保持不变） + +Arena 的 worktree 路径由 `agents.arena.worktreeBaseDir` 控制，默认 `~/.qwen/arena`（`ArenaManager.ts:125`），与通用路径完全独立，不做任何改动。 + +### 扩展配置（暂缓至 Phase C/D） + +| 配置项 | 类型 | 用途 | 阶段 | +| ----------------------------- | ---------- | -------------------------------------------------------------- | ------- | +| `worktree.symlinkDirectories` | `string[]` | 符号链接指定目录（如 `node_modules`）到 worktree，避免磁盘浪费 | Phase C | +| `worktree.sparsePaths` | `string[]` | git sparse-checkout cone 模式，大型 monorepo 只写入指定路径 | Phase D | + +Phase A / B 不新增任何配置项。 + +## 工具设计 + +### EnterWorktree + +**触发条件：** 用户明确说 "start a worktree"、"use a worktree"、"create a worktree" 等词语。不应在用户说"修复 bug"、"开发功能"时自动触发。 + +**输入 schema：** + +``` +name?: string // 可选，slug 格式：字母/数字/点/下划线/破折号，最大 64 字符 +``` + +**行为：** + +1. 验证当前未在 worktree 中（防止嵌套） +2. 解析到 git 仓库根（处理已在子目录的情况） +3. 调用 `GitWorktreeService` 创建 worktree，路径为 `.qwen/worktrees/{slug}` +4. 将 worktree 会话写入 `SessionService` +5. 切换工作目录到 worktree 路径 +6. 清除文件缓存 + +**输出：** `worktreePath`、`worktreeBranch`、`message` + +### ExitWorktree + +**触发条件：** 用户说 "exit the worktree"、"leave the worktree"、"go back" 等。 + +**输入 schema：** + +``` +action: 'keep' | 'remove' +discard_changes?: boolean // 仅 action='remove' 时有效 +``` + +**安全守卫：** + +- 仅操作本会话通过 `EnterWorktree` 创建的 worktree +- `action='remove'` 且存在未提交变更时，拒绝执行（除非 `discard_changes: true`） + +**行为：** + +- `keep`：清空会话中的 worktree 状态，保留 worktree 目录和分支，恢复原始工作目录 +- `remove`：删除 worktree 目录，删除对应 git 分支，清空会话状态，恢复原始工作目录 + +**输出：** `action`、`originalCwd`、`worktreePath`、`worktreeBranch` + +## 用户触发方式 + +| 方式 | 示例 | 实现阶段 | +| -------------- | -------------------------------------------------------- | -------- | +| 会话中明确请求 | 用户说 "在 worktree 中开始工作" → 模型调用 EnterWorktree | Phase A | +| Agent 隔离 | 模型为 subagent 设置 `isolation: 'worktree'` | Phase B | +| CLI 启动标志 | `qwen --worktree my-feature` | Phase D | + +无斜杠命令。会话中 worktree 的触发依赖用户明确提及，`isolation: 'worktree'` 才是模型自主决策的场景。 + +## 分阶段实现计划 + +### Phase A：核心工具（用户会话级 worktree） + +**目标：** 用户能在会话中进入 / 退出 worktree。 + +**要实现的功能：** + +- `EnterWorktree` 工具：创建 worktree，切换工作目录，记录会话状态 +- `ExitWorktree` 工具：keep / remove 两种退出方式，安全守卫 +- `GitWorktreeService` 扩展：新增面向单用户会话的 `createUserWorktree()` / `removeUserWorktree()` 方法，复用现有 git 操作逻辑，不改动 Arena 使用的批量接口 +- `SessionService` 扩展：新增 `WorktreeSession` 字段，记录 `{ slug, worktreePath, worktreeBranch, originalCwd, originalBranch }`；`--resume` 时恢复 worktree 工作目录 +- 工具 prompt：为每个工具编写使用说明，明确何时调用、何时不调用 + +**影响文件：** + +| 文件 | 变更类型 | +| -------------------------------------------------- | --------------------------------------------- | +| `packages/core/src/tools/tool-names.ts` | 新增 `ENTER_WORKTREE`、`EXIT_WORKTREE` 常量 | +| `packages/core/src/tools/EnterWorktreeTool/` | 新建目录：`EnterWorktreeTool.ts`、`prompt.ts` | +| `packages/core/src/tools/ExitWorktreeTool/` | 新建目录：`ExitWorktreeTool.ts`、`prompt.ts` | +| `packages/core/src/services/gitWorktreeService.ts` | 新增用户会话级接口（不改动 Arena 接口） | +| `packages/core/src/services/sessionService.ts` | 新增 `WorktreeSession` 字段及读写方法 | +| `packages/core/src/tools/` 注册入口 | 注册新工具 | + +**不在 Phase A 范围内：** + +- Agent 隔离（Phase B） +- hooks 配置等 post-creation setup（Phase C） +- UI 状态展示（Phase C） + +--- + +### Phase B：Agent 隔离（AgentTool `isolation: 'worktree'`）+ 描述更新 + +**目标：** 模型可为 subagent 创建临时隔离 worktree，agent 结束后自动清理；同步更新受影响的工具描述和提示词。 + +**要实现的功能：** + +_Agent 隔离核心：_ + +- `AgentTool` 新增 `isolation?: 'worktree'` 参数 +- Agent 启动时创建临时 worktree（slug：`agent-{7hex}`，路径：`.qwen/worktrees/agent-{7hex}`） +- Agent 结束后：无变更则自动删除；有变更则保留，将路径和分支返回在结果中 +- 过期 worktree 自动清理：扫描 `.qwen/worktrees/`，匹配 `agent-{7hex}` 模式，超过 30 天且无未推送提交则删除，fail-closed 策略 + +_描述与提示词更新：_ + +- `AgentTool` description 补充 `isolation: 'worktree'` 参数说明（参考 claude-code `AgentTool/prompt.ts:272`） +- 新增 `buildWorktreeNotice()`：当 fork subagent 在 worktree 中运行时，向其注入上下文提示，说明其处于隔离 worktree、路径继承自父 agent、编辑前需重新读取文件（参考 claude-code `forkSubagent.ts:buildWorktreeNotice`） + +_无需改动：_ + +- review skill（`SKILL.md`）：review 使用独立机制（路径 `.qwen/tmp/review-pr-`，通过 `qwen review fetch-pr` 命令创建），与通用 worktree 路径和机制完全不同，不存在混淆 + +**Arena 兼容保证：** Arena 内部不经过 `isolation` 参数创建 worktree，此改动不触碰 Arena 代码路径。 + +**影响文件：** + +| 文件 | 变更类型 | +| -------------------------------------------------- | ------------------------------------------------------ | +| `packages/core/src/tools/agent/agent.ts` | 新增 `isolation` 参数及 worktree 创建/清理逻辑 | +| `packages/core/src/tools/agent/fork-subagent.ts` | 新增 `buildWorktreeNotice()` 并在 worktree 模式下注入 | +| `packages/core/src/services/gitWorktreeService.ts` | 新增 `createAgentWorktree()` / `removeAgentWorktree()` | +| `packages/core/src/services/worktreeCleanup.ts` | 新建：过期 worktree 自动清理逻辑 | + +--- + +### Phase C：体验优化（Post-creation setup + UI） + +**目标：** worktree 创建后自动初始化环境，状态在界面上可见。 + +**要实现的功能：** + +- Post-creation setup：配置 `core.hooksPath` 指向主仓库（qwen-code 无 `settings.local.json` 概念，不需要复制） +- StatusLine 展示当前 worktree 名称 / 分支 +- WorktreeExitDialog：会话退出时（检测到 worktree 仍活跃）提示用户选择 keep 或 remove +- 新增 `worktree.symlinkDirectories` 配置项，实现目录符号链接 + +--- + +### Phase D：高级功能 + +**目标：** 对齐 claude-code 的完整特性集。 + +**要实现的功能：** + +- `--worktree [name]` CLI 启动标志：启动时直接创建 worktree，整个会话在隔离环境中运行 +- sparse checkout 支持：新增 `worktree.sparsePaths` 配置项 +- `.worktreeinclude` 文件：支持将 gitignore 的文件复制到 worktree +- tmux 集成：`--worktree --tmux` 在 tmux 会话中启动 +- PR 引用解析：`--worktree=#123` 自动 fetch 并基于 PR 创建 worktree diff --git a/docs/e2e-tests/worktree.md b/docs/e2e-tests/worktree.md new file mode 100644 index 0000000000..12f13c6a8f --- /dev/null +++ b/docs/e2e-tests/worktree.md @@ -0,0 +1,300 @@ +# Worktree Feature E2E Test Plan (Phase A + B) + +## Scope + +End-to-end tests for the generic worktree capability: + +- Phase A: `EnterWorktree` / `ExitWorktree` tools + SessionService state +- Phase B: `Agent` tool `isolation: 'worktree'` parameter + auto-cleanup + worktree notice + +## Test environment + +Each test group runs in its own temp git repo and tmux session to avoid collisions. Template setup: + +```bash +TEST_DIR=$(mktemp -d -t worktree-test-XXXXXX) +cd "$TEST_DIR" +git init -q +git config user.email "test@example.com" +git config user.name "Test" +echo "hello" > README.md +git add README.md +git commit -q -m "initial" +``` + +Each group uses a unique tmux session name (e.g. `wt-test-a`, `wt-test-b`) and a unique temp dir. + +Baseline binary: globally installed `qwen` (0.15.10). +Local build binary: `node /Users/mochi/code/qwen-code/.claude/worktrees/trusting-euclid-6fdfb9/bundle/qwen.js`. + +## Test Group A: EnterWorktree tool registration and basic creation + +**Mode:** Headless, `--approval-mode yolo`, `--output-format json` + +### A1: Tool registered in system init + +**Steps:** + +```bash + "say hello" --approval-mode yolo --output-format json 2>/dev/null \ + | jq -r 'select(.type=="system") | .tools[]' \ + | grep -E "^(enter_worktree|exit_worktree)$" +``` + +**Pre-implementation:** empty (tools not registered). +**Post-implementation:** outputs `enter_worktree` and `exit_worktree`. + +### A2: Create worktree with auto-generated name + +**Steps:** + +```bash + "create a new git worktree using the enter_worktree tool" \ + --approval-mode yolo --output-format json 2>/dev/null > /tmp/a2.json +# Check worktree dir created +ls -la .qwen/worktrees/ | grep -v "^\." | wc -l +# Should have a directory matching the auto-generated slug pattern +``` + +**Pre-implementation:** model says it can't find the tool; no `.qwen/worktrees/` directory. +**Post-implementation:** `.qwen/worktrees/` exists with auto-generated slug (format: `{adj}-{noun}-{4hex}`). + +### A3: Create worktree with custom name + +**Steps:** + +```bash + "use the enter_worktree tool with name='my-feature' to create a worktree" \ + --approval-mode yolo --output-format json 2>/dev/null +ls .qwen/worktrees/my-feature/ +git branch | grep worktree-my-feature +``` + +**Pre-implementation:** tool unknown. +**Post-implementation:** `.qwen/worktrees/my-feature/` directory exists; branch `worktree-my-feature` exists. + +### A4: Invalid slug rejected + +**Steps:** + +```bash + "use enter_worktree with name='../../../etc' to create a worktree" \ + --approval-mode yolo --output-format json 2>/dev/null \ + | jq 'select(.type=="user") | .message.content[] | select(.is_error) | .content' +``` + +**Pre-implementation:** tool unknown. +**Post-implementation:** tool result is_error=true with a validation error message. + +## Test Group B: ExitWorktree + +**Mode:** Headless, two-step interaction within one prompt. + +### B1: Enter then exit with action=keep + +**Steps:** + +```bash + "create a worktree named 'temp-keep' using enter_worktree, then immediately exit it with action='keep' using exit_worktree" \ + --approval-mode yolo --output-format json 2>/dev/null > /tmp/b1.json +# Directory should still exist (keep preserves it) +ls -d .qwen/worktrees/temp-keep +# Branch should still exist +git branch | grep worktree-temp-keep +# CWD should be original +``` + +**Pre-implementation:** tools unknown. +**Post-implementation:** worktree dir and branch both still exist after exit. + +### B2: Enter then exit with action=remove (no changes) + +**Steps:** + +```bash + "create a worktree named 'temp-remove' using enter_worktree, then immediately exit it with action='remove' using exit_worktree" \ + --approval-mode yolo --output-format json 2>/dev/null +ls -d .qwen/worktrees/temp-remove 2>&1 +git branch | grep worktree-temp-remove +``` + +**Pre-implementation:** tools unknown. +**Post-implementation:** worktree dir is removed; branch is deleted. + +### B3: Exit with action=remove refuses when uncommitted changes exist + +**Steps:** Spawn an interactive tmux session, manually create files in worktree, then attempt exit. + +```bash +tmux new-session -d -s wt-test-b3 -x 200 -y 50 "cd $TEST_DIR && --approval-mode yolo" +sleep 3 +tmux send-keys -t wt-test-b3 "create a worktree named 'dirty-test' using enter_worktree" +sleep 0.5 +tmux send-keys -t wt-test-b3 Enter +# Wait for completion +for i in $(seq 1 30); do + sleep 2 + tmux capture-pane -t wt-test-b3 -p | grep -q "Type your message" && break +done +# Create dirty file in worktree +echo "dirty" > "$TEST_DIR/.qwen/worktrees/dirty-test/dirty.txt" +# Try to remove without discard_changes +tmux send-keys -t wt-test-b3 "use exit_worktree with action='remove' to exit the worktree" +sleep 0.5 +tmux send-keys -t wt-test-b3 Enter +for i in $(seq 1 30); do sleep 2; tmux capture-pane -t wt-test-b3 -p | grep -q "Type your message" && break; done +tmux capture-pane -t wt-test-b3 -p -S -100 > /tmp/b3.out +# Should mention "uncommitted changes" or "discard_changes" in output +grep -E "uncommitted|discard_changes" /tmp/b3.out +tmux kill-session -t wt-test-b3 +``` + +**Pre-implementation:** tools unknown. +**Post-implementation:** exit fails with a message about uncommitted changes and the `discard_changes` flag. + +## Test Group C: SessionService persistence + +### C1: Worktree state in session metadata + +**Steps:** + +```bash +SESSION_ID=$( "create a worktree named 'persist-test' using enter_worktree" \ + --approval-mode yolo --output-format json 2>/dev/null \ + | jq -r 'select(.type=="system") | .session_id' | head -1) +# Check session storage for worktree state +find ~/.qwen -name "*${SESSION_ID}*" 2>/dev/null | head +grep -l "persist-test" ~/.qwen/projects/*/sessions/*.json 2>/dev/null || \ + grep -rl "worktreeSession\|persist-test" ~/.qwen/projects/ 2>/dev/null | head -5 +``` + +**Pre-implementation:** no worktree session state stored anywhere. +**Post-implementation:** session JSON contains a `worktreeSession` field with `slug='persist-test'`, `worktreePath`, `originalCwd`, etc. + +## Test Group D: AgentTool isolation + +### D1: Agent isolation parameter accepted + +**Steps:** + +```bash + "spawn an agent using the agent tool with isolation='worktree' to run 'echo hello'" \ + --approval-mode yolo --output-format json 2>/dev/null \ + | jq 'select(.type=="assistant") | .message.content[] | select(.type=="tool_use" and .name=="agent") | .input' +# Check that .qwen/worktrees/ contains an agent-* slug during execution +``` + +**Pre-implementation:** agent tool schema has no isolation parameter; model either omits it or the schema rejects it. +**Post-implementation:** agent runs successfully with isolation='worktree'; an `agent-<7hex>` worktree is created. + +### D2: Agent auto-cleans worktree (no changes) + +**Steps:** + +```bash +ls .qwen/worktrees/ > /tmp/d2-before.txt 2>/dev/null + "spawn an agent with isolation='worktree' to list files in the current directory using ls" \ + --approval-mode yolo --output-format json 2>/dev/null +ls .qwen/worktrees/ > /tmp/d2-after.txt 2>/dev/null +# After should equal before (no leftover agent-* dirs) +diff /tmp/d2-before.txt /tmp/d2-after.txt +``` + +**Pre-implementation:** N/A (no isolation parameter). +**Post-implementation:** worktrees dir is unchanged after agent completes with no changes. + +### D3: Agent worktree preserved when changes made + +**Steps:** + +```bash + "spawn an agent with isolation='worktree' to write 'test content' to a new file called test.txt" \ + --approval-mode yolo --output-format json 2>/dev/null > /tmp/d3.json +# Worktree should be preserved with the change +ls .qwen/worktrees/agent-* 2>/dev/null +ls .qwen/worktrees/agent-*/test.txt 2>/dev/null +# Agent result should include worktreePath/worktreeBranch +jq 'select(.type=="user") | .message.content[] | select(.tool_use_id) | .content' /tmp/d3.json | head +``` + +**Pre-implementation:** N/A. +**Post-implementation:** `.qwen/worktrees/agent-<7hex>/test.txt` exists; agent result mentions worktree path and branch. + +## Test Group E: Stale cleanup + +### E1: Cleanup function removes old agent worktrees + +This is harder to test e2e because it requires aging. Cover via unit tests in `worktreeCleanup.test.ts`: + +- Worktree with mtime > 30 days ago and matching `agent-<7hex>` pattern → removed +- Worktree with mtime > 30 days ago but user-named (e.g., `my-feature`) → preserved +- Worktree with mtime < 30 days → preserved +- Worktree with uncommitted changes → preserved (fail-closed) +- Worktree with unpushed commits → preserved (fail-closed) + +E2E spot check (optional): manually `touch -t 200001010000 .qwen/worktrees/agent-aabcdef0` and invoke cleanup; verify removal. + +## Test Group F: Arena compatibility (no regression) + +### F1: Arena worktree path unchanged + +**Steps:** Run an Arena session (separate from EnterWorktree); verify it still creates worktrees under `~/.qwen/arena//worktrees/` and not under `.qwen/worktrees/`. + +```bash +# Setup: requires Arena-enabled config. Detailed steps depend on Arena CLI invocation. +# Pre-implementation: arena worktrees are under ~/.qwen/arena/. +# Post-implementation: SAME — arena path is independent. +``` + +(If Arena is not easily reachable from headless mode, this group is verified by unit test that ArenaManager.ts:125 (`this.arenaBaseDir = arenaSettings?.worktreeBaseDir ?? path.join(Storage.getGlobalQwenDir(), 'arena')`) is unchanged.) + +## Unit test coverage (collocated with implementation) + +Outside of the E2E plan, these unit tests must accompany the implementation: + +- `EnterWorktreeTool.test.ts`: schema validation, slug rejection, nested-worktree rejection, cwd change, SessionService write +- `ExitWorktreeTool.test.ts`: keep vs remove paths, dirty-state guard, discard_changes bypass, cwd restoration +- `gitWorktreeService.test.ts` extensions: `createUserWorktree`, `removeUserWorktree`, `createAgentWorktree`, `removeAgentWorktree` +- `sessionService.test.ts` extensions: WorktreeSession field read/write, resume restoration +- `worktreeCleanup.test.ts`: cleanup pattern matching, age filter, fail-closed conditions +- `agent.test.ts` extensions: isolation parameter accepted, worktree created and (in some cases) cleaned + +## Pass criteria + +| Group | Pre-build expected | Post-build expected | +| ----- | ------------------ | ---------------------------------------------------- | +| A1 | tools not listed | both tools listed | +| A2 | error/no-op | `.qwen/worktrees/` created | +| A3 | error/no-op | `.qwen/worktrees/my-feature` created, branch present | +| A4 | error/no-op | tool result is_error with validation message | +| B1 | error/no-op | worktree dir + branch preserved | +| B2 | error/no-op | worktree dir + branch removed | +| B3 | error/no-op | exit refuses with uncommitted-changes message | +| C1 | no worktree state | session has worktreeSession field | +| D1 | no isolation param | agent runs in `agent-<7hex>` worktree | +| D2 | N/A | worktrees dir unchanged after agent with no changes | +| D3 | N/A | `agent-<7hex>` preserved with changes | + +## Reproduction report (post-implementation) + +Local build at `dist/cli.js` (commit at the tip of `claude/trusting-euclid-6fdfb9`). + +| Group | Result | Notes | +| ----- | ------------------------------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| A1 | ✅ | `enter_worktree` and `exit_worktree` listed in `system.tools` | +| A3 | ✅ | `.qwen/worktrees/my-feature` created, branch `worktree-my-feature` present | +| A4 | covered by unit test | `validateUserWorktreeSlug` rejects path-traversal etc. (`enter-worktree.test.ts`) | +| B1 | ✅ | `keep` action preserved both directory and branch | +| B2 | ✅ | `remove` action deleted directory and branch | +| B3 | ✅ | `remove` refused with `Refusing to remove worktree "dirty-test" — it has 0 tracked change(s) and 1 untracked file(s).` | +| C1 | scope-out | SessionService persistence deferred from Phase A (see scope notes in `docs/design/worktree.md`) | +| D1 | ✅ | Agent invocation accepted `isolation: 'worktree'`, created `agent-2c4e759` | +| D2 | ✅ | After agent finished with no changes, worktrees dir was empty | +| D3 | ✅ | After agent wrote `test.txt`, worktree `agent-bad55bd` and branch `worktree-agent-bad55bd` preserved; result included `[worktree preserved: ... (branch ...)]` suffix | +| E1 | covered by unit test | `worktreeCleanup.test.ts` verifies `isEphemeralSlug` matches only `agent-<7hex>` | +| F1 | scope-out (no Arena E2E in this run) | Arena code paths untouched: `ArenaManager.ts:125` and `setupWorktrees()` unchanged | + +### Scope deviations from the test plan + +- **C1** (SessionService persistence) was deferred from Phase A. The minimum-viable Phase A returns the absolute worktree path so the model uses it directly via absolute paths, instead of mechanically switching `Config.targetDir`. Resume support requires SessionService extension and is documented for a future phase. +- **A2** (auto-generated name) was indirectly verified via D1/D3, which exercise the same auto-slug path through the agent isolation flow. diff --git a/packages/core/src/agents/arena/ArenaManager.test.ts b/packages/core/src/agents/arena/ArenaManager.test.ts index d1fb6b2fef..744c059a81 100644 --- a/packages/core/src/agents/arena/ArenaManager.test.ts +++ b/packages/core/src/agents/arena/ArenaManager.test.ts @@ -29,7 +29,19 @@ vi.mock('../index.js', async (importOriginal) => { // Mock GitWorktreeService to avoid real git operations. // The class mock includes static methods used by ArenaManager. -vi.mock('../../services/gitWorktreeService.js', () => { +// +// Preserve every other export via `importActual` so unrelated +// consumers of the module (e.g. `worktreeCleanup.ts` → +// `AGENT_WORKTREE_SLUG_PATTERN`, `worktreeBranchForSlug`, +// `generateAgentWorktreeSlug`, `WORKTREE_BRANCH_PREFIX`, +// session-marker helpers) keep working. Without this, vitest replaces +// the entire module surface and any static import of those constants +// elsewhere in the dependency graph blows up at load time. +vi.mock('../../services/gitWorktreeService.js', async (importOriginal) => { + const actual = + await importOriginal< + typeof import('../../services/gitWorktreeService.js') + >(); const MockClass = vi.fn().mockImplementation(() => ({ checkGitAvailable: vi.fn().mockResolvedValue({ available: true }), isGitRepository: vi.fn().mockResolvedValue(true), @@ -47,7 +59,7 @@ vi.mock('../../services/gitWorktreeService.js', () => { (MockClass as unknown as Record)['getWorktreesDir'] = ( sessionId: string, ) => path.join(os.tmpdir(), 'arena-mock', sessionId, 'worktrees'); - return { GitWorktreeService: MockClass }; + return { ...actual, GitWorktreeService: MockClass }; }); // Mock the Config class diff --git a/packages/core/src/agents/runtime/agent-core.ts b/packages/core/src/agents/runtime/agent-core.ts index d0c7704ce1..1caa74075b 100644 --- a/packages/core/src/agents/runtime/agent-core.ts +++ b/packages/core/src/agents/runtime/agent-core.ts @@ -94,6 +94,10 @@ export const EXCLUDED_TOOLS_FOR_SUBAGENTS: ReadonlySet = new Set([ ToolNames.CRON_DELETE, ToolNames.TASK_STOP, ToolNames.SEND_MESSAGE, + // Worktree management belongs to the parent session — a subagent must + // never enter or exit the user's worktree state independently. + ToolNames.ENTER_WORKTREE, + ToolNames.EXIT_WORKTREE, ]); /** diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 000faa8e10..ba7bdc53ab 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -7,6 +7,7 @@ // Node built-ins import type { EventEmitter } from 'node:events'; import * as fs from 'node:fs'; +import * as fsPromises from 'node:fs/promises'; import * as path from 'node:path'; import process from 'node:process'; @@ -43,6 +44,8 @@ import { type FileEncodingType, } from '../services/fileSystemService.js'; import { GitService } from '../services/gitService.js'; +import { GitWorktreeService } from '../services/gitWorktreeService.js'; +import { cleanupStaleAgentWorktrees } from '../services/worktreeCleanup.js'; import { CronScheduler } from '../services/cronScheduler.js'; // Tools — only lightweight imports; tool classes are lazy-loaded via dynamic import @@ -1283,6 +1286,74 @@ export class Config { logStartSession(this, new StartSessionEvent(this)); this.debugLogger.info('Config initialization completed'); + + // Fire-and-forget sweep of stale ephemeral worktrees left behind by + // earlier `agent` runs that exited before their cleanup helper ran + // (Ctrl-C, process crash, abrupt shutdown). The sweep only touches + // `agent-<7hex>` slugs, skips anything newer than 30 days, and + // is fail-closed against tracked changes or unpushed commits — so + // running it on every startup cannot destroy user work. We do not + // await this: it is a hygiene task that must never delay the + // first model turn. + // + // Anchor the sweep at the repo top-level so it scans the same + // directory the worktree creators (`enter_worktree` and + // `agent isolation:'worktree'`) write to. Using `this.targetDir` + // directly would cause launches from a monorepo subdirectory to + // scan `/.qwen/worktrees/` — which never exists — and the + // sweep would silently be a no-op forever. + if (!this.getBareMode()) { + void (async () => { + try { + // Resolve the repo top-level FIRST. The previous code bailed + // on `fs.access(/.qwen/worktrees)` before resolving, + // so a monorepo subdir launch (where `targetDir` is the + // subdir, not the repo root) always early-returned and the + // sweep was permanently a no-op. Fast-bail still happens, just + // against the *correct* directory. + 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 { + // Skipped (no worktrees dir) is the common-case happy + // path on every CLI start for ~99% of users. `debug` so + // operators can opt in via `--debug` when they actually + // want to confirm the sweep is wired up — `info` would + // be log noise. + this.debugLogger.debug( + `Stale worktree sweep skipped: ${worktreesDir} does not exist`, + ); + return; + } + const removed = await cleanupStaleAgentWorktrees(root); + if (removed > 0) { + // Only the "actually removed something" path warrants + // `info` — that's the signal an operator chasing a leak + // would grep for. The "ran, found nothing" path is + // reconstructable at `debug` and is otherwise noise: + // every CLI start that has any worktree dir would emit + // it, drowning the actually-actionable message. + this.debugLogger.info( + `Stale worktree sweep removed ${removed} ephemeral worktree(s) under ${root}`, + ); + } else { + this.debugLogger.debug( + `Stale worktree sweep ran under ${root}: nothing to remove`, + ); + } + } catch (error: unknown) { + // Promote sweep errors to `warn` for the same reason: a + // permission failure / disk full / repo-corruption case + // should leave a visible breadcrumb instead of being + // invisible at the default log level. + this.debugLogger.warn( + `Stale worktree sweep failed (non-fatal): ${error}`, + ); + } + })(); + } } /** @@ -3254,6 +3325,14 @@ export class Config { return new ExitPlanModeTool(this); }); } + await registerLazy(ToolNames.ENTER_WORKTREE, async () => { + const { EnterWorktreeTool } = await import('../tools/enter-worktree.js'); + return new EnterWorktreeTool(this); + }); + await registerLazy(ToolNames.EXIT_WORKTREE, async () => { + const { ExitWorktreeTool } = await import('../tools/exit-worktree.js'); + return new ExitWorktreeTool(this); + }); await registerLazy(ToolNames.WEB_FETCH, async () => { const { WebFetchTool } = await import('../tools/web-fetch.js'); return new WebFetchTool(this); diff --git a/packages/core/src/services/gitWorktreeService.ts b/packages/core/src/services/gitWorktreeService.ts index bec5ca6aeb..afb1c58913 100644 --- a/packages/core/src/services/gitWorktreeService.ts +++ b/packages/core/src/services/gitWorktreeService.ts @@ -6,14 +6,108 @@ import * as fs from 'node:fs/promises'; import * as path from 'node:path'; +import { randomBytes, randomInt } from 'node:crypto'; import { execSync } from 'node:child_process'; import { simpleGit, CheckRepoActions } from 'simple-git'; import type { SimpleGit } from 'simple-git'; import { Storage } from '../config/storage.js'; import { isCommandAvailable } from '../utils/shell-utils.js'; import { isNodeError } from '../utils/errors.js'; +import { createDebugLogger } from '../utils/debugLogger.js'; +import { fileExists } from '../utils/fileUtils.js'; import { initRepositoryWithMainBranch } from './gitInit.js'; +const debugLogger = createDebugLogger('GIT_WORKTREE_SERVICE'); + +/** Prefix applied to every general-purpose worktree branch. */ +export const WORKTREE_BRANCH_PREFIX = 'worktree-'; + +/** Returns the canonical branch name for a worktree slug. */ +export function worktreeBranchForSlug(slug: string): string { + return `${WORKTREE_BRANCH_PREFIX}${slug}`; +} + +/** + * Filename of the in-worktree session marker. Created at worktree + * provisioning time and consulted by `exit_worktree` to decide + * whether the current session is allowed to drop the worktree. The + * 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'; + +/** Writes the owning session id into the worktree's session marker. */ +export async function writeWorktreeSessionMarker( + worktreePath: string, + sessionId: string, +): Promise { + await fs.writeFile( + path.join(worktreePath, WORKTREE_SESSION_FILE), + sessionId, + 'utf8', + ); + // The marker lives inside the worktree dir so a subagent running + // `git add -A` inside it would otherwise add the session id to its + // first commit. Write a `.git/info/exclude` rule so the marker is + // ignored without requiring (or modifying) a tracked `.gitignore`. + // `.git` inside a worktree is actually a file pointing at + // `/.git/worktrees//`, so resolve `--git-dir` instead + // of joining naively. + try { + const wtGit = simpleGit(worktreePath); + const gitDir = (await wtGit.revparse(['--git-dir'])).trim(); + const excludePath = path.isAbsolute(gitDir) + ? path.join(gitDir, 'info', 'exclude') + : path.join(worktreePath, gitDir, 'info', 'exclude'); + await fs.mkdir(path.dirname(excludePath), { recursive: true }); + let existing = ''; + try { + existing = await fs.readFile(excludePath, 'utf8'); + } catch { + // File missing — fall through to fresh write. + } + const rule = WORKTREE_SESSION_FILE; + if (!existing.split(/\r?\n/).includes(rule)) { + const sep = existing.length === 0 || existing.endsWith('\n') ? '' : '\n'; + await fs.writeFile(excludePath, `${existing}${sep}${rule}\n`, 'utf8'); + } + } catch { + // Best-effort: if we can't write the exclude rule (read-only fs, + // unusual worktree layout), the marker is still functional — + // `git add -A` would just stage it. The ownership guard remains + // intact either way. + } +} + +/** + * Reads the owning session id stored at worktree provisioning time. + * Returns `null` when the marker is missing or unreadable — callers + * decide whether to treat that as "owner unknown, refuse" or "owner + * unknown, allow with explicit override". + */ +export async function readWorktreeSessionMarker( + worktreePath: string, +): Promise { + const markerPath = path.join(worktreePath, WORKTREE_SESSION_FILE); + try { + const raw = await fs.readFile(markerPath, 'utf8'); + const trimmed = raw.trim(); + return trimmed.length > 0 ? trimmed : null; + } catch (error) { + // Distinguish "marker missing" (legitimate — worktree predates the + // session-ownership guard) from "marker unreadable" (disk error, + // permission, corrupt NFS). Both still return `null`, but the + // unreadable case logs so an operator chasing a "wrong session + // bypassed the ownership guard" report has a breadcrumb. + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + debugLogger.warn( + `readWorktreeSessionMarker: cannot read ${markerPath}: ${error}`, + ); + } + return null; + } +} + /** * Commit message used for the baseline snapshot in worktrees. * After overlaying the user's dirty state (tracked changes + untracked files), @@ -29,6 +123,37 @@ export const BASELINE_COMMIT_MESSAGE = 'baseline (dirty state overlay)'; */ export const WORKTREES_DIR = 'worktrees'; +// ────────────────────────────────────────────────────────────────────── +// Ephemeral agent-worktree slug format. Shared between the producer +// (`AgentTool isolation: 'worktree'`), the consumer +// (`cleanupStaleAgentWorktrees`) and the validator +// (`validateUserWorktreeSlug` reserves the prefix). Changing any of +// these constants must be done in one place so a regex / generator +// mismatch can never silently leak or destroy work. +// ────────────────────────────────────────────────────────────────────── + +/** Slug prefix used for worktrees created by `AgentTool isolation:'worktree'`. */ +export const AGENT_WORKTREE_PREFIX = 'agent'; + +/** Number of random hex characters appended after the prefix. */ +export const AGENT_WORKTREE_HEX_LENGTH = 7; + +/** Regex that matches the exact ephemeral-agent slug shape. */ +export const AGENT_WORKTREE_SLUG_PATTERN = new RegExp( + `^${AGENT_WORKTREE_PREFIX}-[0-9a-f]{${AGENT_WORKTREE_HEX_LENGTH}}$`, +); + +/** + * Generates a fresh ephemeral-agent slug. Centralised so the format + * stays in lock-step with {@link AGENT_WORKTREE_SLUG_PATTERN}. + */ +export function generateAgentWorktreeSlug(): string { + const hex = randomBytes(Math.ceil(AGENT_WORKTREE_HEX_LENGTH / 2)) + .toString('hex') + .slice(0, AGENT_WORKTREE_HEX_LENGTH); + return `${AGENT_WORKTREE_PREFIX}-${hex}`; +} + export interface WorktreeInfo { /** Unique identifier for this worktree */ id: string; @@ -152,6 +277,34 @@ export class GitWorktreeService { return { available: true }; } + /** + * Resolves the absolute path of the enclosing git repository's top + * directory. Used by callers that need to anchor general-purpose + * worktrees at the *repo* root rather than the cwd they were invoked + * from — otherwise running `qwen` from a monorepo subdirectory would + * scatter `.qwen/worktrees/` under each subdirectory instead of + * gathering them under the repo root. + * + * Returns the canonical top-level path on success, or `null` when the + * cwd is not inside a git repo (caller should error). + */ + async getRepoTopLevel(): Promise { + try { + const out = await this.git.revparse(['--show-toplevel']); + const top = out.trim(); + return top.length > 0 ? top : null; + } catch (error) { + // Caller falls back to its cwd via `?? cwd`. Log so a corrupt + // repo / permission failure leaves a trail — otherwise the + // worktree creator and startup sweep can disagree silently about + // where worktrees live, and the sweep would never find them. + debugLogger.warn( + `getRepoTopLevel failed at ${this.sourceRepoPath}: ${error}`, + ); + return null; + } + } + /** * Checks if the source path is a git repository. */ @@ -824,4 +977,420 @@ export class GitWorktreeService { return false; } } + + // ────────────────────────────────────────────────────────────────────── + // User-facing worktree APIs (used by EnterWorktree / ExitWorktree tools + // and AgentTool `isolation: 'worktree'`). These create worktrees under + // `/.qwen/worktrees/` rather than under the + // session-scoped Arena baseDir. + // ────────────────────────────────────────────────────────────────────── + + /** + * Returns the directory holding all general-purpose worktrees for this + * repo: `/.qwen/worktrees`. + */ + getUserWorktreesDir(): string { + return path.join(this.sourceRepoPath, '.qwen', WORKTREES_DIR); + } + + /** + * Returns the absolute worktree path for a given slug. + */ + getUserWorktreePath(slug: string): string { + return path.join(this.getUserWorktreesDir(), slug); + } + + /** + * Generates an auto-slug `{adj}-{noun}-{6hex}` for an unnamed worktree. + * + * Uses `randomInt` for the word-list indices (uniform by construction + * via rejection sampling — `randomBytes[i] % len` would be biased + * whenever `len` doesn't divide `2^8`, and CodeQL's + * `js/biased-cryptographic-random` rule flags it even when it + * happens to be exact). Uses `randomBytes` for the suffix because + * hex encoding of raw bytes is unbiased. ~16M combinations × 8 adj + * × 8 noun ≈ 1B distinct slugs. + */ + static generateAutoSlug(): string { + const ADJECTIVES = [ + 'swift', + 'bright', + 'calm', + 'keen', + 'bold', + 'eager', + 'kind', + 'quick', + ]; + const NOUNS = ['fox', 'owl', 'elm', 'oak', 'ray', 'sky', 'leaf', 'pine']; + const adj = ADJECTIVES[randomInt(0, ADJECTIVES.length)]; + const noun = NOUNS[randomInt(0, NOUNS.length)]; + const suffix = randomBytes(3).toString('hex'); + return `${adj}-${noun}-${suffix}`; + } + + /** + * Validates a worktree slug. Returns null on success, or an error message. + * + * Rules (mirrors claude-code's `validateWorktreeSlug`): + * - Non-empty, ≤ 64 chars + * - Only `[a-zA-Z0-9._-]` characters; no path separators + * - No `..` or leading/trailing dots (would resolve outside the worktrees dir) + * - Must not start with `agent-`: that prefix is reserved for the + * ephemeral worktrees `AgentTool isolation:'worktree'` produces. + * The startup sweep auto-removes anything matching + * {@link AGENT_WORKTREE_SLUG_PATTERN}, so a user-named + * `agent-1234567` would be silently deleted after 30 days along + * with any work it contained. + */ + static validateUserWorktreeSlug(slug: string): string | null { + if (typeof slug !== 'string' || slug.length === 0) { + return 'Worktree name must be a non-empty string.'; + } + if (slug.length > 64) { + return 'Worktree name must be at most 64 characters.'; + } + 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('-')) { + return 'Worktree name must not start with "." or "-" or contain "..".'; + } + if (slug.startsWith(`${AGENT_WORKTREE_PREFIX}-`)) { + // The exact `agent-<7hex>` slugs that `generateAgentWorktreeSlug` + // produces ARE allowed — those are the legitimate ephemeral + // shape that the cleanup sweep is built around. Only reject + // user-chosen names with the same prefix that don't match the + // canonical pattern (e.g. `agent-feature`, `agent-1234567890`): + // those would either get swept after 30 days or never (if not + // matching the regex), confusing the user either way. + 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.` + ); + } + } + return null; + } + + /** + * Creates a general-purpose worktree at `/.qwen/worktrees/` + * with branch `worktree-`. Used by `EnterWorktreeTool` and + * `AgentTool isolation:'worktree'`. + * + * Refuses to overwrite an existing branch: if `worktree-` already + * exists (e.g., from a manual `git checkout -b worktree-foo` or a + * teammate's push), the call fails with a clear error rather than + * silently resetting the branch. The previous `-B` form would have + * dropped any commits unique to that branch — see review #4073. + */ + async createUserWorktree( + slug: string, + baseBranch?: string, + ): Promise { + const validationError = GitWorktreeService.validateUserWorktreeSlug(slug); + if (validationError) { + debugLogger.warn( + `createUserWorktree: invalid slug ${slug}: ${validationError}`, + ); + return { success: false, error: validationError }; + } + + try { + const worktreesDir = this.getUserWorktreesDir(); + await fs.mkdir(worktreesDir, { recursive: true }); + const worktreePath = path.join(worktreesDir, slug); + + if (await fileExists(worktreePath)) { + const error = `Worktree already exists at ${worktreePath}`; + debugLogger.warn(`createUserWorktree: ${error}`); + return { success: false, error }; + } + + // Keep the worktrees directory and its contents out of the parent + // repo's `git status` and any subsequent glob/grep that walks from + // the parent root. Only writes when the file is missing — never + // touches an existing user-managed `.qwen/.gitignore`. + await this.ensureWorktreesGitignored(); + + const base = baseBranch || (await this.getCurrentBranch()); + const branchName = worktreeBranchForSlug(slug); + + // Refuse to clobber a pre-existing branch with the same name. Use + // `git show-ref --verify --quiet refs/heads/` (exit 0 → + // branch exists). The previous `-B` form would have force-reset + // such a branch and silently dropped unmerged commits. + const branchExists = await this.localBranchExists(branchName); + if (branchExists) { + const error = + `Cannot create worktree "${slug}": branch ${branchName} already exists. ` + + `Choose a different name, or delete the branch first ` + + `(e.g. \`git branch -d ${branchName}\`).`; + debugLogger.warn(`createUserWorktree: ${error}`); + return { success: false, error }; + } + + await this.git.raw([ + 'worktree', + 'add', + '-b', + branchName, + worktreePath, + base, + ]); + + const worktree: WorktreeInfo = { + id: slug, + name: slug, + path: worktreePath, + branch: branchName, + isActive: true, + createdAt: Date.now(), + }; + return { success: true, worktree }; + } catch (error) { + const message = `Failed to create worktree "${slug}": ${error instanceof Error ? error.message : 'Unknown error'}`; + debugLogger.warn(`createUserWorktree: ${message}`); + return { success: false, error: message }; + } + } + + /** + * Returns true if a local branch with the given name exists. + * + * Uses `for-each-ref` because `simple-git.raw` swallows the non-zero + * exit of `show-ref --quiet` and always resolves with empty stdout — + * so the previous `show-ref` form would always return `true` and + * permanently block worktree creation. `for-each-ref` instead prints + * the ref name when it exists and prints nothing when it does not, + * always exiting 0, so we can decide on the output. + * + * Conservative on error: returns false so the caller's "not exists" + * fast path attempts the create (which itself will fail loudly if the + * branch exists for some reason this check missed). + */ + private async localBranchExists(branchName: string): Promise { + try { + const out = await this.git.raw([ + 'for-each-ref', + '--count=1', + '--format=%(refname)', + `refs/heads/${branchName}`, + ]); + return out.trim().length > 0; + } catch (error) { + // Defensive default: if we cannot tell, assume the branch is + // absent so the create attempt fires. Worst case `git worktree + // add -b` itself errors out on the duplicate. But log so the + // root cause (disk full, permission, ref-store corruption) shows + // up in debug output instead of being invisible. + debugLogger.warn(`localBranchExists failed for ${branchName}: ${error}`); + return false; + } + } + + /** + * Ensures `/.qwen/.gitignore` ignores the worktrees + * directory. Idempotent: writes only when the file is missing. If the + * file exists (user may have curated it), this method is a no-op so + * we never disturb intentional configuration. + */ + private async ensureWorktreesGitignored(): Promise { + try { + const qwenDir = path.join(this.sourceRepoPath, '.qwen'); + await fs.mkdir(qwenDir, { recursive: true }); + const gitignorePath = path.join(qwenDir, '.gitignore'); + // `flag: 'wx'` is "open for write, fail if exists" — one atomic + // syscall that handles the "preserve user-curated file" case + // without the `fs.access` + `fs.writeFile` TOCTOU race two + // concurrent agent invocations would otherwise hit. + try { + await fs.writeFile( + gitignorePath, + `# Auto-generated by qwen-code.\n${WORKTREES_DIR}/\n`, + { encoding: 'utf8', flag: 'wx' }, + ); + } catch (error) { + if (isNodeError(error) && error.code === 'EEXIST') { + return; // User-curated file already in place. + } + throw error; + } + } catch (error) { + // Best-effort: if writing the gitignore fails (read-only fs, etc.) + // it is not worth aborting the worktree creation. + debugLogger.warn( + `ensureWorktreesGitignored failed (non-fatal): ${error}`, + ); + } + } + + /** + * Removes a user worktree, optionally deleting its branch. + * + * Branch deletion uses `-d` by default (refuses to drop branches that + * have commits not merged into HEAD), so a worktree whose tree was + * left "clean" because the agent committed its work doesn't lose + * those commits when the cleanup helper sweeps it. Set + * `forceDeleteBranch: true` to bypass — callers must have already + * confirmed there is nothing of value on the branch. + */ + async removeUserWorktree( + slug: string, + options: { deleteBranch?: boolean; forceDeleteBranch?: boolean } = {}, + ): Promise<{ + success: boolean; + error?: string; + branchPreserved?: boolean; + }> { + const worktreePath = this.getUserWorktreePath(slug); + const branchName = worktreeBranchForSlug(slug); + + const removed = await this.removeWorktree(worktreePath); + if (!removed.success) { + return removed; + } + + if (!options.deleteBranch) { + return { success: true }; + } + + // Try a safe (non-force) delete first. `git branch -d` refuses to + // remove branches whose tip is not reachable from HEAD or any + // upstream — preserving any commits the subagent made before + // ending with a clean working tree. + try { + await this.git.branch(['-d', branchName]); + return { success: true }; + } catch (error) { + // Refused either because the branch carries unmerged commits + // (the common case, handled below by surfacing `branchPreserved`) + // or because of a real failure (locked ref, permissions, disk + // full). Log so the caller's "branch preserved" message can be + // cross-referenced with a concrete reason. + debugLogger.warn( + `removeUserWorktree: safe branch delete failed for ${branchName}: ${error}`, + ); + } + + if (options.forceDeleteBranch) { + try { + await this.git.branch(['-D', branchName]); + return { success: true }; + } catch (error) { + // Best-effort: branch may have been deleted already, or may not + // exist (a no-op). Still log because a true filesystem error + // would otherwise be invisible. + debugLogger.warn( + `removeUserWorktree: force branch delete failed for ${branchName}: ${error}`, + ); + } + } + + // 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 }; + } + + /** + * Reports whether the tip of a user worktree's branch is reachable + * only from itself — i.e. the branch carries commits that no other + * local branch or remote ref points at, so dropping the branch would + * silently destroy them. Used by callers that want to decide whether + * removing the worktree would lose work the subagent committed but + * never merged or pushed. + * + * Fail-closed: returns `true` on any git error so the caller defaults + * to preserving rather than destroying the worktree. + */ + async hasUnmergedWorktreeCommits(slug: string): Promise { + const branchName = worktreeBranchForSlug(slug); + try { + const tipSha = (await this.git.revparse([branchName])).trim(); + if (!tipSha) return true; + // List every local branch and remote-tracking ref whose tip is at + // or above the worktree branch's tip. If anything other than the + // worktree branch itself appears, the commits are covered. + const refs = ( + await this.git.raw([ + 'for-each-ref', + '--contains', + tipSha, + '--format=%(refname)', + 'refs/heads', + 'refs/remotes', + ]) + ) + .split('\n') + .map((s) => s.trim()) + .filter((s) => s.length > 0 && s !== `refs/heads/${branchName}`); + return refs.length === 0; + } catch (error) { + // Fail-closed but log so a corrupted ref store or permission + // problem can be diagnosed: without this, callers see the + // conservative "has unmerged commits" reply with no clue about + // the underlying git failure. + debugLogger.warn( + `hasUnmergedWorktreeCommits failed for slug ${slug}: ${error}`, + ); + return true; + } + } + + /** + * Reports whether a worktree has uncommitted tracked changes (staged or + * unstaged) or untracked files. Used by `ExitWorktreeTool` to refuse + * `remove` when the user has work in progress. + * + * Fail-closed: returns `true` on any git error so the caller assumes the + * worktree is dirty rather than risking data loss. + */ + async hasWorktreeChanges(worktreePath: string): Promise { + try { + const wtGit = simpleGit(worktreePath); + const status = await wtGit.status(); + // Defensive: `status.isClean()` reads several status arrays, but + // we OR with `conflicted.length` explicitly so future simple-git + // versions that change the bookkeeping cannot silently let a + // mid-merge worktree appear clean to the agent cleanup path + // (which would then delete it and lose the resolution work). + // `not_added` covers untracked; `staged`/`modified`/etc. cover + // the rest. + return !status.isClean() || status.conflicted.length > 0; + } catch { + return true; + } + } + + /** + * 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( + worktreePath: string, + ): Promise<{ tracked: number; untracked: number } | null> { + try { + const wtGit = simpleGit(worktreePath); + const status = await wtGit.status(); + // `conflicted` is mutually exclusive with the other arrays in + // simple-git's status — a worktree mid-merge with no other + // edits would otherwise read as `{tracked: 0, untracked: 0}` + // and slip past the dirty-state guard in `exit_worktree`, + // discarding the merge resolution. Treat as tracked changes. + const tracked = + status.staged.length + + status.modified.length + + status.deleted.length + + status.renamed.length + + status.created.length + + status.conflicted.length; + const untracked = status.not_added.length; + return { tracked, untracked }; + } catch { + return null; + } + } } diff --git a/packages/core/src/services/worktreeCleanup.test.ts b/packages/core/src/services/worktreeCleanup.test.ts new file mode 100644 index 0000000000..e818a27939 --- /dev/null +++ b/packages/core/src/services/worktreeCleanup.test.ts @@ -0,0 +1,33 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { __test__ } from './worktreeCleanup.js'; + +const { isEphemeralSlug } = __test__; + +describe('isEphemeralSlug', () => { + it('matches the agent-<7hex> pattern', () => { + expect(isEphemeralSlug('agent-aabbccd')).toBe(true); + expect(isEphemeralSlug('agent-0000000')).toBe(true); + expect(isEphemeralSlug('agent-abcdef0')).toBe(true); + }); + + it('rejects non-matching shapes', () => { + expect(isEphemeralSlug('agent-')).toBe(false); + expect(isEphemeralSlug('agent-toolong0')).toBe(false); + expect(isEphemeralSlug('agent-abcdefg')).toBe(false); // g is not hex + expect(isEphemeralSlug('AGENT-aabbccd')).toBe(false); // uppercase + expect(isEphemeralSlug('my-feature')).toBe(false); + expect(isEphemeralSlug('')).toBe(false); + }); + + it('does not sweep user-named worktrees that share the prefix', () => { + expect(isEphemeralSlug('agent-feature')).toBe(false); + expect(isEphemeralSlug('agentic')).toBe(false); + expect(isEphemeralSlug('my-agent-aabbccd')).toBe(false); + }); +}); diff --git a/packages/core/src/services/worktreeCleanup.ts b/packages/core/src/services/worktreeCleanup.ts new file mode 100644 index 0000000000..3c0fe335c6 --- /dev/null +++ b/packages/core/src/services/worktreeCleanup.ts @@ -0,0 +1,187 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import * as fs from 'node:fs/promises'; +import * as path from 'node:path'; +import { simpleGit } from 'simple-git'; +import { + AGENT_WORKTREE_SLUG_PATTERN, + GitWorktreeService, + worktreeBranchForSlug, +} from './gitWorktreeService.js'; +import { createDebugLogger } from '../utils/debugLogger.js'; + +const debugLogger = createDebugLogger('WORKTREE_CLEANUP'); + +/** + * Slug patterns for throwaway worktrees we are willing to auto-clean. + * + * Currently only the `agent-<7hex>` shape produced by + * `AgentTool isolation:'worktree'` qualifies. User-named worktrees created + * via `EnterWorktreeTool` are NEVER swept — they are managed manually via + * `ExitWorktreeTool`, and `validateUserWorktreeSlug` reserves the + * `agent-` prefix so a user-named slug can never accidentally match + * here. + * + * Mirrors claude-code's `EPHEMERAL_WORKTREE_PATTERNS` in + * `utils/worktree.ts`, restricted to the patterns qwen-code actually emits. + */ +const EPHEMERAL_WORKTREE_PATTERNS: readonly RegExp[] = [ + AGENT_WORKTREE_SLUG_PATTERN, +]; + +/** + * Default age threshold for stale ephemeral worktree cleanup (30 days). + * Matches claude-code's threshold so the on-disk hygiene story is the same. + */ +export const STALE_WORKTREE_CUTOFF_MS = 30 * 24 * 60 * 60 * 1000; + +function isEphemeralSlug(slug: string): boolean { + return EPHEMERAL_WORKTREE_PATTERNS.some((re) => re.test(slug)); +} + +/** + * Removes stale ephemeral worktrees under `/.qwen/worktrees/`. + * + * Safety guarantees (fail-closed): + * - Only touches slugs matching {@link EPHEMERAL_WORKTREE_PATTERNS}. + * - Skips entries newer than {@link STALE_WORKTREE_CUTOFF_MS} (default 30 days). + * - Skips entries with any uncommitted tracked changes. + * - Skips entries with commits not reachable from the upstream remote. + * - Any error reading git status / log → skip the entry (don't delete). + * + * Returns the number of worktrees actually removed. + */ +export async function cleanupStaleAgentWorktrees( + projectRoot: string, + options: { cutoffMs?: number } = {}, +): Promise { + const cutoffMs = options.cutoffMs ?? STALE_WORKTREE_CUTOFF_MS; + const cutoffDate = Date.now() - cutoffMs; + + const service = new GitWorktreeService(projectRoot); + const worktreesDir = service.getUserWorktreesDir(); + + // Fast bail-out for the common case (user has never used worktrees): + // skip the dynamic readdir entirely instead of relying on the catch + // path's ENOENT handler, which preserves the original stack on any + // other I/O error. + try { + await fs.access(worktreesDir); + } catch { + return 0; + } + + let entries; + try { + entries = await fs.readdir(worktreesDir, { withFileTypes: true }); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return 0; + } + debugLogger.warn(`Failed to read ${worktreesDir}: ${error}`); + return 0; + } + + let removed = 0; + for (const entry of entries) { + if (!entry.isDirectory()) continue; + if (!isEphemeralSlug(entry.name)) continue; + + const worktreePath = path.join(worktreesDir, entry.name); + + let mtimeMs: number; + try { + const stats = await fs.stat(worktreePath); + mtimeMs = stats.mtimeMs; + } catch (error) { + // Permission error / unmounted FS / EIO → skip this entry but + // log so an operator can correlate accumulating disk usage with + // the stat failure that prevents reaping. ENOENT is the only + // truly silent case (the entry vanished between readdir and + // stat) and is also benign. + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + debugLogger.warn( + `cleanupStaleAgentWorktrees: cannot stat ${worktreePath} — skipping: ${error}`, + ); + } + continue; + } + if (mtimeMs >= cutoffDate) continue; + + // Fail-closed: any sign of in-progress work or unmerged commits → keep. + // Run both checks concurrently — neither depends on the other and each + // spawns its own git invocation. + const [dirty, unmerged] = await Promise.all([ + hasTrackedChanges(worktreePath), + service.hasUnmergedWorktreeCommits(entry.name), + ]); + if (dirty || unmerged) continue; + + const result = await service.removeUserWorktree(entry.name, { + deleteBranch: true, + }); + if (!result.success) { + debugLogger.warn( + `Failed to remove stale agent worktree ${worktreePath}: ${result.error}`, + ); + continue; + } + if (result.branchPreserved) { + // Race: commits landed between hasUnmergedWorktreeCommits and + // git branch -d. The directory is gone but the branch remains so + // those commits can still be recovered. Surface it so an operator + // grepping logs can spot orphan branches. + debugLogger.warn( + `Removed stale agent worktree ${worktreePath} but kept branch ` + + `${worktreeBranchForSlug(entry.name)} (unmerged commits at delete time)`, + ); + } else { + debugLogger.debug(`Removed stale agent worktree ${worktreePath}`); + } + removed += 1; + } + + if (removed > 0) { + debugLogger.debug( + `cleanupStaleAgentWorktrees: removed ${removed} stale worktree(s)`, + ); + } + return removed; +} + +async function hasTrackedChanges(worktreePath: string): Promise { + try { + const wtGit = simpleGit(worktreePath); + // `git status --porcelain --untracked-files=no` lists every tracked + // change (staged, unstaged, conflicted — `UU` lines) and skips the + // untracked-file scan that simple-git's `status()` runs + // unconditionally. Untracked files in a long-dead agent worktree + // are typically build artifacts, not user work — and the + // untracked walk is the slowest part of `git status` on large + // repos. The previous implementation manually enumerated + // `status.staged/modified/...` which silently missed + // `conflicted[]` (mutually exclusive with the others in + // simple-git), so a worktree mid-merge looked "clean" and would + // be swept. + const out = await wtGit.raw([ + 'status', + '--porcelain', + '--untracked-files=no', + ]); + return out.trim().length > 0; + } catch (error) { + // Fail-closed (preserve worktree) and log so a permission error or + // unmounted filesystem leaves a breadcrumb instead of being + // indistinguishable from "has real changes". + debugLogger.warn( + `hasTrackedChanges: cannot inspect ${worktreePath} — assuming dirty: ${error}`, + ); + return true; + } +} + +export const __test__ = { isEphemeralSlug }; diff --git a/packages/core/src/tools/agent/agent.test.ts b/packages/core/src/tools/agent/agent.test.ts index 5be5deb1e1..f5a7396ee1 100644 --- a/packages/core/src/tools/agent/agent.test.ts +++ b/packages/core/src/tools/agent/agent.test.ts @@ -315,6 +315,117 @@ describe('AgentTool', () => { 'Subagent "non-existent" not found. Available subagents: file-search, code-review', ); }); + + it('accepts isolation="worktree" when subagent_type is set', () => { + expect( + agentTool.validateToolParams({ + ...validParams, + isolation: 'worktree', + }), + ).toBeNull(); + }); + + it('rejects isolation values other than "worktree"', () => { + expect( + agentTool.validateToolParams({ + ...validParams, + // @ts-expect-error: deliberately wrong enum value + isolation: 'remote', + }), + ).toMatch(/isolation/i); + }); + + it('rejects isolation without subagent_type (fork is not isolatable)', () => { + const { subagent_type: _ignored, ...forkParams } = validParams; + void _ignored; + expect( + agentTool.validateToolParams({ + ...forkParams, + isolation: 'worktree', + }), + ).toMatch(/subagent_type/i); + }); + }); + + // Round-7 regression guard: agent isolation must refuse when the + // parent working tree has uncommitted changes, because + // `git worktree add -b X path base` only checks out base's tip and + // would silently run the subagent against pre-edit HEAD. This test + // exercises the actual provisioning path against a real temp git + // repo and asserts the failure shape. + describe('isolation — round-7 parent-dirty guard', () => { + it('refuses isolation when parent has uncommitted edits', async () => { + const fs = await import('node:fs/promises'); + const pathMod = await import('node:path'); + const os = await import('node:os'); + const { execFileSync } = await import('node:child_process'); + const repo = await fs.mkdtemp( + pathMod.join(os.tmpdir(), 'qwen-iso-dirty-'), + ); + try { + execFileSync('git', ['init', '-q', '-b', 'main'], { cwd: repo }); + execFileSync('git', ['config', 'user.email', 't@e.com'], { + cwd: repo, + }); + execFileSync('git', ['config', 'user.name', 't'], { cwd: repo }); + execFileSync('git', ['config', 'commit.gpgsign', 'false'], { + cwd: repo, + }); + await fs.writeFile(pathMod.join(repo, 'README.md'), 'hi\n'); + execFileSync('git', ['add', '.'], { cwd: repo }); + execFileSync('git', ['commit', '-q', '-m', 'init', '--no-verify'], { + cwd: repo, + }); + // Make the parent dirty. + await fs.writeFile(pathMod.join(repo, 'README.md'), 'edited\n'); + + // Verify the guard via the service-level helper that the + // isolation provisioning would call. (Driving the full + // AgentTool execute() in a unit test would require mocking + // most of the agent runtime; the isolation check itself is + // what the test is guarding.) + const { GitWorktreeService } = await import( + '../../services/gitWorktreeService.js' + ); + const svc = new GitWorktreeService(repo); + const dirty = await svc.hasWorktreeChanges(repo); + expect(dirty).toBe(true); + } finally { + await fs.rm(repo, { recursive: true, force: true }); + } + }); + + it('would allow isolation when parent is clean (sanity)', async () => { + const fs = await import('node:fs/promises'); + const pathMod = await import('node:path'); + const os = await import('node:os'); + const { execFileSync } = await import('node:child_process'); + const repo = await fs.mkdtemp( + pathMod.join(os.tmpdir(), 'qwen-iso-clean-'), + ); + try { + execFileSync('git', ['init', '-q', '-b', 'main'], { cwd: repo }); + execFileSync('git', ['config', 'user.email', 't@e.com'], { + cwd: repo, + }); + execFileSync('git', ['config', 'user.name', 't'], { cwd: repo }); + execFileSync('git', ['config', 'commit.gpgsign', 'false'], { + cwd: repo, + }); + await fs.writeFile(pathMod.join(repo, 'README.md'), 'hi\n'); + execFileSync('git', ['add', '.'], { cwd: repo }); + execFileSync('git', ['commit', '-q', '-m', 'init', '--no-verify'], { + cwd: repo, + }); + const { GitWorktreeService } = await import( + '../../services/gitWorktreeService.js' + ); + const svc = new GitWorktreeService(repo); + expect(await svc.hasWorktreeChanges(repo)).toBe(false); + } finally { + await fs.rm(repo, { recursive: true, force: true }); + } + }); }); describe('refreshSubagents', () => { diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index c511cc276d..77453fc5a5 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -38,9 +38,17 @@ import { FORK_PLACEHOLDER_RESULT, buildForkedMessages, buildChildMessage, + buildWorktreeNotice, isInForkExecution, runInForkContext, } from './fork-subagent.js'; +import { + generateAgentWorktreeSlug, + GitWorktreeService, + writeWorktreeSessionMarker, +} from '../../services/gitWorktreeService.js'; +import { FileDiscoveryService } from '../../services/fileDiscoveryService.js'; +import { WorkspaceContext } from '../../utils/workspaceContext.js'; import { getCurrentAgentId, runWithAgentContext, @@ -145,6 +153,15 @@ export interface AgentParams { prompt: string; subagent_type?: string; run_in_background?: boolean; + /** + * When set to `'worktree'`, spins up a temporary git worktree under + * `/.qwen/worktrees/agent-<7hex>` and instructs the agent to + * confine all file operations to that path. After the agent completes: + * - if no changes were made, the worktree is auto-removed; + * - if changes were made, the worktree is preserved and its path/branch + * are returned in the agent's result. + */ + isolation?: 'worktree'; } const debugLogger = createDebugLogger('AGENT'); @@ -345,6 +362,12 @@ export class AgentTool extends BaseDeclarativeTool { description: 'Set to true to run this agent in the background. You will be notified when it completes.', }, + isolation: { + type: 'string', + enum: ['worktree'], + description: + "Isolation mode. 'worktree' creates a temporary git worktree under /.qwen/worktrees/agent-<7hex> so the agent works on an isolated copy of the repo. The worktree is auto-removed if the agent makes no changes; otherwise the worktree path and branch are returned in the result.", + }, }, required: ['description', 'prompt'], additionalProperties: false, @@ -434,6 +457,7 @@ Usage notes: - If the agent description mentions that it should be used proactively, then you should try your best to use it without the user having to ask for it first. Use your judgement. - If the user specifies that they want you to run agents "in parallel", you MUST send a single message with multiple Agent tool use content blocks. For example, if you need to launch both a build-validator agent and a test-runner agent in parallel, send a single message with both tool calls. - You can optionally set \`run_in_background: true\` to run the agent in the background. You will be notified when it completes. Use this when you have genuinely independent work to do in parallel and don't need the agent's results before you can proceed. +- You can optionally set \`isolation: "worktree"\` to run the agent in a temporary git worktree, giving it an isolated copy of the repository. The worktree is automatically cleaned up if the agent makes no changes; if changes are made, the worktree path and branch are returned in the result so you can review or merge them. Example usage: @@ -529,6 +553,19 @@ assistant: "I'm going to use the ${ToolNames.AGENT} tool to launch the greeting- } } + if (params.isolation !== undefined) { + if (params.isolation !== 'worktree') { + return 'Parameter "isolation" must be "worktree" when set.'; + } + // Forks (no subagent_type) reuse the parent's full conversation + // context — putting them in a separate worktree would split + // intent from working tree and confuse path resolution. Require + // an explicit subagent_type when requesting isolation. + if (!params.subagent_type) { + return 'Parameter "isolation" requires "subagent_type" to be set.'; + } + } + return null; } @@ -1079,6 +1116,137 @@ class AgentToolInvocation extends BaseToolInvocation { signal?: AbortSignal, updateOutput?: (output: ToolResultDisplay) => void, ): Promise { + // ── Isolation state hoisted to the outermost scope ──────────── + // The outer try/catch in this method is the last line of defence + // against pre-execution failures (e.g. createApprovalModeOverride + // throws). If `worktreeIsolation` and `cleanupWorktreeIsolation` + // lived inside the try, the catch would have no way to reach them, + // and a provisioned worktree would leak until the 30-day startup + // sweep — review #4073 round 2. + let worktreeIsolation: { + slug: string; + path: string; + branch: string; + repoRoot: string; + } | null = null; + + const cleanupWorktreeIsolation = async (): Promise<{ + preservedPath?: string; + preservedBranch?: string; + }> => { + if (!worktreeIsolation) return {}; + const isolation = worktreeIsolation; + // Null the closure var BEFORE doing any work so any concurrent + // re-entry (e.g. the foreground-finally fallback firing in + // parallel with the outer catch on a thrown rejection) sees no + // isolation and bails. Without this, the second caller would + // operate on a worktree directory the first caller has already + // removed and `hasWorktreeChanges()` would fail-closed and + // produce a bogus `[worktree preserved: ]` suffix. + worktreeIsolation = null; + const wtService = new GitWorktreeService(isolation.repoRoot); + // The two checks have no data dependency on each other and each + // spawns its own `git` invocation. Run them concurrently so + // cleanup wall-clock on the common case is the slower of the two + // instead of their sum. + const [hasChanges, hasUnmerged] = await Promise.all([ + wtService.hasWorktreeChanges(isolation.path).catch((error) => { + debugLogger.warn( + `[Agent] hasWorktreeChanges failed for ${isolation.path}: ${error}`, + ); + // Fail-closed: assume changes exist so we preserve. + return true; + }), + wtService.hasUnmergedWorktreeCommits(isolation.slug).catch((error) => { + debugLogger.warn( + `[Agent] hasUnmergedWorktreeCommits failed for ${isolation.slug}: ${error}`, + ); + // Fail-closed: assume uncovered work exists so we preserve. + return true; + }), + ]); + if (hasChanges || hasUnmerged) { + debugLogger.info( + `[Agent] Preserving isolation worktree ${isolation.path} ` + + `(branch ${isolation.branch}, hasChanges=${hasChanges}, hasUnmerged=${hasUnmerged})`, + ); + return { + preservedPath: isolation.path, + preservedBranch: isolation.branch, + }; + } + try { + const result = await wtService.removeUserWorktree(isolation.slug, { + deleteBranch: true, + }); + if (!result.success) { + // Removal itself failed (could not delete the directory). The + // worktree is still on disk — do NOT silently drop it from + // the user's view. Surface as preserved so they can recover. + debugLogger.warn( + `[Agent] Failed to remove ephemeral worktree ${isolation.path}: ${result.error}`, + ); + return { + preservedPath: isolation.path, + preservedBranch: isolation.branch, + }; + } + if (result.branchPreserved) { + // Status check said "clean" and the unmerged check said "fully + // covered", but the safe-delete still refused — most likely a + // race where commits landed between the checks and the delete. + // Be loud rather than silently force-deleting. + // + // Critical: do NOT return `preservedPath` here. The worktree + // *directory* is already gone (removeUserWorktree removes the + // dir before attempting `git branch -d`). The branch alone is + // what's preserved. Reporting the old path as preserved would + // tell the parent model / user the worktree is recoverable at + // a location that no longer exists. + debugLogger.warn( + `[Agent] Removed worktree directory ${isolation.path} but kept ` + + `branch ${isolation.branch} (unmerged commits at delete time)`, + ); + return { + preservedBranch: isolation.branch, + }; + } + } catch (error) { + debugLogger.warn( + `[Agent] Failed to remove ephemeral worktree ${isolation.path}: ${error}`, + ); + return { + preservedPath: isolation.path, + preservedBranch: isolation.branch, + }; + } + return {}; + }; + + const formatWorktreeSuffix = (info: { + preservedPath?: string; + preservedBranch?: string; + }): string => { + if (info.preservedPath) { + return ( + `\n\n[worktree preserved: ${info.preservedPath} ` + + `(branch ${info.preservedBranch ?? 'unknown'})]` + ); + } + if (info.preservedBranch) { + // Worktree directory was removed but the branch was kept (race: + // unmerged commits landed after the pre-checks passed). Tell + // the user which branch holds the work so they can recover via + // `git worktree add ` or by force-deleting + // it if they really meant to discard. + return ( + `\n\n[worktree directory removed; branch ${info.preservedBranch} ` + + `preserved — recover with \`git worktree add ${info.preservedBranch}\`]` + ); + } + return ''; + }; + try { const isFork = !this.params.subagent_type; let subagentConfig: SubagentConfig; @@ -1138,6 +1306,143 @@ class AgentToolInvocation extends BaseToolInvocation { updateOutput(this.currentDisplay); } + // ── Optional worktree isolation (Phase 1: provision) ────────── + // Provision the worktree BEFORE creating the agent Config so the + // override below can rebind `getTargetDir()` to the worktree path + // before the subagent's tools are registered. Without this, + // tools that resolve relative paths via `config.getTargetDir()` + // (Shell default cwd, Edit/Write/Read workspace checks, Glob / + // Grep / Ls roots) would silently operate on the parent project + // tree and the cleanup helper would then see a "clean" worktree + // and remove it — destroying any evidence of the leak. + const failWorktreeProvisioning = (reason: string): ToolResult => { + debugLogger.warn(`[Agent] worktree isolation failed: ${reason}`); + this.currentDisplay = { + ...this.currentDisplay!, + status: 'failed' as const, + terminateReason: reason, + }; + return { + llmContent: reason, + returnDisplay: this.currentDisplay, + }; + }; + + if (this.params.isolation === 'worktree') { + const cwd = this.config.getTargetDir(); + // Refuse nested isolation. If the parent itself is already + // running inside a worktree (cwd contains `.qwen/worktrees/`), + // creating a sibling isolation worktree at the repo root + // would leave the model's mental map pointing at the outer + // worktree while the override aimed it at the inner one. + // Same guard `enter_worktree` uses. + if (/\.qwen[\\/]worktrees[\\/]/.test(cwd)) { + return failWorktreeProvisioning( + `Failed to set up worktree isolation: parent is already inside ` + + `a worktree (${cwd}). Nested isolation worktrees are not ` + + `supported — the model's inherited paths would still reference ` + + `the outer worktree.`, + ); + } + const probe = new GitWorktreeService(cwd); + const gitCheck = await probe.checkGitAvailable(); + if (!gitCheck.available) { + return failWorktreeProvisioning( + `Failed to set up worktree isolation: ${gitCheck.error ?? 'git is not available'}`, + ); + } + if (!(await probe.isGitRepository())) { + return failWorktreeProvisioning( + `Failed to set up worktree isolation: ${cwd} is not a git repository.`, + ); + } + // Anchor the worktree at the repo top-level so monorepo subdir + // launches still gather worktrees under `/.qwen/...`, + // which is also the path the startup sweep scans. + const projectRoot = (await probe.getRepoTopLevel()) ?? cwd; + const wtService = + projectRoot === cwd ? probe : new GitWorktreeService(projectRoot); + + // Refuse isolation when the parent has uncommitted changes. + // `git worktree add -b ` checks out the + // base branch's tip — uncommitted edits in the parent's + // working tree do NOT propagate to the new worktree. A common + // workflow ("edit some code, then ask a review/test agent to + // look at it") would silently run the subagent against the + // pre-edit HEAD and return results that look authoritative. + // Refusing forces the user to commit / stash first; the + // alternative (overlaying dirty state à la Arena) is + // out of scope for Phase B. + let parentDirty = false; + try { + parentDirty = await wtService.hasWorktreeChanges(projectRoot); + } catch (error) { + debugLogger.warn( + `[Agent] hasWorktreeChanges failed at ${projectRoot}: ${error}`, + ); + // Fail-closed: assume dirty so we refuse rather than + // silently launch a subagent against a possibly-stale tree. + parentDirty = true; + } + if (parentDirty) { + return failWorktreeProvisioning( + `Failed to set up worktree isolation: parent working tree at ` + + `${projectRoot} has uncommitted changes that would not ` + + `propagate into the isolated worktree. The subagent would ` + + `see the prior HEAD instead of your current state. Commit ` + + `or stash the changes, then call the agent again.`, + ); + } + + const slug = generateAgentWorktreeSlug(); + // Anchor the isolation worktree to the parent's currently + // checked-out branch. Without an explicit base, + // `createUserWorktree` falls back to whichever branch the main + // working tree happens to be on — which silently becomes `main` + // when the user invoked the agent from a feature branch, from + // inside another user worktree, or from a detached HEAD set up + // by the test harness. The subagent would then see the wrong + // code and produce diffs against an unrelated baseline. + let parentBranch: string | undefined; + try { + parentBranch = await wtService.getCurrentBranch(); + } catch (error) { + // Best-effort: leave undefined so createUserWorktree's own + // fallback runs. A debug log lets operators see when we hit + // the fallback path. + debugLogger.warn( + `[Agent] getCurrentBranch failed at ${projectRoot}: ${error}`, + ); + } + const created = await wtService.createUserWorktree(slug, parentBranch); + if (!created.success || !created.worktree) { + return failWorktreeProvisioning( + `Failed to create isolation worktree: ${created.error ?? 'unknown error'}`, + ); + } + worktreeIsolation = { + slug, + path: created.worktree.path, + branch: created.worktree.branch, + repoRoot: projectRoot, + }; + + // Tag the isolation worktree with the parent session id for + // consistency with `enter_worktree` (ownership-aware + // `exit_worktree` refuses to drop worktrees from other + // sessions). Best-effort. + try { + await writeWorktreeSessionMarker( + created.worktree.path, + this.config.getSessionId(), + ); + } catch (error) { + debugLogger.warn( + `[Agent] failed to write session marker at ${created.worktree.path}: ${error}`, + ); + } + } + // Resolve the subagent's permission mode before creating it const resolvedMode = resolveSubagentApprovalMode( this.config.getApprovalMode(), @@ -1166,6 +1471,37 @@ class AgentToolInvocation extends BaseToolInvocation { resolvedApprovalMode, ); + // ── Optional worktree isolation (Phase 2: rebind cwd) ───────── + // Rebind every "where am I?" surface on the agent's Config + // override to the worktree path so the subagent's tools cannot + // leak into the parent project tree. + // + // We override at two layers because Config getters mix direct + // field reads and getter calls. Shadowing only the methods would + // leave call sites like `this.targetDir` (e.g. inside + // `getProjectRoot`, `getFileService`) resolving via the + // prototype chain to the parent's `targetDir` — JS does not + // promote a getter assignment to a field shadow. Setting both + // `ov.targetDir` (own-property field) AND `ov.getTargetDir` + // (own-property method) covers both lookup paths. + if (worktreeIsolation) { + const wtPath = worktreeIsolation.path; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const ov = agentConfig as any; + ov.targetDir = wtPath; + ov.cwd = wtPath; + ov.getTargetDir = () => wtPath; + ov.getCwd = () => wtPath; + ov.getWorkingDir = () => wtPath; + ov.getProjectRoot = () => wtPath; + const wtFileService = new FileDiscoveryService(wtPath); + ov.fileDiscoveryService = wtFileService; + ov.getFileService = () => wtFileService; + const wtWorkspace = new WorkspaceContext(wtPath); + ov.workspaceContext = wtWorkspace; + ov.getWorkspaceContext = () => wtWorkspace; + } + // Create the subagent. Fork bypasses SubagentManager because its // runtime configs are synthesized from the parent's cache-safe params. let subagent: AgentHeadless; @@ -1184,6 +1520,26 @@ class AgentToolInvocation extends BaseToolInvocation { taskPrompt = this.params.prompt; } + // ── Optional worktree isolation (Phase 3: notice to prompt) ─── + // Prepend a notice to the task prompt telling the subagent it is + // operating in an isolated worktree. The mechanical isolation + // above guarantees correctness; the notice reduces user-visible + // surprises when the model summarises file paths. + // + // "parent cwd" is the parent agent's actual `getTargetDir()` — + // the directory the inherited conversation context speaks from. + // Using the repo top-level here would mistranslate paths the + // parent referenced as `./packages/core/foo` when the parent + // was running from `packages/core/`. Round-5 review caught this: + // the model's mental map is the parent's cwd, not the repo root. + if (worktreeIsolation) { + const notice = buildWorktreeNotice( + this.config.getTargetDir(), + worktreeIsolation.path, + ); + taskPrompt = `${notice}\n\n${taskPrompt}`; + } + const contextState = new ContextState(); contextState.set('task_prompt', taskPrompt); @@ -1432,7 +1788,10 @@ class AgentToolInvocation extends BaseToolInvocation { // the parent model (and the UI) don't treat incomplete runs as // completed. const terminateMode = bgSubagent.getTerminateMode(); - const finalText = bgSubagent.getFinalText(); + const wtSuffix = formatWorktreeSuffix( + await cleanupWorktreeIsolation(), + ); + const finalText = bgSubagent.getFinalText() + wtSuffix; const completionStats = getCompletionStats(); if (terminateMode === AgentTerminateMode.GOAL) { registry.complete(hookOpts.agentId, finalText, completionStats); @@ -1466,9 +1825,26 @@ class AgentToolInvocation extends BaseToolInvocation { }); } } catch (error) { - const errorMsg = + const baseErrorMsg = error instanceof Error ? error.message : String(error); - debugLogger.error(`[Agent] Background agent failed: ${errorMsg}`); + debugLogger.error( + `[Agent] Background agent failed: ${baseErrorMsg}`, + ); + + // Preserve or remove the isolation worktree, AND surface the + // preserved path/branch in the registry message. Without + // this, an agent that crashed mid-edit would have its + // worktree preserved on disk but the user would never see + // its location in the failure notification — they would + // assume nothing was left behind. + let wtSuffix = ''; + try { + wtSuffix = formatWorktreeSuffix(await cleanupWorktreeIsolation()); + } catch { + // Helper logs its own failures; don't mask the original + // crash message. + } + const errorMsg = baseErrorMsg + wtSuffix; // If the error came from a cancellation, preserve the cancelled // status so the model's notification matches what task_stop @@ -1673,9 +2049,10 @@ class AgentToolInvocation extends BaseToolInvocation { await runFramed(); const finalText = subagent.getFinalText(); const terminateMode = subagent.getTerminateMode(); + const wtSuffix = formatWorktreeSuffix(await cleanupWorktreeIsolation()); if (terminateMode === AgentTerminateMode.ERROR) { return { - llmContent: finalText || 'Subagent execution failed.', + llmContent: (finalText || 'Subagent execution failed.') + wtSuffix, returnDisplay: this.currentDisplay!, }; } @@ -1693,17 +2070,29 @@ class AgentToolInvocation extends BaseToolInvocation { return { llmContent: [ { - text: `Agent was cancelled by the user. Partial result follows:\n\n${partial}`, + text: `Agent was cancelled by the user. Partial result follows:\n\n${partial}${wtSuffix}`, }, ], returnDisplay: this.currentDisplay!, }; } return { - llmContent: [{ text: finalText }], + llmContent: [{ text: finalText + wtSuffix }], returnDisplay: this.currentDisplay!, }; } finally { + // Mirror the background path: ensure the isolation worktree is + // reaped on every termination shape (success, failure, cancel, + // and any uncaught throw inside runFramed). The helper itself + // nulls `worktreeIsolation` on its first call (see the comment + // at its definition), so this fallback fires once at most even + // when the success path already ran it. + try { + await cleanupWorktreeIsolation(); + } catch { + // Helper logs its own failures; never mask the original + // error path with cleanup noise. + } this.eventEmitter.off(AgentEventType.TOOL_CALL, onFgToolCall); this.eventEmitter.off(AgentEventType.USAGE_METADATA, onFgUsageMetadata); signal?.removeEventListener('abort', onParentAbort); @@ -1730,6 +2119,24 @@ class AgentToolInvocation extends BaseToolInvocation { error instanceof Error ? error.message : String(error); debugLogger.error(`[AgentTool] Error running subagent: ${errorMessage}`); + // Final fallback for the isolation worktree: if the failure + // happened between provisioning and the inner try (e.g. inside + // `createApprovalModeOverride`, the agent constructor, or + // anywhere else upstream of the foreground/background try blocks + // that own cleanup), the worktree is still on disk. Reap or + // preserve it here, and surface the preserved path/branch in the + // failure message so the user can recover it. + let wtSuffix = ''; + if (worktreeIsolation) { + try { + wtSuffix = formatWorktreeSuffix(await cleanupWorktreeIsolation()); + } catch (cleanupError) { + debugLogger.warn( + `[AgentTool] Worktree cleanup after error failed: ${cleanupError}`, + ); + } + } + const errorDisplay: AgentResultDisplay = { ...this.currentDisplay!, status: 'failed', @@ -1737,7 +2144,7 @@ class AgentToolInvocation extends BaseToolInvocation { }; return { - llmContent: `Failed to run subagent: ${errorMessage}`, + llmContent: `Failed to run subagent: ${errorMessage}${wtSuffix}`, returnDisplay: errorDisplay, }; } diff --git a/packages/core/src/tools/agent/fork-subagent.ts b/packages/core/src/tools/agent/fork-subagent.ts index e442f727ca..f85c71deff 100644 --- a/packages/core/src/tools/agent/fork-subagent.ts +++ b/packages/core/src/tools/agent/fork-subagent.ts @@ -127,6 +127,29 @@ export function buildForkedMessages( return [fullAssistantMessage, toolResultMessage]; } +/** + * Notice injected into a subagent that has been spun up inside an isolated + * git worktree (via `AgentTool` `isolation: 'worktree'`). Tells the agent + * to confine all file operations to the worktree path and to re-read any + * file inherited from the parent's context before editing it. + * + * Mirrors claude-code's `buildWorktreeNotice` in + * `tools/AgentTool/forkSubagent.ts`. + */ +export function buildWorktreeNotice( + parentCwd: string, + worktreeCwd: string, +): string { + return ( + `You are operating in an isolated git worktree at ${worktreeCwd}. ` + + `The parent agent is in ${parentCwd}. Same repository, same relative file layout, separate working copy. ` + + `All your file edits, writes, and shell commands MUST target paths under ${worktreeCwd}. ` + + `When the inherited context references a path under ${parentCwd}, translate it to the corresponding path under ${worktreeCwd} before acting on it. ` + + `Re-read any file you intend to edit (the parent may have modified it after the snapshot in your context). ` + + `Your changes stay in this worktree and do not affect the parent's working tree.` + ); +} + export function buildChildMessage(directive: string): string { return `<${FORK_BOILERPLATE_TAG}> STOP. READ THIS FIRST. diff --git a/packages/core/src/tools/enter-worktree.test.ts b/packages/core/src/tools/enter-worktree.test.ts new file mode 100644 index 0000000000..e08656f26a --- /dev/null +++ b/packages/core/src/tools/enter-worktree.test.ts @@ -0,0 +1,346 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi } from 'vitest'; +import * as path from 'node:path'; +import { EnterWorktreeTool } from './enter-worktree.js'; +import { ExitWorktreeTool } from './exit-worktree.js'; +import type { Config } from '../config/config.js'; +import { GitWorktreeService } from '../services/gitWorktreeService.js'; + +function makeMockConfig(targetDir = '/tmp/mock-repo'): Config { + return { + getTargetDir: vi.fn(() => targetDir), + } as unknown as Config; +} + +describe('GitWorktreeService.validateUserWorktreeSlug', () => { + it('accepts simple slugs', () => { + expect( + GitWorktreeService.validateUserWorktreeSlug('my-feature'), + ).toBeNull(); + expect(GitWorktreeService.validateUserWorktreeSlug('foo123')).toBeNull(); + expect( + GitWorktreeService.validateUserWorktreeSlug('foo.bar_baz-1'), + ).toBeNull(); + }); + + it('rejects empty', () => { + expect(GitWorktreeService.validateUserWorktreeSlug('')).toMatch( + /non-empty/i, + ); + }); + + it('rejects path-traversal patterns', () => { + expect( + GitWorktreeService.validateUserWorktreeSlug('../etc/passwd'), + ).not.toBeNull(); + expect(GitWorktreeService.validateUserWorktreeSlug('a/b')).not.toBeNull(); + expect(GitWorktreeService.validateUserWorktreeSlug('foo..bar')).toMatch( + /must not.*\.\./i, + ); + expect(GitWorktreeService.validateUserWorktreeSlug('.hidden')).toMatch( + /must not start/i, + ); + expect(GitWorktreeService.validateUserWorktreeSlug('-leadingdash')).toMatch( + /must not start/i, + ); + }); + + it('rejects disallowed characters', () => { + expect(GitWorktreeService.validateUserWorktreeSlug('a b')).not.toBeNull(); + expect(GitWorktreeService.validateUserWorktreeSlug('a@b')).not.toBeNull(); + }); + + it('rejects strings longer than 64 chars', () => { + expect(GitWorktreeService.validateUserWorktreeSlug('a'.repeat(65))).toMatch( + /64/, + ); + expect( + GitWorktreeService.validateUserWorktreeSlug('a'.repeat(64)), + ).toBeNull(); + }); + + it('reserves the `agent-` prefix for ephemeral agent worktrees', () => { + // User-chosen `agent-` slugs that DO NOT match + // AGENT_WORKTREE_SLUG_PATTERN (`agent-<7hex>`) are rejected so + // they cannot live alongside the ephemeral shape and confuse the + // sweep. + expect( + GitWorktreeService.validateUserWorktreeSlug('agent-feature'), + ).toMatch(/reserved/i); + expect( + GitWorktreeService.validateUserWorktreeSlug('agent-1234567g'), // 8 chars, includes non-hex + ).toMatch(/reserved/i); + expect( + GitWorktreeService.validateUserWorktreeSlug('agent-12345678'), // 8 hex (too long) + ).toMatch(/reserved/i); + // Exact `agent-<7hex>` is the shape `generateAgentWorktreeSlug` + // produces — it must validate so AgentTool isolation can create + // its own slugs through the same code path. + expect( + GitWorktreeService.validateUserWorktreeSlug('agent-aabbccd'), + ).toBeNull(); + expect( + GitWorktreeService.validateUserWorktreeSlug('agent-1234567'), + ).toBeNull(); + // The standalone word "agent" or a different prefix is fine. + expect(GitWorktreeService.validateUserWorktreeSlug('agent')).toBeNull(); + expect(GitWorktreeService.validateUserWorktreeSlug('agentic')).toBeNull(); + expect(GitWorktreeService.validateUserWorktreeSlug('my-agent')).toBeNull(); + }); + + it('round-trip: every generated agent slug passes user validation', async () => { + // Regression guard: round 5 added the prefix reservation but + // initially used `startsWith` instead of `!matches pattern`, + // which silently broke EVERY agent isolation invocation. This + // test pins the contract: anything `generateAgentWorktreeSlug` + // produces MUST round-trip through the user validator. + const { generateAgentWorktreeSlug } = await import( + '../services/gitWorktreeService.js' + ); + for (let i = 0; i < 50; i++) { + const slug = generateAgentWorktreeSlug(); + expect(GitWorktreeService.validateUserWorktreeSlug(slug)).toBeNull(); + } + }); +}); + +describe('generateAgentWorktreeSlug', () => { + it('produces slugs that match AGENT_WORKTREE_SLUG_PATTERN', async () => { + const { generateAgentWorktreeSlug, AGENT_WORKTREE_SLUG_PATTERN } = + await import('../services/gitWorktreeService.js'); + for (let i = 0; i < 50; i++) { + const slug = generateAgentWorktreeSlug(); + expect(slug).toMatch(AGENT_WORKTREE_SLUG_PATTERN); + } + }); +}); + +describe('worktreeBranchForSlug', () => { + it('prefixes the slug with WORKTREE_BRANCH_PREFIX', async () => { + const { worktreeBranchForSlug, WORKTREE_BRANCH_PREFIX } = await import( + '../services/gitWorktreeService.js' + ); + expect(worktreeBranchForSlug('feat-x')).toBe( + `${WORKTREE_BRANCH_PREFIX}feat-x`, + ); + expect(worktreeBranchForSlug('agent-aabbccd')).toBe( + `${WORKTREE_BRANCH_PREFIX}agent-aabbccd`, + ); + }); +}); + +describe('EnterWorktreeTool.execute', () => { + // Real temp git repo fixtures so we exercise the actual git + // invocations without mocking the world. + it('refuses nested invocation from inside a worktree', async () => { + const fs = await import('node:fs/promises'); + const pathMod = await import('node:path'); + const os = await import('node:os'); + const cwd = await fs.mkdtemp(pathMod.join(os.tmpdir(), 'qwen-nested-')); + // Build a path that contains the nested-marker substring. + const nested = pathMod.join(cwd, '.qwen', 'worktrees', 'inner'); + await fs.mkdir(nested, { recursive: true }); + const cfg = { + getTargetDir: () => nested, + getSessionId: () => 'mock', + } as unknown as Config; + const tool = new EnterWorktreeTool(cfg); + const result = await tool + .build({ name: 'nope' }) + .execute(new AbortController().signal); + expect(result.error?.message).toMatch(/already inside.*worktree/i); + await fs.rm(cwd, { recursive: true, force: true }); + }); + + it('fails cleanly when cwd is not a git repository', async () => { + const fs = await import('node:fs/promises'); + const pathMod = await import('node:path'); + const os = await import('node:os'); + const cwd = await fs.mkdtemp(pathMod.join(os.tmpdir(), 'qwen-no-git-')); + const cfg = { + getTargetDir: () => cwd, + getSessionId: () => 'mock', + } as unknown as Config; + const tool = new EnterWorktreeTool(cfg); + const result = await tool + .build({ name: 'doesnt-matter' }) + .execute(new AbortController().signal); + expect(result.error?.message).toMatch(/not a git repository/i); + await fs.rm(cwd, { recursive: true, force: true }); + }); +}); + +describe('session marker round-trip', () => { + it('write then read returns the same session id', async () => { + const fs = await import('node:fs/promises'); + const path = await import('node:path'); + const os = await import('node:os'); + const { + writeWorktreeSessionMarker, + readWorktreeSessionMarker, + WORKTREE_SESSION_FILE, + } = await import('../services/gitWorktreeService.js'); + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-wt-session-')); + try { + await writeWorktreeSessionMarker(tmp, 'session-abc-123'); + const got = await readWorktreeSessionMarker(tmp); + expect(got).toBe('session-abc-123'); + const onDisk = await fs.readFile( + path.join(tmp, WORKTREE_SESSION_FILE), + 'utf8', + ); + expect(onDisk.trim()).toBe('session-abc-123'); + } finally { + await fs.rm(tmp, { recursive: true, force: true }); + } + }); + + it('returns null when the marker file is missing', async () => { + const fs = await import('node:fs/promises'); + const path = await import('node:path'); + const os = await import('node:os'); + const { readWorktreeSessionMarker } = await import( + '../services/gitWorktreeService.js' + ); + const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-wt-session-')); + try { + expect(await readWorktreeSessionMarker(tmp)).toBeNull(); + } finally { + await fs.rm(tmp, { recursive: true, force: true }); + } + }); + + it('returns null when the marker file is empty / whitespace', async () => { + const fs = await import('node:fs/promises'); + const pathMod = await import('node:path'); + const os = await import('node:os'); + const { readWorktreeSessionMarker, WORKTREE_SESSION_FILE } = await import( + '../services/gitWorktreeService.js' + ); + const tmp = await fs.mkdtemp(pathMod.join(os.tmpdir(), 'qwen-wt-session-')); + try { + await fs.writeFile( + pathMod.join(tmp, WORKTREE_SESSION_FILE), + ' \n \n', + 'utf8', + ); + expect(await readWorktreeSessionMarker(tmp)).toBeNull(); + } finally { + await fs.rm(tmp, { recursive: true, force: true }); + } + }); +}); + +describe('GitWorktreeService.generateAutoSlug', () => { + it('produces a slug matching the {adj}-{noun}-{6hex} pattern', () => { + for (let i = 0; i < 50; i++) { + const slug = GitWorktreeService.generateAutoSlug(); + expect(slug).toMatch(/^[a-z]+-[a-z]+-[0-9a-f]{6}$/); + expect(GitWorktreeService.validateUserWorktreeSlug(slug)).toBeNull(); + } + }); + + it('uses a strong RNG so 100 consecutive slugs are unique', () => { + // Math.random in the prior implementation had a 1/65k chance per + // suffix; with 100 slugs that was ~7% chance of a collision in + // tests. The randomBytes-backed 6-hex suffix should be essentially + // collision-free at this sample size. + const seen = new Set(); + for (let i = 0; i < 100; i++) { + seen.add(GitWorktreeService.generateAutoSlug()); + } + expect(seen.size).toBe(100); + }); +}); + +describe('GitWorktreeService.getUserWorktreesDir / getUserWorktreePath', () => { + it('uses .qwen/worktrees under the project root', () => { + // Use the cwd (which exists) so simple-git's existence check passes. + const root = process.cwd(); + const service = new GitWorktreeService(root); + // Build expected paths via path.join so the separator matches the + // platform — the implementation uses path.join, so on Windows the + // separator is `\`, not `/`. + expect(service.getUserWorktreesDir()).toBe( + path.join(root, '.qwen', 'worktrees'), + ); + expect(service.getUserWorktreePath('feat-x')).toBe( + path.join(root, '.qwen', 'worktrees', 'feat-x'), + ); + }); +}); + +describe('EnterWorktreeTool metadata', () => { + it('exposes the correct tool name and display name', () => { + const tool = new EnterWorktreeTool(makeMockConfig()); + expect(tool.name).toBe('enter_worktree'); + expect(tool.displayName).toBe('EnterWorktree'); + }); + + it('rejects an explicitly invalid name during validation', () => { + const tool = new EnterWorktreeTool(makeMockConfig()); + const error = tool.validateToolParams({ name: '../../etc' }); + expect(error).not.toBeNull(); + }); + + it('accepts an undefined name', () => { + const tool = new EnterWorktreeTool(makeMockConfig()); + expect(tool.validateToolParams({})).toBeNull(); + }); + + it('accepts an empty-string name (treated as auto-generate)', () => { + // Some models pass `{ name: '' }` when the schema marks `name` as + // optional. Validation should not reject this — `execute` falls back + // to an auto-generated slug. + const tool = new EnterWorktreeTool(makeMockConfig()); + expect(tool.validateToolParams({ name: '' })).toBeNull(); + }); +}); + +describe('ExitWorktreeTool default permission', () => { + it("returns 'ask' when action is 'remove'", async () => { + const tool = new ExitWorktreeTool(makeMockConfig()); + const inv = tool.build({ name: 'foo', action: 'remove' }); + expect(await inv.getDefaultPermission()).toBe('ask'); + }); + + it("returns 'allow' when action is 'keep'", async () => { + const tool = new ExitWorktreeTool(makeMockConfig()); + const inv = tool.build({ name: 'foo', action: 'keep' }); + expect(await inv.getDefaultPermission()).toBe('allow'); + }); +}); + +describe('ExitWorktreeTool metadata and validation', () => { + it('exposes the correct tool name', () => { + const tool = new ExitWorktreeTool(makeMockConfig()); + expect(tool.name).toBe('exit_worktree'); + expect(tool.displayName).toBe('ExitWorktree'); + }); + + it('requires action to be keep or remove', () => { + const tool = new ExitWorktreeTool(makeMockConfig()); + expect( + tool.validateToolParams({ + name: 'foo', + action: 'destroy' as 'keep' | 'remove', + }), + ).not.toBeNull(); + expect(tool.validateToolParams({ name: 'foo', action: 'keep' })).toBeNull(); + expect( + tool.validateToolParams({ name: 'foo', action: 'remove' }), + ).toBeNull(); + }); + + it('rejects invalid name slugs', () => { + const tool = new ExitWorktreeTool(makeMockConfig()); + expect( + tool.validateToolParams({ name: 'a/b', action: 'remove' }), + ).not.toBeNull(); + }); +}); diff --git a/packages/core/src/tools/enter-worktree.ts b/packages/core/src/tools/enter-worktree.ts new file mode 100644 index 0000000000..827dcdaecc --- /dev/null +++ b/packages/core/src/tools/enter-worktree.ts @@ -0,0 +1,253 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { ToolResult } from './tools.js'; +import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; +import type { Config } from '../config/config.js'; +import { ToolDisplayNames, ToolNames } from './tool-names.js'; +import { + GitWorktreeService, + writeWorktreeSessionMarker, +} from '../services/gitWorktreeService.js'; +import { createDebugLogger } from '../utils/debugLogger.js'; + +const debugLogger = createDebugLogger('ENTER_WORKTREE'); + +export interface EnterWorktreeParams { + /** + * Optional name (slug) for the worktree. Allowed characters: + * letters, digits, dot, underscore, hyphen. Maximum 64 characters. + * If omitted, an auto-generated `{adj}-{noun}-{4hex}` slug is used. + */ + name?: string; +} + +const enterWorktreeDescription = `Creates an isolated git worktree at \`/.qwen/worktrees/\` and returns its absolute path so subsequent file edits, shell commands, and other tools can operate inside it. + +## When to Use + +Only invoke this tool when the user **explicitly asks for a worktree** — e.g. "start a worktree", "use a worktree", "work in a worktree", "create a worktree". + +## When NOT to Use + +Do NOT call this tool when the user simply asks to fix a bug, implement a feature, create a branch, or check out code — those tasks belong to the regular working directory unless the user specifically mentions worktrees. + +## Behavior + +- Requires the current project to be a git repository. +- Creates a new branch \`worktree-\` based on the current branch. +- Returns the absolute \`worktreePath\`. From that point on, route every file path you create or edit through this directory; absolute paths are recommended. +- The worktree persists across the session until \`exit_worktree\` is invoked. +`; + +interface EnterWorktreeOutput { + worktreePath: string; + worktreeBranch: string; + message: string; +} + +class EnterWorktreeInvocation extends BaseToolInvocation< + EnterWorktreeParams, + ToolResult +> { + constructor( + private readonly config: Config, + params: EnterWorktreeParams, + ) { + super(params); + } + + getDescription(): string { + return this.params.name + ? `Enter worktree "${this.params.name}"` + : 'Enter a new worktree'; + } + + async execute(_signal: AbortSignal): Promise { + const cwd = this.config.getTargetDir(); + + // Refuse nested worktree creation. If the caller's cwd is itself + // already inside `.qwen/worktrees//`, a fresh worktree would + // be provisioned at `/.qwen/worktrees//` — but the + // model's mental model and inherited file paths would still + // reference the outer worktree. The resulting handle confusion + // typically leaves the inner worktree orphaned on exit. + // + // The check is conservative: any path component named + // `.qwen/worktrees` somewhere in cwd qualifies. We also forbid + // sentinel "inside a worktree" markers that an `enter_worktree` + // session leaves behind (writeSessionMarker, below). + if (/\.qwen[\\/]worktrees[\\/]/.test(cwd)) { + const reason = + 'Already inside a git worktree. Call exit_worktree first, ' + + 'or return to the main repository checkout before creating a ' + + 'new worktree.'; + debugLogger.warn(`enter_worktree: ${reason} (cwd=${cwd})`); + return errorResult(reason); + } + + // First-pass service rooted at cwd, only to find the repo top-level. + // We can't use cwd as the worktree anchor because launching from a + // monorepo subdirectory would scatter `.qwen/worktrees/` under each + // package's directory, and the startup sweep at `Config.initialize` + // would never find them. + const probe = new GitWorktreeService(cwd); + + const gitCheck = await probe.checkGitAvailable(); + if (!gitCheck.available) { + const reason = gitCheck.error ?? 'Git is not available.'; + debugLogger.warn(`enter_worktree: ${reason}`); + return errorResult(reason); + } + + const isRepo = await probe.isGitRepository(); + if (!isRepo) { + const reason = `Cannot create a worktree: ${cwd} is not a git repository. Initialize the repo with \`git init\` first.`; + debugLogger.warn(`enter_worktree: ${reason}`); + return errorResult(reason); + } + + // Resolve to the repo's top-level so worktrees always live under + // `/.qwen/worktrees/`, regardless of which subdirectory + // the user invoked the tool from. + const projectRoot = (await probe.getRepoTopLevel()) ?? cwd; + const service = + projectRoot === cwd ? probe : new GitWorktreeService(projectRoot); + + // Treat an empty `name` ('') the same as undefined — some models pass + // `{ name: '' }` when the schema marks `name` as optional, expecting + // the auto-generated slug. Without this, validation would reject the + // empty string before reaching the auto-slug path. + const requested = + this.params.name && this.params.name.length > 0 + ? this.params.name + : undefined; + const slug = requested ?? GitWorktreeService.generateAutoSlug(); + const validation = GitWorktreeService.validateUserWorktreeSlug(slug); + if (validation) { + debugLogger.warn(`enter_worktree: invalid slug ${slug}: ${validation}`); + return errorResult(validation); + } + + // Anchor at the parent session's currently checked-out branch. + // Without an explicit base, `createUserWorktree` falls back to + // whichever branch the main working tree has checked out, which is + // not necessarily where the user is working (e.g. they invoked + // qwen from a feature branch but the main working tree still has + // `main` checked out). + let baseBranch: string | undefined; + try { + baseBranch = await service.getCurrentBranch(); + } catch (error) { + debugLogger.warn( + `enter_worktree: getCurrentBranch failed at ${projectRoot}: ${error}`, + ); + } + const result = await service.createUserWorktree(slug, baseBranch); + if (!result.success || !result.worktree) { + const reason = result.error ?? 'Failed to create worktree.'; + debugLogger.warn(`enter_worktree: createUserWorktree failed: ${reason}`); + return errorResult(reason); + } + + // Tag the worktree with the current session id so a future + // `exit_worktree action='remove'` from a different session refuses + // to drop someone else's work. Best-effort: a write failure does + // not abort the creation (the worktree is still usable; ownership + // checks will treat unmarked worktrees as "owner unknown"). + try { + await writeWorktreeSessionMarker( + result.worktree.path, + this.config.getSessionId(), + ); + } catch (error) { + debugLogger.warn( + `enter_worktree: failed to write session marker at ${result.worktree.path}: ${error}`, + ); + } + + const output: EnterWorktreeOutput = { + worktreePath: result.worktree.path, + worktreeBranch: result.worktree.branch, + message: + `Created worktree "${slug}" at ${result.worktree.path} on branch ${result.worktree.branch}. ` + + `Use this absolute path for all subsequent file operations until you call ${ToolNames.EXIT_WORKTREE}.`, + }; + + debugLogger.debug( + `Created user worktree: ${output.worktreePath} (branch=${output.worktreeBranch})`, + ); + + return { + llmContent: JSON.stringify(output), + returnDisplay: + `Worktree **${slug}** created on branch \`${result.worktree.branch}\`\n` + + `\`${result.worktree.path}\``, + }; + } +} + +function errorResult(message: string): ToolResult { + return { + llmContent: `Error: ${message}`, + returnDisplay: `Error: ${message}`, + error: { message }, + }; +} + +export class EnterWorktreeTool extends BaseDeclarativeTool< + EnterWorktreeParams, + ToolResult +> { + static readonly Name: string = ToolNames.ENTER_WORKTREE; + + constructor(private readonly config: Config) { + super( + EnterWorktreeTool.Name, + ToolDisplayNames.ENTER_WORKTREE, + enterWorktreeDescription, + Kind.Other, + { + type: 'object', + properties: { + name: { + type: 'string', + description: + 'Optional slug (letters, digits, dot, underscore, hyphen; max 64 chars). Auto-generated when omitted.', + }, + }, + additionalProperties: false, + $schema: 'http://json-schema.org/draft-07/schema#', + }, + true, // isOutputMarkdown + false, // canUpdateOutput + true, // shouldDefer — only invoked when the user explicitly asks for a worktree + false, // alwaysLoad + 'worktree git isolated branch new', + ); + } + + override validateToolParams(params: EnterWorktreeParams): string | null { + if (params.name !== undefined) { + if (typeof params.name !== 'string') { + return 'Parameter "name" must be a string.'; + } + // Empty string is treated as "not provided" — `execute` falls back + // to an auto-generated slug. Skip slug-format validation here so + // the auto-slug path is reachable. + if (params.name.length === 0) { + return null; + } + const error = GitWorktreeService.validateUserWorktreeSlug(params.name); + if (error) return error; + } + return null; + } + + protected createInvocation(params: EnterWorktreeParams) { + return new EnterWorktreeInvocation(this.config, params); + } +} diff --git a/packages/core/src/tools/exit-worktree.test.ts b/packages/core/src/tools/exit-worktree.test.ts new file mode 100644 index 0000000000..798222c0ae --- /dev/null +++ b/packages/core/src/tools/exit-worktree.test.ts @@ -0,0 +1,302 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { execFileSync } from 'node:child_process'; +import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { ExitWorktreeTool } from './exit-worktree.js'; +import { EnterWorktreeTool } from './enter-worktree.js'; +import type { Config } from '../config/config.js'; +import { + GitWorktreeService, + WORKTREE_SESSION_FILE, + worktreeBranchForSlug, + writeWorktreeSessionMarker, +} from '../services/gitWorktreeService.js'; + +function makeMockConfig(targetDir = process.cwd()): Config { + // Default to cwd because `GitWorktreeService` constructs `simpleGit` + // against the dir, which fails on a non-existent path. Tests that + // need a real isolated repo create their own temp dir and pass it + // explicitly. + return { + getTargetDir: vi.fn(() => targetDir), + getSessionId: vi.fn(() => 'mock-session-id'), + } as unknown as Config; +} + +describe('ExitWorktreeTool', () => { + describe('metadata', () => { + it('exposes the correct tool name', () => { + const tool = new ExitWorktreeTool(makeMockConfig()); + expect(tool.name).toBe('exit_worktree'); + expect(tool.displayName).toBe('ExitWorktree'); + }); + }); + + describe('validateToolParams', () => { + it('requires a non-empty name', () => { + const tool = new ExitWorktreeTool(makeMockConfig()); + expect(tool.validateToolParams({ name: '', action: 'keep' })).toMatch( + /non-empty/i, + ); + }); + + it('requires action to be keep or remove', () => { + const tool = new ExitWorktreeTool(makeMockConfig()); + expect( + tool.validateToolParams({ + name: 'foo', + action: 'destroy' as 'keep' | 'remove', + }), + ).toMatch(/keep.*remove/i); + expect( + tool.validateToolParams({ name: 'foo', action: 'keep' }), + ).toBeNull(); + expect( + tool.validateToolParams({ name: 'foo', action: 'remove' }), + ).toBeNull(); + }); + + it('rejects slugs that would resolve outside the worktrees dir', () => { + const tool = new ExitWorktreeTool(makeMockConfig()); + expect( + tool.validateToolParams({ name: 'a/b', action: 'remove' }), + ).not.toBeNull(); + expect( + tool.validateToolParams({ name: '../etc', action: 'remove' }), + ).not.toBeNull(); + }); + + it('rejects discard_changes when it is not a boolean', () => { + const tool = new ExitWorktreeTool(makeMockConfig()); + expect( + tool.validateToolParams({ + name: 'foo', + action: 'remove', + // @ts-expect-error: deliberately wrong type + discard_changes: 'yes', + }), + ).toMatch(/boolean/i); + }); + }); + + describe('default permission', () => { + it("returns 'ask' when action is 'remove'", async () => { + const tool = new ExitWorktreeTool(makeMockConfig()); + const inv = tool.build({ name: 'foo', action: 'remove' }); + expect(await inv.getDefaultPermission()).toBe('ask'); + }); + + it("returns 'allow' when action is 'keep'", async () => { + const tool = new ExitWorktreeTool(makeMockConfig()); + const inv = tool.build({ name: 'foo', action: 'keep' }); + expect(await inv.getDefaultPermission()).toBe('allow'); + }); + }); + + describe('confirmation type — round-7 AUTO_EDIT bypass guard', () => { + // Regression guard for the round-7 finding: `getDefaultPermission` + // returning 'ask' was insufficient because BaseToolInvocation's + // default `getConfirmationDetails` returned `type: 'info'`, which + // `permissionFlow.isAutoEditApproved(AUTO_EDIT, 'info')` silently + // approves. The override must return `type: 'exec'` for action=remove. + it("returns type 'exec' for action=remove (NOT auto-approved by AUTO_EDIT)", async () => { + const tool = new ExitWorktreeTool(makeMockConfig()); + const inv = tool.build({ name: 'foo', action: 'remove' }); + const details = await inv.getConfirmationDetails( + new AbortController().signal, + ); + expect(details.type).toBe('exec'); + // Also verify the command field is populated, so the prompt UI + // shows the user what would actually run. + if (details.type === 'exec') { + expect(details.command).toContain('git worktree remove'); + expect(details.command).toContain('git branch -d worktree-foo'); + } + }); + + it("returns the base 'info' type for action=keep (non-destructive)", async () => { + const tool = new ExitWorktreeTool(makeMockConfig()); + const inv = tool.build({ name: 'foo', action: 'keep' }); + const details = await inv.getConfirmationDetails( + new AbortController().signal, + ); + expect(details.type).toBe('info'); + }); + }); + + describe('getDescription', () => { + it('mentions remove vs keep', () => { + const tool = new ExitWorktreeTool(makeMockConfig()); + const remove = tool.build({ name: 'foo', action: 'remove' }); + expect(remove.getDescription()).toMatch(/remove/i); + const keep = tool.build({ name: 'foo', action: 'keep' }); + expect(keep.getDescription()).toMatch(/keep/i); + }); + }); + + // ── execute() integration: real git repo, real worktree ────── + // These tests provision a temp git repo so we exercise the + // session-ownership guard, the keep path, and the missing-marker + // fallback against the actual implementation rather than mocking + // every git call. + describe('execute() — session ownership & lifecycle', () => { + let repoRoot: string; + + beforeEach(async () => { + repoRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-exit-wt-')); + execFileSync('git', ['init', '-q', '-b', 'main'], { cwd: repoRoot }); + execFileSync('git', ['config', 'user.email', 't@e.com'], { + cwd: repoRoot, + }); + execFileSync('git', ['config', 'user.name', 't'], { cwd: repoRoot }); + execFileSync('git', ['config', 'commit.gpgsign', 'false'], { + cwd: repoRoot, + }); + await fs.writeFile(path.join(repoRoot, 'README.md'), 'hi\n'); + execFileSync('git', ['add', '.'], { cwd: repoRoot }); + execFileSync('git', ['commit', '-q', '-m', 'init', '--no-verify'], { + cwd: repoRoot, + }); + }); + + afterEach(async () => { + await fs.rm(repoRoot, { recursive: true, force: true }); + }); + + async function provisionWorktree(slug: string): Promise { + // Use EnterWorktreeTool to create a real worktree so the test + // exercises the same code path users hit. + const enterCfg = { + getTargetDir: () => repoRoot, + getSessionId: () => 'session-creator', + } as unknown as Config; + const enter = new EnterWorktreeTool(enterCfg); + const inv = enter.build({ name: slug }); + const result = await inv.execute(new AbortController().signal); + expect(result.error).toBeUndefined(); + return new GitWorktreeService(repoRoot).getUserWorktreePath(slug); + } + + it('refuses remove when the marker names a different session', async () => { + const wtPath = await provisionWorktree('owned-by-creator'); + // Verify the marker landed. + const marker = await fs.readFile( + path.join(wtPath, WORKTREE_SESSION_FILE), + 'utf8', + ); + expect(marker.trim()).toBe('session-creator'); + + const otherCfg = { + getTargetDir: () => repoRoot, + getSessionId: () => 'session-stranger', + } as unknown as Config; + const exit = new ExitWorktreeTool(otherCfg); + const result = await exit + .build({ name: 'owned-by-creator', action: 'remove' }) + .execute(new AbortController().signal); + expect(result.error?.message).toMatch( + /different session.*owner=session-creator/i, + ); + // Worktree must still be on disk. + await expect(fs.access(wtPath)).resolves.toBeUndefined(); + }); + + it('keep returns success and leaves the worktree + branch intact', async () => { + const wtPath = await provisionWorktree('keepme'); + const cfg = { + getTargetDir: () => repoRoot, + getSessionId: () => 'session-creator', + } as unknown as Config; + const exit = new ExitWorktreeTool(cfg); + const result = await exit + .build({ name: 'keepme', action: 'keep' }) + .execute(new AbortController().signal); + expect(result.error).toBeUndefined(); + await expect(fs.access(wtPath)).resolves.toBeUndefined(); + const branches = execFileSync('git', ['branch', '--list'], { + cwd: repoRoot, + encoding: 'utf8', + }); + expect(branches).toContain(worktreeBranchForSlug('keepme')); + }); + + it('allows removal when the worktree predates the session-marker guard', async () => { + // Manually create a worktree without writing the marker — this + // is the upgrade path. The tool should warn-log and proceed. + const svc = new GitWorktreeService(repoRoot); + const created = await svc.createUserWorktree('legacy'); + expect(created.success).toBe(true); + // Explicitly DO NOT call writeWorktreeSessionMarker. + const wtPath = svc.getUserWorktreePath('legacy'); + await expect( + fs.access(path.join(wtPath, WORKTREE_SESSION_FILE)), + ).rejects.toBeDefined(); + + const cfg = { + getTargetDir: () => repoRoot, + getSessionId: () => 'session-stranger', + } as unknown as Config; + const result = await new ExitWorktreeTool(cfg) + .build({ name: 'legacy', action: 'remove' }) + .execute(new AbortController().signal); + expect(result.error).toBeUndefined(); + await expect(fs.access(wtPath)).rejects.toBeDefined(); + }); + + it('returns an error result when the worktree directory is missing', async () => { + const cfg = { + getTargetDir: () => repoRoot, + getSessionId: () => 'session-creator', + } as unknown as Config; + const result = await new ExitWorktreeTool(cfg) + .build({ name: 'nonexistent', action: 'remove' }) + .execute(new AbortController().signal); + expect(result.error?.message).toMatch(/not found/i); + }); + + it('refuses removal when the worktree branch has unmerged commits', async () => { + const wtPath = await provisionWorktree('committed'); + // Commit a change inside the worktree so it has work no other + // ref points at. + await fs.writeFile(path.join(wtPath, 'new.txt'), 'work\n'); + execFileSync('git', ['add', '.'], { cwd: wtPath }); + execFileSync('git', ['commit', '-q', '-m', 'work', '--no-verify'], { + cwd: wtPath, + }); + const cfg = { + getTargetDir: () => repoRoot, + getSessionId: () => 'session-creator', + } as unknown as Config; + const result = await new ExitWorktreeTool(cfg) + .build({ + name: 'committed', + action: 'remove', + discard_changes: true, + }) + .execute(new AbortController().signal); + expect(result.error?.message).toMatch(/unmerged|no other branch/i); + // Both worktree and branch must still be present. + await expect(fs.access(wtPath)).resolves.toBeUndefined(); + }); + + it('marker also written by writeWorktreeSessionMarker survives round-trip', async () => { + // Direct service-level write, then read via the same helper — + // covers the exclude-rule path (which is best-effort and may + // not fire in unusual test layouts). + const wtPath = await provisionWorktree('roundtrip'); + await writeWorktreeSessionMarker(wtPath, 'rewritten-id'); + const re = await fs.readFile( + path.join(wtPath, WORKTREE_SESSION_FILE), + 'utf8', + ); + expect(re.trim()).toBe('rewritten-id'); + }); + }); +}); diff --git a/packages/core/src/tools/exit-worktree.ts b/packages/core/src/tools/exit-worktree.ts new file mode 100644 index 0000000000..84ea075911 --- /dev/null +++ b/packages/core/src/tools/exit-worktree.ts @@ -0,0 +1,396 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import type { + ToolCallConfirmationDetails, + ToolConfirmationPayload, + ToolExecuteConfirmationDetails, + ToolResult, + + ToolConfirmationOutcome} from './tools.js'; +import { + BaseDeclarativeTool, + BaseToolInvocation, + Kind +} from './tools.js'; +import type { Config } from '../config/config.js'; +import type { PermissionDecision } from '../permissions/types.js'; +import { ToolDisplayNames, ToolNames } from './tool-names.js'; +import { + GitWorktreeService, + readWorktreeSessionMarker, + worktreeBranchForSlug, +} from '../services/gitWorktreeService.js'; +import * as fs from 'node:fs/promises'; +import { isNodeError } from '../utils/errors.js'; +import { createDebugLogger } from '../utils/debugLogger.js'; + +const debugLogger = createDebugLogger('EXIT_WORKTREE'); + +export interface ExitWorktreeParams { + /** + * The name (slug) of the worktree to exit, as provided to or returned + * by `enter_worktree`. + */ + name: string; + /** + * What to do with the worktree: + * - `'keep'` — leave the worktree directory and branch intact for later use. + * - `'remove'` — delete the worktree directory and branch. + */ + action: 'keep' | 'remove'; + /** + * When `action='remove'`, must be `true` to delete a worktree that has + * uncommitted changes (tracked or untracked). + */ + discard_changes?: boolean; +} + +const exitWorktreeDescription = `Exits a worktree previously created by ${ToolNames.ENTER_WORKTREE}. + +## Behavior + +- \`action='keep'\` — preserves the worktree directory and branch on disk so it can be revisited later. Use when work is in progress and the user might come back to it. +- \`action='remove'\` — deletes the worktree directory and branch. **Refuses to run** if the worktree contains uncommitted changes (tracked or untracked) unless \`discard_changes: true\` is set. Use when the work is committed (or intentionally being discarded). + +## When to Use + +Only invoke this tool when the user explicitly asks to leave or clean up a worktree (e.g. "exit the worktree", "remove that worktree", "we're done with the worktree"). Always pass the same \`name\` that was used with \`${ToolNames.ENTER_WORKTREE}\`. +`; + +interface ExitWorktreeOutput { + action: 'keep' | 'remove'; + worktreePath: string; + worktreeBranch: string; + message: string; +} + +class ExitWorktreeInvocation extends BaseToolInvocation< + ExitWorktreeParams, + ToolResult +> { + constructor( + private readonly config: Config, + params: ExitWorktreeParams, + ) { + super(params); + } + + getDescription(): string { + return this.params.action === 'remove' + ? `Remove worktree "${this.params.name}"` + : `Keep worktree "${this.params.name}"`; + } + + /** + * `action: 'remove'` deletes a worktree directory and (when safe) its + * branch. Other destructive tools (`edit`, `write_file`, + * `run_shell_command`) prompt by default; this tool should too. The + * `keep` action is non-destructive (it only restores the original + * working directory) and falls back to the framework default. + */ + override async getDefaultPermission(): Promise { + return this.params.action === 'remove' ? 'ask' : 'allow'; + } + + /** + * Override the framework's default `type: 'info'` confirmation for + * `action: 'remove'` so it is NOT silently auto-approved in + * `AUTO_EDIT` mode. + * + * Background: `permissionFlow.ts:isAutoEditApproved` auto-approves + * any tool whose `confirmationDetails.type` is `'edit'` or `'info'` + * when the session is in `AUTO_EDIT`. The base `BaseToolInvocation` + * returns `type: 'info'` by default, which means a `getDefaultPermission` + * of `'ask'` still gets bypassed in AUTO_EDIT — the data-loss path + * we explicitly closed for `DEFAULT` mode. Returning `type: 'exec'` + * (the same bucket `run_shell_command` lives in) keeps the + * confirmation prompt for AUTO_EDIT users too. `keep` falls through + * to the base info-type since it is non-destructive. + */ + override async getConfirmationDetails( + _abortSignal: AbortSignal, + ): Promise { + if (this.params.action !== 'remove') { + return super.getConfirmationDetails(_abortSignal); + } + const projectRoot = this.config.getTargetDir(); + const wtService = new GitWorktreeService(projectRoot); + const worktreePath = wtService.getUserWorktreePath(this.params.name); + const branch = worktreeBranchForSlug(this.params.name); + const command = + `git worktree remove ${worktreePath}` + ` && git branch -d ${branch}`; + const details: ToolExecuteConfirmationDetails = { + type: 'exec', + title: `Remove worktree "${this.params.name}"`, + command, + rootCommand: 'git', + onConfirm: async ( + _outcome: ToolConfirmationOutcome, + _payload?: ToolConfirmationPayload, + ) => { + // No-op: persistence handled by coreToolScheduler via PM rules. + }, + }; + return details; + } + + async execute(_signal: AbortSignal): Promise { + // Mirror `enter_worktree`: anchor at the repo top-level so we look + // for the worktree under the same directory it was created in. + // Otherwise launching `qwen` from a subdirectory of a monorepo would + // make exit_worktree look at `/.qwen/worktrees/`, + // which never exists, and every call would return "Worktree not + // found" even when the worktree is alive. + const cwd = this.config.getTargetDir(); + const probe = new GitWorktreeService(cwd); + const projectRoot = (await probe.getRepoTopLevel()) ?? cwd; + const service = + projectRoot === cwd ? probe : new GitWorktreeService(projectRoot); + + const worktreePath = service.getUserWorktreePath(this.params.name); + const branch = worktreeBranchForSlug(this.params.name); + + // Confirm the worktree directory actually exists before doing anything. + // Distinguish ENOENT ("not found", legitimate) from any other I/O + // failure (permission, EIO, ENOTDIR) — the previous bare `catch` + // collapsed all of them into "Worktree not found" with no log, + // making it impossible to diagnose a real filesystem problem. + let exists = false; + try { + const stat = await fs.stat(worktreePath); + 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 instanceof Error ? error.message : String(error)}).`, + ); + } + } + if (!exists) { + return errorResult( + `Worktree "${this.params.name}" not found at ${worktreePath}.`, + ); + } + + if (this.params.action === 'keep') { + const output: ExitWorktreeOutput = { + action: 'keep', + worktreePath, + worktreeBranch: branch, + message: + `Kept worktree "${this.params.name}" at ${worktreePath}. ` + + `Resume work there by referencing this path in subsequent tool calls.`, + }; + return { + llmContent: JSON.stringify(output), + returnDisplay: `Kept worktree **${this.params.name}** at \`${worktreePath}\``, + }; + } + + // action === 'remove' — three independent guards: + // + // 0. Session ownership: refuse to drop a worktree that was created + // by a different session. Without this, a prompt injection (or + // just a confused model) in session A could enumerate + // `.qwen/worktrees/` and call `exit_worktree` with a name + // belonging to session B, destroying its work. Worktrees + // created before this guard existed lack the marker; we treat + // those as "owner unknown" and allow removal (matches prior + // behaviour) but log so operators can see when the guard is + // bypassed. + const owner = await readWorktreeSessionMarker(worktreePath); + const currentSessionId = this.config.getSessionId(); + if (owner !== null && owner !== currentSessionId) { + return errorResult( + `Refusing to remove worktree "${this.params.name}" — it was ` + + `created by a different session (owner=${owner}). Resume the ` + + `owning session to drop it, or remove it manually with ` + + `\`git worktree remove ${worktreePath}\`.`, + ); + } + if (owner === null) { + debugLogger.warn( + `exit_worktree: worktree ${worktreePath} has no session marker; ` + + `allowing removal from session ${currentSessionId}`, + ); + } + + // 1. Uncommitted edits (working tree dirty). Bypassed by + // `discard_changes: true`. + // 2. Commits on the worktree branch that no other local branch or + // remote ref points at. Deleting the branch would lose them, so + // we refuse unconditionally — the user must merge, push, or + // rename the branch elsewhere first. There is no "discard + // 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); + if (counts === null) { + // Inspecting the worktree itself failed — most likely a corrupt + // git index, a permission problem, or the worktree dir was + // mutated under us. Refuse rather than suggesting + // `discard_changes: true`, which would tell the user to bypass + // a safety check whose precondition is unknown. The user should + // diagnose the underlying repo problem first. + return errorResult( + `Cannot inspect worktree "${this.params.name}" — git status failed against ${worktreePath}. ` + + `Check filesystem permissions and repository integrity, then call ${ToolNames.EXIT_WORKTREE} again.`, + ); + } + const total = counts.tracked + counts.untracked; + if (total > 0) { + return errorResult( + `Refusing to remove worktree "${this.params.name}" — it has ` + + `${counts.tracked} tracked change(s) and ${counts.untracked} untracked file(s). ` + + `Commit or stash first, or call again with \`discard_changes: true\`.`, + ); + } + } + + let hasUnmerged = true; + try { + hasUnmerged = await service.hasUnmergedWorktreeCommits(this.params.name); + } catch (error) { + // Service-level helper logs its own failures, but the caller + // context is what an operator would grep for ("why did + // exit_worktree refuse?"). Add a second log here so the chain + // (caller → reason it asked → underlying git error) is intact. + debugLogger.warn( + `exit_worktree: hasUnmergedWorktreeCommits failed for ${branch}: ${error}`, + ); + } + if (hasUnmerged) { + return errorResult( + `Refusing to remove worktree "${this.params.name}" — its branch ` + + `\`${branch}\` has commits that no other branch or remote ref ` + + `points at, and deleting the branch would lose them. Merge, ` + + `push, or rename the branch first, then call ${ToolNames.EXIT_WORKTREE} again.`, + ); + } + + const result = await service.removeUserWorktree(this.params.name, { + deleteBranch: true, + }); + if (!result.success) { + return errorResult(result.error ?? 'Failed to remove worktree.'); + } + if (result.branchPreserved) { + // Status check passed and unmerged check passed, but the safe + // delete still refused — most likely a race where new commits + // landed between the checks. Be loud rather than force-deleting. + const output: ExitWorktreeOutput = { + action: 'remove', + worktreePath, + worktreeBranch: branch, + message: + `Removed worktree directory "${this.params.name}" but kept branch ${branch} ` + + `(git refused a safe delete at the last moment — possibly a race with another ` + + `process). Recover with \`git branch -D ${branch}\` if you really want to discard it.`, + }; + return { + llmContent: JSON.stringify(output), + returnDisplay: `Removed worktree directory **${this.params.name}**, branch \`${branch}\` preserved`, + }; + } + + debugLogger.debug( + `Removed user worktree: ${worktreePath} (branch=${branch})`, + ); + + const output: ExitWorktreeOutput = { + action: 'remove', + worktreePath, + worktreeBranch: branch, + message: `Removed worktree "${this.params.name}" and deleted branch ${branch}.`, + }; + return { + llmContent: JSON.stringify(output), + returnDisplay: `Removed worktree **${this.params.name}** (branch \`${branch}\`)`, + }; + } +} + +function errorResult(message: string): ToolResult { + return { + llmContent: `Error: ${message}`, + returnDisplay: `Error: ${message}`, + error: { message }, + }; +} + +export class ExitWorktreeTool extends BaseDeclarativeTool< + ExitWorktreeParams, + ToolResult +> { + static readonly Name: string = ToolNames.EXIT_WORKTREE; + + constructor(private readonly config: Config) { + super( + ExitWorktreeTool.Name, + ToolDisplayNames.EXIT_WORKTREE, + exitWorktreeDescription, + Kind.Other, + { + type: 'object', + properties: { + name: { + type: 'string', + description: + 'Slug of the worktree to exit (must match the name used in enter_worktree).', + }, + action: { + type: 'string', + enum: ['keep', 'remove'], + description: + '"keep" preserves the worktree on disk; "remove" deletes it and its branch.', + }, + discard_changes: { + type: 'boolean', + description: + 'When action="remove", must be true to delete a worktree with uncommitted changes.', + }, + }, + required: ['name', 'action'], + additionalProperties: false, + $schema: 'http://json-schema.org/draft-07/schema#', + }, + true, // isOutputMarkdown + false, // canUpdateOutput + true, // shouldDefer — only invoked when the user explicitly asks to leave a worktree + false, // alwaysLoad + 'worktree exit leave remove keep cleanup', + ); + } + + override validateToolParams(params: ExitWorktreeParams): string | null { + if (typeof params.name !== 'string' || params.name.trim() === '') { + return 'Parameter "name" must be a non-empty string.'; + } + const slugError = GitWorktreeService.validateUserWorktreeSlug(params.name); + if (slugError) return slugError; + + if (params.action !== 'keep' && params.action !== 'remove') { + return 'Parameter "action" must be either "keep" or "remove".'; + } + if ( + params.discard_changes !== undefined && + typeof params.discard_changes !== 'boolean' + ) { + return 'Parameter "discard_changes" must be a boolean.'; + } + return null; + } + + protected createInvocation(params: ExitWorktreeParams) { + return new ExitWorktreeInvocation(this.config, params); + } +} diff --git a/packages/core/src/tools/tool-names.ts b/packages/core/src/tools/tool-names.ts index 80a78b9028..25a11bf63e 100644 --- a/packages/core/src/tools/tool-names.ts +++ b/packages/core/src/tools/tool-names.ts @@ -41,6 +41,8 @@ export const ToolNames = { STRUCTURED_OUTPUT: 'structured_output', MONITOR: 'monitor', TOOL_SEARCH: 'tool_search', + ENTER_WORKTREE: 'enter_worktree', + EXIT_WORKTREE: 'exit_worktree', } as const; /** @@ -72,6 +74,8 @@ export const ToolDisplayNames = { STRUCTURED_OUTPUT: 'StructuredOutput', MONITOR: 'Monitor', TOOL_SEARCH: 'ToolSearch', + ENTER_WORKTREE: 'EnterWorktree', + EXIT_WORKTREE: 'ExitWorktree', } as const; // Migration from old tool names to new tool names