From e12e5d16c671e639509a8b5dc6f9f8600585b500 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 21:25:15 +0200 Subject: [PATCH] docs(relay): record worktree-autodetect leg 1 --- .pi-web/relays/worktree-autodetect/log.md | 78 ++++++++++++++++++++ .pi-web/relays/worktree-autodetect/status.md | 48 ++++++++---- 2 files changed, 110 insertions(+), 16 deletions(-) diff --git a/.pi-web/relays/worktree-autodetect/log.md b/.pi-web/relays/worktree-autodetect/log.md index 05baee3..2f5131f 100644 --- a/.pi-web/relays/worktree-autodetect/log.md +++ b/.pi-web/relays/worktree-autodetect/log.md @@ -195,3 +195,81 @@ git status, open diff, terminal selection). **Yes.** Packet committed, `status.md` un-parked with the decisions recorded, leg 1 dispatched via `spawn_session`. + +--- + +## Leg 1 — Stop reporting removed worktrees (server truth) + +**Commit:** `266f941` — `fix(workspaces): hide worktrees whose checkout directory is gone` + +### What I did + +1. **Parser reports facts.** `src/server/workspaces/gitWorktreeDiscovery.ts` now reads the + `prunable` and `locked` keys. Extracted the pure parsing step into an exported + `parseGitWorktreeList(stdout)`; `discoverGitWorktrees` is now just the `execFile` + boundary plus that call. `GitWorktreeInfo` gained optional `prunable` / `locked`. +2. **Service decides policy.** `src/server/workspaces/workspaceService.ts` filters prunable + linked worktrees in a small private `selectable()` step, keeping the entry whose path + equals `project.path` unconditionally. Existing "no worktrees" fallback then also covers + the case where every listed worktree was filtered away, so a project can never present + an empty workspace list. +3. **Injected the git boundary.** `WorkspaceService` now takes an optional + `WorkspaceGitPort` (`{ isGitRepository, discoverGitWorktrees }`) defaulting to the real + implementation. This is what made the policy testable without a real repo or a + subclass-override fake, and it left every existing `new WorkspaceService()` call site + (`app.ts`, `app.testSupport.ts`, `sessiond.ts`) untouched. +4. **Two new test files** (the plan predicted one; the policy assertions belong next to the + service that owns them, not next to the parser): + - `gitWorktreeDiscovery.test.ts` — parser only, fixtures captured verbatim from real git. + - `workspaceService.test.ts` — prunable hidden, locked kept, project path kept even when + marked prunable, fallback when everything is filtered, plus pre-existing labeling and + non-git behavior pinned so leg 2/3 have a regression net. + +### Decisions and why + +- **Facts in the parser, policy in the service**, as `plan.md` preferred. The words + `prunable` and `project.path` now appear in exactly one place each. +- **Constructor injection rather than subclass-override fakes.** `workspaceDeletionRoutes.test.ts` + fakes `WorkspaceService` by subclassing and overriding `list()`, which cannot test `list()` + itself. A narrow port is the smaller seam and keeps the production default unchanged. +- **Read-only, as mandated.** No `git worktree prune`, no repo metadata mutation anywhere. +- **`locked` is parsed but deliberately not acted on.** A locked worktree is a real checkout + and stays selectable. It is surfaced now because git emits it in the same records and + future UI may want it; adding it later would mean touching the parser again. +- **Re-verified the git behavior** rather than trusting it: created a throwaway repo, + `rm -rf`'d a linked worktree and locked another. Confirmed `prunable gitdir file points to + non-existent location` and `locked keep me`. Test fixtures are that exact output. Probe + repo deleted afterwards. +- **No API/type change.** `prunable`/`locked` never leave the server, so the shared + `Workspace` type and the client are untouched. Leg 2 needs no type work. + +### Checks run + +- `npm test -- --run src/server/workspaces/gitWorktreeDiscovery.test.ts src/server/workspaces/workspaceService.test.ts` → 10 passed +- `npm test -- --run src/server/workspaces/` → 12 files, 88 passed +- `npm test -- --run src/server/app.projects.test.ts` → 5 passed (workspace list route contract) +- `npm run typecheck` → clean +- `npx eslint` on all four changed files → clean +- pre-commit `verify:staged` → cached typecheck, knip, eslint, 14 related test files / 63 tests, all green + +`npm run verify` was not run; per `plan.md` that is leg 3's gate. + +### Artifacts changed + +- `src/server/workspaces/gitWorktreeDiscovery.ts` (modified) +- `src/server/workspaces/workspaceService.ts` (modified) +- `src/server/workspaces/gitWorktreeDiscovery.test.ts` (new) +- `src/server/workspaces/workspaceService.test.ts` (new) +- `status.md` (leg tracking → last completed 1 / next 2, next task set to leg 2, added a + note about the new `WorkspaceGitPort` seam) + +No changeset yet — charter allows it any time up to leg 3, and leg 3 owns one fragment for +the whole user-visible behavior. + +### Blockers + +None. Nothing ambiguous, no design decision needed, no intervention trigger fired. + +### Handing off? + +**Yes.** Work committed, packet updated, leg 2 dispatched via `spawn_session`. diff --git a/.pi-web/relays/worktree-autodetect/status.md b/.pi-web/relays/worktree-autodetect/status.md index abea448..b7f2f3f 100644 --- a/.pi-web/relays/worktree-autodetect/status.md +++ b/.pi-web/relays/worktree-autodetect/status.md @@ -3,32 +3,39 @@ ## ✅ APPROVED — relay is live The human approved the reduced scope in leg 0 and answered every open question. There are -no outstanding decisions. Run leg 1. +no outstanding decisions. Run leg 2. ## Current position -Leg 0 (assessment/design) is complete. No production code has been written. +Leg 1 is complete and committed (`266f941`). The **server** side of the feature is done: +prunable worktrees are no longer reported as workspaces. Nothing in the client has changed +yet, so newly created worktrees still only appear on a full page load. -The recommendation is **reduced scope**: detection piggybacked on browser resume, plus a -fix for the inverse (removed-worktree) case. No watchers, no timers, no new processes, -no new push channel. Rationale is in `log.md` leg 0; the implementation breakdown is in -`plan.md`. +The design remains reduced scope: detection piggybacked on browser resume. No watchers, +no timers, no new processes, no new push channel. Breakdown is in `plan.md`. ## Leg tracking -- **Last completed leg:** 0 (assessment and packet creation) -- **Next leg to run:** 1 +- **Last completed leg:** 1 (server: hide removed worktrees) +- **Next leg to run:** 2 -## Next task — leg 1 +## Next task — leg 2 -Stop reporting worktrees whose checkout directory has been removed outside PI WEB. +Non-disruptive workspace topology refresh in the client. -See `plan.md` → "Leg 1". Summary: teach the `git worktree list --porcelain` parser about -the `prunable` and `locked` keys, exclude prunable linked worktrees from the workspace -list, never filter the main worktree, never mutate the repo (no `git worktree prune`). +See `plan.md` → "Leg 2" and **read it fully before writing the method** — it spells out the +plausible-looking wrong implementation and exactly why it is destructive. Summary: add +`refreshSelectedProjectTopology()` to `WorkspaceController` that re-lists the selected +project's workspaces and applies them through `applyProjectWorkspaces` **only** — never +through `selectWorkspace`, which has no already-selected guard and would clear the active +session and all workspace-scoped state on every alt-tab. Guard against machine/project +changing mid-flight; `console.warn` on failure, never `setState({ error })`. -Files: `src/server/workspaces/gitWorktreeDiscovery.ts`, `src/server/workspaces/workspaceService.ts`, -new `src/server/workspaces/gitWorktreeDiscovery.test.ts`. +Files: `src/client/src/controllers/workspaceController.ts`, +new `src/client/src/controllers/workspaceController.test.ts`. + +Do **not** wire anything into `PiWebApp` in leg 2 — that is leg 3, deliberately after the +refresh is proven inert. ## Relevant context for the next runner @@ -58,6 +65,13 @@ real git; do not re-derive them: - **Remote machines come for free.** `workspacesApi.workspaces(projectId, machineId)` routes through `machinePrefix`, and `GET /projects/:projectId/workspaces` is already in `FEDERATED_HTTP_ROUTES` in `src/shared/federatedRoutes.ts`. No transport work needed. +- **Leg 1 shipped a seam leg 2 does not need but should know about.** `WorkspaceService` + now takes an optional `WorkspaceGitPort` (`{ isGitRepository, discoverGitWorktrees }`) + in its constructor, defaulting to the real git implementation, so workspace policy is + testable without a repo. `parseGitWorktreeList(stdout)` is exported from + `gitWorktreeDiscovery.ts` for pure parser tests. Server-side `Workspace` shape is + unchanged — `prunable`/`locked` live on `GitWorktreeInfo` only and never reach the API, + so the client needs no type changes. - **The danger is `selectWorkspace`.** It calls `clearActiveSession()` and `resetWorkspaceScopedState()`. A refresh must apply the new list via `applyProjectWorkspaces` **only**, and must not route through `selectWorkspace` when the @@ -87,4 +101,6 @@ real git; do not re-derive them: ## Blockers -None. Leg 1 is clear to run. +None. Leg 2 is clear to run. The one thing to watch is leg 2's own intervention trigger: +if the refresh cannot be made non-disruptive without visible UI churn, stop rather than +working around it.