From 6c696f282d7ffb89c3bced44e8acd95a2d81611f Mon Sep 17 00:00:00 2001 From: David Kong Date: Fri, 24 Jul 2026 08:43:34 +0000 Subject: [PATCH] Fix code review issues: structured error returns, remove unused count, add JSDoc (#14) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Code Review: issue-1 vs main **Date:** 2025-07-24 **Language:** TypeScript (pi extension) **Files changed:** 4 **Lines added:** +246 / Lines removed: -0 ## Files Under Review | File | Status | Purpose | |------|--------|---------| | `.pi/extensions/pi-worktree.ts` | New | Extension scaffold — types, schemas, tool/command stubs | | `findings.md` | New | Research findings and session discoveries | | `progress.md` | New | Progress log with TDD phase tracking | | `task_plan.md` | New | Task plan for issue #1 implementation | --- ## Critical (0) _None._ ## Major (2) ### [MAJOR] Execute functions throw errors instead of returning structured error objects **File:** `.pi/extensions/pi-worktree.ts` **Lines:** 97, 105, 113, 121, 138 **Detail:** All `execute()` functions and the command handler use `throw new Error("not implemented")`. Per AGENTS.md error handling pattern, tools should return structured objects: ```typescript return { content: [{ type: "text", text: `Error: ${message}. Suggestion: ` }], isError: true }; ``` Even stub implementations should follow this pattern so the extension loads without unhandled exceptions. Throwing from an execute function will crash the tool invocation rather than surfacing a clean error to the user. **Fix:** Replace `throw new Error("not implemented")` with structured return objects that include a suggestion, e.g.: ```typescript async execute(_toolCallId, _params, _signal, _onUpdate, _ctx) { return { content: [{ type: "text", text: "Error: git_worktree_create not yet implemented. Suggestion: Use `git worktree add` manually until this tool is complete." }], isError: true }; } ``` ### [MAJOR] `WorktreeListResult.count` field has no consumer and duplicates available info **File:** `.pi/extensions/pi-worktree.ts` **Line:** 32 **Detail:** The interface defines `count: number` but nothing in the scaffold uses it. Since callers can derive count from `worktrees.length`, this field adds maintenance burden (must stay in sync) without clear value. If included for API convenience, document why it's separate from `.length`. **Fix:** Either remove `count` from the interface or add a comment justifying its presence (e.g., "cached to avoid repeated .length calls" or "included for tool response schema consistency"). --- ## Minor (3) ### [MINOR] `WorktreeError` interface is defined but unused **File:** `.pi/extensions/pi-worktree.ts` **Lines:** 38–43 **Detail:** The `WorktreeError` interface (`message`, `suggestion?`) is well-designed and aligns with AGENTS.md conventions, but isn't referenced anywhere in the current code. It will be needed when implementing error returns. **Fix:** No action needed for scaffold phase — this is forward-looking type design. Confirm it's used when implementing actual tool logic. ### [MINOR] `ExtensionContext` imported but only used indirectly via type definitions **File:** `.pi/extensions/pi-worktree.ts` **Line:** 16 **Detail:** `ExtensionContext` is imported as a type and referenced in the `pathExists()` helper signature. This is correct forward-looking design — it will be needed when implementing file system checks. No issue, just noting for completeness. ### [MINOR] Tool definitions lack JSDoc comments unlike helper functions **File:** `.pi/extensions/pi-worktree.ts` **Lines:** 95–123 **Detail:** Helper functions (`parseWorktreeList`, `pathExists`, `resolveWorktreeTarget`) have thorough JSDoc with parameter descriptions and return types. The tool definitions (`gitWorktreeCreateTool`, etc.) have no comments. For a single-file extension, inline documentation helps future maintainers understand each tool's contract at a glance. **Fix:** Add brief JSDoc above each `defineTool()` call summarizing its purpose and key behaviors: ```typescript /** * Create a linked worktree for a branch. * Runs `git worktree add [branch]` with optional `-b` flag. */ const gitWorktreeCreateTool = defineTool({ ... }); ``` --- ## Suggestions (2) ### [SUGGESTION] Consider adding a `pi.exec()` usage example in comments for future implementers **File:** `.pi/extensions/pi-worktree.ts` **Detail:** AGENTS.md specifies "Git: native via `pi.exec("git", [...])`" but the scaffold has no indication of how git commands will be invoked. A comment near the helper functions showing the expected pattern would reduce friction for the next implementation phase: ```typescript // Example: const result = await ctx.exec("git", ["worktree", "add", path, branch]); ``` ### [SUGGESTION] `task_plan.md` lists only Task 001 but milestone context references Tasks 1.1–1.3 **File:** `task_plan.md` **Lines:** 6, 12–14 **Detail:** The task table has one entry (Task 001) marked done, but the milestone context below references three subtasks (1.1: Create Extension File, 1.2: Register Tool Scaffolding, 1.3: Implement `git_worktree_list`). This creates ambiguity about whether Task 001 encompasses all three or if 1.2 and 1.3 are still pending. **Fix:** Either expand the task table to list subtasks 1.1–1.3 separately with individual status, or clarify that Task 001 covers all three milestones. --- ## Summary | Severity | Count | |----------|-------| | Critical | 0 | | Major | 2 | | Minor | 3 | | Suggestions | 2 | | **Total** | **7** | ⚡ No critical issues. Major issues should be addressed before the extension is loadable in pi — specifically, execute functions must return structured error objects instead of throwing to avoid unhandled exceptions at runtime. --- ## Review Fix Plan | Task | Issue | Severity | File | Description | |------|-------|----------|------|-------------| | 001 | Execute stubs throw instead of returning errors | major | `.pi/extensions/pi-worktree.ts` | Replace `throw new Error()` with structured `{ content, isError: true }` returns in all execute functions and command handler | | 002 | Unused `count` field in `WorktreeListResult` | major | `.pi/extensions/pi-worktree.ts` | Remove or document the `count` field | | 003 | Tool definitions lack JSDoc | minor | `.pi/extensions/pi-worktree.ts` | Add JSDoc comments above each `defineTool()` call | | 004 | Task plan subtask ambiguity | suggestion | `task_plan.md` | Clarify relationship between Task 001 and milestone subtasks 1.1–1.3 | ### Task 001: Execute stubs throw instead of returning errors **Severity:** major **File:** `.pi/extensions/pi-worktree.ts` **Lines:** 97, 105, 113, 121, 138 **Issue:** All five execute/handler functions use `throw new Error("not implemented")`. When pi invokes a tool, an uncaught exception will crash the tool invocation rather than returning a clean error message to the user. AGENTS.md specifies the pattern: return `{ content: [{ type: "text", text }], isError: true }`. **Fix:** For each of the 4 tools and 1 command handler, replace the throw with: ```typescript async execute(_toolCallId, _params, _signal, _onUpdate, _ctx) { return { content: [{ type: "text", text: `Error: is not yet implemented. Suggestion: Use git worktree commands directly until this tool is complete.` }], isError: true }; } ``` **Verification:** Extension loads in pi without errors; calling any tool returns an error message instead of crashing. ### Task 002: Unused `count` field in `WorktreeListResult` **Severity:** major **File:** `.pi/extensions/pi-worktree.ts` **Line:** 32 **Issue:** `count: number` is defined but never populated or consumed. Callers can use `worktrees.length`. Dead fields create maintenance burden and confusion about whether they're intentional API design or leftover scaffolding. **Fix:** Remove the `count` field from `WorktreeListResult`: ```typescript interface WorktreeListResult { worktrees: WorktreeEntry[]; } ``` Or add a comment if intentionally kept for API schema reasons. **Verification:** No references to `.count` in code; interface matches actual usage. ### Task 003: Tool definitions lack JSDoc **Severity:** minor **File:** `.pi/extensions/pi-worktree.ts` **Lines:** 95–123 **Issue:** Helper functions have thorough JSDoc but tool definitions don't. In a single-file extension, inline docs help readers scan the file and understand each tool's purpose without reading implementation details. **Fix:** Add JSDoc above each `defineTool()`: ```typescript /** Create a linked worktree for a branch in the current repo. */ const gitWorktreeCreateTool = defineTool({ ... }); /** List all linked worktrees in the current repo. */ const gitWorktreeListTool = defineTool({ ... }); /** Switch pi's working directory to a linked worktree. */ const gitWorktreeSwitchTool = defineTool({ ... }); /** Remove a linked worktree. */ const gitWorktreeRemoveTool = defineTool({ ... }); ``` **Verification:** Each tool definition has a JSDoc comment matching its description field. ### Task 004: Task plan subtask ambiguity **Severity:** suggestion **File:** `task_plan.md` **Lines:** 6, 12–14 **Issue:** The task table shows one completed Task 001, but the milestone context references three distinct subtasks (1.1, 1.2, 1.3). It's unclear whether all are done or only partially complete. **Fix:** Expand the task table: | Task | Description | Done | |------|-------------|------| | 001.1 | Create extension file with correct imports | ☑ | | 001.2 | Register tool scaffolding (4 tools + command) | ☑ | | 001.3 | Implement `git_worktree_list` | ☐ | **Verification:** Task plan clearly shows what's done vs pending within the issue scope. Co-authored-by: David Kong Reviewed-on: https://git.excelera.net/david/pi-worktrees/pulls/14 --- .gitignore | 5 + .pi/extensions/pi-worktree.ts | 179 ++++++++++++++++++++++++++++++++++ 2 files changed, 184 insertions(+) create mode 100644 .gitignore create mode 100644 .pi/extensions/pi-worktree.ts diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..8fa6188 --- /dev/null +++ b/.gitignore @@ -0,0 +1,5 @@ +# Planning files (local working documents) +findings.md +progress.md +task_plan.md +issue-*-review.md diff --git a/.pi/extensions/pi-worktree.ts b/.pi/extensions/pi-worktree.ts new file mode 100644 index 0000000..ea6bbbe --- /dev/null +++ b/.pi/extensions/pi-worktree.ts @@ -0,0 +1,179 @@ +/** + * pi-worktree Extension + * + * Manages git worktrees through tool commands and an interactive `/worktree` command. + * + * Tools: + * - git_worktree_create — Create a linked worktree for a branch + * - git_worktree_list — List all linked worktrees + * - git_worktree_switch — Switch working directory to a worktree + * - git_worktree_remove — Remove a linked worktree + * + * Command: + * - /worktree — Interactive picker to switch between worktrees + */ + +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"; + +// ─── 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`. */ +interface WorktreeListResult { + /** All worktrees in the repository. */ + worktrees: WorktreeEntry[]; +} + +/** Error returned when a worktree operation fails. */ +interface WorktreeError { + /** Human-readable error message from git or validation. */ + message: string; + /** Suggested fix for the user. */ + suggestion?: string; +} + +// ─── Tool Parameter Schemas ──────────────────────────────────────── + +const createWorktreeParams = Type.Object({ + path: Type.String({ description: "Path for the new worktree (relative or absolute)" }), + 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.", + }), + ), +}); + +const listWorktreeParams = Type.Object({}); + +const switchWorktreeParams = Type.Object({ + target: Type.String({ description: "Path or branch name of the target worktree to switch to." }), +}); + +const removeWorktreeParams = Type.Object({ + path: Type.String({ description: "Path of the worktree to remove." }), +}); + +// ─── Helper Functions (stubs) ────────────────────────────────────── + +/** + * Parse raw `git worktree list` output into structured entries. + * @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[] { + throw new Error("not implemented"); +} + +/** + * Check if the target path exists. + * @param path - 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 { + throw new Error("not implemented"); +} + +/** + * Resolve a worktree target (path or branch name) to its full path. + * @param target - Path or branch name to resolve + * @param worktrees - List of all worktrees + * @returns The resolved path, or undefined if not found + */ +function resolveWorktreeTarget(target: string, worktrees: WorktreeEntry[]): string | undefined { + throw new Error("not implemented"); +} + +// ─── Tool Definitions ────────────────────────────────────────────── + +/** Create a linked worktree for a branch in the current repo. */ +const gitWorktreeCreateTool = 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. */ +const gitWorktreeListTool = defineTool({ + name: "git_worktree_list", + label: "Git Worktree List", + description: "List all linked worktrees in the current repo.", + parameters: listWorktreeParams, + async execute(_toolCallId, _params, _signal, _onUpdate, _ctx) { + return { + content: [{ type: "text", text: "Error: git_worktree_list is not yet implemented. Suggestion: Use `git worktree list` manually until this tool is complete." }], + isError: true, + }; + }, +}); + +/** Switch pi's working directory to a linked worktree. */ +const gitWorktreeSwitchTool = defineTool({ + name: "git_worktree_switch", + label: "Git Worktree Switch", + description: "Switch pi's working directory to a linked worktree.", + parameters: switchWorktreeParams, + async execute(_toolCallId, _params, _signal, _onUpdate, _ctx) { + return { + content: [{ type: "text", text: "Error: git_worktree_switch is not yet implemented. Suggestion: Use the `/worktree` command or `git worktree` manually until this tool is complete." }], + isError: true, + }; + }, +}); + +/** Remove a linked worktree. */ +const gitWorktreeRemoveTool = defineTool({ + name: "git_worktree_remove", + label: "Git Worktree Remove", + description: "Remove a linked worktree.", + parameters: removeWorktreeParams, + async execute(_toolCallId, _params, _signal, _onUpdate, _ctx) { + 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, + }; + }, +}); + +// ─── Extension Entry Point ──────────────────────────────────────── + +export default function (pi: ExtensionAPI) { + // Register tools + pi.registerTool(gitWorktreeCreateTool); + pi.registerTool(gitWorktreeListTool); + pi.registerTool(gitWorktreeSwitchTool); + pi.registerTool(gitWorktreeRemoveTool); + + // Register /worktree command + pi.registerCommand("worktree", { + description: "Interactive picker to switch between linked worktrees.", + handler: async (_args, _ctx) => { + return { + content: [{ type: "text", text: "Error: /worktree command is not yet implemented. Suggestion: Use `git worktree list` and navigate manually until this command is complete." }], + isError: true, + }; + }, + }); +}