diff --git a/.pi-web/relays/worktree-autodetect/charter.md b/.pi-web/relays/worktree-autodetect/charter.md deleted file mode 100644 index a722b09..0000000 --- a/.pi-web/relays/worktree-autodetect/charter.md +++ /dev/null @@ -1,131 +0,0 @@ -# Charter — relay "worktree-autodetect" - -## Relay identity - -- **Name:** `worktree-autodetect` -- **Root:** `.pi-web/relays/worktree-autodetect/` in worktree `/srv/dev/pi-web-worktrees/worktree-autodetect` -- **Branch:** `feat/worktree-autodetect` (based on `main`) - -## Goal / finish line - -Worktrees created or removed outside PI WEB become visible in the browser workspace -list **with no user action of any kind**, on the next natural browser resume, on both -local and remote machines. - -Concretely, the relay is finished when all of the following are true: - -1. `discoverGitWorktrees` no longer reports worktrees whose checkout directory is gone - (`prunable` in `git worktree list --porcelain`), so worktrees deleted outside PI WEB - stop appearing as selectable ghost workspaces. -2. `WorkspaceController` can re-list the workspaces of the selected project and apply the - result **without disturbing the current selection, session, or scroll state** when the - selected workspace still exists, and without silently yanking the user out of a - workspace that vanished while they were working in it. -3. `PiWebApp` calls that refresh from the existing browser-resume path - (`refreshAfterBrowserResume`) and the existing plugin-facing `refreshAppData` path. - No new timer, no new watcher, no new process, no new WebSocket channel. -4. Tests cover: prunable parsing/filtering, refresh-preserves-selection, - refresh-when-selected-workspace-disappeared, and the resume wiring. -5. `npm run verify` is green, and a changeset exists describing the user-visible behavior. - -**Explicitly out of scope** (decided in leg 0, do not re-open without the human): - -- Filesystem watchers on `.git/worktrees` or anywhere else. -- Polling timers for worktree discovery. -- Any server→browser push channel for workspace topology. -- Instant (sub-second) detection while the browser tab already has focus. -- Auto-*adopting* anything into `projects.json`. Worktrees are derived, never registered; - nothing is being adopted, and no project registry write is part of this work. - -## Sizing - -**One leg = one vertical slice that leaves the tree green and committed.** - -A leg is done when its slice is implemented, its tests are written and passing, the -narrowest meaningful checks are run (`npm test -- --run `, plus `npm run typecheck` -if exported types changed), and the work is committed. Do not carry uncommitted work -across a handoff. - -Expected shape is three legs (see `plan.md`). If a leg turns out bigger than one slice, -split it and hand off the remainder rather than doing "just a bit more". - -## Task selection policy - -1. Take the explicit **next leg** named in `status.md`. -2. If `status.md` does not name one, take the next unfinished slice in `plan.md` in order. -3. If neither is clear, or the next slice would change the design rather than implement it, - **stop and raise the intervention signal**. Do not redesign inside a leg. - -## Handover protocol - -Before handing off, in this order: - -1. Make the work durable: source + tests written, checks run, changes **committed** with a - Conventional Commit message. -2. Update `status.md`: current position, last completed leg, next leg to run, next task, - relevant context for the next runner, blockers. -3. Append a concise entry to `log.md`: what you did, decisions and why, artifacts changed, - exact checks run and their results, handing-off vs stopping. -4. Then `spawn_session` **once**, with a prompt starting: - -```text -Relay "worktree-autodetect" leg begins now. - -You are the next runner in this Relay method chain. - -Read: -- .pi-web/relays/worktree-autodetect/charter.md -- .pi-web/relays/worktree-autodetect/status.md - -Do not read log.md end-to-end. Use it only for targeted lookup if status.md or charter.md points you there. - -Run one leg according to the charter. Before handing off, update status.md, append log.md, make work durable, then either spawn the next leg once or stop with a clear intervention note. -``` - -## Intervention signal - -**Stop, do not spawn**, and write a clearly marked `## BLOCKED` section at the top of -`status.md` plus a log entry, if any of these happen: - -- The next task is ambiguous, or doing it would require a design decision not in this charter. -- You are tempted to add a watcher, a timer, a new process, or a new push channel. That - means the design boundary is being crossed — get the human. -- Refresh-on-resume cannot be made to preserve selection without visible UI churn - (list reordering, chat scroll jump, session reload, terminal teardown). This is the - main known risk; it is a stop, not a workaround. -- Filtering `prunable` would remove a workspace the user could plausibly still want - (for example a temporarily unmounted network path) and you cannot bound that safely. -- `npm run verify` fails for a reason you did not introduce. - -## Reading discipline - -Read to orient: `charter.md`, then `status.md`, then only the files `status.md` names. - -Do **not** read `log.md` end-to-end; use it only for targeted lookup when pointed there. -Do **not** read the sibling worktrees `/srv/dev/pi-web-worktrees/worktree-create-ui` or -`/srv/dev/pi-web-worktrees/model-questions-ux` — they are separate, parallel efforts. Per -the human's decision, assume they contribute nothing to this relay; this relay owns the -workspace-topology refresh seam outright. - -Relevant source surface, small enough to read directly when your leg touches it: - -- `src/server/workspaces/gitWorktreeDiscovery.ts` (39 lines) -- `src/server/workspaces/workspaceService.ts` (44 lines) -- `src/client/src/controllers/workspaceController.ts` (~105 lines) -- `src/client/src/appShell/browserResumeController.ts` + its test -- `src/client/src/components/PiWebApp.ts` — only `refreshAfterBrowserResume` - (~line 432) and `refreshAppData` (~line 485). Do not read this 2300-line file whole. - -## Project conventions that apply - -- **Changesets:** this is user-visible. Add a `.changeset/*.md` fragment - (see `.agents/skills/changeset-changelog/SKILL.md`). Never hand-edit `CHANGELOG.md`. -- **Skills:** use `.agents/skills/code-quality-architecture/SKILL.md` when writing - production code and `.agents/skills/testing-guide/SKILL.md` when writing tests. -- **Session daemon:** this design deliberately touches **no** sessiond code, no session - runtime ownership, and no daemon protocol. **No manual session daemon restart is - required.** Changes land on the autoreloading `pi-web-web-ui-dev.service` path only. - If a leg finds itself editing `src/server/sessiond.ts`, that is the intervention signal. -- **Client URL conventions:** no new endpoints are added; the existing - `workspacesApi.workspaces()` request path is reused unchanged. -- **No `npm install`** — `node_modules` here is a symlink to the main checkout. diff --git a/.pi-web/relays/worktree-autodetect/log.md b/.pi-web/relays/worktree-autodetect/log.md deleted file mode 100644 index 86b0639..0000000 --- a/.pi-web/relays/worktree-autodetect/log.md +++ /dev/null @@ -1,580 +0,0 @@ -# Log — relay "worktree-autodetect" - -Append-only. One entry per leg. Do not read end-to-end unless `status.md` points you here. - ---- - -## Leg 0 — Assessment, design, packet creation - -**Runner:** assessment/design session -**Outcome:** recommendation = **reduced scope**. Packet created. Relay parked pending -human approval. No production code written. - -### Feature request - -> "auto detect and show new worktrees, even when created outside of pi-web." - -With the user's framing: nice-to-have, not a must, expects it may not be feasible, and -**must require zero user intervention**. - -### What I found in the codebase - -The single most important finding reframed the whole feature: - -**Worktree discovery is already fully dynamic. There is no cache and no registry.** - -`WorkspaceService.list()` (`src/server/workspaces/workspaceService.ts`, 44 lines) calls -`isGitRepository()` then `discoverGitWorktrees()` — which shells out to -`git worktree list --porcelain` — on **every single** `GET /projects/:projectId/workspaces` -request. I grepped for any cache/memo in that path: there is none. Workspace ids are -derived by hashing `${project.id}:${worktree.path}`, so they are stable across calls -without being stored anywhere. - -And `projects.json` (`src/server/storage/projectStore.ts`) stores only -`{ id, name, path, createdAt }` per **project**. Workspaces are never persisted. - -Two consequences, both of which delete a large chunk of the anticipated problem: - -1. **A worktree created outside PI WEB is already detected.** The server has no stale - state to invalidate. The gap is not detection at all — it is that **the browser never - re-asks**. `WorkspaceController` fetches workspaces in `selectProject()` and in - `refreshProjectWorkspaces()`, and the only caller of the latter is the - workspace-*deletion* flow. So the list is fetched on project selection and then frozen - for the lifetime of that selection. -2. **"Auto-adoption" is a non-question.** The brief asked whether zero-intervention - detection implies zero-intervention adoption, and whether a discovered worktree should - be a distinct "discovered" state. Since worktrees are derived and nothing is written to - a registry, there is nothing to adopt and no state to distinguish. A new worktree is - simply a member of the derived list. This was the design's biggest apparent risk and it - evaporated on inspection. - -Measured cost of the discovery itself: **~2ms**. 20 sequential -`git worktree list --porcelain` runs on this repo took 42ms total. - -I also found the delivery mechanism already built and already debounced: -`src/client/src/appShell/browserResumeController.ts` listens to window `focus` and -document `visibilitychange`, batches signals per animation frame, and collapses -concurrent refreshes through `TrailingRefreshCoordinator`. It drives -`PiWebApp.refreshAfterBrowserResume()`, which today refreshes the selected session, -machine activities, and workspace-deletion runs. Workspace topology is conspicuously -absent from that list. - -And remote machines need no work: `GET /projects/:projectId/workspaces` is already in -`FEDERATED_HTTP_ROUTES` (`src/shared/federatedRoutes.ts:25`) and `workspacesApi.workspaces` -already takes a `machineId` and routes via `machinePrefix`. - -### The inverse case, verified against real git - -I built a throwaway repo in `/tmp/wtprobe` and checked what git actually reports. - -- `.git/worktrees/` does not exist until the first linked worktree is added, then gains - one directory per worktree. -- After `rm -rf`ing a worktree's directory **without** `git worktree remove`, - `git worktree list --porcelain` **still lists it**, with an extra line: - `prunable gitdir file points to non-existent location`. -- A locked worktree gets a bare valueless `locked` line. - -The current parser ignores both keys. So **PI WEB today shows worktrees that no longer -exist as normal, selectable workspaces** — a real bug, present regardless of whether the -detection feature is built. Selecting one produces a workspace whose path does not exist. - -### Options compared - -**A. `git worktree list` on a timer (server or client poll).** -Rejected. It is the obvious answer and it is the wrong one. A timer runs forever to catch -an event that happens a few times a week, and it must run per project, per machine, or it -does not actually satisfy "no intervention". For a nice-to-have, a permanent background -cost to serve a rare event is exactly the trade the user warned against. It also has no -natural interval: fast enough to feel automatic is wasteful, slow enough to be cheap is -not noticeably better than the resume trigger, which is free. - -**B. Watch `.git/worktrees/` with `fs.watch`/inotify.** -This was the most interesting candidate and the one I most wanted to work. The watch -target is genuinely small and precise — one directory in the main repo, one entry per -worktree, written by git itself. That is far better than watching filesystems for new -directories. - -Rejected anyway, on cost and correctness: - -- **Lifecycle ownership is the real problem, not the watcher.** A watcher must be created - and destroyed as projects are added/removed, and it must live somewhere long-lived. The - web/API process autoreloads (`pi-web-web-ui-dev.service`), so watchers there churn - constantly. The natural long-lived home is the session daemon — but that would drag a - purely presentational concern into session runtime ownership and, per `AGENTS.md`, make - every change to it require a manual daemon restart. For a nice-to-have, that is a - disproportionate architectural commitment. -- **The directory does not exist until the first worktree exists**, so a repo with no - linked worktrees needs a watch on `.git/` itself to catch `worktrees/` being created — - a noisier target that fires on every ref update, index write, and fetch. -- **It only fixes the local case.** Remote machines would need the event pushed over the - machine transport, which means a new workspace-topology realtime event type, publishing - it from the daemon, adding it to `FEDERATED_WEBSOCKET_ROUTES` plumbing, parsing it in - `sessionSocket.ts`, and handling it in `PiWebApp`. That is a meaningful new protocol - surface for a feature the user called optional. -- **Environment caveats are real.** `fs.watch` is unreliable on Docker bind mounts on - macOS/Windows (the repo ships `docker/compose.yml` with bind-mounted checkouts) and on - network filesystems, and inotify watch limits are a known operational failure mode. So - the "instant" promise would be silently broken for a subset of users — worse than an - honest "updates when you come back to the tab". -- **It still would not be enough.** `fs.watch` on `.git/worktrees` catches creation but - the removal case still needs the `prunable` fix, because `rm -rf` of the *checkout* - does not touch `.git/worktrees/` at all — I verified the metadata directory - survives. So the watcher does not even subsume the cheaper fix. - -**C. Piggyback on events that already happen.** ← chosen -The refresh already exists, is already debounced, already covers remote machines, and -costs one ~2ms request per tab refocus. Marginal cost is as close to zero as this feature -can get, and the code surface is ~60 production lines. - -### Recommendation: reduced scope - -Do the cheap 90%: - -1. **Fix the inverse case** — filter `prunable` worktrees out of the workspace list. - Read-only; PI WEB must not run `git worktree prune` as a side effect of listing. -2. **Add a non-disruptive topology refresh** to `WorkspaceController` that re-lists the - selected project's workspaces without touching selection or session state. -3. **Call it from the existing resume path** — no new timer, watcher, process, or channel. - -**Scope boundary, stated plainly:** detection is **resume-scoped, not instant**. A worktree -created in another terminal while the PI WEB tab already has focus is not noticed until the -tab is refocused or becomes visible again. That is the honest limit, and it should be -documented rather than papered over. - -Within that boundary it is genuinely zero-intervention: no button, no config, no opt-in, -works on local and remote machines, works for creation and removal. - -### The risk that could kill it - -`WorkspaceController.selectWorkspace()` calls `clearActiveSession()` and -`resetWorkspaceScopedState()`. If a background refresh routes through it, the user's chat -is torn down every time they refocus the tab. The refresh must apply the list through -`applyProjectWorkspaces` only. `ProjectActivityOwnershipCoordinator` is the existing -precedent for background topology hydration that deliberately leaves selection alone, and -is the model to follow. Leg 2 proves this with tests before leg 3 wires the trigger — and -if it cannot be made non-disruptive, that is an explicit stop. - -### What would change the recommendation - -- If the user says instant-while-focused is actually required, option B comes back on the - table — but with the session daemon commitment, the new realtime event type, and the - Docker/network-filesystem caveats accepted as the price. -- If a future need arises for reliable server-pushed workspace topology for another reason, - the watcher becomes incremental rather than a feature-specific cost, and the ledger flips. - -### Artifacts created - -- `.pi-web/relays/worktree-autodetect/charter.md` -- `.pi-web/relays/worktree-autodetect/status.md` -- `.pi-web/relays/worktree-autodetect/plan.md` -- `.pi-web/relays/worktree-autodetect/log.md` (this file) - -### Checks run - -None — no code was changed in this leg. - -### Human decisions received at the end of leg 0 - -All three open questions were answered, and the reduced scope was approved: - -1. **Latency** — resume-scoped detection is acceptable. No timer, no watcher. -2. **Removed worktrees** — yes, hide worktrees whose directory is gone. -3. **Sibling overlap** — assume the other session does nothing; this relay owns the - refresh seam outright and should not design for sharing. - -The human also asked for the `selectWorkspace` risk to be explained concretely. The -worked failure mode is now recorded inline in `plan.md` → Leg 2, because the buggy version -is the one that looks correct: re-resolving the selection after a refresh via -`selectPreferredWorkspace` + `selectWorkspace` (mirroring `selectProject`) tears down the -session on **every** browser resume, since `selectWorkspace` has no already-selected guard -and always runs `clearActiveSession()` (closing the session socket mid-stream and dropping -buffered deltas) plus `resetWorkspaceScopedState()` (clearing chat, file tree, open file, -git status, open diff, terminal selection). - -### Handing off? - -**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`. - ---- - -## Leg 2 — Non-disruptive workspace topology refresh in the client - -**Commit:** `d0f8f9f` — `feat(workspaces): add non-disruptive workspace topology refresh` - -### What I did - -Added `WorkspaceController.refreshSelectedProjectTopology()`: it reads the selected project -and machine, calls `api.workspaces(project.id, machineId)`, re-reads state, discards the -response if machine or selected project changed mid-flight, and applies the list via the -existing private `applyProjectWorkspaces` — and nothing else. No selection is re-derived, no -session is cleared, no workspace-scoped state is reset, no URL update. - -Added `src/client/src/controllers/workspaceController.test.ts` (7 tests, new file). - -### Decisions and why - -- **Did not use `selectPreferredWorkspace` / `selectWorkspace`.** `plan.md` documented this - as the plausible-looking destructive shape; I confirmed it in the source before writing. - `selectWorkspace` has no already-selected guard, so it unconditionally calls - `sessions.clearActiveSession()` and `setState({ ...resetWorkspaceScopedState() })` — which - clears `sessions`, `fileTree`, `expandedDirs`, `selectedFilePath`, `gitStatus`, the three - diff fields, and `selectedTerminalId`. Since leg 3 calls this on every browser resume, that - would blank the UI on every alt-tab. Applying the list alone is sufficient. -- **Selected workspace that disappeared: selection left untouched**, per plan. No new - recovery path; `refreshAfterWorkspaceDeleted` still owns that. Covered by a test asserting - the vanished workspace stays selected and `clearActiveSession` is not called. -- **Errors: injected sink, not `state.error`.** Added optional - `onBackgroundError(message, error)` to `WorkspaceControllerDependencies`, defaulting to - `console.warn`, mirroring `SessionNotificationController`'s existing pattern. This keeps the - behavior testable without stubbing `console` and guarantees a background refresh never - paints an error banner over a working session. The method therefore never rejects. -- **Guard style copied from `selectProject`** (compare `selectedMachineId` and - `selectedProject?.id` after the await) rather than inventing a generation counter — the - method is a single request with no trailing-pass semantics, so - `ProjectActivityOwnershipCoordinator`'s heavier machinery would have been disproportionate. -- **Wrote a `why` comment on the method**, not a `what` one: it records the invariant and why - the simpler-looking `selectWorkspace` route is unsafe, so a future editor cannot "simplify" - it back into the bug. -- **Nothing wired into `PiWebApp`**, per plan — deliberately leg 3. - -### Verification that the guard is not vacuous - -Mutation-tested the tests: temporarily patched the method to the plausible wrong version -(`selectPreferredWorkspace(...)` + `await this.selectWorkspace(...)`) and re-ran the file — -**2 of 7 tests failed** (selection-preserved and vanished-workspace). Restored the file from -backup and re-confirmed 7/7 green. The tests genuinely pin the invariant. - -### Checks run - -- `npx vitest --run src/client/src/controllers/workspaceController.test.ts` → **7 passed** -- `npx vitest --run src/client/src/controllers` → **26 files, 204 tests passed** (no - regression in sibling controllers) -- `npm run typecheck` → **clean** (one iteration: `expandedDirs` is - `Record`, not booleans — fixed in the fixture) -- `npx eslint` on both changed files → **clean** -- pre-commit `npm run verify:staged` on commit → typecheck (cached), knip, eslint, and - related vitest (5 files / 21 tests) all **passed** - -`npm run verify` was not run: the charter assigns it to leg 3, and this leg's change is -contained to one controller. - -### Artifacts changed - -- `src/client/src/controllers/workspaceController.ts` (modified: new method, new optional - `onBackgroundError` dependency) -- `src/client/src/controllers/workspaceController.test.ts` (new, 7 tests) -- `status.md` (leg tracking → last completed 2 / next 3, next task set to leg 3 with the - method's exact call signature, added notes on test coverage + the mutation check + the new - `onBackgroundError` dependency, rewrote Blockers to record that the UI-churn risk did not - materialize) - -Still no changeset — leg 3 owns it, and `status.md` now flags it explicitly as missing. - -### Blockers - -None. The charter's main intervention trigger for this leg (refresh cannot be made -non-disruptive without visible UI churn) did not fire: no workaround was needed. No watcher, -timer, process, endpoint, or push channel added. No sessiond code touched, so **no manual -session daemon restart is required**. - -### Handing off? - -**Yes.** Work committed, packet updated, leg 3 dispatched via `spawn_session`. - ---- - -## Leg 3 — wire the refresh to browser resume, document, changeset, full verify (final) - -### What I did - -Wired the leg-2 refresh into the two existing refresh paths, documented the behavior, -added the changeset, and ran the full `npm run verify` the charter requires of the last leg. - -1. **Wiring (`src/client/src/components/PiWebApp.ts`, 2 lines).** Added - `this.workspaces.refreshSelectedProjectTopology()` to the `Promise.all` in - `refreshAfterBrowserResume` (~438) and in `refreshAppData` (~496). Nothing else in that - 2300-line file was read or touched, per the charter's reading discipline. -2. **Wiring test (`src/client/src/components/PiWebApp.workspaceTopology.test.ts`, new, - 3 tests).** Verifies the refresh is invoked by the exact callback - `BrowserResumeController` calls (`browserResume.callbacks.refreshAfterResume`, not a - hand-picked method reference), by `refreshAppData`, and that it still runs when a sibling - refresh in the same batch rejects. Sibling refreshes are stubbed so the assertions observe - only the topology call. -3. **Docs (`docs/faq.html`).** New FAQ entry `#worktree-list-out-of-date` plus its TOC link: - worktrees are listed on demand and never registered; the list is re-read on tab - focus/visibility rather than continuously, so refocusing updates it; selection/session/ - scroll are preserved; worktrees with a missing checkout directory are hidden and git still - tracks them until `git worktree prune`. Placed in the FAQ per - `.agents/skills/documentation-guide/SKILL.md` (troubleshooting/edge-case content); - `README.md` deliberately untouched. -4. **Changeset (`.changeset/worktree-autodetect.md`).** `patch` for `@jmfederico/pi-web` - (CalVer: patch is correct for a non-breaking user-facing capability), written as user - behavior rather than an implementation log. - -### Decisions and why - -- **Did not add the `connectRealtime` `onReconnect` call.** `plan.md` marked it optional - "only if it costs nothing". It is not free: `onReconnect` captures the machine id at - connect time, while `refreshSelectedProjectTopology` reads the selected machine at call - time, so wiring them together would either need a machine-scoped variant or would fire a - refresh for a machine the user has since left. Adding a second concept for no user-visible - gain over the resume path failed the cost test. Socket reconnect on resume is already - covered by the resume path itself. -- **Did not pass a message-prefixing `onBackgroundError` sink from `PiWebApp`.** Leg 2 made - it optional with a `console.warn` default that already includes project and machine in the - message. `status.md` explicitly recorded this as not-required work; adding it would be - scope creep for identical output. -- **Verified the wiring test is not vacuous.** Removed both call sites with `perl`, re-ran the - file: 3/3 failed. Restored the file (verified 2 occurrences back) and re-ran: 3/3 passed. -- **No charter boundary crossed.** No timer, watcher, process, endpoint, or push channel; - no change inside `refreshSelectedProjectTopology`; leg 2's tests were not weakened; the - resume path re-derives no selection from the refreshed list. - -### Checks run - -- `npx vitest --run src/client/src/components/PiWebApp.workspaceTopology.test.ts` → - **3 passed** (first run had 1 failure from a stub-shape mistake in my own test helper, - fixed by failing one named sibling refresh instead of all of them) -- mutation check (call sites removed) → **3 failed**, as intended; restored → **3 passed** -- `git diff --check` → clean -- `npx eslint` on the new test and `PiWebApp.ts` → **clean** -- **`npm run verify` → green**: typecheck, lint, knip, and 228 test files / - 1840 passed, 2 skipped -- pre-commit `npm run verify:staged` → typecheck (cached), knip, eslint, related vitest - (6 files / 20 tests) all **passed** - -### Artifacts changed - -- `src/client/src/components/PiWebApp.ts` (2 lines added) -- `src/client/src/components/PiWebApp.workspaceTopology.test.ts` (new, 3 tests) -- `docs/faq.html` (new FAQ entry + TOC link) -- `.changeset/worktree-autodetect.md` (new) -- committed as `84545fb feat(workspaces): refresh worktrees on browser resume` -- `status.md` rewritten as a finished-relay baton: finish-line conditions checked off one by - one with their commits, leg tracking set to last completed 3 / next none, shipped behavior - described in user terms, and the never-route-through-`selectWorkspace` invariant recorded - for whoever edits this code next - -### Blockers - -None. No intervention signal fired in this leg or any earlier one. No sessiond code touched, -so **no manual session daemon restart is required**; the change lands on the autoreloading -web/UI service path only. - -### Handing off? - -**No — this was the final leg.** All five charter finish-line conditions are met and -`npm run verify` is green, so per the charter this runner stops instead of spawning. The -branch `feat/worktree-autodetect` is ready for human review and merge. - ---- - -## Post-relay — selected-workspace freshness fix (review follow-up, not a leg) - -Triggered by a human review question after the relay finished: "do we have pragmatic -reasonable and stable code?" I reviewed the production diff against -`code-quality-architecture` and probed the runtime rather than trusting the leg summaries. - -### What the review checked and found sound - -- `handleWorkspaceChange` early-returns on equal workspace id → no `clearActiveSession`, - no terminal teardown on resume. -- `WorkspaceList.updated()` re-scrolls on any `workspaces` change, but via - `scrollIntoView({ block: "nearest" })`, a no-op when the row is already visible. -- Open row menu survives refresh: guarded by an id membership check, and ids are path-derived. -- Stale responses guarded on both machine and project id; background failures go to - `onBackgroundError`, never `state.error`, so a flaky resume shows no error toast. - -### The one real gap, and the fix - -`applyProjectWorkspaces` wrote a fresh `workspaces` array but left `selectedWorkspace` -pointing at the pre-refresh object. Reproduced with a scratch test: after a refresh where a -worktree's branch changed outside PI WEB, the list row showed `feature-b` while -`selectedWorkspace.branch` was still `feature-a`. User-visible in the collapsed Workspaces -header and the mobile context bar until reselect. Not a regression (both were stale before), -but a new fresh/stale inconsistency introduced by making the list refresh. - -Fixed by re-pointing `selectedWorkspace` at its refreshed entry, keyed by `id`. Safety rests -on two things: `id` is derived from the path, so this can never change *which* workspace is -selected; and `handleWorkspaceChange` gates on `id`, so no session/terminal teardown fires. -The patch is skipped when metadata is unchanged, because `patchChangesState` is -identity-based and a real HTTP response returns fresh-but-equal objects every resume — -without that guard every browser focus would push a new object into state. - -### Decisions - -- **Left `locked` parsed-but-unconsumed.** Flagged it as YAGNI in review, then kept it: it - documents the deliberate policy that locked worktrees are *kept*, and that policy is pinned - by a real `workspaceService` test. Removing the field would not remove the policy. -- **Left the two refresh entry points un-deduped.** Both are idempotent, stale-guarded, and - ~2ms; cross-path collapsing would add a concept for no user-visible gain. -- **Compared metadata field-by-field** (`sameWorkspaceMetadata`) rather than `JSON.stringify`, - which is key-order sensitive, or a deep-equal helper this file does not otherwise need. - -### Checks run - -- `npx vitest --run src/client/src/controllers/workspaceController.test.ts` → **9 passed** -- mutation A, never re-point (restores the original bug) → the re-point test failed, alone -- mutation B, always re-point (drops the unchanged guard) → the identity test failed, alone -- `npx eslint` on both changed files → clean; `npm run typecheck` → clean -- **`npm run verify` → green**: 228 files, **1842 passed**, 2 skipped (was 1840) -- pre-commit `verify:staged` → 6 files / 26 tests passed - -### Artifacts changed - -- `src/client/src/controllers/workspaceController.ts` (`refreshedSelection` + - `sameWorkspaceMetadata`; `applyProjectWorkspaces` early-returns for the non-selected project) -- `src/client/src/controllers/workspaceController.test.ts` (+2 tests, 9 total) -- committed as `79577e4 fix(workspaces): keep selected workspace metadata fresh on refresh` -- no changeset added: `.changeset/worktree-autodetect.md` already promises the list stays - correct without user action, and this fix delivers that promise rather than adding to it -- `status.md`: new "Post-relay review fix" section and the second invariant recorded - -### Blockers - -None. No new timer, watcher, process, endpoint, or push channel; no sessiond code touched, so -**no manual session daemon restart is required**. Branch still ready for review and merge. - ---- - -## Post-relay — review nits 2 and 3 (locked flag, overlapping refreshes) - -Human asked to fix nit 2 and to fold in nit 3 if it belonged to the same group. It did: -both are small, contained changes to code this branch already introduced, and neither adds a -concept. Done as two commits because they are different concerns. - -### Nit 2 — `8f060d1` remove the unused `locked` flag - -I had defended keeping it in the previous review, arguing it documented the keep-locked -policy. That was wrong on inspection: the *test input* documents the policy, not the field. -Removed `GitWorktreeInfo.locked` and its parse line. The parser test still feeds a real -`locked keep me` porcelain line and now asserts it is ignored, and the service test still -pins that a present non-prunable worktree is kept. So the policy is covered with less surface, -and unknown porcelain keys were already ignored anyway. - -### Nit 3 — `f8c3fdb` serialize overlapping topology refreshes - -**This was not the cosmetic nit I rated it. It was a correctness bug**, and probing it first -is what revealed that. Scratch reproduction: two `refreshSelectedProjectTopology()` calls -issued 2 HTTP requests; resolving the newer one first and the older one second left -`state.workspaces` at the *older* list. So a worktree created outside PI WEB could appear and -then vanish again, which is precisely the bug this whole branch exists to fix. The machine and -project stale guards do not order responses. - -Fixed by routing the refresh through `TrailingRefreshCoordinator` keyed by -`machineProjectKey(machineId, project.id)` — the primitive already used by -`browserResumeController`, `sessionController`, and `activityController`, so this is reuse, -not a new mechanism. - -**Learned mid-implementation:** the coordinator does *not* collapse overlapping calls into one -request. It runs the second as a single trailing pass after the first finishes. My first test -asserted `toHaveBeenCalledOnce()` and hung for 5s until the vitest timeout. The code was -right and my assumption was wrong, so I rewrote the test to assert the real and stronger -guarantee: max-in-flight is 1, and the last response applied is the newest. Recorded in -`status.md` so the next person does not repeat the mistaken assertion. - -### Checks run - -- `npx vitest --run workspaceController.test.ts src/server/workspaces/` → **98 passed** - (10 in `workspaceController.test.ts`) -- mutation check for nit 3: bypassed the coordinator by invoking the refresh body directly → - the new serialization test failed, **alone**; restored → all pass -- `grep` for leftover `locked` references → only the intentional test input/comments -- `npx eslint` on all four changed files → clean -- **`npm run verify` → green**: 228 files, **1843 passed**, 2 skipped -- pre-commit `verify:staged` on each commit → 14 files/63 tests and 6 files/27 tests, passed - -### Artifacts changed - -- `src/server/workspaces/gitWorktreeDiscovery.ts` (removed `locked` field + parse line) -- `src/server/workspaces/gitWorktreeDiscovery.test.ts`, - `src/server/workspaces/workspaceService.test.ts` (retargeted to the policy, not the field) -- `src/client/src/controllers/workspaceController.ts` (+`TrailingRefreshCoordinator`) -- `src/client/src/controllers/workspaceController.test.ts` (+1 test, 10 total) -- `status.md`: commit list, verify count, and the coordinator-semantics warning - -### Blockers - -None. No changeset added: `f8c3fdb` fixes a defect in unreleased work from this same branch, -and the existing fragment already promises the list stays correct without user action. Still -no timer, watcher, process, endpoint, or push channel, and no sessiond code touched, so -**no manual session daemon restart is required**. diff --git a/.pi-web/relays/worktree-autodetect/plan.md b/.pi-web/relays/worktree-autodetect/plan.md deleted file mode 100644 index 9dabddb..0000000 --- a/.pi-web/relays/worktree-autodetect/plan.md +++ /dev/null @@ -1,168 +0,0 @@ -# Implementation plan — worktree-autodetect (reduced scope) - -Three legs. Each is a vertical slice: source + tests + checks + commit. - -The order is deliberate: server truth first, then client application of that truth, -then the trigger that makes it zero-intervention. - ---- - -## Leg 1 — Stop reporting removed worktrees (the inverse case) - -**Why first:** it is independently valuable, has zero UI risk, and is the only part of -the feature that is a straight bug fix. Today a worktree deleted with `rm -rf` outside -PI WEB stays in the workspace list forever as a selectable ghost. - -**Files** - -- `src/server/workspaces/gitWorktreeDiscovery.ts` -- new `src/server/workspaces/gitWorktreeDiscovery.test.ts` - -**Work** - -1. Extend the porcelain parser to read the valueless `prunable` and `locked` keys. - `git worktree list --porcelain` emits `prunable ` for a linked worktree whose - checkout directory no longer exists, and a bare `locked` line for a locked one. - Verified in leg 0 against real git. -2. Surface `prunable` on `GitWorktreeInfo`, and filter prunable entries out of what - `discoverGitWorktrees` returns — or return them and filter in `WorkspaceService`, - whichever keeps the parser honest and the policy visible. Prefer: parser reports - facts, `workspaceService` decides policy. -3. Do **not** run `git worktree prune`. Read-only. PI WEB must not mutate the user's - repo metadata as a side effect of listing. -4. Keep the main worktree unconditionally: never filter the entry whose path equals - `project.path`, so a project can never end up with an empty workspace list. - -**Tests** (pure parser tests, no git process needed — inject or fake the exec boundary) - -- parses `prunable` with a reason and `locked` without a value -- a prunable linked worktree is excluded from the workspace list -- a locked worktree is still included -- the main worktree survives even if git somehow marks it prunable - -**Checks:** `npm test -- --run src/server/workspaces/gitWorktreeDiscovery.test.ts`, -plus the workspaceService/app.projects tests if they touch the shape, plus -`npm run typecheck` (`GitWorktreeInfo` is exported). - ---- - -## Leg 2 — Non-disruptive workspace topology refresh in the client - -**Why second:** this is the risky part, and it must be provably non-disruptive before -anything starts calling it automatically. - -**Files** - -- `src/client/src/controllers/workspaceController.ts` -- new `src/client/src/controllers/workspaceController.test.ts` - -**Work** - -1. Add a method — suggested name `refreshSelectedProjectTopology()` — that re-lists the - selected project's workspaces and applies them via the existing - `applyProjectWorkspaces` path. -2. **Selection invariants it must hold:** - - If the currently selected workspace is still present, do **not** call - `selectWorkspace`, do **not** clear the active session, do **not** reset - workspace-scoped state. Only `workspaces` / `workspacesByProjectId` change. - - **Read this before writing the method — the wrong version looks correct.** The - tempting shape, mirroring `selectProject()` six lines above it, is: refresh the list, - then "re-resolve the selection to be safe" via - `selectPreferredWorkspace(...)` + `await this.selectWorkspace(...)`. That is the bug. - `selectWorkspace` has **no already-selected guard**, so even when it re-picks the very - same workspace it unconditionally runs: - - `sessions.clearActiveSession()` → `socket.close()` (closes the session WebSocket - mid-stream), `clearPendingUpdates()`, `streamWatermark = undefined` (buffered deltas - dropped), and `setState({ selectedSession: undefined, messages: [] })` (chat empties); - - `setState({ ...resetWorkspaceScopedState() })` → clears `sessions`, `fileTree`, - `expandedDirs`, `selectedFilePath`, `selectedFileContent`, `gitStatus`, - `selectedDiffPath`, `selectedDiff`, `selectedStagedDiff`, `selectedTerminalId`. - - Because leg 3 calls this from `refreshAfterBrowserResume`, that would fire on **every** - alt-tab back into PI WEB — not only when a worktree actually changed — blanking the - chat, collapsing the file tree, and closing any open diff every time, and losing stream - deltas that arrive while the socket is down. Applying the list via - `applyProjectWorkspaces` alone is sufficient for the feature; `handleWorkspaceChange` - early-returns when the selected workspace id is unchanged, so a fresh-but-equal list - causes no downstream churn on its own. - - If nothing is selected, just apply the list. - - If the selected workspace **disappeared**, do not silently jump. Leave the - selection as-is and let the existing deletion path own recovery; the user is - currently working there and a surprise switch is worse than a stale label. - If leg 2 finds this cannot be left alone safely, that is the intervention signal. -3. Guard against machine/project changing mid-flight, exactly like `selectProject` does - (compare `selectedMachineId` and `selectedProject?.id` before applying). -4. Swallow-and-report errors the way sibling background refreshes do (`console.warn`, - not `setState({ error })`) — a background topology refresh must never paint an error - banner over a working session. - -**Tests** (controller-layer, fake `api.workspaces`) - -- a newly appeared worktree lands in `workspaces` and `workspacesByProjectId` -- the selected workspace is preserved; `sessions.clearActiveSession` is **not** called -- a stale response for a project the user has since left is discarded -- a rejected request does not set `state.error` - -**Checks:** `npm test -- --run src/client/src/controllers/workspaceController.test.ts`. - ---- - -## Leg 3 — Wire it to the existing resume path, document, changeset - -**Why last:** only after leg 2 proves the refresh is inert. - -**Files** - -- `src/client/src/components/PiWebApp.ts` — `refreshAfterBrowserResume` (~432) and - `refreshAppData` (~485). Touch only these two methods. -- possibly `src/client/src/components/PiWebApp.*.test.ts` (a focused new test file is fine) -- `docs/` — one short paragraph where workspaces/worktrees are explained; follow - `.agents/skills/documentation-guide/SKILL.md` and do **not** grow `README.md` -- `.changeset/*.md` - -**Work** - -1. Add `this.workspaces.refreshSelectedProjectTopology()` to the `Promise.all` in - `refreshAfterBrowserResume` and to `refreshAppData`. - - `BrowserResumeController` already debounces per animation frame and collapses - concurrent requests through `TrailingRefreshCoordinator`, so no extra throttling - is needed. Verified in leg 0. - - This inherits remote-machine support for free: `api.workspaces(projectId, machineId)` - already routes through the machine proxy, and `/projects/:projectId/workspaces` is - already in `FEDERATED_HTTP_ROUTES`. -2. Optionally also refresh on realtime-socket reconnect (`connectRealtime`'s - `onReconnect`), which is the same class of natural event. Only if it costs nothing. - Add the call directly; this relay owns the seam and is not coordinating with any - other branch. -3. Document the behavior honestly: detection happens when the tab regains focus / - becomes visible, not instantly. -4. Add the changeset (`npm run changeset`, or write the fragment directly). - -**Checks:** the new/affected client tests, then **`npm run verify`** — this is the final -leg and the change is cross-cutting. - ---- - -## Cost ledger (accepted in leg 0) - -| Cost | Amount | -|---|---| -| New processes | 0 | -| New watchers (inotify/fs.watch) | 0 | -| New timers | 0 | -| New endpoints / push channels | 0 | -| Extra request per browser resume, per selected project | 1 (~2ms of `git worktree list` server-side) | -| Production lines changed | ~60 | -| New test files | 3 | - -## Known risks - -- **UI churn on refresh.** Mitigated by leg 2's invariants and its tests. This is the - one that can kill the feature; it is an explicit intervention trigger. -- **Latency expectation.** Detection is resume-scoped. A user staring at an already-focused - tab while a worktree appears in another window sees nothing until they refocus. This is - an accepted, documented limit — not a bug to fix with a timer. -- **Overlap with the sibling `worktree-create-ui` effort.** Settled by the human: assume - that session does nothing. This relay owns the refresh seam; build it here without - designing for reuse, and do not read that worktree. diff --git a/.pi-web/relays/worktree-autodetect/status.md b/.pi-web/relays/worktree-autodetect/status.md deleted file mode 100644 index 1248875..0000000 --- a/.pi-web/relays/worktree-autodetect/status.md +++ /dev/null @@ -1,98 +0,0 @@ -# Status — relay "worktree-autodetect" - -## 🏁 FINISHED — the relay reached its finish line - -All three legs are complete and committed on `feat/worktree-autodetect`. `npm run verify` is -green. No further leg was spawned; leg 3 was the last one by design. - -Remaining human action: review the branch and merge it. Nothing is blocked. - -## Post-relay review fix - -A human review question after leg 3 ("do we have pragmatic reasonable and stable code?") -found one real gap, fixed in `79577e4`: `applyProjectWorkspaces` replaced the list but left -`selectedWorkspace` pointing at the old object, so a branch switched inside a worktree -outside PI WEB showed the new name in the list and the old one in the collapsed Workspaces -header and mobile context bar. Now re-pointed by id, and skipped entirely when metadata is -unchanged so a normal resume does not churn identity. Two tests added (9 total in -`workspaceController.test.ts`), each mutation-checked in both directions. `npm run verify` -green: 1842 passed, 2 skipped (1843 after the two follow-up fixes below). - -The other two review nits were then fixed as well, after the human asked: - -- `8f060d1` removed the unused `GitWorktreeInfo.locked` flag. The parser test still feeds a - real `locked` porcelain line and asserts it is ignored, so the keep-locked-worktrees policy - stays covered without a speculative field. -- `f8c3fdb` fixed what turned out to be a **correctness bug, not the cosmetic nit I first - rated it**. The resume path and the plugin-facing app refresh call - `refreshSelectedProjectTopology` independently, and the stale guards check machine and - project but *not ordering* — so a slower earlier response landing last overwrote a newer - list and a just-created worktree disappeared again. Reproduced with a scratch test before - fixing. Now routed through `TrailingRefreshCoordinator` keyed by machine+project. - -Note for anyone extending this: `TrailingRefreshCoordinator` does **not** collapse two -overlapping calls into one HTTP request. It runs the second as one trailing pass after the -first completes, so the newest response is always applied last. An assertion of -`toHaveBeenCalledOnce()` will hang against it. - -## Current position - -Every charter finish-line condition is satisfied: - -1. ✅ **Prunable worktrees hidden** — `266f941`. `discoverGitWorktrees` parses `prunable`; - `WorkspaceService` filters those worktrees out, while always keeping the project's own - worktree so a project is never empty. -2. ✅ **Non-disruptive client refresh** — `d0f8f9f`. - `WorkspaceController.refreshSelectedProjectTopology()` applies results through - `applyProjectWorkspaces` only, never `selectWorkspace`, so selection, session, file tree, - git status, and terminal selection survive. A vanished selected workspace is left alone - for the existing deletion path to handle. -3. ✅ **Wired to the existing resume path** — `84545fb`. Called from - `PiWebApp.refreshAfterBrowserResume` and `refreshAppData`. No new timer, watcher, - process, endpoint, or push channel. Remote machines work through the existing - `machinePrefix` + `FEDERATED_HTTP_ROUTES` plumbing. -4. ✅ **Tests** — prunable parsing/filtering (`gitWorktreeDiscovery.test.ts`, - `workspaceService.test.ts`), refresh-preserves-selection and - refresh-when-selected-workspace-disappeared (`workspaceController.test.ts`, 7 tests), - resume + app-data wiring (`PiWebApp.workspaceTopology.test.ts`, 3 tests). -5. ✅ **`npm run verify` green** (228 files / 1843 passed, 2 skipped) and - `.changeset/worktree-autodetect.md` exists. - -Nothing in this work touched `src/server/sessiond.ts`, session runtime ownership, or the -daemon protocol. **No manual session daemon restart is required.** - -## Leg tracking - -- **Last completed leg:** 3 (final — wiring, docs, changeset, full verify) -- **Next leg to run:** none. Relay complete; do not spawn another runner. - -## Shipped behavior, as a user sees it - -A worktree created or deleted outside PI WEB shows up in (or disappears from) the workspace -list the next time the browser tab regains focus or becomes visible. Detection is -resume-scoped, not instant, by explicit human decision. A worktree whose checkout directory -was `rm -rf`ed no longer appears as a selectable workspace. Documented in `docs/faq.html` -under `#worktree-list-out-of-date`. - -## Relevant context if anyone picks this branch up - -- Commits: `266f941` (server filter), `d0f8f9f` (client refresh method), - `84545fb` (wiring + docs + changeset), `79577e4` (selected-workspace freshness fix), - `8f060d1` (drop unused `locked`), `f8c3fdb` (serialize overlapping refreshes), - plus the `docs(relay)` packet commits. -- Second invariant, added by `79577e4`: re-pointing `selectedWorkspace` is safe **only** - while it is keyed by `id` and skipped on unchanged metadata. Keying it by anything that can - differ between two lists would change the selection on a background refresh; dropping the - unchanged-metadata guard would push a new object into state on every browser focus. -- The invariant to protect on any future edit: **never route a background topology refresh - through `selectWorkspace`.** It calls `clearActiveSession()` and - `resetWorkspaceScopedState()` with no already-selected guard, which would close the session - socket and blank the chat on every browser resume. `workspaceController.test.ts` was - mutation-checked against exactly that regression in leg 2. -- The relay packet lives under `.pi-web/`, which is gitignored; packet commits used - `git add -f`. - -## Blockers - -None. No intervention signal fired in any leg. The one serious known risk — visible UI churn -on refresh — did not materialize and needed no workaround.