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 +5 -315
5 changed files with 5 additions and 315 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit ce0a350181 - Show all commits

5
.gitignore vendored Normal file
View file

@ -0,0 +1,5 @@
# Planning files (local working documents)
findings.md
progress.md
task_plan.md
issue-*-review.md

40
findings.md
View file

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

229
issue-1-review.md
View file

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

26
progress.md
View file

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

20
task_plan.md
View file

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