pi-worktrees/issue-1-review.md

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:

return {
  content: [{ type: "text", text: `Error: ${message}. Suggestion: <fix>` }],
  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.:

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:

/**
 * Create a linked worktree for a branch.
 * Runs `git worktree add <path> [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:

// 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:

async execute(_toolCallId, _params, _signal, _onUpdate, _ctx) {
  return {
    content: [{ type: "text", text: `Error: <tool_name> 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:

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():

/** 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.