feat(worktree): Phase D — startup --worktree flag + symlinkDirectories + PR refs#4381
Merged
Conversation
Contributor
📋 Review SummaryThis PR implements Phase D of the worktree capability roadmap, adding three cross-cutting features: (1) 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
…s + PR refs Three cross-cutting capabilities on top of the Phase A-C worktree foundation (PRs #4073, #4174). D-1: --worktree [name] CLI flag creates a worktree (or re-attaches to one that already exists) before any model turn runs. Supports bare, plain-slug, `=`, and PR-reference forms; --worktree + --acp rejected with a clear error; --worktree + --resume overrides the resumed session's saved sidecar and emits a stderr line. D-2: worktree.symlinkDirectories: string[] settings key opts into symlinking main-repo directories (e.g. node_modules) into every newly-created general-purpose worktree. Applies to all three creation paths: --worktree flag, EnterWorktreeTool, AgentTool isolation. Path traversal, absolute paths, and existing destinations all guarded; missing source dirs and EEXIST silently skipped (fail-open). D-3: --worktree=#<N> / --worktree <github-url> resolves a PR number, runs `git fetch origin pull/<N>/head` (30s timeout, no `gh` CLI dependency, LANG=C for stable error-taxonomy matching), and creates the worktree off FETCH_HEAD. URL regex tolerates /files, /commits, /checks sub-paths so users can paste any GitHub PR URL. Phase 6 verification fixes also included: - Re-attach to an existing worktree instead of failing with "Worktree already exists" — the common `qwen --resume <sid> --worktree foo` workflow now succeeds. The session ownership marker is preserved on re-attach so cross-session exit_worktree action="remove" still fails for non-owners. - Normalize path-taking argv fields (mcpConfig, jsonSchema @<path>, openaiLoggingDir, jsonFile, inputFile, telemetryOutfile, includeDirectories) to absolute paths against the launch cwd BEFORE the worktree chdir. Otherwise downstream fs.existsSync('./mcp.json') resolves into the worktree, where the file doesn't exist. Phase 7 code-review fixes: - buildStartupWorktreeNotice differentiates "Active worktree" (fresh create) from "Re-attached to worktree" (re-attach path). - Notice survives sidecar persist failure: set before the try block, refreshed inside with override addendum if persist succeeded. - getRegisteredWorktreeBranch verifies the candidate path's git common-dir matches the source repo's — rejects sibling `git init` directories that happen to be on a worktree-<slug> branch. Three-mode parity for the startup notice: TUI consumes via AppContainer effect, headless prepends a <system-reminder> + emits a worktree_started JSON event. ACP path is mutually exclusive with --worktree (ACP hosts supply per-session cwd separately). Tests (66 + 15 new): - 15 cli/src/startup/worktreeStartup.test.ts (slug forms, PR fetch against local fake remote, re-attach happy + wrong-branch guard) - 8 core/src/services/gitWorktreeService.test.ts (parsePRReference: #N, URLs, malformed, traversal, leading zeros, non-string) - 10 core/src/services/gitWorktreeService.symlinks.integ.test.ts (symlink loop + fetchPullRequestRef error taxonomy) Known limitations (documented in docs/users/features/worktree.md): - Cross-slug --resume <sid> --worktree <different-new-slug> is unsupported by design (sessions are bound to projectHash(cwd)); future Config refactor anchoring storage at repo root would lift this. - Mid-session enter_worktree still does NOT switch cwd/targetDir (Phase A's simplification); only the startup --worktree flag does. - yargs ambiguity: `qwen --worktree "say hi"` consumes the prompt as the slug. Quick Start shows the `=` form and reordering workarounds. Docs: - docs/users/features/worktree.md (new): Quick Start with --worktree flag, CLI Reference table for all four input forms + error codes, settings table, Limitations. - docs/design/worktree.md: Phase D section expanded into D-1/D-2/D-3 with open questions resolved; capability table updated. - docs/e2e-tests/worktree-phase-d.md (new): full E2E plan with Phase 4 dry-run baseline + Phase 6 post-impl reproduction tables. Refs #4056
Self-review pass over the Phase D commit (2636f59) catching one real typecheck regression plus a batch of small quality + efficiency improvements. No user-visible behavior change beyond fixing the build. Build fix: - worktreeStartup.ts imports — pre-commit prettier had reorganized `writeWorktreeSession` and `readWorktreeSession` under an `import type { ... }` block, erasing them at compile time (verbatimModuleSyntax). `tsc --noEmit` was failing with TS1361. Bundle path still worked (esbuild is lenient) so this only surfaced when running typecheck. Startup-path efficiency (~10-25 ms saved per --worktree invocation on macOS; more on Windows): - Drop redundant `isGitRepository()` probe — `getRepoTopLevel()` returns null on non-git paths and covers both gates in one subprocess. - Run `getCurrentBranch()` + `getCurrentCommitHash()` in parallel via Promise.all (independent calls). - Combine the two `git rev-parse` probes inside `getRegisteredWorktreeBranch` into a single multi-arg call, and run it in parallel with the source-repo common-dir lookup. Saves one fork+exec on the re-attach path. Quality: - Extract `withReminder()` local helper in nonInteractiveCli.ts so the startup-notice and resume-restore branches share the system-reminder wrapping. - Log `readWorktreeSession` failures in `persistStartupWorktreeSidecar` with the sidecar path so operators can recover the previous slug from a backup. Silent swallow was making "where did my worktree binding go?" undebuggable. - Drop the dead `Config.getWorktreeSettings()` accessor (only `getWorktreeSymlinkDirectories()` has callers); keep the underlying `WorktreeSettings` interface for future fields. - Document the `pendingStartupWorktreeNotice` invariant: at most one consumer per process; ACP path is gated out earlier so only TUI XOR headless reads it. - Add a maintainer note in the gemini.tsx path-normalization block: the argv path-field allowlist is hand-maintained, register new path-bearing flags there or `--worktree` silently breaks for them. - Drop `Phase 6 fix (G1)/(G2)` parenthetical labels from inline comments — internal review-cycle identifiers that decay to noise post-merge. Substantive prose retained. Tests: cli 15/15 (unchanged) + core 66/66 (unchanged); bundle smoke verified fresh / re-attach / invalid slug / non-git cases. Findings deliberately left for follow-up: - Larger refactor extracting a shared `provisionUserWorktree` helper for the EnterWorktreeTool / startup overlap (~80% duplicate). - Splitting the re-attach branch out of `setupStartupWorktree` into its own function. - `isPathWithinRoot` / `isInsideManagedWorktree` shared utils. - `symlinkConfiguredDirectories` loop concurrency (saves 5-15 ms on a cold path that runs only when symlinkDirectories is configured).
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.
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).
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.
…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).
…tion 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.
470679f to
23289d6
Compare
Collaborator
Author
|
Review triage for the github-actions Review Summary (round 1) + the CodeQL alert (round 2). New HEAD: 🟡 High
🟢 Medium
🔵 Low
CodeQL Second order command injection (round 2)
The two Test failures and the empty Code Coverage Summary should clear after CI re-runs against the rebased + CodeQL-fixed HEAD. |
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.
… files Round 4 of the same Phase D-2 mock-drift class. CI surfaced 9 test failures across three files whose `Config` mocks construct `EnterWorktreeTool` for setup but lack the new `getWorktreeSymlinkDirectories` method `createUserWorktree` now calls: - enter-worktree.session.integ.test.ts (2 tests) - exit-worktree.session.integ.test.ts (3 tests) — provisions worktrees via EnterWorktreeTool before exercising exit paths - exit-worktree.test.ts (4 tests) — same provisioning pattern via `provisionWorktree()` and the `makeMockConfig` helper Add a `getWorktreeSymlinkDirectories: () => []` stub to each so the symlink loop is a no-op in tests. `enter-worktree.test.ts` and `agent/agent.test.ts` intentionally skipped — they mock `GitWorktreeService.createUserWorktree` outright, so the method call never fires in their code paths. Adding the stub there would be defensive speculation. If a future test exercises the real path, it'll surface there too and we'll add it then. Local: core tools tests now 123 passed (was 9 failed / 114 passed on CI run 26213122427 against commit 000c9f6).
…in tests
Round 5 of CI: Windows-only test failures on the latest HEAD. Two
unrelated Windows-specific bugs, both in / around worktreeStartup.
1. `setupStartupWorktree` stored the raw `getRepoTopLevel()` output
in `context.repoRoot`. git always emits POSIX paths via
`--show-toplevel` (`C:/Users/...`), so on Windows the value was
forward-slash where `fs.realpath` and `path.join` produce
backslash. The sidecar's `originalCwd` field got the
inconsistent format and a downstream `expect(...).toBe(tempRepo)`
in the round-trip test compared `C:/Users/.../tmp/...` against
`C:\Users\.../tmp/...`.
Wrap the value in `path.resolve()` to normalize to the
platform-native separator before storing. Downstream consumers
(`path.join(session.originalCwd, '.qwen', 'worktrees')` in
`restoreWorktreeContext`, `new GitWorktreeService(originalCwd)`
in `AppContainer`) already handle either format, so no migration
concern for older sidecars.
2. `makeTempRepo` in worktreeStartup.test.ts didn't configure
`core.autocrlf=false`. On Windows runners the default is `true`,
so files committed and pushed to the test's fake-remote `pull/<N>/head`
ref get CRLF-converted on the worktree's checkout. The PR-content
assertion `expect(prFile).toBe('from PR 42\n')` then failed with
`'from PR 42\r\n'`.
Add `core.autocrlf=false` + `core.eol=lf` to the temp-repo setup
so test files round-trip byte-for-byte regardless of host platform.
Local mac: cli worktreeStartup 15/15 still pass. Windows verification
deferred to CI.
Contributor
There was a problem hiding this comment.
Pull request overview
Implements Phase D of the generic worktree roadmap by wiring worktrees into CLI startup (--worktree), adding opt-in dependency sharing via worktree.symlinkDirectories, and supporting PR references (#<N> / GitHub PR URL) by fetching refs/pull/<N>/head from origin.
Changes:
- Add
--worktreestartup flow that creates or re-attaches to a managed worktree,chdirs before config load, and persists a WorktreeSession sidecar + one-shot notice. - Add
worktree.symlinkDirectoriessetting and propagate it throughenter_worktree, agent worktree isolation, and startup worktree creation. - Add PR reference parsing + fetch support in
GitWorktreeServiceand document the end-to-end workflow and limitations.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/vscode-ide-companion/schemas/settings.schema.json | Expose worktree.symlinkDirectories in the generated settings schema. |
| packages/core/src/tools/exit-worktree.test.ts | Update test Config mocks for new getWorktreeSymlinkDirectories() dependency. |
| packages/core/src/tools/exit-worktree.session.integ.test.ts | Update integration test Config mock for symlink settings accessor. |
| packages/core/src/tools/enter-worktree.ts | Pass symlinkDirectories into createUserWorktree() from Config. |
| packages/core/src/tools/enter-worktree.session.integ.test.ts | Update integration test Config mock for symlink settings accessor. |
| packages/core/src/tools/agent/agent.ts | Pass symlinkDirectories into agent isolation worktree creation. |
| packages/core/src/services/gitWorktreeService.ts | Add PR reference parsing/fetch, worktree registration probe, and symlink loop; extend createUserWorktree() options. |
| packages/core/src/services/gitWorktreeService.test.ts | Unit tests for parsePRReference(). |
| packages/core/src/services/gitWorktreeService.symlinks.integ.test.ts | New integration coverage for symlink behavior + PR-fetch error taxonomy. |
| packages/core/src/config/config.ts | Add WorktreeSettings, plumb worktree config, and implement startup notice stash/consume + symlink accessor. |
| packages/cli/src/ui/AppContainer.tsx | Prefer startup worktree notice over resume-based restore in TUI, and surface it in transcript + prompt injection. |
| packages/cli/src/startup/worktreeStartup.ts | New startup helper to resolve/create/reattach worktrees, fetch PR refs, and persist sidecar + notice message. |
| packages/cli/src/startup/worktreeStartup.test.ts | New tests covering startup flag behavior, PR refs via local remote, and re-attach semantics. |
| packages/cli/src/nonInteractiveCli.ts | Headless prompt injection: startup notice takes precedence over resume restore; emit worktree_started event. |
| packages/cli/src/nonInteractiveCli.test.ts | Update Config mock for consumePendingStartupWorktreeNotice(). |
| packages/cli/src/gemini.tsx | Startup ordering: normalize path args pre-chdir, enforce --worktree × ACP mutex, invoke startup worktree setup and persist notice/sidecar. |
| packages/cli/src/config/settingsSchema.ts | Add worktree settings namespace and symlinkDirectories field. |
| packages/cli/src/config/config.ts | Add --worktree CLI arg, and plumb worktree.symlinkDirectories into core Config params. |
| docs/users/features/worktree.md | New user-facing documentation for worktrees including --worktree, PR refs, symlink setting, and limitations. |
| docs/users/features/_meta.ts | Add Worktrees page to docs nav. |
| docs/e2e-tests/worktree-phase-d.md | Add comprehensive Phase D E2E test plan + reproduction notes. |
| docs/design/worktree.md | Update design doc to reflect Phase D completion (startup flag, symlinks, PR refs) and precedence/limitations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two Copilot findings on symlinkConfiguredDirectories (PR #4381 round 3): 1. The settingsSchema description, docs/users/features/worktree.md, and WorktreeSettings JSDoc all promise that entries containing `..` are rejected — but the post-resolve isWithinRoot check accepted `foo/../bar` (resolves to `bar`, inside the repo). Add a literal `..` segment check before path.resolve so the code matches the contract. 2. On Windows, fs.symlink(..., 'dir') requires SeCreateSymbolicLinkPrivilege (admin / Developer Mode) and EPERMs on default consumer installs. Use 'junction' for directory entries on win32 — junctions are reparse points that achieve the same semantics without elevation. Keep 'dir' on POSIX and 'file' for non-directory sources (no junction-equivalent for files; rare path). Adds an integration test exercising `foo/../bar` to lock in the syntactic guard; existing absolute-path and traversal tests already covered the other rejection forms.
wenshao
requested changes
May 21, 2026
Three findings from wenshao round 4 (PR #4381): 1. For --worktree=#42 (PR worktrees), originalHeadCommit was captured from the parent repo's HEAD via getCurrentCommitHash() — but the worktree branches off FETCH_HEAD (the PR tip), not main. Downstream, WorktreeExitDialog's `rev-list <originalHeadCommit>..HEAD` would count every commit in the fetched PR as "new work this session" alongside the user's actual commits. Same root cause covers the FETCH_HEAD TOCTOU window: between `git fetch origin pull/<N>/head` and `git worktree add ... FETCH_HEAD`, a concurrent `git fetch` from any other process sharing this repo could overwrite .git/FETCH_HEAD, causing the worktree to branch off an unrelated commit. Fix: add GitWorktreeService.resolveRef(ref) that returns a 40-char SHA (or null). In setupStartupWorktree, immediately after fetchPullRequestRef succeeds, resolve FETCH_HEAD to an immutable SHA; pass that SHA both as the baseRef to createUserWorktree (closes the TOCTOU) AND as originalHeadCommit in the returned context (closes the exit-dialog miscount). Fail-close on null resolve. 2. Orphaned JSDoc block at gitWorktreeService.ts:1035-1048 — originally wrote validateUserWorktreeSlug's docs, stranded above parsePRReference after that function was inserted between them. Move the block down to sit immediately above validateUserWorktreeSlug at its current line. 3. `.git` / `.qwen` symlink rejection guards (~20 lines of security- critical code at gitWorktreeService.ts:1640-1655) had no regression tests — only absolute paths, `..` traversal, isWithinRoot escapes, and missing sources were covered. Add two integ tests in gitWorktreeService.symlinks.integ.test.ts: one asserts `.git/hooks` is refused, one asserts `.qwen/projects` is refused. Also extends the existing PR-worktree integration test in worktreeStartup.test.ts to assert originalHeadCommit equals the resolved FETCH_HEAD SHA AND does NOT equal the parent repo's main HEAD — the assertion would fail loudly if the new SHA-capture path were reverted.
Collaborator
Author
Round 4 round-up — commit 6e1b62e@wenshao Thanks for the review pass — disposition of all 8 findings: ✅ Fixed
❌ Declined as overthinking (round-weighted bar, round 4)
|
wenshao
reviewed
May 25, 2026
Collaborator
Author
|
@wenshao The latest finding on All 13 review threads on this PR are now resolved/replied, CI is fully green, and the previous round's |
wenshao
reviewed
May 25, 2026
6 tasks
Collaborator
Author
Round 6 round-up — no code changes@wenshao Thanks for the pass. After verifying each finding against current code, all 6 are real but classified as overthinking at round 6 per the round-weighted decline bar — none gate correctness, all sit on already-correct code. Filed #4529 to track them as a single follow-up:
This PR has reached 6 review rounds, ~25 accepted findings, with the last 3 rounds producing increasingly marginal nits on code that's been green across all CI dimensions for 4 days. Per the skill source lessons (PRs #4151 / #4168), round-6+ Suggestions without escalation default to overthinking + follow-up issue rather than bundled-in commits. CI: green. Threads: 19/19 resolved. The PR is ready to merge once the round-4 |
wenshao
requested changes
May 27, 2026
Security fix from PR #4381 round 7 (wenshao/qwen3.7-max). The lexical isWithinRoot + .git/.qwen blocklist checks in symlinkConfiguredDirectories all operated on path.resolve(repoRoot, raw) — a STRING operation that doesn't follow symlinks. A committed (or out-of-band) symlink at <repo>/node_modules pointing into .git would pass every gate: 1. path.resolve gives `<repo>/node_modules` (lexical, passes isWithinRoot against repo root). 2. The .git/.qwen blocklists also see the lexical path — they don't detect that the realpath chains into .git. 3. fs.stat() follows the symlink and succeeds against .git/. 4. fs.symlink writes `<worktree>/node_modules → <repo>/node_modules`, which OS-side resolves through to <repo>/.git. Any tool inside the worktree that writes to node_modules/hooks/post-merge then has RCE on the next hook-firing git operation. Fix: after fs.stat succeeds, fs.realpath the source and RE-RUN the three containment checks against the realpath. Refuse on any escape. Use the realpath (not the lexical sourceAbs) as the symlink target so the new link is one-hop canonical rather than preserving the chain. Also closes the dest-side variant of the same root cause — flagged in round 4 thread #5 (declined then as overthinking) but now in scope per the skill's iteration rule (two consecutive rounds raising the same root-cause class). path.join(worktreePath, raw) is also lexical: if git worktree add materialized a committed worktree-level symlink (e.g. HEAD ships tools → /etc), then fs.mkdir / fs.symlink for a nested entry like "tools/cache" writes OUTSIDE the worktree. Realpath the dest parent before mkdir and refuse if it escapes the worktree. New integ test covers both source-side variants (escape-to-git via out-of-band symlink + escape-to-outside-dir) in one block. Was RED against the pre-fix code: <wt>/escape-to-git was created as a symlink that chained into the source repo's .git. GREEN after the fix.
Collaborator
E2E 验证报告 — PR #4381环境：Debian GNU/Linux 13 (trixie) · Linux 6.12 x86_64 · Node v22.22.2 · tmux 3.5a · 在新 worktree PR 描述中的"在 macOS 上验证 / Linux 未跑"这条留白，本报告在 Linux 下补齐。除单元/集成测试外，所有场景均通过 本地 tmux + 真实命令行 + 磁盘断言 完成，不依赖任何 mock。 1. 单元 / 集成测试
2. E2E 场景测试根目录： 2.1
|
| 条目 | 期望 | 实际 |
|---|---|---|
node_modules |
symlink → 源 | lrwxrwxrwx node_modules -> /tmp/pr4381-tests/symlink/node_modules ✅ |
scripts |
symlink → 源 | lrwxrwxrwx scripts -> /tmp/pr4381-tests/symlink/scripts ✅ |
../etc、/etc |
拒绝 | 未在 worktree 内出现 ✅ |
.git、.qwen、.qwen/worktrees |
拒绝 | 未在 worktree 内出现 ✅（.git 是 git 自己写的 ASCII 指针文件，非我们创建的 symlink） |
missing-dir |
静默跳过 | 未在 worktree 内出现 ✅ |
✅ 黑名单逻辑（绝对、..、.git、整个 .qwen 树）与 PR 描述完全一致。
2.4 --worktree=#<N> 与 --worktree=<github-pr-url>
新 shallow clone git@github.com:QwenLM/qwen-code.git，无任何 pull/* ref。
$ node $QWEN --worktree '#4544' ...
worktree_started: [Startup] Active worktree: "pr-4544" at .../.qwen/worktrees/pr-4544 (branch: worktree-pr-4544).
$ git worktree list
.../pr-ref-test/.qwen/worktrees/pr-4544 7f068731 [worktree-pr-4544]
$ gh pr view 4544 --json headRefOid -q .headRefOid
7f06873157ac75a9d13be7b7d1bb680dbf91a533 ← 完全一致 ✅
$ node $QWEN --worktree 'https://github.com/QwenLM/qwen-code/pull/4540' ...
NOTICE: [Startup] Active worktree: "pr-4540" ... (branch: worktree-pr-4540)
HEAD = 482e0a1f... ← 与 GitHub PR head OID 完全一致 ✅
负向：
$ node $QWEN --worktree '#999999' ...
--worktree: Failed to fetch PR #999999: the PR does not exist on origin,
or origin is not a GitHub repository (only GitHub exposes refs/pull/<N>/head).
REAL_EXIT=1
✅ git fetch origin pull/<N>/head 路径走通；无 gh CLI 依赖；错误文案精准。
2.5 path-arg 在 chdir 前归一化（gemini.tsx 白名单）
测试目录：/tmp/pr4381-tests/pathnorm/，里面有 extra-files/note.txt。
$ node $QWEN --worktree feat --include-directories ./extra-files ...
init.cwd = /tmp/pr4381-tests/pathnorm/.qwen/worktrees/feat
TOOL_USE: list_directory {'path': '/tmp/pr4381-tests/pathnorm/extra-files'} ← 绝对路径 = launch cwd
TOOL_RESULT: Listed 1 item(s)
对照断言：
/tmp/pr4381-tests/pathnorm/extra-files/note.txt存在/tmp/pr4381-tests/pathnorm/.qwen/worktrees/feat/extra-files不存在
✅ 如果归一化没在 chdir 前发生，相对路径 ./extra-files 就会落到 worktree 下、找不到。这反向证明白名单生效，与 PR 描述里 "resolve mcpConfig, jsonFile, inputFile, telemetryOutfile, openaiLoggingDir, jsonSchema @<path>, includeDirectories against launch cwd BEFORE chdir" 一致。
3. Phase C 退出对话框
/quit 后 WorktreeExitDialog 正常弹出，三项选择（Keep / Remove / Cancel）渲染正常，"Keep" 选项保留 worktree 与分支，stats panel 正常显示。Phase D 没有破坏 Phase C 路径。
4. 结论
- 单元 + 集成：85 / 85 通过，typecheck clean
- 6 大端到端场景：全部通过，含 PR 描述明确指出的
originalHeadCommit、symlink 黑名单、path-arg 归一化等关键不变量 - 跨平台留白：本报告在 Linux x86_64 / Node 22.22 下补齐，无任何平台依赖问题
- 没有发现 regression；从 maintainer 视角，建议 merge
Out-of-scope 风险（PR 已自述，本次未复测）：跨 slug 的 --resume × --worktree 组合（Group C / G1，session bound to projectHash(cwd) 这条架构约束）。
完整测试痕迹：tmux session pr4381，磁盘工件保留在 /tmp/pr4381-tests/。
wenshao
requested changes
May 27, 2026
wenshao
previously approved these changes
May 27, 2026
Round-7's source-side realpath fix introduced a canonical-vs-lexical mismatch: `repoRootAbs = path.resolve(this.sourceRepoPath)` is purely lexical, while `realSource = await fs.realpath(sourceAbs)` is canonical. On macOS where `/tmp → /private/tmp` and `/var → /private/var` are ubiquitous, and on any Linux/Windows setup where the user's checkout sits behind a symlink, the prefixes diverge at the symlink boundary and `isWithinRoot(realSource, repoRootAbs)` silently rejects every configured entry. Production callers (worktreeStartup.ts, EnterWorktreeTool, agent isolation) all pass the lexical path returned by `git rev-parse --show-toplevel`. The integ tests masked the bug because the shared `beforeEach` did `repoRoot = await fs.realpath(dir)` upfront. Round 8 fix: - Hoist `repoRootAbs`, `gitDirAbs`, `qwenDirAbs`, and `realWorktreePath` outside the for-loop — they're loop invariants and were being recomputed once per entry. - `await fs.realpath(this.sourceRepoPath)` for `repoRootAbs` so every containment check below is canonical-vs-canonical. The derived `gitDirAbs` / `qwenDirAbs` blocklist paths inherit the canonical prefix automatically. `sourceAbs = path.resolve(repoRootAbs, raw)` inherits it too, so the early lexical reject paths (absolute, `..`, repo-root equality, isWithinRoot) stay self-consistent. - Fail-close: if the repo root itself doesn't realpath (deleted / inaccessible), bail out of the entire symlink loop rather than continuing with comparisons we can't trust. Non-destructive — the worktree was created earlier by `git worktree add`. New integ test provisions the production shape: a symlink path used as `sourceRepoPath`, distinct from its canonical realpath. RED on the pre-fix code (assertion fired with "symlinkDirectories entry was silently rejected — canonical vs lexical isWithinRoot mismatch"), GREEN after.
Collaborator
Author
|
@wenshao Thanks for the Linux E2E validation — that closes the macOS-only-tested caveat I called out in the PR description. The inline finding posted right after it (canonical-vs-lexical Good catch — and worth noting your Linux E2E run wouldn't have caught this directly either, since the symlink-boundary case only fires when |
wenshao
approved these changes
May 27, 2026
pomelo-nwu
approved these changes
May 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What changed: Phase D of the worktree capability roadmap (refs Phase A+B in feat(tools): add generic worktree support — EnterWorktree/ExitWorktree + Agent isolation #4073, Phase C in feat(worktree): Phase C — session persistence, hooksPath, Footer + WorktreeExitDialog, three-mode --resume restore #4174). Three cross-cutting features:
--worktree [name]CLI startup flag — creates a worktree (or re-attaches to an existing one) andprocess.chdirs the entire session into it before any model turn runs. Supports bare (auto-slug), explicit slug,=form, PR-reference forms (#<N>/ GitHub URL). Mutually exclusive with--acp. Re-attach is registration-checked against the source repo (rejects plain dirs that walked up to outer.git).worktree.symlinkDirectories: string[]settings key — opt-in symlinking of main-repo directories (e.g.node_modules) into every newly-created general-purpose worktree (covers--worktreeflag,enter_worktreetool, ANDagent isolation: "worktree"paths). Path traversal, absolute paths, the whole.gittree, and the whole.qwentree are rejected; missing source dirs and EEXIST destinations are silently skipped.--worktree=#<N>/ full GitHub PR URL —git fetch origin pull/<N>/head(30s timeout, noghCLI dependency,LANG=Cfor stable error-taxonomy), thengit worktree add -b worktree-pr-<N> <path> FETCH_HEAD.Why it changed: Phases A-C shipped the mid-session worktree primitives (tools, agent isolation, session persistence, Footer / dialog UI). Users still had to manually
git worktree addoutside the CLI when they wanted to START a session inside a worktree, and there was no opt-in for dependency sharing or PR-ref worktrees. D wires the three remaining capabilities into the startup path. Multiple self-review rounds caught and fixed a handful of real correctness bugs along the way — most notably wrong baseline for the exit dialog on re-attach, a plain-directory false re-attach, and a.qwenparent-directory bypass of the symlink blocklist.Reviewer focus:
packages/cli/src/startup/worktreeStartup.ts— the new startup helper. Slug resolution, PR fetch, re-attach branch, single-rev-parse probe,--worktree×--resumeprecedence.packages/core/src/services/gitWorktreeService.ts— new public methodsparsePRReference,fetchPullRequestRef,getRegisteredWorktreeBranch, plus the privatesymlinkConfiguredDirectoriesloop with.git/.qwenblocklist.packages/cli/src/gemini.tsx— startup ordering: path-arg normalization (resolvemcpConfig,jsonFile,inputFile,telemetryOutfile,openaiLoggingDir,jsonSchema @<path>,includeDirectoriesagainst launch cwd BEFORE chdir),--worktree×--acpmutex,setupStartupWorktreecall,persistStartupWorktreeSidecar+ first-prompt notice plumbing.packages/cli/src/ui/AppContainer.tsx+packages/cli/src/nonInteractiveCli.ts— both consumeConfig.consumePendingStartupWorktreeNoticeand skip the Phase CrestoreWorktreeContextpath when startup already injected a notice. ACP is mutex-rejected earlier so no consumer there.Validation
docs/e2e-tests/worktree-phase-d.md(24 cases / 7 groups covering A–G)..qwen/worktrees/foo+ branchworktree-foo, emits[Startup] Active worktree…system event; second invocation re-attaches and emits[Startup] Re-attached to worktree….originalHeadCommitin the second run's sidecar correctly captures the worktree's HEAD (not the launch cwd's HEAD), soWorktreeExitDialog's commit count means "new commits THIS session" rather than "everything since launch HEAD".docs/e2e-tests/worktree-phase-d.md— 22/24 cases passed cleanly; 2 cases (C1/C2 cross-slug session override) are a documented architectural limitation, not a bug.Scope / Risk
projectHash(process.cwd()).--worktree Xand--resume <sid>where<sid>was created from a different cwd will fail to find the session (exit 1, no session lookup wires up). Documented indocs/users/features/worktree.md(Limitations) anddocs/design/worktree.md(precedence table). A future Config refactor anchoring storage at repo root would lift this; out of scope here.gemini.tsxis hand-maintained — adding a new CLI flag that takes a relative path WITHOUT adding it to the allowlist will silently break under--worktree(chdir resolves the path against the worktree where the file doesn't exist). A maintainer comment flags this; longer-term a yargs metadata extension would centralize it.--resume × --worktreescenarios — architectural limitation as noted.simple-git,fs/promises, andexecFile(all platform-agnostic). The macOS/var→/private/varrealpath canonicalisation IS handled ingetRegisteredWorktreeBranch.enter_worktree/exit_worktree/ Agent isolation worktree callers keep working;createUserWorktreegained an optional thirdoptionsparameter that is back-compatible. New settings (worktree.symlinkDirectories) default toundefined— no behavior change unless the user opts in.Testing Matrix
Testing matrix notes:
simple-git,fs/promises,fs.realpath, andexecFile— all platform-agnostic. No platform-specific code paths added beyond the existing macOS realpath canonicalisation ingetRegisteredWorktreeBranch.Linked Issues / Bugs
Refs #4056 (delivers Phase D of the worktree roadmap; Phase A+B shipped in #4073, Phase C in #4174). Future items (sparse checkout,
.worktreeinclude, tmux integration) tracked indocs/design/worktree.md.