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
This commit is contained in:
parent
774e6005a3
commit
2cc2fa5ba3
3 changed files with 116 additions and 25 deletions
|
|
@ -2,10 +2,10 @@
|
||||||
* Unit tests for git_worktree_remove validation and command-building functions.
|
* Unit tests for git_worktree_remove validation and command-building functions.
|
||||||
*
|
*
|
||||||
* Tests cover input validation, main repo detection, uncommitted changes check,
|
* 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 {
|
import {
|
||||||
validateRemoveWorktreeInput,
|
validateRemoveWorktreeInput,
|
||||||
isMainRepository,
|
isMainRepository,
|
||||||
|
|
@ -57,8 +57,24 @@ describe("isMainRepository (Issue #5)", () => {
|
||||||
expect(result).toBe(true);
|
expect(result).toBe(true);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("returns false for subdirectory of main repo (worktree outside)", () => {
|
it("returns false for relative path that resolves outside main repo", () => {
|
||||||
const result = isMainRepository("../worktrees/feature-x", "/home/user/project");
|
// 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);
|
expect(result).toBe(false);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
@ -115,3 +131,46 @@ describe("buildWorktreeRemoveCommand (Issue #5)", () => {
|
||||||
expect(result).toEqual(["worktree", "remove", "--force", "/tmp/worktrees/feature-x"]);
|
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");
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
|
||||||
|
|
@ -4,16 +4,11 @@
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import path from "path";
|
import path from "path";
|
||||||
|
import type { WorktreeError } from "./pi-worktree-create";
|
||||||
|
|
||||||
// ─── Type Definitions ──────────────────────────────────────────────
|
// ─── Type Definitions ──────────────────────────────────────────────
|
||||||
|
|
||||||
/** Error returned when a worktree operation fails. */
|
// WorktreeError is imported from pi-worktree-create (shared error shape)
|
||||||
export interface WorktreeRemoveError {
|
|
||||||
/** Human-readable error message. */
|
|
||||||
message: string;
|
|
||||||
/** Suggested fix for the user. */
|
|
||||||
suggestion?: string;
|
|
||||||
}
|
|
||||||
|
|
||||||
/** Options for removing a worktree. */
|
/** Options for removing a worktree. */
|
||||||
export interface RemoveWorktreeOptions {
|
export interface RemoveWorktreeOptions {
|
||||||
|
|
@ -29,11 +24,11 @@ export interface RemoveWorktreeOptions {
|
||||||
* Checks that path is non-empty and well-formed.
|
* Checks that path is non-empty and well-formed.
|
||||||
*
|
*
|
||||||
* @param worktreePath - Path of the worktree to remove
|
* @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(
|
export function validateRemoveWorktreeInput(
|
||||||
worktreePath: string,
|
worktreePath: string,
|
||||||
): WorktreeRemoveError | null {
|
): WorktreeError | null {
|
||||||
if (!worktreePath || !worktreePath.trim()) {
|
if (!worktreePath || !worktreePath.trim()) {
|
||||||
return {
|
return {
|
||||||
message: "Path cannot be empty.",
|
message: "Path cannot be empty.",
|
||||||
|
|
@ -54,10 +49,15 @@ export function validateRemoveWorktreeInput(
|
||||||
*
|
*
|
||||||
* @param targetPath - Path of the worktree to remove
|
* @param targetPath - Path of the worktree to remove
|
||||||
* @param mainRepoRoot - Root path of the main repository
|
* @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)
|
* @returns true if target is the main repo (removal should be refused)
|
||||||
*/
|
*/
|
||||||
export function isMainRepository(targetPath: string, mainRepoRoot: string): boolean {
|
export function isMainRepository(
|
||||||
return path.resolve(targetPath) === path.resolve(mainRepoRoot);
|
targetPath: string,
|
||||||
|
mainRepoRoot: string,
|
||||||
|
baseDir: string = process.cwd(),
|
||||||
|
): boolean {
|
||||||
|
return path.resolve(baseDir, targetPath) === path.resolve(mainRepoRoot);
|
||||||
}
|
}
|
||||||
|
|
||||||
// ─── Uncommitted Changes Check ──────────────────────────────────────
|
// ─── Uncommitted Changes Check ──────────────────────────────────────
|
||||||
|
|
|
||||||
|
|
@ -30,7 +30,6 @@ import {
|
||||||
isMainRepository,
|
isMainRepository,
|
||||||
hasUncommittedChanges,
|
hasUncommittedChanges,
|
||||||
buildWorktreeRemoveCommand,
|
buildWorktreeRemoveCommand,
|
||||||
type WorktreeRemoveError,
|
|
||||||
} from "./pi-worktree-remove";
|
} from "./pi-worktree-remove";
|
||||||
|
|
||||||
// ─── Type Definitions ──────────────────────────────────────────────
|
// ─── Type Definitions ──────────────────────────────────────────────
|
||||||
|
|
@ -65,6 +64,7 @@ const switchWorktreeParams = Type.Object({
|
||||||
|
|
||||||
const removeWorktreeParams = Type.Object({
|
const removeWorktreeParams = Type.Object({
|
||||||
path: Type.String({ description: "Path of the worktree to remove." }),
|
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 ──────────────────────────────────────────────
|
// ─── 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.
|
* 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 target - Path or branch name to resolve
|
||||||
* @param worktrees - List of all worktrees
|
* @param worktrees - List of all worktrees
|
||||||
|
|
@ -124,7 +124,7 @@ function resolveWorktreeTarget(target: string, worktrees: WorktreeEntry[]): stri
|
||||||
* @param error - The validation error
|
* @param error - The validation error
|
||||||
* @returns Formatted error string with suggestion if available
|
* @returns Formatted error string with suggestion if available
|
||||||
*/
|
*/
|
||||||
function formatValidationError(error: WorktreeError | WorktreeRemoveError): string {
|
function formatValidationError(error: WorktreeError): string {
|
||||||
const base = `Error: ${error.message}`;
|
const base = `Error: ${error.message}`;
|
||||||
return error.suggestion ? `${base} Suggestion: ${error.suggestion}` : base;
|
return error.suggestion ? `${base} Suggestion: ${error.suggestion}` : base;
|
||||||
}
|
}
|
||||||
|
|
@ -301,8 +301,9 @@ function createRemoveWorktreeTool(pi: ExtensionAPI) {
|
||||||
label: "Git Worktree Remove",
|
label: "Git Worktree Remove",
|
||||||
description: "Remove a linked worktree.",
|
description: "Remove a linked worktree.",
|
||||||
parameters: removeWorktreeParams,
|
parameters: removeWorktreeParams,
|
||||||
async execute(_toolCallId, params, _signal, _onUpdate, ctx) {
|
async execute(_toolCallId, params, _signal, onUpdate, ctx) {
|
||||||
const worktreePath = params.path;
|
const worktreePath = params.path;
|
||||||
|
const force = params.force ?? false;
|
||||||
|
|
||||||
// 1. Validate input parameters
|
// 1. Validate input parameters
|
||||||
const inputError = validateRemoveWorktreeInput(worktreePath);
|
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
|
// 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 });
|
const toplevelResult = await pi.exec("git", ["rev-parse", "--show-toplevel"], { cwd: ctx.cwd });
|
||||||
if (toplevelResult.exitCode !== 0) {
|
if (toplevelResult.exitCode !== 0) {
|
||||||
|
|
@ -323,7 +333,7 @@ function createRemoveWorktreeTool(pi: ExtensionAPI) {
|
||||||
}
|
}
|
||||||
|
|
||||||
const mainRepoRoot = toplevelResult.stdout.trim();
|
const mainRepoRoot = toplevelResult.stdout.trim();
|
||||||
if (isMainRepository(worktreePath, mainRepoRoot)) {
|
if (isMainRepository(worktreePath, mainRepoRoot, ctx.cwd)) {
|
||||||
return {
|
return {
|
||||||
content: [{ type: "text", text: `Error: Can't remove the main repository. Suggestion: Use 'git worktree prune' instead.` }],
|
content: [{ type: "text", text: `Error: Can't remove the main repository. Suggestion: Use 'git worktree prune' instead.` }],
|
||||||
isError: true,
|
isError: true,
|
||||||
|
|
@ -332,26 +342,48 @@ function createRemoveWorktreeTool(pi: ExtensionAPI) {
|
||||||
|
|
||||||
// 3. Check for uncommitted changes in the target worktree
|
// 3. Check for uncommitted changes in the target worktree
|
||||||
const statusResult = await pi.exec("git", ["status", "--porcelain"], { cwd: worktreePath });
|
const statusResult = await pi.exec("git", ["status", "--porcelain"], { cwd: worktreePath });
|
||||||
if (statusResult.exitCode === 0 && hasUncommittedChanges(statusResult.stdout)) {
|
if (statusResult.exitCode !== 0) {
|
||||||
return {
|
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,
|
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
|
// 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 });
|
const result = await pi.exec("git", commandArgs, { cwd: ctx.cwd });
|
||||||
|
|
||||||
if (result.exitCode !== 0) {
|
if (result.exitCode !== 0) {
|
||||||
return {
|
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,
|
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 {
|
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,
|
||||||
|
},
|
||||||
};
|
};
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue