From 16bae514c105f6c70d95fbbd36724b537b1434ee Mon Sep 17 00:00:00 2001 From: Alex Lavaee Date: Sat, 10 Oct 2026 19:49:39 -0700 Subject: [PATCH 1/4] Make attaching the remote Planner own its document and refuse unattached requests Under HARNESS=remote, attaching a client now makes its account the document's Planner owner through the same generation-guarded claim every owner takes. The owner record names the account's live browser login, since Chopin's repository tools run with that login's GitHub App token and never with the link's bearer; an account without one is refused with sign-in-required (4401). Attaching needs the eligibility every Planner owner already needs, instance admission plus push or administration access, rather than the design's pull access, and the periodic recheck now holds a link to that too. A document has one Planner. Another attach while one is attached is refused with planner-attached (4409), naming the holder's login. A detach or dropped socket keeps the ownership for PLANNER_ATTACH_GRACE_MS (two minutes by default) so the same account can reattach under the same generation; then it is cleared under that generation. Attachments live in the process, and startup already clears every owner reference. Members, MCP callers and research requests no longer claim ownership under this harness. An @chopin message or invoke_planner call for a document with no attached client is refused with planner-not-attached before anything is posted or queued; the message names the document URL and says to run pdd-plan with it as plan_document from a checkout of the repository. The browser shows that text in full on the request, and MCP returns it beside the code. With a client attached, both arrive as turns on its link, and Stop and Resume Planner reach it. Other harnesses are unchanged. Assistant-model: Claude Opus 5.5 (fast) Assistant-workflow: hosted-planner-slices Assistant-duration: 13m converged, estimated unmeasured Assistant-verification: bun run types passed: every workspace package and e2e Assistant-verification: GIT_CONFIG_GLOBAL=/dev/null bun test passed: 4657 pass, 0 fail Assistant-verification: bun run ci passed: dprint, oxlint (0 errors), tokens, design contract, Impeccable Assistant-verification: PostgreSQL contract passed: bun test apps/server/src/storage/postgres against a disposable postgres:17-alpine on port 55432, 83 pass, 0 fail Assistant-verification: mutation check passed: disabling the planner-not-attached refusal in processSend fails the new chat service test Co-authored-by: Alex Lavaee --- AGENTS.md | 7 +- apps/server/src/chat/remote-planner.test.ts | 227 +++++++++++++++++ apps/server/src/chat/service.ts | 21 +- apps/server/src/config.test.ts | 12 + apps/server/src/config.ts | 17 ++ .../conversation-plan/accepted-research.ts | 9 +- apps/server/src/harness/remote/attachments.ts | 240 ++++++++++++++++++ apps/server/src/harness/remote/endpoint.ts | 47 +++- apps/server/src/harness/remote/link.test.ts | 152 ++++++++++- apps/server/src/harness/remote/link.ts | 32 ++- apps/server/src/harness/remote/registry.ts | 15 +- apps/server/src/main.ts | 30 ++- apps/server/src/mcp.ts | 43 +++- apps/server/src/mcp/hosted.test.ts | 24 ++ apps/server/src/mcp/hosted.ts | 19 +- apps/server/src/mcp/invoke.test.ts | 20 ++ apps/server/src/storage/contract.ts | 2 + .../storage/planner-attachment-contract.ts | 168 ++++++++++++ apps/server/src/testing/config.ts | 1 + apps/server/src/testing/planner-link.ts | 117 +++++++-- apps/server/src/wire.ts | 11 +- apps/web/src/chat/references.test.ts | 7 + apps/web/src/chat/references.ts | 1 + apps/web/src/wire.ts | 16 +- docs/architecture.md | 7 +- docs/hosted-agent.md | 67 +++-- docs/local-agent-mcp.md | 21 +- docs/planner-link.md | 102 ++++++-- docs/self-hosting.md | 22 +- packages/planner-link/src/client.ts | 2 +- packages/planner-link/src/protocol.ts | 3 + packages/protocol/index.d.ts | 6 + 32 files changed, 1319 insertions(+), 149 deletions(-) create mode 100644 apps/server/src/chat/remote-planner.test.ts create mode 100644 apps/server/src/harness/remote/attachments.ts create mode 100644 apps/server/src/storage/planner-attachment-contract.ts diff --git a/AGENTS.md b/AGENTS.md index 7347d1556..87d7585ef 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -138,9 +138,12 @@ collaborative state and external implementation runs are durable. summary and research workers stay isolated. There is no flag for this; the harness is the choice. - **`HARNESS=remote` moves the model, not the authority.** Turns run in a client - attached at `/planner-link` with a GitHub bearer and pull access; Chopin's + attached at `/planner-link` with a GitHub bearer and push access; Chopin's host tools, Decisions, and room writes still run in the server. The attached - client sees what a turn sees and steers its document writes. See + client sees what a turn sees and steers its document writes. Attaching makes + the account's live browser login the document's Planner owner, one client per + document; nothing else claims ownership, and Planner requests to a document + without one fail with `planner-not-attached`. See [Hosted agent](docs/hosted-agent.md#remote-planner). - **Repository node IDs are authoritative.** Owner and repository names resolve GitHub requests but never replace the stored node identity. diff --git a/apps/server/src/chat/remote-planner.test.ts b/apps/server/src/chat/remote-planner.test.ts new file mode 100644 index 000000000..90ba2abdc --- /dev/null +++ b/apps/server/src/chat/remote-planner.test.ts @@ -0,0 +1,227 @@ +import { afterEach, expect, test } from "bun:test"; + +import { ActiveOwnerBindings } from "../agent/active-owner"; +import { createPlannerAgent } from "../harness/agents"; +import { createRemoteAdapter } from "../harness/remote/adapter"; +import { notAttachedMessage } from "../harness/remote/attachments"; +import { openPlannerSession } from "../harness/session"; +import * as Plan from "../plan/service"; +import { configured } from "../testing/config"; +import { attach, finishTurn, linkAuth, linkServer, scriptedPlanner } from "../testing/planner-link"; +import * as Chat from "./service"; + +import type { Server } from "bun"; +import type { PlannerLinkClient } from "@chopin/planner-link/client"; +import type { HostedAuth } from "../auth/routes"; +import type { Socket, SocketData } from "../wire"; + +let cleanups: Array<() => unknown> = []; +afterEach(async () => { + for (let cleanup of cleanups.splice(0).toReversed()) await cleanup(); +}); + +const REMOTE = { HARNESS: "remote", BACKGROUND_JOBS: "off" }; +const RIVAL = { id: "U_rival", login: "rival" }; + +function card(status: "running" | "paused") { + return { + id: "run-1", + name: "plan-review", + status, + started: 1, + updated: 1, + stages: [], + waiting: 0, + }; +} + +/** + * A document's Chat under `HARNESS=remote`, served as `main.ts` serves it, + * with a real Planner link server. The rival sends from a browser with a + * login of their own; only the attached member may own the Planner. + */ +async function remoteRoom() { + let { auth, storage, channel, logins } = await linkAuth(); + let server = linkServer(auth); + cleanups.push(server.stop); + let lease = (await storage.leases.acquire("writer", "test", 60_000))!; + let events: Array<{ kind: string; [key: string]: unknown }> = []; + let publisher = { + publish(_topic: string, message: string) { + events.push(JSON.parse(message)); + }, + } as unknown as Server; + let plan = await Plan.open(channel.id, { + storage, + lease: () => lease, + fatal: error => { + throw error; + }, + }, publisher); + let owners = new ActiveOwnerBindings(auth as unknown as HostedAuth); + let url = `https://chopin.test/documents/octo-org/score/${channel.slug}`; + let agent = createPlannerAgent(createRemoteAdapter()); + let context: Chat.Room = { + chat: plan.chat, + plan, + room: channel.id, + server: publisher, + auth: auth as unknown as HostedAuth, + repository: { id: "R_score", owner: "octo-org", name: "score", defaultBranch: "main" }, + config: configured(REMOTE), + claimantSessionId: logins.get("rival"), + plannerNotAttached: async () => + server.attachments.attached(channel.id) + ? undefined + : notAttachedMessage(url, "octo-org/score"), + activeOwner: () => owners.resolve(channel.id), + persist: () => Plan.persist(plan), + openPlannerSession: (binding, options) => + openPlannerSession(binding, options, { + githubTools: async () => ({ ok: true, value: {} }), + createSandbox: async () => + ({ + defaultWorkingDirectory: "/tmp", + run: async () => ({ exitCode: 0, stdout: "", stderr: "" }), + destroy: async () => {}, + }) as never, + registerCredential: () => () => {}, + agent, + }), + }; + cleanups.push(async () => { + owners.revokeAll(); + await Chat.close(plan.chat); + await Plan.close(plan); + }); + let replies: Array<{ kind: string; rid?: string; [key: string]: unknown }> = []; + let ws = { + data: { handle: RIVAL.login, principalId: RIVAL.id, sessionId: logins.get("rival") }, + send(text: string) { + replies.push(JSON.parse(text)); + }, + } as unknown as Socket; + let send = async (text: string, to: "planner" | "room" = "planner") => { + let rid = crypto.randomUUID(); + await Chat.send(context, ws, { + kind: "chat:send", + rid, + requestId: crypto.randomUUID(), + to, + text, + ts: 0, + } as never); + return replies.find(reply => reply.rid === rid); + }; + let owner = async () => + (await storage.channels.readAgent(channel.id, new Date()))?.agent?.ownerSessionId; + return { context, server, channel, logins, url, ws, send, owner, events }; +} + +test("with no attached Planner, @chopin and invoke_planner are refused with how to attach one, and nothing is posted or queued", async () => { + let room = await remoteRoom(); + let { context } = room; + let saves = 0; + let persist = context.persist; + context.persist = async chat => { + saves++; + await persist(chat); + }; + let message = "No Planner is attached to this document. To attach one, run pdd-plan with " + + `plan_document set to ${room.url} from a session in a checkout of octo-org/score; ` + + "that session becomes this document's Planner."; + + expect(await room.send("@chopin draft the plan")).toMatchObject({ + kind: "session:error", + code: "planner-not-attached", + message, + }); + context.chat.busy = true; + expect(await room.send("@chopin and then this")).toMatchObject({ code: "planner-not-attached" }); + context.chat.busy = false; + expect(await Chat.invoke(context, RIVAL, "Draft the plan")).toBe("planner-not-attached"); + expect(context.chat.entries).toEqual([]); + expect(context.chat.waiting).toEqual([]); + expect(saves).toBe(0); + expect(await room.owner()).toBeUndefined(); + + expect(await room.send("Just talking", "room")).toMatchObject({ kind: "chat:send" }); + expect(context.chat.entries.map(entry => entry.text)).toEqual(["Just talking"]); +}); + +test("other harnesses keep claiming ownership for the sender as before", async () => { + let room = await remoteRoom(); + let { context } = room; + context.config = configured({}); + context.plannerNotAttached = undefined; + let started: string[] = []; + context.openPlannerSession = async () => ({ + ok: true, + value: { + stream: async (prompt: string) => { + started.push(prompt); + return { fullStream: (async function*() {})() }; + }, + destroy: async () => {}, + } as never, + }); + expect(await room.send("@chopin draft the plan")).toMatchObject({ kind: "chat:send" }); + await context.chat.running; + expect(started).toHaveLength(1); + expect(await room.owner()).toBe(room.logins.get("rival")); +}); + +test("an attached Planner receives @chopin and invoke_planner turns as their owner, and Stop and Resume reach it", async () => { + let room = await remoteRoom(); + let { context } = room; + let script = scriptedPlanner(); + let controls: Array<{ type: string; run?: string }> = []; + let client: PlannerLinkClient; + client = await attach(room.server.url, room.channel.id, { + ...script.handlers, + async turn(turn) { + script.turns.push(turn.message); + if (turn.message.prompt.includes("Run the review")) { + client.reportRuns(turn.message.session, { + active: ["run-1"], + paused: [], + cards: [card("running")], + }); + } + finishTurn(turn); + }, + control(message) { + controls.push({ type: message.type, run: message.run }); + client.reportRuns( + message.session, + message.type === "stop" + ? { active: [], paused: ["run-1"], cards: [card("paused")] } + : { active: ["run-1"], paused: [], cards: [card("running")] }, + ); + }, + }); + cleanups.push(() => client.detach()); + expect(await room.owner()).toBe(room.logins.get("member")); + + expect(await room.send("@chopin draft the plan")).toMatchObject({ kind: "chat:send" }); + await context.chat.running; + expect(await Chat.invoke(context, RIVAL, "Run the review")).toBeUndefined(); + await context.chat.running; + expect(script.turns.map(turn => turn.prompt)).toEqual([ + expect.stringContaining("draft the plan"), + expect.stringContaining("Run the review"), + ]); + expect(await room.owner()).toBe(room.logins.get("member")); + expect(context.chat.retained).toBeDefined(); + + await Chat.abort(context, room.ws); + await Bun.sleep(20); + await Chat.resume(context, room.ws); + expect(controls).toEqual([{ type: "stop", run: undefined }, { type: "resume", run: undefined }]); + expect(context.chat.entries.map(entry => entry.text)).toContain( + "@rival stopped the Planner and paused its workflows.", + ); + expect(context.chat.entries.map(entry => entry.text)).toContain( + "@rival resumed the Planner's workflows.", + ); +}); diff --git a/apps/server/src/chat/service.ts b/apps/server/src/chat/service.ts index f35714372..eb4e0f79d 100644 --- a/apps/server/src/chat/service.ts +++ b/apps/server/src/chat/service.ts @@ -533,6 +533,12 @@ export type Room = { */ claimantSessionId: string | undefined; repository: HostedRepository; + /** + * Under `HARNESS=remote`, how to attach a Planner when none is attached, or + * undefined while one is. Planner requests are refused before anything is + * posted or queued, and only an attachment takes Planner ownership. + */ + plannerNotAttached?: () => Promise; activeOwner?: () => Promise; persist: (chat?: () => Pick) => Promise; commitRoomMessage?: (entry: Wire.Entry) => Promise; @@ -596,6 +602,7 @@ export async function invoke( checkout?: string, ): Promise< | "planner-unavailable" + | "planner-not-attached" | "planner-owner-unavailable" | "planner-queue-full" | "checkout-unverified" @@ -606,13 +613,14 @@ export async function invoke( chat.pendingSends++; let post = async () => { if (!context.config.agent || chat.closed) return "planner-unavailable" as const; + if (await context.plannerNotAttached?.()) return "planner-not-attached" as const; let verified: string | undefined; if (checkout !== undefined && context.config.harness === "atomic") { verified = await verifiedCheckout(context.repository, checkout); if (!verified) return "checkout-unverified" as const; } try { - await resolveOwner(context.auth, context.repository, room, context.claimantSessionId); + await resolveOwner(context.auth, context.repository, room, claimant(context)); } catch { return "planner-owner-unavailable" as const; } @@ -774,6 +782,10 @@ async function processSend(context: Room, ws: Socket, msg: Request): return; } + let unattached = await context.plannerNotAttached?.(); + if (unattached) return fail(ws, msg.rid, unattached, "planner-not-attached"); + if (chat.closed) return fail(ws, msg.rid, "chat is closed"); + if (chat.busy) { if (chat.waiting.length >= MAX_QUEUE) { say(chat, server, room, { @@ -1621,7 +1633,7 @@ async function repositorySession( context.auth, context.repository, context.room, - claimantSessionId, + context.plannerNotAttached ? undefined : claimantSessionId, ); if (context.ownerAvailable) void context.ownerAvailable().catch(() => {}); let { chat, auth } = context; @@ -1706,6 +1718,11 @@ async function repositorySession( } } +/** The login that may claim Planner ownership for the room; none where only an attachment may. */ +function claimant(context: Room): string | undefined { + return context.plannerNotAttached ? undefined : context.claimantSessionId; +} + /** * The channel's Planner owner, claimed for the claimant when it has none. * Without a claimant only an owner the channel already has will do. diff --git a/apps/server/src/config.test.ts b/apps/server/src/config.test.ts index 92a9ec59f..25762ca70 100644 --- a/apps/server/src/config.test.ts +++ b/apps/server/src/config.test.ts @@ -114,6 +114,18 @@ describe("configuration", () => { .toThrow("HARNESS=remote requires BACKGROUND_JOBS=off"); }); + it("keeps a detached remote Planner's ownership two minutes unless PLANNER_ATTACH_GRACE_MS says otherwise", () => { + let remote = { HARNESS: "remote", BACKGROUND_JOBS: "off" }; + expect(configured(remote).plannerAttachGraceMs).toBe(120_000); + expect(configured({ ...remote, PLANNER_ATTACH_GRACE_MS: "0" }).plannerAttachGraceMs).toBe(0); + expect(configured({ ...remote, PLANNER_ATTACH_GRACE_MS: "3600000" }).plannerAttachGraceMs) + .toBe(3_600_000); + for (let value of ["", "-1", "1.5", "3600001", "soon"]) { + expect(() => configured({ ...remote, PLANNER_ATTACH_GRACE_MS: value })) + .toThrow("PLANNER_ATTACH_GRACE_MS must be an integer between 0 and 3600000 milliseconds"); + } + }); + it("chooses a full Atomic Planner by harness alone, in hosted and local configuration", () => { let atomic = { HARNESS: "atomic", HARNESS_AUTH: "ai-gateway", MODEL: "stub/model" }; for (let mode of [{}, LOCAL]) { diff --git a/apps/server/src/config.ts b/apps/server/src/config.ts index d510778b2..0424b7637 100644 --- a/apps/server/src/config.ts +++ b/apps/server/src/config.ts @@ -25,6 +25,11 @@ export type Config = { * passed to the CLI's `--extension`. Split on the platform path delimiter. */ harnessExtensions: string[]; + /** + * Under `HARNESS=remote`, how long a detached or dropped Planner link keeps + * the document's Planner ownership for the same account to reattach. + */ + plannerAttachGraceMs: number; /** * Whether to run the agent at all. * @@ -59,6 +64,7 @@ export type Config = { }; const DEFAULT_PORT = 8787; +const DEFAULT_ATTACH_GRACE_MS = 120_000; const DEFAULT_MODEL = "gpt-6-luna"; /** The default is a Copilot model ID. Pi and Atomic resolve IDs against their @@ -139,6 +145,16 @@ export function load(): Config { ) { throw new Error("JEV_MODEL must be a valid model alias"); } + let graceRaw = process.env.PLANNER_ATTACH_GRACE_MS; + let plannerAttachGraceMs = graceRaw === undefined ? DEFAULT_ATTACH_GRACE_MS : Number(graceRaw); + if ( + (graceRaw !== undefined && !/^\d+$/.test(graceRaw)) + || !Number.isSafeInteger(plannerAttachGraceMs) || plannerAttachGraceMs > 3_600_000 + ) { + throw new Error( + "PLANNER_ATTACH_GRACE_MS must be an integer between 0 and 3600000 milliseconds", + ); + } let selection = harnessSelection(); if (selection.harness === "remote" && backgroundJobs) { throw new Error( @@ -150,6 +166,7 @@ export function load(): Config { ...selection, port: serverPort, model: model(selection.harness), + plannerAttachGraceMs, agent, backgroundJobs, webResearch: agent && backgroundJobs && process.env.WEB_RESEARCH !== "off", diff --git a/apps/server/src/conversation-plan/accepted-research.ts b/apps/server/src/conversation-plan/accepted-research.ts index 136f63f50..0bf0861ea 100644 --- a/apps/server/src/conversation-plan/accepted-research.ts +++ b/apps/server/src/conversation-plan/accepted-research.ts @@ -13,6 +13,8 @@ type AcceptedResearchDeps = { refreshAccess: (ws: Socket, force: boolean) => Promise; unavailable: (id: string) => boolean; ownerAvailable: (id: string) => Promise; + /** False under `HARNESS=remote`, where only an attached Planner takes ownership. */ + claimsOwnership?: boolean; placeReference: (channelId: string, workspaceId: string) => Promise<"placed" | "deferred">; scheduleRecovery: (deferred: number) => void; }; @@ -44,7 +46,12 @@ export async function startAcceptedResearch( || deps.unavailable(room.id) ) return false; try { - await Chat.resolveOwner(deps.auth, repository, room.id, ws.data.sessionId); + await Chat.resolveOwner( + deps.auth, + repository, + room.id, + deps.claimsOwnership === false ? undefined : ws.data.sessionId, + ); return true; } catch { return false; diff --git a/apps/server/src/harness/remote/attachments.ts b/apps/server/src/harness/remote/attachments.ts new file mode 100644 index 000000000..58bf7943d --- /dev/null +++ b/apps/server/src/harness/remote/attachments.ts @@ -0,0 +1,240 @@ +/** + * Which client is each document's remote Planner. + * + * Attaching makes the client's account the document's Planner owner through + * the same generation-guarded storage claim a browser owner takes elsewhere. + * The owner is the account's live browser login, because Chopin's repository + * tools act with that login's GitHub App token, never with the link's bearer. + * A dropped or detached link keeps its ownership for a grace period, so the + * same account can reattach without losing it; then the ownership is cleared. + */ + +import { GitHubError } from "../../github/client"; +import { attachLink } from "./registry"; + +import type { Admission } from "../../auth/admission"; +import type { AuthConfig } from "../../auth/config"; +import type { AuthenticatedSession, Sessions } from "../../auth/session"; +import type { GitHub } from "../../github/client"; +import type { StorageAdapter } from "../../storage/port"; +import type { PlannerLink } from "./link"; + +export type AttachmentAuth = { + config: Pick; + storage: Pick; + github: Pick; + admission: Pick; + sessions: Pick; + clock: () => Date; +}; + +export type AttachRequest = { + channelId: string; + repository: { id: string; owner: string; name: string }; + user: { id: string; login: string }; +}; + +export type AttachRefusal = { + ok: false; + code: "planner-attached" | "sign-in-required" | "repository-forbidden"; + message: string; +}; + +export type AttachGrant = { ok: true; detach: () => void }; + +type Hold = { + channelId: string; + userId: string; + login: string; + ownerSessionId: string; + generation: number; + link?: PlannerLink; + forget?: () => void; + timer?: ReturnType; + releaseAt?: Date; +}; + +/** What `@chopin` and `invoke_planner` say when no Planner is attached to the document. */ +export function notAttachedMessage(url: string, repository: string): string { + return `No Planner is attached to this document. To attach one, run pdd-plan with ` + + `plan_document set to ${url} from a session in a checkout of ${repository}; ` + + `that session becomes this document's Planner.`; +} + +export function forbiddenMessage(repository: string): string { + return `Attaching as this document's Planner needs push or administration access to ${repository}.`; +} + +export class PlannerAttachments { + #auth: AttachmentAuth; + #graceMs: number; + #holds = new Map(); + #queues = new Map>(); + + constructor(auth: AttachmentAuth, graceMs: number) { + this.#auth = auth; + this.#graceMs = graceMs; + } + + /** Whether a client is attached to the document now, not merely holding it through a grace period. */ + attached(channelId: string): boolean { + return this.#holds.get(channelId)?.link !== undefined; + } + + /** The GitHub login holding the document's Planner, attached or within its grace period. */ + holder(channelId: string): string | undefined { + return this.#holds.get(channelId)?.login; + } + + /** Makes `link` the document's Planner and its account the owner, or says why not. */ + attach(request: AttachRequest, link: PlannerLink): Promise { + return this.#serialized(request.channelId, () => this.#attach(request, link)); + } + + /** Stops every grace timer. Ownership stays in storage, which startup clears. */ + close(): void { + for (let hold of this.#holds.values()) clearTimeout(hold.timer); + this.#holds.clear(); + } + + async #attach(request: AttachRequest, link: PlannerLink): Promise { + let { channelId, user } = request; + let held = this.#holds.get(channelId); + if (held && (held.link || held.userId !== user.id)) return attachedRefusal(held); + let login = await this.#eligibleLogin(request); + if ("ok" in login) return login; + let now = this.#auth.clock(); + if (held) { + let stored = await this.#auth.storage.channels.readAgent(channelId, now); + if ( + stored?.agent?.ownerSessionId === held.ownerSessionId + && stored.agent.generation === held.generation + ) return this.#bind(held, link); + this.#drop(held); + } + let claimed = await this.#claim(channelId, login.session.id, now); + if (!claimed) throw new Error("The document's Planner owner changed while attaching."); + let hold: Hold = { + channelId, + userId: user.id, + login: user.login, + ownerSessionId: login.session.id, + generation: claimed.generation, + }; + this.#holds.set(channelId, hold); + return this.#bind(hold, link); + } + + async #eligibleLogin(request: AttachRequest): Promise { + let { repository, user } = request; + let fullName = `${repository.owner}/${repository.name}`; + let login = await this.#auth.sessions.forUser(user.id); + if (!login) return signInRefusal(this.#auth.config.origin, user.login); + let checked; + try { + checked = await this.#auth.sessions.use( + login, + token => this.#auth.github.repositoryAccess(token, repository.owner, repository.name), + ); + } catch (error) { + if (error instanceof GitHubError && error.status === 401) { + return signInRefusal(this.#auth.config.origin, user.login); + } + throw error; + } + let access = checked.value; + if ( + !access || access.id !== repository.id + || (!access.permissions.push && !access.permissions.admin) + || !await this.#auth.admission.allowed(checked.authenticated.access.token, user.id) + ) return { ok: false, code: "repository-forbidden", message: forbiddenMessage(fullName) }; + return checked.authenticated; + } + + /** + * Claims the document for `sessionId`. Under the remote Planner only an + * attachment takes ownership, so an owner no attachment holds is stale and + * is cleared, guarded by its generation, before claiming again. + */ + async #claim(channelId: string, sessionId: string, now: Date) { + let channels = this.#auth.storage.channels; + let state = await channels.claimAgentOwner(channelId, sessionId, now); + if (state.ownerSessionId === sessionId) return state; + if (state.ownerSessionId) { + await channels.clearAgentOwner(channelId, state.ownerSessionId, state.generation, now); + } + state = await channels.claimAgentOwner(channelId, sessionId, now); + return state.ownerSessionId === sessionId ? state : undefined; + } + + #bind(hold: Hold, link: PlannerLink): AttachGrant { + clearTimeout(hold.timer); + hold.timer = undefined; + hold.releaseAt = undefined; + hold.link = link; + hold.forget = attachLink(hold.channelId, link); + return { ok: true, detach: () => this.#detached(hold, link) }; + } + + #detached(hold: Hold, link: PlannerLink): void { + if (hold.link !== link) return; + hold.link = undefined; + hold.forget?.(); + hold.forget = undefined; + if (this.#holds.get(hold.channelId) !== hold) return; + hold.releaseAt = new Date(this.#auth.clock().getTime() + this.#graceMs); + hold.timer = setTimeout(() => void this.#expire(hold), this.#graceMs); + hold.timer.unref?.(); + } + + #expire(hold: Hold): Promise { + return this.#serialized(hold.channelId, async () => { + if (this.#holds.get(hold.channelId) !== hold || hold.link) return; + this.#drop(hold); + try { + await this.#auth.storage.channels.clearAgentOwner( + hold.channelId, + hold.ownerSessionId, + hold.generation, + this.#auth.clock(), + ); + } catch (error) { + console.error("[planner-link] releasing the Planner owner failed:", error); + } + }); + } + + #drop(hold: Hold): void { + clearTimeout(hold.timer); + hold.timer = undefined; + if (this.#holds.get(hold.channelId) === hold) this.#holds.delete(hold.channelId); + } + + #serialized(channelId: string, action: () => Promise): Promise { + let previous = this.#queues.get(channelId) ?? Promise.resolve(); + let next = previous.then(action); + let settled = next.then(() => {}, () => {}); + this.#queues.set(channelId, settled); + void settled.then(() => { + if (this.#queues.get(channelId) === settled) this.#queues.delete(channelId); + }); + return next; + } +} + +function attachedRefusal(hold: Hold): AttachRefusal { + let message = hold.link || !hold.releaseAt + ? `${hold.login} is attached as this document's Planner. A document has one Planner at a time.` + : `${hold.login} detached from this document's Planner and holds it until ` + + `${hold.releaseAt.toISOString()} unless they reattach. A document has one Planner at a time.`; + return { ok: false, code: "planner-attached", message }; +} + +function signInRefusal(origin: string, login: string): AttachRefusal { + return { + ok: false, + code: "sign-in-required", + message: `Sign in to Chopin at ${origin} in a browser as ${login}, then attach again. ` + + `The Planner's repository tools act with that sign-in.`, + }; +} diff --git a/apps/server/src/harness/remote/endpoint.ts b/apps/server/src/harness/remote/endpoint.ts index 90cccf450..de2891c23 100644 --- a/apps/server/src/harness/remote/endpoint.ts +++ b/apps/server/src/harness/remote/endpoint.ts @@ -2,24 +2,30 @@ * `/planner-link`: where a client attaches as a document's Planner. * * The bearer is a GitHub token checked as hosted MCP checks it, before the - * socket upgrades. The `attach` that follows names a document its account must - * be able to read. Nothing is posted to the document by attaching or refusing. + * socket upgrades. The `attach` that follows names a document whose repository + * its account can push to or administer, and makes that account the document's + * Planner owner. Nothing is posted to the document by attaching or refusing. */ import { AdmissionDenied } from "../../auth/admission"; import { documentUrl } from "../../channels/document-url"; import { GitHubError } from "../../github/client"; import { locateDocument } from "../../mcp/hosted"; +import { forbiddenMessage } from "./attachments"; import { PlannerLink } from "./link"; -import { attachLink } from "./registry"; import type { ServerWebSocket } from "bun"; import type { HostedAuth } from "../../auth/routes"; import type { HostedCaller } from "../../mcp/hosted"; +import type { PlannerAttachments } from "./attachments"; import type { AttachOutcome, LinkOptions, LinkSocket } from "./link"; export type PlannerLinkAuth = Pick; +function writable(repository: { permissions: { push: boolean; admin: boolean } }): boolean { + return repository.permissions.push || repository.permissions.admin; +} + export type PlannerLinkSocketData = { plannerLink: true; sidebar?: undefined; @@ -28,8 +34,11 @@ export type PlannerLinkSocketData = { }; export type PlannerLinkHooks = { + /** One Planner per document, and its ownership. */ + attachments: PlannerAttachments; /** A document being deleted cannot be attached. */ unavailable?: (channelId: string) => boolean; + /** Observes attached links. */ register?: LinkOptions["register"]; timeoutMs?: number; revalidateEveryMs?: number; @@ -84,14 +93,15 @@ export async function admitPlannerLink( } } -/** A link for one admitted caller, attaching only documents the caller can read. */ +/** A link for one admitted caller, attaching only documents whose repository it can write. */ export function plannerLink( caller: HostedCaller, auth: PlannerLinkAuth, socket: LinkSocket, - hooks: PlannerLinkHooks = {}, + hooks: PlannerLinkHooks, ): PlannerLink { let channelId: string | undefined; + let link: PlannerLink; async function attach(message: { document: string }): Promise { let located = await locateDocument(auth, caller, message.document); if (located === "forbidden") { @@ -112,15 +122,22 @@ export function plannerLink( if (channel.archivedAt) { return { ok: false, code: "document-archived", message: "This document is archived." }; } + let fullName = `${repository.owner}/${repository.name}`; + if (!writable(repository)) { + return { ok: false, code: "repository-forbidden", message: forbiddenMessage(fullName) }; + } + let url = new URL(await documentUrl(channel, auth.storage.channels), auth.config.origin).href; + let granted = await hooks.attachments.attach({ + channelId: channel.id, + repository: { id: repository.id, owner: repository.owner, name: repository.name }, + user: { id: caller.user.id, login: caller.user.login }, + }, link); + if (!granted.ok) return granted; channelId = channel.id; return { ok: true, - document: { - id: channel.id, - title: channel.title, - repository: `${repository.owner}/${repository.name}`, - url: new URL(await documentUrl(channel, auth.storage.channels), auth.config.origin).href, - }, + document: { id: channel.id, title: channel.title, repository: fullName, url }, + detach: granted.detach, }; } async function revalidate(): Promise { @@ -129,19 +146,21 @@ export function plannerLink( await auth.admission.user(caller.oauthToken); let located = await locateDocument(auth, caller, channelId); return typeof located === "object" && located.channel.id === channelId + && writable(located.repository) && !located.channel.archivedAt && !hooks.unavailable?.(channelId); } catch (err) { if (revoked(err)) return false; throw err; } } - return new PlannerLink(socket, { + link = new PlannerLink(socket, { attach, revalidate, - register: hooks.register ?? attachLink, + register: hooks.register, timeoutMs: hooks.timeoutMs, revalidateEveryMs: hooks.revalidateEveryMs, }); + return link; } export function isPlannerLinkSocket( @@ -153,7 +172,7 @@ export function isPlannerLinkSocket( export function openPlannerLinkSocket( ws: ServerWebSocket, auth: PlannerLinkAuth, - hooks: PlannerLinkHooks = {}, + hooks: PlannerLinkHooks, ): void { ws.data.link = plannerLink(ws.data.caller, auth, { send: text => void ws.send(text), diff --git a/apps/server/src/harness/remote/link.test.ts b/apps/server/src/harness/remote/link.test.ts index 2120217cb..8f3427c0e 100644 --- a/apps/server/src/harness/remote/link.test.ts +++ b/apps/server/src/harness/remote/link.test.ts @@ -16,12 +16,15 @@ import { attach, DENIED_TOKEN, finishTurn, + GUEST_TOKEN, linkAuth, linkServer, + MEMBER_TOKEN, OUTSIDER_TOKEN, rawLink, - READER_TOKEN, + RIVAL_TOKEN, scriptedPlanner, + VIEWER_TOKEN, } from "../../testing/planner-link"; import { createRemoteAdapter } from "./adapter"; import { admitPlannerLink } from "./endpoint"; @@ -145,7 +148,7 @@ describe("Planner link admission", () => { .toBe(403); expect( (await upgradeRequest(server.url, { - authorization: `Bearer ${READER_TOKEN}`, + authorization: `Bearer ${MEMBER_TOKEN}`, origin: "https://elsewhere.test", })).status, ).toBe(403); @@ -157,7 +160,7 @@ describe("Planner link admission", () => { it("answers 404 where the server does not run the remote Planner", async () => { let { auth } = await linkAuth(); let request = new Request("https://chopin.test/planner-link", { - headers: { authorization: `Bearer ${READER_TOKEN}`, upgrade: "websocket" }, + headers: { authorization: `Bearer ${MEMBER_TOKEN}`, upgrade: "websocket" }, }); let refused = await admitPlannerLink(request, auth, false); expect(refused).toBeInstanceOf(Response); @@ -174,18 +177,18 @@ describe("Planner link admission", () => { (error: PlannerLinkRefused) => error.code, ); expect(await refusal(OUTSIDER_TOKEN, channel.id)).toBe("repository-forbidden"); - expect(await refusal(READER_TOKEN, crypto.randomUUID())).toBe("document-unavailable"); + expect(await refusal(MEMBER_TOKEN, crypto.randomUUID())).toBe("document-unavailable"); let raw = await rawLink(server.url); raw.send({ type: "attach", version: PLANNER_LINK_VERSION + 1, document: channel.id }); expect(await raw.next("refused")).toMatchObject({ code: "unsupported-version" }); expect((await raw.closed).code).toBe(LINK_CLOSE.unsupportedVersion); expect(await storage.channels.get(channel.id)).toEqual(before); await storage.channels.archive({ id: channel.id, now: new Date() }); - expect(await refusal(READER_TOKEN, channel.id)).toBe("document-archived"); + expect(await refusal(MEMBER_TOKEN, channel.id)).toBe("document-archived"); expect(server.links).toEqual([]); }); - it("attaches a reader to the document by id or URL", async () => { + it("attaches a member to the document by id or URL", async () => { let { server, channel } = await setup(); let client = await attach(server.url, channel.id, scriptedPlanner().handlers); expect(client.document).toEqual({ @@ -194,11 +197,136 @@ describe("Planner link admission", () => { repository: "octo-org/score", url: `https://chopin.test/documents/octo-org/score/${channel.slug}`, }); + await client.detach(); let byUrl = await attach(server.url, client.document.url!, scriptedPlanner().handlers); expect(byUrl.document.id).toBe(channel.id); - expect(server.links.map(link => link.channelId)).toEqual([channel.id, channel.id]); + expect(server.links.map(link => link.channelId)).toEqual([channel.id]); await byUrl.detach(); - await client.detach(); + expect(server.links).toEqual([]); + }); +}); + +describe("Planner link ownership", () => { + /** The code, message, and close code an attach is refused with. */ + async function refused(url: string, document: string, token: string) { + let raw = await rawLink(url, token); + raw.send({ type: "attach", version: PLANNER_LINK_VERSION, document }); + let refusal = await raw.next("refused"); + return { code: refusal.code, message: refusal.message, close: (await raw.closed).code }; + } + + async function owner( + storage: Awaited>["storage"], + channelId: string, + ) { + return (await storage.channels.readAgent(channelId, new Date()))?.agent; + } + + it("makes the attaching member the owner through their browser login, replacing an unattached owner", async () => { + let { auth, storage, channel, logins } = await linkAuth(); + let server = linkServer(auth); + cleanups.push(server.stop); + let stale = await storage.channels.claimAgentOwner( + channel.id, + logins.get("rival")!, + new Date(), + ); + expect(stale).toMatchObject({ ownerSessionId: logins.get("rival"), generation: 1 }); + let client = await attach(server.url, channel.id, scriptedPlanner().handlers); + cleanups.push(() => client.detach()); + expect(await owner(storage, channel.id)) + .toMatchObject({ ownerSessionId: logins.get("member"), generation: 2, status: "ready" }); + expect(server.attachments.attached(channel.id)).toBe(true); + expect(server.attachments.holder(channel.id)).toBe("member"); + }); + + it("refuses a second attach while a Planner is attached, naming the holder", async () => { + let { auth, storage, channel, logins } = await linkAuth(); + let server = linkServer(auth); + cleanups.push(server.stop); + let client = await attach(server.url, channel.id, scriptedPlanner().handlers); + cleanups.push(() => client.detach()); + let held = await owner(storage, channel.id); + expect(held).toMatchObject({ ownerSessionId: logins.get("member"), generation: 1 }); + for (let token of [RIVAL_TOKEN, MEMBER_TOKEN]) { + expect(await refused(server.url, channel.id, token)).toEqual({ + code: "planner-attached", + message: "member is attached as this document's Planner. " + + "A document has one Planner at a time.", + close: LINK_CLOSE.attached, + }); + } + expect(await owner(storage, channel.id)).toEqual(held); + expect(server.links).toHaveLength(1); + }); + + it("refuses an account that cannot push or has no browser login, and takes no ownership", async () => { + let { auth, storage, channel } = await linkAuth(); + let server = linkServer(auth); + cleanups.push(server.stop); + expect(await refused(server.url, channel.id, VIEWER_TOKEN)).toEqual({ + code: "repository-forbidden", + message: "Attaching as this document's Planner needs push or administration access to " + + "octo-org/score.", + close: LINK_CLOSE.forbidden, + }); + expect(await refused(server.url, channel.id, GUEST_TOKEN)).toEqual({ + code: "sign-in-required", + message: "Sign in to Chopin at https://chopin.test in a browser as guest, then attach " + + "again. The Planner's repository tools act with that sign-in.", + close: LINK_CLOSE.unauthorized, + }); + expect((await owner(storage, channel.id))?.ownerSessionId).toBeUndefined(); + expect(server.links).toEqual([]); + }); + + it("keeps ownership through the grace period for the same member, then releases it", async () => { + let { auth, storage, channel, logins } = await linkAuth(); + let server = linkServer(auth, 2_000, undefined, 400); + cleanups.push(server.stop); + let first = await attach(server.url, channel.id, scriptedPlanner().handlers); + await first.detach(); + expect(await owner(storage, channel.id)) + .toMatchObject({ ownerSessionId: logins.get("member"), generation: 1 }); + let refusal = await refused(server.url, channel.id, RIVAL_TOKEN); + expect(refusal).toMatchObject({ code: "planner-attached", close: LINK_CLOSE.attached }); + expect(refusal.message).toStartWith( + "member detached from this document's Planner and holds it until ", + ); + + let dropped = await rawLink(server.url); + dropped.send({ type: "attach", version: PLANNER_LINK_VERSION, document: channel.id }); + await dropped.next("attached"); + expect(await owner(storage, channel.id)) + .toMatchObject({ ownerSessionId: logins.get("member"), generation: 1 }); + dropped.socket.close(); + await dropped.closed; + await Bun.sleep(100); + expect((await owner(storage, channel.id))?.ownerSessionId).toBe(logins.get("member")); + for (let deadline = Date.now() + 3_000; Date.now() < deadline;) { + if (!(await owner(storage, channel.id))?.ownerSessionId) break; + await Bun.sleep(25); + } + expect(await owner(storage, channel.id)) + .toMatchObject({ ownerSessionId: undefined, generation: 1, status: "unavailable" }); + + let rival = await attach(server.url, channel.id, scriptedPlanner().handlers, RIVAL_TOKEN); + cleanups.push(() => rival.detach()); + expect(await owner(storage, channel.id)) + .toMatchObject({ ownerSessionId: logins.get("rival"), generation: 2 }); + }); + + it("detaches a link whose account loses push access while it stays attached", async () => { + let { auth, channel } = await linkAuth(); + let server = linkServer(auth, 300, 20); + cleanups.push(server.stop); + let client = await attach(server.url, channel.id, scriptedPlanner().handlers); + let repository = auth.github.repository; + auth.github.repository = async (token, owner, name) => ({ + ...await repository(token, owner, name), + permissions: { pull: true, push: false, admin: false }, + }); + expect(await client.closed).toEqual({ code: LINK_CLOSE.forbidden, reason: "access-revoked" }); expect(server.links).toEqual([]); }); }); @@ -416,7 +544,7 @@ describe("Planner link protocol", () => { throw new GitHubError("Not Found", 404); }; await expect(harness.doStart({ sessionId: "second", sandboxSession, sessionWorkDir: "/tmp" })) - .rejects.toThrow("can no longer read this document"); + .rejects.toThrow("can no longer act for this document"); expect(server.links).toEqual([]); }); @@ -430,7 +558,7 @@ describe("Planner link protocol", () => { throw new GitHubError("Not Found", 404); }; let revoked = await streamOn(agent, session, "second"); - expect(revoked.errors.join("\n")).toContain("can no longer read this document"); + expect(revoked.errors.join("\n")).toContain("can no longer act for this document"); expect(server.links).toEqual([]); }); @@ -457,7 +585,7 @@ describe("Planner link protocol", () => { throw new GitHubError("Bad credentials", 401); }; let revoked = await streamOn(agent, session, "second"); - expect(revoked.errors.join("\n")).toContain("can no longer read this document"); + expect(revoked.errors.join("\n")).toContain("can no longer act for this document"); expect(await client.closed).toEqual({ code: LINK_CLOSE.forbidden, reason: "access-revoked" }); expect(server.links).toEqual([]); }); @@ -527,6 +655,8 @@ describe("Planner link protocol", () => { })(); let outcome = await streamTurn(harness, "reuse"); await driving; + raw.socket.close(); + await raw.closed; return outcome.errors.join("\n"); } readPlanRuns = 0; diff --git a/apps/server/src/harness/remote/link.ts b/apps/server/src/harness/remote/link.ts index 80679f2a2..5a299e1d7 100644 --- a/apps/server/src/harness/remote/link.ts +++ b/apps/server/src/harness/remote/link.ts @@ -44,11 +44,16 @@ export type LinkSocket = { }; export type AttachOutcome = - | { ok: true; document: ServerMessageOf<"attached">["document"] } + | { + ok: true; + document: ServerMessageOf<"attached">["document"]; + /** Lets go of what attaching took, such as the document's Planner ownership. */ + detach?: () => void; + } | { ok: false; code: RefusalCode; message: string }; export type LinkOptions = { - /** Resolves the document the client asked for, refusing what its caller cannot read. */ + /** Resolves the document the client asked for and takes its Planner, or refuses. */ attach(message: ClientMessageOf<"attach">): Promise; /** Rechecks the caller's access; false closes the link. */ revalidate?(): Promise; @@ -65,6 +70,8 @@ const REFUSAL_CLOSE: Record = { "access-revoked": LINK_CLOSE.forbidden, "document-unavailable": LINK_CLOSE.unavailable, "document-archived": LINK_CLOSE.unavailable, + "planner-attached": LINK_CLOSE.attached, + "sign-in-required": LINK_CLOSE.unauthorized, unavailable: LINK_CLOSE.failed, }; @@ -601,11 +608,14 @@ export class PlannerLink { } } - /** Closes the link with `access-revoked` and throws when the caller can no longer read the document. */ + /** Closes the link with `access-revoked` and throws when the caller may no longer be the Planner. */ async #recheck(): Promise { if (this.#options.revalidate && !await this.#options.revalidate()) { - this.#refuse("access-revoked", "The attached account can no longer read this document."); - throw new Error("The attached Planner can no longer read this document."); + this.#refuse( + "access-revoked", + "The attached account can no longer act as this document's Planner.", + ); + throw new Error("The attached Planner can no longer act for this document."); } if (this.#state !== "attached") throw new Error("The Planner link closed."); } @@ -693,12 +703,20 @@ export class PlannerLink { console.error("[planner-link] attach failed:", error); outcome = { ok: false, code: "unavailable", message: "The document could not be checked." }; } - if (this.#state !== "attaching") return; + if (this.#state !== "attaching") { + if (outcome.ok) outcome.detach?.(); + return; + } if (!outcome.ok) return this.#refuse(outcome.code, outcome.message); this.#channelId = outcome.document.id; this.#state = "attached"; this.#send({ type: "attached", version: PLANNER_LINK_VERSION, document: outcome.document }); - this.#forget = this.#options.register?.(outcome.document.id, this); + let unregister = this.#options.register?.(outcome.document.id, this); + let detach = outcome.detach; + this.#forget = () => { + unregister?.(); + detach?.(); + }; this.#watch = setInterval( () => void this.#recheckAttached(), this.#options.revalidateEveryMs ?? REVALIDATE_EVERY_MS, diff --git a/apps/server/src/harness/remote/registry.ts b/apps/server/src/harness/remote/registry.ts index a50ed83eb..1b4075413 100644 --- a/apps/server/src/harness/remote/registry.ts +++ b/apps/server/src/harness/remote/registry.ts @@ -26,7 +26,7 @@ export type RemotePlanner = }; let planners = new Map(); -let links = new Map(); +let links = new Map(); export function registerRemotePlanner(sessionId: string, planner: RemotePlanner): () => void { if (planners.has(sessionId)) throw new Error("Remote Planner session is already registered"); @@ -40,19 +40,14 @@ export function remotePlanner(sessionId: string): RemotePlanner | undefined { return planners.get(sessionId); } -/** - * Records a link attached to a document. Until one Planner per document is - * enforced, the most recent attachment serves new sessions. - */ +/** Records the link attached as a document's Planner; `PlannerAttachments` admits one per document. */ export function attachLink(channelId: string, link: PlannerLink): () => void { - links.set(channelId, [...links.get(channelId) ?? [], link]); + links.set(channelId, link); return () => { - let remaining = (links.get(channelId) ?? []).filter(candidate => candidate !== link); - if (remaining.length) links.set(channelId, remaining); - else links.delete(channelId); + if (links.get(channelId) === link) links.delete(channelId); }; } export function attachedLink(channelId: string): PlannerLink | undefined { - return links.get(channelId)?.at(-1); + return links.get(channelId); } diff --git a/apps/server/src/main.ts b/apps/server/src/main.ts index aebbbd425..76e59f3e1 100644 --- a/apps/server/src/main.ts +++ b/apps/server/src/main.ts @@ -18,6 +18,8 @@ import { isPlannerLinkSocket, openPlannerLinkSocket, } from "./harness/remote/endpoint"; +import { notAttachedMessage, PlannerAttachments } from "./harness/remote/attachments"; +import { documentUrl } from "./channels/document-url"; import { ActiveOwnerBindings } from "./agent/active-owner"; import { registerAuthRoutes } from "./auth/routes"; import { registerExperimentRoutes } from "./experiments/routes"; @@ -79,7 +81,7 @@ import { StorageError } from "./storage/errors"; import { createStorage } from "./storage/registry"; import { broadcast, fail, relay, reply, repositoryTopic, tell, topic } from "./wire"; -import { PLANNER_LINK_PATH } from "@chopin/planner-link"; +import { LINK_CLOSE, PLANNER_LINK_PATH } from "@chopin/planner-link"; import type { Server, ServerWebSocket } from "bun"; import type { PlannerLinkSocketData } from "./harness/remote/endpoint"; @@ -128,6 +130,7 @@ let renewingLease: Promise | undefined; let sessionCleanup: ReturnType | undefined; let cleaningSessions: Promise | undefined; let ownerBindings: ActiveOwnerBindings | undefined; +let plannerAttachments: PlannerAttachments | undefined; let jobRunner: JobRunner | undefined; let researchService: ResearchWorkspaceService | undefined; let researchRecoveryTimer: ReturnType | undefined; @@ -307,6 +310,19 @@ function conversation( auth: hostedAuth, claimantSessionId, repository, + plannerNotAttached: plannerAttachments + ? async () => { + if (plannerAttachments!.attached(room.id)) return undefined; + let channel = await storage.channels.get(room.id); + let path = channel + ? await documentUrl(channel, storage.channels) + : `/channels/${room.id}`; + return notAttachedMessage( + new URL(path, hostedAuth.config.origin).href, + `${repository.owner}/${repository.name}`, + ); + } + : undefined, persist: chat => Service.persist(opened, chat), activeOwner: () => ownerBindings!.resolve(room.id), ownerAvailable: () => jobRunner?.ownerAvailable(room.id) ?? Promise.resolve(), @@ -674,6 +690,7 @@ async function receive(ws: Socket, raw: string): Promise { refreshAccess, unavailable: id => archivingChannels.has(id) || deletingChannels.has(id), ownerAvailable: id => jobRunner?.ownerAvailable(id) ?? Promise.resolve(), + claimsOwnership: !plannerAttachments, placeReference: placeResearchReference, scheduleRecovery: scheduleResearchRecovery, }), @@ -1148,8 +1165,7 @@ function listen(): Server { return upgraded(req, self, outcome.data); } if (url.pathname === PLANNER_LINK_PATH) { - let enabled = config.agent && config.harness === "remote"; - let outcome = await admitPlannerLink(req, hostedAuth, enabled); + let outcome = await admitPlannerLink(req, hostedAuth, !!plannerAttachments); if (outcome instanceof Response) return outcome; return upgraded(req, self, outcome); } @@ -1169,7 +1185,9 @@ function listen(): Server { websocket: { open(ws: AnySocket) { if (isPlannerLinkSocket(ws)) { + if (!plannerAttachments) return ws.close(LINK_CLOSE.failed, "unavailable"); openPlannerLinkSocket(ws, hostedAuth, { + attachments: plannerAttachments, unavailable: channelId => deletingChannels.has(channelId), }); } else if (isSidebarSocket(ws)) scheduleSidebarRecheck(ws); @@ -1234,6 +1252,7 @@ function drain(): Promise { summaryCoordinator?.close(); let stoppingJobs = jobRunner?.shutdown(); ownerBindings?.revokeAll(); + plannerAttachments?.close(); for (let result of await Promise.allSettled([stoppingJobs])) { if (result.status === "rejected") record(result.reason); } @@ -1672,6 +1691,9 @@ experiments = registerExperimentRoutes(router, hostedAuth, { }, }); ownerBindings = new ActiveOwnerBindings(hostedAuth); +if (config.agent && config.harness === "remote") { + plannerAttachments = new PlannerAttachments(hostedAuth, config.plannerAttachGraceMs); +} let definitions: JobDefinition[] = []; if (config.backgroundJobs) { definitions.push(researchBriefDefinition({ config })); @@ -1888,7 +1910,7 @@ registerResearchWorkspaceRoutes(router, hostedAuth, { defaultBranch: repository.defaultBranch, }, channel.id, - session.session.id, + plannerAttachments ? undefined : session.session.id, ); await jobRunner?.ownerAvailable(channel.id); }, diff --git a/apps/server/src/mcp.ts b/apps/server/src/mcp.ts index 00ed96b66..93c01a309 100644 --- a/apps/server/src/mcp.ts +++ b/apps/server/src/mcp.ts @@ -155,6 +155,7 @@ export type InvokePlannerError = | "document-unavailable" | "repository-forbidden" | "document-archived" + | "planner-not-attached" | "planner-owner-unavailable" | "checkout-unverified" | "planner-unavailable" @@ -163,7 +164,7 @@ export type InvokePlannerError = export type InvokePlanner = { invoke(caller: Caller, input: InvokePlannerInput): Promise< | { kind: "invoked"; document: DocumentSummary & { url: string } } - | { kind: "refused"; code: InvokePlannerError } + | { kind: "refused"; code: InvokePlannerError; message?: string } >; }; @@ -518,7 +519,10 @@ export const TOOLS: Tool[] = [ description: "Post an instruction to a document's Planner, attributed to the caller, and " + "return its URL without waiting for the turn. The turn runs under the document's " + "Planner owner; without one, the caller's live Chopin browser login becomes the " - + "owner, and otherwise the call is refused. Questions appear in Decisions.", + + "owner, and otherwise the call is refused. Where the Planner runs in an attached " + + "client, a document with no attached Planner refuses the call with " + + "planner-not-attached and a message saying how to attach one. Questions appear in " + + "Decisions.", inputSchema: { type: "object", properties: { @@ -554,15 +558,27 @@ export const TOOLS: Tool[] = [ required: ["id", "title", "url"], additionalProperties: false, }, - outcome([ - "document-unavailable", - "repository-forbidden", - "document-archived", - "planner-owner-unavailable", - "checkout-unverified", - "planner-unavailable", - "planner-queue-full", - ]), + { + type: "object", + properties: { + code: { + type: "string", + enum: [ + "document-unavailable", + "repository-forbidden", + "document-archived", + "planner-not-attached", + "planner-owner-unavailable", + "checkout-unverified", + "planner-unavailable", + "planner-queue-full", + ], + }, + message: { type: "string" }, + }, + required: ["code"], + additionalProperties: false, + }, ], }, }, @@ -1098,7 +1114,10 @@ export function handler( let result = await options.invoke.invoke(caller, args as InvokePlannerInput); return result.kind === "invoked" ? respond(text(result.document)) - : respond(text({ code: result.code }, true)); + : respond(text({ + code: result.code, + ...(result.message ? { message: result.message } : {}), + }, true)); } if (tool.name === "upload_image" && options.upload) { let prepared = prepareImage(tool.arguments); diff --git a/apps/server/src/mcp/hosted.test.ts b/apps/server/src/mcp/hosted.test.ts index fe33a4164..9675a868d 100644 --- a/apps/server/src/mcp/hosted.test.ts +++ b/apps/server/src/mcp/hosted.test.ts @@ -276,6 +276,30 @@ describe("the hosted MCP adapter", () => { } }); + it("tells an invocation with no attached Planner how to attach one, naming the document URL", async () => { + let context = setup(); + context.github.repositoryValue.permissions.push = true; + let opened = await plan(context); + let adapter = hosted(context.auth, undefined, { + invokePlanner: async () => "planner-not-attached", + }); + let caller = (await adapter.caller(request("Bearer allowed")))!; + try { + expect( + await adapter.invoke!.invoke(caller, { id: opened.channel.id, instruction: "Plan it." }), + ).toEqual({ + kind: "refused", + code: "planner-not-attached", + message: "No Planner is attached to this document. To attach one, run pdd-plan with " + + "plan_document set to " + + "https://chopin.test/documents/octo-org/score/release-readiness from a session in " + + "a checkout of octo-org/score; that session becomes this document's Planner.", + }); + } finally { + await Service.close(opened.plan); + } + }); + it("passes a URL invocation's checkout through unverified for the harness to judge", async () => { let context = setup(); context.github.repositoryValue.permissions.admin = true; diff --git a/apps/server/src/mcp/hosted.ts b/apps/server/src/mcp/hosted.ts index 69a585c0f..6c574489c 100644 --- a/apps/server/src/mcp/hosted.ts +++ b/apps/server/src/mcp/hosted.ts @@ -7,6 +7,7 @@ import { } from "@chopin/protocol/document-url"; import { documentUrl } from "../channels/document-url"; +import { notAttachedMessage } from "../harness/remote/attachments"; import { deterministicChannelId, isChannelId } from "../channels/id"; import { documentSlug } from "../channels/slug"; import { GitHubError } from "../github/client"; @@ -336,15 +337,17 @@ export function hosted( instruction: input.instruction, ...(input.checkout === undefined ? {} : { checkout: input.checkout }), }); + let url = + new URL(await documentUrl(channel, auth.storage.channels), auth.config.origin).href; + if (code === "planner-not-attached") { + return { + kind: "refused", + code, + message: notAttachedMessage(url, `${repository.owner}/${repository.name}`), + }; + } if (code) return { kind: "refused", code }; - return { - kind: "invoked", - document: { - ...summary(channel), - url: - new URL(await documentUrl(channel, auth.storage.channels), auth.config.origin).href, - }, - }; + return { kind: "invoked", document: { ...summary(channel), url } }; }, } : undefined, diff --git a/apps/server/src/mcp/invoke.test.ts b/apps/server/src/mcp/invoke.test.ts index e774e92eb..b17f3b7c2 100644 --- a/apps/server/src/mcp/invoke.test.ts +++ b/apps/server/src/mcp/invoke.test.ts @@ -93,6 +93,7 @@ test("invoke_planner exposes each refusal as an object code", async () => { "document-unavailable", "repository-forbidden", "document-archived", + "planner-not-attached", "planner-owner-unavailable", "checkout-unverified", "planner-unavailable", @@ -107,3 +108,22 @@ test("invoke_planner exposes each refusal as an object code", async () => { expect(result.result).toMatchObject({ isError: true, structuredContent: { code } }); } }); + +test("invoke_planner carries a refusal's message beside its code", async () => { + let message = "No Planner is attached to this document. Run pdd-plan to attach one."; + let mcp = endpoint({ + invoke: async () => ({ kind: "refused", code: "planner-not-attached", message }), + }); + let result = await call(mcp, "tools/call", { + name: "invoke_planner", + arguments: { id: "document-id", instruction: "go" }, + }); + expect(result.result).toMatchObject({ + isError: true, + structuredContent: { code: "planner-not-attached", message }, + }); + expect(JSON.parse(result.result.content[0].text)).toEqual({ + code: "planner-not-attached", + message, + }); +}); diff --git a/apps/server/src/storage/contract.ts b/apps/server/src/storage/contract.ts index 36c7ce3f6..f9931735a 100644 --- a/apps/server/src/storage/contract.ts +++ b/apps/server/src/storage/contract.ts @@ -1,6 +1,7 @@ import { researchWorkspace } from "./research-inline-contract-fixtures"; import { experimentContract } from "./experiment-contract"; import { imageContract } from "./image-contract"; +import { plannerAttachmentContract } from "./planner-attachment-contract"; import { researchInlineContract } from "./research-inline-contract"; import { describe, expect, it } from "bun:test"; @@ -35,6 +36,7 @@ function attempt(action: () => Promise): Promise { export function storageContract(name: string, factory: Factory): void { experimentContract(name, factory); imageContract(name, factory); + plannerAttachmentContract(name, factory); describe(`${name} storage`, () => { it("supports expiring metadata-only sessions for local authentication", async () => { let storage = await opened(factory); diff --git a/apps/server/src/storage/planner-attachment-contract.ts b/apps/server/src/storage/planner-attachment-contract.ts new file mode 100644 index 000000000..047e18826 --- /dev/null +++ b/apps/server/src/storage/planner-attachment-contract.ts @@ -0,0 +1,168 @@ +import { describe, expect, test } from "bun:test"; + +import { Sessions } from "../auth/session"; +import { PlannerAttachments } from "../harness/remote/attachments"; +import { contractId as id, openedStorage } from "./contract-support"; + +import type { AttachmentAuth } from "../harness/remote/attachments"; +import type { PlannerLink } from "../harness/remote/link"; +import type { StorageFactory } from "./contract-support"; +import type { StorageAdapter } from "./port"; + +const GRANT = { accessExpiresIn: 28_800, refreshToken: "refresh", refreshExpiresIn: 86_400 }; + +async function attachable(storage: StorageAdapter) { + let now = new Date(); + let member = { id: id("member"), login: "member" }; + let rival = { id: id("rival"), login: "rival" }; + for (let user of [member, rival]) await storage.users.put({ ...user, avatarUrl: "", now }); + let channelId = id("channel"); + let repository = { id: id("repository"), owner: "octo-org", name: "score" }; + await storage.channels.create({ + id: channelId, + repositoryId: repository.id, + repositoryOwner: repository.owner, + repositoryName: repository.name, + title: "Attached document", + createdBy: member.id, + now, + }); + let sessions = new Sessions(storage, false); + let signIn = async (user: { id: string }) => + (await sessions.issue(user.id, { ...GRANT, accessToken: `${user.id}-login` })).id; + let logins = { member: await signIn(member), rival: await signIn(rival) }; + let auth: AttachmentAuth = { + config: { origin: "https://chopin.test" }, + storage, + sessions, + clock: () => new Date(), + admission: { allowed: async () => true }, + github: { + repositoryAccess: async () => ({ + ...repository, + fullName: "octo-org/score", + private: true, + url: "https://github.test/octo-org/score", + defaultBranch: "main", + permissions: { pull: true, push: true, admin: false }, + }), + }, + }; + let owner = async () => (await storage.channels.readAgent(channelId, new Date()))?.agent; + let request = (user: { id: string; login: string }) => ({ channelId, repository, user }); + return { auth, sessions, member, rival, logins, channelId, owner, request }; +} + +function link(): PlannerLink { + return {} as PlannerLink; +} + +async function released(owner: () => Promise<{ ownerSessionId?: string } | undefined>) { + for (let deadline = Date.now() + 3_000; Date.now() < deadline;) { + if (!(await owner())?.ownerSessionId) return; + await Bun.sleep(10); + } +} + +/** Remote Planner attachments over each storage adapter's generation-guarded ownership. */ +export function plannerAttachmentContract(name: string, factory: StorageFactory): void { + describe(`${name} Planner attachments`, () => { + test("an attachment owns the document, refuses another login, and survives its own reattach", async () => { + let storage = await openedStorage(factory); + let f = await attachable(storage); + let attachments = new PlannerAttachments(f.auth, 60_000); + try { + let granted = await attachments.attach(f.request(f.member), link()); + expect(granted.ok).toBe(true); + let held = await f.owner(); + expect(held).toMatchObject({ ownerSessionId: f.logins.member, status: "ready" }); + expect(await attachments.attach(f.request(f.rival), link())).toMatchObject({ + ok: false, + code: "planner-attached", + message: expect.stringContaining("member is attached"), + }); + if (granted.ok) granted.detach(); + expect(attachments.attached(f.channelId)).toBe(false); + expect(await f.owner()).toEqual(held); + expect(await attachments.attach(f.request(f.rival), link())).toMatchObject({ + ok: false, + code: "planner-attached", + message: expect.stringContaining("member detached"), + }); + expect((await attachments.attach(f.request(f.member), link())).ok).toBe(true); + expect(attachments.attached(f.channelId)).toBe(true); + expect(await f.owner()).toEqual(held); + } finally { + attachments.close(); + await storage.close(); + } + }); + + test("the grace period's end clears only the ownership the attachment still holds", async () => { + let storage = await openedStorage(factory); + let f = await attachable(storage); + let attachments = new PlannerAttachments(f.auth, 30); + try { + let first = await attachments.attach(f.request(f.member), link()); + let generation = (await f.owner())!.generation; + if (first.ok) first.detach(); + await released(f.owner); + expect(await f.owner()).toMatchObject({ + ownerSessionId: undefined, + generation, + status: "unavailable", + }); + expect(attachments.holder(f.channelId)).toBeUndefined(); + + let rival = await attachments.attach(f.request(f.rival), link()); + expect(await f.owner()) + .toMatchObject({ ownerSessionId: f.logins.rival, generation: generation + 1 }); + if (rival.ok) rival.detach(); + let channels = storage.channels; + expect( + await channels.clearAgentOwner(f.channelId, f.logins.rival, generation + 1, new Date()), + ) + .toBe(true); + await channels.claimAgentOwner(f.channelId, f.logins.member, new Date()); + await Bun.sleep(80); + expect(await f.owner()) + .toMatchObject({ ownerSessionId: f.logins.member, generation: generation + 2 }); + } finally { + attachments.close(); + await storage.close(); + } + }); + + test("startup clears an attachment's ownership and a new process starts unattached", async () => { + let storage = await openedStorage(factory); + let f = await attachable(storage); + let attachments = new PlannerAttachments(f.auth, 60_000); + try { + await attachments.attach(f.request(f.member), link()); + let generation = (await f.owner())!.generation; + let lease = await storage.leases.acquire(id("writer"), id("instance"), 60_000); + await storage.sessions.reset(new Date(), lease!, 60_000); + expect(await f.owner()).toMatchObject({ ownerSessionId: undefined, generation }); + + let sessions = new Sessions(storage, false); + let restarted = new PlannerAttachments({ ...f.auth, sessions }, 60_000); + try { + expect(restarted.attached(f.channelId)).toBe(false); + expect(await restarted.attach(f.request(f.member), link())).toMatchObject({ + ok: false, + code: "sign-in-required", + }); + let login = await sessions.issue(f.member.id, { ...GRANT, accessToken: "again" }); + expect((await restarted.attach(f.request(f.member), link())).ok).toBe(true); + expect(await f.owner()) + .toMatchObject({ ownerSessionId: login.id, generation: generation + 1 }); + } finally { + restarted.close(); + } + } finally { + attachments.close(); + await storage.close(); + } + }); + }); +} diff --git a/apps/server/src/testing/config.ts b/apps/server/src/testing/config.ts index cc6e64fd6..55cd854f6 100644 --- a/apps/server/src/testing/config.ts +++ b/apps/server/src/testing/config.ts @@ -29,6 +29,7 @@ export function configured(overrides: Record = {}) { JEV_API_KEY: undefined, JEV_MODEL: undefined, JEV_TIMEOUT_MS: undefined, + PLANNER_ATTACH_GRACE_MS: undefined, TYPESAFE_API_KEY: undefined, HARNESS: undefined, HARNESS_AUTH: undefined, diff --git a/apps/server/src/testing/planner-link.ts b/apps/server/src/testing/planner-link.ts index 42626f853..ce4acbee1 100644 --- a/apps/server/src/testing/planner-link.ts +++ b/apps/server/src/testing/planner-link.ts @@ -2,6 +2,8 @@ import { connectPlannerLink, linkSocket, plannerLinkUrl } from "@chopin/planner- import { AdmissionDenied } from "../auth/admission"; import { GitHubError } from "../github/client"; +import { Sessions } from "../auth/session"; +import { PlannerAttachments } from "../harness/remote/attachments"; import { admitPlannerLink, openPlannerLinkSocket } from "../harness/remote/endpoint"; import { MemoryStorage } from "../storage/memory/adapter"; @@ -11,64 +13,122 @@ import type { TurnContext, } from "@chopin/planner-link/client"; import type { Repository } from "../github/client"; +import type { AttachmentAuth } from "../harness/remote/attachments"; import type { PlannerLinkAuth, PlannerLinkSocketData } from "../harness/remote/endpoint"; import type { PlannerLink } from "../harness/remote/link"; -export const READER_TOKEN = "reader-token"; +export const MEMBER_TOKEN = "member-token"; +export const RIVAL_TOKEN = "rival-token"; +export const VIEWER_TOKEN = "viewer-token"; +export const GUEST_TOKEN = "guest-token"; export const OUTSIDER_TOKEN = "outsider-token"; export const DENIED_TOKEN = "denied-token"; +const ACCOUNTS: Record = { + [MEMBER_TOKEN]: { id: "U_member", login: "member", push: true, signedIn: true }, + [RIVAL_TOKEN]: { id: "U_rival", login: "rival", push: true, signedIn: true }, + [VIEWER_TOKEN]: { id: "U_viewer", login: "viewer", push: false, signedIn: true }, + [GUEST_TOKEN]: { id: "U_guest", login: "guest", push: true, signedIn: false }, +}; + +function loginToken(login: string): string { + return `${login}-browser-login`; +} + +function scoreRepository(owner: string, name: string, push: boolean): Repository { + return { + id: "R_score", + owner, + name, + fullName: `${owner}/${name}`, + private: true, + url: `https://github.test/${owner}/${name}`, + defaultBranch: "main", + permissions: { pull: true, push, admin: false }, + }; +} + /** - * GitHub as hosted MCP sees it: the reader can pull `octo-org/score`, the - * outsider is a valid account without access, the denied account fails - * admission, and every other token is rejected by GitHub. + * GitHub as hosted MCP sees it: the member and the rival can push to + * `octo-org/score` and are signed in to Chopin in a browser, the viewer can + * only pull, the guest can push but never signed in, the outsider is a valid + * account without access, the denied account fails admission, and every other + * token is rejected by GitHub. */ export async function linkAuth(channelId?: string) { let storage = new MemoryStorage(); let now = new Date(); - await storage.users.put({ id: "U_reader", login: "reader", avatarUrl: "", now }); + for (let account of Object.values(ACCOUNTS)) { + await storage.users.put({ id: account.id, login: account.login, avatarUrl: "", now }); + } + let sessions = new Sessions(storage, false); + let logins = new Map(); + for (let account of Object.values(ACCOUNTS)) { + if (!account.signedIn) continue; + let issued = await sessions.issue(account.id, { + accessToken: loginToken(account.login), + accessExpiresIn: 28_800, + refreshToken: "refresh-token", + refreshExpiresIn: 86_400, + }); + logins.set(account.login, issued.id); + } let channel = await storage.channels.create({ id: channelId ?? crypto.randomUUID(), repositoryId: "R_score", repositoryOwner: "octo-org", repositoryName: "score", title: "Linked document", - createdBy: "U_reader", + createdBy: "U_member", now, }); let auth = { config: { origin: "https://chopin.test" }, storage, + sessions, + clock: () => new Date(), admission: { async user(token: string) { if (token === DENIED_TOKEN) throw new AdmissionDenied(); - if (token === READER_TOKEN) return { id: "U_reader", login: "reader", avatarUrl: "" }; + let account = ACCOUNTS[token]; + if (account) return { id: account.id, login: account.login, avatarUrl: "" }; if (token === OUTSIDER_TOKEN) return { id: "U_outsider", login: "outsider", avatarUrl: "" }; throw new GitHubError("Bad credentials", 401); }, + async allowed(token: string) { + return token !== DENIED_TOKEN; + }, }, github: { async repository(token: string, owner: string, name: string): Promise { - if (token !== READER_TOKEN) throw new GitHubError("Not Found", 404); - return { - id: "R_score", - owner, - name, - fullName: `${owner}/${name}`, - private: true, - url: `https://github.test/${owner}/${name}`, - defaultBranch: "main", - permissions: { pull: true, push: false, admin: false }, - }; + let account = ACCOUNTS[token]; + if (!account) throw new GitHubError("Not Found", 404); + return scoreRepository(owner, name, account.push); + }, + async repositoryAccess(token: string, owner: string, name: string) { + let account = Object.values(ACCOUNTS).find(value => loginToken(value.login) === token); + return account ? scoreRepository(owner, name, account.push) : undefined; }, }, - } as unknown as PlannerLinkAuth; - return { auth, storage, channel }; + }; + return { + auth: auth as unknown as PlannerLinkAuth & AttachmentAuth & { github: typeof auth.github }, + storage, + sessions, + logins, + channel, + }; } /** A Bun server that serves only the Planner link, as the Chopin server does under HARNESS=remote. */ -export function linkServer(auth: PlannerLinkAuth, timeoutMs = 2_000, revalidateEveryMs?: number) { +export function linkServer( + auth: PlannerLinkAuth & AttachmentAuth, + timeoutMs = 2_000, + revalidateEveryMs?: number, + graceMs = 2_000, +) { let links: PlannerLink[] = []; + let attachments = new PlannerAttachments(auth, graceMs); let server = Bun.serve({ hostname: "127.0.0.1", port: 0, @@ -82,6 +142,7 @@ export function linkServer(auth: PlannerLinkAuth, timeoutMs = 2_000, revalidateE websocket: { open(ws) { openPlannerLinkSocket(ws, auth, { + attachments, timeoutMs, revalidateEveryMs, register(_channelId, link) { @@ -102,15 +163,19 @@ export function linkServer(auth: PlannerLinkAuth, timeoutMs = 2_000, revalidateE }); return { url: `http://127.0.0.1:${server.port}`, + attachments, get links() { return links; }, - stop: () => server.stop(true), + stop: () => { + attachments.close(); + return server.stop(true); + }, }; } -/** Raw frames over a socket that authenticated as the reader, for protocol violations. */ -export async function rawLink(url: string, token = READER_TOKEN) { +/** Raw frames over a socket that authenticated as the member, for protocol violations. */ +export async function rawLink(url: string, token = MEMBER_TOKEN) { let socket = linkSocket(plannerLinkUrl(url), token); let messages: any[] = []; let arrivals = new Set<() => void>(); @@ -205,7 +270,7 @@ export function attach( url: string, document: string, handlers: PlannerLinkHandlers, - token = READER_TOKEN, + token = MEMBER_TOKEN, ): Promise { return connectPlannerLink({ url, token, document, handlers }); } @@ -223,7 +288,7 @@ export async function nodeClient(url: string, document: string) { node, new URL("./planner-link-node-client.mjs", import.meta.url).pathname, url, - READER_TOKEN, + MEMBER_TOKEN, document, ], { stdin: "pipe", stdout: "pipe", stderr: "inherit" }); let reader = child.stdout.getReader(); diff --git a/apps/server/src/wire.ts b/apps/server/src/wire.ts index 314aced8a..d0edba037 100644 --- a/apps/server/src/wire.ts +++ b/apps/server/src/wire.ts @@ -11,7 +11,7 @@ */ import type { Server, ServerWebSocket } from "bun"; -import type { Frame, Outgoing } from "@chopin/protocol"; +import type { Frame, Outgoing, Session } from "@chopin/protocol"; import type { DecisionWatch } from "./socket/decision-watch"; export type Identity = { @@ -82,8 +82,13 @@ export function reply(ws: Sender, rid: string, frame: T): vo } /** Refuse one request, on the socket that made it. */ -export function fail(ws: Sender, rid: string, message: string): void { - ws.send(stamp({ kind: "session:error", ts: 0, message }, { rid })); +export function fail( + ws: Sender, + rid: string, + message: string, + code?: Session.Failure["code"], +): void { + ws.send(stamp({ kind: "session:error", ts: 0, message, ...(code ? { code } : {}) }, { rid })); } /** Tell one socket something it did not ask for. */ diff --git a/apps/web/src/chat/references.test.ts b/apps/web/src/chat/references.test.ts index ff4d39172..585fd8ee0 100644 --- a/apps/web/src/chat/references.test.ts +++ b/apps/web/src/chat/references.test.ts @@ -18,6 +18,7 @@ import { selectedReplacement, withNoticesAfter, } from "./references"; +import { WireRefusal } from "../wire"; import type { ReferenceDraft, ReferenceTarget } from "./references"; @@ -366,4 +367,10 @@ describe("acknowledged composer drafts", () => { expect(bounded).toEndWith("..."); expect(boundedChatError(new Error("connection lost"))).toContain("Check the connection"); }); + + test("shows a missing remote Planner's instructions whole, however long", () => { + let message = `No Planner is attached to this document. ${"Run pdd-plan. ".repeat(20)}`.trim(); + expect(boundedChatError(new WireRefusal(message, "planner-not-attached"))).toBe(message); + expect(boundedChatError(new WireRefusal(message))).toEndWith("..."); + }); }); diff --git a/apps/web/src/chat/references.ts b/apps/web/src/chat/references.ts index 21b641a32..ffb666b07 100644 --- a/apps/web/src/chat/references.ts +++ b/apps/web/src/chat/references.ts @@ -364,6 +364,7 @@ export function acknowledgeDraft(current: ComposerDraft, submitted: ComposerDraf export function boundedChatError(error: unknown): string { let detail = error instanceof Error ? error.message.trim() : ""; + if (detail && (error as { code?: unknown }).code === "planner-not-attached") return detail; if (!detail || /^(connection lost|disposed|not connected)$/i.test(detail)) { return "Message not sent. Check the connection and try again."; } diff --git a/apps/web/src/wire.ts b/apps/web/src/wire.ts index f7accad90..7248aa37b 100644 --- a/apps/web/src/wire.ts +++ b/apps/web/src/wire.ts @@ -10,6 +10,8 @@ * same build work on localhost, over a LAN address, and through a tunnel. */ +import type { Session } from "@chopin/protocol"; + export type Status = | "connecting" | "connected" @@ -20,6 +22,17 @@ export type Status = export type Unsubscribe = () => void; +/** A request the server refused; `code` marks a refusal to present in full. */ +export class WireRefusal extends Error { + readonly code: Session.Failure["code"]; + + constructor(message: string, code?: Session.Failure["code"]) { + super(message); + this.name = "WireRefusal"; + this.code = code; + } +} + type Frame = { kind: string; ts: number; rid?: string; sender?: string }; type Listener = (frame: never) => void; @@ -267,7 +280,8 @@ export class Wire { if (waiting) { this.#pending.delete(frame.rid); if (frame.kind === "session:error") { - waiting.reject(new Error(String((frame as { message?: string }).message))); + let failure = frame as Partial; + waiting.reject(new WireRefusal(String(failure.message), failure.code)); } else { waiting.resolve(frame as never); } diff --git a/docs/architecture.md b/docs/architecture.md index ef0cacd61..6dba7cebf 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -145,8 +145,11 @@ harness sessions; see [Background jobs](background-jobs.md). Under `HARNESS=remote`, `/planner-link` accepts a WebSocket whose upgrade carries a GitHub bearer, checked like `/mcp`. The client attaches to one document and -needs pull access to its repository, rechecked before every session and turn and -periodically while attached. Turns run on the client's model; Chopin's host +needs push or administration access to its repository, rechecked before every +session and turn and periodically while attached. Attaching makes the account's +live browser login the document's Planner owner, one client per document; a +document without one refuses Planner requests with `planner-not-attached`. +Turns run on the client's model; Chopin's host tools, Decisions, and room writes still run in the server under the document's Planner owner and the permission checks above. The client sees what a turn sees and steers its document writes during turns writers start; its run reports never diff --git a/docs/hosted-agent.md b/docs/hosted-agent.md index d800024c0..94d3f99ea 100644 --- a/docs/hosted-agent.md +++ b/docs/hosted-agent.md @@ -35,7 +35,8 @@ the default `copilot-sdk` harness, the Copilot entitlement. Under `HARNESS=pi` and `HARNESS=atomic` the owner supplies only the GitHub token; model access comes from the operator's `HARNESS_AUTH` mode. Under `HARNESS=remote` the owner also supplies only the GitHub token, and the model runs in the client attached -to the document. The user must pass instance +to the document; only [attaching](#remote-planner) makes an account the owner +there. The user must pass instance admission and have repository push or administration access. Ownership is assigned atomically in storage and guarded by a generation token. @@ -51,6 +52,10 @@ call follows the same rules. Its instruction is posted as the caller's own message; the turn runs under the channel's current owner, whoever that is. A channel without one is claimed for the caller's live browser login, hosted or local, and without such a login the call is refused and nothing is posted. +Under `HARNESS=remote` neither `invoke_planner`, `@chopin`, nor a research +request claims ownership: a document without an attached Planner refuses them +with `planner-not-attached`, and one with an attached Planner runs them under +its owner. Planner ownership stores only the owner session ID, referring to a login loaded in this process. Hosted logins separately persist encrypted credentials in @@ -286,14 +291,48 @@ checked exactly as hosted MCP checks it: the token must resolve through instance admission, a bad or missing token gets HTTP 401, a refused account 403, a foreign `Origin` 403, and GitHub outages 503, all before anything is upgraded. The first message must be `attach` with protocol version 1. Its document is resolved like -an MCP document locator, and the bearer's own account must have pull access to -the document's repository. An unsupported version, an unreadable repository, or -a missing or archived document is refused with a `refused` message and close -code 4426, 4403, or 4404. Neither attaching nor a refusal posts anything to the -document. The account and repository access are checked again before every -session and every turn the link starts, including turns of a session kept for -its runs, and every 5 minutes while the link stays attached; losing either -fails that session or turn and closes the link with `access-revoked`. +an MCP document locator, and the bearer's own account must have push or +administration access to the document's repository. An unsupported version, a +repository it cannot write, or a missing or archived document is refused with a +`refused` message and close code 4426, 4403, or 4404. Neither attaching nor a +refusal posts anything to the document. The account and repository access are +checked again before every session and every turn the link starts, including +turns of a session kept for its runs, and every 5 minutes while the link stays +attached; losing either fails that session or turn and closes the link with +`access-revoked`. + +**Ownership.** Attaching makes the account the document's Planner owner through +the same generation-guarded storage claim as any other owner +(`apps/server/src/harness/remote/attachments.ts`). The owner record names the +account's live browser login, not the link, because Chopin's repository tools +run with that login's GitHub App token and never with the link's bearer; an +account with no live sign-in since the server started is refused with +`sign-in-required` (4401). The login must pass the same eligibility as every +Planner owner: instance admission and push or administration access. This is +stricter than the design's "can read the repository", because an attached +client steers what the Planner writes and runs repository tools as the owner. +An owner left without an attachment, which only a race or an operator reset can +produce, is cleared under its generation and the document is claimed again. + +One client holds a document's Planner. While it is attached, every other +`attach` for that document is refused with `planner-attached` (4409) and a +message naming its GitHub login. A link that detaches or drops keeps the +ownership for `PLANNER_ATTACH_GRACE_MS` (2 minutes by default): the same account +can attach again within it and keeps the same ownership generation, and anyone +else is refused. When it ends, the ownership is cleared under its generation, so +a newer owner is never released by an old timer. Attachments live in the server +process; startup already clears every owner reference, which releases every +attachment. + +**Unattached documents.** Members, MCP callers, and research requests never claim +ownership under `HARNESS=remote`. An `@chopin` message or `invoke_planner` call +for a document with no attached client, including one within its grace period, +is refused with `planner-not-attached` before anything is posted or queued. The +refusal names the document URL and says to run `pdd-plan` with it as +`plan_document` from a session in a checkout of the repository, which attaches +that session as the Planner; the browser shows that text on the `@chopin` +request. Room messages are unaffected. Once a client is attached, both arrive as +turns on its link and run under its ownership whoever sent them. **Turns and tools.** Chopin's host tools (document, repository, and job tools) still run in the server against the room, under the document's Planner owner and @@ -333,12 +372,10 @@ gains no shell or filesystem access from this harness. The client, in turn, receives everything a Planner turn sees: the conversation, the document, and the results of repository tools that run with the document Planner owner's GitHub App token, limited to the document's repository, which its own account -can already read. Attaching requires only pull access, yet the attached client -decides what the Planner writes to the document during turns writers start, so -attach only clients you would let edit the document. Until one Planner per -document is enforced, the most recently attached link serves new sessions and -earlier ones stay connected. Summary and research workers have no model in the -server, so `HARNESS=remote` requires `BACKGROUND_JOBS=off`. +can already read. The attached client decides what the Planner writes to the +document during turns writers start, which is why attaching requires the push +or administration access every Planner owner needs. Summary and research workers +have no model in the server, so `HARNESS=remote` requires `BACKGROUND_JOBS=off`. ## Permission checks diff --git a/docs/local-agent-mcp.md b/docs/local-agent-mcp.md index 1fa31cc87..05680d698 100644 --- a/docs/local-agent-mcp.md +++ b/docs/local-agent-mcp.md @@ -92,6 +92,20 @@ browser `@chopin` message would: - Without either, the call is refused with `planner-owner-unavailable` and nothing is posted. +A server running `HARNESS=remote` claims no ownership here. Its Planner runs in +a client attached to the document over the [Planner link](planner-link.md), and +only attaching makes an account the owner. With a client attached the turn goes +to it and runs under its owner, whoever invoked it. Without one, including +while a detached Planner's grace period runs, the call is refused with +`planner-not-attached` and a `message`, and nothing is posted or queued: + +```json +{ + "code": "planner-not-attached", + "message": "No Planner is attached to this document. To attach one, run pdd-plan with plan_document set to https://chopin.example/documents/octo-org/score/release-readiness from a session in a checkout of octo-org/score; that session becomes this document's Planner." +} +``` + The MCP bearer never becomes the owner or supplies its credentials. The bearer needs repository write access; an owner must also pass the GitHub App installation check. @@ -131,8 +145,13 @@ browser need not already have the document open: Chopin keeps its room alive while the invoked turn and queue run. An interrupted turn is not replayed automatically on restart. -Refusals return an object with `code`: +Refusals return an object with `code`, and `planner-not-attached` also a +`message`: +- `planner-not-attached`: under `HARNESS=remote`, no client is attached as the + document's Planner. `message` names the document URL and says to run + `pdd-plan` with it as `plan_document` from a session in a checkout of the + repository, which attaches that session as the Planner. - `planner-owner-unavailable`: the document has no usable Planner owner and the caller has no live browser login to claim it; sign in to Chopin in a browser as the MCP account, or ask the owner to sign in again. diff --git a/docs/planner-link.md b/docs/planner-link.md index 6d8b9f054..d3dff86a7 100644 --- a/docs/planner-link.md +++ b/docs/planner-link.md @@ -81,23 +81,81 @@ The server answers: ``` `attached.document.repository` is `owner/name`, at most 257 characters, and -`url` at most 2048. `attach` succeeds when the bearer's GitHub account has pull -access to the document's repository. After `refused` the server closes the link. - -The server checks the account's admission and pull access again before every -`start` and every `turn`, and every 5 minutes while the link is attached. Once -either is lost it sends `refused` with `access-revoked` and closes the link with -4403; the turn that triggered the check fails. A GitHub outage during a periodic -check leaves the link open. - -| Refusal code | Close code | Meaning | -| ---------------------- | ---------- | ------------------------------------------------- | -| `unsupported-version` | 4426 | The server speaks another `version` | -| `repository-forbidden` | 4403 | The account cannot read the document's repository | -| `document-unavailable` | 4404 | No document matches, or it is being deleted | -| `document-archived` | 4404 | The document is archived | -| `access-revoked` | 4403 | Access was lost after attaching (see below) | -| `unavailable` | 1011 | The server could not check the document | +`url` at most 2048. After `refused` the server closes the link. + +### Ownership + +Attaching makes the client the document's Planner and its account the +document's Planner owner. `attach` succeeds when all of these hold: + +- The bearer's GitHub account has push or administration access to the + document's repository. Pull access alone is refused with + `repository-forbidden`. +- The same account is signed in to Chopin in a browser, on this server, since + the server last started. Chopin's repository tools run with that sign-in's + GitHub App token, never with the link's bearer. Without one, `attach` is + refused with `sign-in-required`; sign in at the server's origin and attach + again. +- That sign-in also passes instance admission and has push or administration + access to the repository. +- No other client holds the document's Planner (see below). + +These are the checks every Planner owner passes. A document has one Planner at a +time. While a client is attached, any other `attach` for that document is +refused with `planner-attached`, including one from the same account, and the +`message` names the holder's GitHub login: + +```text +octocat is attached as this document's Planner. A document has one Planner at a time. +``` + +When the link closes, whether by `detach` or a dropped socket, its account keeps +the ownership for a grace period, 2 minutes unless the operator sets +`PLANNER_ATTACH_GRACE_MS`. During it the same account can attach again and keeps +the ownership; another account is refused with `planner-attached`, and the +`message` says until when the holder keeps it: + +```text +octocat detached from this document's Planner and holds it until 2026-10-10T19:42:00.000Z unless they reattach. A document has one Planner at a time. +``` + +When the grace period ends the ownership is released and any eligible account +can attach. A server restart releases every attachment and ownership at once; +reattach after it. + +The server checks the account's admission and push or administration access +again before every `start` and every `turn`, and every 5 minutes while the link +is attached. Once either is lost, or the document is archived or deleted, it +sends `refused` with `access-revoked` and closes the link with 4403; the turn +that triggered the check fails. A GitHub outage during a periodic check leaves +the link open. + +| Refusal code | Close code | Meaning | +| ---------------------- | ---------- | ---------------------------------------------------------------------- | +| `unsupported-version` | 4426 | The server speaks another `version` | +| `repository-forbidden` | 4403 | The account cannot push to or administer the document's repository | +| `sign-in-required` | 4401 | The account has no live Chopin browser sign-in on this server | +| `planner-attached` | 4409 | Another client holds the document's Planner; `message` names its login | +| `document-unavailable` | 4404 | No document matches, or it is being deleted | +| `document-archived` | 4404 | The document is archived | +| `access-revoked` | 4403 | Access was lost after attaching (see above) | +| `unavailable` | 1011 | The server could not check the document | + +### Without an attached Planner + +On a server running `HARNESS=remote`, an `@chopin` message in the browser and +an MCP `invoke_planner` call for a document with no attached client are refused +with the code `planner-not-attached`, and nothing is posted or queued. A +document whose Planner is within its grace period counts as unattached. The +refusal's message names the document URL: + +```text +No Planner is attached to this document. To attach one, run pdd-plan with plan_document set to from a session in a checkout of ; that session becomes this document's Planner. +``` + +[Local agent MCP](local-agent-mcp.md#hand-an-instruction-to-the-planner) shows +how `invoke_planner` returns it. Once a client is attached, both arrive as +`turn`s on its link. ## Server to client @@ -185,7 +243,8 @@ check leaves the link open. before `turn-end` so the server keeps the session while runs are live. Reports only change run cards and Chat notices; they never start a Planner turn. - `control-result` answers `stop` or `resume`. -- `detach` closes the link with code 1000. +- `detach` closes the link with code 1000. The account keeps the document's + Planner for the grace period in [Ownership](#ownership). Messages for a session or turn that has ended are ignored, except a `host-tool-call`, which gets an error result (`The turn has ended.` or @@ -321,16 +380,19 @@ ids. When the link closes, the server shows the reported live runs as stopped. | 1008 | `error` with `malformed-message` or `protocol` | Unparseable, invalid, or out-of-order message | | 1009 | `error` with `message-too-large` | A frame over 2 MiB | | 1011 | `refused` with `unavailable` | The server could not check the document | +| 4401 | `refused` | `sign-in-required` | | 4403 | `refused` | `repository-forbidden` or `access-revoked` | | 4404 | `refused` | `document-unavailable` or `document-archived` | +| 4409 | `refused` | `planner-attached` | | 4426 | `refused` | `unsupported-version` | `protocol` covers a message before `attach` is answered, a second `attach`, and a repeated `input-request` id the server remembers. When the link closes for any reason, the server ends a running turn with an error, stops the turn's host tools still running, withdraws open questions and the Decisions those tools -opened, and forgets the link; reattaching starts new sessions. A server never -replays an interrupted turn. +opened, and forgets the link; reattaching starts new sessions. The account keeps +the document's Planner ownership for the grace period described in +[Ownership](#ownership). A server never replays an interrupted turn. A client that receives a frame it cannot parse should close with 4400 and reason `malformed-message`. Node.js's `WebSocket` lets a caller close only with 1000 or diff --git a/docs/self-hosting.md b/docs/self-hosting.md index 203981da6..62da78f2c 100644 --- a/docs/self-hosting.md +++ b/docs/self-hosting.md @@ -255,15 +255,18 @@ bind to a public interface. Startup refuses `HARNESS_AUTH` and requires model. The route exists only with `AGENT` on and `HARNESS=remote`. The trust boundary moves to whoever attaches. A client authenticates with a -GitHub token bearer, checked like hosted MCP, and needs pull access to the -document's repository, rechecked before every session and turn and every 5 -minutes while it stays attached. It then receives what a Planner turn sees, -including repository reads made with the document Planner owner's token, and -decides what the Planner writes during turns writers start; its run reports -never start a turn. The server never runs the client's -tools and gains no shell or filesystem access from this harness. Until one -Planner per document is enforced, the most recently attached link serves new -sessions. Without an attached client, a Planner turn fails as unavailable. See +GitHub token bearer, checked like hosted MCP, and needs push or administration +access to the document's repository, rechecked before every session and turn and +every 5 minutes while it stays attached. The same account must have signed in to +this server in a browser: attaching makes that login the document's Planner +owner, one client per document, and a detached or dropped link keeps it for +`PLANNER_ATTACH_GRACE_MS` so the same account can reattach. The client then +receives what a Planner turn sees, including repository reads made with that +owner's token, and decides what the Planner writes during turns writers start; +its run reports never start a turn. The server never runs the client's tools and +gains no shell or filesystem access from this harness. Without an attached +client, `@chopin` and `invoke_planner` are refused with `planner-not-attached` +and instructions to attach one. See [Remote Planner](hosted-agent.md#remote-planner) for the behaviour and [Planner link](planner-link.md) for the wire format. The link shares the application's origin and port with `/ws` and `/mcp`, so a TLS-terminating proxy @@ -339,6 +342,7 @@ the image. | `PORT` | `8787` | Source-process HTTP and WebSocket port. The supplied image and health check expect internal port 8787. | | `MODEL` | `gpt-6-luna` | Model requested for hosted agent sessions. Required under `HARNESS=pi` and `HARNESS=atomic`; under `atomic` it must be `provider/model` from Atomic's catalog. Optional under `HARNESS=remote`, where the attached client chooses its model and a set value is only passed along as the turn's request. | | `HARNESS` | `copilot-sdk` | Adapter name selected from Chopin's harness map (`copilot-sdk`, `pi`, `atomic`, or `remote`). An unknown name refuses at startup. `remote` runs Planner turns in a client attached at `/planner-link`; see [The remote Planner](#the-remote-planner). `atomic` gives every Planner session shell and filesystem access as the server process's user, hosted instances included; see [Choose and trust a harness](#choose-and-trust-a-harness). | +| `PLANNER_ATTACH_GRACE_MS` | `120000` | Under `HARNESS=remote`, how long a detached or dropped Planner link keeps the document's Planner ownership so the same account can reattach, in milliseconds, an integer between 0 and 3600000. | | `HARNESS_AUTH` | unset | Auth mode forwarded to the selected adapter. For `copilot-sdk`, `direct` and `ai-gateway` are allowed on any bind and `auto` requires a loopback-only `SERVER_HOST`; the adapter does not otherwise consume it. For `pi`, it is required: `auto`, `openai`, `anthropic`, and `custom` require a loopback-only `SERVER_HOST`, only `ai-gateway` is allowed otherwise, and `direct` is always refused. For `atomic`, it is required and must be `auto`, which requires a loopback-only `SERVER_HOST`, or `ai-gateway`; every other value is refused. For `remote`, any value is refused. | | `HARNESS_EXTENSIONS` | unset | Extension or package paths every atomic Planner session loads, as Atomic's `--extension` flag would, separated by the platform path delimiter (`:`, or `;` on Windows). A package's extensions, skills, and workflows all register. Each path must be absolute and exist; any other harness refuses the variable at startup. Background workers never load them. The code runs in the server process as its user. | | `AGENT` | on | Set exactly `off` to prevent hosted agent turns, disable the entire background-job runner, and avoid Copilot CLI startup. | diff --git a/packages/planner-link/src/client.ts b/packages/planner-link/src/client.ts index b81a0a113..de6c5b061 100644 --- a/packages/planner-link/src/client.ts +++ b/packages/planner-link/src/client.ts @@ -67,7 +67,7 @@ export type PlannerLinkHandlers = { export type PlannerLinkOptions = { /** The Chopin origin, or the link's own `ws:`/`wss:` URL. */ url: string | URL; - /** A GitHub token whose account can read the document's repository. */ + /** A GitHub token whose account can push to or administer the document's repository. */ token: string; /** The document's id or URL. */ document: string; diff --git a/packages/planner-link/src/protocol.ts b/packages/planner-link/src/protocol.ts index 07ecea1b2..329805662 100644 --- a/packages/planner-link/src/protocol.ts +++ b/packages/planner-link/src/protocol.ts @@ -18,6 +18,7 @@ export const LINK_CLOSE = { unauthorized: 4401, forbidden: 4403, unavailable: 4404, + attached: 4409, unsupportedVersion: 4426, } as const; @@ -252,6 +253,8 @@ export const refusalCodeSchema = z.enum([ "document-unavailable", "document-archived", "access-revoked", + "planner-attached", + "sign-in-required", "unavailable", ]); diff --git a/packages/protocol/index.d.ts b/packages/protocol/index.d.ts index e789f2183..f042b0843 100644 --- a/packages/protocol/index.d.ts +++ b/packages/protocol/index.d.ts @@ -110,6 +110,12 @@ export declare namespace Session { /** A request could not be served. Carries the `rid` it answers. */ export type Failure = KIND<"session:error"> & { message: string; + /** + * Set for a refusal the client presents in full rather than as a generic + * failure. `planner-not-attached`: a Planner request under `HARNESS=remote` + * to a document no client is attached to; `message` says how to attach one. + */ + code?: "planner-not-attached"; }; /** Liveness, and the smallest thing that proves request correlation works. */ From 7690141e594e9b8530c55aff9f28dea70cdb1218 Mon Sep 17 00:00:00 2001 From: Alex Lavaee Date: Sat, 10 Oct 2026 20:03:30 -0700 Subject: [PATCH 2/4] Release a remote Planner attachment when its owner signs out or is reset An attachment trusted its in-memory link after the stored ownership under it was gone. Signing the owner's browser login out, its expiry, or a repository writer's Planner reset left the link attached, so @chopin and invoke_planner failed later with planner-owner-unavailable instead of planner-not-attached, and no one could attach until the old socket closed. Ending a login now closes every link it owns with sign-in-required (4401) and clears that ownership under its generation; a reset closes the link with access-revoked (4403). Neither starts a grace period, so any eligible account can attach at once. The checks before every start and turn, and the periodic one, also close a link whose stored owner no longer names its login and generation, or whose login expired without being revoked. The documentation now limits planner-not-attached to @chopin and invoke_planner, says how other Planner requests behave unattached, and says model-backed research is unavailable under the remote Planner. Assistant-model: Claude Opus 5.5 (fast) Assistant-workflow: hosted-planner-slices Assistant-duration: 6m converged, estimated 10m Assistant-verification: bun run types passed: every workspace package and e2e Assistant-verification: GIT_CONFIG_GLOBAL=/dev/null bun test passed: 4663 pass, 0 fail Assistant-verification: bun run ci passed: dprint, oxlint, tokens, design contract, Impeccable Assistant-verification: mutation check passed: disabling sessionRevoked, ownerReset and verify fails all six new link and chat service tests Co-authored-by: Alex Lavaee --- AGENTS.md | 5 +- apps/server/src/chat/remote-planner.test.ts | 58 +++++++++- apps/server/src/harness/remote/attachments.ts | 96 ++++++++++++++--- apps/server/src/harness/remote/endpoint.ts | 3 +- apps/server/src/harness/remote/link.test.ts | 101 ++++++++++++++++++ apps/server/src/harness/remote/link.ts | 6 ++ apps/server/src/main.ts | 7 +- apps/server/src/testing/planner-link.ts | 19 +++- docs/architecture.md | 4 +- docs/hosted-agent.md | 18 +++- docs/local-agent-mcp.md | 3 +- docs/planner-link.md | 25 ++++- 12 files changed, 317 insertions(+), 28 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 87d7585ef..59c8fca4e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -142,8 +142,9 @@ collaborative state and external implementation runs are durable. host tools, Decisions, and room writes still run in the server. The attached client sees what a turn sees and steers its document writes. Attaching makes the account's live browser login the document's Planner owner, one client per - document; nothing else claims ownership, and Planner requests to a document - without one fail with `planner-not-attached`. See + document; nothing else claims ownership. `@chopin` and `invoke_planner` on a + document without one fail with `planner-not-attached`. Signing out the + owner's login, its expiry, or a Planner reset closes the link. See [Hosted agent](docs/hosted-agent.md#remote-planner). - **Repository node IDs are authoritative.** Owner and repository names resolve GitHub requests but never replace the stored node identity. diff --git a/apps/server/src/chat/remote-planner.test.ts b/apps/server/src/chat/remote-planner.test.ts index 90ba2abdc..f011c2fb8 100644 --- a/apps/server/src/chat/remote-planner.test.ts +++ b/apps/server/src/chat/remote-planner.test.ts @@ -7,7 +7,14 @@ import { notAttachedMessage } from "../harness/remote/attachments"; import { openPlannerSession } from "../harness/session"; import * as Plan from "../plan/service"; import { configured } from "../testing/config"; -import { attach, finishTurn, linkAuth, linkServer, scriptedPlanner } from "../testing/planner-link"; +import { + attach, + finishTurn, + linkAuth, + linkServer, + RIVAL_TOKEN, + scriptedPlanner, +} from "../testing/planner-link"; import * as Chat from "./service"; import type { Server } from "bun"; @@ -41,9 +48,10 @@ function card(status: "running" | "paused") { * login of their own; only the attached member may own the Planner. */ async function remoteRoom() { - let { auth, storage, channel, logins } = await linkAuth(); + let { auth, storage, channel, logins, revocations, signOut } = await linkAuth(); let server = linkServer(auth); cleanups.push(server.stop); + revocations.add(id => server.attachments.sessionRevoked(id)); let lease = (await storage.leases.acquire("writer", "test", 60_000))!; let events: Array<{ kind: string; [key: string]: unknown }> = []; let publisher = { @@ -115,7 +123,7 @@ async function remoteRoom() { }; let owner = async () => (await storage.channels.readAgent(channel.id, new Date()))?.agent?.ownerSessionId; - return { context, server, channel, logins, url, ws, send, owner, events }; + return { context, server, storage, channel, logins, url, ws, send, owner, events, signOut }; } test("with no attached Planner, @chopin and invoke_planner are refused with how to attach one, and nothing is posted or queued", async () => { @@ -225,3 +233,47 @@ test("an attached Planner receives @chopin and invoke_planner turns as their own "@rival resumed the Planner's workflows.", ); }); + +test("once the attached Planner's owner signs out or is reset, @chopin and invoke_planner are refused again, and another member can attach", async () => { + let room = await remoteRoom(); + let { context } = room; + let refusals = async () => { + expect(await room.send("@chopin draft the plan")).toMatchObject({ + kind: "session:error", + code: "planner-not-attached", + }); + expect(await Chat.invoke(context, RIVAL, "Draft the plan")).toBe("planner-not-attached"); + expect(context.chat.entries).toEqual([]); + expect(context.chat.waiting).toEqual([]); + }; + + let member = await attach(room.server.url, room.channel.id, scriptedPlanner().handlers); + expect(await room.owner()).toBe(room.logins.get("member")); + await room.signOut("member"); + expect(await member.closed).toEqual({ code: 4401, reason: "sign-in-required" }); + expect(await room.owner()).toBeUndefined(); + await refusals(); + + let script = scriptedPlanner(); + let rival = await attach(room.server.url, room.channel.id, script.handlers, RIVAL_TOKEN); + expect(await room.owner()).toBe(room.logins.get("rival")); + let held = (await room.storage.channels.readAgent(room.channel.id, new Date()))!.agent!; + await room.storage.channels.clearAgentOwner( + room.channel.id, + held.ownerSessionId!, + held.generation, + new Date(), + ); + room.server.attachments.ownerReset(room.channel.id); + expect(await rival.closed).toEqual({ code: 4403, reason: "access-revoked" }); + await refusals(); + expect(script.turns).toEqual([]); + + let again = await attach(room.server.url, room.channel.id, script.handlers, RIVAL_TOKEN); + cleanups.push(() => again.detach()); + expect(await room.send("@chopin draft the plan")).toMatchObject({ kind: "chat:send" }); + await context.chat.running; + expect(script.turns.map(turn => turn.prompt)).toEqual([ + expect.stringContaining("draft the plan"), + ]); +}); diff --git a/apps/server/src/harness/remote/attachments.ts b/apps/server/src/harness/remote/attachments.ts index 58bf7943d..4c915f612 100644 --- a/apps/server/src/harness/remote/attachments.ts +++ b/apps/server/src/harness/remote/attachments.ts @@ -7,6 +7,8 @@ * tools act with that login's GitHub App token, never with the link's bearer. * A dropped or detached link keeps its ownership for a grace period, so the * same account can reattach without losing it; then the ownership is cleared. + * Ending that login, resetting the document's Planner, or any other release of + * the stored ownership closes the link and frees the document at once. */ import { GitHubError } from "../../github/client"; @@ -24,7 +26,7 @@ export type AttachmentAuth = { storage: Pick; github: Pick; admission: Pick; - sessions: Pick; + sessions: Pick; clock: () => Date; }; @@ -36,7 +38,7 @@ export type AttachRequest = { export type AttachRefusal = { ok: false; - code: "planner-attached" | "sign-in-required" | "repository-forbidden"; + code: "planner-attached" | "sign-in-required" | "repository-forbidden" | "access-revoked"; message: string; }; @@ -91,6 +93,55 @@ export class PlannerAttachments { return this.#serialized(request.channelId, () => this.#attach(request, link)); } + /** The browser login ended: every document it owns through an attachment is released. */ + async sessionRevoked(sessionId: string): Promise { + let held = [...this.#holds.values()].filter(hold => hold.ownerSessionId === sessionId); + for (let hold of held) { + this.#invalidate(hold, signInEndedRefusal(this.#auth.config.origin, hold.login)); + } + await Promise.all(held.map(hold => this.#release(hold))); + } + + /** A repository writer reset the document's Planner, which already cleared its stored owner. */ + ownerReset(channelId: string): void { + let hold = this.#holds.get(channelId); + if (!hold) return; + this.#invalidate(hold, { + ok: false, + code: "access-revoked", + message: `A repository writer reset this document's Planner, so ${hold.login} is no longer ` + + `attached. Attach again to resume.`, + }); + } + + /** + * Whether `link` still holds the document: the stored owner is still its + * attachment's login and generation, and that login is still live. + * Otherwise the link closes and the document is free to attach. + */ + async verify(channelId: string, link: PlannerLink): Promise { + let hold = this.#holds.get(channelId); + if (hold?.link !== link) return false; + let stored = await this.#auth.storage.channels.readAgent(channelId, this.#auth.clock()); + if (this.#holds.get(channelId) !== hold || hold.link !== link) return false; + if ( + stored?.agent?.ownerSessionId !== hold.ownerSessionId + || stored.agent.generation !== hold.generation + ) { + this.#invalidate(hold, { + ok: false, + code: "access-revoked", + message: `This document's Planner ownership was released, so ${hold.login} is no longer ` + + `attached. Attach again to resume.`, + }); + return false; + } + if (await this.#auth.sessions.resolve(hold.ownerSessionId)) return true; + this.#invalidate(hold, signInEndedRefusal(this.#auth.config.origin, hold.login)); + await this.#release(hold); + return false; + } + /** Stops every grace timer. Ownership stays in storage, which startup clears. */ close(): void { for (let hold of this.#holds.values()) clearTimeout(hold.timer); @@ -191,19 +242,30 @@ export class PlannerAttachments { return this.#serialized(hold.channelId, async () => { if (this.#holds.get(hold.channelId) !== hold || hold.link) return; this.#drop(hold); - try { - await this.#auth.storage.channels.clearAgentOwner( - hold.channelId, - hold.ownerSessionId, - hold.generation, - this.#auth.clock(), - ); - } catch (error) { - console.error("[planner-link] releasing the Planner owner failed:", error); - } + await this.#release(hold); }); } + /** Forgets the hold before its link closes, so closing starts no grace period. */ + #invalidate(hold: Hold, refusal: AttachRefusal): void { + if (this.#holds.get(hold.channelId) !== hold) return; + this.#drop(hold); + hold.link?.revoke(refusal.code, refusal.message); + } + + async #release(hold: Hold): Promise { + try { + await this.#auth.storage.channels.clearAgentOwner( + hold.channelId, + hold.ownerSessionId, + hold.generation, + this.#auth.clock(), + ); + } catch (error) { + console.error("[planner-link] releasing the Planner owner failed:", error); + } + } + #drop(hold: Hold): void { clearTimeout(hold.timer); hold.timer = undefined; @@ -238,3 +300,13 @@ function signInRefusal(origin: string, login: string): AttachRefusal { + `The Planner's repository tools act with that sign-in.`, }; } + +function signInEndedRefusal(origin: string, login: string): AttachRefusal { + return { + ok: false, + code: "sign-in-required", + message: `The Chopin browser sign-in that made ${login} this document's Planner ended, so the ` + + `Planner was released. Sign in to Chopin at ${origin} in a browser as ${login}, then ` + + `attach again.`, + }; +} diff --git a/apps/server/src/harness/remote/endpoint.ts b/apps/server/src/harness/remote/endpoint.ts index de2891c23..59531b47e 100644 --- a/apps/server/src/harness/remote/endpoint.ts +++ b/apps/server/src/harness/remote/endpoint.ts @@ -147,7 +147,8 @@ export function plannerLink( let located = await locateDocument(auth, caller, channelId); return typeof located === "object" && located.channel.id === channelId && writable(located.repository) - && !located.channel.archivedAt && !hooks.unavailable?.(channelId); + && !located.channel.archivedAt && !hooks.unavailable?.(channelId) + && await hooks.attachments.verify(channelId, link); } catch (err) { if (revoked(err)) return false; throw err; diff --git a/apps/server/src/harness/remote/link.test.ts b/apps/server/src/harness/remote/link.test.ts index 8f3427c0e..b23f6d750 100644 --- a/apps/server/src/harness/remote/link.test.ts +++ b/apps/server/src/harness/remote/link.test.ts @@ -329,6 +329,107 @@ describe("Planner link ownership", () => { expect(await client.closed).toEqual({ code: LINK_CLOSE.forbidden, reason: "access-revoked" }); expect(server.links).toEqual([]); }); + + /** A member's raw link attached to the document, with ended logins reaching `attachments`. */ + async function attachedMember(revalidateEveryMs?: number) { + let linked = await linkAuth(); + let server = linkServer(linked.auth, 2_000, revalidateEveryMs, 2_000); + cleanups.push(server.stop); + linked.revocations.add(id => server.attachments.sessionRevoked(id)); + let raw = await rawLink(server.url); + raw.send({ type: "attach", version: PLANNER_LINK_VERSION, document: linked.channel.id }); + await raw.next("attached"); + expect(server.attachments.attached(linked.channel.id)).toBe(true); + return { ...linked, server, raw }; + } + + async function rivalAttaches( + url: string, + channelId: string, + storage: Awaited>["storage"], + rival: string, + ) { + let client = await attach(url, channelId, scriptedPlanner().handlers, RIVAL_TOKEN); + cleanups.push(() => client.detach()); + expect(await owner(storage, channelId)).toMatchObject({ ownerSessionId: rival }); + } + + it("releases the Planner at once when its owner signs out of the browser", async () => { + let { server, raw, channel, storage, logins, signOut } = await attachedMember(); + expect(await signOut("member")).toBe(logins.get("member")); + expect(await raw.next("refused")).toEqual({ + type: "refused", + code: "sign-in-required", + message: "The Chopin browser sign-in that made member this document's Planner ended, so " + + "the Planner was released. Sign in to Chopin at https://chopin.test in a browser as " + + "member, then attach again.", + }); + expect((await raw.closed).code).toBe(LINK_CLOSE.unauthorized); + expect(server.attachments.attached(channel.id)).toBe(false); + expect(server.attachments.holder(channel.id)).toBeUndefined(); + expect((await owner(storage, channel.id))?.ownerSessionId).toBeUndefined(); + await rivalAttaches(server.url, channel.id, storage, logins.get("rival")!); + }); + + it("releases the Planner when its owner's browser sign-in expires", async () => { + let { server, raw, channel, storage, sessions, clock } = await attachedMember(); + clock.skewMs = 31 * 24 * 60 * 60 * 1_000; + await sessions.cleanupExpired(); + expect(await raw.next("refused")).toMatchObject({ code: "sign-in-required" }); + expect((await raw.closed).code).toBe(LINK_CLOSE.unauthorized); + expect(server.attachments.attached(channel.id)).toBe(false); + expect((await owner(storage, channel.id))?.ownerSessionId).toBeUndefined(); + }); + + it("notices an expired owner sign-in on its periodic recheck when nothing revoked it", async () => { + let { server, raw, channel, storage, clock } = await attachedMember(20); + clock.skewMs = 31 * 24 * 60 * 60 * 1_000; + expect(await raw.next("refused")).toMatchObject({ code: "sign-in-required" }); + expect((await raw.closed).code).toBe(LINK_CLOSE.unauthorized); + expect(server.attachments.attached(channel.id)).toBe(false); + expect((await owner(storage, channel.id))?.ownerSessionId).toBeUndefined(); + }); + + it("releases the attachment when a repository writer resets the document's Planner", async () => { + let { server, raw, channel, storage, logins } = await attachedMember(); + let held = (await owner(storage, channel.id))!; + await storage.channels.clearAgentOwner( + channel.id, + held.ownerSessionId!, + held.generation, + new Date(), + ); + server.attachments.ownerReset(channel.id); + expect(await raw.next("refused")).toEqual({ + type: "refused", + code: "access-revoked", + message: "A repository writer reset this document's Planner, so member is no longer " + + "attached. Attach again to resume.", + }); + expect((await raw.closed).code).toBe(LINK_CLOSE.forbidden); + expect(server.attachments.attached(channel.id)).toBe(false); + await rivalAttaches(server.url, channel.id, storage, logins.get("rival")!); + }); + + it("closes a link whose stored ownership was released elsewhere on its next recheck", async () => { + let { server, raw, channel, storage, logins } = await attachedMember(20); + let held = (await owner(storage, channel.id))!; + await storage.channels.clearAgentOwner( + channel.id, + held.ownerSessionId!, + held.generation, + new Date(), + ); + expect(await raw.next("refused")).toEqual({ + type: "refused", + code: "access-revoked", + message: "This document's Planner ownership was released, so member is no longer " + + "attached. Attach again to resume.", + }); + expect((await raw.closed).code).toBe(LINK_CLOSE.forbidden); + expect(server.attachments.attached(channel.id)).toBe(false); + await rivalAttaches(server.url, channel.id, storage, logins.get("rival")!); + }); }); describe("Planner link protocol", () => { diff --git a/apps/server/src/harness/remote/link.ts b/apps/server/src/harness/remote/link.ts index 5a299e1d7..f84f370f3 100644 --- a/apps/server/src/harness/remote/link.ts +++ b/apps/server/src/harness/remote/link.ts @@ -643,6 +643,12 @@ export class PlannerLink { this.#dispatch(message); } + /** Tells the client why it is no longer the Planner, then closes the link. */ + revoke(code: RefusalCode, message: string): void { + if (this.#state === "closed") return; + this.#refuse(code, message); + } + /** Ends the link from the server side. */ close(code: number, reason: string): void { if (this.#state === "closed") return; diff --git a/apps/server/src/main.ts b/apps/server/src/main.ts index 76e59f3e1..727abcd05 100644 --- a/apps/server/src/main.ts +++ b/apps/server/src/main.ts @@ -1620,7 +1620,11 @@ async function sessionRevoked(sessionId: string): Promise { experiments?.connections.revokeSession(sessionId); let jobs = jobRunner?.ownerRevoked(sessionId); ownerBindings?.revokeSession(sessionId); - await Promise.all([resetOpenAgents(() => true, sessionId), jobs]); + await Promise.all([ + resetOpenAgents(() => true, sessionId), + jobs, + plannerAttachments?.sessionRevoked(sessionId), + ]); } async function credentialsWillRotate(sessionId: string, revision: number): Promise { @@ -1640,6 +1644,7 @@ async function credentialsWillRotate(sessionId: string, revision: number): Promi async function channelOwnerReset(channelId: string): Promise { let jobs = jobRunner?.channelOwnerReset(channelId); ownerBindings?.revokeChannel(channelId); + plannerAttachments?.ownerReset(channelId); await Promise.all([resetOpenAgents(room => room.id === channelId), jobs]); } diff --git a/apps/server/src/testing/planner-link.ts b/apps/server/src/testing/planner-link.ts index ce4acbee1..55a4b38a9 100644 --- a/apps/server/src/testing/planner-link.ts +++ b/apps/server/src/testing/planner-link.ts @@ -61,8 +61,15 @@ export async function linkAuth(channelId?: string) { for (let account of Object.values(ACCOUNTS)) { await storage.users.put({ id: account.id, login: account.login, avatarUrl: "", now }); } - let sessions = new Sessions(storage, false); + let clock = { skewMs: 0 }; + let revocations = new Set<(sessionId: string) => Promise>(); + let sessions = new Sessions(storage, false, () => new Date(Date.now() + clock.skewMs), { + onRevoked: async id => { + for (let revoked of revocations) await revoked(id); + }, + }); let logins = new Map(); + let cookies = new Map(); for (let account of Object.values(ACCOUNTS)) { if (!account.signedIn) continue; let issued = await sessions.issue(account.id, { @@ -72,6 +79,7 @@ export async function linkAuth(channelId?: string) { refreshExpiresIn: 86_400, }); logins.set(account.login, issued.id); + cookies.set(account.login, issued.cookie.split(";")[0]!); } let channel = await storage.channels.create({ id: channelId ?? crypto.randomUUID(), @@ -117,6 +125,15 @@ export async function linkAuth(channelId?: string) { sessions, logins, channel, + /** Moves the browser logins' clock, so they expire without the server's clock moving. */ + clock, + /** Called with every browser login that ends, as `main.ts` wires `onSessionRevoked`. */ + revocations, + /** Signs `login` out of the browser, as the logout route does. */ + async signOut(login: string) { + let cookie = cookies.get(login)!; + return sessions.revoke(new Request("https://chopin.test/", { headers: { cookie } })); + }, }; } diff --git a/docs/architecture.md b/docs/architecture.md index 6dba7cebf..0499232b3 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -148,7 +148,9 @@ a GitHub bearer, checked like `/mcp`. The client attaches to one document and needs push or administration access to its repository, rechecked before every session and turn and periodically while attached. Attaching makes the account's live browser login the document's Planner owner, one client per document; a -document without one refuses Planner requests with `planner-not-attached`. +document without one refuses `@chopin` and `invoke_planner` with +`planner-not-attached`. The link closes when that login ends or the document's +Planner is reset. Turns run on the client's model; Chopin's host tools, Decisions, and room writes still run in the server under the document's Planner owner and the permission checks above. The client sees what a turn sees diff --git a/docs/hosted-agent.md b/docs/hosted-agent.md index 94d3f99ea..24a9456b0 100644 --- a/docs/hosted-agent.md +++ b/docs/hosted-agent.md @@ -52,10 +52,20 @@ call follows the same rules. Its instruction is posted as the caller's own message; the turn runs under the channel's current owner, whoever that is. A channel without one is claimed for the caller's live browser login, hosted or local, and without such a login the call is refused and nothing is posted. -Under `HARNESS=remote` neither `invoke_planner`, `@chopin`, nor a research -request claims ownership: a document without an attached Planner refuses them -with `planner-not-attached`, and one with an attached Planner runs them under -its owner. +Under `HARNESS=remote` only attaching claims ownership. An `@chopin` message or +`invoke_planner` call for a document without an attached Planner is refused with +`planner-not-attached`, and with one it runs as a turn on that Planner under its +owner. Other Planner requests, such as sending a comment to the Planner or a +Build draft, claim nothing either: with a client attached they run on it, and +without one they fail as any request without a usable owner does, not with +`planner-not-attached`. Model-backed research is unavailable, because the remote +Planner requires `BACKGROUND_JOBS=off`; a research request claims no ownership +and is refused as not ready. + +An attachment lasts only as long as its owner. Signing that browser login out, +its expiry, or a repository writer's reset of the document's Planner closes the +attached link at once and frees the document for any eligible account to attach; +see [Planner link ownership](planner-link.md#ownership). Planner ownership stores only the owner session ID, referring to a login loaded in this process. Hosted logins separately persist encrypted credentials in diff --git a/docs/local-agent-mcp.md b/docs/local-agent-mcp.md index 05680d698..5e161a909 100644 --- a/docs/local-agent-mcp.md +++ b/docs/local-agent-mcp.md @@ -96,7 +96,8 @@ A server running `HARNESS=remote` claims no ownership here. Its Planner runs in a client attached to the document over the [Planner link](planner-link.md), and only attaching makes an account the owner. With a client attached the turn goes to it and runs under its owner, whoever invoked it. Without one, including -while a detached Planner's grace period runs, the call is refused with +while a detached Planner's grace period runs and once the attached Planner's +owner signs out of the browser or its Planner is reset, the call is refused with `planner-not-attached` and a `message`, and nothing is posted or queued: ```json diff --git a/docs/planner-link.md b/docs/planner-link.md index d3dff86a7..f365549ed 100644 --- a/docs/planner-link.md +++ b/docs/planner-link.md @@ -130,15 +130,36 @@ sends `refused` with `access-revoked` and closes the link with 4403; the turn that triggered the check fails. A GitHub outage during a periodic check leaves the link open. +The ownership also ends, without a grace period, when the browser sign-in it +rests on ends or the document's Planner is reset. The server then sends +`refused` and closes an attached link at once, and any eligible account, +including the same one, can attach again immediately: + +| Cause | Refusal code | Close code | +| -------------------------------------------------------------- | ------------------ | ---------- | +| The owner's browser sign-in signs out or expires | `sign-in-required` | 4401 | +| A repository writer resets the document's Planner | `access-revoked` | 4403 | +| The stored ownership no longer names this attachment's sign-in | `access-revoked` | 4403 | + +The same checks before every `start` and `turn`, and the periodic one, also +find an expired sign-in or a released ownership that nothing reported. The +`message` says what happened: + +```text +The Chopin browser sign-in that made octocat this document's Planner ended, so the Planner was released. Sign in to Chopin at https://chopin.example in a browser as octocat, then attach again. +A repository writer reset this document's Planner, so octocat is no longer attached. Attach again to resume. +This document's Planner ownership was released, so octocat is no longer attached. Attach again to resume. +``` + | Refusal code | Close code | Meaning | | ---------------------- | ---------- | ---------------------------------------------------------------------- | | `unsupported-version` | 4426 | The server speaks another `version` | | `repository-forbidden` | 4403 | The account cannot push to or administer the document's repository | -| `sign-in-required` | 4401 | The account has no live Chopin browser sign-in on this server | +| `sign-in-required` | 4401 | The account has no live Chopin browser sign-in, or it ended | | `planner-attached` | 4409 | Another client holds the document's Planner; `message` names its login | | `document-unavailable` | 4404 | No document matches, or it is being deleted | | `document-archived` | 4404 | The document is archived | -| `access-revoked` | 4403 | Access was lost after attaching (see above) | +| `access-revoked` | 4403 | Access or the Planner ownership was lost after attaching (see above) | | `unavailable` | 1011 | The server could not check the document | ### Without an attached Planner From ae7334e17242b3e9b926a14828ac4f79f871e1a8 Mon Sep 17 00:00:00 2001 From: Alex Lavaee Date: Sat, 10 Oct 2026 20:18:45 -0700 Subject: [PATCH 3/4] Refuse an attach whose ownership ended while it was attaching A grace reattach read the stored owner and then bound the hold it had captured, without checking whether a Planner reset or the owner's sign-out had released that hold during the read. A snapshot taken before the reset could still match, so the client reported attached while the server had no owner, refused attached turns, and kept an orphaned registry link. Each attach now records the sign-ins it rests on while it runs. A reset of its document or the end of one of those sign-ins marks it, and it binds only if nothing marked it and its owner sign-in is still live after its last await; otherwise it releases any ownership it claimed, under its generation, and is refused with the code an attached link would be closed with. The recheck of an attached link now asks whether the owner's sign-in is live before comparing the stored owner. PostgreSQL and memory storage report an expired sign-in's ownership as released, so an expiry that cleanup had not yet seen closed the link with access-revoked (4403) instead of sign-in-required (4401). Assistant-model: Claude Opus 5.5 (fast) Assistant-workflow: hosted-planner-slices Assistant-duration: 9m converged, estimated 15m Assistant-verification: bun run types passed: every workspace package and e2e Assistant-verification: GIT_CONFIG_GLOBAL=/dev/null bun test passed: 4668 pass, 4 skip, 0 fail Assistant-verification: bun run ci passed: dprint, oxlint, tokens, design contract, Impeccable Assistant-verification: PostgreSQL contract passed: bun test apps/server/src/storage/postgres against disposable postgres:17-alpine on 55432, 85 pass, container stopped Assistant-verification: mutation check passed: the previous attachments.ts fails all five new link and storage contract tests Co-authored-by: Alex Lavaee --- apps/server/src/harness/remote/attachments.ts | 112 +++++++++++++----- apps/server/src/harness/remote/link.test.ts | 89 ++++++++++++++ .../storage/planner-attachment-contract.ts | 90 +++++++++++++- docs/hosted-agent.md | 8 +- docs/planner-link.md | 8 +- 5 files changed, 272 insertions(+), 35 deletions(-) diff --git a/apps/server/src/harness/remote/attachments.ts b/apps/server/src/harness/remote/attachments.ts index 4c915f612..aeec2cd12 100644 --- a/apps/server/src/harness/remote/attachments.ts +++ b/apps/server/src/harness/remote/attachments.ts @@ -56,6 +56,13 @@ type Hold = { releaseAt?: Date; }; +type PendingAttach = { + login: string; + held?: Hold; + sessionIds: Set; + invalidated?: AttachRefusal; +}; + /** What `@chopin` and `invoke_planner` say when no Planner is attached to the document. */ export function notAttachedMessage(url: string, repository: string): string { return `No Planner is attached to this document. To attach one, run pdd-plan with ` @@ -71,6 +78,7 @@ export class PlannerAttachments { #auth: AttachmentAuth; #graceMs: number; #holds = new Map(); + #pending = new Map(); #queues = new Map>(); constructor(auth: AttachmentAuth, graceMs: number) { @@ -95,50 +103,52 @@ export class PlannerAttachments { /** The browser login ended: every document it owns through an attachment is released. */ async sessionRevoked(sessionId: string): Promise { - let held = [...this.#holds.values()].filter(hold => hold.ownerSessionId === sessionId); - for (let hold of held) { - this.#invalidate(hold, signInEndedRefusal(this.#auth.config.origin, hold.login)); + let origin = this.#auth.config.origin; + for (let pending of this.#pending.values()) { + if (!pending.sessionIds.has(sessionId)) continue; + pending.invalidated ??= pending.held?.ownerSessionId === sessionId + ? signInEndedRefusal(origin, pending.login) + : signInRefusal(origin, pending.login); } + let held = [...this.#holds.values()].filter(hold => hold.ownerSessionId === sessionId); + for (let hold of held) this.#invalidate(hold, signInEndedRefusal(origin, hold.login)); await Promise.all(held.map(hold => this.#release(hold))); } /** A repository writer reset the document's Planner, which already cleared its stored owner. */ ownerReset(channelId: string): void { + let pending = this.#pending.get(channelId); + if (pending) pending.invalidated ??= resetRefusal(pending.login); let hold = this.#holds.get(channelId); - if (!hold) return; - this.#invalidate(hold, { - ok: false, - code: "access-revoked", - message: `A repository writer reset this document's Planner, so ${hold.login} is no longer ` - + `attached. Attach again to resume.`, - }); + if (hold) this.#invalidate(hold, resetRefusal(hold.login)); } /** - * Whether `link` still holds the document: the stored owner is still its - * attachment's login and generation, and that login is still live. + * Whether `link` still holds the document: its login is still live and the + * stored owner is still its attachment's login and generation. * Otherwise the link closes and the document is free to attach. */ async verify(channelId: string, link: PlannerLink): Promise { let hold = this.#holds.get(channelId); if (hold?.link !== link) return false; let stored = await this.#auth.storage.channels.readAgent(channelId, this.#auth.clock()); + let live = await this.#auth.sessions.resolve(hold.ownerSessionId); if (this.#holds.get(channelId) !== hold || hold.link !== link) return false; - if ( - stored?.agent?.ownerSessionId !== hold.ownerSessionId - || stored.agent.generation !== hold.generation - ) { - this.#invalidate(hold, { - ok: false, - code: "access-revoked", - message: `This document's Planner ownership was released, so ${hold.login} is no longer ` - + `attached. Attach again to resume.`, - }); + if (!live) { + this.#invalidate(hold, signInEndedRefusal(this.#auth.config.origin, hold.login)); + await this.#release(hold); return false; } - if (await this.#auth.sessions.resolve(hold.ownerSessionId)) return true; - this.#invalidate(hold, signInEndedRefusal(this.#auth.config.origin, hold.login)); - await this.#release(hold); + if ( + stored?.agent?.ownerSessionId === hold.ownerSessionId + && stored.agent.generation === hold.generation + ) return true; + this.#invalidate(hold, { + ok: false, + code: "access-revoked", + message: `This document's Planner ownership was released, so ${hold.login} is no longer ` + + `attached. Attach again to resume.`, + }); return false; } @@ -152,17 +162,49 @@ export class PlannerAttachments { let { channelId, user } = request; let held = this.#holds.get(channelId); if (held && (held.link || held.userId !== user.id)) return attachedRefusal(held); + let pending: PendingAttach = { + login: user.login, + held, + sessionIds: new Set(held ? [held.ownerSessionId] : []), + }; + this.#pending.set(channelId, pending); + try { + return await this.#attachPending(request, link, pending); + } finally { + if (this.#pending.get(channelId) === pending) this.#pending.delete(channelId); + } + } + + /** + * Takes or keeps the document for `request` while `pending` collects every + * reset or ended login that lands during its awaits, so a hold binds only + * if nothing released its ownership in the meantime. + */ + async #attachPending( + request: AttachRequest, + link: PlannerLink, + pending: PendingAttach, + ): Promise { + let { channelId, user } = request; let login = await this.#eligibleLogin(request); if ("ok" in login) return login; + pending.sessionIds.add(login.session.id); + if (pending.invalidated) return pending.invalidated; let now = this.#auth.clock(); - if (held) { + let held = pending.held; + if (held && this.#holds.get(channelId) === held) { let stored = await this.#auth.storage.channels.readAgent(channelId, now); + let live = await this.#auth.sessions.resolve(held.ownerSessionId); + if (pending.invalidated) return pending.invalidated; if ( - stored?.agent?.ownerSessionId === held.ownerSessionId + live && this.#holds.get(channelId) === held + && stored?.agent?.ownerSessionId === held.ownerSessionId && stored.agent.generation === held.generation ) return this.#bind(held, link); this.#drop(held); + if (!live) await this.#release(held); } + if (pending.invalidated) return pending.invalidated; let claimed = await this.#claim(channelId, login.session.id, now); if (!claimed) throw new Error("The document's Planner owner changed while attaching."); let hold: Hold = { @@ -172,6 +214,13 @@ export class PlannerAttachments { ownerSessionId: login.session.id, generation: claimed.generation, }; + let live = await this.#auth.sessions.resolve(hold.ownerSessionId); + let refusal = pending.invalidated + ?? (live ? undefined : signInRefusal(this.#auth.config.origin, user.login)); + if (refusal) { + await this.#release(hold); + return refusal; + } this.#holds.set(channelId, hold); return this.#bind(hold, link); } @@ -284,6 +333,15 @@ export class PlannerAttachments { } } +function resetRefusal(login: string): AttachRefusal { + return { + ok: false, + code: "access-revoked", + message: `A repository writer reset this document's Planner, so ${login} is no longer ` + + `attached. Attach again to resume.`, + }; +} + function attachedRefusal(hold: Hold): AttachRefusal { let message = hold.link || !hold.releaseAt ? `${hold.login} is attached as this document's Planner. A document has one Planner at a time.` diff --git a/apps/server/src/harness/remote/link.test.ts b/apps/server/src/harness/remote/link.test.ts index b23f6d750..9255f1590 100644 --- a/apps/server/src/harness/remote/link.test.ts +++ b/apps/server/src/harness/remote/link.test.ts @@ -390,6 +390,95 @@ describe("Planner link ownership", () => { expect((await owner(storage, channel.id))?.ownerSessionId).toBeUndefined(); }); + it("releases the Planner as sign-in-required when storage already sees its owner's sign-in expired", async () => { + let { auth, server, raw, channel, storage, clock } = await attachedMember(20); + let held = (await owner(storage, channel.id))!; + auth.clock = () => new Date(Date.now() + clock.skewMs); + clock.skewMs = 31 * 24 * 60 * 60 * 1_000; + expect(await raw.next("refused")).toMatchObject({ + code: "sign-in-required", + message: expect.stringContaining("The Chopin browser sign-in that made member"), + }); + expect((await raw.closed).code).toBe(LINK_CLOSE.unauthorized); + expect(server.attachments.attached(channel.id)).toBe(false); + clock.skewMs = 0; + expect(await owner(storage, channel.id)).toMatchObject({ + ownerSessionId: undefined, + generation: held.generation, + status: "unavailable", + }); + }); + + /** Holds the next attach's ownership read until `resume`, once `reading` says it started. */ + function pauseOwnerRead(storage: Awaited>["storage"]) { + let readAgent = storage.channels.readAgent; + let reading = Promise.withResolvers(); + let resume = Promise.withResolvers(); + storage.channels.readAgent = async (channelId, now) => { + storage.channels.readAgent = readAgent; + let snapshot = await readAgent(channelId, now); + reading.resolve(); + await resume.promise; + return snapshot; + }; + return { reading: reading.promise, resume: () => resume.resolve() }; + } + + /** A member detached within the grace period, reattaching on a raw link paused at its ownership read. */ + async function reattachingMember() { + let linked = await linkAuth(); + let server = linkServer(linked.auth, 2_000, undefined, 2_000); + cleanups.push(server.stop); + linked.revocations.add(id => server.attachments.sessionRevoked(id)); + let first = await attach(server.url, linked.channel.id, scriptedPlanner().handlers); + await first.detach(); + let held = (await owner(linked.storage, linked.channel.id))!; + let paused = pauseOwnerRead(linked.storage); + let raw = await rawLink(server.url); + raw.send({ type: "attach", version: PLANNER_LINK_VERSION, document: linked.channel.id }); + await paused.reading; + return { ...linked, server, raw, held, resume: paused.resume }; + } + + it("refuses a grace reattach whose ownership a reset released while it was attaching", async () => { + let { server, raw, channel, storage, logins, held, resume } = await reattachingMember(); + await storage.channels.clearAgentOwner( + channel.id, + held.ownerSessionId!, + held.generation, + new Date(), + ); + server.attachments.ownerReset(channel.id); + resume(); + expect(await raw.next("refused")).toEqual({ + type: "refused", + code: "access-revoked", + message: "A repository writer reset this document's Planner, so member is no longer " + + "attached. Attach again to resume.", + }); + expect((await raw.closed).code).toBe(LINK_CLOSE.forbidden); + expect(server.attachments.attached(channel.id)).toBe(false); + expect(server.attachments.holder(channel.id)).toBeUndefined(); + expect(server.links).toEqual([]); + expect((await owner(storage, channel.id))?.ownerSessionId).toBeUndefined(); + await rivalAttaches(server.url, channel.id, storage, logins.get("rival")!); + }); + + it("refuses a grace reattach whose owner signed out while it was attaching", async () => { + let { server, raw, channel, storage, logins, signOut, resume } = await reattachingMember(); + expect(await signOut("member")).toBe(logins.get("member")); + resume(); + expect(await raw.next("refused")).toMatchObject({ + code: "sign-in-required", + message: expect.stringContaining("The Chopin browser sign-in that made member"), + }); + expect((await raw.closed).code).toBe(LINK_CLOSE.unauthorized); + expect(server.attachments.attached(channel.id)).toBe(false); + expect(server.links).toEqual([]); + expect((await owner(storage, channel.id))?.ownerSessionId).toBeUndefined(); + await rivalAttaches(server.url, channel.id, storage, logins.get("rival")!); + }); + it("releases the attachment when a repository writer resets the document's Planner", async () => { let { server, raw, channel, storage, logins } = await attachedMember(); let held = (await owner(storage, channel.id))!; diff --git a/apps/server/src/storage/planner-attachment-contract.ts b/apps/server/src/storage/planner-attachment-contract.ts index 047e18826..6b1745d48 100644 --- a/apps/server/src/storage/planner-attachment-contract.ts +++ b/apps/server/src/storage/planner-attachment-contract.ts @@ -27,7 +27,9 @@ async function attachable(storage: StorageAdapter) { createdBy: member.id, now, }); - let sessions = new Sessions(storage, false); + let skew = { ms: 0 }; + let clock = () => new Date(Date.now() + skew.ms); + let sessions = new Sessions(storage, false, clock); let signIn = async (user: { id: string }) => (await sessions.issue(user.id, { ...GRANT, accessToken: `${user.id}-login` })).id; let logins = { member: await signIn(member), rival: await signIn(rival) }; @@ -35,7 +37,7 @@ async function attachable(storage: StorageAdapter) { config: { origin: "https://chopin.test" }, storage, sessions, - clock: () => new Date(), + clock, admission: { allowed: async () => true }, github: { repositoryAccess: async () => ({ @@ -50,13 +52,19 @@ async function attachable(storage: StorageAdapter) { }; let owner = async () => (await storage.channels.readAgent(channelId, new Date()))?.agent; let request = (user: { id: string; login: string }) => ({ channelId, repository, user }); - return { auth, sessions, member, rival, logins, channelId, owner, request }; + return { auth, sessions, member, rival, logins, channelId, owner, request, skew }; } function link(): PlannerLink { return {} as PlannerLink; } +function revocable() { + let refusals: { code: string; message: string }[] = []; + let revocable = { revoke: (code: string, message: string) => refusals.push({ code, message }) }; + return { link: revocable as unknown as PlannerLink, refusals }; +} + async function released(owner: () => Promise<{ ownerSessionId?: string } | undefined>) { for (let deadline = Date.now() + 3_000; Date.now() < deadline;) { if (!(await owner())?.ownerSessionId) return; @@ -164,5 +172,81 @@ export function plannerAttachmentContract(name: string, factory: StorageFactory) await storage.close(); } }); + + test("an owner sign-in storage already sees expired releases the attachment as sign-in-required", async () => { + let storage = await openedStorage(factory); + let f = await attachable(storage); + let attachments = new PlannerAttachments(f.auth, 60_000); + try { + let attached = revocable(); + expect((await attachments.attach(f.request(f.member), attached.link)).ok).toBe(true); + let generation = (await f.owner())!.generation; + f.skew.ms = 31 * 24 * 60 * 60 * 1_000; + expect((await storage.channels.readAgent(f.channelId, f.auth.clock()))?.agent) + .toMatchObject({ ownerSessionId: undefined, generation }); + expect(await attachments.verify(f.channelId, attached.link)).toBe(false); + expect(attached.refusals).toEqual([{ + code: "sign-in-required", + message: expect.stringContaining("The Chopin browser sign-in that made member"), + }]); + f.skew.ms = 0; + expect(await f.owner()) + .toMatchObject({ ownerSessionId: undefined, generation, status: "unavailable" }); + expect(attachments.holder(f.channelId)).toBeUndefined(); + } finally { + attachments.close(); + await storage.close(); + } + }); + + test("a reset during a grace reattach refuses it and leaves the document free", async () => { + let storage = await openedStorage(factory); + let f = await attachable(storage); + let reading = Promise.withResolvers(); + let resume = Promise.withResolvers(); + let paused = false; + let channels = { + ...storage.channels, + readAgent: async (channelId: string, now: Date) => { + let snapshot = await storage.channels.readAgent(channelId, now); + if (!paused) return snapshot; + paused = false; + reading.resolve(); + await resume.promise; + return snapshot; + }, + }; + let attachments = new PlannerAttachments({ ...f.auth, storage: { channels } }, 60_000); + try { + let first = await attachments.attach(f.request(f.member), link()); + if (first.ok) first.detach(); + let held = (await f.owner())!; + paused = true; + let reattached = revocable(); + let reattaching = attachments.attach(f.request(f.member), reattached.link); + await reading.promise; + expect( + await storage.channels.clearAgentOwner( + f.channelId, + held.ownerSessionId!, + held.generation, + new Date(), + ), + ).toBe(true); + attachments.ownerReset(f.channelId); + resume.resolve(); + expect(await reattaching).toMatchObject({ ok: false, code: "access-revoked" }); + expect(attachments.attached(f.channelId)).toBe(false); + expect(attachments.holder(f.channelId)).toBeUndefined(); + expect(await f.owner()) + .toMatchObject({ ownerSessionId: undefined, generation: held.generation }); + expect((await attachments.attach(f.request(f.rival), link())).ok).toBe(true); + expect(await f.owner()) + .toMatchObject({ ownerSessionId: f.logins.rival, generation: held.generation + 1 }); + } finally { + attachments.close(); + await storage.close(); + } + }); }); } diff --git a/docs/hosted-agent.md b/docs/hosted-agent.md index 24a9456b0..088288893 100644 --- a/docs/hosted-agent.md +++ b/docs/hosted-agent.md @@ -330,9 +330,11 @@ message naming its GitHub login. A link that detaches or drops keeps the ownership for `PLANNER_ATTACH_GRACE_MS` (2 minutes by default): the same account can attach again within it and keeps the same ownership generation, and anyone else is refused. When it ends, the ownership is cleared under its generation, so -a newer owner is never released by an old timer. Attachments live in the server -process; startup already clears every owner reference, which releases every -attachment. +a newer owner is never released by an old timer. An attach, including a grace +reattach, binds only if no sign-out, expiry, or Planner reset of its ownership +landed while it was checking; otherwise it is refused like an attached link +would be closed. Attachments live in the server process; startup already clears +every owner reference, which releases every attachment. **Unattached documents.** Members, MCP callers, and research requests never claim ownership under `HARNESS=remote`. An `@chopin` message or `invoke_planner` call diff --git a/docs/planner-link.md b/docs/planner-link.md index f365549ed..131d035fd 100644 --- a/docs/planner-link.md +++ b/docs/planner-link.md @@ -142,8 +142,12 @@ including the same one, can attach again immediately: | The stored ownership no longer names this attachment's sign-in | `access-revoked` | 4403 | The same checks before every `start` and `turn`, and the periodic one, also -find an expired sign-in or a released ownership that nothing reported. The -`message` says what happened: +find an expired sign-in or a released ownership that nothing reported; an +expired sign-in is reported as `sign-in-required` even when the stored +ownership already reads as released. An `attach` that is still in progress when +one of these causes lands, including a reattach within the grace period, is +refused with the same code and close code instead of attaching. The `message` +says what happened: ```text The Chopin browser sign-in that made octocat this document's Planner ended, so the Planner was released. Sign in to Chopin at https://chopin.example in a browser as octocat, then attach again. From 114ba56f66bf5d5630f396fc5133e0d40c996ff7 Mon Sep 17 00:00:00 2001 From: Alex Lavaee Date: Sat, 10 Oct 2026 20:33:29 -0700 Subject: [PATCH 4/4] Confirm a remote Planner attachment before accepting a Planner request The Chat and invoke_planner preflight under HARNESS=remote accepted any in-memory attachment with a link, without asking whether its owner's browser sign-in was still live or storage still assigned that owner. If the sign-in expired before the link's periodic recheck, Chat saved and announced the member's message before owner resolution failed, and invoke_planner returned planner-owner-unavailable instead of planner-not-attached. Both now confirm the attachment as the periodic recheck does. A stale one is closed with the code an attached link would get (sign-in-required or access-revoked), its ownership is released, and the request is refused with planner-not-attached and the attach guidance, with nothing saved or queued. A fresh attach registered its sign-in with the attempt only after the repository and admission checks, so a sign-out during them went unnoticed and the ownership claim then threw once storage had deleted the session, refusing the client with unavailable (1011). The sign-in is now registered before those checks, and a claim that storage refuses because the sign-in ended is refused with sign-in-required (4401). Assistant-model: Claude Opus 5.5 (fast) Assistant-workflow: hosted-planner-slices (run b51d4e3c) repair (worker subagent) Assistant-verification: bun run types passed: every workspace package and e2e, 14 exited with code 0 Assistant-verification: GIT_CONFIG_GLOBAL=/dev/null bun test passed: 4672 pass, 4 skip, 0 fail Assistant-verification: bun run ci passed: dprint, oxlint 0 errors, tokens, design contract, Impeccable Assistant-verification: PostgreSQL contract passed: bun test apps/server/src/storage/postgres against disposable postgres:17-alpine on 55432, 85 pass, container stopped Assistant-verification: mutation check passed: the previous attachments.ts and main.ts preflight fail all four new tests; removing only the claim fallback fails the storage-expiry attach test Co-authored-by: Alex Lavaee --- apps/server/src/chat/remote-planner.test.ts | 69 ++++++++++++++++++- apps/server/src/harness/remote/attachments.ts | 35 ++++++++-- apps/server/src/harness/remote/link.test.ts | 56 +++++++++++++++ apps/server/src/main.ts | 2 +- docs/hosted-agent.md | 11 ++- docs/local-agent-mcp.md | 3 +- docs/planner-link.md | 17 +++-- 7 files changed, 175 insertions(+), 18 deletions(-) diff --git a/apps/server/src/chat/remote-planner.test.ts b/apps/server/src/chat/remote-planner.test.ts index f011c2fb8..c265ca51b 100644 --- a/apps/server/src/chat/remote-planner.test.ts +++ b/apps/server/src/chat/remote-planner.test.ts @@ -48,7 +48,7 @@ function card(status: "running" | "paused") { * login of their own; only the attached member may own the Planner. */ async function remoteRoom() { - let { auth, storage, channel, logins, revocations, signOut } = await linkAuth(); + let { auth, storage, channel, logins, revocations, signOut, clock } = await linkAuth(); let server = linkServer(auth); cleanups.push(server.stop); revocations.add(id => server.attachments.sessionRevoked(id)); @@ -79,7 +79,7 @@ async function remoteRoom() { config: configured(REMOTE), claimantSessionId: logins.get("rival"), plannerNotAttached: async () => - server.attachments.attached(channel.id) + await server.attachments.confirmed(channel.id) ? undefined : notAttachedMessage(url, "octo-org/score"), activeOwner: () => owners.resolve(channel.id), @@ -123,7 +123,40 @@ async function remoteRoom() { }; let owner = async () => (await storage.channels.readAgent(channel.id, new Date()))?.agent?.ownerSessionId; - return { context, server, storage, channel, logins, url, ws, send, owner, events, signOut }; + return { + context, + server, + storage, + channel, + logins, + url, + ws, + send, + owner, + events, + signOut, + clock, + }; +} + +/** + * The room with the member attached as its Planner and every browser sign-in + * expired, before the link's periodic recheck or any cleanup has noticed, + * counting what Chat saves from then on. + */ +async function expiredOwnerRoom() { + let room = await remoteRoom(); + let member = await attach(room.server.url, room.channel.id, scriptedPlanner().handlers); + cleanups.push(() => member.detach()); + expect(await room.owner()).toBe(room.logins.get("member")); + room.clock.skewMs = 31 * 24 * 60 * 60 * 1_000; + let saves = 0; + let persist = room.context.persist; + room.context.persist = async chat => { + saves++; + await persist(chat); + }; + return { ...room, member, saves: () => saves }; } test("with no attached Planner, @chopin and invoke_planner are refused with how to attach one, and nothing is posted or queued", async () => { @@ -234,6 +267,36 @@ test("an attached Planner receives @chopin and invoke_planner turns as their own ); }); +test("once the attached Planner's owner sign-in expires, @chopin is refused with how to attach one before its periodic recheck, and nothing is posted or queued", async () => { + let room = await expiredOwnerRoom(); + let { context } = room; + expect(await room.send("@chopin draft the plan")).toMatchObject({ + kind: "session:error", + code: "planner-not-attached", + message: "No Planner is attached to this document. To attach one, run pdd-plan with " + + `plan_document set to ${room.url} from a session in a checkout of octo-org/score; ` + + "that session becomes this document's Planner.", + }); + expect(context.chat.entries).toEqual([]); + expect(context.chat.waiting).toEqual([]); + expect(room.saves()).toBe(0); + expect(room.events).toEqual([]); + expect(await room.member.closed).toEqual({ code: 4401, reason: "sign-in-required" }); + expect(room.server.attachments.attached(room.channel.id)).toBe(false); + expect(await room.owner()).toBeUndefined(); +}); + +test("once the attached Planner's owner sign-in expires, invoke_planner is refused as planner-not-attached before its periodic recheck", async () => { + let room = await expiredOwnerRoom(); + let { context } = room; + expect(await Chat.invoke(context, RIVAL, "Draft the plan")).toBe("planner-not-attached"); + expect(context.chat.entries).toEqual([]); + expect(context.chat.waiting).toEqual([]); + expect(room.saves()).toBe(0); + expect(await room.member.closed).toEqual({ code: 4401, reason: "sign-in-required" }); + expect(await room.owner()).toBeUndefined(); +}); + test("once the attached Planner's owner signs out or is reset, @chopin and invoke_planner are refused again, and another member can attach", async () => { let room = await remoteRoom(); let { context } = room; diff --git a/apps/server/src/harness/remote/attachments.ts b/apps/server/src/harness/remote/attachments.ts index aeec2cd12..106d5719c 100644 --- a/apps/server/src/harness/remote/attachments.ts +++ b/apps/server/src/harness/remote/attachments.ts @@ -91,6 +91,16 @@ export class PlannerAttachments { return this.#holds.get(channelId)?.link !== undefined; } + /** + * Whether a client is attached and its ownership still stands, checked as + * the link's periodic recheck does. A stale attachment is closed and + * released here, so a Planner request never proceeds on it. + */ + async confirmed(channelId: string): Promise { + let link = this.#holds.get(channelId)?.link; + return link !== undefined && await this.verify(channelId, link); + } + /** The GitHub login holding the document's Planner, attached or within its grace period. */ holder(channelId: string): string | undefined { return this.#holds.get(channelId)?.login; @@ -186,9 +196,8 @@ export class PlannerAttachments { pending: PendingAttach, ): Promise { let { channelId, user } = request; - let login = await this.#eligibleLogin(request); + let login = await this.#eligibleLogin(request, pending); if ("ok" in login) return login; - pending.sessionIds.add(login.session.id); if (pending.invalidated) return pending.invalidated; let now = this.#auth.clock(); let held = pending.held; @@ -205,7 +214,17 @@ export class PlannerAttachments { if (!live) await this.#release(held); } if (pending.invalidated) return pending.invalidated; - let claimed = await this.#claim(channelId, login.session.id, now); + let claimed; + try { + claimed = await this.#claim(channelId, login.session.id, now); + } catch (error) { + // Storage refuses a claim for a sign-in that ended after its last check. + if (pending.invalidated) return pending.invalidated; + if (!await this.#auth.sessions.resolve(login.session.id)) { + return signInRefusal(this.#auth.config.origin, user.login); + } + throw error; + } if (!claimed) throw new Error("The document's Planner owner changed while attaching."); let hold: Hold = { channelId, @@ -225,11 +244,19 @@ export class PlannerAttachments { return this.#bind(hold, link); } - async #eligibleLogin(request: AttachRequest): Promise { + /** + * The caller's browser login, registered with `pending` before its access + * checks so that its sign-out during them refuses the attach. + */ + async #eligibleLogin( + request: AttachRequest, + pending: PendingAttach, + ): Promise { let { repository, user } = request; let fullName = `${repository.owner}/${repository.name}`; let login = await this.#auth.sessions.forUser(user.id); if (!login) return signInRefusal(this.#auth.config.origin, user.login); + pending.sessionIds.add(login.session.id); let checked; try { checked = await this.#auth.sessions.use( diff --git a/apps/server/src/harness/remote/link.test.ts b/apps/server/src/harness/remote/link.test.ts index 9255f1590..70cb7a0c6 100644 --- a/apps/server/src/harness/remote/link.test.ts +++ b/apps/server/src/harness/remote/link.test.ts @@ -479,6 +479,62 @@ describe("Planner link ownership", () => { await rivalAttaches(server.url, channel.id, storage, logins.get("rival")!); }); + /** A member's fresh attach on a raw link, paused while GitHub checks its repository access. */ + async function checkingMember() { + let linked = await linkAuth(); + let server = linkServer(linked.auth); + cleanups.push(server.stop); + linked.revocations.add(id => server.attachments.sessionRevoked(id)); + let { github } = linked.auth; + let repositoryAccess = github.repositoryAccess; + let checking = Promise.withResolvers(); + let resume = Promise.withResolvers(); + github.repositoryAccess = async (token, repositoryOwner, name) => { + github.repositoryAccess = repositoryAccess; + checking.resolve(); + await resume.promise; + return repositoryAccess(token, repositoryOwner, name); + }; + let raw = await rawLink(server.url); + raw.send({ type: "attach", version: PLANNER_LINK_VERSION, document: linked.channel.id }); + await checking.promise; + return { ...linked, server, raw, resume: () => resume.resolve() }; + } + + async function refusedSignIn( + attempt: Awaited>, + ) { + let { server, raw, channel, storage } = attempt; + expect(await raw.next("refused")).toEqual({ + type: "refused", + code: "sign-in-required", + message: "Sign in to Chopin at https://chopin.test in a browser as member, then attach " + + "again. The Planner's repository tools act with that sign-in.", + }); + expect(await raw.closed).toEqual({ code: LINK_CLOSE.unauthorized, reason: "sign-in-required" }); + expect(server.attachments.attached(channel.id)).toBe(false); + expect(server.attachments.holder(channel.id)).toBeUndefined(); + expect(server.links).toEqual([]); + expect((await owner(storage, channel.id))?.ownerSessionId).toBeUndefined(); + } + + it("refuses a fresh attach whose owner signed out while its repository access was checked", async () => { + let attempt = await checkingMember(); + expect(await attempt.signOut("member")).toBe(attempt.logins.get("member")); + attempt.resume(); + await refusedSignIn(attempt); + let { server, channel, storage, logins } = attempt; + await rivalAttaches(server.url, channel.id, storage, logins.get("rival")!); + }); + + it("refuses a fresh attach whose sign-in storage saw expire before it claimed ownership", async () => { + let attempt = await checkingMember(); + attempt.auth.clock = () => new Date(Date.now() + attempt.clock.skewMs); + attempt.clock.skewMs = 31 * 24 * 60 * 60 * 1_000; + attempt.resume(); + await refusedSignIn(attempt); + }); + it("releases the attachment when a repository writer resets the document's Planner", async () => { let { server, raw, channel, storage, logins } = await attachedMember(); let held = (await owner(storage, channel.id))!; diff --git a/apps/server/src/main.ts b/apps/server/src/main.ts index 727abcd05..e9de009c9 100644 --- a/apps/server/src/main.ts +++ b/apps/server/src/main.ts @@ -312,7 +312,7 @@ function conversation( repository, plannerNotAttached: plannerAttachments ? async () => { - if (plannerAttachments!.attached(room.id)) return undefined; + if (await plannerAttachments!.confirmed(room.id)) return undefined; let channel = await storage.channels.get(room.id); let path = channel ? await documentUrl(channel, storage.channels) diff --git a/docs/hosted-agent.md b/docs/hosted-agent.md index 088288893..4e04bc00e 100644 --- a/docs/hosted-agent.md +++ b/docs/hosted-agent.md @@ -332,14 +332,19 @@ can attach again within it and keeps the same ownership generation, and anyone else is refused. When it ends, the ownership is cleared under its generation, so a newer owner is never released by an old timer. An attach, including a grace reattach, binds only if no sign-out, expiry, or Planner reset of its ownership -landed while it was checking; otherwise it is refused like an attached link -would be closed. Attachments live in the server process; startup already clears +landed while it was checking, including the eligibility checks of the sign-in +it is about to claim with; otherwise it is refused like an attached link would +be closed. Attachments live in the server process; startup already clears every owner reference, which releases every attachment. **Unattached documents.** Members, MCP callers, and research requests never claim ownership under `HARNESS=remote`. An `@chopin` message or `invoke_planner` call for a document with no attached client, including one within its grace period, -is refused with `planner-not-attached` before anything is posted or queued. The +is refused with `planner-not-attached` before anything is posted or queued. +Before accepting either, the server rechecks an attached link as its periodic +recheck would: if the owner's browser sign-in has ended or storage no longer +assigns that owner, it closes the link, releases the ownership, and refuses the +request the same way. The refusal names the document URL and says to run `pdd-plan` with it as `plan_document` from a session in a checkout of the repository, which attaches that session as the Planner; the browser shows that text on the `@chopin` diff --git a/docs/local-agent-mcp.md b/docs/local-agent-mcp.md index 5e161a909..2352c9e4c 100644 --- a/docs/local-agent-mcp.md +++ b/docs/local-agent-mcp.md @@ -97,7 +97,8 @@ a client attached to the document over the [Planner link](planner-link.md), and only attaching makes an account the owner. With a client attached the turn goes to it and runs under its owner, whoever invoked it. Without one, including while a detached Planner's grace period runs and once the attached Planner's -owner signs out of the browser or its Planner is reset, the call is refused with +owner signs out of the browser, that sign-in expires, or its Planner is reset, +the call is refused with `planner-not-attached` and a `message`, and nothing is posted or queued: ```json diff --git a/docs/planner-link.md b/docs/planner-link.md index 131d035fd..1519f9a2e 100644 --- a/docs/planner-link.md +++ b/docs/planner-link.md @@ -144,10 +144,13 @@ including the same one, can attach again immediately: The same checks before every `start` and `turn`, and the periodic one, also find an expired sign-in or a released ownership that nothing reported; an expired sign-in is reported as `sign-in-required` even when the stored -ownership already reads as released. An `attach` that is still in progress when -one of these causes lands, including a reattach within the grace period, is -refused with the same code and close code instead of attaching. The `message` -says what happened: +ownership already reads as released. An `@chopin` message or `invoke_planner` +call makes the same check first, so a link it finds stale is closed this way and +the request is refused as [without an attached Planner](#without-an-attached-planner). +An `attach` that is still in progress when one of these causes lands, including +a reattach within the grace period, is refused with the same code and close code +instead of attaching; a fresh attach whose sign-in ends while the server checks +it is refused with `sign-in-required` (4401). The `message` says what happened: ```text The Chopin browser sign-in that made octocat this document's Planner ended, so the Planner was released. Sign in to Chopin at https://chopin.example in a browser as octocat, then attach again. @@ -171,8 +174,10 @@ This document's Planner ownership was released, so octocat is no longer attached On a server running `HARNESS=remote`, an `@chopin` message in the browser and an MCP `invoke_planner` call for a document with no attached client are refused with the code `planner-not-attached`, and nothing is posted or queued. A -document whose Planner is within its grace period counts as unattached. The -refusal's message names the document URL: +document whose Planner is within its grace period counts as unattached, and so +does one whose attached link the request finds stale: its owner's browser +sign-in has ended or the stored ownership no longer names it. The request closes +that link before it is refused. The refusal's message names the document URL: ```text No Planner is attached to this document. To attach one, run pdd-plan with plan_document set to from a session in a checkout of ; that session becomes this document's Planner.