From d4e921ae29ffaa638abeb1effff7ca4bc2cc4399 Mon Sep 17 00:00:00 2001 From: LaZzyMan Date: Mon, 18 May 2026 12:00:46 +0800 Subject: [PATCH] fix(worktree): address PR #4174 round 4 findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- packages/cli/src/ui/AppContainer.tsx | 44 +++++++++++++++---- .../src/services/worktreeSessionService.ts | 15 +++++++ 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index 59a3e80c15..3950df292e 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -1164,10 +1164,25 @@ export const AppContainer = (props: AppContainerProps) => { // removeUserWorktree returns {success, error} on failure — it // does NOT throw — so the previous try/catch never tripped on // 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.) + // so the next --resume can still see the worktree. Surface + // the error in history and stay in the session so the user + // can decide what to do (retry via exit_worktree, fix the + // underlying problem, or force-quit). Previously the dialog + // silently /quit on failure, contradicting the "discards N + // commits, M files" intent the user clicked Remove on. + // (Findings 3252368640 part 1 + 3256237933.) if (!result.success) { - handleSlashCommand('/quit'); + historyManager.addItem( + { + type: MessageType.ERROR, + text: + `Failed to remove worktree "${activeWorktree.slug}": ` + + `${result.error ?? 'unknown error'}. The worktree is ` + + `still on disk; use \`exit_worktree\` to retry or ` + + `remove it manually with \`git worktree remove\`.`, + }, + Date.now(), + ); return; } await clearWorktreeSession( @@ -1175,15 +1190,28 @@ export const AppContainer = (props: AppContainerProps) => { .getSessionService() .getWorktreeSessionPath(config.getSessionId()), ); - } catch { - // Hard failure (e.g. git binary missing) — proceed with quit - // anyway so the user isn't stranded. Sidecar stays so - // --resume can recover. + } catch (error) { + // Hard failure (e.g. git binary missing, GitWorktreeService + // constructor threw). Same treatment as the soft failure + // path: surface to the user and stay alive — silent /quit + // here would leave the user wondering whether the worktree + // was actually removed. + historyManager.addItem( + { + type: MessageType.ERROR, + text: + `Worktree removal failed for "${activeWorktree.slug}": ` + + `${error instanceof Error ? error.message : String(error)}. ` + + `Use \`exit_worktree\` or remove it manually.`, + }, + Date.now(), + ); + return; } } handleSlashCommand('/quit'); }, - [activeWorktree, config, handleSlashCommand], + [activeWorktree, config, handleSlashCommand, historyManager], ); const performMemoryRefresh = useCallback(async () => { diff --git a/packages/core/src/services/worktreeSessionService.ts b/packages/core/src/services/worktreeSessionService.ts index 0e35a633ab..3b37f96de2 100644 --- a/packages/core/src/services/worktreeSessionService.ts +++ b/packages/core/src/services/worktreeSessionService.ts @@ -21,6 +21,21 @@ export interface WorktreeSession { slug: string; worktreePath: string; worktreeBranch: string; + /** + * The repo top-level (output of `GitWorktreeService.getRepoTopLevel()`) + * captured when the worktree was created — NOT the user's launch cwd. + * + * Named `originalCwd` for on-disk back-compat with sidecars written + * by earlier Phase C builds; semantically this is the value to pass + * back to `new GitWorktreeService(...)` for any subsequent cleanup + * (e.g. `handleWorktreeExit`'s remove path), because the worktree + * always lives under `/.qwen/worktrees/`. When the + * CLI is launched from a monorepo subdirectory, `process.cwd()` and + * `getRepoTopLevel()` differ — this field stores the latter. + * + * Consumers expecting `process.cwd()` semantics should NOT use this + * field; capture cwd separately at the time of need. + */ originalCwd: string; originalBranch: string; /**