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

Compare commits

base: david:3c3f33037d3796e5307f05bb425a9a83ddf6eb80
Branches Tags
david:main
david:issue-6
david:issue-4-review-fixes
...
compare: david:637003d70c6f1e5dfb3750df407d04be04c6c95c
Branches Tags
david:issue-6
david:main
david:issue-4-review-fixes

3 commits
3c3f33037d ... 637003d70c

Author SHA1 Message Date
David Kong
637003d70c docs: add repo details and fj CLI usage to AGENTS.md 2026-07-24 22:20:44 +10:00
David Kong
2e4ef7035a docs: address code review findings for issue-1 
- Split Validation Checklist into verifiable docs vs deferred implementation checks (AGENTS.md)
- Correct README example output to match actual `git worktree list` format
- Add pi platform version requirement (`pi >= 0.5.0`) to Requirements section
- Add Testing Strategy section with unit, integration, and manual verification procedures (DESIGN.md)
- Document annotated `git worktree list` raw output format with parsing rules (DOMAIN.md)
- Add permission fail condition for `create` tool in Tool Behaviors table
 2026-07-24 22:20:22 +10:00
David Kong
6c696f282d Fix code review issues: structured error returns, remove unused count, add JSDoc (#14) 
# 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.

Co-authored-by: David Kong <davkon@gmail.com>
Reviewed-on: #14
 2026-07-24 08:43:34 +00:00
6 changed files with 271 additions and 7 deletions
Show stats Expand all files Collapse all files

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

179
.pi/extensions/pi-worktree.ts Normal file
View file

@ -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,
};
},
});
}

30
AGENTS.md
View file

