feat(worktree): Phase C — session persistence, hooksPath, Footer + WorktreeExitDialog, three-mode --resume restore#4174
Merged
Merged
Conversation
- 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>
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>
- 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>
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>
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>
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>
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>
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>
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>
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>
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>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- 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>
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>
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>
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>
Contributor
📋 Review SummaryThis PR implements Phase C of the worktree capability, adding session persistence, hooks configuration, UI visibility, and exit dialogs for git worktrees. The implementation is thorough with comprehensive test coverage (92 core tests + 25 CLI tests passing), clean abstraction through 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
…l-b6e48c # Conflicts: # packages/cli/src/acp-integration/acpAgent.ts
…-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>
Collaborator
Author
|
Triage of the automated review — applied the review-response five-filter framework. Two findings accepted (doc-only), one false-positive, six declined with rationale. Pushed Per-finding outcomes
Rationale for declines#1 handleExit deps — #2 fs.watch — the reviewer suggested "watch the parent directory instead". Re-reading #3 maybeClearWorktreeSession in 3 sites — the three call-sites are the three SUCCESS return paths. Five earlier #4 execGit stderr — the dialog is interactive; on git failure we default to #6 ⎇ Unicode — Footer already uses Unicode glyphs throughout ( #7 originalHeadCommit type — empty-string sentinel chosen for JSON-write friendliness ( #8 spacing inconsistency — TUI uses What changed in
|
Collaborator
Author
E2E Test Report — Phase CFull end-to-end validation against Test plan
Run matrix
Two regressions caught by the harnessBug 1 (Group E 0/5 → 5/5) — Scope gap (Group C-headless 0/2 → 2/2) — Phase C task 7's Evidence highlightsGroup A1 — sidecar from {
"slug": "a1-test",
"worktreePath": "/tmp/.../.qwen/worktrees/a1-test",
"worktreeBranch": "worktree-a1-test",
"originalCwd": "/tmp/...",
"originalBranch": "main",
"originalHeadCommit": "d4eca75fe964f30a974e5a1e4f5184502e934047"
}Group B3 — Group D1 — Footer capture: Group E2 — Dialog body for a dirty worktree (1 new commit + 1 uncommitted file): Group F2 — JSON payload captured by a custom statusline command via stdin: "worktree": {
"name": "f2-test",
"path": ".../.qwen/worktrees/f2-test",
"branch": "worktree-f2-test",
"original_cwd": ".../qwen-wt-phc-F2-XXXXXX",
"original_branch": "main"
}Built-in C-headless TC1 — {
"type": "system",
"subtype": "worktree_restored",
"data": {
"slug": "c1head",
"path": ".../.qwen/worktrees/c1head",
"branch": "worktree-c1head"
}
}Unit test coverage (collocated)Run as a one-liner regression net: cd packages/core && npx vitest run \
src/services/worktreeSessionService.test \
src/services/gitWorktreeService.test \
src/services/gitWorktreeService.hooks.integ.test \
src/tools/enter-worktree.test \
src/tools/enter-worktree.session.integ.test \
src/tools/exit-worktree.test \
src/tools/exit-worktree.session.integ.test
# → 7 test files, 84 passed
cd packages/cli && npx vitest run \
src/ui/components/Footer.test \
src/ui/components/DialogManager.test \
src/acp-integration/acpAgent.worktree.test \
src/acp-integration/session/Session.worktree.test
# → 4 test files, 25 passedLimitations
|
CI surfaced TypeError: config.getResumedSessionData is not a function across 12 tests in nonInteractiveCli.test.ts. The Phase C ada0837 commit added a worktree-restore call in the headless path that probes config.getResumedSessionData(); the mock Config never had that method. Return undefined to short-circuit the restore block — these tests don't exercise --resume. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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. |
This was referenced May 15, 2026
Closed
wenshao
requested changes
May 16, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Test Coverage Gaps
The following new code paths have zero test coverage and need tests before merge:
WorktreeExitDialog.tsx(189 new lines): no unit/integration/ACP test exists for loading state, dirty-state display, escape key handler, radio choices, useEffect cleanup withcancelledflag, ororiginalHeadCommitempty branch.useWorktreeSession.ts(80 new lines): no test forload(),fs.watchwatcher,cancelledflag, orfs.mkdirfallback.nonInteractiveCli.ts:375-408: headless--resumeworktree restore is stubbed out in existing tests (mockReturnValue(undefined)). No test verifies<system-reminder>injection oradapter.emitSystemMessage('worktree_restored', ...).AppContainer.tsxworktree changes (~100 lines): historically untested file;handleWorktreeExit,useWorktreeSessionconsumption, and--resumerestore path are uncovered.
| setShowWorktreeExitDialog(false); | ||
| if (choice === 'remove' && activeWorktree) { | ||
| try { | ||
| const svc = new GitWorktreeService(config.getTargetDir()); |
Collaborator
There was a problem hiding this comment.
[Critical] handleWorktreeExit constructs GitWorktreeService with config.getTargetDir(), but worktrees are created under the repo root (getRepoTopLevel()). When the CLI is launched from a subdirectory (e.g., a monorepo package), paths won't match and removeUserWorktree will silently fail. activeWorktree.originalCwd already stores the correct repo root.
Suggested change
| const svc = new GitWorktreeService(config.getTargetDir()); | |
| const svc = new GitWorktreeService(activeWorktree.originalCwd); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in c0c0e71. Anchor GitWorktreeService at activeWorktree.originalCwd (captured repo top-level) instead of config.getTargetDir(). Monorepo subdir launches now resolve the worktree correctly.
| if (choice === 'remove' && activeWorktree) { | ||
| try { | ||
| const svc = new GitWorktreeService(config.getTargetDir()); | ||
| await svc.removeUserWorktree(activeWorktree.slug, { |
Collaborator
There was a problem hiding this comment.
[Critical] The remove path has three gaps: (1) removeUserWorktree return value {success, error} is never checked — if removal fails, clearWorktreeSession still runs, orphaning the worktree. (2) forceDeleteBranch is not passed, so branches with unmerged commits are silently preserved contrary to the dialog warning. (3) The entire code path bypasses ExitWorktreeTool's three-guard safety sequence (session ownership, uncommitted changes gate, unmerged commits gate).
Suggested change
| await svc.removeUserWorktree(activeWorktree.slug, { | |
| const result = await svc.removeUserWorktree(activeWorktree.slug, { | |
| deleteBranch: true, | |
| forceDeleteBranch: true, | |
| }); | |
| if (!result.success) { | |
| // Keep the sidecar so --resume can recover the worktree. | |
| return; | |
| } | |
| await clearWorktreeSession(/* ... */); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed parts (1) and (2) in 38db00f. Now checks removeUserWorktree's {success} return and only clears the sidecar on success — if removal fails the sidecar stays so --resume can recover. Also passes forceDeleteBranch: true to honour the dialog's "discards N commits" label. Part (3) declined as design: the dialog IS the safety affordance for this path — it surfaces the dirty-state + commit counts before the user confirms, equivalent to the tool's discard_changes: true opt-in.
| const handleWorktreeExit = useCallback( | ||
| async (choice: 'keep' | 'remove' | 'cancel') => { | ||
| if (choice === 'cancel') { | ||
| setShowWorktreeExitDialog(false); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The keep path does not call clearWorktreeSession, while exit_worktree tool's action='keep' calls maybeClearWorktreeSession. On --resume, the stale sidecar re-injects the worktree context reminder even though the user already chose to exit.
Suggested change
| setShowWorktreeExitDialog(false); | |
| if (choice === 'keep') { | |
| if (activeWorktree) { | |
| await clearWorktreeSession( | |
| config | |
| .getSessionService() | |
| .getWorktreeSessionPath(config.getSessionId()), | |
| ); | |
| } | |
| handleSlashCommand('/quit'); | |
| return; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Declined-design. Dialog Keep ≠ tool Keep. exit_worktree action='keep' is invoked mid-session when the user wants to leave the worktree context (sidecar cleared, footer indicator off). Dialog Keep is invoked at app-exit when the user wants to come back to this worktree on the next --resume (sidecar preserved, --resume re-injects the context reminder). The dialog label "Keep worktree (exit without deleting)" supports the preserve semantics; clearing here would silently break --resume restoration for the most common case.
| // count new commits created inside the worktree. Empty string when | ||
| // rev-parse fails (e.g. unborn HEAD) — the dialog treats empty as | ||
| // "unknown" and skips the commit-count display. | ||
| let originalHeadCommit = ''; |
Collaborator
There was a problem hiding this comment.
[Critical] originalHeadCommit is always captured via getCurrentCommitHash() (which runs git rev-parse HEAD on the main repo), regardless of the base_branch parameter. If the user is on main but specifies base_branch='develop', the captured commit is main's HEAD, not develop's. The dialog then computes an inflated commit count. Use the base branch's tip instead.
Suggested change
| let originalHeadCommit = ''; | |
| let originalHeadCommit = ''; | |
| try { | |
| const baseRef = baseBranch ?? 'HEAD'; | |
| originalHeadCommit = (await service.git.raw(['rev-parse', baseRef])).trim(); | |
| } catch (error) { |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
False-positive (Filter 1). There is no base_branch parameter in EnterWorktreeTool (see EnterWorktreeParams at enter-worktree.ts:20). baseBranch is set from service.getCurrentBranch() which is the same commit as service.getCurrentCommitHash() (both read the main repo's HEAD), so originalHeadCommit is equivalent to the base branch's tip at creation time. The dialog's rev-list --count <originalHeadCommit>..HEAD inside the worktree therefore correctly counts commits made IN the worktree.
| session: WorktreeSession, | ||
| ): Promise<void> { | ||
| await fs.mkdir(path.dirname(filePath), { recursive: true }); | ||
| await fs.writeFile(filePath, JSON.stringify(session, null, 2), 'utf-8'); |
Collaborator
There was a problem hiding this comment.
[Critical] writeWorktreeSession uses a direct fs.writeFile() — not atomic. If the process crashes mid-write, the sidecar file is truncated or contains partial JSON. readWorktreeSession throws SyntaxError from JSON.parse, which all consumers silently degrade to "no active worktree". Use the project's existing atomicWriteJSON utility.
Suggested change
| await fs.writeFile(filePath, JSON.stringify(session, null, 2), 'utf-8'); | |
| import { atomicWriteJSON } from '../utils/atomicFileWrite.js'; | |
| // inside writeWorktreeSession: | |
| await fs.mkdir(path.dirname(filePath), { recursive: true }); | |
| await atomicWriteJSON(filePath, session); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in 38db00f. writeWorktreeSession now uses atomicWriteJSON (write-to-temp + rename), so a crash mid-write can no longer leave a truncated sidecar.
| const gitHooksPath = path.join(this.sourceRepoPath, '.git', 'hooks'); | ||
| let hooksPath: string | null = null; | ||
| for (const candidate of [huskyPath, gitHooksPath]) { | ||
| try { |
Collaborator
There was a problem hiding this comment.
[Critical] The fs.stat loop in configureHooksPath catches ALL errors identically, treating them as "candidate not found". If .husky/ exists but fs.stat fails with EACCES (permission), EIO, or any transient filesystem error, the function silently falls through. If .git/hooks/ is also inaccessible, NO hooks are configured and worktree commits silently bypass all main-repo hooks.
Suggested change
| try { | |
| } catch (error) { | |
| if (!isNodeError(error) || error.code !== 'ENOENT') { | |
| debugLogger.warn( | |
| `configureHooksPath: cannot stat ${candidate}: ${error}`, | |
| ); | |
| } | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in 38db00f. configureHooksPath now distinguishes ENOENT (the expected "this candidate isn't present" signal) from any other code (EACCES/EIO/ENOTDIR). The latter are warn-logged via debugLogger.warn so a silently-degraded hooksPath is visible to operators.
| // Watch the parent dir so create/delete/rename events on the | ||
| // sidecar (which may not exist at mount time) are caught. | ||
| watcher = fs.watch(dirPath, (_eventType, filename) => { | ||
| if (filename === fileName) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] fs.watch on Linux returns filename as a Buffer when no encoding is specified. The comparison filename === fileName (string) always evaluates to false, so the watcher silently never fires on Linux. The Footer indicator won't react to enter_worktree / exit_worktree calls.
Suggested change
| if (filename === fileName) { | |
| if (filename?.toString() === fileName) { |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in 38db00f. fs.watch callback now normalizes filename via toString() so the Linux Buffer code path triggers reloads (previously the === fileName string comparison silently never matched). Also treats null filename as "unknown, reload to be safe" — recursive watchers on some platforms emit events without a payload.
| args: string[], | ||
| cwd: string, | ||
| ): Promise<{ stdout: string; code: number }> { | ||
| return new Promise((resolve) => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] execGit always resolves (never rejects), and on error returns {stdout: '', code: 1}. Callers only read stdout. If git status --porcelain or git rev-list --count fails (e.g., corrupt index, git not in PATH), the dialog silently shows "0 files, 0 commits" instead of surfacing the error. The user may choose "remove" thinking nothing is at risk.
Expose the error state so the dialog can warn the user when git commands fail.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in 38db00f. The dialog now tracks a probeError state; when git status or git rev-list returns non-zero or errors at spawn level, the dialog renders an ⚠ Could not measure worktree state (...) banner with the actual exit code or errno. Users see the failure explicitly before choosing Remove.
| * | ||
| * Shared by TUI / headless / ACP entry points so all three behave | ||
| * consistently on `--resume`. Failures are logged via the supplied | ||
| * `onWarn` callback but never thrown — worktree restore is best-effort, |
Collaborator
There was a problem hiding this comment.
[Suggestion] When readWorktreeSession throws due to corrupted JSON (SyntaxError), restoreWorktreeContext returns nulls but does NOT delete the corrupted sidecar file. Every subsequent --resume will repeat the same parse error and silently fail to restore worktree context. The stale-directory cleanup path correctly calls clearWorktreeSession, but the corrupted-JSON path does not.
Suggested change
| * `onWarn` callback but never thrown — worktree restore is best-effort, | |
| } catch (error) { | |
| onWarn?.(error); | |
| // Clean up corrupted sidecar so it doesn't block every --resume. | |
| try { await clearWorktreeSession(sidecarPath); } catch {} | |
| return { contextMessage: null, session: null }; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in 38db00f. restoreWorktreeContext now calls clearWorktreeSession when readWorktreeSession returns null (malformed JSON / missing fields / wrong types) AND when reading throws a non-ENOENT I/O error. Subsequent --resume calls no longer keep tripping on the same broken file.
wenshao
requested changes
May 17, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Additional findings (not inline-mappable):
-
[Critical]
handleWorktreeExitremove path bypassesExitWorktreeToolsafety guards (AppContainer.tsx:1105-1132): The dialog's "Remove" path callsremoveUserWorktree()directly without the dirty-state, unmerged-commit, or session-ownership checks thatExitWorktreeTool.execute()enforces. A user who clicks "Remove" on a dirty worktree permanently loses uncommitted changes. Consider factoring the safety guards into a shared function. -
[Suggestion]
loadDirtyStatedoesn't check git exit code (WorktreeExitDialog.tsx:88-110): Ifgit status --porcelainfails (e.g., worktree deleted between dialog mount and git execution),statusRes.stdoutis empty and the dialog silently shows "0 uncommitted files" — presenting a clean-worktree state that's actually an error state. -
[Suggestion]
closeAnyOpenDialogdoesn't handleshowWorktreeExitDialog(useDialogClose.ts:76-142): Every other dialog in the application can be dismissed by Ctrl+C viacloseAnyOpenDialog, butWorktreeExitDialogis the only exception — it can only be dismissed via Escape. -
[Suggestion] Test coverage gaps:
nonInteractiveCli.tsheadless--resumeworktree restore (3 branches untested),useWorktreeSession.tshook (no test file),Footer.tsxworktree indicator (0 worktree assertions),useStatusLine.tsworktree payload (0 assertions).
— mimo-v2.5-pro via Qwen Code /review
| ): Promise<WorktreeSession | null> { | ||
| try { | ||
| const raw = await fs.readFile(filePath, 'utf-8'); | ||
| return JSON.parse(raw) as WorktreeSession; |
Collaborator
There was a problem hiding this comment.
[Critical] readWorktreeSession performs JSON.parse(raw) as WorktreeSession with zero runtime validation. If the sidecar file is partially written (process killed during writeFile), truncated by disk full, or manually edited, the parsed object may have undefined for critical fields like worktreePath or slug. This corrupted object propagates through the entire system: WorktreeExitDialog runs git status with cwd: undefined, removeUserWorktree(undefined, ...) may operate on the wrong worktree, and Footer renders ⎇ undefined (undefined).
Suggested change
| return JSON.parse(raw) as WorktreeSession; | |
| const parsed = JSON.parse(raw); | |
| if ( | |
| typeof parsed?.slug !== 'string' || | |
| typeof parsed?.worktreePath !== 'string' || | |
| typeof parsed?.worktreeBranch !== 'string' | |
| ) { | |
| return null; | |
| } | |
| return parsed as WorktreeSession; |
— mimo-v2.5-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in 38db00f. readWorktreeSession now schema-validates the parsed object via isValidWorktreeSession and returns null when any of the 6 required string fields is missing or wrong-typed. Partial writes, schema drift, and manual edits can no longer propagate undefined paths into removeUserWorktree / git status / Footer rendering.
| code: error | ||
| ? typeof (error as NodeJS.ErrnoException).code === 'number' | ||
| ? ((error as NodeJS.ErrnoException).code as unknown as number) | ||
| : 1 |
Collaborator
There was a problem hiding this comment.
[Suggestion] execGit extracts the exit code via (error as NodeJS.ErrnoException).code, but NodeJS.ErrnoException.code is always a string (e.g., 'ENOENT', 'EPERM') — never a number. The typeof ... === 'number' check always evaluates to false, so code is always 1 on any error regardless of the actual git exit code. The real exit code is on (error as any).status.
Suggested change
| : 1 | |
| code: error | |
| ? ((error as NodeJS.ErrnoException & { status?: number }).status ?? 1) | |
| : 0, |
— mimo-v2.5-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in 38db00f. execGit now reads exit codes from both possible fields: .code as number (subprocess exit codes like 128 "not a git repository") AND .status as number (some Node versions). String .code (e.g. 'ENOENT' from spawn failure) goes into a separate errno field surfaced to the dialog. The previous typeof === 'number' check on .code always evaluated false because for git exits the field is typed as number but my old read path tested the string-typed shape.
| worktreePath={uiState.activeWorktree.path} | ||
| originalHeadCommit={uiState.activeWorktree.originalHeadCommit} | ||
| onKeep={() => void uiActions.handleWorktreeExit('keep')} | ||
| onRemove={() => void uiActions.handleWorktreeExit('remove')} |
Collaborator
There was a problem hiding this comment.
[Critical] handleWorktreeExit is async (it awaits removeUserWorktree and clearWorktreeSession), but the callback discards the returned Promise via () => void. This creates a fire-and-forget pattern: the dialog dismisses immediately, the /quit sequence starts, and if process.exit() fires before the git worktree remove subprocess completes, the worktree is left on disk as an orphan with no log or error.
Suggested change
| onRemove={() => void uiActions.handleWorktreeExit('remove')} | |
| onRemove={async () => { await uiActions.handleWorktreeExit('remove'); }} |
— mimo-v2.5-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Declined — false-positive (Filter 1). handleSlashCommand('/quit') is the LAST statement inside handleWorktreeExit's async function body, gated by the preceding await svc.removeUserWorktree(...) and await clearWorktreeSession(...). The race described ("process.exit fires before git worktree remove completes") cannot occur because /quit is sequenced AFTER both awaits resolve. The Promise discard at the () => void callsite only suppresses the outer Promise — it doesn't break the internal await chain.
…l-b6e48c # Conflicts: # packages/cli/src/ui/AppContainer.tsx # packages/cli/src/ui/hooks/useStatusLine.ts
Bundled response to the two review rounds. Per-thread replies follow.
CORE — worktree sidecar robustness (Findings 3252368644, 3252368651, 3255171690):
- atomicWriteJSON instead of fs.writeFile (no more half-written sidecar after a crash)
- readWorktreeSession now schema-validates the parsed object and returns null
on missing/wrong-type fields instead of propagating undefined into consumers
- restoreWorktreeContext clears the sidecar on JSON parse failure / read I/O
error so a corrupted file doesn't block every subsequent --resume
CORE — hooksPath setup (Finding 3252368645):
- configureHooksPath distinguishes ENOENT (benign "candidate not present")
from real stat errors (EACCES/EIO/ENOTDIR); the latter are warn-logged
so a silently-degraded hooksPath is visible to operators
CLI — handleWorktreeExit Remove path (Findings 3252368637, 3252368640 a+b):
- Anchor GitWorktreeService at activeWorktree.originalCwd (the captured
repo root), not config.getTargetDir() — fixes monorepo-subdirectory
launches where the worktree lives under the repo root but getTargetDir
points at a subpackage
- Check removeUserWorktree return value; on failure, leave the sidecar
intact so --resume can recover (previous code cleared it regardless)
- Pass forceDeleteBranch:true to honour the dialog's "discards N commits"
label — without it `git branch -d` refused unmerged commits and the
branch was silently preserved
CLI — useWorktreeSession watcher (Finding 3252368648):
- Normalize fs.watch filename via toString() so the Linux-Buffer code
path triggers reloads (previous comparison silently never matched)
- Treat null filename as "unknown, reload to be safe" (recursive watchers
on some platforms emit events without a payload)
CLI — WorktreeExitDialog (Findings 3252368650, 3255171694):
- execGit now correctly reads numeric exit codes from .code/.status
(NodeJS.ErrnoException.code is a string for spawn errors, number for
subprocess exits); previous typeof === 'number' check always missed
- Dialog body shows an "⚠ Could not measure worktree state (...)" banner
when git status / rev-list failed, so the user doesn't see a misleading
"0 files, 0 commits" before choosing Remove
CLI — closeAnyOpenDialog (Round 2 review body):
- Wire WorktreeExitDialog into the standard dialog-dismissal path so
Ctrl+C dismisses it the same way it dismisses every other dialog
TEST FIXES — vitest timeouts:
- Real git invocations + user-global hooks (e.g. trustup post-commit
webhooks) can take 10–20s per setUp on CI. Bump testTimeout +
hookTimeout to 30s for the three integ test suites that spawn git
(Phase B/C worktree integ tests) so the suite isn't flaky.
NEW TESTS:
- worktreeSessionService.test: 3 new cases covering malformed JSON,
missing required fields, wrong-type fields, malformed sidecar cleanup,
partial sidecar cleanup (16 total, up from 13).
- useWorktreeSession.test.tsx: 4 new cases — null when no sidecar,
parsed sidecar at mount, reacts to delete, reacts to creation.
- WorktreeExitDialog.test.tsx: 1 new case — loading frame renders before
git probes resolve. (Async dialog states tested via E2E — vi.mock of
execFile in ink-testing-library doesn't fire mock impl reliably.)
- nonInteractiveCli.test: 3 new "Phase C --resume" cases — system-reminder
injection on live worktree, no injection when sidecar absent, stale
sidecar cleanup when worktree dir is gone.
DECLINED FINDINGS (replied on threads):
- 3252368642 (Dialog Keep clears sidecar) — declined-design. Dialog
Keep = "exit app, keep worktree for next --resume"; tool Keep =
"I'm done with this worktree". Intentionally different semantics.
- 3252368643 (originalHeadCommit base branch) — false-positive. There
is no base_branch parameter; getCurrentCommitHash() returns HEAD which
equals the tip of the current branch (== baseBranch in createUserWorktree).
- 3252368640 part c (bypass safety guards) — declined-design. The
dialog IS the safety affordance for this path — it shows dirty-state
counts and asks for explicit user confirmation before removal.
- 3255171696 (DialogManager async fire-and-forget) — false-positive.
handleSlashCommand('/quit') is inside the await chain in
handleWorktreeExit, so the described race ("process.exit before remove
completes") cannot occur.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Pre-commit hook auto-fixed imports collapsed value imports (writeWorktreeSession, clearWorktreeSession) into an `import type` block, breaking runtime resolution. Split back into value + type imports. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
Author
|
Round 2 + Round 3 review summary — addressed in commits Per-finding outcomes
Test coverage added
Round 2 review body items (no inline ids)
Other notable changes in this push
Verification# Core: 87/87
cd packages/core && npx vitest run \
src/services/worktreeSessionService.test \
src/services/gitWorktreeService.test \
src/services/gitWorktreeService.hooks.integ.test \
src/tools/enter-worktree.test \
src/tools/exit-worktree.test \
src/tools/enter-worktree.session.integ.test \
src/tools/exit-worktree.session.integ.test
# CLI: 68/68 (worktree-scoped)
cd packages/cli && npx vitest run \
src/ui/components/Footer.test \
src/ui/components/DialogManager.test \
src/ui/components/WorktreeExitDialog.test \
src/ui/hooks/useWorktreeSession.test \
src/acp-integration/acpAgent.worktree.test \
src/acp-integration/session/Session.worktree.test \
src/nonInteractiveCli.test
npm run typecheck --workspace=@qwen-code/qwen-code-core # clean
npm run typecheck --workspace=@qwen-code/qwen-code # clean
npm run build && npm run bundle # dist/cli.js produced |
…integ Windows CI failure: `repoRoot` from Node's `fs.mkdtemp` returns backslash-separated paths (`C:\Users\runneradmin\…`), but `originalCwd` in the sidecar comes from `getRepoTopLevel()` which delegates to `git rev-parse --show-toplevel` — git on Windows returns forward slashes (`C:/Users/runneradmin/…`). The Windows-only assertion `expect(originalCwd).toBe(repoRoot)` was comparing two different representations of the same canonical path and rightly failed on `Object.is` equality. Compare via path.normalize on both sides so the assertion holds across platforms without changing the runtime path (originalCwd still records git's output verbatim, which is what consumers expect since other places in the codebase that read `getRepoTopLevel()` also work with that shape). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
wenshao
reviewed
May 18, 2026
| slug: string; | ||
| worktreePath: string; | ||
| worktreeBranch: string; | ||
| originalCwd: string; |
Collaborator
There was a problem hiding this comment.
Nit — originalCwd 字段名与存储值不一致
enter-worktree.ts:201 写入这个字段时用的是 projectRoot（即 getRepoTopLevel() 的返回值，repo top-level），不是用户调用时的 cwd。AppContainer.handleWorktreeExit 的注释里也明确说明：
// Anchor at the repo top-level (captured at enter time) rather
// than the current targetDir — when the CLI was launched from
// a monorepo subdirectory, `config.getTargetDir()` is that
// subdir but the worktree lives at `<repoRoot>/.qwen/worktrees/`
const svc = new GitWorktreeService(activeWorktree.originalCwd);字段名读起来是"用户最初的 cwd"，实际语义是"repo top-level"。两者在 monorepo subdir 场景下不同——这正是 #3252368637 这条 review 修复的根因。
字段名让未来的维护者容易再次踩坑：看到 originalCwd 会假定它是 process.cwd() 当时的值，而不是 getRepoTopLevel() 的结果。建议改名为 originalProjectRoot 或 repoTopLevel。当前行为正确，仅是命名问题。
Collaborator
Author
There was a problem hiding this comment.
Doc-only fix in d4e921a. Renaming the on-disk field would force every existing sidecar to be migrated (each active --resume breaks until the user wipes the old file), so I kept the name for back-compat and added an explicit JSDoc on WorktreeSession.originalCwd that calls out: (1) the field stores getRepoTopLevel() not process.cwd(), (2) consumers expecting cwd semantics should NOT use this field, (3) it's named originalCwd for historical reasons. Future maintainers reading the type definition will see the divergence the name doesn't surface.
wenshao
reviewed
May 18, 2026
| // a soft failure. If removal failed, leave the sidecar intact | ||
| // so the next --resume can still see the worktree and let the | ||
| // user retry. (Finding 3252368640 part 1.) | ||
| if (!result.success) { |
Collaborator
There was a problem hiding this comment.
[Critical] handleWorktreeExit 在 remove 失败时静默 /quit（follow-up #3252368640）
之前 #3252368640 part 1 的修复加了 !result.success 检测，但是失败分支仍然直接 /quit：
if (!result.success) {
handleSlashCommand('/quit');
return;
}用户的 mental model：在 dialog 里看到"discards N commits, M files"+点了 Remove → 期望 worktree 被删除。如果 git worktree remove --force 因为权限/锁/损坏失败，用户收不到任何反馈就 quit 了。下次重启时 sidecar 还在（注释里说这是设计），但用户不知道发生了什么、需要去哪里看、worktree 还在不在磁盘上。
建议在 quit 前给个 UI feedback：
if (!result.success) {
historyManager.addItem(
{
type: MessageType.ERROR,
text: `Failed to remove worktree "${activeWorktree.slug}": ${result.error}. The worktree is still on disk; use \`exit_worktree\` or remove it manually.`,
},
Date.now(),
);
// 给用户一帧渲染时间再退出
setTimeout(() => handleSlashCommand('/quit'), 0);
return;
}或者更保守的：失败时不退出，把 dialog 关掉让用户继续在 session 里处理（Cancel 路径就是这样）。Remove 失败应该让用户决定下一步，而不是替他们决定退出。
另外硬失败 catch 块同样问题——/* Hard failure */ 注释承认了这点但仍 fall-through 到 /quit，也建议加 feedback。
Collaborator
Author
There was a problem hiding this comment.
Fixed in d4e921a. Took the more conservative option you suggested: failure path now surfaces an ERROR history item (Failed to remove worktree "X": <reason>. The worktree is still on disk; use exit_worktree to retry...) and stays in the session instead of /quit'ing. User can then decide — retry via exit_worktree, fix the underlying problem, or force-quit manually. Same treatment applied to the hard-failure catch block (previously it caught the throw and fell through to /quit silently — now emits the error and returns).
wenshao
reviewed
May 18, 2026
| // Watcher setup is best-effort: the hook still returns whatever | ||
| // load() resolved with on mount. Without a watcher, the UI just | ||
| // doesn't react to sidecar changes until the next re-mount. | ||
| } |
Collaborator
There was a problem hiding this comment.
Nit — fs.watch 不会幸存 chatsDir 被删除/轮转
fs.watch(dirPath) 持有的是 inode 句柄。如果 ~/.qwen/projects/.../chats/ 被删除（用户重置状态、杀毒软件隔离、清理脚本）然后被另一个进程重建，watcher 不会重新挂到新 inode 上——Footer 的 worktree 指示器从此停止响应 sidecar 变化。
mount-time 的 mkdir({ recursive: true }) 解决了"首次启动目录不存在"，但解决不了"运行中目录消失"。
两种处理方式：
- 加一个降级的 polling tick（比如 5s 一次 stat sidecar 文件）作为 fallback；watcher 仍然是主路径，polling 只在 watcher 失效时兜底；
- 监听
error事件并尝试重新setupWatcher()：watcher = fs.watch(dirPath, ...); watcher.on('error', () => { watcher?.close(); void setupWatcher(); });
实操中这是边缘情况——chatsDir 在正常使用中不会被删除——可以接受作为已知 limitation 不修。但建议在 hook 顶部加一行 JSDoc 说明 "watcher does not self-heal if chatsDir is rotated"，便于未来排障。
Collaborator
Author
There was a problem hiding this comment.
Doc-only fix in 80f9cb4. Took the documented-limitation path you proposed — chatsDir deletion mid-session is rare enough that the polling-fallback / error-event-reset complexity isn't justified. Added a JSDoc block on useWorktreeSession explaining the limitation and pointing future maintainers at the two fix shapes if rotation ever becomes a real failure mode.
wenshao
reviewed
May 18, 2026
| statusline (their script already gets `worktree` in stdin payload | ||
| and likely renders it itself). Also hidden during ctrl-quit | ||
| warnings so they take precedence. */} | ||
| {uiState.activeWorktree && |
Collaborator
There was a problem hiding this comment.
Nit — 自定义 statusline 时 worktree 指示器完全消失
{uiState.activeWorktree &&
statusLineLines.length === 0 && // ← 这里
!uiState.ctrlCPressedOnce &&
!uiState.ctrlDPressedOnce && (
<Text dimColor wrap="truncate">
{`⎇ ${uiState.activeWorktree.branch} (${uiState.activeWorktree.slug})`}
</Text>
)}statusLineLines.length === 0 这个条件假设了：如果用户有自定义 statusline，那他们会自己在脚本里渲染 payload.worktree。但脚本是用户写的：
- 用户脚本可能在 PR 之前就写好了，根本不知道
payload.worktree字段存在； - 用户脚本可能故意忽略
worktree（觉得不需要），但用户其实希望看到内置指示器； - 用户脚本可能只渲染了部分字段，没渲染 worktree。
这三种情况下，用户有 active worktree 但 Footer 上完全看不到 — 容易让用户忘记自己在 worktree 里、对着错误的目录操作。
两种处理：
- 保持现状但在
docs/design/worktree.md里明确说明"自定义 statusline 时需要自行渲染 worktree"，并在payload.worktree字段加个 docstring； - 加个 settings 开关
statusline.suppressBuiltinWorktree: false让用户主动 opt out（默认 false = 即使有 custom statusline 也保留内置 ⎇ 指示器，避免静默丢失）。
(b) 更安全，因为 worktree 指示器是 awareness UX 而不是装饰——丢了它用户可能在错的 cwd 上做破坏性操作。
Collaborator
Author
There was a problem hiding this comment.
Fixed in 80f9cb4 — went with your option (b). Dropped the statusLineLines.length === 0 gate; built-in ⎇ indicator now shows by default whenever activeWorktree is non-null. Added ui.hideBuiltinWorktreeIndicator (default false) as the explicit opt-out for users whose custom statusline already renders worktree and want to avoid duplication. Default-safe + opt-out matches the awareness-UX priority you flagged.
Finding #3256237933 (Critical, follow-up to #3252368640 part 1):
handleWorktreeExit silently /quit'd when removeUserWorktree returned
{success:false}, contradicting the user's intent after they clicked
"Remove worktree and branch (discards N commits, M files)". Now
surfaces an ERROR history item with the underlying error message
and STAYS in the session so the user can decide what to do
(retry via exit_worktree, fix the lock/permission/corruption issue,
or quit anyway). Same treatment applied to the hard-failure catch
block — previously it caught the throw and proceeded to /quit with
no log; now it emits the error and stays alive.
Finding #3256236050 (Nit): originalCwd field name implies "user's
launch cwd" but actually stores `getRepoTopLevel()` (different in
monorepo subdir launches — the gap closed by #3252368637). Renaming
the field would force on-disk migration of every existing sidecar
(every active --resume breaks until users wipe the old file).
Doc-only fix: WorktreeSession.originalCwd now carries an explicit
JSDoc explaining the semantics and warning consumers expecting
process.cwd() to NOT use this field.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Finding #3256241831 (Nit, but awareness UX): the built-in `⎇` indicator used to disappear whenever `statusLineLines.length > 0`, on the assumption that the user's custom statusline rendered worktree itself. That assumption is unsafe — scripts written before Phase C don't know about `payload.worktree`, scripts can deliberately ignore the field, and partial scripts may render some fields but not worktree. In any of those cases the user sees no worktree UI while having an active worktree, risking destructive operations in the wrong cwd. New behavior: indicator shows by default regardless of statusline. Added an opt-out setting `ui.hideBuiltinWorktreeIndicator` (default false) for users whose custom statusline already renders worktree and want to avoid duplication. Finding #3256239608 (Nit): `fs.watch` in useWorktreeSession holds an inode handle to `chatsDir` at mount time. If the directory is deleted out-of-band (manual cleanup, antivirus quarantine, reset scripts) and recreated, the watcher does NOT re-attach to the new inode and the Footer indicator stops reacting to sidecar changes. Reviewer explicitly accepted this as a documented limitation rather than adding polling-fallback or error-event-handler complexity for an edge case that doesn't arise in normal use. Added a JSDoc block on the hook explaining the limitation and pointing to the future fix shapes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…reeIndicator CI Lint step caught that the JSON schema mirror in packages/vscode-ide-companion was out of date after adding the new ui.hideBuiltinWorktreeIndicator setting in 80f9cb4. Regenerated via `npm run generate:settings-schema`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
wenshao
reviewed
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Previous review round fixes look good. Two remaining test coverage gaps:
WorktreeExitDialogunit tests only assert the loading state; post-load states (dirty counts, remove-label variants,probeErrorbanner) are covered by E2E Group E only — notnpm test.AppContainer.handleWorktreeExitcallback (~80 lines, 4+ branches: cancel/keep/remove-success/remove-failure) has zero unit test coverage.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| let worktreeAlive = false; | ||
| try { | ||
| const stat = await fs.stat(session.worktreePath); |
Collaborator
There was a problem hiding this comment.
[Suggestion] restoreWorktreeContext verifies worktreePath exists and is a directory via fs.stat, but does not validate it's actually a git worktree (e.g., check that .git is a file pointing back to the main repo, or that the path starts with <repo>/.qwen/worktrees/). If the sidecar JSON is corrupted, the model could be directed to operate in an arbitrary existing directory.
Suggested change
| const stat = await fs.stat(session.worktreePath); | |
| // After fs.stat confirms the path is a directory, also verify it's a worktree: | |
| const gitFileStat = await fs.stat(path.join(session.worktreePath, '.git')).catch(() => null); | |
| worktreeAlive = gitFileStat?.isFile() ?? false; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in 7943487 (option chosen: structural prefix check, not the .git file probe). restoreWorktreeContext now requires worktreePath to resolve under <originalCwd>/.qwen/worktrees/ — schema validation already enforces the shape, this adds the boundary. A tampered/copied sidecar pointing at /etc, ~/, or any other existing dir is rejected and cleared. Went with the prefix check over the .git-is-file probe because the prefix is a structural invariant Qwen itself owns (every enter_worktree writes into that subtree), whereas .git file vs dir is a git-internal detail that depends on linked/main worktree state.
wenshao
requested changes
May 18, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] packages/cli/src/acp-integration/acpAgent.ts: loadSession() restores the persisted worktree context and queues pendingWorktreeNotice, but the parallel unstable_resumeSession() ACP resume path just creates the session and returns. ACP clients that call unstable_resumeSession() will resume a live worktree session without the first prompt being reminded to keep using the worktree path, and stale sidecars also won't be cleaned. Please factor the restore block used by loadSession() into a helper and call it from unstable_resumeSession() after createAndStoreSession() returns the Session.
— gpt-5.5 via Qwen Code /review
| */ | ||
| private async configureHooksPath(worktreePath: string): Promise<void> { | ||
| const huskyPath = path.join(this.sourceRepoPath, '.husky'); | ||
| const gitHooksPath = path.join(this.sourceRepoPath, '.git', 'hooks'); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The fallback assumes <sourceRepoPath>/.git is a directory, but when Qwen itself is launched from an existing git worktree, .git is a file that points at the real git dir. In that setup, repositories without .husky/ will probe <worktree>/.git/hooks, get ENOTDIR, and skip hook configuration entirely, so commits in the newly-created Qwen worktree can silently bypass the repo's hooks.
Please resolve the hooks directory via git (for example git rev-parse --git-common-dir / --git-dir) instead of constructing .git/hooks from the working tree path.
— gpt-5.5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in 7943487. configureHooksPath now resolves the canonical hooks dir via git rev-parse --git-common-dir instead of constructing <sourceRepoPath>/.git/hooks. When Qwen runs from a linked worktree, .git is a file pointing at the real gitdir — the old construction ENOTDIRed and silently skipped hook configuration. The new path returns the gitdir-relative hooks/ directory regardless of worktree/main-repo shape.
| // Key not set — empty string means "proceed with the write". | ||
| } | ||
| if (existing !== hooksPath) { | ||
| await worktreeGit.raw(['config', 'core.hooksPath', hooksPath]); |
Collaborator
There was a problem hiding this comment.
[Suggestion] This overwrites any existing custom core.hooksPath in the worktree-local config whenever it differs from Qwen's preferred .husky / .git/hooks path. Repositories or users can already configure a custom hooks directory, and creating a Qwen worktree should not silently replace that policy.
Please preserve a non-empty existing value, or only write when the key is unset. If a worktree-specific override is required, treat inherited/custom values as intentional instead of replacing them unconditionally.
— gpt-5.5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in 7943487. configureHooksPath now writes core.hooksPath only when the worktree-local key is unset (empty string from git config --local). A non-empty inherited or user-configured value is logged at debug level and preserved verbatim — the user/system policy wins. The Phase C "set hooksPath on creation" behavior is unchanged for fresh worktrees (whose local config starts empty).
| } | ||
|
|
||
| if (this.params.action === 'keep') { | ||
| await this.maybeClearWorktreeSession(); |
Collaborator
There was a problem hiding this comment.
[Critical] keep leaves the worktree and branch on disk but clears the active sidecar. That makes subsequent --resume lose the persisted worktree binding, so the footer/exit dialog/restore path no longer know this session is still attached to the kept worktree and the model can fall back to editing the parent checkout.
Please keep the sidecar for action: 'keep' and only clear it after a successful remove of the currently tracked worktree. The success message can still tell the model to reference the path, but the persisted state must survive the session boundary.
— gpt-5.5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in 7943487. exit_worktree action='keep' no longer clears the sidecar — preserving it lets --resume / Footer / WorktreeExitDialog continue to know about the kept worktree, which is exactly the user-visible promise of "keep". The model can still reference the worktree path via the tool's return value as before; only the persisted binding stops being torn down. The corresponding integ test was rewritten to assert preservation (clears-after-keep was a misguided behavior, not the spec).
| // intent — without it, `git branch -d` refuses unmerged | ||
| // commits and the branch is silently preserved, contradicting | ||
| // the dialog text. (Finding 3252368640 part 2.) | ||
| const result = await svc.removeUserWorktree(activeWorktree.slug, { |
Collaborator
There was a problem hiding this comment.
[Critical] The TUI dialog's remove path bypasses the ownership guard used by exit_worktree. The tool path reads the in-worktree session marker and refuses to remove another session's worktree, but this UI path trusts the sidecar and directly calls removeUserWorktree(..., forceDeleteBranch: true). A stale or copied sidecar can therefore let one session delete another session's worktree and branch through the dialog.
Please apply the same readWorktreeSessionMarker / config.getSessionId() check here before removal, and refuse or direct the user to the owning session when the marker belongs to a different session.
— gpt-5.5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in 7943487. handleWorktreeExit's remove path now reads the in-worktree session marker via readWorktreeSessionMarker(activeWorktree.path) and compares against config.getSessionId() — same guard the ExitWorktreeTool tool path applies. When the marker names a different session, the dialog surfaces an ERROR with the owner id + manual recovery command and stays in the session (does not /quit). Stale or copied sidecars can no longer trigger cross-session destruction through the dialog.
| console.debug('worktree session restore warning:', err); | ||
| }); | ||
| if (restored.contextMessage) { | ||
| historyManager.addItem( |
Collaborator
There was a problem hiding this comment.
[Critical] This restores the worktree only as a UI MessageType.INFO item. Unlike headless and ACP, the TUI path does not prepend the restored context to the next model request; useGeminiStream still sends only the user's next prompt to sendMessageStream(). The user will see the active-worktree notice, but the model is not reminded to keep using session.worktreePath, so resumed interactive sessions can still edit the parent checkout.
Please keep a one-shot pending worktree reminder for the TUI path as well and inject it into the next UserQuery sent to the model. The INFO item can remain for visibility, but it should not be the only restore mechanism.
— gpt-5.5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in 7943487. AppContainer now owns a pendingWorktreeNoticeRef set during --resume restore and consumed by handleFinalSubmit on the user's first non-slash prompt. The submitted value is prefixed with the same <system-reminder>...</system-reminder> block headless and ACP use, so the model actually sees the worktree reminder in the next API request. The INFO history item remains for user visibility but is no longer the only restore mechanism.
Critical fixes: - #3259975247: TUI dialog Remove now reads the in-worktree session marker and refuses to delete a worktree owned by a different session — same ownership guard ExitWorktreeTool already applies. Stale/copied sidecars can no longer destroy another session's work. - #3259975249: TUI --resume queues a one-shot pendingWorktreeNotice ref consumed by handleFinalSubmit; the user's first prompt is prefixed with the same <system-reminder> block headless/ACP use. Previously only the INFO history item showed in the transcript (UI-only), so resumed models could silently edit the parent checkout. - #3259975245: exit_worktree action='keep' no longer clears the sidecar. `keep` means "preserve the worktree for later"; clearing the persisted binding broke --resume / Footer / WorktreeExitDialog for kept worktrees. Now matches the Dialog keep semantics. Test updated to assert preservation instead of clearing. - ACP unstable_resumeSession parity: factored the worktree restore block into #restoreWorktreeOnResume() and called from both loadSession() and unstable_resumeSession(). ACP clients using resume no longer miss the worktree context. Suggestion-level fixes: - #3259975237: configureHooksPath now resolves the canonical hooks dir via `git rev-parse --git-common-dir` instead of constructing `<sourceRepoPath>/.git/hooks`. The construction assumed .git is a directory, but when Qwen runs from a linked worktree it's a file pointing at the real gitdir → ENOTDIR → silent no-hooks worktree. - #3259975242: only writes core.hooksPath when the key is unset. A non-empty inherited or user-configured value is preserved instead of being silently replaced. - #3256839787: restoreWorktreeContext adds a structural invariant check — worktreePath must live under <originalCwd>/.qwen/worktrees/. A tampered/copied sidecar pointing at an arbitrary existing dir is rejected and cleared so the model can't be redirected. Tests: - worktreeSessionService.test: 17/17 (added prefix-escape rejection case + restructured the existing live-worktree case to satisfy the new structural invariant). - exit-worktree.session.integ.test: rewrote keep test to assert preservation (matches new behavior). - nonInteractiveCli.test: updated fixture worktreeDir to live under <originalCwd>/.qwen/worktrees/ for the prefix invariant. - All other suites pass without modification. Test coverage gap acknowledgement (no comment_id reply): per-handler unit tests for handleWorktreeExit + dialog post-load states remain covered by the E2E Group E suite in docs/e2e-tests/worktree-phase-c.md. The execFile mock path in ink-testing-library still doesn't deliver async useEffect state transitions reliably, so unit testing those states adds more harness than signal; deferring. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
Author
|
Round 6 — addressed in commit Per-finding outcomes
ACP
|
wenshao
approved these changes
May 19, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No new issues found after 6 prior review rounds. All previously reported findings have been addressed in the latest commit. Build, typecheck, and tests pass (17/17 core worktree tests, 25 CLI tests). The one remaining low-confidence item (handleWorktreeExit not checking result.branchPreserved) is a rare edge case with forceDeleteBranch: true and does not block merge. LGTM ✅ — qwen-latest-series-invite-beta-v28 via Qwen Code /review
dreamWB
added a commit
to dreamWB/qwen-code
that referenced
this pull request
May 20, 2026
…Session AppContainer.test.tsx mocks every hook that AppContainer.tsx imports, but the two new hooks (usePreferredEditor from this PR, useWorktreeSession from main's QwenLM#4174) were not mocked — causing the real hooks to execute during tests, crash on missing context, and fail all 47 downstream assertions.
pomelo-nwu
pushed a commit
that referenced
this pull request
May 21, 2026
* feat(cli): respect /editor preference in Ctrl+X external editor The Ctrl+X external editor prompt previously ignored the general.preferredEditor setting, always falling back to $VISUAL/$EDITOR env vars. Now it consults the preferred editor first, using the correct --wait flags for GUI editors, and falls back to env vars only when no preference is set or the preferred editor is unavailable. Closes #4165 * fix(cli): address review feedback on external editor feature - Fix command injection risk: quote args when needsShell is true - Move writeFileSync inside try/finally with mode 0o600 - Change temp file extension from .md to .txt - Extend needsShell check to cover .bat extension - Fix import formatting in AgentComposer.tsx - Extract usePreferredEditor hook to deduplicate validation - Add 12 tests for openInExternalEditor covering all branches * test(cli): add missing vi.mock for usePreferredEditor and useWorktreeSession AppContainer.test.tsx mocks every hook that AppContainer.tsx imports, but the two new hooks (usePreferredEditor from this PR, useWorktreeSession from main's #4174) were not mocked — causing the real hooks to execute during tests, crash on missing context, and fail all 47 downstream assertions. * fix(cli): address review feedback on env-var fallback and spawnSync timeout - Detect .cmd/.bat in env-var fallback path on Windows and enable shell mode with quoted args, matching the preferred-editor path behavior - Add 30-minute timeout to spawnSync to prevent terminal freeze when a GUI editor hangs - Add test cases for both changes * fix(cli): propagate preferredEditor to TextInput component TextInput creates its own useTextBuffer but was not passing preferredEditor, so Ctrl+X in secondary inputs (dialogs, settings prompts, etc.) silently ignored the /editor preference. * fix(cli): document why simple double-quoting is safe for shell args The args passed to cmd.exe are program-controlled (tmpdir path + fixed flags), never arbitrary user input. cmd.exe does not expand $() or backticks inside double quotes. This matches Claude Code's approach. * fix(cli): handle signal-killed editor and defer undo snapshot - Check spawnSync signal field to avoid reading stale temp file when editor is killed by SIGTERM/SIGKILL - Move undo snapshot creation after successful file read to prevent phantom no-op undo entries on editor failure * fix(cli): restore private tmpdir, skip undo on unchanged content - Restore mkdtempSync isolation directory (was flattened to os.tmpdir) - Skip undo snapshot when editor content is unchanged - Update JSDoc to reflect deferred-snapshot behavior - Remove unused crypto import - Add tests: unchanged content skip, tmpDir cleanup, undo precision * fix(cli): use path.join in external editor tests for Windows compat Tests hardcoded forward-slash paths which fail on Windows where path.join produces backslashes. Use pathMod.join for the expected temp file path so assertions pass on all platforms. * fix(cli): quote editorCmd in shell mode, wrap setRawMode, improve logging - Quote editorCmd along with args when shell: true, so Windows paths with spaces (e.g. C:\Program Files\...\code.cmd) survive cmd.exe. - Wrap setRawMode restore in try/catch so a destroyed stdin doesn't skip temp file cleanup. - Include command, shell mode, and resolution source in error log. - Add tests: CRLF normalization, readFileSync failure, editorCmd quoting. * refactor(core): remove unused isTerminal from ExternalEditorCommand The field was never consumed by any caller — only command, args, and needsShell are destructured. The standalone isTerminalEditor() function already serves the same purpose for openDiff. * docs(cli): update stale JSDoc on openInExternalEditor Reflect the new editor resolution order (/editor → $VISUAL → $EDITOR → vi) and the moved undo-snapshot timing (after editor exit, not before). * fix(cli): address review round 3 — temp dir leak, mkdtemp safety, TextInput stdin - Split unlinkSync/rmdirSync into separate try/catch blocks to prevent temp directory leak when unlinkSync throws (regression from main) - Move mkdtempSync inside try block with early return on failure - Pass stdin/setRawMode from TextInput to useTextBuffer so terminal editors (vim/neovim/emacs) correctly toggle raw mode via Ctrl+X * test(cli): add undo-after-successful-edit test for external editor * fix(cli): opts.editor priority, filePath in error log, warn on invalid editor * fix(cli): address sandbox gap and Windows env-var safety in external editor - usePreferredEditor now checks allowEditorTypeInSandbox() and returns undefined for GUI editors when SANDBOX env is set - env/default editor fallback rejects commands containing " or | before enabling shell mode on Windows * fix(cli): address wenshao review — unsafe-char guard, debug logs, test coverage - Add unsafe-character rejection for opts.editor .cmd paths on Windows - Change env-var unsafe-char handling from throw to graceful return + cleanup - Add debug logging before spawnSync and in setRawMode catch block - Add tests for opts.editor path, .cmd shell mode, and unsafe-char rejection * fix(cli): expand unsafe-char guard, remove stale comment, add tests - Expand Windows unsafe-character regex to include % and ! (cmd.exe variable expansion and delayed expansion) - Remove stale "no hooks needed" comment in TextInput.tsx - Add setRawMode lifecycle test (disable before editor, restore after) - Add default fallback tests for vi (linux) and notepad (win32) * fix(cli): remove explicit type annotation on mock.calls.findIndex callback The `[boolean]` tuple annotation conflicts with vitest's `any[][]` mock.calls type, causing TS2345 in CI. * fix(cli): replace unlinkSync+rmdirSync with recursive rmSync for temp cleanup Leftover swap files from vim/neovim would cause rmdirSync to silently fail on non-empty directories, leaking temp dirs. Use rmSync with recursive+force to handle this. Also fix stale JSDoc fallback comment. * test(cli): add % and ! unsafe-char coverage and error-path raw mode test - Expand opts.editor and env-var unsafe-char tests to cover %, !, and " independently via it.each, preventing silent regex regressions - Add error-path test verifying setRawMode restore when editor exits with non-zero status
LaZzyMan
added a commit
that referenced
this pull request
May 21, 2026
…s + PR refs Three cross-cutting capabilities on top of the Phase A-C worktree foundation (PRs #4073, #4174). D-1: --worktree [name] CLI flag creates a worktree (or re-attaches to one that already exists) before any model turn runs. Supports bare, plain-slug, `=`, and PR-reference forms; --worktree + --acp rejected with a clear error; --worktree + --resume overrides the resumed session's saved sidecar and emits a stderr line. D-2: worktree.symlinkDirectories: string[] settings key opts into symlinking main-repo directories (e.g. node_modules) into every newly-created general-purpose worktree. Applies to all three creation paths: --worktree flag, EnterWorktreeTool, AgentTool isolation. Path traversal, absolute paths, and existing destinations all guarded; missing source dirs and EEXIST silently skipped (fail-open). D-3: --worktree=#<N> / --worktree <github-url> resolves a PR number, runs `git fetch origin pull/<N>/head` (30s timeout, no `gh` CLI dependency, LANG=C for stable error-taxonomy matching), and creates the worktree off FETCH_HEAD. URL regex tolerates /files, /commits, /checks sub-paths so users can paste any GitHub PR URL. Phase 6 verification fixes also included: - Re-attach to an existing worktree instead of failing with "Worktree already exists" — the common `qwen --resume <sid> --worktree foo` workflow now succeeds. The session ownership marker is preserved on re-attach so cross-session exit_worktree action="remove" still fails for non-owners. - Normalize path-taking argv fields (mcpConfig, jsonSchema @<path>, openaiLoggingDir, jsonFile, inputFile, telemetryOutfile, includeDirectories) to absolute paths against the launch cwd BEFORE the worktree chdir. Otherwise downstream fs.existsSync('./mcp.json') resolves into the worktree, where the file doesn't exist. Phase 7 code-review fixes: - buildStartupWorktreeNotice differentiates "Active worktree" (fresh create) from "Re-attached to worktree" (re-attach path). - Notice survives sidecar persist failure: set before the try block, refreshed inside with override addendum if persist succeeded. - getRegisteredWorktreeBranch verifies the candidate path's git common-dir matches the source repo's — rejects sibling `git init` directories that happen to be on a worktree-<slug> branch. Three-mode parity for the startup notice: TUI consumes via AppContainer effect, headless prepends a <system-reminder> + emits a worktree_started JSON event. ACP path is mutually exclusive with --worktree (ACP hosts supply per-session cwd separately). Tests (66 + 15 new): - 15 cli/src/startup/worktreeStartup.test.ts (slug forms, PR fetch against local fake remote, re-attach happy + wrong-branch guard) - 8 core/src/services/gitWorktreeService.test.ts (parsePRReference: #N, URLs, malformed, traversal, leading zeros, non-string) - 10 core/src/services/gitWorktreeService.symlinks.integ.test.ts (symlink loop + fetchPullRequestRef error taxonomy) Known limitations (documented in docs/users/features/worktree.md): - Cross-slug --resume <sid> --worktree <different-new-slug> is unsupported by design (sessions are bound to projectHash(cwd)); future Config refactor anchoring storage at repo root would lift this. - Mid-session enter_worktree still does NOT switch cwd/targetDir (Phase A's simplification); only the startup --worktree flag does. - yargs ambiguity: `qwen --worktree "say hi"` consumes the prompt as the slug. Quick Start shows the `=` form and reordering workarounds. Docs: - docs/users/features/worktree.md (new): Quick Start with --worktree flag, CLI Reference table for all four input forms + error codes, settings table, Limitations. - docs/design/worktree.md: Phase D section expanded into D-1/D-2/D-3 with open questions resolved; capability table updated. - docs/e2e-tests/worktree-phase-d.md (new): full E2E plan with Phase 4 dry-run baseline + Phase 6 post-impl reproduction tables. Refs #4056
5 tasks
wenshao
added a commit
that referenced
this pull request
May 24, 2026
…4469) * fix(core): decouple auto-memory recall from main-agent request path (#4172) * docs: add async memory recall design spec and implementation plan * refactor(core): introduce MemoryPrefetchHandle, replace pendingRecallAbortController field * refactor(core): fire memory recall as non-blocking prefetch with settledAt flag * refactor(core): replace blocking await with zero-wait settledAt poll at UserQuery consume point Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(core): inject recalled memory on first ToolResult when UserQuery consume point misses Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(core): replace pendingRecallAbortController with pendingMemoryPrefetch in all cleanup paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(memory): remove 1s AbortSignal.timeout from relevanceSelector — caller controls lifetime Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(core): update auto-memory tests for async prefetch pattern — drop fake timers and deadline references Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(core): add ToolResult inject test — memory injected on first ToolResult when recall settles after UserQuery Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(core): address codex review findings on async memory recall Three findings fixed: 1. Abort previous prefetch before installing a new one (line 1059): A new UserQuery/Cron used to overwrite pendingMemoryPrefetch without aborting the old controller, leaking an unbounded background recall now that the 1s side-query timeout is gone. 2. Move the UserQuery consume poll AFTER the async reminder setup: ensureTool + listSubagents are awaited between the old poll location and the final assembly, so recalls that settled during those awaits used to be missed (and a tool-less turn never got a ToolResult retry). The poll now runs immediately before requestToSend assembly, and unshifts memory to the front of systemReminders to preserve ordering. 3. Append memory after functionResponse on ToolResult turns: The Qwen API requires the functionResponse part to immediately follow the model's functionCall (see lines 1209-1213). Prepending memory text risked breaking that pairing on the native Gemini path. Appending keeps the pair intact on Gemini and produces the same OpenAI output (text becomes a separate user message after the tool messages). Tests: - Updated ToolResult inject test to assert memory index > functionResponse - Added abort-previous-prefetch test (mid-flight UserQuery aborts old handle) 224/224 tests pass; tsc clean on changed files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs(core): add JSDoc + clarifying comments per review feedback Annotations only, no behavior change: - MemoryPrefetchHandle: full JSDoc covering lifecycle (create → consume → discard) - UserQuery consume site: explain why we unshift (front of systemReminders) - ToolResult inject site: reference hasPendingToolCall pattern instead of brittle line numbers when citing the Qwen functionCall/Response constraint - relevanceSelector.ts: explain why the side-query has no inline timeout (caller controls lifetime via MemoryPrefetchHandle.controller) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(core): bridge caller abort signal into memory prefetch + doc accuracy fixes Behavior fix (addresses copilot review on client.ts:1071): - When the parent sendMessageStream signal aborts (user Ctrl-C / Esc), the prefetch controller now aborts too. Previously the recall side-query would keep running until a later cleanup (next UserQuery / /clear / etc), wasting fast-model tokens on work whose result no one would consume. - Listener uses { once: true } and is also removed in the promise's finally() so a long-lived parent signal doesn't accumulate listeners across many turns under normal completion. - Edge case: if signal is already aborted when fire runs, abort the controller synchronously instead of attaching a listener. Test: - New regression guard: "should abort the pending prefetch when the caller signal aborts" — verifies the abort handler installed on the recall side fires once the parent signal aborts. Doc accuracy (addresses copilot review on the design spec): - ToolResult inject: was documented as "prepend", actual implementation appends to preserve functionCall/functionResponse pairing. Updated both the prose summary and the code sample. - Cleanup section: was documented as 6 abort-locations including the "post-consume clear"; the consume sites don't actually abort (the promise has already settled). Reorganized as 5 abort-and-clear sites + 2 clear-only sites with the distinction made explicit. - Fire path snippet: added the abort-previous-prefetch line and the caller-signal bridge so the spec matches the current implementation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(core): consolidate memory-prefetch lifecycle + safety nets per round-3 review Architectural (root-cause fix for cleanup-path sibling drift): - New private cancelPendingMemoryPrefetch() consolidates the abort+clear idiom (was duplicated across 6 sites). Logs at debug when discarding a settled-but-unconsumed handle so missing-memory scenarios are diagnosable. - New private tryConsumeMemoryPrefetch() consolidates the consume-and-mark-consumed dance (was duplicated UserQuery + ToolResult). - All existing cleanup sites + the two newly-flagged early-return sites (LoopDetected, Error) now use the helper; future early-returns can rely on the finally-block safety net. - sendMessageStream try-finally now uses a `normalCompletion` flag: only the bottom-of-try return path preserves the prefetch (intentional — next ToolResult turn may consume it); every other exit (uncaught exception, abnormal early-return) goes through cancelPendingMemoryPrefetch in finally. Diagnostics: - Restored AbortError debug log in fire-path catch (was silent after removing the deadline mechanism; aborts now come from 4+ sources so a trace is valuable). - Updated stale "deadline" log in recall.ts to reflect current abort sources (caller signal / new UserQuery / cleanup / 30 s safety timeout). Safety net: - Added 30 s ceiling in relevanceSelector via AbortSignal.any(...). Generous enough that normal ~1 s recalls don't trip it; bounds zombie side-queries if the model API hangs and the caller never aborts. Replaces the uncancellable `new AbortController().signal` fallback that would have left callerless invocations running indefinitely. Doc sync: - Design doc updated: UserQuery consume code sample now shows `unshift` (matches implementation) with an inline note on the prepend-vs-append contrast. Tests: - New regression guard: resetChat aborts pending prefetch and clears the handle. - New regression guard: LoopDetected mid-stream aborts pending prefetch and clears the handle (catches the sibling-drift bug this round caught). 227/227 tests pass; tsc clean on changed files. Declined from this round: - `await Promise.resolve()` after fire path: defensive — current code has multiple natural microtask drains before consume point. Added comment documenting the dependency instead. - Renaming `settledAt: number | null` to `settled: boolean`: timestamp has diagnostic value for future instrumentation; current consumers' null-check usage is documented in the JSDoc. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(test): correct getLastLoopType mock return type — null, not undefined CI tsc --build (stricter than --noEmit) caught: src/core/client.test.ts(2996,65): error TS2345: Argument of type 'undefined' is not assignable to parameter of type 'LoopType | null'. getLastLoopType()'s contract returns LoopType | null; the test mock was returning undefined. Switched to null to match the type. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(core): preserve memory prefetch across hook/next-speaker continuations + accurate recall abort log Round-4 review findings (self-inflicted regression from round-3): 1. Preserve pending prefetch on `return hookTurn` (Stop-hook continuation) and `return continueTurn` (next-speaker continuation). The round-3 `normalCompletion = true` was only set at the bottom-of-try `return turn`, leaving these two recursive-yield paths to trip the finally cleanup. When the inner Hook turn produced tool calls, the subsequent ToolResult turn found `pendingMemoryPrefetch === undefined` and memory was silently dropped. 2. recall.ts catch log distinguishes caller-driven aborts (heuristic genuinely skipped below) from the 30s safety-net timeout in relevanceSelector (the caller's signal is NOT aborted by that path, so the heuristic fallback actually runs). Regression guard added: - "should PRESERVE the pending prefetch when next-speaker continueTurn returns" — was red before this commit, green after. 258/258 tests pass; tsc --build clean. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * 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 ada0837e2 commit added a worktree-restore call in the headless path that probes config.getResumedSessionData(); the mock Config never had that method. Return undefined to short-circuit the restore block — these tests don't exercise --resume. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address PR #4174 reviewer findings Bundled response to the two review rounds. Per-thread replies follow. CORE — worktree sidecar robustness (Findings 3252368644, 3252368651, 3255171690): - atomicWriteJSON instead of fs.writeFile (no more half-written sidecar after a crash) - readWorktreeSession now schema-validates the parsed object and returns null on missing/wrong-type fields instead of propagating undefined into consumers - restoreWorktreeContext clears the sidecar on JSON parse failure / read I/O error so a corrupted file doesn't block every subsequent --resume CORE — hooksPath setup (Finding 3252368645): - configureHooksPath distinguishes ENOENT (benign "candidate not present") from real stat errors (EACCES/EIO/ENOTDIR); the latter are warn-logged so a silently-degraded hooksPath is visible to operators CLI — handleWorktreeExit Remove path (Findings 3252368637, 3252368640 a+b): - Anchor GitWorktreeService at activeWorktree.originalCwd (the captured repo root), not config.getTargetDir() — fixes monorepo-subdirectory launches where the worktree lives under the repo root but getTargetDir points at a subpackage - Check removeUserWorktree return value; on failure, leave the sidecar intact so --resume can recover (previous code cleared it regardless) - Pass forceDeleteBranch:true to honour the dialog's "discards N commits" label — without it `git branch -d` refused unmerged commits and the branch was silently preserved CLI — useWorktreeSession watcher (Finding 3252368648): - Normalize fs.watch filename via toString() so the Linux-Buffer code path triggers reloads (previous comparison silently never matched) - Treat null filename as "unknown, reload to be safe" (recursive watchers on some platforms emit events without a payload) CLI — WorktreeExitDialog (Findings 3252368650, 3255171694): - execGit now correctly reads numeric exit codes from .code/.status (NodeJS.ErrnoException.code is a string for spawn errors, number for subprocess exits); previous typeof === 'number' check always missed - Dialog body shows an "⚠ Could not measure worktree state (...)" banner when git status / rev-list failed, so the user doesn't see a misleading "0 files, 0 commits" before choosing Remove CLI — closeAnyOpenDialog (Round 2 review body): - Wire WorktreeExitDialog into the standard dialog-dismissal path so Ctrl+C dismisses it the same way it dismisses every other dialog TEST FIXES — vitest timeouts: - Real git invocations + user-global hooks (e.g. trustup post-commit webhooks) can take 10–20s per setUp on CI. Bump testTimeout + hookTimeout to 30s for the three integ test suites that spawn git (Phase B/C worktree integ tests) so the suite isn't flaky. NEW TESTS: - worktreeSessionService.test: 3 new cases covering malformed JSON, missing required fields, wrong-type fields, malformed sidecar cleanup, partial sidecar cleanup (16 total, up from 13). - useWorktreeSession.test.tsx: 4 new cases — null when no sidecar, parsed sidecar at mount, reacts to delete, reacts to creation. - WorktreeExitDialog.test.tsx: 1 new case — loading frame renders before git probes resolve. (Async dialog states tested via E2E — vi.mock of execFile in ink-testing-library doesn't fire mock impl reliably.) - nonInteractiveCli.test: 3 new "Phase C --resume" cases — system-reminder injection on live worktree, no injection when sidecar absent, stale sidecar cleanup when worktree dir is gone. DECLINED FINDINGS (replied on threads): - 3252368642 (Dialog Keep clears sidecar) — declined-design. Dialog Keep = "exit app, keep worktree for next --resume"; tool Keep = "I'm done with this worktree". Intentionally different semantics. - 3252368643 (originalHeadCommit base branch) — false-positive. There is no base_branch parameter; getCurrentCommitHash() returns HEAD which equals the tip of the current branch (== baseBranch in createUserWorktree). - 3252368640 part c (bypass safety guards) — declined-design. The dialog IS the safety affordance for this path — it shows dirty-state counts and asks for explicit user confirmation before removal. - 3255171696 (DialogManager async fire-and-forget) — false-positive. handleSlashCommand('/quit') is inside the await chain in handleWorktreeExit, so the described race ("process.exit before remove completes") cannot occur. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(test): correct linter-mangled imports in useWorktreeSession.test Pre-commit hook auto-fixed imports collapsed value imports (writeWorktreeSession, clearWorktreeSession) into an `import type` block, breaking runtime resolution. Split back into value + type imports. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(test): normalize path separators for Windows in worktree session integ Windows CI failure: `repoRoot` from Node's `fs.mkdtemp` returns backslash-separated paths (`C:\Users\runneradmin\…`), but `originalCwd` in the sidecar comes from `getRepoTopLevel()` which delegates to `git rev-parse --show-toplevel` — git on Windows returns forward slashes (`C:/Users/runneradmin/…`). The Windows-only assertion `expect(originalCwd).toBe(repoRoot)` was comparing two different representations of the same canonical path and rightly failed on `Object.is` equality. Compare via path.normalize on both sides so the assertion holds across platforms without changing the runtime path (originalCwd still records git's output verbatim, which is what consumers expect since other places in the codebase that read `getRepoTopLevel()` also work with that shape). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address PR #4174 round 4 findings Finding #3256237933 (Critical, follow-up to #3252368640 part 1): handleWorktreeExit silently /quit'd when removeUserWorktree returned {success:false}, contradicting the user's intent after they clicked "Remove worktree and branch (discards N commits, M files)". Now surfaces an ERROR history item with the underlying error message and STAYS in the session so the user can decide what to do (retry via exit_worktree, fix the lock/permission/corruption issue, or quit anyway). Same treatment applied to the hard-failure catch block — previously it caught the throw and proceeded to /quit with no log; now it emits the error and stays alive. Finding #3256236050 (Nit): originalCwd field name implies "user's launch cwd" but actually stores `getRepoTopLevel()` (different in monorepo subdir launches — the gap closed by #3252368637). Renaming the field would force on-disk migration of every existing sidecar (every active --resume breaks until users wipe the old file). Doc-only fix: WorktreeSession.originalCwd now carries an explicit JSDoc explaining the semantics and warning consumers expecting process.cwd() to NOT use this field. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address PR #4174 round 5 findings Finding #3256241831 (Nit, but awareness UX): the built-in `⎇` indicator used to disappear whenever `statusLineLines.length > 0`, on the assumption that the user's custom statusline rendered worktree itself. That assumption is unsafe — scripts written before Phase C don't know about `payload.worktree`, scripts can deliberately ignore the field, and partial scripts may render some fields but not worktree. In any of those cases the user sees no worktree UI while having an active worktree, risking destructive operations in the wrong cwd. New behavior: indicator shows by default regardless of statusline. Added an opt-out setting `ui.hideBuiltinWorktreeIndicator` (default false) for users whose custom statusline already renders worktree and want to avoid duplication. Finding #3256239608 (Nit): `fs.watch` in useWorktreeSession holds an inode handle to `chatsDir` at mount time. If the directory is deleted out-of-band (manual cleanup, antivirus quarantine, reset scripts) and recreated, the watcher does NOT re-attach to the new inode and the Footer indicator stops reacting to sidecar changes. Reviewer explicitly accepted this as a documented limitation rather than adding polling-fallback or error-event-handler complexity for an edge case that doesn't arise in normal use. Added a JSDoc block on the hook explaining the limitation and pointing to the future fix shapes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(worktree): regenerate settings.schema.json for hideBuiltinWorktreeIndicator CI Lint step caught that the JSON schema mirror in packages/vscode-ide-companion was out of date after adding the new ui.hideBuiltinWorktreeIndicator setting in 80f9cb495. Regenerated via `npm run generate:settings-schema`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(worktree): address PR #4174 round 6 findings Critical fixes: - #3259975247: TUI dialog Remove now reads the in-worktree session marker and refuses to delete a worktree owned by a different session — same ownership guard ExitWorktreeTool already applies. Stale/copied sidecars can no longer destroy another session's work. - #3259975249: TUI --resume queues a one-shot pendingWorktreeNotice ref consumed by handleFinalSubmit; the user's first prompt is prefixed with the same <system-reminder> block headless/ACP use. Previously only the INFO history item showed in the transcript (UI-only), so resumed models could silently edit the parent checkout. - #3259975245: exit_worktree action='keep' no longer clears the sidecar. `keep` means "preserve the worktree for later"; clearing the persisted binding broke --resume / Footer / WorktreeExitDialog for kept worktrees. Now matches the Dialog keep semantics. Test updated to assert preservation instead of clearing. - ACP unstable_resumeSession parity: factored the worktree restore block into #restoreWorktreeOnResume() and called from both loadSession() and unstable_resumeSession(). ACP clients using resume no longer miss the worktree context. Suggestion-level fixes: - #3259975237: configureHooksPath now resolves the canonical hooks dir via `git rev-parse --git-common-dir` instead of constructing `<sourceRepoPath>/.git/hooks`. The construction assumed .git is a directory, but when Qwen runs from a linked worktree it's a file pointing at the real gitdir → ENOTDIR → silent no-hooks worktree. - #3259975242: only writes core.hooksPath when the key is unset. A non-empty inherited or user-configured value is preserved instead of being silently replaced. - #3256839787: restoreWorktreeContext adds a structural invariant check — worktreePath must live under <originalCwd>/.qwen/worktrees/. A tampered/copied sidecar pointing at an arbitrary existing dir is rejected and cleared so the model can't be redirected. Tests: - worktreeSessionService.test: 17/17 (added prefix-escape rejection case + restructured the existing live-worktree case to satisfy the new structural invariant). - exit-worktree.session.integ.test: rewrote keep test to assert preservation (matches new behavior). - nonInteractiveCli.test: updated fixture worktreeDir to live under <originalCwd>/.qwen/worktrees/ for the prefix invariant. - All other suites pass without modification. Test coverage gap acknowledgement (no comment_id reply): per-handler unit tests for handleWorktreeExit + dialog post-load states remain covered by the E2E Group E suite in docs/e2e-tests/worktree-phase-c.md. The execFile mock path in ink-testing-library still doesn't deliver async useEffect state transitions reliably, so unit testing those states adds more harness than signal; deferring. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(core): apply defaultModalities() on env-var-only model config (#4219) (#4262) * fix(core): apply defaultModalities() on env-var-only model config (#4219) When qwen-code is configured only via env vars (OPENAI_API_KEY / OPENAI_BASE_URL / OPENAI_MODEL) with no modelProviders entry, resolveGenerationConfig() never invoked defaultModalities(), so generationConfig.modalities stayed undefined for image-capable models. The two other config paths (modelRegistry.resolveModelConfig and modelsConfig.applyResolvedModelDefaults) already call it. This aligns the env-var-only path with both so multimodal models like qwen3.6-35b-a3b correctly accept @image attachments. Fixes #4219 * test(core): lock modalities fallback invariants on env-var-only path Address review feedback on PR #4262: - Strengthen the positive regression test to also assert video:true and source kind ('computed'), matching the source-tracking convention used elsewhere in this file and catching regex regressions in modalityDefaults. - Add negative case: unknown model → modalities resolves to {} (text-only), never undefined — the key invariant introduced by the fix. - Add negative case: explicit settings.generationConfig.modalities is not clobbered by the fallback (lock the `=== undefined` guard). - Extend the fallback's comment to document the undefined → {} semantic so future maintainers don't reintroduce `modalities === undefined` branches. No behavior change. * test(core): pin Qwen OAuth modalities auto-detect for coder-model Round-2 review feedback on #4262: `resolveGenerationConfig` is shared by both the OpenAI/env-var-only path and `resolveQwenOAuthConfig`, which passes `resolvedModel` (defaults to 'coder-model') as modelId. So the new modalities fallback also activates for Qwen OAuth — a real behavior change (was undefined, now { image: true, video: true }). The change is desired (coder-model supports vision per the existing warning text in resolveQwenOAuthConfig), but no test pinned it down. Add a regression test so future MODALITY_PATTERNS edits can't silently shift Qwen OAuth behavior. * fix(cli): block Windows Tab approval-mode toggle when input has a Tab consumer (#4308) * fix(cli): block Windows Tab approval-mode toggle when input has a Tab consumer Closes #4171. On Windows, Shift+Tab is indistinguishable from a bare Tab in many terminals, so useAutoAcceptIndicator accepts a bare Tab as the approval-mode cycle shortcut. To avoid double-firing with the input area, AppContainer passes a `shouldBlockTab` callback that suppresses the cycle when the input has its own Tab handler. Until now that callback only tracked the autocomplete dropdown (`shouldShowSuggestions`). When the buffer was empty and the followup prompt-suggestion ("input prediction") was visible, pressing Tab on Windows accepted the suggestion *and* cycled approval mode at the same time — the exact behaviour reported in #4171. The mid-input ghost-text and reverse/command-search paths had the same gap. Broaden the signal: compute `hasTabConsumer` from every Tab consumer inside InputPrompt — autocomplete dropdown, followup suggestion, mid-input ghost text, reverse-search, command-search — and feed that into `shouldBlockTab`. A single Tab keystroke now triggers exactly one action on Windows; macOS and Linux behaviour is unchanged. Tests cover the four states (followup visible, ghost text visible, autocomplete visible, idle). * fix(cli): tighten hasTabConsumer, add unmount cleanup + tests (#4308 review) Three review findings on PR #4308 addressed together — all touch the same `hasTabConsumer` signal surface exposed from InputPrompt to AppContainer. 1. **Tighten signal semantics (Copilot)**: drop the standalone `reverseSearchActive || commandSearchActive` terms. When those overlays have matches, their `showSuggestions` flag already flows into `shouldShowSuggestions` and Tab is consumed via `ACCEPT_SUGGESTION_REVERSE_SEARCH`. When they're active without matches, Tab is NOT consumed — including the bare flags misrepresented the signal as "Tab consumer present" when it really meant "modal overlay open". `hasTabConsumer` now strictly matches its name. 2. **useEffect cleanup on unmount (wenshao)**: previously, if any Tab consumer was active when InputPrompt unmounted (e.g. streaming begins while autocomplete is open), AppContainer's `hasTabConsumer` state retained the stale `true` value and kept blocking Windows Tab approval-mode cycling for the entire unmount window. Effect now resets to `false` on cleanup. The pre-existing code had the same gap with one trigger; expanding to 3 triggers materially raised the likelihood. 3. **JSDoc on prop name (wenshao)**: `onSuggestionsVisibilityChange` now carries broader "Tab consumer" semantics than the name suggests. Cross-file rename across UIActionsContext + Composer + AppContainer is too much churn for #4308's scope; add JSDoc on the prop declaration documenting the broader signal and that the name is retained for backward compatibility. 4. **Test coverage (wenshao)**: add two tests — autocomplete dismissal reports `false` (true→false transition); unmount-while-active reports `false` (cleanup regression guard). * fix(cli): split Tab-consumer signal so it doesn't hide Footer (#4308 review) Self-inflicted regression caught by wenshao: the previous round broadened `onSuggestionsVisibilityChange` from "autocomplete dropdown visible" to "any Tab consumer present", but Composer.tsx was using that same callback for a different purpose — hiding the Footer / KeyboardShortcuts when the dropdown would overlap their vertical space. As a result, followup prompt suggestions and mid-input ghost text (both inline within the input box, neither competing for vertical space) were also hiding the Footer on every platform. Split into two signals: - `onSuggestionsVisibilityChange` — narrow, autocomplete dropdown only. Kept local to Composer for Footer hiding. Restored to pre-PR semantics; no cleanup-on-unmount needed (the entire conditional in Composer.tsx is already gated by `uiState.isInputActive`, which goes false when InputPrompt unmounts). - `onTabConsumerChange` — broad, any input-side Tab consumer (autocomplete + followup + ghost text). Plumbed through UIActionsContext to AppContainer's `hasTabConsumer` state → useAutoAcceptIndicator's `shouldBlockTab`. Retains the cleanup-on-unmount wenshao added last round (the broad signal IS read while InputPrompt is unmounted). Tests: - All 6 broad-signal regression tests renamed to assert `onTabConsumerChange`. - 3 new narrow-signal regression tests pin that `onSuggestionsVisibilityChange` does NOT fire `true` for followup or ghost text. Catches the exact shape of my regression. * fix(core): mirror Qwen3 reasoning on outbound history (#4294) * feat(core): extend cross-auth fast models to agents (#4153) * feat(core): extend cross-auth fast models to agents * fix(core): tighten cross-auth model resolution fallbacks When a forked-agent caller passes a selector that cannot resolve (e.g. `fast` with no fast model configured), fall back to the parent session model instead of forwarding the raw selector string to the provider. Matches the subagent path, where unresolvable selectors mean "inherit parent". In BaseLlmClient.createContentGeneratorForModel, do not cache the unregistered-model fallback. getCurrentContentGenerator() reads the runtime view from AsyncLocalStorage, which can differ between calls; caching would pin the first call's view-bound generator under the selector key and reuse it on later calls after that view has unwound. * docs(core): drop stale getFastModelForSideQuery from sideQuery JSDoc The function was removed when fast-model resolution collapsed onto getFastModel(); the JSDoc fallback chain still mentioned it. * feat(cli,core): add Auto approval mode with LLM classifier (#4151) * feat(cli,core): add Auto approval mode with LLM classifier (#auto-mode) Add a fifth approval mode positioned between Auto-Edit and YOLO that uses an LLM classifier to evaluate each tool call and auto-approve safe ones while blocking risky ones — letting agents work autonomously on long sessions without forcing users to confirm every shell/network call. Three-layer filter when L4 returns 'ask'/'default': L5.1 acceptEdits fast-path: Edit/Write inside workspace -> allow L5.2 safe-tool allowlist: Read/Grep/LS/TodoWrite/... -> allow L5.3 LLM classifier: two-stage (fast/thinking) via sideQuery Anti-injection: assistant text and tool results are stripped from the classifier transcript; each tool projects its args through a new `toAutoClassifierInput` method to redact sensitive/voluminous fields. Pending action is rendered as a user-role text turn so it survives the OpenAI Chat Completions converter (which drops orphan tool_calls). Safety: fail-closed on classifier failure; denial-tracking caps 3 consecutive blocks / 2 consecutive unavailable before falling back to manual confirmation; dangerous allow rules (Bash interpreter wildcards, any Agent/Skill allow) are temporarily stripped while in AUTO and restored on exit — settings.json is never modified. Config: --approval-mode auto # CLI flag tools.approvalMode: "auto" # settings.json permissions.autoMode.hints.{allow,deny}: string[] # natural-lang permissions.autoMode.environment: string[] * chore(schema): regenerate settings.schema.json after adding tools.approvalMode 'auto' The autogenerated VS Code settings schema was out of sync with the runtime SETTINGS_SCHEMA after the AUTO mode addition; CI's Lint job caught the drift. No behavior change — this is purely the regenerated output of `npm run generate:settings-schema`. * test(cli): update expected error message after adding 'auto' to approval-mode choices Two tests in `loadCliConfig`'s error-path coverage hard-coded the list of valid approval modes in the expected error string. Add `auto` to match the runtime message produced by the new five-mode enum. * test(core): fix autoMode test fixture on Windows The fixture's mock isPathWithinWorkspace used path.sep to join the root prefix, but the hard-coded test paths use forward slashes regardless of OS. On Windows path.sep is '\\', so prefix matching failed and L5.1 fast-path tests returned false (and the L5.1-gating test then fell into the classifier branch, hitting an undefined getToolRegistry mock). Hard-code '/' in the fixture — it controls only intra-file consistency between mock roots and mock paths, not real workspace behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cli,core): three asymmetries surfaced by self-review of PR #4151 ACP path (Session.ts) had two asymmetries with the CLI scheduler that silently degraded AUTO behavior, and the classifier transcript builder left historical tool_use calls vulnerable to the OpenAI converter's orphan-tool_call filter on the default Qwen / DashScope backend. 1) ACP runs the classifier even when finalPermission === 'allow' The CLI scheduler short-circuits when L4 returned 'allow' (user- explicit rule matched) so the classifier never sees the call. The ACP duplicate only short-circuits on 'deny'. Mirror the scheduler: set autoModeAllowed = (finalPermission === 'allow') before the AUTO L5 block. Without this, a user-written `Bash(git push *)` allow rule in an ACP session could reach the classifier and be blocked by a conservative Stage-1 verdict. 2) ACP never records a successful fallback approval When the denialTracking streak forced fallback, ACP correctly dropped into requestPermission — but after the user approved, the streak was never reset. consecutiveBlock stayed at 3, so every subsequent call re-fell into fallback. The session was permanently downgraded to manual approval until the mode toggled. Add the post-outcome recordFallbackApprove call paralleling coreToolScheduler.ts:1705- 1717 (approve outcomes only; cancel/abort preserve the streak). 3) Classifier transcript: historical functionCalls become orphans on OpenAI-compatible backends buildClassifierContents kept model.functionCall parts but stripped tool results entirely (anti-injection). On Anthropic-native APIs that's fine, but the OpenAI Chat Completions converter (converter.ts:1422-1455) filters out tool_calls without a matching tool response, and since the assistant message has no text content either, the entire turn gets dropped. The classifier on Qwen / DashScope ended up seeing only user prompts plus the pending action — zero record of prior tool actions in the chain. Match ClaudeCode's `buildTranscriptEntries` (yoloClassifier.ts): render every historical model.functionCall as a user-role text turn ("Prior action: tool(args)") projected through toAutoClassifierInput. The result contains only user-role text — no functionCall parts, no assistant tool_calls — so it is converter-agnostic by construction. Tests updated to assert the new shape and added a regression guard verifying no functionCall part survives anywhere in the output. ACP fixes have no new unit tests: their logic is mechanically symmetric with the CLI scheduler branch, the underlying recordFallbackApprove state machine is covered by denialTracking.test.ts, and adding ACP integration tests for these two-to-four-line branches would dwarf the fix itself. The fix correctness is verifiable from the diff against the existing scheduler comparison. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(core): recordFallbackApprove resets BOTH consecutive counters Asymmetry caught by copilot[bot] on PR #4151: the original implementation only cleared consecutiveBlock when the user approved a fallback prompt, leaving consecutiveUnavailable at its threshold. A transient classifier API blip (2 consecutive unavailable verdicts) therefore permanently downgraded the rest of the session to manual approval — even after the user explicitly approved the prompt — because every subsequent shouldFallback() call kept seeing the {reason: 'consecutive_unavailable'} branch. The fix mirrors recordAllow: a manual approval signals the user accepted the action and the next call should re-engage the classifier. If the API is still degraded, the next call simply re- arms the counter (one unavailable / one block), same recovery curve as initial onset. No permanent lock-out, and the documented "Counter resets on user approve or mode switch" behavior from the PR body now actually holds for both reasons. Existing test 'does not reset consecutiveUnavailable' was codifying the bug — replaced with three positive cases (unavailable recovery, total-counter preservation as telemetry, and the no-op guard). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cli,core): address PR #4151 review findings (defense-in-depth + sibling-drift) 20 findings from reviewers wenshao (gpt-5.5 / deepseek-v4-pro / mimo-v2.5-pro) on PR #4151. Triaged through the five-filter framework, accepted findings clustered into four root-cause groups + a misc group. A) Sibling drift: AUTO mode missing in entry-point allowlists - packages/core/src/agents/background-agent-resume.ts — `normalizeApprovalMode` now accepts `'auto'`; `reconcileResumedApprovalMode` now treats `'auto'` as privileged (downgrade in untrusted folder). - packages/cli/src/nonInteractive/control/controllers/permissionController.ts — `validModes` for `set_permission_mode` includes `'auto'`; the non-interactive tool-permission switch handles AUTO (delegates to the scheduler's classifier). - packages/cli/src/config/config.ts — non-interactive deny-list switch adds an AUTO arm that mirrors PLAN/DEFAULT (no fallback UI available). - packages/sdk-typescript/{types/protocol,types/queryOptionsSchema}.ts — `PermissionMode` and the SDK `permissionMode` zod enum accept `'auto'`. - packages/vscode-ide-companion/* — `ApprovalModeValue`, `ApprovalMode` enum, `APPROVAL_MODE_MAP`, `APPROVAL_MODE_INFO`, `APPROVAL_MODE_VALUES`, and all ACP-session mode unions now include AUTO. B) Sub-agent AUTO path (architectural) - agent.ts: untrusted-folder guard in `resolveSubagentApprovalMode` now blocks the `AUTO` privileged mode the same way it blocks YOLO / AUTO_EDIT. - agent.ts: `createApprovalModeOverride(_, AUTO)` now triggers `PermissionManager.stripDangerousRulesForAutoMode()` on the shared manager, so the override path matches the top-level entry path. - agent.ts: `AgentTool.toAutoClassifierInput` forwards the full prompt (was truncated to 200 chars, which hid attack payloads past character 200 from the classifier while the sub-agent received the full text). C) Sibling drift: dangerous-rule surface - dangerousRules.ts: interpreter list expanded with php / lua / julia / R / rscript / groovy / awk / pwsh / cargo / npm / pnpm / yarn / make / gradle / mvn / rake / just / eval / exec / source. Token-based detection now catches multi-word interpreter subcommands (`bun run *`, `npm run *`), absolute-path forms (`/usr/bin/python3 *`), and Monitor-tool allow rules with the same logic. Literal concrete commands (`Bash(npm test)`, `Bash(python script.py)`) are NOT flagged. - permission-manager.ts: `addSessionAllowRule` / `addPersistentRule` now stash newly added dangerous allow rules into `strippedAllowRules` while in AUTO mode, instead of letting an "Always allow" choice on a fallback prompt persist a broad rule that bypasses the classifier. - tools/tools.ts: default `toAutoClassifierInput` returns `''` (the no-security-relevance sentinel) instead of `undefined` (which fell through to raw args). Third-party MCP tools no longer leak raw parameters — potentially API keys, tokens, file contents — into the classifier LLM prompt by default. Internal tools that need their args inspected for safety override the method explicitly. D) Classifier defense-in-depth (architectural) - autoMode.ts: `send_message` removed from SAFE_TOOL_ALLOWLIST so the classifier sees destination + body and can judge inter-agent steering. - autoMode.ts: when `pmForcedAsk=true` (user wrote an explicit ask rule), the function now returns `{ via: 'fallback' }` instead of falling through to the classifier — honoring the documented "ask rules force manual confirmation" guarantee. - classifier.ts: new `sanitizeClassifierReason` strips angle-bracket pseudo-tags, collapses whitespace, and clamps length to 200 chars; applied at the stage-2 boundary so `decision.reason` cannot smuggle a `<system>...` payload into the main model's tool-error message. - classifier.ts: `buildClassifierContents` / `buildClassifierSystemPrompt` are now wrapped in a try/catch that funnels to the existing `failClosed` handler, so any pathological input (circular projected args, registry lookup error, …) becomes an `unavailable=true` block result instead of crashing the tool-execution loop. - classifier-transcript.ts: transcript now truncates to the most recent 40 messages so long autonomous sessions don't overflow the fast classifier's context window — which would otherwise tip the session into the `consecutive_unavailable` fallback after two overflow-induced failures. E) Misc - coreToolScheduler.ts + Session.ts: `finalPermission === 'allow'` path now calls `recordAllow` in AUTO mode so an explicit allow-rule match resets the denialTracking streak (otherwise a 3-block streak would silently force the next classifier-eligible call into manual approval right after an allow-ruled call just worked). - useAutoAcceptIndicator.ts: mount-time effect emits the first-time AUTO information notice + stripped-rules notice when the session starts already in AUTO (`--approval-mode auto` flag or `tools.approvalMode: "auto"` in settings). Previously the notices only fired on Shift+Tab / `/approval-mode` switches. Test updates: - permissions/autoMode.test.ts: SAFE_TOOL_ALLOWLIST snapshot updated (no longer contains send_message). pmForcedAsk regression test now asserts the new `via: 'fallback'` semantics. - permissions/dangerousRules.test.ts: 25 new cases covering extended interpreter list, multi-word subcommands, absolute paths, and Monitor tool. - tools/toAutoClassifierInput.test.ts: AgentTool now asserts full- prompt passthrough rather than 200-char truncation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(vscode-ide-companion): include 'auto' in NEXT_APPROVAL_MODE cycle The cycle map in `acpTypes.ts` is typed as `{ [k in ApprovalModeValue]: ApprovalModeValue }`. After adding `'auto'` to `ApprovalModeValue` in the previous commit, this map became missing the `auto` arm — caught by CI's tsc check (`error TS2741: Property 'auto' is missing`). Add it between `auto-edit` and `yolo` so the cycle order remains plan → default → auto-edit → auto → yolo → plan, matching the core APPROVAL_MODES ordering. Local lint/typecheck only — not introduced or surfaced by review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(core): silence two CodeQL findings on PR #4151 CodeQL 223 — Incomplete multi-character sanitization (packages/core/src/permissions/classifier.ts:258) A single `/<[^>]*>/g` pass can leave residual angle-brackets when the input is crafted to overlap (e.g. `<scr<script>ipt>`). In our actual use case the sanitized string is a prompt fragment, not HTML output, so a "reconstituted script tag" doesn't matter — but iterating the strip until the string stabilises is cheap defense-in-depth and removes the warning. Bounded by 8 iterations so the loop is always O(n) regardless of how the attacker structures the input. CodeQL 222 — Polynomial regex on uncontrolled data (packages/core/src/permissions/dangerousRules.ts:93) The regex `/[*]+$/` is actually linear (single-character class + `$` anchor, no backtracking), but CodeQL flags any `replace(<regex>, ...)` applied to user-controlled input. Replace the regex with a manual trailing-`*` strip via `slice` + a counted loop — same semantics, no regex engine involved, warning cleared. Existing tests cover both branches (classifier transcript sanitizer test suite, dangerousRules interpreter coverage). No regressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cli,core,docs): address 4 non-blocker findings from PR #4151 review Top-level review on c5cf60ee8 declared "可以合并" (good to merge) but flagged 5 non-blocker items. Four are mechanical / low-cost; the fifth (thresholds → config) is intentionally deferred — see review reply. 1. docs/users/features/auto-mode.md:223 The "agent classifier sees first 200 chars of prompt" line was a stale leftover from before the truncation was removed (the AgentTool.toAutoClassifierInput regression guard now asserts full- prompt passthrough). Updated to describe the actual behavior plus the safety rationale (same shape as run_shell_command forwarding the full command). Also expanded the projection table with a note that MCP tools default to argument-stripped projection — pairing with the Limitations addendum below. 2. coreToolScheduler.ts:1425 + Session.ts:1945 The unavailable error message was overwriting `failClosed`'s classified reason ('Conversation transcript exceeds classifier context window' / 'Classifier prompt construction failed' / etc.) with a generic "blocked for safety" line. Operators lose the diagnostic distinction. Both sites now append the original reason in parentheses when present: 'Auto mode classifier unavailable; action blocked for safety (Classifier stage 1 unavailable - …)'. 3. permission-manager.ts:771 The session branch of the dangerous-rule stash didn't dedupe by raw string, while the persistent branch did. A user repeatedly clicking "Always allow" on the same fallback prompt would have piled duplicate stash entries that all activate on AUTO exit. Mirror the persistent-branch dedup. 4. docs/users/features/auto-mode.md (Limitations) Added a bullet making MCP-tool conservative-blocking explicit: third-party tools that haven't overridden toAutoClassifierInput show only their name to the classifier, so most calls will be blocked unless the user has written an explicit allow rule. This was a deliberate fail-closed choice from the previous round, but users wouldn't predict it without documentation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(cli,core): inline classifier reason inside unavailable message Minor nit from review on a3138cf5d: the previous wording put the specific failClosed reason at the tail — "unavailable; action blocked for safety (Conversation transcript exceeds classifier context window)" — which separates the reason from the "unavailable" context. wenshao's suggested wording inlines the reason right after the noun it qualifies: "Auto mode classifier unavailable (Conversation transcript exceeds classifier context window); action blocked for safety". Both forms preserve the diagnostic content. The inlined version reads more naturally for operators scanning a tool-error trace. Mirror the change in the ACP Session.ts path so CLI and ACP keep parallel diagnostic shapes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cli,core): address 10 review findings from PR #4151 round 4 Two reviewers (DeepSeek/deepseek-v4-pro + qwen-latest-series-invite- beta-v28, both via wenshao /review) flagged 12 inline + 2 out-of-scope findings. 11 accepted and fixed; 1 partially declined (L5 integration tests — see classified reply). Grouped by root-cause class: # Class A — missing tool projections (sibling-drift sweep) `SendMessageTool`, `MonitorTool`, `CronCreateTool` all reach the classifier in AUTO (not on the allowlist, L3 default 'ask') but had no `toAutoClassifierInput` override. The base default returns `''` → `projectFunctionArgs` maps to `{}` → classifier sees just the tool name. For `send_message` this was particularly bad: it was intentionally REMOVED from the safe allowlist in an earlier round so the classifier could inspect message content, but the classifier ended up seeing zero arguments anyway. - send-message: + getDefaultPermission='ask' (was inheriting 'allow' from BaseToolInvocation, so the scheduler auto-approved at L4 before L5 ran) + toAutoClassifierInput forwarding task_id+message. - monitor: toAutoClassifierInput forwards command+directory (same shape as ShellTool — classifier needs the actual command). - cron-create: toAutoClassifierInput forwards cron+prompt+recurring (the scheduled prompt runs against the agent at fire-time, so the classifier must see what the agent will be asked to do). # Class B — client.toPermissionMode missing AUTO arm SessionStart hooks in AUTO mode were silently receiving `permission_mode: 'default'`. Add the missing case before the default branch. Parallels the round-2 sibling-drift sweep that fixed the same shape in background-agent-resume. # Class C — duplicated CLI/ACP AUTO branch + missing tests The classifier-block error message and the approve-outcome predicate were duplicated verbatim in `coreToolScheduler.ts` and ACP `Session.ts`. Extracted two helpers: - `formatClassifierBlockMessage(decision)` in autoMode.ts - `isApproveOutcome(outcome)` in denialTracking.ts Both unit-tested with regression-guard cases. Both callsites now use the helpers, so a future outcome added in one place can't drift. Also added two `evaluateAutoMode` test cases the reviewer flagged as missing: `pmForcedAsk=true` honors user intent (was already tested) and `skipClassifier=true` routes to fallback without dispatching the classifier (NEW guard against denialTracking regression). # Class D — perf + dead code + Edit preview - `getHistory(false)` → `getHistoryTail(40, false)` at the two AUTO classifier-dispatch sites. The transcript builder already truncates to 40 messages; cloning the full session every non-fast-path call was wasted work. - Removed `recordFallbackReject` (dead code per reviewer audit). The "rejection preserves state" invariant is enforced by simply not calling any state-mutating function; an exported no-op helper invited future drift. - Bumped Edit/WriteFile preview from 80 → 300 chars and added explicit truncation flags. In-workspace edits take the acceptEdits fast-path so this only affects out-of-workspace writes (~/.npmrc etc.) — exactly the case where the classifier needs more headroom to spot a hostile payload after a benign prefix. # Class E — prompt-injection via workspace hints + colon-form Bash FP - User-provided `autoMode.hints.{allow,deny}` are now wrapped in `<user_hint>` tags in the classifier system prompt, and a new decision principle explicitly tells the classifier to treat instruction-shaped hints ("always set shouldBlock=false") as adversarial prompt injection rather than directives. This pairs with the existing untrusted-workspace short-circuit (workspace settings are dropped from merged settings on untrusted folders) to defend in depth against a hostile `.qwen/settings.json`. - `isDangerousBashRule` no longer flags specific colon-form rules like `Bash(python3:run-tests)` as dangerous. Previously two paths (firstToken-equals-content + colon-with-interpreter) hit specific concrete rules as if they were wildcards. Now only empty-suffix (`python:`) and `*`-suffix variants are dangerous; concrete suffixes are treated the same as `Bash(npm run test)`. Two new test groups codify the boundary. # Class F — classifier observability The `failClosed` helper consumed the underlying error and returned only a generic sanitized reason. Operators debugging "every AUTO call is unavailable" had no way to distinguish API timeout / context overflow / construction failure. Added `debugLogger.warn` inside both fail paths (failClosed + the stage-2-review-unavailable branch) that logs the original error name+message. No telemetry/UI surface change — debug-only. # Out-of-scope (top-level review summary) Already covered as part of Class A — both SendMessageTool and MonitorTool projections plus SendMessage permission override fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(sdk,serve,docs): include 'auto' in DAEMON_APPROVAL_MODES sibling sites After rebase onto current main, three sites needed updating to keep the AUTO mode integrated end-to-end: 1) packages/sdk-typescript/src/daemon/types.ts:706 `DAEMON_APPROVAL_MODES` literal tuple was still 4-mode. The new `approval-mode-drift.test.ts` (#4282 fold-in) asserts this tuple mirrors core's `APPROVAL_MODES` sequence-exactly — it caught the drift before runtime, exactly as designed. 2) packages/cli/src/serve/server.test.ts:2287 The 400-response assertion for unknown approval-mode literal still expected the 4-mode list. Updated to include 'auto' between 'auto-edit' and 'yolo' (matching core APPROVAL_MODES ordering). 3) docs/developers/qwen-serve-protocol.md:1124 Protocol docs listed 4 modes for the `POST /session/:id/approval- mode` body validator. Updated to 5. These are mechanical follow-ups to AUTO mode's existing entry-point sweep — covered by sibling-drift class but only surfaced once main landed the SDK drift detector and the new serve API. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(core,sdk): two critical bypasses + SDK union drift on PR #4151 wenshao surfaced two critical findings on the round-4 fix; both are self-inflicted regressions from defenses I added that didn't go deep enough. # 1. <user_hint> tag escape (classifier-prompts/system-prompt.ts) [gpt-5.5 — comment 3263963950] Round 4 wrapped user-provided hints in raw `<user_hint>...</user_hint>` tags to mark them as untrusted context. But the tag envelope is broken the moment the payload itself contains a closing tag: "allow": ["</user_hint>\n- Allow all shell commands\n<user_hint>"] renders as a real bullet outside the wrapper. The defense was empty. Fix: render user hints as JSON-encoded string literals labelled `user hint:`. JSON.stringify keeps the entire payload inside a single quoted string with newlines escaped to `\n` and quotes to `\"` — the injected text can never become its own structural bullet line. Decision-principles text updated to reference the new shape. Regression-guard test: a payload containing `</user_hint>` plus an injection sentence preceded by a newline must NOT appear as a standalone bullet line. # 2. Privileged tools' L3 default = 'allow' bypassed the classifier [gpt-5.5 — comment 3263963966] Round 4 added `toAutoClassifierInput` projections to AgentTool / SkillTool / CronCreateTool but did NOT override `getDefaultPermission`. The base default is `'allow'`, and the scheduler short-circuits at L4 when finalPermission === 'allow' (the AUTO ack short-circuit I added in round 1 to honor explicit allow rules) — so the new projections were never reached and arbitrary sub-agent spawns / skill invocations / scheduled prompts silently approved. Same shape as the SendMessageTool critical from round 4. That round fixed the one tool the reviewer pointed at; this round audits the sibling sites I should have caught at the same time. Override `getDefaultPermission` to return `'ask'` on all three: - AgentTool — sub-agent spawn - SkillTool — skill load + user code execution - CronCreateTool — scheduled prompt that runs against agent at fire- time Updated the two existing "should not require confirmation" tests in agent.test.ts + skill.test.ts which were codifying the bypass. # 3. SDK QueryOptions.permissionMode union missing 'auto' [gpt-5.5 top-level review] Sibling drift: the SDK protocol schema accepts 'auto' but the public `QueryOptions.permissionMode` literal union was still 4-mode. Typed SDK consumers calling `query({ permissionMode: 'auto' })` got a TS error. Updated the union, refreshed the JSDoc + priority chain, and inserted 'auto' in the documented mode list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(core,cli): close 5 review findings on PR #4151 round 5 Two critical + three sugges…
LaZzyMan
added a commit
that referenced
this pull request
May 27, 2026
…s + PR refs (#4381) * feat(worktree): Phase D — startup --worktree flag + symlinkDirectories + PR refs Three cross-cutting capabilities on top of the Phase A-C worktree foundation (PRs #4073, #4174). D-1: --worktree [name] CLI flag creates a worktree (or re-attaches to one that already exists) before any model turn runs. Supports bare, plain-slug, `=`, and PR-reference forms; --worktree + --acp rejected with a clear error; --worktree + --resume overrides the resumed session's saved sidecar and emits a stderr line. D-2: worktree.symlinkDirectories: string[] settings key opts into symlinking main-repo directories (e.g. node_modules) into every newly-created general-purpose worktree. Applies to all three creation paths: --worktree flag, EnterWorktreeTool, AgentTool isolation. Path traversal, absolute paths, and existing destinations all guarded; missing source dirs and EEXIST silently skipped (fail-open). D-3: --worktree=#<N> / --worktree <github-url> resolves a PR number, runs `git fetch origin pull/<N>/head` (30s timeout, no `gh` CLI dependency, LANG=C for stable error-taxonomy matching), and creates the worktree off FETCH_HEAD. URL regex tolerates /files, /commits, /checks sub-paths so users can paste any GitHub PR URL. Phase 6 verification fixes also included: - Re-attach to an existing worktree instead of failing with "Worktree already exists" — the common `qwen --resume <sid> --worktree foo` workflow now succeeds. The session ownership marker is preserved on re-attach so cross-session exit_worktree action="remove" still fails for non-owners. - Normalize path-taking argv fields (mcpConfig, jsonSchema @<path>, openaiLoggingDir, jsonFile, inputFile, telemetryOutfile, includeDirectories) to absolute paths against the launch cwd BEFORE the worktree chdir. Otherwise downstream fs.existsSync('./mcp.json') resolves into the worktree, where the file doesn't exist. Phase 7 code-review fixes: - buildStartupWorktreeNotice differentiates "Active worktree" (fresh create) from "Re-attached to worktree" (re-attach path). - Notice survives sidecar persist failure: set before the try block, refreshed inside with override addendum if persist succeeded. - getRegisteredWorktreeBranch verifies the candidate path's git common-dir matches the source repo's — rejects sibling `git init` directories that happen to be on a worktree-<slug> branch. Three-mode parity for the startup notice: TUI consumes via AppContainer effect, headless prepends a <system-reminder> + emits a worktree_started JSON event. ACP path is mutually exclusive with --worktree (ACP hosts supply per-session cwd separately). Tests (66 + 15 new): - 15 cli/src/startup/worktreeStartup.test.ts (slug forms, PR fetch against local fake remote, re-attach happy + wrong-branch guard) - 8 core/src/services/gitWorktreeService.test.ts (parsePRReference: #N, URLs, malformed, traversal, leading zeros, non-string) - 10 core/src/services/gitWorktreeService.symlinks.integ.test.ts (symlink loop + fetchPullRequestRef error taxonomy) Known limitations (documented in docs/users/features/worktree.md): - Cross-slug --resume <sid> --worktree <different-new-slug> is unsupported by design (sessions are bound to projectHash(cwd)); future Config refactor anchoring storage at repo root would lift this. - Mid-session enter_worktree still does NOT switch cwd/targetDir (Phase A's simplification); only the startup --worktree flag does. - yargs ambiguity: `qwen --worktree "say hi"` consumes the prompt as the slug. Quick Start shows the `=` form and reordering workarounds. Docs: - docs/users/features/worktree.md (new): Quick Start with --worktree flag, CLI Reference table for all four input forms + error codes, settings table, Limitations. - docs/design/worktree.md: Phase D section expanded into D-1/D-2/D-3 with open questions resolved; capability table updated. - docs/e2e-tests/worktree-phase-d.md (new): full E2E plan with Phase 4 dry-run baseline + Phase 6 post-impl reproduction tables. Refs #4056 * refactor(worktree): apply self-review feedback on Phase D Self-review pass over the Phase D commit (2636f59) catching one real typecheck regression plus a batch of small quality + efficiency improvements. No user-visible behavior change beyond fixing the build. Build fix: - worktreeStartup.ts imports — pre-commit prettier had reorganized `writeWorktreeSession` and `readWorktreeSession` under an `import type { ... }` block, erasing them at compile time (verbatimModuleSyntax). `tsc --noEmit` was failing with TS1361. Bundle path still worked (esbuild is lenient) so this only surfaced when running typecheck. Startup-path efficiency (~10-25 ms saved per --worktree invocation on macOS; more on Windows): - Drop redundant `isGitRepository()` probe — `getRepoTopLevel()` returns null on non-git paths and covers both gates in one subprocess. - Run `getCurrentBranch()` + `getCurrentCommitHash()` in parallel via Promise.all (independent calls). - Combine the two `git rev-parse` probes inside `getRegisteredWorktreeBranch` into a single multi-arg call, and run it in parallel with the source-repo common-dir lookup. Saves one fork+exec on the re-attach path. Quality: - Extract `withReminder()` local helper in nonInteractiveCli.ts so the startup-notice and resume-restore branches share the system-reminder wrapping. - Log `readWorktreeSession` failures in `persistStartupWorktreeSidecar` with the sidecar path so operators can recover the previous slug from a backup. Silent swallow was making "where did my worktree binding go?" undebuggable. - Drop the dead `Config.getWorktreeSettings()` accessor (only `getWorktreeSymlinkDirectories()` has callers); keep the underlying `WorktreeSettings` interface for future fields. - Document the `pendingStartupWorktreeNotice` invariant: at most one consumer per process; ACP path is gated out earlier so only TUI XOR headless reads it. - Add a maintainer note in the gemini.tsx path-normalization block: the argv path-field allowlist is hand-maintained, register new path-bearing flags there or `--worktree` silently breaks for them. - Drop `Phase 6 fix (G1)/(G2)` parenthetical labels from inline comments — internal review-cycle identifiers that decay to noise post-merge. Substantive prose retained. Tests: cli 15/15 (unchanged) + core 66/66 (unchanged); bundle smoke verified fresh / re-attach / invalid slug / non-git cases. Findings deliberately left for follow-up: - Larger refactor extracting a shared `provisionUserWorktree` helper for the EnterWorktreeTool / startup overlap (~80% duplicate). - Splitting the re-attach branch out of `setupStartupWorktree` into its own function. - `isPathWithinRoot` / `isInsideManagedWorktree` shared utils. - `symlinkConfiguredDirectories` loop concurrency (saves 5-15 ms on a cold path that runs only when symlinkDirectories is configured). * docs(worktree): refresh stale docstring in worktreeStartup Top-of-file docstring still said `{adj}-{noun}-{4hex}` (actual format is 6 hex chars) and described the PR form as "detected and rejected with a clear 'coming in D-3' message" — but D-3 shipped in the same PR. Tighten to reflect what the code actually does. * fix(worktree): address findings from dual-reviewer self-check Two real bugs surfaced by an independent dual-reviewer pass (Claude + Codex) on the Phase D commits. Both correctness-affecting; both escaped the earlier internal reviews. P0 — re-attach captured the wrong baseline for the exit dialog (Codex): setupStartupWorktree captured `originalHeadCommit` from the launch cwd (main checkout) before any chdir. On the re-attach path the WorktreeExitDialog later runs `git rev-list <originalHeadCommit>..HEAD` inside the worktree to count "new commits this session". With the main-checkout baseline this counted every commit ever made in the kept worktree as new work from the current session — misleading the keep/remove prompt. Re-capture HEAD from inside the worktree after chdir so the count means what the dialog text says it means. P0 — getRegisteredWorktreeBranch mis-identified plain directories as registered worktrees (Claude): A plain directory at `<repo>/.qwen/worktrees/<slug>/` (e.g. a stale artifact from a previous tool) had no `.git` file of its own, so `git rev-parse --git-common-dir` walked up to the outer repo and returned the outer common-dir — matching the source repo's common-dir check and impersonating a registered worktree. If the outer repo happened to be on `worktree-<slug>`, setupStartupWorktree would silently chdir into the plain directory and treat it as attached; subsequent `exit_worktree action="remove"` would then delete a directory that was never registered. Fix: also probe `--show-toplevel` and require it to equal the candidate path (canonicalised via `realpath` so macOS /var → /private/var doesn't break the equality check). A plain dir under the main repo gets the outer repo's toplevel and is correctly rejected. Smaller polish from the same review: - Normalize the literal string `'HEAD'` returned by `getCurrentBranch` on detached HEAD to `undefined`, so the `baseRef` handed to `git worktree add -b … HEAD` does not implicitly anchor against the loose commit when the launch cwd is detached. - `symlinkConfiguredDirectories`: blocklist `.git` (any nested ancestor) and `.qwen/worktrees` (any nested ancestor). Linking `.git` would silently break commits inside the worktree; linking `.qwen/worktrees` would create a worktrees-inside-worktrees loop that confuses the startup sweep. - `WorktreeSettings.symlinkDirectories` typed `readonly string[]` to match the `createUserWorktree(options.symlinkDirectories)` contract and the immutable-config convention elsewhere. `Config.getWorktreeSymlinkDirectories()` return type updated to match. Docs: - design/worktree.md precedence table rewritten. The previous `--worktree` 赢 row was unreachable in practice (sessions are bound to `projectHash(cwd)`, and the chdir happens before session lookup). New table reflects what actually happens for each combination of `--resume` × `--worktree`, including the documented cross-projectHash limitation. The `persistStartupWorktreeSidecar` override branch is now annotated as dead-on-the-current-architecture but kept so a future Config refactor (anchor storage at repo root) picks it up for free. Tests: cli 15/15 + core 66/66 unchanged. Bundle smoke confirms both P0 fixes end-to-end (re-attach captures worktree HEAD = run-1 tip, plain-dir attempt errors out without clobbering existing content). * refactor(worktree): consolidate probe + name detached-HEAD sentinel Second /simplify pass on the dual-reviewer fixes. Three convergent findings; net effect is one fewer subprocess on the re-attach path and clearer intent on string handling / blocklist guards. Efficiency + quality: - Fold the worktree HEAD SHA into `getRegisteredWorktreeBranch`'s combined rev-parse. The probe already requests common-dir, toplevel, and abbrev-ref HEAD in a single subprocess; adding a leading `HEAD` positional (which must come BEFORE `--abbrev-ref` so the flag doesn't apply to it) returns the SHA on its own line. Return type widened to `{ branch, headCommit } | null`. Removes the second `GitWorktreeService` instantiation and `getCurrentCommitHash` call that `setupStartupWorktree`'s re-attach branch used to do. Quality: - Hoist `'HEAD'` to a module-level `DETACHED_HEAD` constant in `worktreeStartup.ts`. Three uses, two meanings (input filter when normalizing `getCurrentBranch` output, fallback metadata for the sidecar's `originalBranch` field on detached state). Naming the sentinel makes intent self-documenting and pre-empts the "why is the value we just stripped re-appearing as a fallback?" reader stall flagged by the round-3 quality review. Reuse + quality: - `symlinkConfiguredDirectories`: replace two hand-rolled containment checks (`startsWith(prefix + sep)` for `.qwen/worktrees`; `path.relative(...).split(sep)[0]` for `.git`) with `isWithinRoot` from `utils/fileUtils.ts`, which is already imported in this file. Replace the hardcoded `path.join(repoRootAbs, '.qwen', 'worktrees')` with `this.getUserWorktreesDir()` so the layout lives in one place (the exported `WORKTREES_DIR` constant). Split the misleading `sourceAbs === repoRootAbs` clause out of the `.git` branch into its own dedicated "empty / repo-root path" rejection with a clearer warn message. Tests: cli 15/15 + core 66/66 unchanged. Bundle smoke verified the folded probe still captures the worktree's HEAD on re-attach (not the launch-cwd HEAD). Skipped from this review pass: - Moving `'HEAD'` normalization into `GitWorktreeService.getCurrentBranch()` itself — would ripple through `enter-worktree.ts` and `agent.ts` callers that hand the result verbatim to `git worktree add -b ...`. Out of scope for a polish pass; the local const is enough. * fix(worktree): broaden symlink blocklist from .qwen/worktrees to all of .qwen Caught by a second pr-tracker dual-reviewer pass (Codex). The previous guard at `symlinkConfiguredDirectories` only refused paths inside `<repoRoot>/.qwen/worktrees/` — `.qwen` itself (the parent) sailed through because `isWithinRoot` is a strict descendant check. A user setting `symlinkDirectories: ['.qwen']` would therefore symlink the entire CLI metadata tree into the new worktree, recursively pulling in `.qwen/worktrees` and recreating the loop the guard was meant to prevent. Other `.qwen/*` subtrees (`projects`, `tmp`, …) are CLI state with no legitimate cross-worktree sharing use case either. Fix: broaden the guard to reject the whole `<repoRoot>/.qwen` tree. Both `.qwen` itself and any descendant fail closed. Also synced the user-facing settings schema description (the in-IDE help text and the published JSON schema) so it mentions the `.git` and `.qwen` rejection rules. The `WorktreeSettings` interface JSDoc already mentioned them; the schema description had not been updated. Tests: cli 15/15 + core 66/66 unchanged. Smoke confirms `--worktree foo` with `symlinkDirectories: ['.qwen']` configured leaves the worktree free of any `.qwen` symlink (only the legitimate per-worktree `.qwen-session` marker file appears). * fix(worktree): guard fetchPullRequestRef against CodeQL command-injection alert CodeQL flagged a "Second order command injection" finding (rule 235) on the `git fetch origin pull/<N>/head` call in `fetchPullRequestRef`. The taint analyzer doesn't see the type-narrowing at the function entry (`Number.isSafeInteger(prNumber) && prNumber > 0 && prNumber <= 1e9`), so it considers `prNumber` library input that could in principle reach a `--upload-pack=…`-shaped flag and thereby execute an arbitrary program. In practice the entry guard already prevents that, but the alert blocks the CodeQL CI check. Add `--end-of-options` between `origin` and the refspec — git's canonical "stop parsing flags" marker (git ≥ 2.24). Tells git definitively that every subsequent argv element is a positional, not a flag, which (a) satisfies the analyzer, (b) adds defense-in-depth against a future regression that might relax the entry guard, and (c) has zero behavior change for any well-formed PR number. Verified locally: `git fetch --end-of-options origin pull/<N>/head` against a local bare-remote with a seeded `refs/pull/42/head` still fetches the ref correctly; the `--worktree=#42` smoke test reads back the PR content from the materialized worktree. Tests: cli 15/15 + core 66/66 unchanged. * fix(worktree): lexical sanitizer for CodeQL + missing test mock entry Two fixes from the third CI round on PR #4381: 1. CodeQL re-fires (round 2 of the same finding). `--end-of-options` is a git-runtime defense, not a lexical sanitizer that CodeQL's `js/second-order-command-line-injection` taint tracker recognises. The alert re-fired against the same call after the previous fix. Switch to a CodeQL-recognised sanitizer: validate the numeric component against `/^[1-9][0-9]*$/` immediately at the sink. The regex digit-only check is one of the documented sanitizer patterns the rule looks for, and proves at the analyzer level that the resulting argv element cannot resemble a flag (`--foo`). The entry guard at the top of the function still establishes the same fact at runtime; this layer makes the proof visible to static analysis. Keep `--end-of-options` as a runtime fallback against any future regression that loosens the entry guard. 2. `nonInteractiveCli.test.ts` mock was missing the new `consumePendingStartupWorktreeNotice` Config method. Phase D-1 added the method on `Config` and `nonInteractiveCli` calls it on every prompt to pick up the one-shot startup-worktree notice. The test file's `mockConfig` literal was not updated, so all 19 `runNonInteractive` tests threw `TypeError: config.consumePendingStartupWorktreeNotice is not a function` on Ubuntu / macOS CI. Add a stub returning `null` so the helper short-circuits, matching the equivalent Phase C stub for `getResumedSessionData`. Local: cli (worktreeStartup + nonInteractiveCli) 60 passed + 1 skipped; core (gitWorktreeService + symlinks + hooks + enter-worktree) 66 passed. * test(worktree): mock getWorktreeSymlinkDirectories in three more test files Round 4 of the same Phase D-2 mock-drift class. CI surfaced 9 test failures across three files whose `Config` mocks construct `EnterWorktreeTool` for setup but lack the new `getWorktreeSymlinkDirectories` method `createUserWorktree` now calls: - enter-worktree.session.integ.test.ts (2 tests) - exit-worktree.session.integ.test.ts (3 tests) — provisions worktrees via EnterWorktreeTool before exercising exit paths - exit-worktree.test.ts (4 tests) — same provisioning pattern via `provisionWorktree()` and the `makeMockConfig` helper Add a `getWorktreeSymlinkDirectories: () => []` stub to each so the symlink loop is a no-op in tests. `enter-worktree.test.ts` and `agent/agent.test.ts` intentionally skipped — they mock `GitWorktreeService.createUserWorktree` outright, so the method call never fires in their code paths. Adding the stub there would be defensive speculation. If a future test exercises the real path, it'll surface there too and we'll add it then. Local: core tools tests now 123 passed (was 9 failed / 114 passed on CI run 26213122427 against commit 000c9f6). * fix(worktree): normalize repoRoot path separators + disable autocrlf in tests Round 5 of CI: Windows-only test failures on the latest HEAD. Two unrelated Windows-specific bugs, both in / around worktreeStartup. 1. `setupStartupWorktree` stored the raw `getRepoTopLevel()` output in `context.repoRoot`. git always emits POSIX paths via `--show-toplevel` (`C:/Users/...`), so on Windows the value was forward-slash where `fs.realpath` and `path.join` produce backslash. The sidecar's `originalCwd` field got the inconsistent format and a downstream `expect(...).toBe(tempRepo)` in the round-trip test compared `C:/Users/.../tmp/...` against `C:\Users\.../tmp/...`. Wrap the value in `path.resolve()` to normalize to the platform-native separator before storing. Downstream consumers (`path.join(session.originalCwd, '.qwen', 'worktrees')` in `restoreWorktreeContext`, `new GitWorktreeService(originalCwd)` in `AppContainer`) already handle either format, so no migration concern for older sidecars. 2. `makeTempRepo` in worktreeStartup.test.ts didn't configure `core.autocrlf=false`. On Windows runners the default is `true`, so files committed and pushed to the test's fake-remote `pull/<N>/head` ref get CRLF-converted on the worktree's checkout. The PR-content assertion `expect(prFile).toBe('from PR 42\n')` then failed with `'from PR 42\r\n'`. Add `core.autocrlf=false` + `core.eol=lf` to the temp-repo setup so test files round-trip byte-for-byte regardless of host platform. Local mac: cli worktreeStartup 15/15 still pass. Windows verification deferred to CI. * fix(worktree): reject '..' segments + use junction on Windows Two Copilot findings on symlinkConfiguredDirectories (PR #4381 round 3): 1. The settingsSchema description, docs/users/features/worktree.md, and WorktreeSettings JSDoc all promise that entries containing `..` are rejected — but the post-resolve isWithinRoot check accepted `foo/../bar` (resolves to `bar`, inside the repo). Add a literal `..` segment check before path.resolve so the code matches the contract. 2. On Windows, fs.symlink(..., 'dir') requires SeCreateSymbolicLinkPrivilege (admin / Developer Mode) and EPERMs on default consumer installs. Use 'junction' for directory entries on win32 — junctions are reparse points that achieve the same semantics without elevation. Keep 'dir' on POSIX and 'file' for non-directory sources (no junction-equivalent for files; rare path). Adds an integration test exercising `foo/../bar` to lock in the syntactic guard; existing absolute-path and traversal tests already covered the other rejection forms. * fix(worktree): PR-worktree HEAD-SHA capture + symlink guard tests Three findings from wenshao round 4 (PR #4381): 1. For --worktree=#42 (PR worktrees), originalHeadCommit was captured from the parent repo's HEAD via getCurrentCommitHash() — but the worktree branches off FETCH_HEAD (the PR tip), not main. Downstream, WorktreeExitDialog's `rev-list <originalHeadCommit>..HEAD` would count every commit in the fetched PR as "new work this session" alongside the user's actual commits. Same root cause covers the FETCH_HEAD TOCTOU window: between `git fetch origin pull/<N>/head` and `git worktree add ... FETCH_HEAD`, a concurrent `git fetch` from any other process sharing this repo could overwrite .git/FETCH_HEAD, causing the worktree to branch off an unrelated commit. Fix: add GitWorktreeService.resolveRef(ref) that returns a 40-char SHA (or null). In setupStartupWorktree, immediately after fetchPullRequestRef succeeds, resolve FETCH_HEAD to an immutable SHA; pass that SHA both as the baseRef to createUserWorktree (closes the TOCTOU) AND as originalHeadCommit in the returned context (closes the exit-dialog miscount). Fail-close on null resolve. 2. Orphaned JSDoc block at gitWorktreeService.ts:1035-1048 — originally wrote validateUserWorktreeSlug's docs, stranded above parsePRReference after that function was inserted between them. Move the block down to sit immediately above validateUserWorktreeSlug at its current line. 3. `.git` / `.qwen` symlink rejection guards (~20 lines of security- critical code at gitWorktreeService.ts:1640-1655) had no regression tests — only absolute paths, `..` traversal, isWithinRoot escapes, and missing sources were covered. Add two integ tests in gitWorktreeService.symlinks.integ.test.ts: one asserts `.git/hooks` is refused, one asserts `.qwen/projects` is refused. Also extends the existing PR-worktree integration test in worktreeStartup.test.ts to assert originalHeadCommit equals the resolved FETCH_HEAD SHA AND does NOT equal the parent repo's main HEAD — the assertion would fail loudly if the new SHA-capture path were reverted. * fix(worktree): realpath check on symlinkDirectories source + dest paths Security fix from PR #4381 round 7 (wenshao/qwen3.7-max). The lexical isWithinRoot + .git/.qwen blocklist checks in symlinkConfiguredDirectories all operated on path.resolve(repoRoot, raw) — a STRING operation that doesn't follow symlinks. A committed (or out-of-band) symlink at <repo>/node_modules pointing into .git would pass every gate: 1. path.resolve gives `<repo>/node_modules` (lexical, passes isWithinRoot against repo root). 2. The .git/.qwen blocklists also see the lexical path — they don't detect that the realpath chains into .git. 3. fs.stat() follows the symlink and succeeds against .git/. 4. fs.symlink writes `<worktree>/node_modules → <repo>/node_modules`, which OS-side resolves through to <repo>/.git. Any tool inside the worktree that writes to node_modules/hooks/post-merge then has RCE on the next hook-firing git operation. Fix: after fs.stat succeeds, fs.realpath the source and RE-RUN the three containment checks against the realpath. Refuse on any escape. Use the realpath (not the lexical sourceAbs) as the symlink target so the new link is one-hop canonical rather than preserving the chain. Also closes the dest-side variant of the same root cause — flagged in round 4 thread #5 (declined then as overthinking) but now in scope per the skill's iteration rule (two consecutive rounds raising the same root-cause class). path.join(worktreePath, raw) is also lexical: if git worktree add materialized a committed worktree-level symlink (e.g. HEAD ships tools → /etc), then fs.mkdir / fs.symlink for a nested entry like "tools/cache" writes OUTSIDE the worktree. Realpath the dest parent before mkdir and refuse if it escapes the worktree. New integ test covers both source-side variants (escape-to-git via out-of-band symlink + escape-to-outside-dir) in one block. Was RED against the pre-fix code: <wt>/escape-to-git was created as a symlink that chained into the source repo's .git. GREEN after the fix. * fix(worktree): canonicalise repo root before symlinkDirectories checks Round-7's source-side realpath fix introduced a canonical-vs-lexical mismatch: `repoRootAbs = path.resolve(this.sourceRepoPath)` is purely lexical, while `realSource = await fs.realpath(sourceAbs)` is canonical. On macOS where `/tmp → /private/tmp` and `/var → /private/var` are ubiquitous, and on any Linux/Windows setup where the user's checkout sits behind a symlink, the prefixes diverge at the symlink boundary and `isWithinRoot(realSource, repoRootAbs)` silently rejects every configured entry. Production callers (worktreeStartup.ts, EnterWorktreeTool, agent isolation) all pass the lexical path returned by `git rev-parse --show-toplevel`. The integ tests masked the bug because the shared `beforeEach` did `repoRoot = await fs.realpath(dir)` upfront. Round 8 fix: - Hoist `repoRootAbs`, `gitDirAbs`, `qwenDirAbs`, and `realWorktreePath` outside the for-loop — they're loop invariants and were being recomputed once per entry. - `await fs.realpath(this.sourceRepoPath)` for `repoRootAbs` so every containment check below is canonical-vs-canonical. The derived `gitDirAbs` / `qwenDirAbs` blocklist paths inherit the canonical prefix automatically. `sourceAbs = path.resolve(repoRootAbs, raw)` inherits it too, so the early lexical reject paths (absolute, `..`, repo-root equality, isWithinRoot) stay self-consistent. - Fail-close: if the repo root itself doesn't realpath (deleted / inaccessible), bail out of the entire symlink loop rather than continuing with comparisons we can't trust. Non-destructive — the worktree was created earlier by `git worktree add`. New integ test provisions the production shape: a symlink path used as `sourceRepoPath`, distinct from its canonical realpath. RED on the pre-fix code (assertion fired with "symlinkDirectories entry was silently rejected — canonical vs lexical isWithinRoot mismatch"), GREEN after.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--resumecan recover worktree state, (2)core.hooksPathconfiguration inside new worktrees so commits run the main repo's hooks, (3) Footer worktree indicator +worktreefield in the StatusLine payload, (4)WorktreeExitDialogwith dirty-state inspection that intercepts the second Ctrl+C, and (5) a sharedrestoreWorktreeContexthelper that wires--resumeworktree restore through TUI, headless, AND ACP modes consistently.ada0837e2) was added after E2E Group C exposed that the original implementation only ran inAppContainer.tsx, leaving headless and ACP mode users without restore behavior.packages/core/src/services/worktreeSessionService.ts— shared sidecar I/O +restoreWorktreeContexthelper used by all three entry points.packages/cli/src/ui/AppContainer.tsx— both thedialogsVisiblefix (e847bfce8) and the resume-path refactor.packages/cli/src/nonInteractiveCli.tsandpackages/cli/src/acp-integration/{acpAgent,session/Session}.ts— each mode injects the worktree notice via its own mechanism (history INFO item /<system-reminder>prompt prefix / one-shotpendingWorktreeNotice).packages/core/src/services/gitWorktreeService.ts—.husky/preferred over.git/hooks, skips subprocess when value already matches (mirrors claude-code's optimization).Validation
Commands run:
Prompts / inputs used: see
docs/e2e-tests/worktree-phase-c.md(E2E plan covering Groups A–F + a headless--resumeretry).Expected vs Observed: all 17 E2E cases + 7 ACP integration tests pass on the post-fix build. The dialog visibility fix (
e847bfce8) was caught by Group E (0/5 → 5/5) and the three-mode refactor (ada0837e2) by Group C headless (0/2 → 2/2).Quickest reviewer verification path:
Evidence:
.husky/preferred when present; commit inside worktree triggers main-repopre-commitada0837e2refactor)worktree_restoredsystem message +<system-reminder>prompt prefix; stale sidecar cleaned⎇ worktree-X (X)appears / disappears in sync with sidecarkeepworktreepayload)⎇row suppressed when custom statusline is activeloadSessionsets pendingNotice only when worktree alive; firstprompt()injects + clears; subsequent prompts see no leakageScope / Risk
--resumerestore touches one entry point per mode but they each inject the notice differently (history INFO vs prompt prefix vs one-shot session field). A single shared helper (restoreWorktreeContext) ensures the cleanup + decision are identical; only the surface mechanism differs. The Phase A simplification still holds —Config.targetDiris never mutated, the worktree path travels via context messages, notprocess.chdir().--worktreeCLI flag,worktree.symlinkDirectories, sparse checkout,.worktreeinclude, tmux integration, PR ref parsing) are explicitly out of scope and documented indocs/design/worktree.md.simple-git,fs/promises, andexecFile, all platform-agnostic.enter_worktree/exit_worktreecallers keep working; the sidecar is created behind the scenes and ignored by older binaries that don't know about it.Testing Matrix
Testing matrix notes:
simple-git,fs/promises, andexecFile; no platform-specific code paths were added.Linked Issues / Bugs
Refs #4056 (Phase C of the generic worktree roadmap; Phase A+B shipped in #4073). Phase D (
--worktreeCLI flag,symlinkDirectories) and Future items (sparse checkout,.worktreeinclude, tmux, PR refs) are tracked separately indocs/design/worktree.md.🤖 Generated with Claude Code