From bd4a891b95c0fffb58e84a9a2c4391ef47b68498 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 22:10:58 +0200 Subject: [PATCH 01/17] fix(sessions): correlate startup progress by token instead of workspace Startup progress could still be shown on the wrong session's row. Routing by known session id first closed the case where the browser knew the other session, but left open the case where it does not -- which the browser is designed to produce. While a create is pending for a workspace, applyCreatedSession deliberately withholds a session.created event for that workspace and stashes it, to avoid a duplicate row. So during exactly the window this feature exists for, a session created by an agent's spawn or by another tab is intentionally absent from the session list. Its startup events carried an unrecognised id and a matching cwd, and were routed onto the user's pending create row, showing a phase and a label belonging to another session. Workspace path was never evidence of identity; it was the only key both sides happened to share. Give them a real one. The browser already invents a temporary row id for a pending create, so it now sends that id with the create request as an opaque startupToken; the daemon carries it through construction, echoes it on the startup events it publishes for that construction, and the browser matches it exactly. The token is a throwaway label the daemon never interprets. It never becomes the session id: activity.sessionId still carries Pi's SessionManager id, which remains how an open of an already-known session is routed. With exact identity available, the guessing is deleted rather than gated. startupProgressPendingStart goes entirely, and with it the selected-machine comparison, the cwd filter, and the single-match ambiguity rule: a second concurrent create carries a different token, and a foreign workspace or non-selected machine carries no token this browser is waiting on, so those cases stop existing rather than needing detection. One Map lookup replaces a filtered scan. cwd comes off the event, since it existed only as the routing key and nothing else read it. No compatibility path is needed. session.startup is unreleased -- checked against the published tarball, not only git tags -- so no deployed daemon emits these events and no deployed browser parses them. An older daemon ignores the extra request field; a newer daemon talking to an older browser degrades to the pre-existing generic wording, as does any unmatched token. One silent behaviour change to state plainly: startupProgress guarded on `sessionId === "" || cwd === ""`. Removing cwd from the event removes the meaningful half of that guard, and that half had no test. The session-id half is kept, which is the half that actually protects honest reporting. The replaced ambiguity test is rewritten rather than dropped, so the same three scenarios still pin the user-visible guarantee -- no match means the generic wording stays -- now including the reproduced foreign-session case, which fails against the previous code. Session creation ordering, semantics, and queueing are unchanged; the token is a passthrough label read only to build an event. --- src/client/src/api/clients.test.ts | 20 ++++ src/client/src/api/clients.ts | 2 +- src/client/src/api/parsers.test.ts | 30 +++--- src/client/src/api/parsers.ts | 10 +- .../sessionController.startupProgress.test.ts | 91 +++++++++++++------ .../src/controllers/sessionController.ts | 32 ++----- src/client/src/sessionSocket.test.ts | 13 +-- .../piSessionService.startupProgress.test.ts | 35 ++++++- src/server/sessions/piSessionService.ts | 36 +++++--- src/server/sessions/sessionRoutes.test.ts | 37 +++++++- src/server/sessions/sessionRoutes.ts | 8 +- src/server/sessions/sessionService.ts | 7 +- src/shared/apiTypes.ts | 12 +-- 13 files changed, 231 insertions(+), 102 deletions(-) diff --git a/src/client/src/api/clients.test.ts b/src/client/src/api/clients.test.ts index eda5907..26f990a 100644 --- a/src/client/src/api/clients.test.ts +++ b/src/client/src/api/clients.test.ts @@ -253,6 +253,22 @@ describe("session API compatibility", () => { expect(JSON.parse(requestBody(fetchCall(fetchMock, 1)[1]))).toEqual({ sessions: [{ id: "s 1", cwd: "/repo" }] }); }); + it("carries a create's correlation token in the start request body when one is supplied", async () => { + const fetchMock = stubSequenceFetch([ + jsonResponse(sessionInfoResponse("s 1")), + jsonResponse(sessionInfoResponse("s 2")), + ]); + + await sessionsApi.startSession("/repo", "remote a", "pending-session-3-k2x9"); + await sessionsApi.startSession("/repo", "remote a"); + + expect(fetchCall(fetchMock, 0)[0]).toBe("https://pi.example.test/api/machines/remote%20a/sessions"); + expect(JSON.parse(requestBody(fetchCall(fetchMock, 0)[1]))).toEqual({ cwd: "/repo", startupToken: "pending-session-3-k2x9" }); + // The token is optional, so a caller with no row to label sends none rather + // than an empty one. + expect(JSON.parse(requestBody(fetchCall(fetchMock, 1)[1]))).toEqual({ cwd: "/repo" }); + }); + it("keeps legacy session-id calls free of cwd context", async () => { const fetchMock = stubJsonFetch({ accepted: true }); @@ -572,6 +588,10 @@ function requestBody(init: RequestInit | undefined): string { return init.body; } +function sessionInfoResponse(id: string) { + return { id, path: `/tmp/${id}.jsonl`, cwd: "/repo", created: "now", modified: "now", messageCount: 0, firstMessage: "" }; +} + function piWebConfigResponse(config: PiWebConfigValues) { return { path: "/tmp/pi-web/config.json", diff --git a/src/client/src/api/clients.ts b/src/client/src/api/clients.ts index d5ca7d0..e58012e 100644 --- a/src/client/src/api/clients.ts +++ b/src/client/src/api/clients.ts @@ -214,7 +214,7 @@ export const sessionsApi = { notificationInbox: (session: SessionLookup, machineId = "local") => request(sessionQueryPath(session, "notifications", machineId), parseSessionNotificationInboxSnapshot), dismissNotification: (session: SessionLookup, daemonInstanceId: string, notificationId: string, machineId = "local") => request(sessionPath(session, "notifications/dismiss", machineId), parseSessionNotificationInboxSnapshot, { method: "POST", body: sessionBody(session, { daemonInstanceId, notificationId }) }), dismissAllNotifications: (session: SessionLookup, daemonInstanceId: string, through: SessionNotificationDismissThrough, machineId = "local") => request(sessionPath(session, "notifications/dismiss-all", machineId), parseSessionNotificationInboxSnapshot, { method: "POST", body: sessionBody(session, { daemonInstanceId, throughOrder: through.order, throughOverflowWatermark: through.overflowWatermark }) }), - startSession: (cwd: string, machineId = "local") => request(`${machinePrefix(machineId)}/sessions`, parseSessionInfo, { method: "POST", body: JSON.stringify({ cwd }) }), + startSession: (cwd: string, machineId = "local", startupToken?: string) => request(`${machinePrefix(machineId)}/sessions`, parseSessionInfo, { method: "POST", body: JSON.stringify(startupToken === undefined ? { cwd } : { cwd, startupToken }) }), cleanupPreview: (input: SessionCleanupRequest, machineId = "local") => request(`${machinePrefix(machineId)}/sessions/cleanup/preview`, parseSessionCleanupPreviewResponse, { method: "POST", body: JSON.stringify(input) }), cleanup: (input: SessionCleanupRequest, machineId = "local") => request(`${machinePrefix(machineId)}/sessions/cleanup`, parseSessionCleanupExecuteResponse, { method: "POST", body: JSON.stringify(input) }), archiveMany: (sessions: readonly SessionLookup[], machineId = "local") => request(`${machinePrefix(machineId)}/sessions/bulk/archive`, parseSessionBulkArchiveResponse, { method: "POST", body: sessionBulkMutationBody(sessions) }), diff --git a/src/client/src/api/parsers.test.ts b/src/client/src/api/parsers.test.ts index d356dc2..a05ec35 100644 --- a/src/client/src/api/parsers.test.ts +++ b/src/client/src/api/parsers.test.ts @@ -289,18 +289,18 @@ describe("API parsers", () => { })).toThrow("positive safe integer"); }); - it("parses session startup progress with and without a wait detail", () => { + it("parses session startup progress with and without a correlation token", () => { const activity = { sessionId: "session-1", phase: "active", label: "Creating session", detail: "Starting the Pi session", at: "2026-07-20T00:00:01.000Z" }; - expect(parseSessionStartupProgressEvent({ type: "session.startup", cwd: "/repo", activity })).toEqual({ + expect(parseSessionStartupProgressEvent({ type: "session.startup", startupToken: "pending-session-1-abc", activity })).toEqual({ type: "session.startup", - cwd: "/repo", + startupToken: "pending-session-1-abc", activity, }); + // An open carries no token: the activity's own session id is the only route. const idle = { sessionId: "session-1", phase: "idle", label: "idle", at: "2026-07-20T00:00:02.000Z" }; - expect(parseSessionStartupProgressEvent({ type: "session.startup", cwd: "/repo", activity: idle })).toEqual({ + expect(parseSessionStartupProgressEvent({ type: "session.startup", activity: idle })).toEqual({ type: "session.startup", - cwd: "/repo", activity: idle, }); }); @@ -308,15 +308,17 @@ describe("API parsers", () => { it("rejects session startup progress that cannot be routed or rendered honestly", () => { const activity = { sessionId: "session-1", phase: "active", label: "Creating session", at: "2026-07-20T00:00:01.000Z" }; - expect(() => parseSessionStartupProgressEvent({ type: "activity.update", cwd: "/repo", activity })).toThrow("Invalid session startup event type"); - expect(() => parseSessionStartupProgressEvent({ type: "session.startup", activity })).toThrow("Expected string field: cwd"); - expect(() => parseSessionStartupProgressEvent({ type: "session.startup", cwd: "", activity })).toThrow("Expected non-empty string field: cwd"); - expect(() => parseSessionStartupProgressEvent({ type: "session.startup", cwd: "/repo" })).toThrow("Expected object response"); - expect(() => parseSessionStartupProgressEvent({ type: "session.startup", cwd: "/repo", activity: { ...activity, phase: "waiting" } })).toThrow("Expected session activity phase field: phase"); - expect(() => parseSessionStartupProgressEvent({ type: "session.startup", cwd: "/repo", activity: { ...activity, label: 7 } })).toThrow("Expected string field: label"); - expect(() => parseSessionStartupProgressEvent({ type: "session.startup", cwd: "/repo", activity: { ...activity, label: "" } })).toThrow("Expected non-empty string field: label"); - expect(() => parseSessionStartupProgressEvent({ type: "session.startup", cwd: "/repo", activity: { ...activity, detail: 7 } })).toThrow("Expected optional string field: detail"); - expect(() => parseSessionStartupProgressEvent({ type: "session.startup", cwd: "/repo", activity: { ...activity, sessionId: "" } })).toThrow("Expected non-empty string field: sessionId"); + expect(() => parseSessionStartupProgressEvent({ type: "activity.update", activity })).toThrow("Invalid session startup event type"); + expect(() => parseSessionStartupProgressEvent({ type: "session.startup" })).toThrow("Expected object response"); + expect(() => parseSessionStartupProgressEvent({ type: "session.startup", startupToken: 7, activity })).toThrow("Expected optional string field: startupToken"); + // An empty token would match nothing but must still be rejected rather than + // silently carried, so a malformed frame never reaches the routing at all. + expect(() => parseSessionStartupProgressEvent({ type: "session.startup", startupToken: "", activity })).toThrow("Expected non-empty string field: startupToken"); + expect(() => parseSessionStartupProgressEvent({ type: "session.startup", activity: { ...activity, phase: "waiting" } })).toThrow("Expected session activity phase field: phase"); + expect(() => parseSessionStartupProgressEvent({ type: "session.startup", activity: { ...activity, label: 7 } })).toThrow("Expected string field: label"); + expect(() => parseSessionStartupProgressEvent({ type: "session.startup", activity: { ...activity, label: "" } })).toThrow("Expected non-empty string field: label"); + expect(() => parseSessionStartupProgressEvent({ type: "session.startup", activity: { ...activity, detail: 7 } })).toThrow("Expected optional string field: detail"); + expect(() => parseSessionStartupProgressEvent({ type: "session.startup", activity: { ...activity, sessionId: "" } })).toThrow("Expected non-empty string field: sessionId"); }); it("parses session cleanup preview and execute responses", () => { diff --git a/src/client/src/api/parsers.ts b/src/client/src/api/parsers.ts index 9941aa0..4621037 100644 --- a/src/client/src/api/parsers.ts +++ b/src/client/src/api/parsers.ts @@ -275,15 +275,19 @@ export function parseSessionUnreadEvent(value: unknown): SessionUnreadEvent { /** * Validate a startup progress frame. The browser substitutes its own wording * from this event, so a malformed frame must be dropped rather than rendered: - * `cwd` is the routing key, and an activity missing its phase or label could - * otherwise blank out or freeze the text a user is reading while they wait. + * `startupToken` is the routing key when present, and an activity missing its + * phase or label could otherwise blank out or freeze the text a user is reading + * while they wait. An absent token is valid — an open routes by session id — but + * a present empty one is not, since it could match no row honestly. */ export function parseSessionStartupProgressEvent(value: unknown): SessionStartupProgressEvent { const record = requireRecord(value); if (record["type"] !== "session.startup") throw new Error("Invalid session startup event type"); + const startupToken = optionalString(record, "startupToken"); + if (startupToken === "") throw new Error("Expected non-empty string field: startupToken"); return { type: "session.startup", - cwd: requireNonEmptyString(record, "cwd"), + ...optionalField("startupToken", startupToken), activity: parseSessionActivity(record["activity"]), }; } diff --git a/src/client/src/controllers/sessionController.startupProgress.test.ts b/src/client/src/controllers/sessionController.startupProgress.test.ts index 0804e18..8b59c8d 100644 --- a/src/client/src/controllers/sessionController.startupProgress.test.ts +++ b/src/client/src/controllers/sessionController.startupProgress.test.ts @@ -3,8 +3,6 @@ import { initialAppState } from "../appState"; import { SessionController } from "./sessionController"; import { defaultApi, deferred, emptyPage, FakeSocket, oldSession, runPendingAnimationFrames, sessionLookupId, status, workspace, type AppState, type SessionActivity, type SessionInfo } from "./sessionController.testSupport"; -const REMOTE_MACHINE = { id: "remote", name: "Remote", kind: "remote" as const, createdAt: "now", updatedAt: "now" }; - function startupActivity(patch: Partial = {}): SessionActivity { return { sessionId: "backend-session", @@ -21,8 +19,15 @@ function idleStartupActivity(): SessionActivity { return { sessionId: "backend-session", phase: "idle", label: "idle", at: "2026-07-20T00:00:02.000Z" }; } +interface StartCall { + cwd: string; + machineId: string | undefined; + startupToken: string | undefined; +} + function pendingStartController(state: { current: AppState }, api: Partial = {}) { const startRequest = deferred(); + const startCalls: StartCall[] = []; const controller = new SessionController( () => state.current, (patch) => { state.current = { ...state.current, ...patch }; }, @@ -31,7 +36,10 @@ function pendingStartController(state: { current: AppState }, api: Partial startRequest.promise, + startSession: (cwd: string, machineId?: string, startupToken?: string) => { + startCalls.push({ cwd, machineId, startupToken }); + return startRequest.promise; + }, messages: () => Promise.resolve(emptyPage), status: (session) => Promise.resolve(status(sessionLookupId(session))), ...api, @@ -39,7 +47,7 @@ function pendingStartController(state: { current: AppState }, api: Partial { @@ -51,7 +59,7 @@ describe("SessionController session startup progress", () => { const temporaryId = state.current.selectedSession?.id; if (temporaryId === undefined) throw new Error("Expected temporary session id"); - controller.applyGlobalEvent({ type: "session.startup", cwd: workspace.path, activity: startupActivity() }); + controller.applyGlobalEvent({ type: "session.startup", startupToken: temporaryId, activity: startupActivity() }); runPendingAnimationFrames(); // The label changes while the user is waiting, before the start resolves, @@ -59,7 +67,7 @@ describe("SessionController session startup progress", () => { expect(state.current.activity).toMatchObject({ sessionId: temporaryId, phase: "active", label: "Creating session", detail: "Starting the Pi session" }); expect(state.current.sessionActivities[temporaryId]).toMatchObject({ detail: "Starting the Pi session" }); - controller.applyGlobalEvent({ type: "session.startup", cwd: workspace.path, activity: startupActivity({ detail: "Loading session extensions" }) }); + controller.applyGlobalEvent({ type: "session.startup", startupToken: temporaryId, activity: startupActivity({ detail: "Loading session extensions" }) }); runPendingAnimationFrames(); expect(state.current.activity?.detail).toBe("Loading session extensions"); @@ -75,10 +83,10 @@ describe("SessionController session startup progress", () => { const start = controller.startSession(); const temporaryId = state.current.selectedSession?.id; if (temporaryId === undefined) throw new Error("Expected temporary session id"); - controller.applyGlobalEvent({ type: "session.startup", cwd: workspace.path, activity: startupActivity() }); + controller.applyGlobalEvent({ type: "session.startup", startupToken: temporaryId, activity: startupActivity() }); runPendingAnimationFrames(); - controller.applyGlobalEvent({ type: "session.startup", cwd: workspace.path, activity: idleStartupActivity() }); + controller.applyGlobalEvent({ type: "session.startup", startupToken: temporaryId, activity: idleStartupActivity() }); runPendingAnimationFrames(); expect(state.current.activity).toMatchObject({ @@ -98,7 +106,9 @@ describe("SessionController session startup progress", () => { const start = controller.startSession(); await controller.send("queued while starting"); - controller.applyGlobalEvent({ type: "session.startup", cwd: workspace.path, activity: idleStartupActivity() }); + const temporaryId = state.current.selectedSession?.id; + if (temporaryId === undefined) throw new Error("Expected temporary session id"); + controller.applyGlobalEvent({ type: "session.startup", startupToken: temporaryId, activity: idleStartupActivity() }); runPendingAnimationFrames(); expect(state.current.activity?.detail).toBe("1 queued message will send when the backend session is ready"); @@ -119,7 +129,6 @@ describe("SessionController session startup progress", () => { controller.applyGlobalEvent({ type: "session.startup", - cwd: oldSession.cwd, activity: startupActivity({ sessionId: oldSession.id, label: "Opening session" }), }); runPendingAnimationFrames(); @@ -136,12 +145,11 @@ describe("SessionController session startup progress", () => { const temporaryId = state.current.selectedSession?.id; if (temporaryId === undefined) throw new Error("Expected temporary session id"); - // Opening an existing session in the same workspace publishes the same cwd as - // the pending create. The known id is the proof of which row it belongs to, so - // the pending row must keep its own wording instead of the other row's phase. + // Opening an existing session in the same workspace carries no create token, + // so the known id is the only proof of which row it belongs to and the pending + // row must keep its own wording instead of the other row's phase. controller.applyGlobalEvent({ type: "session.startup", - cwd: workspace.path, activity: startupActivity({ sessionId: existing.id, label: "Opening session" }), }); runPendingAnimationFrames(); @@ -153,7 +161,7 @@ describe("SessionController session startup progress", () => { await start; }); - it("keeps the generic wording when the startup progress cannot be attributed to one row", async () => { + it("keeps the generic wording when no pending row's token matches the startup progress", async () => { const state = { current: { ...initialAppState(), selectedWorkspace: workspace, sessions: [] } }; const { controller, startRequest } = pendingStartController(state); @@ -161,29 +169,54 @@ describe("SessionController session startup progress", () => { const temporaryId = state.current.selectedSession?.id; if (temporaryId === undefined) throw new Error("Expected temporary session id"); - // Another workspace's startup. - controller.applyGlobalEvent({ type: "session.startup", cwd: "/elsewhere", activity: startupActivity() }); - // The selected machine's socket is the only feed for these events, so a cwd - // that matches while another machine is selected belongs to a different row. - state.current = { ...state.current, selectedMachine: REMOTE_MACHINE }; - controller.applyGlobalEvent({ type: "session.startup", cwd: workspace.path, activity: startupActivity() }); - state.current = { ...state.current, selectedMachine: undefined }; + // Another browser tab's create, or another workspace's: its token is one this + // browser never minted, so there is no row here that it belongs to. + controller.applyGlobalEvent({ type: "session.startup", startupToken: "pending-session-9-other-tab", activity: startupActivity() }); + // A session this browser has not been told about — an agent's spawned + // subsession, say, whose `session.created` a pending create suppresses — is + // opened rather than created, so it carries no token at all. + controller.applyGlobalEvent({ type: "session.startup", activity: startupActivity({ sessionId: "foreign-session", label: "Opening session", detail: "Loading session extensions" }) }); runPendingAnimationFrames(); expect(state.current.activity?.detail).toBe("Waiting for the backend session to be ready"); + expect(state.current.activity?.label).toBe("Creating session"); - // A second concurrent start in the same workspace makes the target ambiguous, - // so neither row is given a phase that might belong to the other. + startRequest.resolve({ ...oldSession, id: "backend-session", path: "/tmp/backend-session.jsonl" }); + await start; + }); + + it("gives each of two concurrent creates only the progress its own token carries", async () => { + const state = { current: { ...initialAppState(), selectedWorkspace: workspace, sessions: [] } }; + const { controller, startRequest } = pendingStartController(state); + + const start = controller.startSession(); + const firstId = state.current.selectedSession?.id; const secondStart = controller.startSession(); - controller.applyGlobalEvent({ type: "session.startup", cwd: workspace.path, activity: startupActivity() }); + const secondId = state.current.selectedSession?.id; + if (firstId === undefined || secondId === undefined || firstId === secondId) throw new Error("Expected two distinct temporary session ids"); + + // Two creates in the same workspace are indistinguishable by workspace path; + // the token each request carried is what tells them apart. + controller.applyGlobalEvent({ type: "session.startup", startupToken: secondId, activity: startupActivity({ detail: "Loading session extensions" }) }); runPendingAnimationFrames(); - const secondTemporaryId = state.current.selectedSession?.id; - expect(secondTemporaryId).not.toBe(temporaryId); - expect(state.current.sessionActivities[temporaryId]?.detail).toBe("Waiting for the backend session to be ready"); - expect(state.current.sessionActivities[secondTemporaryId ?? ""]?.detail).toBe("Waiting for the backend session to be ready"); + expect(state.current.sessionActivities[secondId]).toMatchObject({ sessionId: secondId, detail: "Loading session extensions" }); + expect(state.current.sessionActivities[firstId]?.detail).toBe("Waiting for the backend session to be ready"); startRequest.resolve({ ...oldSession, id: "backend-session", path: "/tmp/backend-session.jsonl" }); await Promise.all([start, secondStart]); }); + + it("sends the pending row's own id as the create request's correlation token", async () => { + const state = { current: { ...initialAppState(), selectedWorkspace: workspace, sessions: [] } }; + const { controller, startRequest, startCalls } = pendingStartController(state); + + const start = controller.startSession(); + const temporaryId = state.current.selectedSession?.id; + + expect(startCalls).toEqual([{ cwd: workspace.path, machineId: "local", startupToken: temporaryId }]); + + startRequest.resolve({ ...oldSession, id: "backend-session", path: "/tmp/backend-session.jsonl" }); + await start; + }); }); diff --git a/src/client/src/controllers/sessionController.ts b/src/client/src/controllers/sessionController.ts index 2d7f65e..46fbaa0 100644 --- a/src/client/src/controllers/sessionController.ts +++ b/src/client/src/controllers/sessionController.ts @@ -185,7 +185,7 @@ export class SessionController { this.pendingSessionStarts.set(pending.tempId, pending); this.insertAndSelectPendingSession(pending.session); try { - const session = await this.api.startSession(workspace.path, machineId); + const session = await this.api.startSession(workspace.path, machineId, pending.tempId); await this.resolvePendingSessionStart(pending.tempId, session); } catch (error) { this.failPendingSessionStart(pending.tempId, error); @@ -1309,25 +1309,20 @@ export class SessionController { } // Session startup progress arrives while the daemon is still constructing the - // session, so the target row is resolved by session id when the browser knows - // it and by workspace path when it does not: a pending start knows its cwd but - // not the session id the daemon is creating. Once the row is resolved the - // progress goes through the normal activity buffer, so it renders exactly like - // any other activity and stays batched per frame. + // session, so the target row is resolved by exact identity only: a session id + // the browser already knows (an open), else the correlation token this browser + // minted for its own create and the daemon echoed back. Matching neither means + // the row is not one this browser shows — an agent's or another tab's session is + // *deliberately* absent while a create is pending — so it is ignored rather than + // guessed at. A resolved row goes through the normal activity buffer, rendering + // like any other activity and staying batched per frame. private queueStartupProgress(event: SessionStartupProgressEvent): void { - // A known session id is the strongest possible proof of the target, so it is - // checked first: while a create is pending in a workspace, an *existing* - // session in that same workspace can be opened too (another row selected, - // another tab, a subsession), and that open publishes the same cwd. Matching - // on cwd first would paint the pending row with another session's phase. if (this.getState().sessions.some((session) => session.id === event.activity.sessionId)) { this.queueActivityUpdate(event.activity); return; } - // The id is unknown, so this can only be a create whose id the browser has - // not been told yet. Route it by workspace path, the one key both sides share. - const pending = this.startupProgressPendingStart(event.cwd); - if (pending === undefined) return; + const pending = event.startupToken === undefined ? undefined : this.pendingSessionStarts.get(event.startupToken); + if (pending === undefined || pending.discarded) return; // An idle startup phase means the daemon has nothing left to attribute, so // restore this row's own generic wording rather than clearing the text of a // creation request that has not returned yet. @@ -1336,13 +1331,6 @@ export class SessionController { : { ...event.activity, sessionId: pending.tempId }); } - private startupProgressPendingStart(cwd: string): PendingSessionStart | undefined { - const machineId = selectedMachineId(this.getState()); - const matches = Array.from(this.pendingSessionStarts.values()) - .filter((pending) => pending.cwd === cwd && pending.machineId === machineId && !pending.discarded); - return matches.length === 1 ? matches[0] : undefined; - } - private schedulePendingFlush(): void { if (this.pendingFrame !== undefined) return; this.pendingFrame = requestAnimationFrame(() => { diff --git a/src/client/src/sessionSocket.test.ts b/src/client/src/sessionSocket.test.ts index efb2163..0337316 100644 --- a/src/client/src/sessionSocket.test.ts +++ b/src/client/src/sessionSocket.test.ts @@ -93,14 +93,15 @@ describe("notification socket guards", () => { it("accepts validated session startup progress and drops malformed frames", () => { const activity = { sessionId: "session-1", phase: "active", label: "Creating session", detail: "Starting the Pi session", at: "2026-07-20T00:00:01.000Z" }; - expect(parseRealtimeSocketEvent({ type: "session.startup", cwd: "/repo", activity })) - .toMatchObject({ type: "session.startup", cwd: "/repo", activity }); - expect(parseRealtimeSocketEvent({ type: "session.startup", cwd: "", activity })).toBeUndefined(); - expect(parseRealtimeSocketEvent({ type: "session.startup", cwd: "/repo" })).toBeUndefined(); - expect(parseRealtimeSocketEvent({ type: "session.startup", cwd: "/repo", activity: { ...activity, phase: "waiting" } })).toBeUndefined(); + expect(parseRealtimeSocketEvent({ type: "session.startup", startupToken: "pending-session-1-abc", activity })) + .toMatchObject({ type: "session.startup", startupToken: "pending-session-1-abc", activity }); + expect(parseRealtimeSocketEvent({ type: "session.startup", activity })).toMatchObject({ type: "session.startup", activity }); + expect(parseRealtimeSocketEvent({ type: "session.startup", startupToken: "", activity })).toBeUndefined(); + expect(parseRealtimeSocketEvent({ type: "session.startup" })).toBeUndefined(); + expect(parseRealtimeSocketEvent({ type: "session.startup", activity: { ...activity, phase: "waiting" } })).toBeUndefined(); // Startup progress is global-only, so it must not be accepted as a // per-session frame even when it is well formed. - expect(parseSessionSocketEvent({ type: "session.startup", cwd: "/repo", activity })).toBeUndefined(); + expect(parseSessionSocketEvent({ type: "session.startup", activity })).toBeUndefined(); }); it("preserves existing event acceptance without treating unknown types as realtime events", () => { diff --git a/src/server/sessions/piSessionService.startupProgress.test.ts b/src/server/sessions/piSessionService.startupProgress.test.ts index 57bf67f..aa55a8d 100644 --- a/src/server/sessions/piSessionService.startupProgress.test.ts +++ b/src/server/sessions/piSessionService.startupProgress.test.ts @@ -77,7 +77,7 @@ describe("PiSessionService session startup progress", () => { // The proof that matters: the user is told what is being waited on before // the wait ends, not after it. expect(startupText(hub)).toEqual(["Creating session: Starting the Pi session"]); - expect(startupEvents(hub).at(0)).toMatchObject({ cwd: "/workspace", activity: { sessionId: "session-1", phase: "active" } }); + expect(startupEvents(hub).at(0)).toMatchObject({ activity: { sessionId: "session-1", phase: "active" } }); runtimeResult.resolve(fake.runtime); await started; @@ -157,11 +157,42 @@ describe("PiSessionService session startup progress", () => { await service.start("/workspace"); - expect(startupEvents(hub).at(-1)).toMatchObject({ cwd: "/workspace", activity: { sessionId: "session-1", phase: "idle", label: "idle" } }); + expect(startupEvents(hub).at(-1)).toMatchObject({ activity: { sessionId: "session-1", phase: "idle", label: "idle" } }); expect(startupEvents(hub).at(-1)?.activity.detail).toBeUndefined(); await service.dispose(); }); + it("echoes a create's correlation token on every startup report of that construction", async () => { + const { hub, service } = startupService(); + + await service.start("/workspace", { startupToken: "pending-session-3-k2x9" }); + + // The token labels the browser row that is waiting, so it must ride every + // report of this construction, the closing idle one included. + expect(startupEvents(hub).map((event) => event.startupToken)).toEqual([ + "pending-session-3-k2x9", + "pending-session-3-k2x9", + "pending-session-3-k2x9", + ]); + // The token is an opaque throwaway label, never the session's identity. + expect(startupEvents(hub).map((event) => event.activity.sessionId)).toEqual(["session-1", "session-1", "session-1"]); + await service.dispose(); + }); + + it("publishes no correlation token when a create supplies none, and none for an open", async () => { + const created = startupService(); + await created.service.start("/workspace"); + const opened = startupService({ sessionRecords: [sessionRecord("session-1")] }); + await opened.service.status(sessionRef("session-1")); + + for (const hub of [created.hub, opened.hub]) { + expect(startupEvents(hub).length).toBeGreaterThan(0); + expect(startupEvents(hub).every((event) => event.startupToken === undefined)).toBe(true); + } + await created.service.dispose(); + await opened.service.dispose(); + }); + it("ends the startup window when the runtime construction itself fails", async () => { const failure = new Error("runtime unavailable"); const { hub, service } = startupService({ createAgentRuntime: () => Promise.reject(failure) }); diff --git a/src/server/sessions/piSessionService.ts b/src/server/sessions/piSessionService.ts index da3522b..8ccc346 100644 --- a/src/server/sessions/piSessionService.ts +++ b/src/server/sessions/piSessionService.ts @@ -199,6 +199,11 @@ type SessionCreationProvenance = "tracked-subsession"; interface StartSessionOptions { parentSession?: string; initialModel?: AgentModel; + /** + * Opaque label, echoed on this construction's startup progress so a browser + * row with no session id yet can recognise its own. + */ + startupToken?: string; } interface InternalStartSessionOptions extends StartSessionOptions { @@ -390,7 +395,7 @@ interface PendingSessionOpen { promise: Promise>; } -interface CreateSessionRuntimeOptions extends Pick { +interface CreateSessionRuntimeOptions extends Pick { notificationGeneration?: SessionNotificationGeneration; notifications?: "enabled" | "disabled"; /** @@ -992,6 +997,7 @@ export class PiSessionService implements SessionRouteService { cwd, { startupIntent: "create", + ...(options.startupToken === undefined ? {} : { startupToken: options.startupToken }), ...(options.initialModel === undefined ? {} : { initialModel: options.initialModel }), ...(options.creationProvenance === undefined ? {} : { creationProvenance: options.creationProvenance }), }, @@ -2353,7 +2359,7 @@ export class PiSessionService implements SessionRouteService { cwd: string, options: CreateSessionRuntimeOptions = {}, ): Promise> { - const startup = this.startupProgress(sessionManager, cwd, options.startupIntent ?? "open"); + const startup = this.startupProgress(sessionManager, options.startupIntent ?? "open", options.startupToken); try { return await this.createSessionRuntime(sessionManager, cwd, options, startup); } finally { @@ -2999,23 +3005,23 @@ export class PiSessionService implements SessionRouteService { /** * Build the reporter for one session construction. * - * The session id and cwd are both known before any await — a `SessionManager` - * has its id from construction — so the daemon can name what it is starting - * even though the `PiAgentSession` that {@link publishActivity} needs does not - * exist yet. When either is missing there is nothing honest to route on, so - * the reporter stays silent and the browser keeps its own generic wording. + * The session id is known before any await — a `SessionManager` has its id + * from construction — so the daemon can name what it is starting even though + * the `PiAgentSession` that {@link publishActivity} needs does not exist yet. + * Without an id there is nothing to report against, so the reporter stays + * silent and the browser keeps its own generic wording. */ - private startupProgress(sessionManager: PiSessionManager, cwd: string, intent: "create" | "open"): SessionStartupProgressReporter { + private startupProgress(sessionManager: PiSessionManager, intent: "create" | "open", startupToken: string | undefined): SessionStartupProgressReporter { const sessionId = sessionManager.getSessionId(); - if (sessionId === "" || cwd === "") return { report: noop, end: noop }; + if (sessionId === "") return { report: noop, end: noop }; const label = intent === "create" ? "Creating session" : "Opening session"; return { - report: (phase) => { this.publishStartupProgress(sessionId, cwd, label, "active", this.startupDetail(phase)); }, + report: (phase) => { this.publishStartupProgress(sessionId, startupToken, label, "active", this.startupDetail(phase)); }, end: () => { // A real activity published during the window (an extension error, say) // is the truth about this session and must survive the clear. if (this.activities.has(sessionId)) return; - this.publishStartupProgress(sessionId, cwd, "idle", "idle", undefined); + this.publishStartupProgress(sessionId, startupToken, "idle", "idle", undefined); }, }; } @@ -3027,17 +3033,17 @@ export class PiSessionService implements SessionRouteService { } /** - * Report startup progress on the global channel only, keyed by `cwd` so a - * browser row that has no session id yet can find it. + * Report startup progress on the global channel only, echoing the caller's + * correlation token so a waiting browser row recognises its own construction. * * Unlike {@link publishActivity} this deliberately records nothing: no * `activities` entry, no workspace activity, no unread observation. There is * no session to own that state, and a failed creation would leave it stranded. */ - private publishStartupProgress(sessionId: string, cwd: string, label: string, phase: "active" | "idle", detail: string | undefined): void { + private publishStartupProgress(sessionId: string, startupToken: string | undefined, label: string, phase: "active" | "idle", detail: string | undefined): void { const at = new Date().toISOString(); const activity = detail === undefined ? { sessionId, phase, label, at } : { sessionId, phase, label, detail, at }; - this.events.publishGlobal({ type: "session.startup", cwd, activity }); + this.events.publishGlobal(startupToken === undefined ? { type: "session.startup", activity } : { type: "session.startup", startupToken, activity }); } private publishActivity(session: PiAgentSession, label: string, phase: "active" | "idle" | "error", detail?: string): void { diff --git a/src/server/sessions/sessionRoutes.test.ts b/src/server/sessions/sessionRoutes.test.ts index caddfa9..e1e01aa 100644 --- a/src/server/sessions/sessionRoutes.test.ts +++ b/src/server/sessions/sessionRoutes.test.ts @@ -26,6 +26,7 @@ import { PiSessionService, type PiSessionManagerGateway } from "./piSessionServi import { testModelRuntime } from "./piSessionService.testSupport.js"; import { SessionNotificationStore } from "./sessionNotificationStore.js"; import type { SessionRouteLookup, SessionRouteService } from "./sessionService.js"; +import type { ClientSession } from "../types.js"; import { registerSessionRoutes } from "./sessionRoutes.js"; import type { NormalizedSessionCleanupRequest } from "./sessionCleanup.js"; @@ -666,6 +667,35 @@ describe("session routes", () => { } }); + it("forwards a create's optional correlation token alongside the normalized cwd", async () => { + const routeApp = Fastify({ logger: false }); + await routeApp.register(fastifyWebsocket); + const eventHub = new SessionEventHub(); + const routeService = new CapturingRouteSessionService(); + registerSessionRoutes(routeApp, routeService, eventHub); + + try { + const requestCwd = resolve("/repo"); + const withToken = await routeApp.inject({ method: "POST", url: "/sessions", payload: { cwd: requestCwd, startupToken: "pending-session-3-k2x9" } }); + const withoutToken = await routeApp.inject({ method: "POST", url: "/sessions", payload: { cwd: requestCwd } }); + // An older browser, or any non-browser caller, sends no token; and a + // malformed one must not reach the service as a label it would echo. + const malformedToken = await routeApp.inject({ method: "POST", url: "/sessions", payload: { cwd: requestCwd, startupToken: 7 } }); + + expect(withToken.statusCode).toBe(200); + expect(withoutToken.statusCode).toBe(200); + expect(malformedToken.statusCode).toBe(400); + expect(malformedToken.json()).toEqual({ error: "startupToken field must be a string" }); + expect(routeService.startCalls).toEqual([ + { cwd: requestCwd, startupToken: "pending-session-3-k2x9" }, + { cwd: requestCwd, startupToken: undefined }, + ]); + } finally { + await routeService.dispose(); + await routeApp.close(); + } + }); + it("rejects malformed bulk mutation bodies before calling the service", async () => { const routeApp = Fastify({ logger: false }); await routeApp.register(fastifyWebsocket); @@ -706,6 +736,7 @@ class CapturingRouteSessionService implements SessionRouteService { readonly bulkArchiveCalls: SessionBulkMutationRef[][] = []; readonly bulkDeleteCalls: SessionBulkMutationRef[][] = []; readonly navigateTreeCalls: { lookup: SessionRouteLookup; request: SessionTreeNavigateRequest }[] = []; + readonly startCalls: { cwd: string; startupToken: string | undefined }[] = []; reloadError: Error | undefined; clearQueueError: Error | undefined; @@ -772,7 +803,11 @@ class CapturingRouteSessionService implements SessionRouteService { } list(): never { throw unusedRouteMethod("list"); } - start(): never { throw unusedRouteMethod("start"); } + + start(cwd: string, options?: { startupToken?: string }): Promise { + this.startCalls.push({ cwd, startupToken: options?.startupToken }); + return Promise.resolve({ id: "session-1", path: "/tmp/session-1.jsonl", cwd, created: "2026-06-25T00:00:00.000Z", modified: "2026-06-25T00:00:00.000Z", messageCount: 0, firstMessage: "" }); + } dismissWarning(lookup: SessionRouteLookup, dismissId: string): Promise { this.dismissWarningCalls.push({ lookup, dismissId }); diff --git a/src/server/sessions/sessionRoutes.ts b/src/server/sessions/sessionRoutes.ts index da73425..783c411 100644 --- a/src/server/sessions/sessionRoutes.ts +++ b/src/server/sessions/sessionRoutes.ts @@ -45,10 +45,14 @@ export function registerSessionRoutes(app: FastifyInstance, sessions: SessionRou } }); - app.post<{ Body: { cwd?: unknown } | undefined }>(`${prefix}/sessions`, async (request, reply) => { + app.post<{ Body: { cwd?: unknown; startupToken?: unknown } | undefined }>(`${prefix}/sessions`, async (request, reply) => { try { const body = requireRecord(request.body); - return await sessions.start(normalizeRequestCwd(requireString(body, "cwd"))); + // An opaque label the caller uses to recognise its own construction's + // startup reports. Optional: only a browser row waiting for a session id + // has anything to correlate. + const startupToken = body["startupToken"] === undefined ? undefined : requireNonEmptyString(body, "startupToken"); + return await sessions.start(normalizeRequestCwd(requireString(body, "cwd")), optionalField("startupToken", startupToken)); } catch (error) { return reply.code(400).send({ error: errorMessage(error) }); } diff --git a/src/server/sessions/sessionService.ts b/src/server/sessions/sessionService.ts index 7e64414..844d01f 100644 --- a/src/server/sessions/sessionService.ts +++ b/src/server/sessions/sessionService.ts @@ -40,7 +40,12 @@ export type SessionRouteLookup = string | SessionRouteRef; */ export interface SessionRouteService { list(cwd: string): Promise; - start(cwd: string): Promise; + /** + * Create a session. `startupToken` is an opaque label the caller supplies so + * it can recognise this construction's startup progress reports; the service + * echoes it and never interprets it. + */ + start(cwd: string, options?: { startupToken?: string }): Promise; messages(ref: SessionRouteLookup, page?: { before?: number; limit?: number }): Promise; status(ref: SessionRouteLookup): Promise; streamSnapshot(ref: SessionRouteLookup): Promise; diff --git a/src/shared/apiTypes.ts b/src/shared/apiTypes.ts index e934fe4..308f15f 100644 --- a/src/shared/apiTypes.ts +++ b/src/shared/apiTypes.ts @@ -438,18 +438,18 @@ export interface QueuedSessionMessage { * constructing the agent session and no `PiAgentSession` exists yet, so * `activity.update` cannot be published for it. * - * `cwd` is the routing key for a browser row that is still waiting for a - * session id: a client-invented pending start knows its workspace path but not - * the daemon's session id. `activity.sessionId` carries the daemon's real id, so - * the same event also serves the case where the browser already knows it (an - * open of an existing session). + * `startupToken` is the opaque label a create request supplied, echoed back so a + * browser row still waiting for a session id recognises its own construction. + * The daemon never interprets it and it never becomes the session id: + * `activity.sessionId` always carries the real id, which is how an *open* of a + * session the browser already knows is routed instead. * * `activity.phase === "idle"` means the startup window ended with nothing left * to report, so a browser that substituted its own text should restore it. */ export interface SessionStartupProgressEvent { type: "session.startup"; - cwd: string; + startupToken?: string; activity: SessionActivity; } From 8a3fe0daa654c3d81eacf85b0e6d6c745b2812e4 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 21:19:46 +0200 Subject: [PATCH 02/17] docs(relay): add worktree-autodetect relay packet Assessment of auto-detecting worktrees created outside PI WEB. Worktree discovery is already derived per request with no cache and no registry, so the gap is that the browser never re-lists. Recommends the reduced scope: filter prunable worktrees, add a non-disruptive topology refresh, and call it from the existing browser-resume path. No watchers, timers, processes, or push channels. Packet is repo-only and outside the published files allowlist, so no changeset is needed. --- .pi-web/relays/worktree-autodetect/charter.md | 131 ++++++++++++ .pi-web/relays/worktree-autodetect/log.md | 197 ++++++++++++++++++ .pi-web/relays/worktree-autodetect/plan.md | 168 +++++++++++++++ .pi-web/relays/worktree-autodetect/status.md | 90 ++++++++ 4 files changed, 586 insertions(+) create mode 100644 .pi-web/relays/worktree-autodetect/charter.md create mode 100644 .pi-web/relays/worktree-autodetect/log.md create mode 100644 .pi-web/relays/worktree-autodetect/plan.md create mode 100644 .pi-web/relays/worktree-autodetect/status.md diff --git a/.pi-web/relays/worktree-autodetect/charter.md b/.pi-web/relays/worktree-autodetect/charter.md new file mode 100644 index 0000000..a722b09 --- /dev/null +++ b/.pi-web/relays/worktree-autodetect/charter.md @@ -0,0 +1,131 @@ +# 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 new file mode 100644 index 0000000..05baee3 --- /dev/null +++ b/.pi-web/relays/worktree-autodetect/log.md @@ -0,0 +1,197 @@ +# 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`. diff --git a/.pi-web/relays/worktree-autodetect/plan.md b/.pi-web/relays/worktree-autodetect/plan.md new file mode 100644 index 0000000..9dabddb --- /dev/null +++ b/.pi-web/relays/worktree-autodetect/plan.md @@ -0,0 +1,168 @@ +# 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 new file mode 100644 index 0000000..abea448 --- /dev/null +++ b/.pi-web/relays/worktree-autodetect/status.md @@ -0,0 +1,90 @@ +# Status — relay "worktree-autodetect" + +## ✅ 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. + +## Current position + +Leg 0 (assessment/design) is complete. No production code has been written. + +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`. + +## Leg tracking + +- **Last completed leg:** 0 (assessment and packet creation) +- **Next leg to run:** 1 + +## Next task — leg 1 + +Stop reporting worktrees whose checkout directory has been removed outside PI WEB. + +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`). + +Files: `src/server/workspaces/gitWorktreeDiscovery.ts`, `src/server/workspaces/workspaceService.ts`, +new `src/server/workspaces/gitWorktreeDiscovery.test.ts`. + +## Relevant context for the next runner + +Facts established in leg 0 — trust these, they were verified against the running code and +real git; do not re-derive them: + +- **Worktrees are already derived, never registered.** `WorkspaceService.list()` shells out + to `git worktree list --porcelain` on **every** `GET /projects/:projectId/workspaces` + request. There is no server-side cache and no invalidation to design. A worktree created + outside PI WEB is *already* discovered — the gap is purely that the browser never re-asks. +- **`projects.json` holds projects only, not workspaces.** So "auto-adoption" is a + non-problem: nothing needs to be written to a registry, and there is no adopt-vs-visible + distinction to design. This collapsed most of the feature's apparent complexity. +- **Cost of discovery is ~2ms** (measured: 20 sequential `git worktree list --porcelain` + runs in 42ms on this repo). +- **`prunable` is real and load-bearing.** After `rm -rf`ing a worktree directory without + `git worktree remove`, `git worktree list --porcelain` still lists it, with an added + `prunable gitdir file points to non-existent location` line. PI WEB currently shows this + as a normal selectable workspace. `locked` appears as a bare valueless line. +- **The resume path already exists and is already debounced.** + `src/client/src/appShell/browserResumeController.ts` listens to window `focus` and + document `visibilitychange`, batches per animation frame, and collapses concurrent + requests via `TrailingRefreshCoordinator`. It calls + `PiWebApp.refreshAfterBrowserResume()` (~line 432), which already refreshes the selected + session, machine activities, and workspace-deletion runs. Workspace topology is the one + thing missing from that list. +- **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. +- **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 + selection is still valid. `ProjectActivityOwnershipCoordinator` is the existing precedent + for background topology hydration that does not disturb selection — read it if leg 2 + needs a model. +- **No session daemon involvement.** Nothing in this design touches `src/server/sessiond.ts`, + session runtime ownership, or the daemon protocol. **No manual sessiond restart needed.** + +## Progress documentation expected of each runner + +- Commit the slice (Conventional Commit message). +- Update this file: current position, leg tracking, next task, blockers. +- Append to `log.md`: what, why, artifacts, exact checks run and results. +- Add the `.changeset/*.md` fragment no later than leg 3. + +## Decisions settled by the human (do not re-open) + +1. **Latency: resume-scoped is acceptable.** Detection on tab refocus/visibility is the + agreed behavior. Do not add a timer or a watcher to shorten it. +2. **Removed worktrees: hide them.** Filtering `prunable` worktrees out of the workspace + list is wanted and approved. +3. **Sibling overlap: assume the other session does nothing.** This relay owns the + workspace-topology refresh seam outright. Build it here, do not design for sharing, + and do not read `/srv/dev/pi-web-worktrees/worktree-create-ui`. If that branch later + merges something overlapping, resolving it is that branch's problem, not this one's. + +## Blockers + +None. Leg 1 is clear to run. From 4eca5fe6f2242011720d10cd8b41951a0e92a806 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 21:24:08 +0200 Subject: [PATCH 03/17] fix(workspaces): hide worktrees whose checkout directory is gone git worktree list keeps reporting a linked worktree after its directory is deleted outside PI WEB, marking it prunable. The workspace list showed those as normal selectable workspaces. The porcelain parser now reads the prunable and locked keys, and WorkspaceService filters prunable linked worktrees out while always keeping the project's own path. Listing stays read-only; git worktree prune is never run. --- .../workspaces/gitWorktreeDiscovery.test.ts | 60 ++++++++++++++ src/server/workspaces/gitWorktreeDiscovery.ts | 14 ++++ .../workspaces/workspaceService.test.ts | 80 +++++++++++++++++++ src/server/workspaces/workspaceService.ts | 26 +++++- 4 files changed, 177 insertions(+), 3 deletions(-) create mode 100644 src/server/workspaces/gitWorktreeDiscovery.test.ts create mode 100644 src/server/workspaces/workspaceService.test.ts diff --git a/src/server/workspaces/gitWorktreeDiscovery.test.ts b/src/server/workspaces/gitWorktreeDiscovery.test.ts new file mode 100644 index 0000000..6feb326 --- /dev/null +++ b/src/server/workspaces/gitWorktreeDiscovery.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it } from "vitest"; +import { parseGitWorktreeList } from "./gitWorktreeDiscovery.js"; + +// Fixtures below are verbatim `git worktree list --porcelain` output captured from real git. +const mainAndLinked = [ + "worktree /repo", + "HEAD ad580ab86e1eba35a121fa6b9e8be1126aaf18de", + "branch refs/heads/main", + "", + "worktree /repo-worktrees/feature", + "HEAD ad580ab86e1eba35a121fa6b9e8be1126aaf18de", + "branch refs/heads/feat/thing", + "", +].join("\n"); + +const removedAndLocked = [ + "worktree /repo", + "HEAD ad580ab86e1eba35a121fa6b9e8be1126aaf18de", + "branch refs/heads/main", + "", + "worktree /repo-worktrees/gone", + "HEAD ad580ab86e1eba35a121fa6b9e8be1126aaf18de", + "branch refs/heads/gone", + "prunable gitdir file points to non-existent location", + "", + "worktree /repo-worktrees/kept", + "HEAD ad580ab86e1eba35a121fa6b9e8be1126aaf18de", + "branch refs/heads/kept", + "locked keep me", + "", +].join("\n"); + +describe("parseGitWorktreeList", () => { + it("reads paths and short branch names for the main and linked worktrees", () => { + expect(parseGitWorktreeList(mainAndLinked)).toEqual([ + { path: "/repo", branch: "main" }, + { path: "/repo-worktrees/feature", branch: "feat/thing" }, + ]); + }); + + it("reports prunable with its reason and locked with or without a reason", () => { + expect(parseGitWorktreeList(removedAndLocked)).toEqual([ + { path: "/repo", branch: "main" }, + { path: "/repo-worktrees/gone", branch: "gone", prunable: true }, + { path: "/repo-worktrees/kept", branch: "kept", locked: true }, + ]); + + const bareLocked = ["worktree /repo-worktrees/kept", "HEAD abc", "detached", "locked", ""].join("\n"); + expect(parseGitWorktreeList(bareLocked)).toEqual([{ path: "/repo-worktrees/kept", detached: true, locked: true }]); + }); + + it("reads bare repositories and ignores chunks without a worktree path", () => { + const bare = ["worktree /repo.git", "bare", "", "HEAD abc", ""].join("\n"); + expect(parseGitWorktreeList(bare)).toEqual([{ path: "/repo.git", bare: true }]); + }); + + it("returns nothing for empty output", () => { + expect(parseGitWorktreeList("\n")).toEqual([]); + }); +}); diff --git a/src/server/workspaces/gitWorktreeDiscovery.ts b/src/server/workspaces/gitWorktreeDiscovery.ts index f4127bf..e20a656 100644 --- a/src/server/workspaces/gitWorktreeDiscovery.ts +++ b/src/server/workspaces/gitWorktreeDiscovery.ts @@ -9,6 +9,10 @@ export interface GitWorktreeInfo { branch?: string; bare?: boolean; detached?: boolean; + /** Git reports a linked worktree as prunable when its checkout directory no longer exists. */ + prunable?: boolean; + /** Git reports a locked worktree with a bare `locked` line, optionally followed by a reason. */ + locked?: boolean; } export async function isGitRepository(path: string): Promise { @@ -22,6 +26,14 @@ export async function isGitRepository(path: string): Promise { export async function discoverGitWorktrees(path: string): Promise { const { stdout } = await execFileAsync("git", ["-C", path, "worktree", "list", "--porcelain"], { env: sanitizedGitEnv() }); + return parseGitWorktreeList(stdout); +} + +/** + * Parses `git worktree list --porcelain` output into facts only. Deciding which worktrees a + * project should show (for example hiding prunable ones) is workspace policy, not parsing. + */ +export function parseGitWorktreeList(stdout: string): GitWorktreeInfo[] { const chunks = stdout.trim().split(/\n\s*\n/).filter(Boolean); return chunks.map((chunk) => { @@ -33,6 +45,8 @@ export async function discoverGitWorktrees(path: string): Promise w.path); diff --git a/src/server/workspaces/workspaceService.test.ts b/src/server/workspaces/workspaceService.test.ts new file mode 100644 index 0000000..d3b9b49 --- /dev/null +++ b/src/server/workspaces/workspaceService.test.ts @@ -0,0 +1,80 @@ +import { describe, expect, it } from "vitest"; +import type { Project } from "../types.js"; +import type { GitWorktreeInfo } from "./gitWorktreeDiscovery.js"; +import { WorkspaceService, type WorkspaceGitPort } from "./workspaceService.js"; + +const project: Project = { + id: "p1", + name: "Project", + path: "/repo", + createdAt: "2026-05-25T00:00:00.000Z", +}; + +function serviceFor(worktrees: GitWorktreeInfo[], isGitRepo = true): WorkspaceService { + const git: WorkspaceGitPort = { + isGitRepository: () => Promise.resolve(isGitRepo), + discoverGitWorktrees: () => Promise.resolve(worktrees), + }; + return new WorkspaceService(git); +} + +describe("WorkspaceService.list", () => { + it("hides a linked worktree whose checkout directory was removed outside PI WEB", async () => { + const service = serviceFor([ + { path: "/repo", branch: "main" }, + { path: "/repo-worktrees/gone", branch: "gone", prunable: true }, + { path: "/repo-worktrees/live", branch: "live" }, + ]); + + const workspaces = await service.list(project); + + expect(workspaces.map((workspace) => workspace.path)).toEqual(["/repo", "/repo-worktrees/live"]); + }); + + it("keeps a locked worktree, which is still a real checkout", async () => { + const service = serviceFor([ + { path: "/repo", branch: "main" }, + { path: "/repo-worktrees/kept", branch: "kept", locked: true }, + ]); + + const workspaces = await service.list(project); + + expect(workspaces.map((workspace) => workspace.path)).toEqual(["/repo", "/repo-worktrees/kept"]); + }); + + it("keeps the project's own worktree even if git marks it prunable, so a project is never empty", async () => { + const service = serviceFor([{ path: "/repo", branch: "main", prunable: true }]); + + const workspaces = await service.list(project); + + expect(workspaces).toEqual([expect.objectContaining({ path: "/repo", label: "main", isMain: true, isGitWorktree: true })]); + }); + + it("falls back to the project itself when every linked worktree is filtered away", async () => { + const service = serviceFor([{ path: "/repo-worktrees/gone", branch: "gone", prunable: true }]); + + const workspaces = await service.list(project); + + expect(workspaces).toEqual([expect.objectContaining({ path: "/repo", label: "Project", isMain: true, isGitRepo: true, isGitWorktree: false })]); + }); + + it("labels detached and unnamed worktrees without inventing a branch", async () => { + const service = serviceFor([ + { path: "/repo", branch: "main" }, + { path: "/repo-worktrees/detached", detached: true }, + ]); + + const workspaces = await service.list(project); + + expect(workspaces.map((workspace) => ({ label: workspace.label, branch: workspace.branch }))).toEqual([ + { label: "main", branch: "main" }, + { label: "detached", branch: undefined }, + ]); + }); + + it("returns a single non-git workspace when the project is not a repository", async () => { + const service = serviceFor([], false); + + expect(await service.list(project)).toEqual([expect.objectContaining({ path: "/repo", isGitRepo: false, isGitWorktree: false })]); + }); +}); diff --git a/src/server/workspaces/workspaceService.ts b/src/server/workspaces/workspaceService.ts index 786fa6f..8bb98d9 100644 --- a/src/server/workspaces/workspaceService.ts +++ b/src/server/workspaces/workspaceService.ts @@ -1,18 +1,28 @@ import { createHash } from "node:crypto"; import type { Project } from "../types.js"; import type { Workspace } from "../types.js"; -import { discoverGitWorktrees, isGitRepository } from "./gitWorktreeDiscovery.js"; +import { discoverGitWorktrees, isGitRepository, type GitWorktreeInfo } from "./gitWorktreeDiscovery.js"; const idFor = (value: string) => createHash("sha1").update(value).digest("hex").slice(0, 12); +/** The git facts this service needs, injectable so workspace policy is testable without a real repo. */ +export interface WorkspaceGitPort { + isGitRepository(path: string): Promise; + discoverGitWorktrees(path: string): Promise; +} + +const realGit: WorkspaceGitPort = { isGitRepository, discoverGitWorktrees }; + export class WorkspaceService { + constructor(private readonly git: WorkspaceGitPort = realGit) {} + async list(project: Project): Promise { - const isGitRepo = await isGitRepository(project.path); + const isGitRepo = await this.git.isGitRepository(project.path); if (!isGitRepo) { return [this.single(project, false)]; } - const worktrees = await discoverGitWorktrees(project.path); + const worktrees = this.selectable(await this.git.discoverGitWorktrees(project.path), project); if (worktrees.length === 0) return [this.single(project, true)]; return worktrees.map((worktree) => { @@ -30,6 +40,16 @@ export class WorkspaceService { }); } + /** + * Git keeps listing a linked worktree after its checkout directory is deleted outside PI WEB, + * marking it `prunable`. Such an entry is not a usable workspace, so it is hidden rather than + * offered as a selectable ghost. Listing stays read-only: we never run `git worktree prune`. + * The project's own path is always kept so a project cannot end up with no workspace at all. + */ + private selectable(worktrees: GitWorktreeInfo[], project: Project): GitWorktreeInfo[] { + return worktrees.filter((worktree) => worktree.prunable !== true || worktree.path === project.path); + } + private single(project: Project, isGitRepo: boolean): Workspace { return { id: idFor(`${project.id}:${project.path}`), From e12e5d16c671e639509a8b5dc6f9f8600585b500 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 21:25:15 +0200 Subject: [PATCH 04/17] 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. From 213ec104ea4d4591feb633655369ea3d5bee8cc6 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 21:29:45 +0200 Subject: [PATCH 05/17] feat(workspaces): add non-disruptive workspace topology refresh Adds WorkspaceController.refreshSelectedProjectTopology(), which re-lists the selected project's workspaces and applies them through applyProjectWorkspaces only. It never routes through selectWorkspace, which lacks an already-selected guard and would clear the active session and all workspace-scoped state. Stale responses for a project or machine the user has since left are discarded, and failures go to an injectable background error sink instead of state.error. --- .../controllers/workspaceController.test.ts | 231 ++++++++++++++++++ .../src/controllers/workspaceController.ts | 31 +++ 2 files changed, 262 insertions(+) create mode 100644 src/client/src/controllers/workspaceController.test.ts diff --git a/src/client/src/controllers/workspaceController.test.ts b/src/client/src/controllers/workspaceController.test.ts new file mode 100644 index 0000000..ea92d88 --- /dev/null +++ b/src/client/src/controllers/workspaceController.test.ts @@ -0,0 +1,231 @@ +import { describe, expect, it, vi } from "vitest"; +import type { AppState } from "../appState"; +import { initialAppState } from "../appState"; +import type { Machine, Project, SessionInfo, Workspace } from "../api"; +import type { SessionController } from "./sessionController"; +import { WorkspaceController } from "./workspaceController"; + +function machine(id: string): Machine { + return { id, name: id, kind: id === "local" ? "local" : "remote", createdAt: "now", updatedAt: "now" }; +} + +function project(id: string, path: string): Project { + return { id, name: id, path, createdAt: "now" }; +} + +function workspace(projectId: string, path: string, options: { isMain?: boolean } = {}): Workspace { + return { id: path, projectId, path, label: path, isMain: options.isMain ?? false, isGitRepo: true, isGitWorktree: true }; +} + +function session(cwd: string, id = "s1"): SessionInfo { + return { id, cwd, path: `${cwd}/.sessions/${id}`, created: "now", modified: "now", messageCount: 1, firstMessage: "hello" }; +} + +type LoadWorkspaces = (projectId: string, machineId?: string) => Promise; + +interface Harness { + controller: WorkspaceController; + state: () => AppState; + clearActiveSession: ReturnType; + updateUrl: ReturnType; + backgroundErrors: { message: string; error: unknown }[]; + setState: (patch: Partial) => void; +} + +function harness(initial: Partial, loadWorkspaces: LoadWorkspaces): Harness { + let state: AppState = { ...initialAppState(), ...initial }; + const setState = (patch: Partial) => { state = { ...state, ...patch }; }; + const clearActiveSession = vi.fn(); + const sessions: Pick = { + clearActiveSession, + preferredSession: vi.fn(), + selectSession: vi.fn(), + }; + const updateUrl = vi.fn(); + const backgroundErrors: { message: string; error: unknown }[] = []; + const controller = new WorkspaceController( + () => state, + setState, + updateUrl, + sessions, + undefined, + { + api: { workspaces: loadWorkspaces, sessions: vi.fn<(path: string, machineId?: string) => Promise>().mockResolvedValue([]) }, + onBackgroundError: (message, error) => { backgroundErrors.push({ message, error }); }, + }, + ); + return { controller, state: () => state, clearActiveSession, updateUrl, backgroundErrors, setState }; +} + +describe("WorkspaceController.refreshSelectedProjectTopology", () => { + it("surfaces a worktree created outside PI WEB in both the selected list and the per-project cache", async () => { + const repo = project("p1", "/repo"); + const main = workspace(repo.id, repo.path, { isMain: true }); + const created = workspace(repo.id, "/repo-feature"); + const loadWorkspaces = vi.fn().mockResolvedValue([main, created]); + const test = harness( + { + selectedMachine: machine("local"), + projects: [repo], + selectedProject: repo, + selectedWorkspace: main, + workspaces: [main], + workspacesByProjectId: { [repo.id]: [main] }, + }, + loadWorkspaces, + ); + + await test.controller.refreshSelectedProjectTopology(); + + expect(loadWorkspaces).toHaveBeenCalledWith(repo.id, "local"); + expect(test.state().workspaces).toEqual([main, created]); + expect(test.state().workspacesByProjectId[repo.id]).toEqual([main, created]); + }); + + it("preserves the selection and workspace-scoped state when the selected workspace still exists", async () => { + const repo = project("p1", "/repo"); + const main = workspace(repo.id, repo.path, { isMain: true }); + const selected = workspace(repo.id, "/repo-feature"); + const loadWorkspaces = vi.fn().mockResolvedValue([main, selected, workspace(repo.id, "/repo-other")]); + const test = harness( + { + selectedMachine: machine("local"), + projects: [repo], + selectedProject: repo, + selectedWorkspace: selected, + workspaces: [main, selected], + workspacesByProjectId: { [repo.id]: [main, selected] }, + selectedSession: session(selected.path), + sessions: [session(selected.path)], + selectedFilePath: "src/index.ts", + expandedDirs: { src: [] }, + selectedTerminalId: "t1", + }, + loadWorkspaces, + ); + const before = test.state(); + + await test.controller.refreshSelectedProjectTopology(); + + const after = test.state(); + expect(after.selectedWorkspace).toBe(selected); + expect(after.selectedSession).toBe(before.selectedSession); + expect(after.sessions).toBe(before.sessions); + expect(after.selectedFilePath).toBe("src/index.ts"); + expect(after.expandedDirs).toBe(before.expandedDirs); + expect(after.selectedTerminalId).toBe("t1"); + expect(test.clearActiveSession).not.toHaveBeenCalled(); + expect(test.updateUrl).not.toHaveBeenCalled(); + }); + + it("leaves the selection alone when the selected workspace disappeared", async () => { + const repo = project("p1", "/repo"); + const main = workspace(repo.id, repo.path, { isMain: true }); + const removed = workspace(repo.id, "/repo-gone"); + const loadWorkspaces = vi.fn().mockResolvedValue([main]); + const test = harness( + { + selectedMachine: machine("local"), + projects: [repo], + selectedProject: repo, + selectedWorkspace: removed, + workspaces: [main, removed], + workspacesByProjectId: { [repo.id]: [main, removed] }, + }, + loadWorkspaces, + ); + + await test.controller.refreshSelectedProjectTopology(); + + expect(test.state().selectedWorkspace).toBe(removed); + expect(test.state().workspaces).toEqual([main]); + expect(test.clearActiveSession).not.toHaveBeenCalled(); + }); + + it("discards a response for a project the user has since left", async () => { + const repo = project("p1", "/repo"); + const other = project("p2", "/other"); + const main = workspace(repo.id, repo.path, { isMain: true }); + const created = workspace(repo.id, "/repo-feature"); + let resolveWorkspaces: ((workspaces: Workspace[]) => void) | undefined; + const loadWorkspaces = vi.fn().mockReturnValue(new Promise((resolve) => { resolveWorkspaces = resolve; })); + const test = harness( + { + selectedMachine: machine("local"), + projects: [repo, other], + selectedProject: repo, + selectedWorkspace: main, + workspaces: [main], + workspacesByProjectId: { [repo.id]: [main] }, + }, + loadWorkspaces, + ); + + const pending = test.controller.refreshSelectedProjectTopology(); + test.setState({ selectedProject: other, selectedWorkspace: undefined, workspaces: [] }); + resolveWorkspaces?.([main, created]); + await pending; + + expect(test.state().workspaces).toEqual([]); + expect(test.state().workspacesByProjectId[repo.id]).toEqual([main]); + }); + + it("discards a response after the selected machine changed", async () => { + const repo = project("p1", "/repo"); + const main = workspace(repo.id, repo.path, { isMain: true }); + let resolveWorkspaces: ((workspaces: Workspace[]) => void) | undefined; + const loadWorkspaces = vi.fn().mockReturnValue(new Promise((resolve) => { resolveWorkspaces = resolve; })); + const test = harness( + { + selectedMachine: machine("local"), + projects: [repo], + selectedProject: repo, + selectedWorkspace: main, + workspaces: [main], + workspacesByProjectId: { [repo.id]: [main] }, + }, + loadWorkspaces, + ); + + const pending = test.controller.refreshSelectedProjectTopology(); + test.setState({ selectedMachine: machine("remote") }); + resolveWorkspaces?.([main, workspace(repo.id, "/repo-feature")]); + await pending; + + expect(test.state().workspaces).toEqual([main]); + expect(test.state().workspacesByProjectId[repo.id]).toEqual([main]); + }); + + it("reports a failed refresh to the background error sink without painting an error banner", async () => { + const repo = project("p1", "/repo"); + const main = workspace(repo.id, repo.path, { isMain: true }); + const failure = new Error("git worktree list failed"); + const loadWorkspaces = vi.fn().mockRejectedValue(failure); + const test = harness( + { + selectedMachine: machine("local"), + projects: [repo], + selectedProject: repo, + selectedWorkspace: main, + workspaces: [main], + workspacesByProjectId: { [repo.id]: [main] }, + }, + loadWorkspaces, + ); + + await test.controller.refreshSelectedProjectTopology(); + + expect(test.state().error).toBe(""); + expect(test.state().workspaces).toEqual([main]); + expect(test.backgroundErrors).toEqual([{ message: `Failed to refresh workspaces for project ${repo.id} on local`, error: failure }]); + }); + + it("does not request anything when no project is selected", async () => { + const loadWorkspaces = vi.fn(); + const test = harness({ selectedMachine: machine("local") }, loadWorkspaces); + + await test.controller.refreshSelectedProjectTopology(); + + expect(loadWorkspaces).not.toHaveBeenCalled(); + }); +}); diff --git a/src/client/src/controllers/workspaceController.ts b/src/client/src/controllers/workspaceController.ts index 35f422a..581da29 100644 --- a/src/client/src/controllers/workspaceController.ts +++ b/src/client/src/controllers/workspaceController.ts @@ -8,10 +8,12 @@ import { InMemoryWorkspaceSelectionMemory, selectPreferredWorkspace, type Worksp export interface WorkspaceControllerDependencies { api?: Pick; + onBackgroundError?: (message: string, error: unknown) => void; } export class WorkspaceController { private readonly api: Pick; + private readonly onBackgroundError: (message: string, error: unknown) => void; constructor( private readonly getState: GetState, @@ -22,6 +24,7 @@ export class WorkspaceController { deps: WorkspaceControllerDependencies = {}, ) { this.api = deps.api ?? defaultApi; + this.onBackgroundError = deps.onBackgroundError ?? ((message, error) => { console.warn(message, error); }); } clearSelection(options?: { updateUrl?: boolean | undefined }) { @@ -78,6 +81,34 @@ export class WorkspaceController { return workspaces; } + /** + * Re-lists the selected project's workspaces so worktrees created or removed outside + * PI WEB become visible, without disturbing the current selection. + * + * Deliberately never routes through `selectWorkspace`: that has no already-selected + * guard, so re-picking the same workspace would still call `clearActiveSession()` and + * `resetWorkspaceScopedState()`, closing the session socket and blanking chat, file + * tree, git status, and terminal selection. Callers run this on every browser resume, + * so applying the list through `applyProjectWorkspaces` alone is the invariant. + * + * If the selected workspace disappeared, the selection is left alone: the user is + * working there and the existing deletion path owns recovery. + */ + async refreshSelectedProjectTopology(): Promise { + const state = this.getState(); + const project = state.selectedProject; + if (project === undefined) return; + const machineId = selectedMachineId(state); + try { + const workspaces = await this.api.workspaces(project.id, machineId); + const current = this.getState(); + if (selectedMachineId(current) !== machineId || current.selectedProject?.id !== project.id) return; + this.applyProjectWorkspaces(project.id, workspaces); + } catch (error) { + this.onBackgroundError(`Failed to refresh workspaces for project ${project.id} on ${machineId}`, error); + } + } + async refreshAfterWorkspaceDeleted(projectId: string, workspaceId: string): Promise { const workspaces = await this.refreshProjectWorkspaces(projectId); const state = this.getState(); From 1970ba14bc380878067cd7ba54c977b20d479b79 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 21:31:09 +0200 Subject: [PATCH 06/17] docs(relay): record worktree-autodetect leg 2 --- .pi-web/relays/worktree-autodetect/log.md | 86 ++++++++++++++++++++ .pi-web/relays/worktree-autodetect/status.md | 65 ++++++++++----- 2 files changed, 129 insertions(+), 22 deletions(-) diff --git a/.pi-web/relays/worktree-autodetect/log.md b/.pi-web/relays/worktree-autodetect/log.md index 2f5131f..0e629ce 100644 --- a/.pi-web/relays/worktree-autodetect/log.md +++ b/.pi-web/relays/worktree-autodetect/log.md @@ -273,3 +273,89 @@ None. Nothing ambiguous, no design decision needed, no intervention trigger fire ### 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`. diff --git a/.pi-web/relays/worktree-autodetect/status.md b/.pi-web/relays/worktree-autodetect/status.md index b7f2f3f..ced8027 100644 --- a/.pi-web/relays/worktree-autodetect/status.md +++ b/.pi-web/relays/worktree-autodetect/status.md @@ -3,39 +3,40 @@ ## ✅ 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 2. +no outstanding decisions. Run leg 3 — the final leg. ## Current position -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. +Legs 1 and 2 are complete and committed (`266f941`, `d0f8f9f`). The server hides prunable +worktrees, and the client now has a proven-inert refresh method. **Nothing calls it yet**, +so newly created worktrees still only appear on a full page load. Leg 3 wires the trigger +and is the last leg. 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:** 1 (server: hide removed worktrees) -- **Next leg to run:** 2 +- **Last completed leg:** 2 (client: non-disruptive topology refresh method) +- **Next leg to run:** 3 (final) -## Next task — leg 2 +## Next task — leg 3 -Non-disruptive workspace topology refresh in the client. +Wire the refresh to the existing resume path, document it, add the changeset. -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 })`. +See `plan.md` → "Leg 3". Summary: call `this.workspaces.refreshSelectedProjectTopology()` +from `PiWebApp.refreshAfterBrowserResume` (~line 432) and `refreshAppData` (~line 485) — +touch only those two methods in that 2300-line file. Optionally also on +`connectRealtime`'s `onReconnect`, only if it costs nothing. Then one short honest doc +paragraph under `docs/` (detection is resume-scoped, not instant; do not grow `README.md`), +and the `.changeset/*.md` fragment — **still missing, and leg 3 owns it**. -Files: `src/client/src/controllers/workspaceController.ts`, -new `src/client/src/controllers/workspaceController.test.ts`. +Finish with `npm run verify` (charter requires green) — this is the final leg. -Do **not** wire anything into `PiWebApp` in leg 2 — that is leg 3, deliberately after the -refresh is proven inert. +Signature detail leg 3 needs: `refreshSelectedProjectTopology()` takes no arguments, +returns `Promise`, never rejects (failures go to the injected background error sink, +defaulting to `console.warn`), and no-ops when no project is selected. So it can be dropped +directly into the existing `Promise.all` without a `.catch`. ## Relevant context for the next runner @@ -65,6 +66,20 @@ 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 2's method is inert by construction and covered.** `refreshSelectedProjectTopology()` + applies results via `applyProjectWorkspaces` only, so it writes at most `workspaces` and + `workspacesByProjectId`. Seven tests in + `src/client/src/controllers/workspaceController.test.ts` cover: new worktree appears, + selection + session + file tree + terminal state preserved and `clearActiveSession` not + called, selected workspace disappeared (selection deliberately left alone), stale project + response discarded, stale machine response discarded, rejection reported to the error sink + without touching `state.error`, and no-project no-op. Leg 2 mutation-tested this: injecting + the plausible `selectPreferredWorkspace` + `selectWorkspace` version made 2 tests fail, so + the guard is real and not vacuous. Leg 3 must not weaken these tests to fit its wiring. +- **`WorkspaceControllerDependencies` gained `onBackgroundError`** (optional, defaults to + `console.warn`). `PiWebApp` constructs `WorkspaceController`; leg 3 may pass a message- + prefixing sink to match sibling controllers, but the default is already correct — this is + not required work. - **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 @@ -101,6 +116,12 @@ real git; do not re-derive them: ## Blockers -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. +None. Leg 2's main known risk (UI churn on refresh) did **not** materialize: applying the +list through `applyProjectWorkspaces` alone required no workaround, and +`handleWorkspaceChange` early-returns on an unchanged selected workspace id, so a +fresh-but-equal list causes no downstream churn. No intervention signal fired. + +Leg 3's own watch item: after wiring, confirm nothing in the resume path re-derives +selection from the refreshed list. If wiring turns out to need a change *inside* +`refreshSelectedProjectTopology` to stay non-disruptive, that is the intervention signal — +stop rather than relaxing the invariant. From 8a24a7c4a5f10408971e761d1a168abcfeae4d76 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 21:40:54 +0200 Subject: [PATCH 07/17] feat(workspaces): refresh worktrees on browser resume Call WorkspaceController.refreshSelectedProjectTopology() from the existing browser-resume refresh and the plugin-facing refreshAppData path, so worktrees created or removed outside PI WEB become visible with no user action. No new timer, watcher, process, or push channel; the resume path is already debounced per animation frame and collapses concurrent requests. Document the resume-scoped detection and the hiding of gone checkouts in the FAQ, and add the changeset for the user-visible behavior. --- .changeset/worktree-autodetect.md | 5 + docs/faq.html | 20 ++++ src/client/src/components/PiWebApp.ts | 2 + .../PiWebApp.workspaceTopology.test.ts | 105 ++++++++++++++++++ 4 files changed, 132 insertions(+) create mode 100644 .changeset/worktree-autodetect.md create mode 100644 src/client/src/components/PiWebApp.workspaceTopology.test.ts diff --git a/.changeset/worktree-autodetect.md b/.changeset/worktree-autodetect.md new file mode 100644 index 0000000..2054a09 --- /dev/null +++ b/.changeset/worktree-autodetect.md @@ -0,0 +1,5 @@ +--- +"@jmfederico/pi-web": patch +--- + +Pick up git worktrees created or removed outside PI WEB without any user action. The selected project's workspace list is re-read whenever the browser tab regains focus or becomes visible, on local and remote machines, keeping the current workspace, session, and scroll position untouched. Worktrees whose checkout directory no longer exists are hidden instead of being offered as selectable workspaces. diff --git a/docs/faq.html b/docs/faq.html index 1feca7c..60c14d4 100644 --- a/docs/faq.html +++ b/docs/faq.html @@ -101,6 +101,7 @@ How do remote machines work? Laptop or server? Can I use local plugins? + A worktree I created is missing Sessions stop unexpectedly Where are logs? @@ -293,6 +294,25 @@

Read the plugin guide →

+
+

A worktree I created or deleted outside PI WEB is missing or still listed

+

+ PI WEB does not register worktrees. It lists the git worktrees of the selected project on demand, so + worktrees you create or delete with git worktree, a terminal, or another tool are picked up + without any adopt or import step. This works the same way on remote machines. +

+

+ The workspace list is re-read when the PI WEB tab regains focus or becomes visible again, not + continuously. If a worktree appeared while you were already looking at PI WEB, switch to another window + or tab and back, and the list updates. Your selected workspace, session, and scroll position are kept. +

+

+ Worktrees whose checkout directory no longer exists are hidden, so a directory you removed with + rm -rf instead of git worktree remove stops appearing as a selectable + workspace. Git still tracks it until you run git worktree prune. +

+
+

Sessions stop unexpectedly

diff --git a/src/client/src/components/PiWebApp.ts b/src/client/src/components/PiWebApp.ts index 2581a2e..e12cc72 100644 --- a/src/client/src/components/PiWebApp.ts +++ b/src/client/src/components/PiWebApp.ts @@ -435,6 +435,7 @@ export class PiWebApp extends LitElement { this.sessions.refreshSelectedSession(), this.refreshMachineActivities(), this.refreshWorkspaceDeletionRuns(), + this.workspaces.refreshSelectedProjectTopology(), ]); } @@ -492,6 +493,7 @@ export class PiWebApp extends LitElement { this.loadClientConfig(), this.refreshWorkspaceDeletionRuns(), this.refreshCurrentWorkspaceSurface(), + this.workspaces.refreshSelectedProjectTopology(), ]); this.schedulePiWebStatusRefresh(); } finally { diff --git a/src/client/src/components/PiWebApp.workspaceTopology.test.ts b/src/client/src/components/PiWebApp.workspaceTopology.test.ts new file mode 100644 index 0000000..8419fa1 --- /dev/null +++ b/src/client/src/components/PiWebApp.workspaceTopology.test.ts @@ -0,0 +1,105 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { WorkspaceController } from "../controllers/workspaceController"; +import { PiWebApp } from "./PiWebApp"; + +type RefreshCallback = () => void | Promise; + +afterEach(() => { + vi.restoreAllMocks(); + vi.unstubAllGlobals(); +}); + +describe("PiWebApp workspace topology refresh wiring", () => { + it("re-lists the selected project's workspaces on the browser-resume refresh", async () => { + const app = createApp(); + stubBackgroundRefreshes(app); + const refreshTopology = spyOnTopologyRefresh(app); + + await browserResumeRefresh(app)(); + + expect(refreshTopology).toHaveBeenCalledOnce(); + }); + + it("re-lists the selected project's workspaces on the plugin-facing app-data refresh", async () => { + const app = createApp(); + stubBackgroundRefreshes(app); + const refreshTopology = spyOnTopologyRefresh(app); + + await refreshAppData(app); + + expect(refreshTopology).toHaveBeenCalledOnce(); + }); + + it("still re-lists workspaces when a sibling refresh in the same resume batch fails", async () => { + const app = createApp(); + stubBackgroundRefreshes(app); + failBackgroundRefresh(app, "refreshMachineActivities", new Error("machine activity unavailable")); + const refreshTopology = spyOnTopologyRefresh(app); + + await expect(browserResumeRefresh(app)()).rejects.toThrow("machine activity unavailable"); + expect(refreshTopology).toHaveBeenCalledOnce(); + }); +}); + +function createApp(): PiWebApp { + const storage = { + getItem: () => null, + setItem: () => undefined, + removeItem: () => undefined, + }; + vi.stubGlobal("window", { location: { search: "" }, localStorage: storage }); + return new PiWebApp(); +} + +/** + * Replaces the sibling refreshes that already have their own coverage so this test + * observes only whether the resume/app-data paths include workspace topology. + */ +function stubBackgroundRefreshes(app: PiWebApp): void { + const result = () => Promise.resolve(); + for (const name of [ + "renegotiateUnreadMachines", + "refreshMachineActivities", + "refreshWorkspaceDeletionRuns", + "loadClientConfig", + "refreshCurrentWorkspaceSurface", + "schedulePiWebStatusRefresh", + ]) { + if (!Reflect.set(app, name, result)) throw new Error(`Could not replace PiWebApp.${name}`); + } + const sessions: unknown = Reflect.get(app, "sessions"); + if (typeof sessions !== "object" || sessions === null || !Reflect.set(sessions, "refreshSelectedSession", result)) { + throw new Error("Could not replace the selected-session refresh"); + } +} + +function failBackgroundRefresh(app: PiWebApp, name: string, error: Error): void { + if (!Reflect.set(app, name, () => Promise.reject(error))) throw new Error(`Could not fail PiWebApp.${name}`); +} + +function spyOnTopologyRefresh(app: PiWebApp) { + const controller: unknown = Reflect.get(app, "workspaces"); + if (!(controller instanceof WorkspaceController)) throw new Error("PiWebApp WorkspaceController was unavailable"); + return vi.spyOn(controller, "refreshSelectedProjectTopology").mockResolvedValue(undefined); +} + +/** The exact callback `BrowserResumeController` invokes after a focus/visibility signal. */ +function browserResumeRefresh(app: PiWebApp): RefreshCallback { + const resume: unknown = Reflect.get(app, "browserResume"); + if (typeof resume !== "object" || resume === null) throw new Error("PiWebApp BrowserResumeController was unavailable"); + const callbacks: unknown = Reflect.get(resume, "callbacks"); + if (typeof callbacks !== "object" || callbacks === null) throw new Error("Browser resume callbacks were unavailable"); + const refresh: unknown = Reflect.get(callbacks, "refreshAfterResume"); + if (!isRefreshCallback(refresh)) throw new Error("The browser resume refresh callback was unavailable"); + return refresh; +} + +async function refreshAppData(app: PiWebApp): Promise { + const refresh: unknown = Reflect.get(app, "refreshAppData"); + if (!isRefreshCallback(refresh)) throw new Error("PiWebApp.refreshAppData is not callable"); + await refresh.call(app); +} + +function isRefreshCallback(value: unknown): value is RefreshCallback { + return typeof value === "function"; +} From 36d4e9a29636a948811a5e2623b194550af4aa3c Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 21:42:03 +0200 Subject: [PATCH 08/17] docs(relay): record worktree-autodetect leg 3 and relay completion --- .pi-web/relays/worktree-autodetect/log.md | 86 ++++++++++ .pi-web/relays/worktree-autodetect/status.md | 157 ++++++------------- 2 files changed, 133 insertions(+), 110 deletions(-) diff --git a/.pi-web/relays/worktree-autodetect/log.md b/.pi-web/relays/worktree-autodetect/log.md index 0e629ce..19893c4 100644 --- a/.pi-web/relays/worktree-autodetect/log.md +++ b/.pi-web/relays/worktree-autodetect/log.md @@ -359,3 +359,89 @@ 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. diff --git a/.pi-web/relays/worktree-autodetect/status.md b/.pi-web/relays/worktree-autodetect/status.md index ced8027..874a5ac 100644 --- a/.pi-web/relays/worktree-autodetect/status.md +++ b/.pi-web/relays/worktree-autodetect/status.md @@ -1,127 +1,64 @@ # Status — relay "worktree-autodetect" -## ✅ APPROVED — relay is live +## 🏁 FINISHED — the relay reached its finish line -The human approved the reduced scope in leg 0 and answered every open question. There are -no outstanding decisions. Run leg 3 — the final leg. +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. ## Current position -Legs 1 and 2 are complete and committed (`266f941`, `d0f8f9f`). The server hides prunable -worktrees, and the client now has a proven-inert refresh method. **Nothing calls it yet**, -so newly created worktrees still only appear on a full page load. Leg 3 wires the trigger -and is the last leg. +Every charter finish-line condition is satisfied: -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`. +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 / 1840 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:** 2 (client: non-disruptive topology refresh method) -- **Next leg to run:** 3 (final) +- **Last completed leg:** 3 (final — wiring, docs, changeset, full verify) +- **Next leg to run:** none. Relay complete; do not spawn another runner. -## Next task — leg 3 +## Shipped behavior, as a user sees it -Wire the refresh to the existing resume path, document it, add the changeset. +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`. -See `plan.md` → "Leg 3". Summary: call `this.workspaces.refreshSelectedProjectTopology()` -from `PiWebApp.refreshAfterBrowserResume` (~line 432) and `refreshAppData` (~line 485) — -touch only those two methods in that 2300-line file. Optionally also on -`connectRealtime`'s `onReconnect`, only if it costs nothing. Then one short honest doc -paragraph under `docs/` (detection is resume-scoped, not instant; do not grow `README.md`), -and the `.changeset/*.md` fragment — **still missing, and leg 3 owns it**. +## Relevant context if anyone picks this branch up -Finish with `npm run verify` (charter requires green) — this is the final leg. - -Signature detail leg 3 needs: `refreshSelectedProjectTopology()` takes no arguments, -returns `Promise`, never rejects (failures go to the injected background error sink, -defaulting to `console.warn`), and no-ops when no project is selected. So it can be dropped -directly into the existing `Promise.all` without a `.catch`. - -## Relevant context for the next runner - -Facts established in leg 0 — trust these, they were verified against the running code and -real git; do not re-derive them: - -- **Worktrees are already derived, never registered.** `WorkspaceService.list()` shells out - to `git worktree list --porcelain` on **every** `GET /projects/:projectId/workspaces` - request. There is no server-side cache and no invalidation to design. A worktree created - outside PI WEB is *already* discovered — the gap is purely that the browser never re-asks. -- **`projects.json` holds projects only, not workspaces.** So "auto-adoption" is a - non-problem: nothing needs to be written to a registry, and there is no adopt-vs-visible - distinction to design. This collapsed most of the feature's apparent complexity. -- **Cost of discovery is ~2ms** (measured: 20 sequential `git worktree list --porcelain` - runs in 42ms on this repo). -- **`prunable` is real and load-bearing.** After `rm -rf`ing a worktree directory without - `git worktree remove`, `git worktree list --porcelain` still lists it, with an added - `prunable gitdir file points to non-existent location` line. PI WEB currently shows this - as a normal selectable workspace. `locked` appears as a bare valueless line. -- **The resume path already exists and is already debounced.** - `src/client/src/appShell/browserResumeController.ts` listens to window `focus` and - document `visibilitychange`, batches per animation frame, and collapses concurrent - requests via `TrailingRefreshCoordinator`. It calls - `PiWebApp.refreshAfterBrowserResume()` (~line 432), which already refreshes the selected - session, machine activities, and workspace-deletion runs. Workspace topology is the one - thing missing from that list. -- **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 2's method is inert by construction and covered.** `refreshSelectedProjectTopology()` - applies results via `applyProjectWorkspaces` only, so it writes at most `workspaces` and - `workspacesByProjectId`. Seven tests in - `src/client/src/controllers/workspaceController.test.ts` cover: new worktree appears, - selection + session + file tree + terminal state preserved and `clearActiveSession` not - called, selected workspace disappeared (selection deliberately left alone), stale project - response discarded, stale machine response discarded, rejection reported to the error sink - without touching `state.error`, and no-project no-op. Leg 2 mutation-tested this: injecting - the plausible `selectPreferredWorkspace` + `selectWorkspace` version made 2 tests fail, so - the guard is real and not vacuous. Leg 3 must not weaken these tests to fit its wiring. -- **`WorkspaceControllerDependencies` gained `onBackgroundError`** (optional, defaults to - `console.warn`). `PiWebApp` constructs `WorkspaceController`; leg 3 may pass a message- - prefixing sink to match sibling controllers, but the default is already correct — this is - not required work. -- **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 - selection is still valid. `ProjectActivityOwnershipCoordinator` is the existing precedent - for background topology hydration that does not disturb selection — read it if leg 2 - needs a model. -- **No session daemon involvement.** Nothing in this design touches `src/server/sessiond.ts`, - session runtime ownership, or the daemon protocol. **No manual sessiond restart needed.** - -## Progress documentation expected of each runner - -- Commit the slice (Conventional Commit message). -- Update this file: current position, leg tracking, next task, blockers. -- Append to `log.md`: what, why, artifacts, exact checks run and results. -- Add the `.changeset/*.md` fragment no later than leg 3. - -## Decisions settled by the human (do not re-open) - -1. **Latency: resume-scoped is acceptable.** Detection on tab refocus/visibility is the - agreed behavior. Do not add a timer or a watcher to shorten it. -2. **Removed worktrees: hide them.** Filtering `prunable` worktrees out of the workspace - list is wanted and approved. -3. **Sibling overlap: assume the other session does nothing.** This relay owns the - workspace-topology refresh seam outright. Build it here, do not design for sharing, - and do not read `/srv/dev/pi-web-worktrees/worktree-create-ui`. If that branch later - merges something overlapping, resolving it is that branch's problem, not this one's. +- Commits: `266f941` (server filter), `d0f8f9f` (client refresh method), + `84545fb` (wiring + docs + changeset), plus three `docs(relay)` packet commits. +- 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. Leg 2's main known risk (UI churn on refresh) did **not** materialize: applying the -list through `applyProjectWorkspaces` alone required no workaround, and -`handleWorkspaceChange` early-returns on an unchanged selected workspace id, so a -fresh-but-equal list causes no downstream churn. No intervention signal fired. - -Leg 3's own watch item: after wiring, confirm nothing in the resume path re-derives -selection from the refreshed list. If wiring turns out to need a change *inside* -`refreshSelectedProjectTopology` to stay non-disruptive, that is the intervention signal — -stop rather than relaxing the invariant. +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. From 92c39c8f3594c871b71341f333edb52916e6c0dd Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 22:04:54 +0200 Subject: [PATCH 09/17] fix(workspaces): keep selected workspace metadata fresh on refresh A background topology refresh replaced the workspace 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 while the collapsed Workspaces header and the mobile context bar kept the old one until reselect. Re-point selectedWorkspace at its refreshed entry, keyed by id (derived from the path), so which workspace is selected never changes and the session and terminal teardown in handleWorkspaceChange still does not fire. Skip the patch entirely when metadata is unchanged, so an ordinary resume does not churn object identity into state on every focus. --- .../controllers/workspaceController.test.ts | 53 +++++++++++++++++++ .../src/controllers/workspaceController.ts | 33 ++++++++++-- 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/src/client/src/controllers/workspaceController.test.ts b/src/client/src/controllers/workspaceController.test.ts index ea92d88..4cb98f2 100644 --- a/src/client/src/controllers/workspaceController.test.ts +++ b/src/client/src/controllers/workspaceController.test.ts @@ -220,6 +220,59 @@ describe("WorkspaceController.refreshSelectedProjectTopology", () => { expect(test.backgroundErrors).toEqual([{ message: `Failed to refresh workspaces for project ${repo.id} on local`, error: failure }]); }); + it("re-points the selected workspace at refreshed metadata when its branch changed outside PI WEB", async () => { + const repo = project("p1", "/repo"); + const main = workspace(repo.id, repo.path, { isMain: true }); + const selected = { ...workspace(repo.id, "/repo-feature"), label: "feature-a", branch: "feature-a" }; + const switched = { ...selected, label: "feature-b", branch: "feature-b" }; + const loadWorkspaces = vi.fn().mockResolvedValue([main, switched]); + const test = harness( + { + selectedMachine: machine("local"), + projects: [repo], + selectedProject: repo, + selectedWorkspace: selected, + workspaces: [main, selected], + workspacesByProjectId: { [repo.id]: [main, selected] }, + selectedSession: session(selected.path), + }, + loadWorkspaces, + ); + + await test.controller.refreshSelectedProjectTopology(); + + // Same workspace (same id/path), so the session must survive; only the stale label moves. + expect(test.state().selectedWorkspace).toEqual(switched); + expect(test.state().selectedWorkspace?.id).toBe(selected.id); + expect(test.state().selectedSession).toBeDefined(); + expect(test.clearActiveSession).not.toHaveBeenCalled(); + }); + + it("leaves the selected workspace object untouched when the refresh returns identical metadata", async () => { + const repo = project("p1", "/repo"); + const main = workspace(repo.id, repo.path, { isMain: true }); + const selected = workspace(repo.id, "/repo-feature"); + // A fresh, equal object, exactly what a real HTTP response produces every resume. + const loadWorkspaces = vi.fn().mockResolvedValue([{ ...main }, { ...selected }]); + const test = harness( + { + selectedMachine: machine("local"), + projects: [repo], + selectedProject: repo, + selectedWorkspace: selected, + workspaces: [main, selected], + workspacesByProjectId: { [repo.id]: [main, selected] }, + }, + loadWorkspaces, + ); + + await test.controller.refreshSelectedProjectTopology(); + + // Identity preserved: an unchanged resume must not churn selected-workspace identity + // into state, or every focus would re-render surfaces keyed on this object. + expect(test.state().selectedWorkspace).toBe(selected); + }); + it("does not request anything when no project is selected", async () => { const loadWorkspaces = vi.fn(); const test = harness({ selectedMachine: machine("local") }, loadWorkspaces); diff --git a/src/client/src/controllers/workspaceController.ts b/src/client/src/controllers/workspaceController.ts index 581da29..6a067c8 100644 --- a/src/client/src/controllers/workspaceController.ts +++ b/src/client/src/controllers/workspaceController.ts @@ -1,5 +1,5 @@ import { api as defaultApi, type Project, type Workspace } from "../api"; -import { resetWorkspaceScopedState } from "../appState"; +import { resetWorkspaceScopedState, type AppState } from "../appState"; import { mergeCachedNewSessions } from "../cachedNewSessions"; import { machineProjectKey } from "../machineKeys"; import { selectedMachineId, type GetState, type RouteTarget, type SetState, type UpdateUrl } from "./types"; @@ -122,8 +122,26 @@ export class WorkspaceController { private applyProjectWorkspaces(projectId: string, workspaces: Workspace[]): void { const state = this.getState(); const workspacesByProjectId = { ...state.workspacesByProjectId, [projectId]: workspaces }; - if (state.selectedProject?.id === projectId) this.setState({ workspaces, workspacesByProjectId }); - else this.setState({ workspacesByProjectId }); + if (state.selectedProject?.id !== projectId) { + this.setState({ workspacesByProjectId }); + return; + } + this.setState({ workspaces, workspacesByProjectId, ...this.refreshedSelection(state.selectedWorkspace, workspaces) }); + } + + /** + * Re-points `selectedWorkspace` at its refreshed entry when metadata changed outside PI WEB + * (a branch switched in the worktree, say), so the workspace list and the surfaces that read + * the selected workspace cannot disagree. Keyed by id, which is derived from the path, so + * this never changes *which* workspace is selected and never triggers the session/terminal + * teardown in `handleWorkspaceChange`. Returns nothing when the entry is gone or unchanged, + * so an unchanged refresh does not churn object identity into state. + */ + private refreshedSelection(selected: Workspace | undefined, workspaces: Workspace[]): Pick | undefined { + if (selected === undefined) return undefined; + const refreshed = workspaces.find((candidate) => candidate.id === selected.id); + if (refreshed === undefined || sameWorkspaceMetadata(selected, refreshed)) return undefined; + return { selectedWorkspace: refreshed }; } } @@ -135,3 +153,12 @@ function selectFallbackWorkspace(workspaces: Workspace[]): Workspace | undefined return workspaces.find((workspace) => workspace.isMain) ?? workspaces[0]; } +function sameWorkspaceMetadata(left: Workspace, right: Workspace): boolean { + return left.path === right.path + && left.label === right.label + && left.branch === right.branch + && left.isMain === right.isMain + && left.isGitRepo === right.isGitRepo + && left.isGitWorktree === right.isGitWorktree; +} + From d7fc25312dd1827dcd008f7425691f544dc29b12 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 22:06:04 +0200 Subject: [PATCH 10/17] docs(relay): record the post-relay selected-workspace freshness fix --- .pi-web/relays/worktree-autodetect/log.md | 68 ++++++++++++++++++++ .pi-web/relays/worktree-autodetect/status.md | 22 ++++++- 2 files changed, 89 insertions(+), 1 deletion(-) diff --git a/.pi-web/relays/worktree-autodetect/log.md b/.pi-web/relays/worktree-autodetect/log.md index 19893c4..1cfbe23 100644 --- a/.pi-web/relays/worktree-autodetect/log.md +++ b/.pi-web/relays/worktree-autodetect/log.md @@ -445,3 +445,71 @@ web/UI service path only. **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. diff --git a/.pi-web/relays/worktree-autodetect/status.md b/.pi-web/relays/worktree-autodetect/status.md index 874a5ac..d8e9b47 100644 --- a/.pi-web/relays/worktree-autodetect/status.md +++ b/.pi-web/relays/worktree-autodetect/status.md @@ -7,6 +7,21 @@ 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. + +Reviewed and deliberately left alone: `locked` is parsed but unconsumed (it documents the +kept-worktree policy and is pinned by a `workspaceService` test), and the two refresh entry +points are not deduped across each other (idempotent, stale-guarded, ~2ms). + ## Current position Every charter finish-line condition is satisfied: @@ -49,7 +64,12 @@ 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), plus three `docs(relay)` packet commits. + `84545fb` (wiring + docs + changeset), `79577e4` (selected-workspace freshness fix), + 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 From 9848fc3e406fb3f7e4c9db5b811a663f89338893 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 22:12:35 +0200 Subject: [PATCH 11/17] refactor(workspaces): drop the unused locked worktree flag Nothing consumed GitWorktreeInfo.locked: a locked worktree is a real checkout and stays a usable workspace, so no caller needs to distinguish it. Unknown porcelain keys are already ignored, so parsing it was speculative. The parser test still feeds a real `locked` line and asserts it is ignored, and the service test still pins that a present, non-prunable worktree is kept, so the policy stays covered without the field. --- src/server/workspaces/gitWorktreeDiscovery.test.ts | 8 +++++--- src/server/workspaces/gitWorktreeDiscovery.ts | 3 --- src/server/workspaces/workspaceService.test.ts | 4 ++-- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/server/workspaces/gitWorktreeDiscovery.test.ts b/src/server/workspaces/gitWorktreeDiscovery.test.ts index 6feb326..3147261 100644 --- a/src/server/workspaces/gitWorktreeDiscovery.test.ts +++ b/src/server/workspaces/gitWorktreeDiscovery.test.ts @@ -38,15 +38,17 @@ describe("parseGitWorktreeList", () => { ]); }); - it("reports prunable with its reason and locked with or without a reason", () => { + it("reports prunable, and leaves a locked worktree looking like the usable checkout it is", () => { expect(parseGitWorktreeList(removedAndLocked)).toEqual([ { path: "/repo", branch: "main" }, { path: "/repo-worktrees/gone", branch: "gone", prunable: true }, - { path: "/repo-worktrees/kept", branch: "kept", locked: true }, + // `locked` is ignored: a locked worktree is a real checkout and stays a usable + // workspace, so nothing downstream needs to distinguish it. + { path: "/repo-worktrees/kept", branch: "kept" }, ]); const bareLocked = ["worktree /repo-worktrees/kept", "HEAD abc", "detached", "locked", ""].join("\n"); - expect(parseGitWorktreeList(bareLocked)).toEqual([{ path: "/repo-worktrees/kept", detached: true, locked: true }]); + expect(parseGitWorktreeList(bareLocked)).toEqual([{ path: "/repo-worktrees/kept", detached: true }]); }); it("reads bare repositories and ignores chunks without a worktree path", () => { diff --git a/src/server/workspaces/gitWorktreeDiscovery.ts b/src/server/workspaces/gitWorktreeDiscovery.ts index e20a656..8a8524a 100644 --- a/src/server/workspaces/gitWorktreeDiscovery.ts +++ b/src/server/workspaces/gitWorktreeDiscovery.ts @@ -11,8 +11,6 @@ export interface GitWorktreeInfo { detached?: boolean; /** Git reports a linked worktree as prunable when its checkout directory no longer exists. */ prunable?: boolean; - /** Git reports a locked worktree with a bare `locked` line, optionally followed by a reason. */ - locked?: boolean; } export async function isGitRepository(path: string): Promise { @@ -46,7 +44,6 @@ export function parseGitWorktreeList(stdout: string): GitWorktreeInfo[] { if (key === "bare") info.bare = true; if (key === "detached") info.detached = true; if (key === "prunable") info.prunable = true; - if (key === "locked") info.locked = true; } return info; }).filter((w) => w.path); diff --git a/src/server/workspaces/workspaceService.test.ts b/src/server/workspaces/workspaceService.test.ts index d3b9b49..91e2c6a 100644 --- a/src/server/workspaces/workspaceService.test.ts +++ b/src/server/workspaces/workspaceService.test.ts @@ -31,10 +31,10 @@ describe("WorkspaceService.list", () => { expect(workspaces.map((workspace) => workspace.path)).toEqual(["/repo", "/repo-worktrees/live"]); }); - it("keeps a locked worktree, which is still a real checkout", async () => { + it("keeps a worktree that is present but not prunable, such as a locked one", async () => { const service = serviceFor([ { path: "/repo", branch: "main" }, - { path: "/repo-worktrees/kept", branch: "kept", locked: true }, + { path: "/repo-worktrees/kept", branch: "kept" }, ]); const workspaces = await service.list(project); From 12200ab26a7330049768bb57f7c4e6293d8e5415 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 22:13:08 +0200 Subject: [PATCH 12/17] fix(workspaces): serialize overlapping topology refreshes The browser-resume path and the plugin-facing app refresh call refreshSelectedProjectTopology independently, so two requests for the same machine and project could be in flight at once. The stale guards check machine and project but not ordering, so a slower earlier response landing last overwrote a newer list, making a just-created worktree disappear again. Route the refresh through TrailingRefreshCoordinator, the primitive already used for browser resume, session refresh, and activity, keyed by machine and project. A second caller no longer opens its own request while one is in flight; it gets a single trailing pass, so the last applied response is the newest one. --- .../controllers/workspaceController.test.ts | 44 +++++++++++++++++++ .../src/controllers/workspaceController.ts | 24 ++++++---- 2 files changed, 60 insertions(+), 8 deletions(-) diff --git a/src/client/src/controllers/workspaceController.test.ts b/src/client/src/controllers/workspaceController.test.ts index 4cb98f2..cd189cf 100644 --- a/src/client/src/controllers/workspaceController.test.ts +++ b/src/client/src/controllers/workspaceController.test.ts @@ -273,6 +273,50 @@ describe("WorkspaceController.refreshSelectedProjectTopology", () => { expect(test.state().selectedWorkspace).toBe(selected); }); + it("serializes overlapping refreshes so an earlier response cannot overwrite a newer list", async () => { + const repo = project("p1", "/repo"); + const main = workspace(repo.id, repo.path, { isMain: true }); + const created = workspace(repo.id, "/repo-feature"); + const gates: ((workspaces: Workspace[]) => void)[] = []; + let inFlight = 0; + let maxInFlight = 0; + const loadWorkspaces = vi.fn(() => { + inFlight += 1; + maxInFlight = Math.max(maxInFlight, inFlight); + return new Promise((resolve) => { + gates.push((workspaces) => { inFlight -= 1; resolve(workspaces); }); + }); + }); + const test = harness( + { + selectedMachine: machine("local"), + projects: [repo], + selectedProject: repo, + selectedWorkspace: main, + workspaces: [main], + workspacesByProjectId: { [repo.id]: [main] }, + }, + loadWorkspaces, + ); + + const resumeRefresh = test.controller.refreshSelectedProjectTopology(); + await Promise.resolve(); + const appDataRefresh = test.controller.refreshSelectedProjectTopology(); + await Promise.resolve(); + + // The second caller does not open its own request while the first is in flight; it gets + // one trailing pass afterwards. Without this, two responses race and the slower-but-older + // one can land last, making a just-created worktree disappear again. + expect(maxInFlight).toBe(1); + gates[0]?.([main]); + await vi.waitFor(() => { expect(gates).toHaveLength(2); }); + gates[1]?.([main, created]); + await Promise.all([resumeRefresh, appDataRefresh]); + + // The last response wins, so the newly created worktree stays visible. + expect(test.state().workspaces).toEqual([main, created]); + }); + it("does not request anything when no project is selected", async () => { const loadWorkspaces = vi.fn(); const test = harness({ selectedMachine: machine("local") }, loadWorkspaces); diff --git a/src/client/src/controllers/workspaceController.ts b/src/client/src/controllers/workspaceController.ts index 6a067c8..8158d23 100644 --- a/src/client/src/controllers/workspaceController.ts +++ b/src/client/src/controllers/workspaceController.ts @@ -4,6 +4,7 @@ import { mergeCachedNewSessions } from "../cachedNewSessions"; import { machineProjectKey } from "../machineKeys"; import { selectedMachineId, type GetState, type RouteTarget, type SetState, type UpdateUrl } from "./types"; import type { SessionController } from "./sessionController"; +import { TrailingRefreshCoordinator } from "./trailingRefreshCoordinator"; import { InMemoryWorkspaceSelectionMemory, selectPreferredWorkspace, type WorkspaceSelectionMemory } from "./workspaceSelection"; export interface WorkspaceControllerDependencies { @@ -14,6 +15,7 @@ export interface WorkspaceControllerDependencies { export class WorkspaceController { private readonly api: Pick; private readonly onBackgroundError: (message: string, error: unknown) => void; + private readonly topologyRefreshes = new TrailingRefreshCoordinator(); constructor( private readonly getState: GetState, @@ -99,14 +101,20 @@ export class WorkspaceController { const project = state.selectedProject; if (project === undefined) return; const machineId = selectedMachineId(state); - try { - const workspaces = await this.api.workspaces(project.id, machineId); - const current = this.getState(); - if (selectedMachineId(current) !== machineId || current.selectedProject?.id !== project.id) return; - this.applyProjectWorkspaces(project.id, workspaces); - } catch (error) { - this.onBackgroundError(`Failed to refresh workspaces for project ${project.id} on ${machineId}`, error); - } + // Callers are independent (browser resume and the plugin-facing app refresh), so two + // refreshes for the same machine+project can overlap. Sharing one request keeps a slow + // earlier response from landing last and overwriting a newer list, which would make a + // just-created worktree disappear again. + await this.topologyRefreshes.request(machineProjectKey(machineId, project.id), async () => { + try { + const workspaces = await this.api.workspaces(project.id, machineId); + const current = this.getState(); + if (selectedMachineId(current) !== machineId || current.selectedProject?.id !== project.id) return; + this.applyProjectWorkspaces(project.id, workspaces); + } catch (error) { + this.onBackgroundError(`Failed to refresh workspaces for project ${project.id} on ${machineId}`, error); + } + }); } async refreshAfterWorkspaceDeleted(projectId: string, workspaceId: string): Promise { From ba37bd51aedf805e902e7447023c99b7ac015746 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 22:13:58 +0200 Subject: [PATCH 13/17] docs(relay): record review nits 2 and 3 --- .pi-web/relays/worktree-autodetect/log.md | 65 ++++++++++++++++++++ .pi-web/relays/worktree-autodetect/status.md | 24 ++++++-- 2 files changed, 84 insertions(+), 5 deletions(-) diff --git a/.pi-web/relays/worktree-autodetect/log.md b/.pi-web/relays/worktree-autodetect/log.md index 1cfbe23..86b0639 100644 --- a/.pi-web/relays/worktree-autodetect/log.md +++ b/.pi-web/relays/worktree-autodetect/log.md @@ -513,3 +513,68 @@ without that guard every browser focus would push a new object into state. 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/status.md b/.pi-web/relays/worktree-autodetect/status.md index d8e9b47..1248875 100644 --- a/.pi-web/relays/worktree-autodetect/status.md +++ b/.pi-web/relays/worktree-autodetect/status.md @@ -16,11 +16,24 @@ outside PI WEB showed the new name in the list and the old one in the collapsed 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. +green: 1842 passed, 2 skipped (1843 after the two follow-up fixes below). -Reviewed and deliberately left alone: `locked` is parsed but unconsumed (it documents the -kept-worktree policy and is pinned by a `workspaceService` test), and the two refresh entry -points are not deduped across each other (idempotent, stale-guarded, ~2ms). +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 @@ -42,7 +55,7 @@ Every charter finish-line condition is satisfied: `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 / 1840 passed, 2 skipped) and +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 @@ -65,6 +78,7 @@ under `#worktree-list-out-of-date`. - 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 From c9fabbf1b6b69e51125dfa4126b614d8884838c6 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 22:25:54 +0200 Subject: [PATCH 14/17] chore: untrack the relay working notes .pi-web/ is gitignored local state; the relay packet was force-added while running the work and should not land on main. The notes stay on disk for the worktree, they are just no longer tracked. --- .pi-web/relays/worktree-autodetect/charter.md | 131 ---- .pi-web/relays/worktree-autodetect/log.md | 580 ------------------ .pi-web/relays/worktree-autodetect/plan.md | 168 ----- .pi-web/relays/worktree-autodetect/status.md | 98 --- 4 files changed, 977 deletions(-) delete mode 100644 .pi-web/relays/worktree-autodetect/charter.md delete mode 100644 .pi-web/relays/worktree-autodetect/log.md delete mode 100644 .pi-web/relays/worktree-autodetect/plan.md delete mode 100644 .pi-web/relays/worktree-autodetect/status.md 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. From 4940eda352f0241dbc957102fd069fca6e60b3ac Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 22:54:27 +0200 Subject: [PATCH 15/17] fix(sessions): stop counting session startup as work in progress MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Startup progress rides the per-session activity channel with an "active" phase, because a startup phase really is in progress. But isSessionActive() treated any active activity as work, so a session that was merely *opening* enabled "Stop Active Work", disabled "Reload from disk" with the misleading "Stop current session activity before reloading" tooltip, showed the row's active-work indicator, and — for any caller that hands a startup activity to WorkspaceActivityService — reported the whole workspace as busy. Selecting an archived, read-only session reported active work while it opened. Starting is not working. publishStartupProgress now marks its reports with a new optional SessionActivity.startup field, and isSessionActive() does not count a marked activity. Every affected consumer — the session list, the core actions, the app's activity-transition handling, and the server's workspace aggregation — reads that one helper, so the correction lands in all of them at once. The marker is a new field rather than a new phase on purpose: six readers test phase === "active" directly, including the pending row's "creating · " prefix, the chat dock's active styling, and the daemon's own heartbeat re-publication. It can only ever remove the activity-phase reason for being active, so streaming, bash, compaction, and queued prompts still report as active through the status even while a startup report is the latest activity. The chat dock still shows the startup text; this changes what counts as work, not what is shown. The browser's own pending-create row keeps its previous appearance: it borrows only the daemon's phase text and drops the marker, since that row stands for a create the user is waiting on rather than a session the daemon is opening. --- src/client/src/api/parsers.test.ts | 9 ++++ src/client/src/api/parsers.ts | 13 +++++ src/client/src/components/SessionList.test.ts | 8 ++++ .../sessionController.startupProgress.test.ts | 47 +++++++++++++++++++ .../src/controllers/sessionController.ts | 20 +++++++- src/client/src/plugins/registry.test.ts | 19 ++++++++ src/client/src/sessionSocket.test.ts | 10 ++++ .../activity/workspaceActivityService.test.ts | 13 +++++ .../piSessionService.startupProgress.test.ts | 29 ++++++++++++ src/server/sessions/piSessionService.ts | 7 ++- src/shared/activity.test.ts | 25 +++++++++- src/shared/activity.ts | 10 +++- src/shared/apiTypes.ts | 8 ++++ 13 files changed, 214 insertions(+), 4 deletions(-) diff --git a/src/client/src/api/parsers.test.ts b/src/client/src/api/parsers.test.ts index a05ec35..10ccf61 100644 --- a/src/client/src/api/parsers.test.ts +++ b/src/client/src/api/parsers.test.ts @@ -305,6 +305,15 @@ describe("API parsers", () => { }); }); + it("carries the startup marker so an opening session is not mistaken for a working one", () => { + const activity = { sessionId: "session-1", phase: "active", label: "Opening session", detail: "Starting the Pi session", at: "2026-07-20T00:00:01.000Z", startup: true }; + + expect(parseSessionStartupProgressEvent({ type: "session.startup", activity })).toEqual({ type: "session.startup", activity }); + // A malformed marker is dropped like any other malformed field rather than + // being coerced into "this is startup" or "this is work". + expect(() => parseSessionStartupProgressEvent({ type: "session.startup", activity: { ...activity, startup: "yes" } })).toThrow("Expected optional boolean field: startup"); + }); + it("rejects session startup progress that cannot be routed or rendered honestly", () => { const activity = { sessionId: "session-1", phase: "active", label: "Creating session", at: "2026-07-20T00:00:01.000Z" }; diff --git a/src/client/src/api/parsers.ts b/src/client/src/api/parsers.ts index 4621037..c9d54e0 100644 --- a/src/client/src/api/parsers.ts +++ b/src/client/src/api/parsers.ts @@ -300,9 +300,22 @@ function parseSessionActivity(value: unknown): SessionActivity { label: requireNonEmptyString(record, "label"), ...optionalField("detail", optionalString(record, "detail")), at: requireNonEmptyString(record, "at"), + ...optionalField("startup", optionalActivityStartupMarker(record)), }; } +/** + * The startup marker says the activity is a session opening rather than work in + * progress, which decides whether "Stop Active Work" is offered and whether a + * reload is blocked. A malformed marker is rejected rather than guessed at. + */ +function optionalActivityStartupMarker(record: Record): boolean | undefined { + const value = record["startup"]; + if (value === undefined) return undefined; + if (typeof value !== "boolean") throw new Error("Expected optional boolean field: startup"); + return value; +} + function requireSessionActivityPhase(record: Record, key: string): SessionActivity["phase"] { const value = requireString(record, key); if (value !== "active" && value !== "idle" && value !== "error") throw new Error(`Expected session activity phase field: ${key}`); diff --git a/src/client/src/components/SessionList.test.ts b/src/client/src/components/SessionList.test.ts index dd3953e..fcda1d7 100644 --- a/src/client/src/components/SessionList.test.ts +++ b/src/client/src/components/SessionList.test.ts @@ -16,6 +16,14 @@ describe("sessionRowActivityKind", () => { expect(sessionRowActivityKind(session("s"), { ...idle, isStreaming: true }, undefined, false)).toBe("session"); }); + it("shows no active-work indicator for a session that is only starting up", () => { + const startup = { sessionId: "s", phase: "active" as const, label: "Opening session", detail: "Starting the Pi session", at: "now", startup: true }; + + expect(sessionRowActivityKind(session("s"), idle, startup, false)).toBeUndefined(); + // Ordinary activity is work and keeps its indicator. + expect(sessionRowActivityKind(session("s"), idle, { sessionId: "s", phase: "active", label: "running tool", at: "now" }, false)).toBe("session"); + }); + it("reports unread only while the session is idle", () => { expect(sessionRowActivityKind(session("s"), idle, undefined, false, true)).toBe("unread"); expect(sessionRowActivityKind(session("s"), { ...idle, isStreaming: true }, undefined, false, true)).toBe("session"); diff --git a/src/client/src/controllers/sessionController.startupProgress.test.ts b/src/client/src/controllers/sessionController.startupProgress.test.ts index 8b59c8d..74e0e62 100644 --- a/src/client/src/controllers/sessionController.startupProgress.test.ts +++ b/src/client/src/controllers/sessionController.startupProgress.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from "vitest"; +import { isSessionActive } from "../../../shared/activity"; import { initialAppState } from "../appState"; import { SessionController } from "./sessionController"; import { defaultApi, deferred, emptyPage, FakeSocket, oldSession, runPendingAnimationFrames, sessionLookupId, status, workspace, type AppState, type SessionActivity, type SessionInfo } from "./sessionController.testSupport"; @@ -10,6 +11,7 @@ function startupActivity(patch: Partial = {}): SessionActivity label: "Creating session", detail: "Starting the Pi session", at: "2026-07-20T00:00:01.000Z", + startup: true, ...patch, }; } @@ -76,6 +78,28 @@ describe("SessionController session startup progress", () => { await start; }); + it("keeps a pending create row's own appearance while it borrows the daemon's phase text", async () => { + const state = { current: { ...initialAppState(), selectedWorkspace: workspace, sessions: [] } }; + const { controller, startRequest } = pendingStartController(state); + + const start = controller.startSession(); + const temporaryId = state.current.selectedSession?.id; + if (temporaryId === undefined) throw new Error("Expected temporary session id"); + const beforeStartupProgress = isSessionActive(state.current.status, state.current.activity); + + controller.applyGlobalEvent({ type: "session.startup", startupToken: temporaryId, activity: startupActivity() }); + runPendingAnimationFrames(); + + // This row is the browser's own pending create, not a session being opened. + // Substituting the daemon's phase text into it must not silently change what + // the row reports about itself, or its indicator would blink off mid-create. + expect(state.current.activity?.phase).toBe("active"); + expect(isSessionActive(state.current.status, state.current.activity)).toBe(beforeStartupProgress); + + startRequest.resolve({ ...oldSession, id: "backend-session", path: "/tmp/backend-session.jsonl" }); + await start; + }); + it("restores the generic wording when the daemon has nothing left to attribute", async () => { const state = { current: { ...initialAppState(), selectedWorkspace: workspace, sessions: [] } }; const { controller, startRequest } = pendingStartController(state); @@ -136,6 +160,29 @@ describe("SessionController session startup progress", () => { expect(state.activity).toMatchObject({ sessionId: oldSession.id, label: "Opening session", detail: "Starting the Pi session" }); }); + it("shows an archived session's opening progress without reporting it as active work", () => { + const archived = { ...oldSession, id: "archived-session", archived: true, archivedAt: "2026-05-16T00:00:00.000Z" }; + let state: AppState = { ...initialAppState(), selectedSession: archived, sessions: [archived] }; + const controller = new SessionController( + () => state, + (patch) => { state = { ...state, ...patch }; }, + () => undefined, + undefined, + { socket: new FakeSocket() }, + ); + + controller.applyGlobalEvent({ + type: "session.startup", + activity: startupActivity({ sessionId: archived.id, label: "Opening session" }), + }); + runPendingAnimationFrames(); + + // A read-only session cannot be worked on, so opening one must not make it + // look busy — while the text a waiting user reads still arrives. + expect(state.activity).toMatchObject({ sessionId: archived.id, label: "Opening session", detail: "Starting the Pi session" }); + expect(isSessionActive(state.status, state.activity)).toBe(false); + }); + it("gives an existing session's startup its own row rather than a pending start in the same workspace", async () => { const existing = { ...oldSession, id: "existing-session", cwd: workspace.path }; const state = { current: { ...initialAppState(), selectedWorkspace: workspace, sessions: [existing] } }; diff --git a/src/client/src/controllers/sessionController.ts b/src/client/src/controllers/sessionController.ts index 46fbaa0..76ebcd9 100644 --- a/src/client/src/controllers/sessionController.ts +++ b/src/client/src/controllers/sessionController.ts @@ -1328,7 +1328,7 @@ export class SessionController { // creation request that has not returned yet. this.queueActivityUpdate(event.activity.phase === "idle" ? creatingPendingSessionActivity(pending.tempId, pending.queuedSends.length) - : { ...event.activity, sessionId: pending.tempId }); + : pendingStartActivity(event.activity, pending.tempId)); } private schedulePendingFlush(): void { @@ -1429,6 +1429,24 @@ function isClientPendingStartSessionInfo(session: SessionInfo | undefined): sess return session !== undefined && "clientPendingStart" in session && session.clientPendingStart === true; } +/** + * Re-label a startup report onto the browser's own pending create row. + * + * The daemon's startup marker is dropped: this row stands for a create the user + * asked for and is waiting on, which the browser has always reported as active + * work in progress, rather than for a session the daemon is opening. Only the + * phase text is borrowed, so the row keeps its own `creating · ` appearance. + */ +function pendingStartActivity(activity: SessionActivity, sessionId: string): SessionActivity { + return { + sessionId, + phase: activity.phase, + label: activity.label, + ...(activity.detail === undefined ? {} : { detail: activity.detail }), + at: activity.at, + }; +} + function creatingPendingSessionActivity(sessionId: string, queuedCount = 0): SessionActivity { return { sessionId, diff --git a/src/client/src/plugins/registry.test.ts b/src/client/src/plugins/registry.test.ts index bdb1de3..7c74fda 100644 --- a/src/client/src/plugins/registry.test.ts +++ b/src/client/src/plugins/registry.test.ts @@ -249,6 +249,25 @@ describe("PluginRegistry", () => { expect(busy.find((action) => action.id === "core:session.reload")?.enabled).toBe(false); }); + it("treats a session that is only starting up as having no work to stop or block", () => { + const registry = new PluginRegistry(); + registry.register({ id: "core", plugin: corePlugin }); + const reloadRuntime = { local: { machineId: "local", ok: true as const, checkedAt: "now", capabilities: [PI_WEB_CAPABILITIES.sessionsReload, PI_WEB_CAPABILITIES.sessionsPersistedState] } }; + const startupActivity = { sessionId: "s1", phase: "active" as const, label: "Opening session", detail: "Starting the Pi session", at: "now", startup: true }; + + const opening = registry.getActions(createContext({ selectedSession: testSession({ persisted: true }), status: testStatus({ persisted: true }), activity: startupActivity, machineRuntimes: reloadRuntime }).context); + + // Nothing is being worked on, so there is nothing to stop and no reason to + // block a reload with "Stop current session activity before reloading". + expect(opening.find((action) => action.id === "core:session.stop")?.enabled).toBe(false); + expect(opening.find((action) => action.id === "core:session.reload")?.enabled).toBe(true); + + // Real work is still real work, whatever else the session is doing. + const working = registry.getActions(createContext({ selectedSession: testSession({ persisted: true }), status: testStatus({ persisted: true, isStreaming: true }), activity: startupActivity, machineRuntimes: reloadRuntime }).context); + expect(working.find((action) => action.id === "core:session.stop")?.enabled).toBe(true); + expect(working.find((action) => action.id === "core:session.reload")?.enabled).toBe(false); + }); + it("routes session reload through the runtime context", () => { const registry = new PluginRegistry(); registry.register({ id: "core", plugin: corePlugin }); diff --git a/src/client/src/sessionSocket.test.ts b/src/client/src/sessionSocket.test.ts index 0337316..c96cdd1 100644 --- a/src/client/src/sessionSocket.test.ts +++ b/src/client/src/sessionSocket.test.ts @@ -90,6 +90,16 @@ describe("notification socket guards", () => { })).toBeUndefined(); }); + it("carries the startup marker through the socket boundary, marker and all", () => { + const activity = { sessionId: "session-1", phase: "active", label: "Opening session", detail: "Starting the Pi session", at: "2026-07-20T00:00:01.000Z", startup: true }; + + // The marker is what stops an opening session being treated as a working + // one, so dropping it in transit would restore the defect for every frame, + // including those relayed from a remote machine. + expect(parseRealtimeSocketEvent({ type: "session.startup", activity })).toMatchObject({ type: "session.startup", activity: { startup: true } }); + expect(parseRealtimeSocketEvent({ type: "session.startup", activity: { ...activity, startup: 1 } })).toBeUndefined(); + }); + it("accepts validated session startup progress and drops malformed frames", () => { const activity = { sessionId: "session-1", phase: "active", label: "Creating session", detail: "Starting the Pi session", at: "2026-07-20T00:00:01.000Z" }; diff --git a/src/server/activity/workspaceActivityService.test.ts b/src/server/activity/workspaceActivityService.test.ts index 976ec0a..4378016 100644 --- a/src/server/activity/workspaceActivityService.test.ts +++ b/src/server/activity/workspaceActivityService.test.ts @@ -32,6 +32,19 @@ describe("WorkspaceActivityService", () => { expect(events.at(-1)).toMatchObject({ type: "workspace.activity", activity: { cwd: "/repo", hasSessionActivity: false, hasTerminalActivity: false } }); }); + it("does not report a workspace active for a session that is only starting up", () => { + const events: RealtimeEvent[] = []; + const service = new WorkspaceActivityService({ publishRealtime: (event) => events.push(event) }); + + // Startup progress names a phase the daemon is inside; it is not work, so the + // workspace (and the project indicators and remote machines that read it) + // must not be reported as busy because of it. + service.applySessionActivity("/repo", { sessionId: "s1", phase: "active", label: "Opening session", detail: "Starting the Pi session", at: "now", startup: true }); + + expect(service.snapshot().workspaces).toEqual([]); + expect(events.at(-1)).toMatchObject({ type: "workspace.activity", activity: { cwd: "/repo", hasSessionActivity: false } }); + }); + it("clears stale active activity when an idle status arrives", () => { const events: RealtimeEvent[] = []; const service = new WorkspaceActivityService({ publishRealtime: (event) => events.push(event) }); diff --git a/src/server/sessions/piSessionService.startupProgress.test.ts b/src/server/sessions/piSessionService.startupProgress.test.ts index aa55a8d..f4a404f 100644 --- a/src/server/sessions/piSessionService.startupProgress.test.ts +++ b/src/server/sessions/piSessionService.startupProgress.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { PiSessionService, type PiSessionRuntime } from "./piSessionService.js"; import { CapturingSessionEventHub, emptyArchiveStore, fakeRuntime, sessionGateway, sessionRecord, sessionRef, testModelRuntime } from "./piSessionService.testSupport.js"; +import { isSessionActive } from "../../shared/activity.js"; import type { SessionActivity, SessionStartupProgressEvent } from "../../shared/apiTypes.js"; const TEST_AGENT_DIR = "/tmp/pi-web-test-agent"; @@ -235,6 +236,34 @@ describe("PiSessionService session startup progress", () => { await service.dispose(); }); + it("reports startup progress as starting rather than as work in progress", async () => { + const { hub, service } = startupService(); + + await service.start("/workspace"); + + // Startup phases are published with an "active" phase so the waiting user + // sees them, but opening a session is not work: nothing that decides whether + // work is in progress may count them. + const phases = startupEvents(hub).filter((event) => event.activity.phase === "active"); + expect(phases).toHaveLength(2); + expect(phases.map((event) => isSessionActive(undefined, event.activity))).toEqual([false, false]); + await service.dispose(); + }); + + it("still reports a real activity published during startup as work", async () => { + const { hub, fake, service } = startupService(); + + await service.start("/workspace"); + fake.emit({ type: "tool_execution_start", toolName: "bash" }); + + // The marker belongs to the startup channel alone; an ordinary activity for + // the same session still counts, or the fix would hide real work. + const running = activityUpdates(hub).filter((activity) => activity.phase === "active"); + expect(running.length).toBeGreaterThan(0); + expect(running.every((activity) => isSessionActive(undefined, activity))).toBe(true); + await service.dispose(); + }); + it("keeps startup reporting event-only, writing no session or workspace activity state", async () => { const recorder = recordingWorkspaceActivity(); const failure = new Error("runtime unavailable"); diff --git a/src/server/sessions/piSessionService.ts b/src/server/sessions/piSessionService.ts index 8ccc346..b6cbd80 100644 --- a/src/server/sessions/piSessionService.ts +++ b/src/server/sessions/piSessionService.ts @@ -3039,10 +3039,15 @@ export class PiSessionService implements SessionRouteService { * Unlike {@link publishActivity} this deliberately records nothing: no * `activities` entry, no workspace activity, no unread observation. There is * no session to own that state, and a failed creation would leave it stranded. + * + * Every report is marked `startup`, which is what keeps a session that is + * merely opening from counting as one doing work. This is the only publisher + * that sets the marker, and because it writes no `activities` entry no later + * heartbeat re-publication can carry it. */ private publishStartupProgress(sessionId: string, startupToken: string | undefined, label: string, phase: "active" | "idle", detail: string | undefined): void { const at = new Date().toISOString(); - const activity = detail === undefined ? { sessionId, phase, label, at } : { sessionId, phase, label, detail, at }; + const activity = detail === undefined ? { sessionId, phase, label, at, startup: true } : { sessionId, phase, label, detail, at, startup: true }; this.events.publishGlobal(startupToken === undefined ? { type: "session.startup", activity } : { type: "session.startup", startupToken, activity }); } diff --git a/src/shared/activity.test.ts b/src/shared/activity.test.ts index badf315..b99b659 100644 --- a/src/shared/activity.test.ts +++ b/src/shared/activity.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import { isSessionActive, isWorkspaceActivityActive } from "./activity"; -import type { SessionStatus, WorkspaceActivity } from "./apiTypes"; +import type { SessionActivity, SessionStatus, WorkspaceActivity } from "./apiTypes"; const idleStatus: SessionStatus = { sessionId: "s1", @@ -20,6 +20,29 @@ describe("activity helpers", () => { expect(isSessionActive({ ...idleStatus, pendingMessageCount: 2 })).toBe(true); }); + it("does not count a session that is only starting up as doing work", () => { + // Startup is reported on the activity channel with an "active" phase because + // a phase really is in progress, but opening a session is not work a user + // can stop, so the marker is what separates starting from working. + const startup: SessionActivity = { sessionId: "s1", phase: "active", label: "Opening session", detail: "Starting the Pi session", at: "now", startup: true }; + + expect(isSessionActive(undefined, startup)).toBe(false); + expect(isSessionActive(idleStatus, startup)).toBe(false); + }); + + it("still reports genuine work happening while a session starts up", () => { + const startup: SessionActivity = { sessionId: "s1", phase: "active", label: "Opening session", at: "now", startup: true }; + + // The marker only removes the activity-phase reason for being active, so + // work the status proves is unaffected by it. + expect(isSessionActive({ ...idleStatus, isStreaming: true }, startup)).toBe(true); + expect(isSessionActive({ ...idleStatus, isBashRunning: true }, startup)).toBe(true); + expect(isSessionActive({ ...idleStatus, isCompacting: true }, startup)).toBe(true); + expect(isSessionActive({ ...idleStatus, pendingMessageCount: 1 }, startup)).toBe(true); + // An unmarked active activity is ordinary work and keeps counting. + expect(isSessionActive(idleStatus, { sessionId: "s1", phase: "active", label: "running tool", at: "now" })).toBe(true); + }); + it("detects workspace activity presence without exposing details", () => { const idle: WorkspaceActivity = { cwd: "/repo", hasSessionActivity: false, hasTerminalActivity: false, updatedAt: "now" }; expect(isWorkspaceActivityActive(idle)).toBe(false); diff --git a/src/shared/activity.ts b/src/shared/activity.ts index 657b34b..4994562 100644 --- a/src/shared/activity.ts +++ b/src/shared/activity.ts @@ -1,7 +1,15 @@ import type { SessionActivity, SessionStatus, WorkspaceActivity } from "./apiTypes.js"; +/** + * Whether a session has work in progress a user could stop or must wait for. + * + * A startup activity is excluded: the session is being opened, not worked in. + * The exclusion can only ever remove the activity-phase reason for being + * active, so streaming, bash, compaction, and queued prompts still count even + * while a startup report is the latest activity. + */ export function isSessionActive(status?: SessionStatus, activity?: SessionActivity): boolean { - return activity?.phase === "active" + return (activity?.phase === "active" && activity.startup !== true) || status?.isStreaming === true || status?.isBashRunning === true || status?.isCompacting === true diff --git a/src/shared/apiTypes.ts b/src/shared/apiTypes.ts index 308f15f..93839d9 100644 --- a/src/shared/apiTypes.ts +++ b/src/shared/apiTypes.ts @@ -426,6 +426,14 @@ export interface SessionActivity { label: string; detail?: string; at: string; + /** + * Set only on the startup window's own reports. A startup phase is genuinely + * in progress, so it is published as `active` and rendered like any other + * activity, but *starting* a session is not *working* in it: there is nothing + * to stop, nothing that blocks reloading from disk, and no workspace-level + * work to report. `isSessionActive()` reads this to keep the two apart. + */ + startup?: boolean; } export interface QueuedSessionMessage { From e502d569a99f10084ddf8686354983fdc7a36349 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Sun, 26 Jul 2026 23:15:09 +0200 Subject: [PATCH 16/17] test(sessiond): make the concurrent-refresh note's wiring verifiable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One optional line in sessiond.ts passed the model catalog refresher into PiSessionService, and it was the only thing making "· provider model lists are refreshing" reachable in the product. Deleting it left typecheck, lint, Knip, and all 1860 tests green while the note silently disappeared — confirmed by actually deleting it and running each check. sessiond.ts starts a daemon as an import side effect, so nothing in it could be tested. Extract the dependency assembly into a pure sessionServiceDependencies() function, following the sessionDaemonStartup.ts precedent, and assert what a user is actually told: a service built by the real assembly, with a refresh in flight, reports the note on its startup phases. A companion test pins the note to the refresher's answer so the first cannot pass on unconditional wording. Only the object literal moved. Everything createRuntime() constructs before it stays in place and in order — the provider freeze must still precede any real session, for the reason its comment gives — and the extracted function performs no side effect, so construction order, side effects, and routes are unchanged. The wiring is now guarded at both hops. Dropping the field from the assembly fails the new test; dropping it from sessiond.ts fails typecheck, because the assembly's input type requires every collaborator the daemon constructs. PiSessionServiceDependencies.catalogRefreshStatus stays optional, so the 98 existing service constructions in the suite are untouched, and the refresher's getter stays read-only: the test injects a fake in-flight status rather than touching its cadence, timeout, or coalescing. subsessionsEnabled's spawn-capability conjunction moved into the assembly with the literal it lived in. The semantics are identical, and it is now covered by a test instead of being another untested decision in an untestable file. --- src/server/sessiond.ts | 9 +- .../sessionServiceDependencies.test.ts | 90 +++++++++++++++++++ .../sessiond/sessionServiceDependencies.ts | 53 +++++++++++ 3 files changed, 147 insertions(+), 5 deletions(-) create mode 100644 src/server/sessiond/sessionServiceDependencies.test.ts create mode 100644 src/server/sessiond/sessionServiceDependencies.ts diff --git a/src/server/sessiond.ts b/src/server/sessiond.ts index 0a67a5e..87f78d7 100644 --- a/src/server/sessiond.ts +++ b/src/server/sessiond.ts @@ -27,6 +27,7 @@ import { SESSIOND_RUNTIME_CAPABILITIES } from "../shared/capabilities.js"; import { agentSessionDirEnvKeys, effectivePiWebConfig, maxUploadBytes, offlineModeEnabled } from "../config.js"; import { createActiveAgentProfileDescriptor } from "../sessiond/activeAgentProfile.js"; import { runSessionDaemonStartup } from "./sessiond/sessionDaemonStartup.js"; +import { sessionServiceDependencies } from "./sessiond/sessionServiceDependencies.js"; const daemonEnvironment: NodeJS.ProcessEnv = Object.freeze({ ...process.env }); const { config } = effectivePiWebConfig({ env: daemonEnvironment }); @@ -70,24 +71,22 @@ await runSessionDaemonStartup({ const spawnTargets = config.spawnSessions ? new ProjectScopedSpawnTargetResolver({ projects: new ProjectService(new ProjectStore()), workspaces: new WorkspaceService() }) : undefined; - const sessions = new PiSessionService(eventHub, { + const sessions = new PiSessionService(eventHub, sessionServiceDependencies({ modelRuntime: auth.runtime, agentDir: activeAgentProfile.dir, workspaceActivity, logger: app.log, ...(spawnTargets === undefined ? {} : { spawnTargets }), - subsessionsEnabled: spawnTargets !== undefined && config.subsessions, + subsessionsEnabled: config.subsessions, notificationStore, unreadStore, - // Read-only, so session startup can tell a waiting user that provider - // model lists are refreshing at the same time. catalogRefreshStatus: catalogRefresher, sessionManager: createPiSessionManagerGateway({ agentDir: activeAgentProfile.dir, env: daemonEnvironment, sessionDirEnvKeys: activeAgentProfile.sessionDirEnvKeys, }), - }); + })); auth.subscribe((change) => { sessions.applyAuthChange(change); }); const terminals = new TerminalService(eventHub, workspaceActivity); const runtimeComponent = Object.freeze({ diff --git a/src/server/sessiond/sessionServiceDependencies.test.ts b/src/server/sessiond/sessionServiceDependencies.test.ts new file mode 100644 index 0000000..fcc52c9 --- /dev/null +++ b/src/server/sessiond/sessionServiceDependencies.test.ts @@ -0,0 +1,90 @@ +import { describe, expect, it } from "vitest"; +import { WorkspaceActivityService } from "../activity/workspaceActivityService.js"; +import { SessionNotificationStore } from "../sessions/sessionNotificationStore.js"; +import { SessionUnreadStore } from "../sessions/sessionUnreadStore.js"; +import { PiSessionService, type PiSessionServiceDependencies } from "../sessions/piSessionService.js"; +import { CapturingSessionEventHub, emptyArchiveStore, fakeRuntime, sessionGateway, testModelRuntime } from "../sessions/piSessionService.testSupport.js"; +import { sessionServiceDependencies, type SessionServiceDependencyInput } from "./sessionServiceDependencies.js"; + +const AGENT_DIR = "/tmp/pi-web-test-agent"; + +/** + * The collaborators sessiond hands the assembly, with only the process-facing + * ones (agent dir, session store gateway) replaced. Everything the assembly + * decides is left to the assembly. + */ +function daemonCollaborators(patch: Partial = {}): SessionServiceDependencyInput { + return { + agentDir: AGENT_DIR, + modelRuntime: testModelRuntime, + sessionManager: sessionGateway([]), + workspaceActivity: new WorkspaceActivityService(), + logger: { info() { /* no-op */ } }, + notificationStore: new SessionNotificationStore(), + unreadStore: new SessionUnreadStore(), + catalogRefreshStatus: { isRefreshInFlight: () => false }, + subsessionsEnabled: false, + ...patch, + }; +} + +/** + * Start a session through a service built by the real assembly and collect what + * the user is told while waiting. Only test-local seams are patched onto the + * assembled dependencies, never anything the assembly is responsible for + * supplying, so a dependency the assembly stops passing cannot be masked here. + */ +async function startupDetails(deps: PiSessionServiceDependencies): Promise { + const hub = new CapturingSessionEventHub(); + const fake = fakeRuntime(); + const service = new PiSessionService(hub, { + ...deps, + archiveStore: emptyArchiveStore(), + createAgentRuntime: () => Promise.resolve(fake.runtime), + heartbeatIntervalMs: 60_000, + }); + try { + await service.start("/workspace"); + } finally { + await service.dispose(); + } + return hub.globalEvents.flatMap((event) => + event.type === "session.startup" && event.activity.detail !== undefined ? [event.activity.detail] : [], + ); +} + +describe("sessiond session service dependency assembly", () => { + it("reports a concurrent provider model list refresh to a waiting user", async () => { + // The note is only reachable in the product because the assembly hands the + // refresher to the session service. Asserting the note rather than the + // property means dropping that line fails here instead of passing silently. + const details = await startupDetails(sessionServiceDependencies(daemonCollaborators({ + catalogRefreshStatus: { isRefreshInFlight: () => true }, + }))); + + expect(details).toEqual([ + "Starting the Pi session · provider model lists are refreshing", + "Loading session extensions · provider model lists are refreshing", + ]); + }); + + it("states the startup phase alone when no refresh is running", async () => { + const details = await startupDetails(sessionServiceDependencies(daemonCollaborators())); + + // Pins the note to the refresher's answer, so the test above cannot pass on + // wording that is always appended. + expect(details).toEqual(["Starting the Pi session", "Loading session extensions"]); + }); + + it("keeps tracked subsessions off unless spawning is configured as well", () => { + const spawnTargets = { resolveSpawnTarget: () => Promise.reject(new Error("not used")) }; + + const withoutSpawnTargets = sessionServiceDependencies(daemonCollaborators({ subsessionsEnabled: true })); + const withSpawnTargets = sessionServiceDependencies(daemonCollaborators({ subsessionsEnabled: true, spawnTargets })); + + expect(withoutSpawnTargets.spawnTargets).toBeUndefined(); + expect(withoutSpawnTargets.subsessionsEnabled).toBe(false); + expect(withSpawnTargets.spawnTargets).toBe(spawnTargets); + expect(withSpawnTargets.subsessionsEnabled).toBe(true); + }); +}); diff --git a/src/server/sessiond/sessionServiceDependencies.ts b/src/server/sessiond/sessionServiceDependencies.ts new file mode 100644 index 0000000..a2dcfbb --- /dev/null +++ b/src/server/sessiond/sessionServiceDependencies.ts @@ -0,0 +1,53 @@ +import type { PiSessionServiceDependencies } from "../sessions/piSessionService.js"; + +/** + * The collaborators sessiond constructs, in the shape the assembly needs them. + * + * Every field is required unless the capability itself is optional, so a + * collaborator the daemon stops constructing cannot silently vanish from the + * session service. + */ +export interface SessionServiceDependencyInput { + agentDir: string; + sessionManager: PiSessionServiceDependencies["sessionManager"]; + modelRuntime: PiSessionServiceDependencies["modelRuntime"]; + workspaceActivity: NonNullable; + logger: NonNullable; + notificationStore: NonNullable; + unreadStore: NonNullable; + /** Read-only view of the background refresher; see the assembly below. */ + catalogRefreshStatus: NonNullable; + /** Omitted when the operator has not enabled session spawning. */ + spawnTargets?: NonNullable; + /** The operator's subsessions preference, which also requires spawning. */ + subsessionsEnabled: boolean; +} + +/** + * Map sessiond's constructed collaborators onto the session service's + * dependencies. + * + * Extracted from `sessiond.ts`, which starts a daemon as an import side effect + * and so cannot be loaded by a test. This function performs no side effects and + * constructs nothing, so lifting it changes no startup ordering; it exists only + * so a test can build a service the way the daemon does and assert what a user + * is actually told. + */ +export function sessionServiceDependencies(input: SessionServiceDependencyInput): PiSessionServiceDependencies { + return { + modelRuntime: input.modelRuntime, + agentDir: input.agentDir, + workspaceActivity: input.workspaceActivity, + logger: input.logger, + ...(input.spawnTargets === undefined ? {} : { spawnTargets: input.spawnTargets }), + // Tracked subsessions share the spawn capability's project-scope resolver, + // so they stay off unless spawning is configured too. + subsessionsEnabled: input.spawnTargets !== undefined && input.subsessionsEnabled, + notificationStore: input.notificationStore, + unreadStore: input.unreadStore, + // Read-only, so session startup can tell a waiting user that provider + // model lists are refreshing at the same time. + catalogRefreshStatus: input.catalogRefreshStatus, + sessionManager: input.sessionManager, + }; +} From ce4b46972725ac2f32f05142c5925adff7946527 Mon Sep 17 00:00:00 2001 From: Federico Jaramillo Martinez Date: Mon, 27 Jul 2026 09:40:24 +0200 Subject: [PATCH 17/17] feat(ui): add model and thinking selector actions --- .changeset/model-thinking-picker-actions.md | 5 ++++ src/client/src/components/PiWebApp.ts | 2 ++ src/client/src/plugins/core/actions.ts | 21 +++++++++++++++ src/client/src/plugins/registry.test.ts | 30 +++++++++++++++++++++ src/client/src/plugins/types.ts | 2 ++ 5 files changed, 60 insertions(+) create mode 100644 .changeset/model-thinking-picker-actions.md diff --git a/.changeset/model-thinking-picker-actions.md b/.changeset/model-thinking-picker-actions.md new file mode 100644 index 0000000..9a8f0db --- /dev/null +++ b/.changeset/model-thinking-picker-actions.md @@ -0,0 +1,5 @@ +--- +"@jmfederico/pi-web": patch +--- + +Add action-palette commands for selecting a session's model and thinking level, with support for assigning custom shortcuts in Settings. diff --git a/src/client/src/components/PiWebApp.ts b/src/client/src/components/PiWebApp.ts index e12cc72..15dec34 100644 --- a/src/client/src/components/PiWebApp.ts +++ b/src/client/src/components/PiWebApp.ts @@ -1791,6 +1791,8 @@ export class PiWebApp extends LitElement { configureAuth: () => this.auth.openLogin(), logoutAuth: () => this.auth.openLogout(), openThemePicker: () => { this.openThemeDialog(); }, + openModelPicker: () => this.openModelDialog(), + openThinkingLevelPicker: () => this.openThinkingDialog(), selectMainView: (view) => { this.selectMainView(view); }, selectWorkspaceTool: (tool) => { this.openWorkspaceTool(tool); }, openTerminal: (options) => { this.openTerminal(options); }, diff --git a/src/client/src/plugins/core/actions.ts b/src/client/src/plugins/core/actions.ts index 322e1e9..2d0f0b4 100644 --- a/src/client/src/plugins/core/actions.ts +++ b/src/client/src/plugins/core/actions.ts @@ -167,6 +167,22 @@ export function createCoreActions(): PluginAction[] { enabled: hasWorkspace, run: (context) => context.startSession(), }, + { + id: "model.select", + title: "Select Model", + description: "Choose the model for the selected session", + group: "Session", + enabled: hasSelectableSession, + run: (context) => context.openModelPicker(), + }, + { + id: "thinking.select", + title: "Select Thinking Level", + description: "Choose the thinking level for the selected session", + group: "Session", + enabled: hasSelectableSession, + run: (context) => context.openThinkingLevelPicker(), + }, { id: "session.archive", title: "Archive Session", @@ -216,6 +232,11 @@ function hasDeletableWorkspace(context: { state: AppState }): boolean { return workspace !== undefined && workspace.isGitWorktree && !workspace.isMain && !isWorkspaceDeletionPending(context.state, workspace); } +function hasSelectableSession(context: { state: AppState }): boolean { + const session = context.state.selectedSession; + return session !== undefined && session.archived !== true; +} + function hasArchivableSession(context: { state: AppState }): boolean { return isArchivableSessionInfo(context.state.selectedSession, context.state.status, sessionPersistenceOptions(context.state)); } diff --git a/src/client/src/plugins/registry.test.ts b/src/client/src/plugins/registry.test.ts index 7c74fda..cfc53dd 100644 --- a/src/client/src/plugins/registry.test.ts +++ b/src/client/src/plugins/registry.test.ts @@ -38,6 +38,8 @@ function createContext(statePatch: Partial = {}) { configureAuth: vi.fn(() => { calls.push("configureAuth"); }), logoutAuth: vi.fn(() => { calls.push("logoutAuth"); }), openThemePicker: vi.fn(() => { calls.push("openThemePicker"); }), + openModelPicker: vi.fn(() => { calls.push("openModelPicker"); }), + openThinkingLevelPicker: vi.fn(() => { calls.push("openThinkingLevelPicker"); }), selectMainView: vi.fn((view: AppState["mainView"]) => { calls.push(`selectMainView:${view}`); }), selectWorkspaceTool: vi.fn((tool: AppState["workspaceTool"]) => { calls.push(`selectWorkspaceTool:${tool}`); }), openTerminal: vi.fn((options?: { terminalId?: string | undefined }) => { calls.push(`openTerminal:${options?.terminalId ?? ""}`); }), @@ -290,6 +292,34 @@ describe("PluginRegistry", () => { expect(calls).toEqual(["deleteCachedNewSession"]); }); + it("exposes model and thinking selectors as configurable actions for writable sessions", () => { + const registry = new PluginRegistry(); + registry.register({ id: "core", plugin: corePlugin }); + + const unavailable = registry.getActions(createContext().context); + expect(unavailable.find((action) => action.id === "core:model.select")?.enabled).toBe(false); + expect(unavailable.find((action) => action.id === "core:thinking.select")?.enabled).toBe(false); + + const archivedSession = { ...testSession(), archived: true, archivedAt: "2026-05-20T00:00:00.000Z" }; + const archived = registry.getActions(createContext({ selectedSession: archivedSession }).context); + expect(archived.find((action) => action.id === "core:model.select")?.enabled).toBe(false); + expect(archived.find((action) => action.id === "core:thinking.select")?.enabled).toBe(false); + + const { context, calls } = createContext({ selectedSession: testSession() }); + const actions = registry.getActions(context); + const modelAction = actions.find((action) => action.id === "core:model.select"); + const thinkingAction = actions.find((action) => action.id === "core:thinking.select"); + expect(modelAction).toMatchObject({ title: "Select Model", enabled: true }); + expect(modelAction?.shortcut).toBeUndefined(); + expect(thinkingAction).toMatchObject({ title: "Select Thinking Level", enabled: true }); + expect(thinkingAction?.shortcut).toBeUndefined(); + + if (modelAction !== undefined) void modelAction.run(); + if (thinkingAction !== undefined) void thinkingAction.run(); + + expect(calls).toEqual(["openModelPicker", "openThinkingLevelPicker"]); + }); + it("routes refresh current to the active core workspace panel", () => { const registry = new PluginRegistry(); registry.register({ id: "core", plugin: corePlugin }); diff --git a/src/client/src/plugins/types.ts b/src/client/src/plugins/types.ts index a0cc1ee..755cad3 100644 --- a/src/client/src/plugins/types.ts +++ b/src/client/src/plugins/types.ts @@ -106,6 +106,8 @@ export interface PluginRuntimeContext { configureAuth: () => void | Promise; logoutAuth: () => void | Promise; openThemePicker: () => void; + openModelPicker: () => void | Promise; + openThinkingLevelPicker: () => void | Promise; selectMainView: (view: AppState["mainView"]) => void; selectWorkspaceTool: (tool: QualifiedContributionId) => void; openTerminal: (options?: { terminalId?: string | undefined }) => void;