@ -1,5 +1,35 @@
# AGENTS.md — pi-worktree Extension # AGENTS.md — pi-worktree Extension
## Repo Details
| Property | Value |
|----------|-------|
| **Host** | `git.excelera.net` |
| **Remote** | `https://git.excelera.net/david/pi-worktrees.git` |
| **Owner** | david |
| **Repo** | pi-worktrees |
## Using `fj` CLI to Pull Issues
This repo uses the [`fj`](https://git.excelera.net/david/fj) CLI tool (Forgejo CLI) for issue operations. It's auto-detected from the git remote, so `-H` is optional inside this repo:
```bash
# Search open issues in this repo
fj issue search --repo pi-worktrees
# View a specific issue
fj issue view <ISSUE_NUM>
# List all open issues
fj issue search --state open
```
### Prerequisites
- `fj` must be installed and authenticated (`fj whoami`)
- Credentials stored at `~/.local/share/forgejo-cli/keys.json`
- Auth via OAuth login or application token on `git.excelera.net`
## Project Structure ## Project Structure
``` ```

35
DESIGN.md
View file

@ -41,7 +41,7 @@ Update branch display + offer new session creation. No automatic worktree creati
| Tool | Key Behavior | Fail Conditions | | Tool | Key Behavior | Fail Conditions |
|------|-------------|-----------------| |------|-------------|-----------------|
| `create` | `git worktree add <path> [branch]`, `-b` for new branches | Path exists, branch is current | | `create` | `git worktree add <path> [branch]`, `-b` for new branches | Path exists, branch is current, no write permission at target path (check with `fs.access()` before invoking git) |
| `list` | Parses `git worktree list` output into structured data | No git repo present | | `list` | Parses `git worktree list` output into structured data | No git repo present |
| `switch` | Changes cwd to target worktree path | Already there, uncommitted changes in source | | `switch` | Changes cwd to target worktree path | Already there, uncommitted changes in source |
| `remove` | `git worktree remove <path>` | Target is main repo, uncommitted changes | | `remove` | `git worktree remove <path>` | Target is main repo, uncommitted changes |
@ -53,9 +53,36 @@ Update branch display + offer new session creation. No automatic worktree creati
3. **Untracked files on removal**: Suggest removing/moving them first 3. **Untracked files on removal**: Suggest removing/moving them first
4. **Same-branch worktrees**: Let git handle it, surface the error 4. **Same-branch worktrees**: Let git handle it, surface the error
## Testing Strategy
### Unit Tests — Parsing (`git_worktree_list`)
- Parse valid `git worktree list` output into structured data with correct `branch`, `path`, and `isCurrent`
- Handle detached HEAD entries (no branch name → use path for identification)
- Handle bare repos (HEAD line without branch brackets)
- Reject non-git-repo directories
### Unit Tests — Validation (`git_worktree_create`)
- Refuse when target path already exists (suggest `--force` or alternative path)
- Refuse when attempting to create a worktree on the current branch in the main repo
- Surface permission errors with actionable guidance (e.g., "cannot write to /root/ — try ~/worktrees/")
### Integration Tests — Each Tool
| Test | Procedure |
|------|-----------|
| `create` | Create worktree → verify `git worktree list` shows it → remove it |
| `list` | Run against repo with 0, 1, and N worktrees → assert structured output shape |
| `switch` | Switch to existing worktree → verify cwd updated → switch back → verify original state restored |
| `remove` | Remove a linked worktree → verify it's gone from list; refuse if uncommitted changes exist |
### Manual Verification — `/worktree` Command UX
- Run `/worktree` in a repo with multiple worktrees
- Confirm picker shows branch names (not just paths)
- Switch to a worktree → confirm branch display updates
- Return to main repo → confirm session back-switching works
## Rollout Phases ## Rollout Phases
1. Core tools (create, list, switch, remove) with error handling 1. Core tools (create, list, switch, remove) with error handling + unit tests
2. `/worktree` interactive command with picker + back-switching 2. `/worktree` interactive command with picker + back-switching + integration tests
3. Session integration hooks (branch display, new session offer) 3. Session integration hooks (branch display, new session offer)
4. Polish: edge cases, README, user testing 4. Polish: edge cases, README, manual verification pass

22
DOMAIN.md
View file

@ -21,6 +21,28 @@ A git worktree is a linked working directory that shares the same repository but
| **Bare repo** | A `.git/` without a working tree — used as the central store when all worktrees are linked | | **Bare repo** | A `.git/` without a working tree — used as the central store when all worktrees are linked |
| **Detached HEAD** | When a worktree checks out a commit directly instead of a branch | | **Detached HEAD** | When a worktree checks out a commit directly instead of a branch |
### Raw Output Format — `git worktree list`
The parser should expect this exact pipe-separated format from `git worktree list`:
```
/home/user/project abc1234 [main] # main repo (current)
/tmp/feature-x def5678 [feature-x] # linked worktree on a branch
/tmp/detached fedcba HEAD # detached HEAD (no branch name)
```
| Field | Position | Description |
|-------|----------|-------------|
| **Path** | Column 1 | Absolute path to the working tree directory |
| **Hash** | Column 2 | Short commit hash (HEAD) for this worktree |
| **Branch** | Column 3+ | Branch name in `[brackets]`, or `HEAD` if detached |
**Parsing rules:**
- Lines with `[branch-name]` → the branch is identified by what's inside the brackets.
- Lines with `HEAD` (no brackets) → detached HEAD; identify this worktree by path only.
- The first entry in the output is always the main repo — mark it as `isCurrent: true`.
- Worktrees listed after the first entry are linked worktrees (`isCurrent: false`).
### Constraints (from git itself) ### Constraints (from git itself)
- You cannot create a nested worktree (worktree inside a worktree). - You cannot create a nested worktree (worktree inside a worktree).

7
README.md
View file

@ -28,10 +28,10 @@ List all linked worktrees in the current repo.
Returns structured data with `branch`, `path`, and `isCurrent` for each worktree. Returns structured data with `branch`, `path`, and `isCurrent` for each worktree.
**Example output**: **Example output** (raw `git worktree list` format, parsed into structured data):
``` ```
main — /home/user/repo (current) /home/user/repo abc1234 [main]
feature-x — /home/user/repo/feature-x /home/user/repo/feature-x def5678 [feature-x]
``` ```
### `git_worktree_switch` ### `git_worktree_switch`
@ -77,5 +77,6 @@ Lists all worktrees with a branch-first display and lets you select one to switc
## Requirements ## Requirements
- **pi >= 0.5.0** (with `ExtensionAPI` and `registerCommand` support)
- Git 2.x with worktree support (all modern versions) - Git 2.x with worktree support (all modern versions)
- No external npm dependencies — uses only pi's extension API and TypeBox schemas. - No external npm dependencies — uses only pi's extension API and TypeBox schemas.