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

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

.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<boolean> {
+	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,
+			};
+		},
+	});
+}

findings.md

@@ -0,0 +1,40 @@
+# Findings — Issue #1: Create Extension File Structure
+
+## Git Workflow
+- Branch: `issue-1` (based on origin/main)
+- Remote: `https://git.excelera.net/david/pi-worktrees.git`
+- Tool: `fj` CLI for Forgejo issues (auto-detected from git remote)
+- PR: Will use `fj pr create` when complete
+
+## Project Context
+- **Extension location**: `.pi/extensions/pi-worktree.ts`
+- **Pattern**: Single-file extension with default export function
+- **API imports**: `@earendil-works/pi-coding-agent` (ExtensionAPI, ExtensionContext)
+- **Schema library**: TypeBox (`typebox`) for tool parameter schemas
+- **No npm dependencies** beyond what pi provides
+
+## Validation Approach
+This project has no traditional test suite. Validation is:
+1. File exists at `.pi/extensions/pi-worktree.ts`
+2. Correct imports from `@earendil-works/pi-coding-agent` and `typebox`
+3. Follows single-file extension pattern (default export function)
+4. pi loads without errors via `/reload` or restart
+
+## Issue #1 Acceptance Criteria
+- [ ] `.pi/extensions/pi-worktree.ts` exists
+- [ ] Correct imports from `@earendil-works/pi-coding-agent` and `typebox`
+- [ ] Extension loads without errors when pi starts
+- [ ] Follows the single-file extension pattern with default export function
+
+## Related Files (from main branch)
+| File | Purpose |
+|------|---------|
+| `README.md` | User-facing docs — describes 4 tools + `/worktree` command |
+| `DESIGN.md` | Architecture decisions for the extension |
+| `DOMAIN.md` | Git worktree domain concepts and terminology |
+| `AGENTS.md` | Agent conventions, repo details, fj CLI usage |
+
+## Session Discoveries
+- **TypeBox import**: Use `{ Type } from "@earendil-works/pi-ai"` (re-exported) rather than direct `typebox` import — matches pi's extension examples.
+- **Tool definition helper**: Use `defineTool()` from `@earendil-works/pi-coding-agent` for type-safe tool registration.
+- **Validation command**: `bun build <file> --no-bundle --emit=metadata` compiles and validates TypeScript without bundling.

issue-1-review.md

@@ -0,0 +1,229 @@
+# 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.

progress.md

@@ -0,0 +1,26 @@
+# Progress Log — Issue #1: Create Extension File Structure
+
+## Session 1 — [2025-07-24]
+
+### Task 001: Scaffold `.pi/extensions/pi-worktree.ts`
+
+| Phase | Status | Notes |
+|-------|--------|-------|
+| Phase 0 (Context) | ✅ Done | Branch issue-1 from origin/main, docs found, TypeScript + TypeBox confirmed |
+| Step 1 (Read Task) | ✅ Done | Issue #1: Create Extension File Structure. Acceptance: file exists, correct imports, loads without errors |
+| Step 2 (Scaffold) | ✅ Done | Created full scaffold with types, tool schemas, helper stubs, and entry point |
+| Step 3 (RED) | ✅ Done | Scaffold compiles — stub functions throw "not implemented" as expected |
+| Step 4 (GREEN) | ✅ Done | `bun build` succeeds with no errors. All imports resolve from pi's global bun install.
+
+### Scaffold Contents
+- **Types**: `WorktreeEntry`, `WorktreeListResult`, `WorktreeError`
+- **Tool schemas**: `createWorktreeParams`, `listWorktreeParams`, `switchWorktreeParams`, `removeWorktreeParams`
+- **Helper stubs**: `parseWorktreeList()`, `pathExists()`, `resolveWorktreeTarget()`
+- **Tools registered**: `git_worktree_create`, `git_worktree_list`, `git_worktree_switch`, `git_worktree_remove`
+- **Command registered**: `/worktree` (stub)
+
+---
+
+### Test Results
+
+_N/A — validation is pi loading + TypeScript syntax. `bun build` passes with zero errors._

task_plan.md

@@ -0,0 +1,20 @@
+# Task Plan — Issue #1: Create Extension File Structure
+
+| Task | Description | TDD Status | Done |
+|------|-------------|------------|------|
+| 001.1 | Create extension file with correct imports and entry point | ✅ GREEN ✅ REFACTOR | ✅ |
+| 001.2 | Register tool scaffolding (4 tools + command) with structured error returns | ✅ GREEN ✅ REFACTOR | ✅ |
+| 001.3 | Implement `git_worktree_list` | ☐ RED ☐ GREEN ☐ REFACTOR | ☐ |
+
+## Milestone Context (from implementation plan)
+
+- **Milestone 1: Foundation** — Set up extension structure and register basic tools
+- **Task 1.1**: Create Extension File — empty file with correct imports, loads without errors ✅
+- **Task 1.2**: Register Tool Scaffolding — four tool registrations with placeholder execute functions (structured error returns) ✅
+- **Task 1.3**: Implement `git_worktree_list` ☐
+
+## Notes
+
+- No traditional test framework — validation is: file exists at `.pi/extensions/pi-worktree.ts`, correct imports, pi loads without errors via `/reload` or restart.
+- Single-file extension pattern: default export function receiving `ExtensionAPI`.
+- Uses fj CLI for issue operations (auto-detected from git remote).