mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-07-09 17:19:02 +00:00
4 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
顾盼
|
5ad5301805
|
feat(worktree): Phase D — startup --worktree flag + symlinkDirectories + PR refs (#4381)
* feat(worktree): Phase D — startup --worktree flag + symlinkDirectories + PR refs
Three cross-cutting capabilities on top of the Phase A-C worktree
foundation (PRs #4073, #4174).
D-1: --worktree [name] CLI flag creates a worktree (or re-attaches to
one that already exists) before any model turn runs. Supports bare,
plain-slug, `=`, and PR-reference forms; --worktree + --acp rejected
with a clear error; --worktree + --resume overrides the resumed
session's saved sidecar and emits a stderr line.
D-2: worktree.symlinkDirectories: string[] settings key opts into
symlinking main-repo directories (e.g. node_modules) into every
newly-created general-purpose worktree. Applies to all three creation
paths: --worktree flag, EnterWorktreeTool, AgentTool isolation. Path
traversal, absolute paths, and existing destinations all guarded;
missing source dirs and EEXIST silently skipped (fail-open).
D-3: --worktree=#<N> / --worktree <github-url> resolves a PR number,
runs `git fetch origin pull/<N>/head` (30s timeout, no `gh` CLI
dependency, LANG=C for stable error-taxonomy matching), and creates
the worktree off FETCH_HEAD. URL regex tolerates /files, /commits,
/checks sub-paths so users can paste any GitHub PR URL.
Phase 6 verification fixes also included:
- Re-attach to an existing worktree instead of failing with "Worktree
already exists" — the common `qwen --resume <sid> --worktree foo`
workflow now succeeds. The session ownership marker is preserved on
re-attach so cross-session exit_worktree action="remove" still fails
for non-owners.
- Normalize path-taking argv fields (mcpConfig, jsonSchema @<path>,
openaiLoggingDir, jsonFile, inputFile, telemetryOutfile,
includeDirectories) to absolute paths against the launch cwd BEFORE
the worktree chdir. Otherwise downstream fs.existsSync('./mcp.json')
resolves into the worktree, where the file doesn't exist.
Phase 7 code-review fixes:
- buildStartupWorktreeNotice differentiates "Active worktree" (fresh
create) from "Re-attached to worktree" (re-attach path).
- Notice survives sidecar persist failure: set before the try block,
refreshed inside with override addendum if persist succeeded.
- getRegisteredWorktreeBranch verifies the candidate path's git
common-dir matches the source repo's — rejects sibling `git init`
directories that happen to be on a worktree-<slug> branch.
Three-mode parity for the startup notice: TUI consumes via
AppContainer effect, headless prepends a <system-reminder> + emits a
worktree_started JSON event. ACP path is mutually exclusive with
--worktree (ACP hosts supply per-session cwd separately).
Tests (66 + 15 new):
- 15 cli/src/startup/worktreeStartup.test.ts (slug forms, PR fetch
against local fake remote, re-attach happy + wrong-branch guard)
- 8 core/src/services/gitWorktreeService.test.ts (parsePRReference:
#N, URLs, malformed, traversal, leading zeros, non-string)
- 10 core/src/services/gitWorktreeService.symlinks.integ.test.ts
(symlink loop + fetchPullRequestRef error taxonomy)
Known limitations (documented in docs/users/features/worktree.md):
- Cross-slug --resume <sid> --worktree <different-new-slug> is
unsupported by design (sessions are bound to projectHash(cwd));
future Config refactor anchoring storage at repo root would lift this.
- Mid-session enter_worktree still does NOT switch cwd/targetDir
(Phase A's simplification); only the startup --worktree flag does.
- yargs ambiguity: `qwen --worktree "say hi"` consumes the prompt as
the slug. Quick Start shows the `=` form and reordering workarounds.
Docs:
- docs/users/features/worktree.md (new): Quick Start with --worktree
flag, CLI Reference table for all four input forms + error codes,
settings table, Limitations.
- docs/design/worktree.md: Phase D section expanded into D-1/D-2/D-3
with open questions resolved; capability table updated.
- docs/e2e-tests/worktree-phase-d.md (new): full E2E plan with Phase 4
dry-run baseline + Phase 6 post-impl reproduction tables.
Refs #4056
* refactor(worktree): apply self-review feedback on Phase D
Self-review pass over the Phase D commit (2636f59273) catching one real
typecheck regression plus a batch of small quality + efficiency
improvements. No user-visible behavior change beyond fixing the build.
Build fix:
- worktreeStartup.ts imports — pre-commit prettier had reorganized
`writeWorktreeSession` and `readWorktreeSession` under an
`import type { ... }` block, erasing them at compile time
(verbatimModuleSyntax). `tsc --noEmit` was failing with TS1361.
Bundle path still worked (esbuild is lenient) so this only surfaced
when running typecheck.
Startup-path efficiency (~10-25 ms saved per --worktree invocation on
macOS; more on Windows):
- Drop redundant `isGitRepository()` probe — `getRepoTopLevel()`
returns null on non-git paths and covers both gates in one
subprocess.
- Run `getCurrentBranch()` + `getCurrentCommitHash()` in parallel via
Promise.all (independent calls).
- Combine the two `git rev-parse` probes inside
`getRegisteredWorktreeBranch` into a single multi-arg call, and run
it in parallel with the source-repo common-dir lookup. Saves one
fork+exec on the re-attach path.
Quality:
- Extract `withReminder()` local helper in nonInteractiveCli.ts so the
startup-notice and resume-restore branches share the system-reminder
wrapping.
- Log `readWorktreeSession` failures in `persistStartupWorktreeSidecar`
with the sidecar path so operators can recover the previous slug
from a backup. Silent swallow was making "where did my worktree
binding go?" undebuggable.
- Drop the dead `Config.getWorktreeSettings()` accessor (only
`getWorktreeSymlinkDirectories()` has callers); keep the underlying
`WorktreeSettings` interface for future fields.
- Document the `pendingStartupWorktreeNotice` invariant: at most one
consumer per process; ACP path is gated out earlier so only TUI XOR
headless reads it.
- Add a maintainer note in the gemini.tsx path-normalization block:
the argv path-field allowlist is hand-maintained, register new
path-bearing flags there or `--worktree` silently breaks for them.
- Drop `Phase 6 fix (G1)/(G2)` parenthetical labels from inline
comments — internal review-cycle identifiers that decay to noise
post-merge. Substantive prose retained.
Tests: cli 15/15 (unchanged) + core 66/66 (unchanged); bundle smoke
verified fresh / re-attach / invalid slug / non-git cases.
Findings deliberately left for follow-up:
- Larger refactor extracting a shared `provisionUserWorktree` helper
for the EnterWorktreeTool / startup overlap (~80% duplicate).
- Splitting the re-attach branch out of `setupStartupWorktree` into
its own function.
- `isPathWithinRoot` / `isInsideManagedWorktree` shared utils.
- `symlinkConfiguredDirectories` loop concurrency (saves 5-15 ms on a
cold path that runs only when symlinkDirectories is configured).
* docs(worktree): refresh stale docstring in worktreeStartup
Top-of-file docstring still said `{adj}-{noun}-{4hex}` (actual format
is 6 hex chars) and described the PR form as "detected and rejected
with a clear 'coming in D-3' message" — but D-3 shipped in the same
PR. Tighten to reflect what the code actually does.
* fix(worktree): address findings from dual-reviewer self-check
Two real bugs surfaced by an independent dual-reviewer pass (Claude +
Codex) on the Phase D commits. Both correctness-affecting; both
escaped the earlier internal reviews.
P0 — re-attach captured the wrong baseline for the exit dialog
(Codex):
setupStartupWorktree captured `originalHeadCommit` from the launch
cwd (main checkout) before any chdir. On the re-attach path the
WorktreeExitDialog later runs `git rev-list <originalHeadCommit>..HEAD`
inside the worktree to count "new commits this session". With the
main-checkout baseline this counted every commit ever made in the
kept worktree as new work from the current session — misleading the
keep/remove prompt. Re-capture HEAD from inside the worktree after
chdir so the count means what the dialog text says it means.
P0 — getRegisteredWorktreeBranch mis-identified plain directories as
registered worktrees (Claude):
A plain directory at `<repo>/.qwen/worktrees/<slug>/` (e.g. a stale
artifact from a previous tool) had no `.git` file of its own, so
`git rev-parse --git-common-dir` walked up to the outer repo and
returned the outer common-dir — matching the source repo's
common-dir check and impersonating a registered worktree. If the
outer repo happened to be on `worktree-<slug>`, setupStartupWorktree
would silently chdir into the plain directory and treat it as
attached; subsequent `exit_worktree action="remove"` would then
delete a directory that was never registered.
Fix: also probe `--show-toplevel` and require it to equal the
candidate path (canonicalised via `realpath` so macOS /var → /private/var
doesn't break the equality check). A plain dir under the main repo
gets the outer repo's toplevel and is correctly rejected.
Smaller polish from the same review:
- Normalize the literal string `'HEAD'` returned by `getCurrentBranch`
on detached HEAD to `undefined`, so the `baseRef` handed to
`git worktree add -b … HEAD` does not implicitly anchor against
the loose commit when the launch cwd is detached.
- `symlinkConfiguredDirectories`: blocklist `.git` (any nested
ancestor) and `.qwen/worktrees` (any nested ancestor). Linking
`.git` would silently break commits inside the worktree; linking
`.qwen/worktrees` would create a worktrees-inside-worktrees loop
that confuses the startup sweep.
- `WorktreeSettings.symlinkDirectories` typed `readonly string[]` to
match the `createUserWorktree(options.symlinkDirectories)` contract
and the immutable-config convention elsewhere. `Config.getWorktreeSymlinkDirectories()`
return type updated to match.
Docs:
- design/worktree.md precedence table rewritten. The previous
`--worktree` 赢 row was unreachable in practice (sessions are bound
to `projectHash(cwd)`, and the chdir happens before session lookup).
New table reflects what actually happens for each combination of
`--resume` × `--worktree`, including the documented
cross-projectHash limitation. The `persistStartupWorktreeSidecar`
override branch is now annotated as dead-on-the-current-architecture
but kept so a future Config refactor (anchor storage at repo root)
picks it up for free.
Tests: cli 15/15 + core 66/66 unchanged. Bundle smoke confirms both
P0 fixes end-to-end (re-attach captures worktree HEAD = run-1 tip,
plain-dir attempt errors out without clobbering existing content).
* refactor(worktree): consolidate probe + name detached-HEAD sentinel
Second /simplify pass on the dual-reviewer fixes. Three convergent
findings; net effect is one fewer subprocess on the re-attach path
and clearer intent on string handling / blocklist guards.
Efficiency + quality:
- Fold the worktree HEAD SHA into `getRegisteredWorktreeBranch`'s
combined rev-parse. The probe already requests common-dir,
toplevel, and abbrev-ref HEAD in a single subprocess; adding a
leading `HEAD` positional (which must come BEFORE `--abbrev-ref` so
the flag doesn't apply to it) returns the SHA on its own line.
Return type widened to `{ branch, headCommit } | null`. Removes
the second `GitWorktreeService` instantiation and `getCurrentCommitHash`
call that `setupStartupWorktree`'s re-attach branch used to do.
Quality:
- Hoist `'HEAD'` to a module-level `DETACHED_HEAD` constant in
`worktreeStartup.ts`. Three uses, two meanings (input filter when
normalizing `getCurrentBranch` output, fallback metadata for the
sidecar's `originalBranch` field on detached state). Naming the
sentinel makes intent self-documenting and pre-empts the "why is
the value we just stripped re-appearing as a fallback?" reader stall
flagged by the round-3 quality review.
Reuse + quality:
- `symlinkConfiguredDirectories`: replace two hand-rolled containment
checks (`startsWith(prefix + sep)` for `.qwen/worktrees`; `path.relative(...).split(sep)[0]`
for `.git`) with `isWithinRoot` from `utils/fileUtils.ts`, which is
already imported in this file. Replace the hardcoded
`path.join(repoRootAbs, '.qwen', 'worktrees')` with `this.getUserWorktreesDir()`
so the layout lives in one place (the exported `WORKTREES_DIR`
constant). Split the misleading `sourceAbs === repoRootAbs` clause
out of the `.git` branch into its own dedicated "empty / repo-root
path" rejection with a clearer warn message.
Tests: cli 15/15 + core 66/66 unchanged. Bundle smoke verified the
folded probe still captures the worktree's HEAD on re-attach (not
the launch-cwd HEAD).
Skipped from this review pass:
- Moving `'HEAD'` normalization into `GitWorktreeService.getCurrentBranch()`
itself — would ripple through `enter-worktree.ts` and `agent.ts`
callers that hand the result verbatim to `git worktree add -b ...`.
Out of scope for a polish pass; the local const is enough.
* fix(worktree): broaden symlink blocklist from .qwen/worktrees to all of .qwen
Caught by a second pr-tracker dual-reviewer pass (Codex). The previous
guard at `symlinkConfiguredDirectories` only refused paths inside
`<repoRoot>/.qwen/worktrees/` — `.qwen` itself (the parent) sailed
through because `isWithinRoot` is a strict descendant check. A user
setting `symlinkDirectories: ['.qwen']` would therefore symlink the
entire CLI metadata tree into the new worktree, recursively pulling
in `.qwen/worktrees` and recreating the loop the guard was meant to
prevent. Other `.qwen/*` subtrees (`projects`, `tmp`, …) are CLI
state with no legitimate cross-worktree sharing use case either.
Fix: broaden the guard to reject the whole `<repoRoot>/.qwen` tree.
Both `.qwen` itself and any descendant fail closed.
Also synced the user-facing settings schema description (the in-IDE
help text and the published JSON schema) so it mentions the `.git`
and `.qwen` rejection rules. The `WorktreeSettings` interface JSDoc
already mentioned them; the schema description had not been updated.
Tests: cli 15/15 + core 66/66 unchanged. Smoke confirms `--worktree foo`
with `symlinkDirectories: ['.qwen']` configured leaves the worktree
free of any `.qwen` symlink (only the legitimate per-worktree
`.qwen-session` marker file appears).
* fix(worktree): guard fetchPullRequestRef against CodeQL command-injection alert
CodeQL flagged a "Second order command injection" finding (rule 235) on
the `git fetch origin pull/<N>/head` call in `fetchPullRequestRef`. The
taint analyzer doesn't see the type-narrowing at the function entry
(`Number.isSafeInteger(prNumber) && prNumber > 0 && prNumber <= 1e9`),
so it considers `prNumber` library input that could in principle reach
a `--upload-pack=…`-shaped flag and thereby execute an arbitrary
program. In practice the entry guard already prevents that, but the
alert blocks the CodeQL CI check.
Add `--end-of-options` between `origin` and the refspec — git's
canonical "stop parsing flags" marker (git ≥ 2.24). Tells git
definitively that every subsequent argv element is a positional, not
a flag, which (a) satisfies the analyzer, (b) adds defense-in-depth
against a future regression that might relax the entry guard, and
(c) has zero behavior change for any well-formed PR number.
Verified locally: `git fetch --end-of-options origin pull/<N>/head`
against a local bare-remote with a seeded `refs/pull/42/head` still
fetches the ref correctly; the `--worktree=#42` smoke test reads back
the PR content from the materialized worktree.
Tests: cli 15/15 + core 66/66 unchanged.
* fix(worktree): lexical sanitizer for CodeQL + missing test mock entry
Two fixes from the third CI round on PR #4381:
1. CodeQL re-fires (round 2 of the same finding).
`--end-of-options` is a git-runtime defense, not a lexical sanitizer
that CodeQL's `js/second-order-command-line-injection` taint tracker
recognises. The alert re-fired against the same call after the
previous fix.
Switch to a CodeQL-recognised sanitizer: validate the numeric
component against `/^[1-9][0-9]*$/` immediately at the sink. The
regex digit-only check is one of the documented sanitizer patterns
the rule looks for, and proves at the analyzer level that the
resulting argv element cannot resemble a flag (`--foo`). The entry
guard at the top of the function still establishes the same fact
at runtime; this layer makes the proof visible to static analysis.
Keep `--end-of-options` as a runtime fallback against any future
regression that loosens the entry guard.
2. `nonInteractiveCli.test.ts` mock was missing the new
`consumePendingStartupWorktreeNotice` Config method.
Phase D-1 added the method on `Config` and `nonInteractiveCli`
calls it on every prompt to pick up the one-shot startup-worktree
notice. The test file's `mockConfig` literal was not updated, so
all 19 `runNonInteractive` tests threw
`TypeError: config.consumePendingStartupWorktreeNotice is not a
function` on Ubuntu / macOS CI.
Add a stub returning `null` so the helper short-circuits, matching
the equivalent Phase C stub for `getResumedSessionData`.
Local: cli (worktreeStartup + nonInteractiveCli) 60 passed + 1
skipped; core (gitWorktreeService + symlinks + hooks +
enter-worktree) 66 passed.
* test(worktree): mock getWorktreeSymlinkDirectories in three more test files
Round 4 of the same Phase D-2 mock-drift class. CI surfaced 9 test
failures across three files whose `Config` mocks construct
`EnterWorktreeTool` for setup but lack the new
`getWorktreeSymlinkDirectories` method `createUserWorktree` now
calls:
- enter-worktree.session.integ.test.ts (2 tests)
- exit-worktree.session.integ.test.ts (3 tests) — provisions
worktrees via EnterWorktreeTool before exercising exit paths
- exit-worktree.test.ts (4 tests) — same provisioning pattern via
`provisionWorktree()` and the `makeMockConfig` helper
Add a `getWorktreeSymlinkDirectories: () => []` stub to each so
the symlink loop is a no-op in tests.
`enter-worktree.test.ts` and `agent/agent.test.ts` intentionally
skipped — they mock `GitWorktreeService.createUserWorktree` outright,
so the method call never fires in their code paths. Adding the stub
there would be defensive speculation. If a future test exercises
the real path, it'll surface there too and we'll add it then.
Local: core tools tests now 123 passed (was 9 failed / 114 passed
on CI run 26213122427 against commit
|
||
易良
|
a3037889a6
|
fix(core): replace structuredClone with shallow copy to prevent OOM in long sessions (#4286)
* docs: add OOM investigation reports and auto-compaction redesign proposal
- Runtime memory investigation plan
- Non-interactive memory benchmark report
- OOM reproduction report with 2GiB/4GiB synthetic tests
- Runtime diagnostics benchmark report
- Auto-compaction threshold redesign proposal
* fix(core): replace structuredClone with shallow copy to prevent OOM
Replace `structuredClone(this.history)` (called up to 4x per turn on the
send path) with a lightweight shallow copy via `copyContentContainer()`.
This eliminates the OOM root cause in long tool-heavy sessions where the
full deep clone exceeded remaining V8 heap headroom.
Key changes:
- Add `copyContentContainer()` helper ({...content, parts: [...parts]})
- Add `getRequestHistory()` private method for the send path
- Add `getHistoryShallow()`, `getHistoryTailShallow()`,
`peekLastHistoryEntry()`, `getLastModelMessageText()`,
`getHistoryLength()` for read-only callers
- Remove HEAP_PRESSURE_COMPRESSION_RATIO safety net (no longer needed
now that the underlying OOM cause is fixed)
- Update chatCompressionService to use getHistoryShallow(true)
- Update nextSpeakerChecker to send only lastMessage (not full history)
- Update memoryDiagnostics with process-tree RSS measurement
* feat(core): add runtimeDiagnostics utility for heap/memory instrumentation
Required by content generators (anthropic, openai, logging) which import
runtimeDiagnostics for optional heap-pressure telemetry during streaming.
Gated by QWEN_CODE_PROFILE_RUNTIME=1 environment variable.
* fix(cli): update doctorCommand test mocks for new MemoryDiagnostics interface
Add missing maxRSSRaw, maxRSSUnit, and processTree fields to test fixtures
to match the updated MemoryResourceUsage and MemoryDiagnostics interfaces.
* fix(vscode-ide-companion): use public core imports
* fix: address review comments — type guards, dead fallbacks, and doc accuracy
Code:
- Fix unsound type guard: `'text' in part` → `typeof part.text === 'string'`
in geminiChat.ts and client.ts (Copilot + wenshao feedback)
- Remove unnecessary optional chaining and dead fallback chains in client.ts
(getHistoryShallow, peekLastHistoryEntry, getHistoryLength, etc. now call
GeminiChat methods directly)
- Add 5s timeout to `execFileAsync('ps', ...)` in memoryDiagnostics.ts
Docs:
- Fix GiB conversion accuracy and add single-run caveat to summary
- Add Node.js version to test environment table
- Fix auto-compaction attempt count (5→4) in OOM report
- Soften root-cause attribution certainty
- Add MCP child process context to investigation plan
- Clarify "Codex" reference (→ OpenAI Codex)
- Fix truncated MCP server name (chrome → chrome-devtools)
- Remove duplicate verification commands in benchmark table
- Clarify thread exhaustion vs V8 heap OOM distinction
- Add workload confound caveat to before/after comparison
- Fix SUMMARY_RESERVE "hard relationship" vs thinking budget contradiction
* fix(core): restore fallback chains in client.ts for mock compatibility
The previous commit removed optional chaining from client.ts wrapper
methods, but client.test.ts mocks getChat() with partial objects that
lack the new shallow methods. Restore ?. fallback chains so both
production (GeminiChat) and test (mock) paths work correctly.
* docs: clarify memory review follow-ups
* docs: fix runtime benchmark unit conversion
* docs: add default-heap OOM stress report
* fix: update copyright year to 2026 in new files [skip ci]
New files added in this PR had 2025 copyright headers. Updated to 2026
to reflect the current year.
|
||
|
顾盼
|
a7e05302e6
|
feat(worktree): Phase C — session persistence, hooksPath, Footer + WorktreeExitDialog, three-mode --resume restore (#4174)
* docs(worktree): update design doc — split Phase C/D, add Future section
- Phase C: session persistence + hooksPath + StatusLine + WorktreeExitDialog
- Phase D: --worktree CLI flag + symlinkDirectories
- Future: sparse checkout, .worktreeinclude, tmux, PR reference parsing
- Feature comparison table updated with Phase A/B completion status
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* docs(worktree): add Phase C implementation plan
8 tasks: WorktreeSession sidecar storage, hooksPath setup,
EnterWorktree/ExitWorktree session wiring, useWorktreeSession hook,
Footer display, --resume context injection, WorktreeExitDialog.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* docs(worktree): update Phase C plan after claude-code comparison
- WorktreeSession: add originalHeadCommit field
- hooksPath: add .husky/ detection + skip-if-already-set logic
- StatusLine payload: expand worktree field to match claude-code schema
- WorktreeExitDialog: load dirty state on mount, display counts in dialog
- UIState.activeWorktree: add originalCwd, originalBranch, originalHeadCommit
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* feat(worktree): add WorktreeSession sidecar storage
New worktreeSessionService.ts exposes read/write/clear functions for the
sidecar JSON file at <chatsDir>/<sessionId>.worktree.json. SessionService
gains getWorktreeSessionPath() so callers don't need to know the layout.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* feat(worktree): configure core.hooksPath after worktree creation
createUserWorktree() now sets `core.hooksPath` inside the new worktree to
the main repo's hooks directory (.husky preferred, .git/hooks fallback) so
commits inside the worktree run the same pre-commit checks as the main
repo. Mirrors claude-code's performPostCreationSetup logic — skips the
subprocess when the value already matches to avoid ~14ms spawn overhead.
Failures are non-fatal: the worktree is still usable without hooks.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* feat(worktree): persist WorktreeSession sidecar in EnterWorktreeTool
After creating a worktree, EnterWorktreeTool now writes a sidecar JSON
file at <chatsDir>/<sessionId>.worktree.json with the full session state
(slug, paths, branches, original HEAD SHA). --resume reads this in Phase
C task 7 to restore worktree context. Best-effort: write failures don't
abort the creation.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* feat(worktree): clear WorktreeSession sidecar in ExitWorktreeTool
After successful keep or remove, ExitWorktreeTool now clears the sidecar
JSON file iff its slug matches the worktree being exited. The slug check
prevents wiping the sidecar when the user exits a worktree that isn't
currently tracked (multiple worktrees on disk, sidecar tracks one).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* feat(worktree): expose active worktree via useWorktreeSession + UIState
New useWorktreeSession hook watches the sidecar JSON file (created by
EnterWorktreeTool, deleted by ExitWorktreeTool) and returns the current
WorktreeSession or null. AppContainer wires it into a new
UIState.activeWorktree field consumed by Footer (Task 6) and
WorktreeExitDialog (Task 8).
A showWorktreeExitDialog state placeholder is added too, hardcoded false
until Task 8 wires the dialog trigger.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* feat(worktree): show active worktree in Footer + StatusLine payload
Footer renders `⎇ <branch> (<slug>)` when activeWorktree != null, but
only when the user has no custom statusline (their script likely
handles it from the stdin payload itself).
useStatusLine's StatusLineCommandInput gains a `worktree` field with
{name, path, branch, original_cwd, original_branch} — matches claude-code's
schema so statusline scripts can be shared across both CLIs.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* feat(worktree): inject context hint on --resume when worktree is active
On --resume, if the session has a WorktreeSession sidecar, append an
INFO history item pointing the model at the worktree path so it
continues using it for file operations. Stale sidecars (worktree dir
deleted out-of-band) are cleaned up so the Footer indicator doesn't
go stale.
qwen-code can't process.chdir() the way claude-code does because
Config.targetDir is immutable; the context hint is the equivalent
behavioral cue.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* feat(worktree): add WorktreeExitDialog with dirty-state inspection
WorktreeExitDialog renders when the user double-presses Ctrl+C inside a
worktree. On mount it runs `git status --porcelain` and
`git rev-list --count <originalHeadCommit>..HEAD` to show how many
uncommitted files and new commits the user would discard by choosing
"Remove". The dialog never auto-removes — every exit goes through
explicit user confirmation per requirements.
handleExit in AppContainer intercepts the second-press quit when
activeWorktree is set and shows the dialog instead. A new UIAction
handleWorktreeExit(choice) routes the user's choice through removal
(via GitWorktreeService.removeUserWorktree) + sidecar cleanup + /quit.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs(worktree): add Phase C E2E test plan
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs(worktree): fix E2E test plan sidecar path + jq selector
- sidecar lives at ~/.qwen/projects/<sanitized-cwd>/chats/, not ~/.qwen/tmp/<hash>/
- qwen --output-format json emits a JSON array, not NDJSON — jq needs .[]
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(worktree): add showWorktreeExitDialog to dialogsVisible
Phase C task 8 introduced showWorktreeExitDialog state and the dialog
render in DialogManager, but missed adding the flag to the dialogsVisible
OR expression. DefaultAppLayout only renders DialogManager when
dialogsVisible is true, so the dialog was never shown — second Ctrl+C
in a worktree silently absorbed instead of triggering the prompt.
Caught by Group E E2E tests.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* feat(worktree): extend --resume context restore to headless + ACP modes
Phase C task 7 originally placed the worktree-restore logic in
AppContainer.tsx (TUI only). E2E Group C exposed that headless and ACP
modes never run AppContainer, so stale sidecars accumulate and the model
loses worktree context after --resume.
Refactor to a shared `restoreWorktreeContext` helper in core, then wire
the three entry points:
- TUI (AppContainer): keep historyManager.addItem(INFO) UX, route via
the helper.
- Headless (nonInteractiveCli): prepend the notice as a system-reminder
block on the user prompt; emit a `worktree_restored` system message to
the JSON adapter so SDK consumers can react.
- ACP (Session.pendingWorktreeNotice): set by acpAgent.loadSession on
resume, consumed and cleared exactly once on the next #executePrompt.
All three modes call the same helper, so stale-sidecar cleanup is
consistent. Helper covers: missing sidecar, live worktree dir,
deleted worktree dir, regular file at worktreePath, malformed JSON.
5 new unit tests for restoreWorktreeContext (13/13 pass total).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* test(worktree): add ACP-mode integration tests for --resume context
Covers:
- acpAgent.worktree.test.ts (3 tests): loadSession sets
pendingWorktreeNotice only when worktree dir is live, clears
stale sidecar otherwise, swallows restoreWorktreeContext errors.
- Session.worktree.test.ts (4 tests): #executePrompt prepends the
system-reminder block exactly once on first prompt, clears the
pending notice, second prompt sees no leakage, no-op when nothing
was set.
E2E via real ACP protocol is impractical without a Zed client; these
tests cover the integration boundaries directly.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* docs(worktree): clarify hooksPath comment + pendingWorktreeNotice one-shot rationale
Two doc-only fixes from PR #4174 review:
- gitWorktreeService.ts: previous hooksPath comment overstated the
optimization (claimed claude-code's ~14ms saving but we still do a
read subprocess). Rewrite to be explicit: write-skip only, read
retained, parseGitConfigValue's full optimization deliberately not
ported because the read happens once per worktree creation.
- Session.ts: pendingWorktreeNotice doc now explains why it's one-shot
(after the first prompt the worktree path is already in conversation
context; re-injecting would clutter history without adding signal).
No behavior change.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(test): add getResumedSessionData to nonInteractiveCli mock Config
CI surfaced TypeError: config.getResumedSessionData is not a function
across 12 tests in nonInteractiveCli.test.ts. The Phase C
|
||
|
顾盼
|
609e05baee
|
feat(tools): add generic worktree support — EnterWorktree/ExitWorktree + Agent isolation (#4073)
* 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 `<projectRoot>/.qwen/worktrees/<slug>` on a `worktree-<slug>` branch and returns the absolute path. Slug auto-generated when omitted; validated against path traversal and disallowed characters. - exit_worktree: keeps or removes the worktree (and its branch). Refuses to remove a worktree with uncommitted tracked changes or untracked files unless `discard_changes: true` is set. Phase B — Agent isolation - Agent tool gains an `isolation: 'worktree'` parameter that provisions a temporary `agent-<7hex>` worktree, prepends a worktree notice to the task prompt, and on completion either removes the worktree (no changes) or preserves it and reports its path/branch in the result. Background and foreground execution paths both wired up; rejected for fork agents. - worktreeCleanup.cleanupStaleAgentWorktrees: fail-closed sweep for ephemeral `agent-<7hex>` worktrees older than 30 days with no tracked changes and no unpushed commits. User-named worktrees are never swept. - buildWorktreeNotice helper for fork subagents (parity with claude-code). Arena compatibility - The existing Arena worktree implementation (GitWorktreeService.setupWorktrees, ArenaManager, agents.arena.worktreeBaseDir) is untouched. Arena uses its own batch APIs and `~/.qwen/arena` base dir; the new general-purpose APIs live alongside under `<projectRoot>/.qwen/worktrees/`. Subagent safety - enter_worktree / exit_worktree are added to EXCLUDED_TOOLS_FOR_SUBAGENTS so a subagent cannot mutate the parent session's worktree state. Refs #4056 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(worktree): use path.join in expected paths so the test passes on Windows The Windows CI run reported `enter-worktree.test.ts` failing because the expected string was hardcoded with `/` while `getUserWorktreesDir()` uses `path.join`, which returns `\\` on Windows. Build the expected path via `path.join` so the platform-correct separator is compared. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(enter-worktree): treat empty name as auto-generate Some models pass `{ "name": "" }` when calling EnterWorktree, because the schema marks `name` as optional and they emit an empty placeholder. The previous validation rejected the empty string with "Worktree name must be a non-empty string", which surprised users running the auto-slug path. Now both `validateToolParams` and `execute` treat `name: ""` as equivalent to `name: undefined` and fall back to the auto-generated `{adj}-{noun}-{4hex}` slug. Explicit invalid slugs (`'../etc'`, `'a/b'`, etc.) are still rejected as before. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address review findings 1-6 from PR #4073 Six issues raised on the initial review; each addressed with a verifiable guarantee. 1. Real isolation for `agent isolation: 'worktree'` Before: subagent's Config still resolved `getTargetDir()` to the parent project root, so Edit/Write/Read workspace checks and Shell's default cwd silently operated on the parent tree. The cleanup helper then saw a "clean" worktree and removed it — destroying the evidence. After: the worktree is provisioned BEFORE `createApprovalModeOverride`, and the resulting agent Config has `getTargetDir`/`getCwd`/`getWorkingDir` rebound to the worktree path. Relative paths, unqualified shell commands, and glob/grep roots all confine to the worktree. 2. `exit_worktree action='remove'` now prompts in default/auto-edit modes Added `getDefaultPermission()` on the invocation: `'ask'` when action is `remove`, `'allow'` when `keep`. Brings it in line with edit, write_file, and run_shell_command. 3. Force-delete no longer silently destroys unpushed commits `removeUserWorktree` now uses `git branch -d` (refuses unmerged) by default and surfaces `branchPreserved: true` when git refuses. Added `hasUnmergedWorktreeCommits` (checks if branch tip is reachable from any other local branch or remote ref). Both the agent isolation cleanup and `exit_worktree action='remove'` use this check: if the branch has work not covered elsewhere, the worktree+branch are preserved even when `discard_changes: true` is set (there is no `discard_commits` flag — committed work is rarely what `remove` means to discard). 4. Both new tools are now deferred behind ToolSearch `shouldDefer: true` + `searchHint` on both. Verified via openai-logging: `enter_worktree` and `exit_worktree` no longer appear in the function- declaration list sent on every API request. 5. Stale-worktree cleanup is wired in `Config.initialize()` fires `cleanupStaleAgentWorktrees(targetDir)` as a non-awaited startup sweep (skipped in bare mode). Picks up orphaned `agent-<7hex>` worktrees left by crashed runs. 6. Foreground isolation no longer leaks on uncaught throw The foreground try block tracks whether the cleanup helper ran on the success path; the finally block invokes it as a fallback when the try bailed early. Mirrors the background path's pattern. Verification: - Unit tests: 83 passed (16 worktree + 64 existing agent + 3 cleanup) — no regressions. - E2E #1: agent told to write `hello.txt` via RELATIVE path — file landed at `.qwen/worktrees/agent-XXXXXXX/hello.txt`, NOT at the parent root. - E2E #3: created worktree, committed work inside it, called exit_worktree with `discard_changes=true` — refused with clear message; worktree and branch both preserved. - E2E #4: openai-logging confirms worktree tools absent from API tool list (7 tools sent instead of 9). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address review round 2 findings (1 from tanzhenxin, 7+8 from wenshao) The first round closed the data-loss-class issues. This round addresses follow-ups from a deeper audit: 1. Stale-worktree sweep was inert on common-case repos `cleanupStaleAgentWorktrees` previously ran `git log --branches --not --remotes --oneline` from each worktree's directory — that lists unpushed commits across EVERY local branch, not just the worktree's own branch. On any repo with no remote configured (or with stray unpushed branches), the sweep refused to remove every candidate. Replaced with `service.hasUnmergedWorktreeCommits(slug)` which scopes the check to the worktree branch via `for-each-ref --contains <tip>`. Also added the `branchPreserved` warn log requested in M7 and an `fs.access` shortcut for the empty-worktrees-dir case (M8). 2. `cleanupWorktreeIsolation` and `worktreeIsolation` were inside the inner try (~660 lines from the outer catch). Hoisted both to the top of `execute()` so the outer catch can reap or preserve the worktree when anything between provisioning and the inner try throws (e.g. `createApprovalModeOverride`, agent creation). Closure carries the resolved `repoRoot` so cleanup never has to re-resolve. 3. Background error path discarded the cleanup result. Now captures `formatWorktreeSuffix(...)` and appends it to the registry's failure /cancel message, so users see the preserved path/branch even when the agent crashed before reporting. 4. `cleanupWorktreeIsolation` now treats `result.success === false` as "worktree still on disk" and surfaces it as preserved instead of silently dropping it from the result. 5. Override was incomplete. Several Config methods read `this.targetDir` directly (`getProjectRoot`, `getFileService`, etc.) — own-property getter overrides did not redirect them. Now also shadows `targetDir` and `cwd` as own properties on the agent's Config override, swaps in a `FileDiscoveryService` rooted at the worktree, and rebuilds `WorkspaceContext` to point at the worktree only. Verified end-to-end: shell `pwd > pwd-record.txt` (no directory arg) lands at `.qwen/worktrees/agent-<7hex>/pwd-record.txt`, not the parent root. 6. monorepo subdir issue. Both `enter_worktree` and the agent isolation path now resolve `git rev-parse --show-toplevel` first and anchor `.qwen/worktrees/<slug>` at the repo root. Worktrees created from any subdirectory now end up where the startup sweep can find them. 7. Replaced `git worktree add -B` (silent force-reset of pre-existing branches) with `git worktree add -b` plus an explicit existence check via `git for-each-ref` (NOT `show-ref --quiet`, which simple-git swallows). Pre-existing `worktree-<slug>` branches now trigger a clear error instead of clobbering committed work. 8. First worktree creation in a repo writes `<projectRoot>/.qwen/.gitignore` with `worktrees/` so worktree contents stay out of the parent's `git status`, glob/grep results, and bundle tools. Idempotent: never overwrites an existing file. 9. Logging across the failure paths (`enter_worktree` errors, `agent.ts:failWorktreeProvisioning`, `cleanupWorktreeIsolation`, `hasUnmergedWorktreeCommits` swallowed errors, `cleanupStaleAgentWorktrees`'s `branchPreserved` race). 10. `exit_worktree` no longer suggests `discard_changes: true` when the git status check itself fails — that would be advising the user to bypass a safety check whose precondition is unknown. Now points at the underlying repo problem. 11. `generateAutoSlug` switched from `Math.random()` (4 hex, weak RNG, one-in-65k collision) to `randomBytes` (6 hex, ~16M combinations). Two RNG sources in this file collapsed to one. Pushed back: the TOCTOU swap in `removeUserWorktree` (S6 round 1) is left as-is — `git branch -d` is the real safety, and reordering does not eliminate the window. Windows reserved-name validation (M5 round 2) deferred to a follow-up; the current allowlist already rejects path separators, `..`, leading dot/dash, and the >64-char case. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): use randomInt to silence CodeQL biased-modulo finding CodeQL's `js/biased-cryptographic-random` flagged `randomBytes(4)[i] % ARRAY.length` in `generateAutoSlug`. The math is actually exact for the current word-list lengths (256 % 8 == 0), but the lint rule does not know that — and a future contributor changing the list to a non-power-of-two length would silently introduce bias. Switched the index lookups to `crypto.randomInt(0, length)`, which uses rejection sampling and is uniform by construction. Suffix still uses `randomBytes(3).toString('hex')` since hex encoding is unbiased. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address review round 3 findings 1-6 from PR #4073 The previous round added `getRepoTopLevel` for `enter_worktree`'s provisioning, but missed three sibling call sites that still used the raw cwd. The double-cleanup race in the foreground path also leaked stale `[worktree preserved]` suffixes on rejected promises. All six findings from the deeper audit are addressed: 1. exit_worktree now resolves through `getRepoTopLevel()` before building its `GitWorktreeService`, mirroring `enter_worktree`. Without this, launching `qwen` from a monorepo subdirectory created the worktree under the repo root but exit_worktree looked under the subdir's `.qwen/worktrees/` and always returned "Worktree not found". Verified end-to-end: enter + exit from `packages/core/` works. 2. agent.ts cleanup helper now nulls `worktreeIsolation` immediately after capturing the closure value. The previous structure could reach the helper twice — once in the foreground try's success path and once in the foreground finally fallback (or once in the inner try and once in the outer catch on a thrown rejection). The second call would `hasWorktreeChanges()` against a directory the first call already removed, fail-closed, and emit a bogus `[worktree preserved: <missing path>]` suffix. 3. Config.initialize's startup sweep now resolves `getRepoTopLevel()` before invoking `cleanupStaleAgentWorktrees`. Without this, every subdir launch scanned a non-existent `<subdir>/.qwen/worktrees/` and the 30-day expiry sweep was permanently a no-op. 4. agent.ts's `buildWorktreeNotice` now passes `worktreeIsolation.repoRoot` as `parentCwd` instead of `this.config.getTargetDir()`. The notice's path-translation guidance (≈ "translate paths from <parent> to <worktree>") would otherwise misdirect the subagent in a monorepo subdir launch. 5. Removed dead method `GitWorktreeService.listUserWorktrees`. It had no callers anywhere in the codebase and used `execSync` in a loop (would have blocked the event loop if anyone wired it up). 6. `localBranchExists` no longer swallows git failures silently. The defensive `false` default is preserved (so `git worktree add -b` itself surfaces the conflict if the check missed an existing branch), but the catch now logs via `debugLogger.warn` so disk-full / permission / ref-store-corruption cases are visible in debug output instead of being invisible. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address review round 4 findings (data-loss + visibility) Seven actionable findings from a deeper audit, all closed: 1. User worktree slugs could collide with ephemeral-agent shape `validateUserWorktreeSlug` did not reject names starting with `agent-`, so a user-named `agent-1234567` matched the cleanup regex `/^agent-[0-9a-f]{7}$/` and would be silently swept after 30 days along with whatever work was in it. Now reserved — clear error message points users at the cause. 2. Slug producer and consumer were string-coupled across files `agent.ts` hardcoded `agent-${hex(7)}` and `worktreeCleanup.ts` independently hardcoded `/^agent-[0-9a-f]{7}$/`. Future change to hex length on one side would silently break the other. Lifted `AGENT_WORKTREE_PREFIX`, `AGENT_WORKTREE_HEX_LENGTH`, `AGENT_WORKTREE_SLUG_PATTERN`, and `generateAgentWorktreeSlug()` to `gitWorktreeService.ts`; both call sites import them. 3. Startup sweep was invisible at default log level Fire-and-forget sweep used `debug` for errors and discarded the success count. A leak-chasing operator had no log breadcrumb. Errors promoted to `warn`; successful removals (count > 0) logged at `info`. 4. `getRepoTopLevel()` silent catch Returned `null` on any git failure with no log. Combined with `?? cwd` fallback in callers, a flaky git would have made worktree creators and the startup sweep disagree silently about which dir to use. Now logs the underlying error. 5. `hasTrackedChanges()` silent catch Cleanup's fail-closed `return true` had no log. Couldn't tell "has real changes — leave alone" from "git index unreadable — repo may be corrupt". Now logs. 6. `cleanupWorktreeIsolation` claimed `preservedPath` for a removed dir When `removeUserWorktree` returns `{ success: true, branchPreserved: true }` it has already deleted the directory and failed only on `git branch -d`. The helper still reported the (now non-existent) path as preserved. Now returns only `preservedBranch` for that case; `formatWorktreeSuffix` emits a distinct message instructing recovery via `git worktree add <new-path> <branch>`. 7. `removeUserWorktree` swallowed branch-delete failures Both `-d` and `-D` catch blocks were empty. Locked refs, perms, disk full all looked identical to "unmerged commits". Both now `debugLogger.warn` with the underlying error. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(worktree): self-review pass — reuse, parallelism, dead code Self-review caught a handful of issues across three categories: Reuse: - `pathExists` in the new code now uses the existing `fileExists` from `utils/fileUtils.ts` instead of duplicating an `fs.access` wrapper. - `worktree-` branch prefix was string-literalled in five places. Added `WORKTREE_BRANCH_PREFIX` and `worktreeBranchForSlug(slug)` exports in `gitWorktreeService.ts`; updated `gitWorktreeService.ts`, `worktreeCleanup.ts`, and `exit-worktree.ts` to use them. Future prefix changes are a single edit. Efficiency: - `Config.initialize` used two `await import(...)` calls inside the startup-sweep IIFE, paying that cost on every CLI start. Switched to static imports at the top of `config.ts` — the modules are tiny and the dynamic indirection bought nothing. - `cleanupWorktreeIsolation` in `agent.ts` ran `hasWorktreeChanges` and `hasUnmergedWorktreeCommits` sequentially. They have no data dependency on each other and each spawns its own `git` invocation; `Promise.all` halves the cleanup wall-clock on the common path. Same fix in `worktreeCleanup.ts`'s per-entry loop. - `ensureWorktreesGitignored` used `fs.access` then `fs.writeFile`, a TOCTOU race when two agent invocations created worktrees concurrently (both could pass the `access` check and the second would clobber the first's `.gitignore`). Now writes with `flag: 'wx'` and treats `EEXIST` as the no-op case — atomic in one syscall. Quality: - Dropped the `worktreeCleanupRan` boolean in the foreground execution path. `cleanupWorktreeIsolation` already nulls its closure variable at the top of every call (see the comment at its definition), so re-entries are no-ops. The boolean and its tracking were dead weight that obscured the real guard. - Trimmed the Phase-2 override comment block to drop the WHAT-stating enumerations (items 3 and 4 just narrated the lines below) and removed a navigation comment about hoisted helpers — the helpers are visible at the top of the same method. 84 unit tests pass; typecheck clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address review round 5 — design-doc commitments + correctness Five critical findings + four suggestions, all closed. Critical: 1. Wrong base branch for agent isolation. `createUserWorktree(slug)` with no `baseBranch` arg fell back to `getCurrentBranch()` on the **main** working tree, returning `main` regardless of which branch the user was actually on. A subagent invoked from `feature-x` would silently start from `main` and produce diffs against the wrong baseline. `enter_worktree` had the same bug. Both now resolve the parent's current branch first and pass it explicitly. Verified end-to-end: `git checkout feature-x` → `enter_worktree` → worktree HEAD includes the feature-x commit. 2. `countWorktreeChanges` (used by `exit_worktree`'s dirty-state guard) missed `status.conflicted[]`. In simple-git that array is mutually exclusive with the staged/modified/etc. arrays, so a worktree mid-merge with only conflicts looked `{tracked: 0, untracked: 0}` to the guard and `action='remove'` would proceed without `discard_changes: true`. Added `+ status.conflicted.length`. 3. `exit_worktree` had no session-ownership check, contradicting the design doc's "only operates on worktrees created by THIS session". In yolo mode a prompt injection could enumerate `.qwen/worktrees/` and pass any name to drop another session's work. Now: `enter_worktree` and agent isolation write a `.qwen-session` marker into the worktree at provisioning time; `exit_worktree action='remove'` reads it and refuses if it does not match the current `Config.getSessionId()`. Worktrees from before this guard (no marker file) are treated as "owner unknown" — allowed with a warn log so the change is observable. 4. `enter_worktree` did not refuse nested invocations from inside an existing worktree, contradicting the design doc. Now rejects any cwd containing `.qwen/worktrees/` as a path component, with a clear "Already inside a git worktree…" message. Verified: enter from inside a worktree returns is_error with that text. 6. `hasTrackedChanges` (cleanup sweep) had the same `conflicted[]` gap. Rewrote to use raw `git status --porcelain --untracked-files=no` which lists every tracked change including `UU` conflict markers in a single git call and explicitly skips the untracked walk (the prior comment claimed to skip it, but `status()` always does the scan). Suggestion: 7. `buildWorktreeNotice` now receives the parent agent's actual `getTargetDir()` again (was switched to `repoRoot` in round 3 on a different reviewer's suggestion; round-5 caught that the model's inherited paths reference the parent's cwd, not necessarily the repo root, so the prior behaviour was correct). 8. Startup sweep now does `fs.access(<targetDir>/.qwen/worktrees)` *before* importing GitWorktreeService and spawning `git rev-parse --show-toplevel`. The git probe is reserved for users who actually have a worktrees directory locally — 99% of users pay only one syscall on startup. 9. Tests: - New `exit-worktree.test.ts` covers metadata, validation, `getDefaultPermission` (ask vs allow), and getDescription. - `agent.test.ts` adds three `validateToolParams` cases for the `isolation` parameter (accepted with subagent_type, rejected without, rejected for non-"worktree" values). - `enter-worktree.test.ts` adds round-trip tests for `writeWorktreeSessionMarker` / `readWorktreeSessionMarker` plus a `worktreeBranchForSlug` sanity check. - Total: 101 tests pass (was 86 → +15). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(test): drop unused @ts-expect-error in exit-worktree.test.ts Empty string `''` is a valid `string` type, so the @ts-expect-error directive on `validateToolParams({ name: '', action: 'keep' })` did nothing — TypeScript correctly accepted the line, and `tsc --build` in CI reported TS2578 ("Unused '@ts-expect-error' directive"). The runtime assertion already covers the case; the directive was leftover from an earlier draft. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(test): use importActual in ArenaManager mock to preserve new exports The Arena test mocks `gitWorktreeService.js` with a factory that returns only `{ GitWorktreeService }`. PR #4073 added several other exports to that module (`AGENT_WORKTREE_SLUG_PATTERN`, `WORKTREE_BRANCH_PREFIX`, `worktreeBranchForSlug`, `generateAgentWorktreeSlug`, `writeWorktreeSessionMarker`, `readWorktreeSessionMarker`, `WORKTREE_SESSION_FILE`). Other modules in the dep graph reach the mocked surface — most notably `worktreeCleanup.ts` imports `AGENT_WORKTREE_SLUG_PATTERN` and `worktreeBranchForSlug`, and now reaches the mock via the static `config.ts` → `worktreeCleanup.ts` import chain added in the self-review pass. The Arena test failed at module-load with: Caused by: Error: [vitest] No "AGENT_WORKTREE_SLUG_PATTERN" export is defined on the "../../services/gitWorktreeService.js" mock. Did you forget to return it from "vi.mock"? Use `importOriginal` to capture every real export, spread it into the return object, and only replace `GitWorktreeService` (the class the test actually needs to mock). The class-level mock keeps its existing static-method shims. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address review round 6 (5 critical + 6 suggestions) The biggest item — #1 — is a self-inflicted regression from round 5: the new agent- prefix reservation in `validateUserWorktreeSlug` rejected EVERY slug that `generateAgentWorktreeSlug` produces, since that helper emits exactly `agent-<7hex>`. Net effect: every `AgentTool isolation: 'worktree'` invocation failed at validation. The reservation now allows the canonical pattern through (everything the helper can produce) and only rejects user-chosen `agent-*` names that don't match it. Added a round-trip regression guard: 50 `generateAgentWorktreeSlug()` outputs are fed back through `validateUserWorktreeSlug` and must all pass. Other critical fixes: 2. `hasWorktreeChanges` (used by agent isolation cleanup) was the one remaining caller relying solely on `status.isClean()`. Defensive `|| status.conflicted.length > 0` so a future simple-git bookkeeping change can't let a mid-merge worktree appear clean and get auto-deleted. 3. `readWorktreeSessionMarker` swallowed every I/O error as "marker missing", which let a disk error / EACCES silently bypass the session-ownership guard. ENOENT is still treated as missing (legitimate); every other code now logs. 4. `exit_worktree` `fs.stat` catch was the same shape — every error collapsed to "Worktree not found". ENOENT → not found; everything else logs and returns a distinct "cannot access" error. 5. `cleanupStaleAgentWorktrees` `fs.stat` catch was again the same. ENOENT → silently skip (entry vanished between readdir and stat); everything else logs. Suggestions: 6. Startup sweep fast-bail was running BEFORE resolving the repo top-level. For monorepo subdir launches, `targetDir/.qwen/worktrees` never exists and the sweep early-returned — permanently a no-op. Now resolves the root first, then fast-bails against the resolved `<root>/.qwen/worktrees`. Also logs the skip case so operators can tell "skipped" from "ran, found nothing". 7. `.qwen-session` marker was visible to `git add -A` inside the worktree. Now writes a `.git/info/exclude` rule (resolved via `git rev-parse --git-dir`, since worktree `.git` is a file pointing at the parent repo's `.git/worktrees/<name>/`). Best-effort: failure to write the rule does not abort provisioning. 8. Agent isolation now refuses to provision when the parent's cwd is already inside a worktree — same regex guard as `enter_worktree`. 9. `exit_worktree`'s wrapper around `hasUnmergedWorktreeCommits` now logs at the call site so the chain (caller → reason it asked → underlying git error) is complete in operator logs. 10. Sweep now logs unconditionally at `info`. Three distinct messages: "skipped (no worktrees dir)", "ran, nothing to remove", "removed N". Tests: 11. New `execute()` coverage: • exit-worktree: session-ownership refusal, keep happy path, legacy/no-marker fallthrough with warn log, missing-worktree error, unmerged-commits guard with `discard_changes: true`, `writeWorktreeSessionMarker` round-trip. • enter-worktree: nested-guard rejection, non-git-repo error. These spin up real temp git repos (no filesystem mocking) and drive the actual tool invocation pipeline. Total: 135 tests pass (was 101 → +34). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(worktree): demote noise startup-sweep logs to debug Self-review pass applying the round-6 review-triage framework (filter #5: "If a log only fires on the happy path, it's noise.") to my own round-6 changes: - "Stale worktree sweep skipped: <dir> does not exist" — fires on every CLI start for ~99% of users who never use worktrees. - "Stale worktree sweep ran under <root>: nothing to remove" — fires on every CLI start for users who have any worktrees but no stale ones at the moment. Both are happy-path noise at `info`. Demoted to `debug` so an operator can opt in via `--debug` when they want to confirm the sweep is wired up, but normal output stays clean. Only the actually-actionable case ("removed N worktrees") stays at `info` — that's the signal someone chasing a worktree leak would grep for. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): close AUTO_EDIT bypass + parent-dirty stale-code hazard Round-7 review caught two correctness gaps: 1. exit_worktree action='remove' was still auto-approved in AUTO_EDIT `getDefaultPermission` returning 'ask' is necessary but not sufficient. `permissionFlow.isAutoEditApproved` auto-approves any tool whose `confirmationDetails.type` is 'edit' OR 'info', and `BaseToolInvocation` returns 'info' by default. So a session in AUTO_EDIT could silently destroy a worktree (with branch deletion) without a confirmation prompt — the data-loss path the round-1 `'ask'` switch was meant to close. Now overrides `getConfirmationDetails` to return `type: 'exec'` for action=remove, which keeps the prompt in AUTO_EDIT. The `keep` action still falls through to the base info-type since it is non-destructive. Regression-guard test asserts the type is 'exec' (not 'info') for remove and that the command field describes both the worktree-remove and branch-delete operations. 2. Agent isolation worktrees ran against parent's HEAD, not its working tree `git worktree add -b <branch> <path> <base>` only checks out the base ref's tip — uncommitted edits in the parent's working tree do NOT propagate. The "edit code → ask review/test agent before committing" workflow silently ran the subagent against the pre-edit HEAD and returned results that looked authoritative but reflected stale code. Reviewer offered two options: overlay parent's dirty state à la Arena (~50 LOC, edge cases), or refuse isolation when parent is dirty (~10 LOC, clear UX). Chose the latter for Phase B scope — simpler, decisive, and matches the design-doc's explicit commitment that dirty-state overlay is Arena-specific. Users can commit/stash before re-invoking agent isolation; overlay can be a follow-up if users complain about the friction. Fail-closed on the dirty-check itself (assume dirty rather than silently launch on a possibly-stale tree). Test exercises both "dirty parent → guard fires" and "clean parent → guard passes" against real temp git repos. 139 unit tests pass (was 135, +4 regression guards). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> |