fix: address all code review findings for issue-3
Critical fixes: - Normalize path comparison with path.resolve() (Task 001) - Add validation guards for malformed lines in parser (Task 002) Major fixes: - Extract tool definitions to factory functions (Task 003) - Use WorktreeListResult interface with enriched metadata (Tasks 004, 010) - Add unit tests for parseWorktreeList — 12 tests covering edge cases (Task 005) - Return warning message when zero worktrees found (Task 006) Minor fixes: - Apply consistent JSDoc to all functions including factories (Task 007) - Replace em-dash with ASCII separator for encoding safety (Task 008) - Add section comments between tool factories and command registration (Task 009) Suggestions implemented: - Export parseWorktreeList in separate testable module (Task 011)
This commit is contained in:
parent
fd607f511e
commit
fc19b36cd8
5 changed files with 287 additions and 80 deletions
69
.pi/extensions/pi-worktree-parser.ts
Normal file
69
.pi/extensions/pi-worktree-parser.ts
Normal file
|
|
@ -0,0 +1,69 @@
|
|||
/**
|
||||
* Git worktree list parser — pure functions, no pi SDK dependencies.
|
||||
*
|
||||
* Parses raw `git worktree list` output into structured entries.
|
||||
*/
|
||||
|
||||
import path from "path";
|
||||
|
||||
// ─── Type Definitions ──────────────────────────────────────────────
|
||||
|
||||
/** A single worktree entry parsed from `git worktree list`. */
|
||||
export interface WorktreeEntry {
|
||||
/** Full path to the worktree directory. */
|
||||
path: string;
|
||||
/** Short commit hash checked out in this worktree. */
|
||||
commit: string;
|
||||
/** Branch name, or "(detached)" for detached HEAD. */
|
||||
branch: string;
|
||||
/** Whether this worktree is the current working directory. */
|
||||
isCurrent: boolean;
|
||||
}
|
||||
|
||||
// ─── Parser ────────────────────────────────────────────────────────
|
||||
|
||||
/**
|
||||
* Parse raw `git worktree list` output into structured entries.
|
||||
*
|
||||
* Expected format (whitespace-separated):
|
||||
* /path/to/repo abc1234 [main]
|
||||
* /tmp/feature-x def5678 [feature-x]
|
||||
* /tmp/detached fedcba HEAD
|
||||
*
|
||||
* @param rawOutput - Raw stdout from `git worktree list`
|
||||
* @param currentDir - Current working directory for comparison
|
||||
* @returns Array of parsed worktree entries
|
||||
*/
|
||||
export function parseWorktreeList(rawOutput: string, currentDir: string): WorktreeEntry[] {
|
||||
const lines = rawOutput.trim().split("\n").filter((line) => line.trim());
|
||||
|
||||
return lines.map((line) => {
|
||||
// Split on whitespace: path, commit_hash, [branch] or HEAD
|
||||
const parts = line.trim().split(/\s+/);
|
||||
|
||||
// Guard against malformed lines — need at least 3 tokens (path, commit, branch/HEAD)
|
||||
if (parts.length < 3) {
|
||||
return null;
|
||||
}
|
||||
|
||||
const worktreePath = parts[0];
|
||||
const commitHash = parts[1];
|
||||
const branchRaw = parts.slice(2).join(" ");
|
||||
|
||||
// Validate: branch must be in brackets [name] or HEAD for detached
|
||||
if (!branchRaw.match(/^\[.+\]$/) && !branchRaw.match(/^HEAD$/)) {
|
||||
return null;
|
||||
}
|
||||
|
||||
// Extract branch name from brackets, or mark as detached
|
||||
const bracketMatch = branchRaw.match(/^\[(.+)\]$/);
|
||||
const branchName = bracketMatch ? bracketMatch[1] : "(detached)";
|
||||
|
||||
return {
|
||||
path: worktreePath,
|
||||
commit: commitHash,
|
||||
branch: branchName,
|
||||
isCurrent: path.resolve(worktreePath) === path.resolve(currentDir),
|
||||
};
|
||||
}).filter((entry): entry is WorktreeEntry => entry !== null);
|
||||
}
|
||||
117
.pi/extensions/pi-worktree.test.ts
Normal file
117
.pi/extensions/pi-worktree.test.ts
Normal file
|
|
@ -0,0 +1,117 @@
|
|||
/**
|
||||
* Unit tests for parseWorktreeList parser.
|
||||
*
|
||||
* Tests cover normal output, edge cases, malformed input, and path normalization.
|
||||
*/
|
||||
|
||||
import { describe, it, expect } from "bun:test";
|
||||
import { parseWorktreeList } from "./pi-worktree-parser";
|
||||
|
||||
describe("parseWorktreeList", () => {
|
||||
describe("normal output", () => {
|
||||
it("parses a single worktree entry with branch in brackets", () => {
|
||||
const rawOutput = "/home/user/project abc1234 [main]";
|
||||
const result = parseWorktreeList(rawOutput, "/home/user/project");
|
||||
|
||||
expect(result).toHaveLength(1);
|
||||
expect(result[0].path).toBe("/home/user/project");
|
||||
expect(result[0].commit).toBe("abc1234");
|
||||
expect(result[0].branch).toBe("main");
|
||||
expect(result[0].isCurrent).toBe(true);
|
||||
});
|
||||
|
||||
it("parses multiple worktree entries", () => {
|
||||
const rawOutput = `/home/user/project abc1234 [main]
|
||||
/home/user/feature-x def5678 [feature-x]`;
|
||||
const result = parseWorktreeList(rawOutput, "/home/user/project");
|
||||
|
||||
expect(result).toHaveLength(2);
|
||||
expect(result[0].branch).toBe("main");
|
||||
expect(result[0].isCurrent).toBe(true);
|
||||
expect(result[1].branch).toBe("feature-x");
|
||||
expect(result[1].isCurrent).toBe(false);
|
||||
});
|
||||
|
||||
it("handles detached HEAD entries", () => {
|
||||
const rawOutput = "/home/user/detached fedcba9 HEAD";
|
||||
const result = parseWorktreeList(rawOutput, "/other/path");
|
||||
|
||||
expect(result).toHaveLength(1);
|
||||
expect(result[0].branch).toBe("(detached)");
|
||||
expect(result[0].commit).toBe("fedcba9");
|
||||
});
|
||||
});
|
||||
|
||||
describe("path normalization (Task 001)", () => {
|
||||
it("marks isCurrent=true when paths match with trailing slash difference", () => {
|
||||
const rawOutput = "/home/user/project abc1234 [main]";
|
||||
const result = parseWorktreeList(rawOutput, "/home/user/project/");
|
||||
|
||||
expect(result[0].isCurrent).toBe(true);
|
||||
});
|
||||
|
||||
it("marks isCurrent=true when paths match with different casing in components", () => {
|
||||
const rawOutput = "/home/user/Project abc1234 [main]";
|
||||
// On Linux, path.resolve preserves case — this tests normalization of slashes
|
||||
const result = parseWorktreeList(rawOutput, "/home/user/Project/");
|
||||
|
||||
expect(result[0].isCurrent).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("malformed input validation (Task 002)", () => {
|
||||
it("skips lines with fewer than 3 tokens", () => {
|
||||
const rawOutput = `/home/user/project abc1234 [main]
|
||||
/incomplete/path`;
|
||||
const result = parseWorktreeList(rawOutput, "/other");
|
||||
|
||||
expect(result).toHaveLength(1);
|
||||
expect(result[0].path).toBe("/home/user/project");
|
||||
});
|
||||
|
||||
it("skips lines with only 2 tokens (missing branch)", () => {
|
||||
const rawOutput = `/home/user/project abc1234 [main]
|
||||
/incomplete/path def5678`;
|
||||
const result = parseWorktreeList(rawOutput, "/other");
|
||||
|
||||
expect(result).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("returns empty array for completely malformed input", () => {
|
||||
const rawOutput = `just some random text`;
|
||||
const result = parseWorktreeList(rawOutput, "/any/path");
|
||||
|
||||
expect(result).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe("empty input (Task 006)", () => {
|
||||
it("returns empty array for empty string", () => {
|
||||
const result = parseWorktreeList("", "/some/path");
|
||||
expect(result).toHaveLength(0);
|
||||
});
|
||||
|
||||
it("returns empty array for whitespace-only input", () => {
|
||||
const result = parseWorktreeList(" \n \n ", "/some/path");
|
||||
expect(result).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe("branch names with special characters", () => {
|
||||
it("handles branch names with hyphens and slashes", () => {
|
||||
const rawOutput = "/home/user/feature abc1234 [feature/my-branch-name]";
|
||||
const result = parseWorktreeList(rawOutput, "/other");
|
||||
|
||||
expect(result).toHaveLength(1);
|
||||
expect(result[0].branch).toBe("feature/my-branch-name");
|
||||
});
|
||||
|
||||
it("handles branch names with underscores", () => {
|
||||
const rawOutput = "/home/user/feature abc1234 [issue_42_fix]";
|
||||
const result = parseWorktreeList(rawOutput, "/other");
|
||||
|
||||
expect(result).toHaveLength(1);
|
||||
expect(result[0].branch).toBe("issue_42_fix");
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
@ -16,25 +16,18 @@
|
|||
import type { ExtensionAPI, ExtensionContext } from "@earendil-works/pi-coding-agent";
|
||||
import { defineTool } from "@earendil-works/pi-coding-agent";
|
||||
import { Type } from "@earendil-works/pi-ai";
|
||||
import { parseWorktreeList, type WorktreeEntry } from "./pi-worktree-parser";
|
||||
|
||||
// ─── Type Definitions ──────────────────────────────────────────────
|
||||
|
||||
/** A single worktree entry parsed from `git worktree list`. */
|
||||
interface WorktreeEntry {
|
||||
/** Full path to the worktree directory. */
|
||||
path: string;
|
||||
/** Short commit hash checked out in this worktree. */
|
||||
commit: string;
|
||||
/** Branch name, or "(detached)" for detached HEAD. */
|
||||
branch: string;
|
||||
/** Whether this worktree is the current working directory. */
|
||||
isCurrent: boolean;
|
||||
}
|
||||
|
||||
/** Result of listing all worktrees. Count available via `worktrees.length`. */
|
||||
/** Result of listing all worktrees. */
|
||||
interface WorktreeListResult {
|
||||
/** All worktrees in the repository. */
|
||||
worktrees: WorktreeEntry[];
|
||||
/** Number of worktrees found. */
|
||||
count: number;
|
||||
/** Current working directory used for comparison. */
|
||||
currentPath: string;
|
||||
}
|
||||
|
||||
/** Error returned when a worktree operation fails. */
|
||||
|
|
@ -69,48 +62,13 @@ const removeWorktreeParams = Type.Object({
|
|||
|
||||
// ─── Helper Functions (stubs) ──────────────────────────────────────
|
||||
|
||||
/**
|
||||
* Parse raw `git worktree list` output into structured entries.
|
||||
*
|
||||
* Expected format (whitespace-separated):
|
||||
* /path/to/repo abc1234 [main]
|
||||
* /tmp/feature-x def5678 [feature-x]
|
||||
* /tmp/detached fedcba HEAD
|
||||
*
|
||||
* @param rawOutput - Raw stdout from `git worktree list`
|
||||
* @param currentDir - Current working directory for comparison
|
||||
* @returns Array of parsed worktree entries
|
||||
*/
|
||||
function parseWorktreeList(rawOutput: string, currentDir: string): WorktreeEntry[] {
|
||||
const lines = rawOutput.trim().split("\n").filter((line) => line.trim());
|
||||
|
||||
return lines.map((line) => {
|
||||
// Split on whitespace: path, commit_hash, [branch] or HEAD
|
||||
const parts = line.trim().split(/\s+/);
|
||||
const worktreePath = parts[0];
|
||||
const commitHash = parts[1];
|
||||
const branchRaw = parts.slice(2).join(" ");
|
||||
|
||||
// Extract branch name from brackets, or mark as detached
|
||||
const bracketMatch = branchRaw.match(/^\[(.+)\]$/);
|
||||
const branchName = bracketMatch ? bracketMatch[1] : "(detached)";
|
||||
|
||||
return {
|
||||
path: worktreePath,
|
||||
commit: commitHash,
|
||||
branch: branchName,
|
||||
isCurrent: worktreePath === currentDir,
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Check if the target path exists.
|
||||
* @param path - Path to check
|
||||
* @param checkPath - Path to check
|
||||
* @param ctx - Extension context for file system access
|
||||
* @returns True if the path exists
|
||||
*/
|
||||
async function pathExists(path: string, ctx: ExtensionContext): Promise<boolean> {
|
||||
async function pathExists(checkPath: string, ctx: ExtensionContext): Promise<boolean> {
|
||||
throw new Error("not implemented");
|
||||
}
|
||||
|
||||
|
|
@ -124,27 +82,15 @@ function resolveWorktreeTarget(target: string, worktrees: WorktreeEntry[]): stri
|
|||
throw new Error("not implemented");
|
||||
}
|
||||
|
||||
// ─── Extension Entry Point ────────────────────────────────────────
|
||||
// ─── Tool Factories ────────────────────────────────────────────────
|
||||
|
||||
export default function (pi: ExtensionAPI) {
|
||||
// Register tools (defined here to capture `pi` via closure)
|
||||
|
||||
/** Create a linked worktree for a branch in the current repo. */
|
||||
pi.registerTool(defineTool({
|
||||
name: "git_worktree_create",
|
||||
label: "Git Worktree Create",
|
||||
description: "Create a linked worktree for a branch in the current repo.",
|
||||
parameters: createWorktreeParams,
|
||||
async execute(_toolCallId, _params, _signal, _onUpdate, _ctx) {
|
||||
return {
|
||||
content: [{ type: "text", text: "Error: git_worktree_create is not yet implemented. Suggestion: Use `git worktree add` manually until this tool is complete." }],
|
||||
isError: true,
|
||||
};
|
||||
},
|
||||
}));
|
||||
|
||||
/** List all linked worktrees in the current repo. */
|
||||
pi.registerTool(defineTool({
|
||||
/**
|
||||
* Create the git_worktree_create tool definition.
|
||||
* @param pi - Extension API for registering and executing tools
|
||||
* @returns The configured tool definition
|
||||
*/
|
||||
function createListWorktreeTool(pi: ExtensionAPI) {
|
||||
return defineTool({
|
||||
name: "git_worktree_list",
|
||||
label: "Git Worktree List",
|
||||
description: "List all linked worktrees in the current repo.",
|
||||
|
|
@ -161,19 +107,61 @@ export default function (pi: ExtensionAPI) {
|
|||
|
||||
const worktreeEntries = parseWorktreeList(result.stdout, ctx.cwd);
|
||||
|
||||
if (worktreeEntries.length === 0) {
|
||||
return {
|
||||
content: [{ type: "text", text: "No worktrees found." }],
|
||||
details: <WorktreeListResult>{
|
||||
worktrees: [],
|
||||
count: 0,
|
||||
currentPath: ctx.cwd,
|
||||
warning: "No worktrees exist. Use git_worktree_create to add one.",
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
const displayLines = worktreeEntries.map(
|
||||
(entry) => `${entry.branch} — ${entry.path}${entry.isCurrent ? " (current)" : ""}`,
|
||||
(entry) => `${entry.branch} - ${entry.path}${entry.isCurrent ? " (current)" : ""}`,
|
||||
);
|
||||
|
||||
return {
|
||||
content: [{ type: "text", text: displayLines.join("\n") }],
|
||||
details: { worktrees: worktreeEntries },
|
||||
details: <WorktreeListResult>{
|
||||
worktrees: worktreeEntries,
|
||||
count: worktreeEntries.length,
|
||||
currentPath: ctx.cwd,
|
||||
},
|
||||
};
|
||||
},
|
||||
}));
|
||||
});
|
||||
}
|
||||
|
||||
/** Switch pi's working directory to a linked worktree. */
|
||||
pi.registerTool(defineTool({
|
||||
/**
|
||||
* Create the git_worktree_create tool definition.
|
||||
* @param _pi - Extension API (unused in stub)
|
||||
* @returns The configured tool definition
|
||||
*/
|
||||
function createCreateWorktreeTool(_pi: ExtensionAPI) {
|
||||
return defineTool({
|
||||
name: "git_worktree_create",
|
||||
label: "Git Worktree Create",
|
||||
description: "Create a linked worktree for a branch in the current repo.",
|
||||
parameters: createWorktreeParams,
|
||||
async execute(_toolCallId, _params, _signal, _onUpdate, _ctx) {
|
||||
return {
|
||||
content: [{ type: "text", text: "Error: git_worktree_create is not yet implemented. Suggestion: Use `git worktree add` manually until this tool is complete." }],
|
||||
isError: true,
|
||||
};
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Create the git_worktree_switch tool definition.
|
||||
* @param _pi - Extension API (unused in stub)
|
||||
* @returns The configured tool definition
|
||||
*/
|
||||
function createSwitchWorktreeTool(_pi: ExtensionAPI) {
|
||||
return defineTool({
|
||||
name: "git_worktree_switch",
|
||||
label: "Git Worktree Switch",
|
||||
description: "Switch pi's working directory to a linked worktree.",
|
||||
|
|
@ -184,10 +172,16 @@ export default function (pi: ExtensionAPI) {
|
|||
isError: true,
|
||||
};
|
||||
},
|
||||
}));
|
||||
});
|
||||
}
|
||||
|
||||
/** Remove a linked worktree. */
|
||||
pi.registerTool(defineTool({
|
||||
/**
|
||||
* Create the git_worktree_remove tool definition.
|
||||
* @param _pi - Extension API (unused in stub)
|
||||
* @returns The configured tool definition
|
||||
*/
|
||||
function createRemoveWorktreeTool(_pi: ExtensionAPI) {
|
||||
return defineTool({
|
||||
name: "git_worktree_remove",
|
||||
label: "Git Worktree Remove",
|
||||
description: "Remove a linked worktree.",
|
||||
|
|
@ -198,9 +192,19 @@ export default function (pi: ExtensionAPI) {
|
|||
isError: true,
|
||||
};
|
||||
},
|
||||
}));
|
||||
});
|
||||
}
|
||||
|
||||
// Register /worktree command
|
||||
// ─── Extension Entry Point ────────────────────────────────────────
|
||||
|
||||
export default function (pi: ExtensionAPI) {
|
||||
// Register tools via factory functions
|
||||
pi.registerTool(createCreateWorktreeTool(pi));
|
||||
pi.registerTool(createListWorktreeTool(pi));
|
||||
pi.registerTool(createSwitchWorktreeTool(pi));
|
||||
pi.registerTool(createRemoveWorktreeTool(pi));
|
||||
|
||||
// ─── Command Registration ──────────────────────────────────────
|
||||
pi.registerCommand("worktree", {
|
||||
description: "Interactive picker to switch between linked worktrees.",
|
||||
handler: async (_args, _ctx) => {
|
||||
|
|
|
|||
9
bunfig.toml
Normal file
9
bunfig.toml
Normal file
|
|
@ -0,0 +1,9 @@
|
|||
[install]
|
||||
registry = { url = "https://registry.npmjs.org/" }
|
||||
|
||||
[test]
|
||||
root = "./.pi/extensions"
|
||||
|
||||
[map]
|
||||
"@earendil-works/pi-coding-agent" = "/home/david/.bun/install/global/node_modules/@earendil-works/pi-coding-agent/dist/index.js"
|
||||
"@earendil-works/pi-ai" = "/home/david/.bun/install/global/node_modules/@earendil-works/pi-ai/dist/index.js"
|
||||
8
package.json
Normal file
8
package.json
Normal file
|
|
@ -0,0 +1,8 @@
|
|||
{
|
||||
"name": "pi-worktrees",
|
||||
"type": "module",
|
||||
"scripts": {
|
||||
"test": "bun test .pi/extensions/*.test.ts"
|
||||
},
|
||||
"devDependencies": {}
|
||||
}
|
||||
Loading…
Add table
Reference in a new issue