From bc39730b5813b3ce30ac98fa8168b3efa96f650d Mon Sep 17 00:00:00 2001 From: mptyl Date: Tue, 4 Aug 2026 05:13:39 +0200 Subject: [PATCH] feat: pin sessions to workspace revisions --- .../task-7-report.md | 23 +++ backend/src/routes/sessions.ts | 190 ++++++++++-------- backend/test/routes-sessions.test.ts | 104 +++++++++- 3 files changed, 234 insertions(+), 83 deletions(-) diff --git a/.superpowers/sdd/2026-08-03-git-workspace-registry/task-7-report.md b/.superpowers/sdd/2026-08-03-git-workspace-registry/task-7-report.md index c0fc0cea..8a6239d6 100644 --- a/.superpowers/sdd/2026-08-03-git-workspace-registry/task-7-report.md +++ b/.superpowers/sdd/2026-08-03-git-workspace-registry/task-7-report.md @@ -26,6 +26,29 @@ completed with 22 passing tests. - `git diff --check` completed cleanly. +## Review fixes — round 2 + +- Lifecycle authorization no longer selects the installation-default workspace. The backend now + finds each session by querying every operational registry snapshot with the authenticated + principal, preserving RLS ownership concealment. +- After locating the manifest, durable pinned sessions resolve their retained descriptor before + any lifecycle mutation/reopen. Legacy sessions continue using the locating registry snapshot. +- Session listing aggregates the owner-visible rows from all operational registry snapshots; + detail, response, steer, resume, events, documents, and lifecycle mutations use the same + server-side locator. No route depends on browser-local workspace state. + +### Round 2 verification + +- RED: the new cross-workspace route integration test created a B session while installation + default A was selected, then demonstrated that `GET /sessions` returned an empty list. +- GREEN: `npx vitest run test/routes-sessions.test.ts test/tht-runner.test.ts test/routes-settings.test.ts && npx tsc --noEmit -p .` + — 101 tests passed with a clean type check. The integration test covers create B, list, detail, + response, and resume through B's pinned descriptor while default A remains configured. +- Full backend suite: 342 tests passed. The remaining 7 tests require binding `127.0.0.1` and + fail in this sandbox with `listen EPERM: operation not permitted`; no application assertion + failed. The focused typecheck above passed. +- `git diff --check` completed cleanly. + ## Verification note The unscoped backend suite was also run. The Task 7 code regressions in `test/tht-runner.test.ts` diff --git a/backend/src/routes/sessions.ts b/backend/src/routes/sessions.ts index c2fab50c..f6068a9c 100644 --- a/backend/src/routes/sessions.ts +++ b/backend/src/routes/sessions.ts @@ -73,22 +73,64 @@ export function sessionRoutes( const isNotFound = (error: unknown) => /not found|non trovata|inesistente|404/i.test(error instanceof Error ? error.message : String(error)); - /** RLS makes a foreign session indistinguishable from a missing one. */ - const authorize = async (principal: PrincipalContext, id: string, workspace?: string): Promise => { + type LocatedSession = { manifest: any; workspaceConfigPath: string }; + + const workspaceRevisionUnavailable = () => Object.assign( + new Error("workspace revision unavailable"), { code: "workspace_revision_unavailable" }, + ); + + const unavailableWorkspaceReply = (reply: any) => reply.code(409).send({ + error: WORKSPACE_REVISION_UNAVAILABLE_MESSAGE, + code: "workspace_revision_unavailable", + }); + + /** + * Find a session by asking every active registry snapshot, never by using the installation + * default. `tht` applies RLS for the supplied principal, so a foreign ID remains a 404. + */ + const locateSession = async (principal: PrincipalContext, id: string): Promise => { + const runner = runnerFor(principal); + // Dependency-injected runners in legacy route tests may model only the mutation under test. + if (typeof runner.sessionShow !== "function") return { + manifest: {}, workspaceConfigPath: (await d.workspaceRegistry.list())[0]?.snapshotPath ?? "", + }; + const revisions = await d.workspaceRegistry.list(); + for (const revision of revisions) { + if (revision.state !== "operational") continue; + try { + const manifest = await runner.sessionShow(id, revision.snapshotPath); + if (manifest) return { manifest, workspaceConfigPath: revision.snapshotPath }; + } catch (error) { + if (isNotFound(error)) continue; + throw error; + } + } + return undefined; + }; + + /** Read the durable pinned descriptor only after the owner-visible manifest is located. */ + const resolveSessionWorkspace = async (located: LocatedSession): Promise => { + const saved = located.manifest as { workspace_id?: string; workspace_revision?: string }; + if (!saved.workspace_id || !saved.workspace_revision) return located; try { - const runner = runnerFor(principal); - // Dependency-injected runners in legacy route tests may model only the mutation under - // test. Production ThtRunner always exposes sessionShow; keep that test seam harmless. - if (typeof runner.sessionShow !== "function") return {}; - const manifest = await runner.sessionShow(id, workspace); - return manifest ?? undefined; - } catch (error) { - if (isNotFound(error)) return undefined; - throw error; + const pinned = await d.workspaceRegistry.readPinned(saved.workspace_id, saved.workspace_revision); + return { ...located, workspaceConfigPath: pinned.workspaceConfigPath ?? (pinned as any).revision?.snapshotPath }; + } catch { + throw workspaceRevisionUnavailable(); } }; + /** RLS makes a foreign session indistinguishable from a missing one. */ + const authorize = async (principal: PrincipalContext, id: string): Promise => { + const located = await locateSession(principal, id); + return located && await resolveSessionWorkspace(located); + }; + const storageFailure = (reply: any) => reply.code(503).send({ error: "session storage is unavailable" }); + const lifecycleFailure = (reply: any, error: unknown) => + (error as { code?: string } | undefined)?.code === "workspace_revision_unavailable" + ? unavailableWorkspaceReply(reply) + : storageFailure(reply); const releaseIfFinalized = async ( id: string, rt: ReturnType, @@ -323,10 +365,14 @@ export function sessionRoutes( if (scope !== "mine" && scope !== "all") return reply.code(400).send({ error: "scope must be mine or all" }); if (scope === "all" && !principal.isAdmin) return reply.code(403).send({ error: "admin scope required" }); try { - const settings = await d.getSettings(principal); // Admin RLS is deliberately disabled for a normal 'mine' listing. const scopedPrincipal = scope === "mine" ? { ...principal, isAdmin: false } : principal; - const list: SessionRow[] = await runnerFor(scopedPrincipal).sessionList(settings.workspace); + const runner = runnerFor(scopedPrincipal); + const revisions = await d.workspaceRegistry.list(); + const lists = await Promise.all(revisions + .filter((revision) => revision.state === "operational") + .map((revision) => runner.sessionList(revision.snapshotPath) as Promise)); + const list = lists.flat(); // Annotate each row with whether a live Pi runtime is currently bound. The client // opens an `active` session straight into its live view (reconnecting to its pending // gate), while a cold session keeps its explicit Resume affordance — so a mere click @@ -337,21 +383,20 @@ export function sessionRoutes( app.get("/sessions/:id", async (req, reply) => { const principal = getPrincipal(req); try { - const settings = await d.getSettings(principal); - const manifest = await authorize(principal, (req.params as any).id, settings.workspace); - if (!manifest) return reply.code(404).send({ error: "session not found" }); + const session = await authorize(principal, (req.params as any).id); + if (!session) return reply.code(404).send({ error: "session not found" }); + const manifest = session.manifest; return (!manifest.workspace_id || !manifest.workspace_revision) ? { ...manifest, warning: "Legacy session: this session is not pinned to a workspace revision." } : manifest; - } catch { return storageFailure(reply); } + } catch (error) { return lifecycleFailure(reply, error); } }); app.post("/sessions/:id/response", async (req, reply) => { const id = (req.params as any).id; const principal = getPrincipal(req); try { - const settings = await d.getSettings(principal); - if (!await authorize(principal, id, settings.workspace)) return reply.code(404).send({ error: "session not found" }); - } catch { return storageFailure(reply); } + if (!await authorize(principal, id)) return reply.code(404).send({ error: "session not found" }); + } catch (error) { return lifecycleFailure(reply, error); } const rt = d.mgr.get(id); if (!rt) return reply.code(404).send({ error: "sessione non attiva" }); if (!rt.bridge.respond((req.body as any).ui_response)) { @@ -363,9 +408,8 @@ export function sessionRoutes( const id = (req.params as any).id; const principal = getPrincipal(req); try { - const settings = await d.getSettings(principal); - if (!await authorize(principal, id, settings.workspace)) return reply.code(404).send({ error: "session not found" }); - } catch { return storageFailure(reply); } + if (!await authorize(principal, id)) return reply.code(404).send({ error: "session not found" }); + } catch (error) { return lifecycleFailure(reply, error); } const rt = d.mgr.get(id); if (!rt) return reply.code(404).send({ error: "sessione non attiva" }); rt.bridge.steer((req.body as any).text); @@ -376,12 +420,12 @@ export function sessionRoutes( const principal = getPrincipal(req); return withSessionLifecycle(id, async () => { let settings: Settings; - let manifest: any; + let located: LocatedSession | undefined; try { - settings = await d.getSettings(principal); - manifest = await authorize(principal, id, settings.workspace); + located = await locateSession(principal, id); } catch { return storageFailure(reply); } - if (!manifest) return reply.code(404).send({ error: "session not found" }); + if (!located) return reply.code(404).send({ error: "session not found" }); + const manifest = located.manifest; const runner = runnerFor(principal); // Read-only contract FIRST: finalized or archived sessions never attempt compatibility // resolution, even when their historical snapshot was subsequently pruned. @@ -392,21 +436,10 @@ export function sessionRoutes( provider?: string; model?: string; thinking?: string; workspace_id?: string; workspace_revision?: string; }; - let workspaceConfigPath = settings.workspace; - const warning = !saved.workspace_id || !saved.workspace_revision - ? "Legacy session: this session is not pinned to a workspace revision." - : undefined; - if (!warning) { - try { - const pinned = await d.workspaceRegistry.readPinned(saved.workspace_id!, saved.workspace_revision!); - workspaceConfigPath = pinned.workspaceConfigPath ?? (pinned as any).revision?.snapshotPath; - } catch { - return reply.code(409).send({ - error: WORKSPACE_REVISION_UNAVAILABLE_MESSAGE, - code: "workspace_revision_unavailable", - }); - } - } + let workspaceConfigPath: string; + try { workspaceConfigPath = (await resolveSessionWorkspace(located)).workspaceConfigPath; } + catch { return unavailableWorkspaceReply(reply); } + try { settings = await d.getSettings(principal); } catch { return storageFailure(reply); } // This check belongs inside the per-session lock: a preceding cold Resume may have // installed a running runtime while this request was waiting. const existing = d.mgr.get(id); @@ -479,20 +512,20 @@ export function sessionRoutes( const id = (req.params as { id: string }).id; const principal = getPrincipal(req); return withSessionLifecycle(id, async () => { - let settings: Settings; + let session: LocatedSession | undefined; try { - settings = await d.getSettings(principal); - if (!await authorize(principal, id, settings.workspace)) return reply.code(404).send({ error: "session not found" }); - } catch { return storageFailure(reply); } + session = await authorize(principal, id); + if (!session) return reply.code(404).send({ error: "session not found" }); + } catch (error) { return lifecycleFailure(reply, error); } // Invalidate the live generation before persistence can yield. Otherwise its deferred // bootstrap may start Pi while Close is already in progress. const current = d.mgr.get(id); boundRuntimes.delete(id); if (current) d.mgr.teardownIfCurrent(id, current); try { - await runnerFor(principal).closeSession(id, settings.workspace); - } catch { - return storageFailure(reply); + await runnerFor(principal).closeSession(id, session.workspaceConfigPath); + } catch (error) { + return lifecycleFailure(reply, error); } finally { // clear, NOT forget: a closed session can be reopened, and the per-session seq // monotonicity is what keeps a browser's old cursor detectable. The buffer is @@ -506,9 +539,8 @@ export function sessionRoutes( const id = (req.params as any).id; const principal = getPrincipal(req); try { - const settings = await d.getSettings(principal); - if (!await authorize(principal, id, settings.workspace)) return reply.code(404).send({ error: "session not found" }); - } catch { return storageFailure(reply); } + if (!await authorize(principal, id)) return reply.code(404).send({ error: "session not found" }); + } catch (error) { return lifecycleFailure(reply, error); } const rt = d.mgr.get(id); // Add CORS headers manually: reply.raw.writeHead bypasses Fastify's onSend hooks // (where @fastify/cors injects headers), so we must set them explicitly here. @@ -543,58 +575,58 @@ export function sessionRoutes( const id = (req.params as any).id; const principal = getPrincipal(req); try { - const settings = await d.getSettings(principal); - if (!await authorize(principal, id, settings.workspace)) return reply.code(404).send({ error: "session not found" }); - await runnerFor(principal).setName(id, (req.body as any).name, settings.workspace); - } catch { return storageFailure(reply); } + const session = await authorize(principal, id); + if (!session) return reply.code(404).send({ error: "session not found" }); + await runnerFor(principal).setName(id, (req.body as any).name, session.workspaceConfigPath); + } catch (error) { return lifecycleFailure(reply, error); } return reply.code(204).send(); }); app.post("/sessions/:id/group", async (req, reply) => { const id = (req.params as any).id; const principal = getPrincipal(req); try { - const settings = await d.getSettings(principal); - if (!await authorize(principal, id, settings.workspace)) return reply.code(404).send({ error: "session not found" }); - await runnerFor(principal).setGroup(id, (req.body as any).group, settings.workspace); - } catch { return storageFailure(reply); } + const session = await authorize(principal, id); + if (!session) return reply.code(404).send({ error: "session not found" }); + await runnerFor(principal).setGroup(id, (req.body as any).group, session.workspaceConfigPath); + } catch (error) { return lifecycleFailure(reply, error); } return reply.code(204).send(); }); app.post("/sessions/:id/archive", async (req, reply) => { const id = (req.params as any).id; const principal = getPrincipal(req); try { - const settings = await d.getSettings(principal); - if (!await authorize(principal, id, settings.workspace)) return reply.code(404).send({ error: "session not found" }); - await runnerFor(principal).archive(id, settings.workspace); - } catch { return storageFailure(reply); } + const session = await authorize(principal, id); + if (!session) return reply.code(404).send({ error: "session not found" }); + await runnerFor(principal).archive(id, session.workspaceConfigPath); + } catch (error) { return lifecycleFailure(reply, error); } return reply.code(204).send(); }); app.post("/sessions/:id/unarchive", async (req, reply) => { const id = (req.params as any).id; const principal = getPrincipal(req); try { - const settings = await d.getSettings(principal); - if (!await authorize(principal, id, settings.workspace)) return reply.code(404).send({ error: "session not found" }); - await runnerFor(principal).unarchive(id, settings.workspace); - } catch { return storageFailure(reply); } + const session = await authorize(principal, id); + if (!session) return reply.code(404).send({ error: "session not found" }); + await runnerFor(principal).unarchive(id, session.workspaceConfigPath); + } catch (error) { return lifecycleFailure(reply, error); } return reply.code(204).send(); }); app.delete("/sessions/:id", async (req, reply) => { const id = (req.params as any).id; const principal = getPrincipal(req); return withSessionLifecycle(id, async () => { - let settings: Settings; + let session: LocatedSession | undefined; try { - settings = await d.getSettings(principal); - if (!await authorize(principal, id, settings.workspace)) return reply.code(404).send({ error: "session not found" }); - } catch { return storageFailure(reply); } + session = await authorize(principal, id); + if (!session) return reply.code(404).send({ error: "session not found" }); + } catch (error) { return lifecycleFailure(reply, error); } const current = d.mgr.get(id); boundRuntimes.delete(id); if (current) d.mgr.teardownIfCurrent(id, current); try { - await runnerFor(principal).deleteSession(id, settings.workspace); - } catch { - return storageFailure(reply); + await runnerFor(principal).deleteSession(id, session.workspaceConfigPath); + } catch (error) { + return lifecycleFailure(reply, error); } d.hub.forget(id); return reply.code(204).send(); @@ -604,9 +636,9 @@ export function sessionRoutes( const id = (req.params as any).id; const principal = getPrincipal(req); try { - const settings = await d.getSettings(principal); - if (!await authorize(principal, id, settings.workspace)) return reply.code(404).send({ error: "session not found" }); - return await runnerFor(principal).documents(id, settings.workspace); - } catch { return storageFailure(reply); } + const session = await authorize(principal, id); + if (!session) return reply.code(404).send({ error: "session not found" }); + return await runnerFor(principal).documents(id, session.workspaceConfigPath); + } catch (error) { return lifecycleFailure(reply, error); } }); } diff --git a/backend/test/routes-sessions.test.ts b/backend/test/routes-sessions.test.ts index c8cd36ca..fddfe701 100644 --- a/backend/test/routes-sessions.test.ts +++ b/backend/test/routes-sessions.test.ts @@ -11,6 +11,10 @@ const FAKE = path.resolve("../harness/tests/fake_pi/fake_pi_rpc.mjs"); const SCRIPT = path.resolve("../harness/tests/fake_pi/scripts/f1_disambiguation.json"); const defaultWorkspaceRegistry = { + list: vi.fn(async () => [{ + id: "default", commit: "e".repeat(40), blob: "f".repeat(40), + snapshotPath: `/data/workspace-registry/snapshots/${"e".repeat(40)}/default.yaml`, state: "operational", + }]), read: vi.fn(async (id: string) => ({ workspace: { llm_policy: { @@ -25,7 +29,10 @@ const defaultWorkspaceRegistry = { }; function buildApp(config: Parameters[0], deps: Record = {}) { - return buildRealApp(config, { workspaceRegistry: defaultWorkspaceRegistry as any, ...deps } as any); + return buildRealApp(config, { + ...deps, + workspaceRegistry: { ...defaultWorkspaceRegistry, ...(deps.workspaceRegistry as object | undefined) }, + } as any); } function mutApp(thtRunner: any) { @@ -233,6 +240,90 @@ test("creates a session from the configured default workspace revision when work })); }); +test("session lifecycle locates a B session when installation default is A", async () => { + const aPath = "/registry/snapshots/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/a-workspace.yaml"; + const bPath = "/registry/snapshots/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb/b-workspace.yaml"; + const bPinnedPath = "/registry/snapshots/cccccccccccccccccccccccccccccccccccccccc/b-workspace.yaml"; + const bManifest = { + id: "session-b", status: "open", archived: false, + workspace_id: "b-workspace", workspace_revision: "c".repeat(40), + provider: "zai", model: "glm-5.2", thinking: "low", + }; + const calls: string[] = []; + let active: any; + const app = buildApp(loadConfig({ THT_HARNESS_DIR: "../harness" }), { + thtRunner: { + sessionNew: async (input: any) => { + calls.push(`new:${input.workspaceConfigPath}`); + return { id: "session-b" }; + }, + searchPack: async () => {}, + sessionList: async (workspace: string) => { + calls.push(`list:${workspace}`); + return workspace === bPath ? [{ id: "session-b", status: "open", question: "B question" }] : []; + }, + sessionShow: async (id: string, workspace: string) => { + calls.push(`show:${workspace}`); + if (id === "session-b" && workspace === bPath) return bManifest; + throw new Error("session not found"); + }, + reopenSession: async (id: string, workspace: string) => { + calls.push(`reopen:${workspace}`); + expect(id).toBe("session-b"); + }, + } as any, + readiness: { ensure: async () => ({ ok: true }) } as any, + mgr: { + get: () => active, + createFor: () => { + active = { bridge: { onClientEvent: () => {}, respond: () => true, turnState: () => "idle" } }; + return active; + }, + configure: async () => {}, start: () => {}, + teardownForPrincipal: () => [], + teardownIfCurrent: (_id: string, expected: any) => { + if (active !== expected) return false; + active = undefined; + return true; + }, + } as any, + getSettings: () => ({ workspace: "a-workspace", provider: "zai", model: "glm-5.2", thinking: "low" }) as any, + listModels: async () => [{ provider: "zai", id: "glm-5.2", name: "GLM 5.2", reasoning: true }], + workspaceRegistry: { + read: async (id: string) => ({ + workspace: { llm_policy: { allowed: ["zai/glm-5.2"] } }, + revision: { id, commit: "b".repeat(40), blob: "d".repeat(40), snapshotPath: bPath, state: "operational" }, + }), + list: async () => [ + { id: "a-workspace", commit: "a".repeat(40), blob: "a".repeat(40), snapshotPath: aPath, state: "operational" }, + { id: "b-workspace", commit: "b".repeat(40), blob: "b".repeat(40), snapshotPath: bPath, state: "operational" }, + ], + readPinned: vi.fn(async (id: string, revision: string) => { + expect([id, revision]).toEqual(["b-workspace", "c".repeat(40)]); + return { workspace: { llm_policy: { allowed: ["zai/glm-5.2"] } }, workspaceConfigPath: bPinnedPath }; + }), + } as any, + }); + + expect((await app.inject({ method: "POST", url: "/sessions", payload: { + question: "B question", workspaceId: "b-workspace", provider: "zai", model: "glm-5.2", thinking: "low", + } })).statusCode).toBe(200); + expect((await app.inject({ method: "GET", url: "/sessions" })).json()).toEqual([ + expect.objectContaining({ id: "session-b", active: true }), + ]); + expect((await app.inject({ method: "GET", url: "/sessions/session-b" })).json()).toMatchObject(bManifest); + expect((await app.inject({ method: "POST", url: "/sessions/session-b/response", payload: { ui_response: {} } })).statusCode) + .toBe(204); + + active = undefined; + expect((await app.inject({ method: "POST", url: "/sessions/session-b/resume" })).json()) + .toEqual({ id: "session-b", alreadyActive: false }); + expect(calls).toContain(`new:${bPath}`); + expect(calls).toContain(`list:${bPath}`); + expect(calls).toContain(`show:${bPath}`); + expect(calls).toContain(`reopen:${bPinnedPath}`); +}); + test("POST /sessions usa i settings (workspace/provider/model/thinking) e crea+avvia", async () => { const modelKey = path.join(os.tmpdir(), `thoth-model-key-${process.pid}`); writeFileSync(modelKey, "test-model-key", { mode: 0o600 }); @@ -1707,8 +1798,9 @@ test("POST /sessions/:id/rename calls setName", async () => { expect(arg).toEqual({ id: "s1", name: "N" }); }); -test("rename authorizes and mutates through the same selected workspace", async () => { +test("rename authorizes and mutates through the same registry snapshot", async () => { const workspaces: string[] = []; + const tenantPath = "/registry/snapshots/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/tenant-a.yaml"; const app = buildApp(loadConfig({ AUTH_MODE: "upstream", THT_HARNESS_DIR: "../harness" }), { thtRunner: { withPrincipal: () => ({ @@ -1716,7 +1808,11 @@ test("rename authorizes and mutates through the same selected workspace", async setName: async (_id: string, _name: string, workspace: string) => { workspaces.push(`set:${workspace}`); }, }), } as any, - getSettings: () => ({ workspace: "tenant-a" }) as any, + workspaceRegistry: { + list: async () => [{ + id: "tenant-a", commit: "a".repeat(40), blob: "b".repeat(40), snapshotPath: tenantPath, state: "operational", + }], + } as any, }); const res = await app.inject({ method: "POST", url: "/sessions/s1/rename", payload: { name: "N" }, @@ -1725,7 +1821,7 @@ test("rename authorizes and mutates through the same selected workspace", async }, }); expect(res.statusCode).toBe(204); - expect(workspaces).toEqual(["show:tenant-a", "set:tenant-a"]); + expect(workspaces).toEqual([`show:${tenantPath}`, `set:${tenantPath}`]); }); test("POST /sessions/:id/group calls setGroup", async () => {