david/pi-worktrees
1
0
Fork 0
Code Issues 8 Pull requests 1 Projects Releases Packages Wiki Activity Actions

Fix code review issues: structured error returns, remove unused count, add JSDoc #14

Merged
david merged 3 commits from issue-1 into main 2026-07-24 08:43:35 +00:00
Conversation 0 Commits 3 Files changed 2 +184
david commented 2026-07-24 08:35:47 +00:00
Owner
Copy link

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.

# 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: <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.: ```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 <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: ```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: <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`: ```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.
david added 2 commits 2026-07-24 08:35:48 +00:00 
- Add .pi/extensions/pi-worktree.ts with correct imports and default export
- Define WorktreeEntry, WorktreeListResult, WorktreeError types
- Register 4 tools: create, list, switch, remove (stubs)
- Register /worktree command (stub)
- Add helper function stubs: parseWorktreeList, pathExists, resolveWorktreeTarget
- Uses defineTool() and Type from pi SDK for type-safe registration
- Replace throw Error() with { content, isError: true } in all execute functions and command handler
- Remove unused count field from WorktreeListResult interface
- Add JSDoc comments above all defineTool() calls
- Clarify task_plan.md subtask structure (001.1-001.3)
david added 1 commit 2026-07-24 08:39:39 +00:00
david merged commit 6c696f282d into main 2026-07-24 08:43:35 +00:00
david deleted branch issue-1 2026-07-24 08:43:35 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: david/pi-worktrees#14
No description provided.
Delete branch "issue-1"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?