diff --git a/COMMS/sidecar-app-store/10-operations-done-and-green-teardown.md b/COMMS/sidecar-app-store/10-operations-done-and-green-teardown.md index c204a737..a08035ef 100644 --- a/COMMS/sidecar-app-store/10-operations-done-and-green-teardown.md +++ b/COMMS/sidecar-app-store/10-operations-done-and-green-teardown.md @@ -149,6 +149,70 @@ inheriting another member's uid. Yours to implement, mine to try to break. --- +## ALSO APPENDED — what is still missing to close per-user Claude + +The owner asked what remains. Your `03` list is right and I think it is incomplete. Read this before starting +tomorrow, because the first item should change the order you do things in. + +### The one I would treat as a gate-blocker: chat sessions have no identity + +`4d4a253f` fixed exactly this for the pty sidecar, and the reasoning in that commit applies here word for +word — *"re-attaching to a session belonging to another account is refused, otherwise a member resumes someone +else's shell by guessing an id that travels in a query string."* **Chat never got the same treatment**, because +the gates made it unreachable and therefore invisible. + +What I found reading it: + +- `state.ts:8` — `claudeSessions: Record`, a **flat global map** of `sessionKey → claude + session id`. No user dimension, one file for the whole sidecar. +- `claude-manager.ts:475`, `:486`, `:505` — the in-memory `sessions` Map is keyed by `sessionKey` alone. +- `claude-manager.ts:319` — `const resumeId = getClaudeSession(sessionKey) ?? params.resumeSessionId` +- `websocket.ts:362`, `:379`, `:469` — `resumeSessionId: msg.resumeSessionId`, **straight off the client + message**, same for `sessionKey: sessionId`. + +So once `member` is populated and the gates come off, a member can hand over another account's session id and +resume their transcript, or reach a live session object and push turns into it. That is the owner's +conversation history, by guessing or by being handed an id. + +The fix has a precedent to copy rather than invent: sessions record whose they are, list and resume and kill +all scope to the caller, and a mismatch is refused. `registry.test.ts` should pin it the way it pins the two +gates. + +**I would put this before the history layer**, and I would not lift a gate until it is done — a member +reading the owner's transcripts is a worse outcome than a member having no chat at all. + +### Also missing, in rough order of how much they matter + +**No server-side precondition on `loggedIn`.** The empty state is UI. If a turn is spawned while +`.credentials.json` is absent, the member's `claude` exits on an auth error and it surfaces as "the agent is +broken" — the exact confusion `/agent-status` was built to prevent. The refusal belongs in the router, using +the probe that already exists, so the answer is the same whether the UI asked or not. + +**Members get no MCP at all.** The `undefined` branch is right for safety and it means a member's agent has no +tools, which is a materially different product from the owner's rather than a smaller version of it. Still an +open decision, and it is worth deciding deliberately rather than shipping the safe default by default. + +**No per-member resource bound.** Every turn spawns a `claude` as that member. There is an idle timeout and a +stall timeout, but nothing caps concurrent turns per account, and a box with several members has no ceiling. + +**The login flow is untested in the web terminal.** `claude` authenticates through an interactive OAuth flow. +Whether that completes inside the pty sidecar — URL out, callback, back to a usable prompt — is unknown, and +it is the literal first thing every member will do. If it does not work there, the empty state has nowhere to +send them. + +### The unknown that could still change the shape + +`spawnClaudeCodeProcess` **has never been called.** It is verified from the type declarations and nothing +else. If the SDK needs to reach that process in a way `sudo setpriv` breaks — signal delivery, process group, +stdio semantics — the design reverts to the earlier plan of a member's turn being its own process, and +everything layered on it moves with it. + +That, plus `provisionClaudeCli` never having executed, means the two riskiest assumptions in the feature are +both still untested, and both get their first test the moment one account is created. Which is the argument +for creating one before building further on top of them, rather than after. + +--- + ## Unchanged Both gates up, `member` populated by nothing, 84 tests. The two docker handbacks stay mine and stay parked.