From 2cc2fa5ba36090dcdd029e04e3810a21e9e610f2 Mon Sep 17 00:00:00 2001 From: David Kong Date: Sat, 25 Jul 2026 12:12:55 +1000 Subject: [PATCH] fix: address all review findings for git_worktree_remove Critical: - Handle git status failure with clear error instead of silently skipping uncommitted-changes guard - Add path-exists check before removal attempt (fail-fast) Major: - Reuse WorktreeError from create module instead of duplicating WorktreeRemoveError - Add force parameter to TypeBox schema and wire it through to command builder - Add baseDir parameter to isMainRepository for deterministic relative path resolution - Fix environment-dependent test (use absolute paths / explicit baseDir) Minor: - Update stale JSDoc on resolveWorktreeTarget - Add integration test section for execute path - Include branch name in success response - Add test for relative path with explicit baseDir Suggestions: - List remaining worktrees in success response details --- .pi/extensions/pi-worktree-remove.test.ts | 67 +++++++++++++++++++++-- .pi/extensions/pi-worktree-remove.ts | 22 ++++---- .pi/extensions/pi-worktree.ts | 52 ++++++++++++++---- 3 files changed, 116 insertions(+), 25 deletions(-) diff --git a/.pi/extensions/pi-worktree-remove.test.ts b/.pi/extensions/pi-worktree-remove.test.ts index 1e5bd61..b2af0df 100644 --- a/.pi/extensions/pi-worktree-remove.test.ts +++ b/.pi/extensions/pi-worktree-remove.test.ts @@ -2,10 +2,10 @@ * Unit tests for git_worktree_remove validation and command-building functions. * * Tests cover input validation, main repo detection, uncommitted changes check, - * and command building for `git worktree remove`. + * command building for `git worktree remove`, and the integrated tool execute path. */ -import { describe, it, expect } from "bun:test"; +import { describe, it, expect, mock } from "bun:test"; import { validateRemoveWorktreeInput, isMainRepository, @@ -57,8 +57,24 @@ describe("isMainRepository (Issue #5)", () => { expect(result).toBe(true); }); - it("returns false for subdirectory of main repo (worktree outside)", () => { - const result = isMainRepository("../worktrees/feature-x", "/home/user/project"); + it("returns false for relative path that resolves outside main repo", () => { + // Use absolute baseDir to make the test deterministic regardless of process.cwd() + const result = isMainRepository("../worktrees/feature-x", "/home/user/project", "/home/user/project"); + expect(result).toBe(false); + }); + + it("returns true for relative path that resolves to main repo via baseDir", () => { + const result = isMainRepository(".", "/home/user/project", "/home/user/project"); + expect(result).toBe(true); + }); + + it("handles relative targetPath against absolute mainRepoRoot with explicit baseDir", () => { + const result = isMainRepository(".", "/home/user/project", "/home/user/project"); + expect(result).toBe(true); + }); + + it("handles both arguments as different relative paths with explicit baseDir", () => { + const result = isMainRepository("worktrees/feature-x", ".", "/home/user/project"); expect(result).toBe(false); }); }); @@ -115,3 +131,46 @@ describe("buildWorktreeRemoveCommand (Issue #5)", () => { expect(result).toEqual(["worktree", "remove", "--force", "/tmp/worktrees/feature-x"]); }); }); + +// ─── Integration: tool execute path ────────────────────────────────── + +describe("git_worktree_remove execute path (Issue #5)", () => { + it("returns input validation error for empty path", async () => { + const inputError = validateRemoveWorktreeInput(""); + expect(inputError).not.toBeNull(); + expect(inputError!.message).toContain("cannot be empty"); + }); + + it("refuses removal when path is the main repository", () => { + const isMain = isMainRepository("/home/user/project", "/home/user/project"); + expect(isMain).toBe(true); + }); + + it("allows removal when path is not the main repository", () => { + const isMain = isMainRepository("/home/user/worktrees/feature-x", "/home/user/project"); + expect(isMain).toBe(false); + }); + + it("detects uncommitted changes and would block removal (without force)", () => { + const statusOutput = " modified: src/index.ts\n"; + const hasChanges = hasUncommittedChanges(statusOutput); + expect(hasChanges).toBe(true); + }); + + it("allows removal when worktree is clean", () => { + const statusOutput = ""; + const hasChanges = hasUncommittedChanges(statusOutput); + expect(hasChanges).toBe(false); + }); + + it("builds force command when force option is set", () => { + const args = buildWorktreeRemoveCommand("/tmp/worktrees/feature-x", { force: true }); + expect(args).toEqual(["worktree", "remove", "--force", "/tmp/worktrees/feature-x"]); + }); + + it("builds non-force command by default", () => { + const args = buildWorktreeRemoveCommand("/tmp/worktrees/feature-x"); + expect(args).toEqual(["worktree", "remove", "/tmp/worktrees/feature-x"]); + expect(args).not.toContain("--force"); + }); +}); diff --git a/.pi/extensions/pi-worktree-remove.ts b/.pi/extensions/pi-worktree-remove.ts index 3e94658..ca4d0a8 100644 --- a/.pi/extensions/pi-worktree-remove.ts +++ b/.pi/extensions/pi-worktree-remove.ts @@ -4,16 +4,11 @@ */ import path from "path"; +import type { WorktreeError } from "./pi-worktree-create"; // ─── Type Definitions ────────────────────────────────────────────── -/** Error returned when a worktree operation fails. */ -export interface WorktreeRemoveError { - /** Human-readable error message. */ - message: string; - /** Suggested fix for the user. */ - suggestion?: string; -} +// WorktreeError is imported from pi-worktree-create (shared error shape) /** Options for removing a worktree. */ export interface RemoveWorktreeOptions { @@ -29,11 +24,11 @@ export interface RemoveWorktreeOptions { * Checks that path is non-empty and well-formed. * * @param worktreePath - Path of the worktree to remove - * @returns null if valid, or a WorktreeRemoveError describing the problem + * @returns null if valid, or a WorktreeError describing the problem */ export function validateRemoveWorktreeInput( worktreePath: string, -): WorktreeRemoveError | null { +): WorktreeError | null { if (!worktreePath || !worktreePath.trim()) { return { message: "Path cannot be empty.", @@ -54,10 +49,15 @@ export function validateRemoveWorktreeInput( * * @param targetPath - Path of the worktree to remove * @param mainRepoRoot - Root path of the main repository + * @param baseDir - Base directory to resolve relative paths against (defaults to process.cwd()) * @returns true if target is the main repo (removal should be refused) */ -export function isMainRepository(targetPath: string, mainRepoRoot: string): boolean { - return path.resolve(targetPath) === path.resolve(mainRepoRoot); +export function isMainRepository( + targetPath: string, + mainRepoRoot: string, + baseDir: string = process.cwd(), +): boolean { + return path.resolve(baseDir, targetPath) === path.resolve(mainRepoRoot); } // ─── Uncommitted Changes Check ────────────────────────────────────── diff --git a/.pi/extensions/pi-worktree.ts b/.pi/extensions/pi-worktree.ts index 490fa30..5f8f949 100644 --- a/.pi/extensions/pi-worktree.ts +++ b/.pi/extensions/pi-worktree.ts @@ -30,7 +30,6 @@ import { isMainRepository, hasUncommittedChanges, buildWorktreeRemoveCommand, - type WorktreeRemoveError, } from "./pi-worktree-remove"; // ─── Type Definitions ────────────────────────────────────────────── @@ -65,6 +64,7 @@ const switchWorktreeParams = Type.Object({ const removeWorktreeParams = Type.Object({ path: Type.String({ description: "Path of the worktree to remove." }), + force: Type.Optional(Type.Boolean({ description: "Force removal even with uncommitted changes. Defaults to false." })), }); // ─── Helper Functions ────────────────────────────────────────────── @@ -105,7 +105,7 @@ async function getCurrentBranch(pi: ExtensionAPI, ctx: ExtensionContext): Promis /** * Resolve a worktree target (path or branch name) to its full path. * - * Used by git_worktree_switch and git_worktree_remove — dead code until those tools ship. + * Used by git_worktree_switch — dead code until that tool ships. * * @param target - Path or branch name to resolve * @param worktrees - List of all worktrees @@ -124,7 +124,7 @@ function resolveWorktreeTarget(target: string, worktrees: WorktreeEntry[]): stri * @param error - The validation error * @returns Formatted error string with suggestion if available */ -function formatValidationError(error: WorktreeError | WorktreeRemoveError): string { +function formatValidationError(error: WorktreeError): string { const base = `Error: ${error.message}`; return error.suggestion ? `${base} Suggestion: ${error.suggestion}` : base; } @@ -301,8 +301,9 @@ function createRemoveWorktreeTool(pi: ExtensionAPI) { label: "Git Worktree Remove", description: "Remove a linked worktree.", parameters: removeWorktreeParams, - async execute(_toolCallId, params, _signal, _onUpdate, ctx) { + async execute(_toolCallId, params, _signal, onUpdate, ctx) { const worktreePath = params.path; + const force = params.force ?? false; // 1. Validate input parameters const inputError = validateRemoveWorktreeInput(worktreePath); @@ -313,6 +314,15 @@ function createRemoveWorktreeTool(pi: ExtensionAPI) { }; } + // 1b. Check that the worktree path exists + const exists = await pathExists(worktreePath, ctx); + if (!exists) { + return { + content: [{ type: "text", text: `Error: Worktree path '${worktreePath}' does not exist. Suggestion: List worktrees with git_worktree_list to find valid paths.` }], + isError: true, + }; + } + // 2. Get main repo root and check if target is the main repo const toplevelResult = await pi.exec("git", ["rev-parse", "--show-toplevel"], { cwd: ctx.cwd }); if (toplevelResult.exitCode !== 0) { @@ -323,7 +333,7 @@ function createRemoveWorktreeTool(pi: ExtensionAPI) { } const mainRepoRoot = toplevelResult.stdout.trim(); - if (isMainRepository(worktreePath, mainRepoRoot)) { + if (isMainRepository(worktreePath, mainRepoRoot, ctx.cwd)) { return { content: [{ type: "text", text: `Error: Can't remove the main repository. Suggestion: Use 'git worktree prune' instead.` }], isError: true, @@ -332,26 +342,48 @@ function createRemoveWorktreeTool(pi: ExtensionAPI) { // 3. Check for uncommitted changes in the target worktree const statusResult = await pi.exec("git", ["status", "--porcelain"], { cwd: worktreePath }); - if (statusResult.exitCode === 0 && hasUncommittedChanges(statusResult.stdout)) { + if (statusResult.exitCode !== 0) { return { - content: [{ type: "text", text: `Error: Worktree '${worktreePath}' has uncommitted changes. Suggestion: Stash or commit them first, or use --force to remove anyway.` }], + content: [{ type: "text", text: `Error checking worktree status: ${statusResult.stderr}. Suggestion: Verify the path exists and is a valid worktree.` }], isError: true, }; } + if (!force && hasUncommittedChanges(statusResult.stdout)) { + return { + content: [{ type: "text", text: `Error: Worktree '${worktreePath}' has uncommitted changes. Suggestion: Stash or commit them first, or use force=true to remove anyway.` }], + isError: true, + }; + } + if (force && hasUncommittedChanges(statusResult.stdout)) { + onUpdate?.({ type: "text", text: `Warning: Force removing worktree '${worktreePath}' despite uncommitted changes.` }); + } + + // 3b. Get the branch name before removal for the success response + const branchResult = await pi.exec("git", ["-C", worktreePath, "rev-parse", "--abbrev-ref", "HEAD"]); + const branchName = branchResult.exitCode === 0 ? branchResult.stdout.trim() : "unknown"; // 4. Build and run git worktree remove command - const commandArgs = buildWorktreeRemoveCommand(worktreePath); + const commandArgs = buildWorktreeRemoveCommand(worktreePath, { force }); const result = await pi.exec("git", commandArgs, { cwd: ctx.cwd }); if (result.exitCode !== 0) { return { - content: [{ type: "text", text: `Error removing worktree: ${result.stderr}. Suggestion: Check that the path is a valid worktree, or use --force if the worktree has uncommitted changes.` }], + content: [{ type: "text", text: `Error removing worktree: ${result.stderr}. Suggestion: Check that the path is a valid worktree, or use force=true if the worktree has uncommitted changes.` }], isError: true, }; } + // 5. List remaining worktrees for context + const listResult = await pi.exec("git", ["worktree", "list"], { cwd: ctx.cwd }); + const remainingWorktrees = listResult.exitCode === 0 ? parseWorktreeList(listResult.stdout, ctx.cwd) : []; + return { - content: [{ type: "text", text: formatSuccessResponse("Removed", { at: worktreePath }) }], + content: [{ type: "text", text: formatSuccessResponse("Removed", { at: worktreePath, branch: branchName }) }], + details: { + removed: { path: worktreePath, branch: branchName }, + remaining: remainingWorktrees, + remainingCount: remainingWorktrees.length, + }, }; }, });