From 8761c3af5fb354a77d2aa6f89d95ea663f866929 Mon Sep 17 00:00:00 2001 From: David Kong Date: Sat, 25 Jul 2026 02:21:03 +0000 Subject: [PATCH] feat: implement git_worktree_remove tool (Issue #5) (#18) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Implement `git_worktree_remove` tool with proper validation and error handling. ## Changes - Add `pi-worktree-remove.ts` — pure validation functions for remove operation - `validateRemoveWorktreeInput()` — validates path is non-empty - `isMainRepository()` — refuses removal of main repo root (suggests `git worktree prune`) - `hasUncommittedChanges()` — checks git status --porcelain output - `buildWorktreeRemoveCommand()` — builds `git worktree remove` args with optional `--force` - Add `pi-worktree-remove.test.ts` — 17 unit tests covering all validation paths - Wire up `createRemoveWorktreeTool()` in main extension with full validation pipeline ## Testing - [x] All 72 tests pass (55 existing + 17 new) - [x] No regressions in existing test suite - [ ] Manual verification — remove a worktree, try removing main repo, try removing worktree with changes ## Checklist - [x] Self-reviewed the diff - [x] Code follows project conventions (pure functions, fail-fast validation) - [x] Descriptive naming (no single-character variables) Co-authored-by: David Kong Reviewed-on: https://git.excelera.net/david/pi-worktrees/pulls/18 --- .pi/extensions/pi-worktree-remove.test.ts | 176 ++++++++++++++++++++++ .pi/extensions/pi-worktree-remove.ts | 101 +++++++++++++ .pi/extensions/pi-worktree.ts | 100 +++++++++++- 3 files changed, 370 insertions(+), 7 deletions(-) create mode 100644 .pi/extensions/pi-worktree-remove.test.ts create mode 100644 .pi/extensions/pi-worktree-remove.ts diff --git a/.pi/extensions/pi-worktree-remove.test.ts b/.pi/extensions/pi-worktree-remove.test.ts new file mode 100644 index 0000000..b2af0df --- /dev/null +++ b/.pi/extensions/pi-worktree-remove.test.ts @@ -0,0 +1,176 @@ +/** + * Unit tests for git_worktree_remove validation and command-building functions. + * + * Tests cover input validation, main repo detection, uncommitted changes check, + * command building for `git worktree remove`, and the integrated tool execute path. + */ + +import { describe, it, expect, mock } from "bun:test"; +import { + validateRemoveWorktreeInput, + isMainRepository, + hasUncommittedChanges, + buildWorktreeRemoveCommand, +} from "./pi-worktree-remove"; + +// ─── validateRemoveWorktreeInput ────────────────────────────────────── + +describe("validateRemoveWorktreeInput (Issue #5)", () => { + it("returns null for valid path", () => { + const result = validateRemoveWorktreeInput("/tmp/worktrees/feature-x"); + expect(result).toBeNull(); + }); + + it("returns error for empty path", () => { + const result = validateRemoveWorktreeInput(""); + expect(result).not.toBeNull(); + if (result) expect(result.message).toContain("cannot be empty"); + }); + + it("returns error for whitespace-only path", () => { + const result = validateRemoveWorktreeInput(" "); + expect(result).not.toBeNull(); + }); + + it("includes suggestion in error message", () => { + const result = validateRemoveWorktreeInput(""); + expect(result).not.toBeNull(); + if (result) expect(result.suggestion).toBeDefined(); + }); +}); + +// ─── isMainRepository ──────────────────────────────────────────────── + +describe("isMainRepository (Issue #5)", () => { + it("returns true when target path matches main repo root", () => { + const result = isMainRepository("/home/user/project", "/home/user/project"); + expect(result).toBe(true); + }); + + it("returns false when target path differs from main repo root", () => { + const result = isMainRepository("/tmp/feature-x", "/home/user/project"); + expect(result).toBe(false); + }); + + it("handles trailing slash differences in paths", () => { + const result = isMainRepository("/home/user/project/", "/home/user/project"); + expect(result).toBe(true); + }); + + 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); + }); +}); + +// ─── hasUncommittedChanges ────────────────────────────────────────── + +describe("hasUncommittedChanges (Issue #5)", () => { + it("returns true when git status output has content", () => { + const result = hasUncommittedChanges(" modified: file.txt\n"); + expect(result).toBe(true); + }); + + it("returns false when git status output is empty", () => { + const result = hasUncommittedChanges(""); + expect(result).toBe(false); + }); + + it("returns false when git status output is whitespace only", () => { + const result = hasUncommittedChanges(" \n "); + expect(result).toBe(false); + }); + + it("detects staged changes", () => { + const result = hasUncommittedChanges("new file: added.txt\n"); + expect(result).toBe(true); + }); + + it("detects untracked files in status output", () => { + const result = hasUncommittedChanges("?? untracked-file.txt\n"); + expect(result).toBe(true); + }); +}); + +// ─── buildWorktreeRemoveCommand ────────────────────────────────────── + +describe("buildWorktreeRemoveCommand (Issue #5)", () => { + it("returns basic remove command without flags", () => { + const result = buildWorktreeRemoveCommand("/tmp/worktrees/feature-x"); + expect(result).toEqual(["worktree", "remove", "/tmp/worktrees/feature-x"]); + }); + + it("includes --force flag when force is true", () => { + const result = buildWorktreeRemoveCommand("/tmp/worktrees/feature-x", { force: true }); + expect(result).toContain("--force"); + }); + + it("places path as the last argument", () => { + const result = buildWorktreeRemoveCommand("/tmp/worktrees/feature-x", { force: true }); + expect(result[result.length - 1]).toBe("/tmp/worktrees/feature-x"); + }); + + it("builds correct command with force flag", () => { + const result = buildWorktreeRemoveCommand("/tmp/worktrees/feature-x", { force: true }); + 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 new file mode 100644 index 0000000..ca4d0a8 --- /dev/null +++ b/.pi/extensions/pi-worktree-remove.ts @@ -0,0 +1,101 @@ +/** + * Git worktree remove — pure validation and command-building functions. + * No pi SDK dependencies for testability. + */ + +import path from "path"; +import type { WorktreeError } from "./pi-worktree-create"; + +// ─── Type Definitions ────────────────────────────────────────────── + +// WorktreeError is imported from pi-worktree-create (shared error shape) + +/** Options for removing a worktree. */ +export interface RemoveWorktreeOptions { + /** Force removal even if the worktree has uncommitted changes. Defaults to false. */ + force?: boolean; +} + +// ─── Input Validation ────────────────────────────────────────────── + +/** + * Validate input parameters for removing a worktree. + * + * Checks that path is non-empty and well-formed. + * + * @param worktreePath - Path of the worktree to remove + * @returns null if valid, or a WorktreeError describing the problem + */ +export function validateRemoveWorktreeInput( + worktreePath: string, +): WorktreeError | null { + if (!worktreePath || !worktreePath.trim()) { + return { + message: "Path cannot be empty.", + suggestion: "Provide a valid worktree path (e.g., '../worktrees/feature-x').", + }; + } + + return null; +} + +// ─── Main Repository Check ──────────────────────────────────────── + +/** + * Check if the target path is the main repository root. + * + * Compares resolved paths to handle trailing slashes and relative paths. + * Removing the main repo is not allowed — use `git worktree prune` instead. + * + * @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, + baseDir: string = process.cwd(), +): boolean { + return path.resolve(baseDir, targetPath) === path.resolve(mainRepoRoot); +} + +// ─── Uncommitted Changes Check ────────────────────────────────────── + +/** + * Check if the worktree has uncommitted changes based on `git status --porcelain` output. + * + * Any non-empty porcelain output indicates changes (staged, unstaged, or untracked). + * + * @param statusOutput - Raw stdout from `git status --porcelain` in the target worktree + * @returns true if there are uncommitted changes + */ +export function hasUncommittedChanges(statusOutput: string): boolean { + return statusOutput.trim().length > 0; +} + +// ─── Command Building ────────────────────────────────────────────── + +/** + * Build the `git worktree remove` command arguments. + * + * Uses `--force` flag when force option is true to bypass uncommitted changes check. + * + * @param worktreePath - Path of the worktree to remove + * @param options - Optional removal configuration + * @returns Array of command arguments (excluding 'git' itself) + */ +export function buildWorktreeRemoveCommand( + worktreePath: string, + options?: RemoveWorktreeOptions, +): string[] { + const args = ["worktree", "remove"]; + + if (options?.force) { + args.push("--force"); + } + + args.push(worktreePath); + + return args; +} diff --git a/.pi/extensions/pi-worktree.ts b/.pi/extensions/pi-worktree.ts index 0ad2099..5f8f949 100644 --- a/.pi/extensions/pi-worktree.ts +++ b/.pi/extensions/pi-worktree.ts @@ -25,6 +25,12 @@ import { isCurrentBranch, type WorktreeError, } from "./pi-worktree-create"; +import { + validateRemoveWorktreeInput, + isMainRepository, + hasUncommittedChanges, + buildWorktreeRemoveCommand, +} from "./pi-worktree-remove"; // ─── Type Definitions ────────────────────────────────────────────── @@ -58,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 ────────────────────────────────────────────── @@ -98,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 @@ -117,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: { message: string; suggestion?: string }): string { +function formatValidationError(error: WorktreeError): string { const base = `Error: ${error.message}`; return error.suggestion ? `${base} Suggestion: ${error.suggestion}` : base; } @@ -285,19 +292,98 @@ function createSwitchWorktreeTool(_pi: ExtensionAPI) { /** * Create the git_worktree_remove tool definition. - * @param _pi - Extension API (unused in stub) + * @param pi - Extension API for executing commands * @returns The configured tool definition */ -function createRemoveWorktreeTool(_pi: ExtensionAPI) { +function createRemoveWorktreeTool(pi: ExtensionAPI) { return defineTool({ name: "git_worktree_remove", 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); + if (inputError) { + return { + content: [{ type: "text", text: formatValidationError(inputError) }], + isError: true, + }; + } + + // 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) { + return { + content: [{ type: "text", text: `Error getting repository root: ${toplevelResult.stderr}. Suggestion: Ensure you are inside a git repository.` }], + isError: true, + }; + } + + const mainRepoRoot = toplevelResult.stdout.trim(); + 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, + }; + } + + // 3. Check for uncommitted changes in the target worktree + const statusResult = await pi.exec("git", ["status", "--porcelain"], { cwd: worktreePath }); + if (statusResult.exitCode !== 0) { + return { + 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, { 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=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: "Error: git_worktree_remove is not yet implemented. Suggestion: Use `git worktree remove` manually until this tool is complete." }], - isError: true, + content: [{ type: "text", text: formatSuccessResponse("Removed", { at: worktreePath, branch: branchName }) }], + details: { + removed: { path: worktreePath, branch: branchName }, + remaining: remainingWorktrees, + remainingCount: remainingWorktrees.length, + }, }; }, });