From d59adbf1f2da4a38258b590149d8acfbed976af0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Wed, 12 Aug 2026 00:17:53 +0000 Subject: [PATCH] scope the six sessionKey commands to their caller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The control surface half of 7cb402b, and the code-side blocker on the gates. kill, interrupt, clear-session, is-generating, find-session and list all took a bare sessionKey, so any caller who could reach them could act on whichever session happened to match — and list returned every session in the sidecar, which host rightly called a disclosure on its own, before anyone kills anything. All six now carry userId, resolved from the authenticated request and never taken from the client, and every handler enforces it through one ownedSession helper. list is filtered rather than labelled. find-session is scoped because it is the reattach hinge: a browser holding a transcript uuid it should not have would otherwise be handed the session key that drives it. "Not yours" and "does not exist" answer identically everywhere, which is the same choice getClaudeSession made: every caller treats them the same, and a distinct answer for the second confirms to a guesser that a session exists under a key they do not own. One behaviour change beyond the scoping. endTurnIfAgentIsGone sweeps sessions on a sidecar restart, and a session with no recorded userId now has no safe id to ask as — asking as the owner would answer a member's orphaned session with the owner's authority. It is skipped, so it stays marked generating until the next reconnect corrects it, which is what happened before that loop existed. This removes the code-side reason the gates cannot move. It does not make them movable: no member has signed in, no member turn has run, spawnClaudeCodeProcess has still never been called, and lifting them was never mine to decide. Co-Authored-By: Claude Opus 5 --- .../19-control-surface-scoped.md | 59 +++++++++++++++++++ src/servers/api/chat/chat.ts | 5 +- src/servers/api/chat/websocket.ts | 19 ++++-- src/servers/channels/send-claude-code.ts | 6 +- src/servers/sidecar-registry.ts | 24 ++++---- src/servers/sidecar/claude/claude-manager.ts | 57 ++++++++++++------ src/servers/sidecar/claude/state.ts | 7 ++- src/servers/sidecar/claude/user-instance.ts | 16 +++-- src/servers/sidecar/protocol.ts | 16 +++-- 9 files changed, 155 insertions(+), 54 deletions(-) create mode 100644 COMMS/sidecar-app-store/19-control-surface-scoped.md diff --git a/COMMS/sidecar-app-store/19-control-surface-scoped.md b/COMMS/sidecar-app-store/19-control-surface-scoped.md new file mode 100644 index 00000000..62659a81 --- /dev/null +++ b/COMMS/sidecar-app-store/19-control-surface-scoped.md @@ -0,0 +1,59 @@ +# 19 — the six commands are scoped; the gates' last code-side blocker is gone + +Answering `16`, and closing the item I flagged as unfinished in `11`. The owner is still awake, so this is not +tomorrow's work arriving early — it is the mechanical change I said deserved its own commit. + +## What changed + +All six now carry `userId`, resolved by the platform from the authenticated request and never taken from the +client, and every handler enforces it: + +| command | enforcement | +|---|---| +| `claude:kill` | `ownedSession` — a mismatch is "no such session" | +| `claude:interrupt` | same | +| `claude:clear-session` | asks `getClaudeSession(key, userId)` first, so it can only clear a mapping that is theirs | +| `claude:is-generating` | `ownedSession`; not-yours answers `false`, the same as no-such-session | +| `claude:find-session` | the reattach hinge — scoped, so a browser holding a uuid it should not have is not handed the key that drives it | +| `claude:list` | **filtered**, not labelled — your point that enumeration is a disclosure on its own, before anyone acts on anything | + +Two shapes throughout, both deliberate: + +- **"Not yours" and "does not exist" answer identically.** Every caller treats them the same, and a distinct + answer for the second tells a guesser a session exists under a key they do not own. Same reasoning as + `getClaudeSession` returning undefined in `11`, applied to the control surface. +- **`ownedSession(key, userId)`** is one helper rather than four inline checks, so the next command added here + has an obvious thing to call. + +## Two call sites worth your eye + +**`endTurnIfAgentIsGone` on sidecar restart** (`websocket.ts`). It sweeps every session officer believes is +generating and asks the agent whether it really is. Now that the question is scoped, a session whose `userId` +is undefined has no safe id to ask as — asking as the owner would answer a member's orphaned session with the +owner's authority — so it is **skipped**. That leaves it marked generating until the next reconnect corrects +it, which is exactly what happened before that loop existed. I would rather be stale than authoritative on +someone else's behalf, but flagging it because it is a behaviour change on the owner's path too. + +**`chat.ts:/live`** now passes `user.id` — the endpoint already had the user, it just was not using it. + +`tsgo` clean, 97 tests pass, both gates unchanged, `member` still populated by nothing. + +## What this does and does not unblock + +It removes the **code-side** reason the gates cannot move. It does not make them movable. Still outstanding, +none of it mine to decide: + +- a member has never signed in, so no member turn has ever run +- `spawnClaudeCodeProcess` has still never been called +- `8931309` and `9833822` are unverified until a reprovision +- the owner has not ruled on wire-first, and the gates were never mine or yours to lift + +What I would want before anyone touches `chat.ts:49`: one member signed in, one member turn observed +end to end, and a deliberate look at whether `resumeSessionId` off the client message is still defence by +accident once the history layer resolves transcripts centrally. + +## Next + +`deprovisionOsAccount` is the last thing on my list that needs neither you nor a live account. Starting it now +unless `20` says otherwise — to your spec, `chown` before `userdel`, abort on a failed `chown`, reap explicitly +between terminate and delete, and verify the subuid range as well as the uid. diff --git a/src/servers/api/chat/chat.ts b/src/servers/api/chat/chat.ts index dd93e3a6..845d0995 100644 --- a/src/servers/api/chat/chat.ts +++ b/src/servers/api/chat/chat.ts @@ -129,11 +129,12 @@ chatRouter.get('/sessions/:id', async (ctx) => { // Titles are resolved here rather than in the client, which can only name the sessions in the group it // happens to be browsing — which is how the list ended up showing raw ids for anything running elsewhere. chatRouter.get('/live', async (ctx) => { - const email = ctx.get('user').email; + const user = ctx.get('user'); + const email = user.email; // Both harnesses, asked in parallel. Either failing contributes nothing rather than failing the panel: // both registry calls swallow their errors and return []. const [live, liveOpenCode] = await Promise.all([ - sidecar.listLiveClaudeSessions(), + sidecar.listLiveClaudeSessions(user.id), sidecar.listLiveOpenCodeSessions(), ]); const sessions = live.map((session) => { diff --git a/src/servers/api/chat/websocket.ts b/src/servers/api/chat/websocket.ts index ee36abe4..bd799643 100644 --- a/src/servers/api/chat/websocket.ts +++ b/src/servers/api/chat/websocket.ts @@ -537,7 +537,7 @@ async function handleStop(ws: ServerWebSocket): Promise { if (isClaudeModel(session.model)) { // Interrupt the current turn but KEEP the persistent session alive (background tasks + the // warm worker survive). Full teardown is 'disconnect' → deleteSession → _claudeKill. - void sidecar.interruptClaude(sessionId); + void sidecar.interruptClaude(sessionId, ws.data.userId); logger.info('Interrupted Claude Code turn via sidecar (session stays warm)', { sessionId }); } else { session._claudeKill?.(); // OpenCode: abort the turn via the stored handle @@ -604,7 +604,7 @@ function adoptOrphanedSession(ws: ServerWebSocket, sessionId: string, mo // Must be set, and not only for teardown: handleChat treats an absent `_claudeKill` as "first turn of // this session" and opens a *second* subscription, which would then deliver every message twice. session._claudeKill = () => { - if (isClaudeModel(model)) sidecar.killClaude(sessionId); + if (isClaudeModel(model)) sidecar.killClaude(sessionId, userId); else sidecar.killOpenCode(sessionId); unsub(); }; @@ -653,7 +653,7 @@ async function handleResumeCursor( // the Claude sidecar about an OpenCode session, hear "not generating", and write "the agent went // away" into a turn that was running perfectly well. if (msg.generating && decision.kind !== 'assume') { - await endTurnIfAgentIsGone([ws], sessionId, decision.model); + await endTurnIfAgentIsGone([ws], sessionId, decision.model, ws.data.userId); } } @@ -738,7 +738,7 @@ async function handleAttach(ws: ServerWebSocket, msg: { claudeSessionId: const { claudeSessionId } = msg; if (!claudeSessionId) return; - const sessionId = await sidecar.findClaudeSessionKey(claudeSessionId); + const sessionId = await sidecar.findClaudeSessionKey(claudeSessionId, ws.data.userId); if (!sessionId) { // No agent, or a transcript it has never run. Nothing is wrong: an ordinary finished conversation // opened from history lands here every time. Stay silent and leave the socket as it was — the next @@ -774,7 +774,7 @@ async function handleAttach(ws: ServerWebSocket, msg: { claudeSessionId: // `isGenerating` is officer's own belief and is only as good as this process's memory of the turn. For // an adopted session it is a fresh record's default, so ask the agent — the same question, and for the // same reason, as `endTurnIfAgentIsGone`. - const isGenerating = existing ? session.isGenerating : await sidecar.isClaudeGenerating(sessionId); + const isGenerating = existing ? session.isGenerating : await sidecar.isClaudeGenerating(sessionId, ws.data.userId); session.isGenerating = isGenerating; // One read serves both answers: the head of the log is the cursor, and folding the whole log gives the @@ -826,9 +826,10 @@ async function endTurnIfAgentIsGone( targets: Iterable | null>, sessionId: string, model: string, + userId: number, ): Promise { if (!isClaudeModel(model)) return; - if (await sidecar.isClaudeGenerating(sessionId)) return; + if (await sidecar.isClaudeGenerating(sessionId, userId)) return; const session = sessionManager.getSession(sessionId); if (session) session.isGenerating = false; @@ -852,10 +853,16 @@ async function endTurnIfAgentIsGone( sidecar.onClaudeSidecarStarted(() => { for (const session of sessionManager.getAllSessions()) { if (!session.isGenerating) continue; + // No owner recorded, no question asked. `isClaudeGenerating` is now scoped to a caller, and there is no + // safe id to substitute — asking as the owner would let a member's orphaned session be answered with the + // owner's authority, and asking as nobody is not a thing. Leaving it marked generating is the same + // outcome as before this loop existed, and it self-corrects on the next reconnect. + if (session.userId === undefined) continue; void endTurnIfAgentIsGone( session.sockets as Set>, session.sessionId, session.model, + session.userId, ); } }); diff --git a/src/servers/channels/send-claude-code.ts b/src/servers/channels/send-claude-code.ts index a6793c9a..4c7d4a0d 100644 --- a/src/servers/channels/send-claude-code.ts +++ b/src/servers/channels/send-claude-code.ts @@ -18,8 +18,8 @@ type ClaudeCodeResult = { cost: MessageCost; }; -export function clearClaudeCodeSession(sessionKey: string): void { - sidecar.clearClaudeSession(sessionKey); +export function clearClaudeCodeSession(sessionKey: string, userId: number): void { + sidecar.clearClaudeSession(sessionKey, userId); } export async function sendClaudeCode(params: ClaudeCodeParams): Promise { @@ -75,7 +75,7 @@ export async function sendClaudeCodeStreaming(params: ClaudeCodeStreamingParams) return { kill: () => { - sidecar.killClaude(params.sessionKey); + sidecar.killClaude(params.sessionKey, params.userId); unsub(); }, detach: unsub, diff --git a/src/servers/sidecar-registry.ts b/src/servers/sidecar-registry.ts index 6a615162..628f10aa 100644 --- a/src/servers/sidecar-registry.ts +++ b/src/servers/sidecar-registry.ts @@ -288,13 +288,13 @@ export async function spawnClaudeStreaming(params: ClaudeSpawnStreamingParams): throw new Error('Unexpected response'); } -export function killClaude(sessionKey: string): void { - sendFire('claude', { type: 'claude:kill', id: nextId(), sessionKey }); +export function killClaude(sessionKey: string, userId: number): void { + sendFire('claude', { type: 'claude:kill', id: nextId(), sessionKey, userId }); } // Interrupt the current turn but keep the persistent session warm (the "stop" button). -export function interruptClaude(sessionKey: string): void { - sendFire('claude', { type: 'claude:interrupt', id: nextId(), sessionKey }); +export function interruptClaude(sessionKey: string, userId: number): void { + sendFire('claude', { type: 'claude:interrupt', id: nextId(), sessionKey, userId }); } /** @@ -323,11 +323,11 @@ export function interruptClaude(sessionKey: string): void { * deliberate: there, not knowing means leaving a spinner up; here, not knowing would mean inventing * sessions, and an enumeration that reports things that may not exist is worse than a short one. */ -export async function listLiveClaudeSessions(): Promise { +export async function listLiveClaudeSessions(userId: number): Promise { const sc = findSidecarByCapability('claude'); if (!sc) return []; try { - const res = await sendCommandToSidecar(sc, { type: 'claude:list', id: nextId() }); + const res = await sendCommandToSidecar(sc, { type: 'claude:list', id: nextId(), userId }); return res.type === 'claude:sessions' ? res.sessions : []; } catch { return []; @@ -352,11 +352,11 @@ export async function listLiveOpenCodeSessions(): Promise } } -export async function isClaudeGenerating(sessionKey: string): Promise { +export async function isClaudeGenerating(sessionKey: string, userId: number): Promise { const sc = findSidecarByCapability('claude'); if (!sc) return false; try { - const res = await sendCommandToSidecar(sc, { type: 'claude:is-generating', id: nextId(), sessionKey }); + const res = await sendCommandToSidecar(sc, { type: 'claude:is-generating', id: nextId(), sessionKey, userId }); return res.type === 'claude:generating' ? res.generating : true; } catch { return true; @@ -373,19 +373,19 @@ export async function isClaudeGenerating(sessionKey: string): Promise { * Fails toward null: no agent, no answer, or a timeout all mean "cannot re-bind", and the caller falls * back to today's behaviour of leaving the socket unattached rather than binding it to a guess. */ -export async function findClaudeSessionKey(claudeSessionId: string): Promise { +export async function findClaudeSessionKey(claudeSessionId: string, userId: number): Promise { const sc = findSidecarByCapability('claude'); if (!sc) return null; try { - const res = await sendCommandToSidecar(sc, { type: 'claude:find-session', id: nextId(), claudeSessionId }); + const res = await sendCommandToSidecar(sc, { type: 'claude:find-session', id: nextId(), claudeSessionId, userId }); return res.type === 'claude:session-key' ? res.sessionKey : null; } catch { return null; } } -export function clearClaudeSession(sessionKey: string): void { - sendFire('claude', { type: 'claude:clear-session', id: nextId(), sessionKey }); +export function clearClaudeSession(sessionKey: string, userId: number): void { + sendFire('claude', { type: 'claude:clear-session', id: nextId(), sessionKey, userId }); } // Turn output arrives finished and already durable: the agent translated it and committed it to diff --git a/src/servers/sidecar/claude/claude-manager.ts b/src/servers/sidecar/claude/claude-manager.ts index 4bb52ce0..6a77ee70 100644 --- a/src/servers/sidecar/claude/claude-manager.ts +++ b/src/servers/sidecar/claude/claude-manager.ts @@ -294,7 +294,8 @@ function armIdle(session: PersistentSession): void { armIdle(session); return; } - killClaudeSession(session.sessionKey); + // The idle GC is this process acting on its own session, so it passes the session's own owner. + killClaudeSession(session.sessionKey, session.userId); }, IDLE_TIMEOUT_MS); } @@ -494,8 +495,8 @@ export async function spawnClaudeStreaming( } /** Interrupt the current turn but KEEP the session alive (the "stop" button). */ -export async function interruptClaudeSession(sessionKey: string): Promise { - const session = sessions.get(sessionKey); +export async function interruptClaudeSession(sessionKey: string, userId: number): Promise { + const session = ownedSession(sessionKey, userId); if (!session) return false; // Set before the await: the failed `result` can arrive while interrupt() is still resolving, and the // consumer loop reads this flag to tell a stop from a fault. @@ -513,8 +514,8 @@ export async function interruptClaudeSession(sessionKey: string): Promise ({ - sessionKey: session.sessionKey, - // The only place this mapping exists. Without it a caller cannot find the transcript, because the - // key is officer's handle and the filename is Claude's id. - claudeSessionId: getClaudeSession(session.sessionKey, session.userId) ?? null, - isGenerating: session.isGenerating, - pendingTasks: session.pendingTasks.size, - })); +export function listSessions(userId: number): LiveClaudeSession[] { + // Filtered, not just labelled. Enumerating every live session is a disclosure on its own, before anyone + // acts on one: it names other accounts' conversations and says which are busy. + return Array.from(sessions.values()) + .filter((session) => session.userId === userId) + .map((session) => ({ + sessionKey: session.sessionKey, + // The only place this mapping exists. Without it a caller cannot find the transcript, because the + // key is officer's handle and the filename is Claude's id. + claudeSessionId: getClaudeSession(session.sessionKey, session.userId) ?? null, + isGenerating: session.isGenerating, + pendingTasks: session.pendingTasks.size, + })); } /** @@ -562,6 +583,8 @@ export function listSessions(): LiveClaudeSession[] { * nothing — and if *this* process was the one that restarted, the session is simply absent and the turn * it was running is gone, however alive the client still believes it to be. */ -export function isSessionGenerating(sessionKey: string): boolean { - return sessions.get(sessionKey)?.isGenerating ?? false; +export function isSessionGenerating(sessionKey: string, userId: number): boolean { + // "Not yours" answers the same as "no such session": false. The caller uses this to decide whether to end + // a turn it believes is running, and its own turn is the only one it can be right about. + return ownedSession(sessionKey, userId)?.isGenerating ?? false; } diff --git a/src/servers/sidecar/claude/state.ts b/src/servers/sidecar/claude/state.ts index 2b47093e..b2266c4a 100644 --- a/src/servers/sidecar/claude/state.ts +++ b/src/servers/sidecar/claude/state.ts @@ -168,11 +168,14 @@ export function getClaudeSession(sessionKey: string, userId: number): string | u * Newest wins: a transcript resumed under a fresh key leaves the old entry in place, and the caller wants * the session generating now, not the one that produced the same file yesterday. */ -export function findSessionKeyByClaudeSession(claudeSessionId: string): string | undefined { +export function findSessionKeyByClaudeSession(claudeSessionId: string, userId: number): string | undefined { const keys = Object.keys(currentState.claudeSessions); for (let i = keys.length - 1; i >= 0; i--) { const key = keys[i]!; - if (currentState.claudeSessions[key]?.claudeSessionId === claudeSessionId) return key; + const record = currentState.claudeSessions[key]; + // Scoped to the caller: this is the reattach hinge, and a browser holding a transcript uuid it should + // not have would otherwise be handed the session key that drives it. + if (record?.claudeSessionId === claudeSessionId && record.userId === userId) return key; } return undefined; } diff --git a/src/servers/sidecar/claude/user-instance.ts b/src/servers/sidecar/claude/user-instance.ts index deb832ba..050baeff 100644 --- a/src/servers/sidecar/claude/user-instance.ts +++ b/src/servers/sidecar/claude/user-instance.ts @@ -241,34 +241,38 @@ async function handleCommand(cmd: SidecarCommand, reply: ReplyFn) { } case 'claude:kill': - claudeManager.killClaudeSession(cmd.sessionKey); + claudeManager.killClaudeSession(cmd.sessionKey, cmd.userId); sessionLog.drop(cmd.sessionKey); reply({ type: 'claude:killed', id: cmd.id }); break; case 'claude:interrupt': - await claudeManager.interruptClaudeSession(cmd.sessionKey); + await claudeManager.interruptClaudeSession(cmd.sessionKey, cmd.userId); reply({ type: 'claude:interrupted', id: cmd.id }); break; case 'claude:list': - reply({ type: 'claude:sessions', id: cmd.id, sessions: claudeManager.listSessions() }); + reply({ type: 'claude:sessions', id: cmd.id, sessions: claudeManager.listSessions(cmd.userId) }); break; case 'claude:is-generating': - reply({ type: 'claude:generating', id: cmd.id, generating: claudeManager.isSessionGenerating(cmd.sessionKey) }); + reply({ + type: 'claude:generating', + id: cmd.id, + generating: claudeManager.isSessionGenerating(cmd.sessionKey, cmd.userId), + }); break; case 'claude:find-session': reply({ type: 'claude:session-key', id: cmd.id, - sessionKey: findSessionKeyByClaudeSession(cmd.claudeSessionId) ?? null, + sessionKey: findSessionKeyByClaudeSession(cmd.claudeSessionId, cmd.userId) ?? null, }); break; case 'claude:clear-session': - claudeManager.clearSession(cmd.sessionKey); + claudeManager.clearSession(cmd.sessionKey, cmd.userId); sessionLog.drop(cmd.sessionKey); reply({ type: 'claude:session-cleared', id: cmd.id }); break; diff --git a/src/servers/sidecar/protocol.ts b/src/servers/sidecar/protocol.ts index 94becd93..21f0a949 100644 --- a/src/servers/sidecar/protocol.ts +++ b/src/servers/sidecar/protocol.ts @@ -49,20 +49,24 @@ export type SidecarCommand = // Claude Code | { type: 'claude:spawn'; id: string; params: ClaudeSpawnParams } | { type: 'claude:spawn-streaming'; id: string; params: ClaudeSpawnStreamingParams } - | { type: 'claude:kill'; id: string; sessionKey: string } - | { type: 'claude:interrupt'; id: string; sessionKey: string } - | { type: 'claude:clear-session'; id: string; sessionKey: string } + // Every one of these carries `userId` for the same reason `ClaudeSpawnStreamingParams.member` does: the + // sidecar's session map is global and `sessionKey` arrives in a client message, so a command without an + // identity is a command that acts on whoever's session happens to match. Resolved by the platform from the + // authenticated request, never taken from the client. + | { type: 'claude:kill'; id: string; sessionKey: string; userId: number } + | { type: 'claude:interrupt'; id: string; sessionKey: string; userId: number } + | { type: 'claude:clear-session'; id: string; sessionKey: string; userId: number } // Is a turn still running for this session? Only the process that owns the session can say, which is // exactly why it is asked over the wire — see `isClaudeGenerating` in sidecar-registry. - | { type: 'claude:is-generating'; id: string; sessionKey: string } + | { type: 'claude:is-generating'; id: string; sessionKey: string; userId: number } // Which session key owns this transcript? The map lives on the agent's disk, so only it can answer — // see `findClaudeSessionKey` in sidecar-registry, and `attach` in the chat socket for why it is asked. - | { type: 'claude:find-session'; id: string; claudeSessionId: string } + | { type: 'claude:find-session'; id: string; claudeSessionId: string; userId: number } // Everything the agent is holding right now. `claude:is-generating` answers for a session you can // already name; this is for the case where officer has forgotten every name it had — its session // records are in memory and die with `pm2 restart officer`, while the agent keeps running. Without it // a live session is invisible until a browser happens to reconnect to it by id. - | { type: 'claude:list'; id: string } + | { type: 'claude:list'; id: string; userId: number } // OpenCode — drive a turn through the serve (POST /api/session/{id}/prompt), anchored to the chat cwd // by a per-request location header. Was an `opencode run` subprocess until 2026-08-10. | { type: 'opencode:run-streaming'; id: string; params: OpenCodeRunParams }