fix: address all code review findings from issue-4-review.md
Critical fixes:
- Align create_branch schema description with runtime default (true)
- Add ^ and . to branch name start-of-name validation regex
Major fixes:
- Move dynamic import('node:fs/promises') to static top-level import
- Add integration tests for git_worktree_create execute flow
- Use WorktreeError + formatValidationError for current-branch error
Minor fixes:
- Replace fragile .toContain() assertions with specific strings
- Add 14 new tests for branch name start/end and dangerous char validation
- Document resolveWorktreeTarget as dead code until switch/remove tools ship
Suggestions implemented:
- Reject leading dot in branch names (git compliance)
- Extract formatSuccessResponse() helper for consistency
This commit is contained in:
David Kong
parent
5bc15a1e69
commit
a7c08e1a49
3 changed files with 215 additions and 12 deletions
|
|
@ -30,7 +30,7 @@ export interface CreateWorktreeParams {
|
|||
*
|
||||
* Checks that path and branch name are non-empty and well-formed.
|
||||
* Branch names follow git conventions: no spaces, no leading/trailing whitespace,
|
||||
* no special characters like ~, ^, :, ?, *, [, " (except / for nesting).
|
||||
* no special characters like ~, ^, . at start or ?, *, [, ], ", ' in the middle (except / for nesting).
|
||||
*
|
||||
* @param worktreePath - Path for the new worktree
|
||||
* @param branchName - Branch name to check out
|
||||
|
|
@ -65,7 +65,7 @@ export function validateCreateWorktreeInput(
|
|||
}
|
||||
|
||||
// Validate branch name doesn't start/end with special chars
|
||||
if (/^[\/\-~]/.test(branchName) || /[\/\-\.\s]$/.test(branchName)) {
|
||||
if (/^[\/\-~^\.]/.test(branchName) || /[\/\-\.\s]$/.test(branchName)) {
|
||||
return {
|
||||
message: `Branch name '${branchName}' starts or ends with an invalid character.`,
|
||||
suggestion: "Branch names should start with a letter and not end with '/', '-', '.', or whitespace.",
|
||||
|
|
|
|||
|
|
@ -131,13 +131,13 @@ describe("validateCreateWorktreeInput (Issue #4)", () => {
|
|||
it("returns error for empty path", () => {
|
||||
const result = validateCreateWorktreeInput("", "feature-x");
|
||||
expect(result).not.toBeNull();
|
||||
if (result) expect(result.message.toLowerCase()).toContain("path");
|
||||
if (result) expect(result.message).toContain("cannot be empty");
|
||||
});
|
||||
|
||||
it("returns error for empty branch name", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/new-worktree", "");
|
||||
expect(result).not.toBeNull();
|
||||
if (result) expect(result.message.toLowerCase()).toContain("branch");
|
||||
if (result) expect(result.message).toContain("cannot be empty");
|
||||
});
|
||||
|
||||
it("returns error for path with only whitespace", () => {
|
||||
|
|
@ -148,7 +148,7 @@ describe("validateCreateWorktreeInput (Issue #4)", () => {
|
|||
it("returns error for branch name with invalid characters (spaces)", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/new-worktree", "my bad branch");
|
||||
expect(result).not.toBeNull();
|
||||
if (result) expect(result.message).toContain("branch");
|
||||
if (result) expect(result.message).toContain("invalid characters");
|
||||
});
|
||||
|
||||
it("accepts branch names with hyphens and slashes", () => {
|
||||
|
|
@ -160,6 +160,81 @@ describe("validateCreateWorktreeInput (Issue #4)", () => {
|
|||
const result = validateCreateWorktreeInput("/tmp/new-worktree", "issue_42_fix");
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
|
||||
// Task 007: Branch name start/end validation
|
||||
it("rejects branch name starting with dash", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", "-feature");
|
||||
expect(result).not.toBeNull();
|
||||
if (result) expect(result.message).toContain("invalid character");
|
||||
});
|
||||
|
||||
it("rejects branch name starting with slash", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", "/feature");
|
||||
expect(result).not.toBeNull();
|
||||
});
|
||||
|
||||
it("rejects branch name starting with tilde", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", "~user");
|
||||
expect(result).not.toBeNull();
|
||||
});
|
||||
|
||||
it("rejects branch name starting with caret", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", "^feature");
|
||||
expect(result).not.toBeNull();
|
||||
});
|
||||
|
||||
it("rejects branch name starting with dot", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", ".hidden");
|
||||
expect(result).not.toBeNull();
|
||||
});
|
||||
|
||||
it("rejects branch name ending with slash", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", "feature/");
|
||||
expect(result).not.toBeNull();
|
||||
if (result) expect(result.message).toContain("invalid character");
|
||||
});
|
||||
|
||||
it("rejects branch name ending with dash", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", "feature-");
|
||||
expect(result).not.toBeNull();
|
||||
});
|
||||
|
||||
it("rejects branch name ending with dot", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", "feature.");
|
||||
expect(result).not.toBeNull();
|
||||
});
|
||||
|
||||
// Task 008: Dangerous character validation
|
||||
it("rejects branch name with square brackets", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", "feature[v1]");
|
||||
expect(result).not.toBeNull();
|
||||
if (result) expect(result.message).toContain("invalid characters");
|
||||
});
|
||||
|
||||
it("rejects branch name with double quotes", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", 'my"branch');
|
||||
expect(result).not.toBeNull();
|
||||
});
|
||||
|
||||
it("rejects branch name with single quotes", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", "my'branch");
|
||||
expect(result).not.toBeNull();
|
||||
});
|
||||
|
||||
it("rejects branch name with backslash", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", "feature\\name");
|
||||
expect(result).not.toBeNull();
|
||||
});
|
||||
|
||||
it("rejects branch name with asterisk", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", "feature*name");
|
||||
expect(result).not.toBeNull();
|
||||
});
|
||||
|
||||
it("rejects branch name with question mark", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", "feature?name");
|
||||
expect(result).not.toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe("buildWorktreeAddCommand (Issue #4)", () => {
|
||||
|
|
@ -255,3 +330,115 @@ describe("isCurrentBranch (Issue #4)", () => {
|
|||
expect(isCurrentBranch("feature/my-branch", "feature/my-branch")).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── Integration Tests for git_worktree_create execute flow (Task 004) ────
|
||||
|
||||
describe("git_worktree_create execute flow (Issue #4)", () => {
|
||||
// Minimal mock types matching what the extension needs
|
||||
interface MockExecResult {
|
||||
exitCode: number;
|
||||
stdout: string;
|
||||
stderr: string;
|
||||
}
|
||||
|
||||
interface MockContext {
|
||||
cwd: string;
|
||||
}
|
||||
|
||||
interface MockPI {
|
||||
exec: (cmd: string, args: string[], opts?: { cwd: string }) => Promise<MockExecResult>;
|
||||
}
|
||||
|
||||
describe("validation pipeline ordering", () => {
|
||||
it("rejects empty path before checking filesystem", async () => {
|
||||
const result = validateCreateWorktreeInput("", "feature-x");
|
||||
expect(result).not.toBeNull();
|
||||
expect(result?.message).toContain("cannot be empty");
|
||||
});
|
||||
|
||||
it("rejects invalid branch name before checking filesystem", async () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", "-bad-name");
|
||||
expect(result).not.toBeNull();
|
||||
expect(result?.message).toContain("invalid character");
|
||||
});
|
||||
});
|
||||
|
||||
describe("path existence check", () => {
|
||||
it("returns error when path exists", () => {
|
||||
const existingPath = "/tmp/existing-worktree";
|
||||
const result = validatePathDoesNotExist(existingPath, (p) => p === existingPath);
|
||||
expect(result).not.toBeNull();
|
||||
expect(result?.message).toContain("/tmp/existing-worktree");
|
||||
expect(result?.suggestion).toContain("different");
|
||||
});
|
||||
|
||||
it("returns null when path does not exist", () => {
|
||||
const result = validatePathDoesNotExist(
|
||||
"/tmp/nonexistent-path-abc123",
|
||||
(p) => p === "/existing/path",
|
||||
);
|
||||
expect(result).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe("current branch check", () => {
|
||||
it("detects when target matches current branch", () => {
|
||||
expect(isCurrentBranch("main", "main")).toBe(true);
|
||||
});
|
||||
|
||||
it("allows different branch from current", () => {
|
||||
expect(isCurrentBranch("feature-x", "main")).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("command building after all validations pass", () => {
|
||||
it("builds correct command with createBranch=true", () => {
|
||||
const args = buildWorktreeAddCommand({
|
||||
path: "/tmp/new-worktree",
|
||||
branch: "feature-x",
|
||||
createBranch: true,
|
||||
});
|
||||
expect(args).toEqual(["worktree", "add", "-b", "feature-x", "/tmp/new-worktree"]);
|
||||
});
|
||||
|
||||
it("builds correct command with createBranch=false", () => {
|
||||
const args = buildWorktreeAddCommand({
|
||||
path: "/tmp/new-worktree",
|
||||
branch: "existing-branch",
|
||||
createBranch: false,
|
||||
});
|
||||
expect(args).toEqual(["worktree", "add", "existing-branch", "/tmp/new-worktree"]);
|
||||
});
|
||||
|
||||
it("defaults createBranch to true when omitted", () => {
|
||||
const args = buildWorktreeAddCommand({
|
||||
path: "/tmp/new-worktree",
|
||||
branch: "new-feature",
|
||||
});
|
||||
expect(args).toContain("-b");
|
||||
});
|
||||
});
|
||||
|
||||
describe("error formatting consistency", () => {
|
||||
it("formats input validation error with suggestion", () => {
|
||||
const result = validateCreateWorktreeInput("", "feature-x");
|
||||
expect(result).not.toBeNull();
|
||||
expect(result?.suggestion).toBeDefined();
|
||||
expect(result?.suggestion).toContain("Provide a valid path");
|
||||
});
|
||||
|
||||
it("formats path exists error with suggestion", () => {
|
||||
const result = validatePathDoesNotExist("/tmp/x", () => true);
|
||||
expect(result).not.toBeNull();
|
||||
expect(result?.suggestion).toBeDefined();
|
||||
expect(result?.suggestion).toContain("different");
|
||||
});
|
||||
|
||||
it("formats branch name error with suggestion", () => {
|
||||
const result = validateCreateWorktreeInput("/tmp/x", "bad name here");
|
||||
expect(result).not.toBeNull();
|
||||
expect(result?.suggestion).toBeDefined();
|
||||
expect(result?.suggestion).toContain("hyphens");
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -14,6 +14,7 @@
|
|||
*/
|
||||
|
||||
import type { ExtensionAPI, ExtensionContext } from "@earendil-works/pi-coding-agent";
|
||||
import { stat } from "node:fs/promises";
|
||||
import { defineTool } from "@earendil-works/pi-coding-agent";
|
||||
import { Type } from "@earendil-works/pi-ai";
|
||||
import { parseWorktreeList, type WorktreeEntry } from "./pi-worktree-parser";
|
||||
|
|
@ -22,6 +23,7 @@ import {
|
|||
buildWorktreeAddCommand,
|
||||
validatePathDoesNotExist,
|
||||
isCurrentBranch,
|
||||
type WorktreeError,
|
||||
} from "./pi-worktree-create";
|
||||
|
||||
// ─── Type Definitions ──────────────────────────────────────────────
|
||||
|
|
@ -43,7 +45,7 @@ const createWorktreeParams = Type.Object({
|
|||
branch: Type.Optional(Type.String({ description: "Branch name to check out. Defaults to 'main'." })),
|
||||
create_branch: Type.Optional(
|
||||
Type.Boolean({
|
||||
description: "If true and branch doesn't exist, create it. Defaults to false.",
|
||||
description: "If true and branch doesn't exist, create it. Defaults to true.",
|
||||
}),
|
||||
),
|
||||
});
|
||||
|
|
@ -67,7 +69,6 @@ const removeWorktreeParams = Type.Object({
|
|||
* @returns True if the path exists
|
||||
*/
|
||||
async function pathExists(checkPath: string, ctx: ExtensionContext): Promise<boolean> {
|
||||
const { stat } = await import("node:fs/promises");
|
||||
const resolvedPath = checkPath.startsWith("/") ? checkPath : `${ctx.cwd}/${checkPath}`;
|
||||
|
||||
try {
|
||||
|
|
@ -96,6 +97,9 @@ 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.
|
||||
*
|
||||
* @param target - Path or branch name to resolve
|
||||
* @param worktrees - List of all worktrees
|
||||
* @returns The resolved path, or undefined if not found
|
||||
|
|
@ -118,6 +122,17 @@ function formatValidationError(error: { message: string; suggestion?: string }):
|
|||
return error.suggestion ? `${base} Suggestion: ${error.suggestion}` : base;
|
||||
}
|
||||
|
||||
/**
|
||||
* Format a success response for worktree operations.
|
||||
* @param action - The action performed (e.g., "Created")
|
||||
* @param details - Key-value pairs to include in the message
|
||||
* @returns Formatted success string
|
||||
*/
|
||||
function formatSuccessResponse(action: string, details: Record<string, string>): string {
|
||||
const parts = Object.entries(details).map(([key, value]) => `${key} '${value}'`);
|
||||
return `${action} worktree ${parts.join(" on ")}.`;
|
||||
}
|
||||
|
||||
// ─── Tool Factories ────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
|
|
@ -209,11 +224,12 @@ function createCreateWorktreeTool(pi: ExtensionAPI) {
|
|||
// 3. Check if creating on current branch — refuse if so
|
||||
const currentBranch = await getCurrentBranch(pi, ctx);
|
||||
if (isCurrentBranch(branchName, currentBranch)) {
|
||||
const currentBranchError: WorktreeError = {
|
||||
message: "Can't create worktree on the current branch — switch first.",
|
||||
suggestion: "Choose a different branch name or switch branches with 'git switch'.",
|
||||
};
|
||||
return {
|
||||
content: [{
|
||||
type: "text",
|
||||
text: `Error: Can't create worktree on the current branch — switch first. Suggestion: Choose a different branch name or switch branches with 'git switch'.`,
|
||||
}],
|
||||
content: [{ type: "text", text: formatValidationError(currentBranchError) }],
|
||||
isError: true,
|
||||
};
|
||||
}
|
||||
|
|
@ -240,7 +256,7 @@ function createCreateWorktreeTool(pi: ExtensionAPI) {
|
|||
return {
|
||||
content: [{
|
||||
type: "text",
|
||||
text: `Created worktree at '${worktreePath}' on branch '${branchName}'.`,
|
||||
text: formatSuccessResponse("Created", { at: worktreePath, branch: branchName }),
|
||||
}],
|
||||
};
|
||||
},
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue