From bc4dbc51ff3f9e42398206257ae002a020311c7d Mon Sep 17 00:00:00 2001 From: Mathew Goldsborough <1759329+mgoldsborough@users.noreply.github.com> Date: Thu, 21 May 2026 14:43:30 -1000 Subject: [PATCH 1/3] feat(workspace): enforce personal-workspace immutability invariants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Personal workspaces (`isPersonal === true`) are now sole-owner-by-design. The store throws `PersonalWorkspaceInvariantError` on attempts to mutate the locked fields and the HTTP layer maps it to a structured 422 — the silent-strip behavior that allowed multi-admin personal workspaces on hq production is gone. Invariants enforced by `WorkspaceStore`: 1. members locked to `[{ userId: ownerUserId, role: "admin" }]` — `addMember` / `removeMember` / `updateMemberRole` and any `update({ members })` patch reject mutations on personal workspaces. 2. `isPersonal` frozen post-create (both directions). 3. `ownerUserId` frozen on personal workspaces. 4. `ownerUserId` forbidden on non-personal workspaces. `bundles`, `name`, `about`, `customInstructions` remain freely mutable. The HTTP mapping mirrors the `ConversationCorruptedError → 422` precedent: `PersonalWorkspaceInvariantError` becomes `422 personal_workspace_invariant` with `{ workspaceId, reason }`. Because the typed error class doesn't survive the in-process MCP serialization boundary, the workspace-mgmt tool handlers encode it into `structuredContent` so `handleToolCall` can re-detect and emit the 422. Pre-Stage-1.1 data is repaired by `scripts/cleanup-personal-workspace-members.ts` (also `bun run cleanup:personal-workspace-members` and a `make` target). Idempotent, dry-run by default; `--apply` writes. A personal workspace missing `ownerUserId` is a hard-error — operator must triage. --- AGENTS.md | 15 + CHANGELOG.md | 3 + package.json | 1 + scripts/cleanup-personal-workspace-members.ts | 256 +++++++++++++++++ scripts/migrate-personal-workspaces.ts | 8 +- src/api/handlers.ts | 70 +++++ src/tools/workspace-mgmt-tools.ts | 41 +++ src/workspace/errors.ts | 69 +++++ src/workspace/provisioning.ts | 107 +++---- src/workspace/workspace-store.ts | 154 +++++++++- .../api/personal-workspace-invariants.test.ts | 172 +++++++++++ ...cleanup-personal-workspace-members.test.ts | 208 ++++++++++++++ .../personal-workspace-invariants.test.ts | 272 ++++++++++++++++++ test/unit/workspace/provisioning.test.ts | 23 +- test/unit/workspace/workspace-store.test.ts | 24 +- 15 files changed, 1323 insertions(+), 100 deletions(-) create mode 100644 scripts/cleanup-personal-workspace-members.ts create mode 100644 src/workspace/errors.ts create mode 100644 test/integration/api/personal-workspace-invariants.test.ts create mode 100644 test/integration/scripts/cleanup-personal-workspace-members.test.ts create mode 100644 test/unit/workspace/personal-workspace-invariants.test.ts diff --git a/AGENTS.md b/AGENTS.md index 449c6a13..66f95bf9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -137,6 +137,21 @@ When adding a new code path that touches workspace-scoped credentials or identit **Conversations are user-scoped, not workspace-scoped.** Post-Stage-1, every conversation lives at `{workDir}/conversations/{convId}.jsonl` and is authorized by ownership (`Conversation.ownerId === access.userId`). Look up via `runtime.findConversation(convId, { userId })`; write via `runtime.findConversationStore()`. `workspaceId` on conversation metadata is a tool-scoping breadcrumb — it tells the runtime which workspace's tools the chat had access to when a turn ran, NOT where the file lives. Hand-building per-workspace conversation paths (`join(workDir, "workspaces", wsId, "conversations", ...)`) is a regression caught by `check:conversation-paths`. **Personal workspace ids** go through `personalWorkspaceIdFor(userId)` from `src/workspace/workspace-store.ts` — no hand-built `"ws_user_" + userId` or `` `ws_user_${userId}` `` outside that helper (`check:personal-workspace-id` enforces). +### Personal workspace invariants + +Personal workspaces (`isPersonal === true`) are sole-owner-by-design. The store enforces four rules and throws `PersonalWorkspaceInvariantError` (`src/workspace/errors.ts`) on violation: + +1. **Members locked** to `[{ userId: ownerUserId, role: "admin" }]`. `addMember` / `removeMember` / `updateMemberRole` and `update({ members })` all reject mutations on personal workspaces. +2. **`isPersonal` frozen** post-create (both directions). +3. **`ownerUserId` frozen** on personal workspaces. +4. **`ownerUserId` forbidden** on non-personal workspaces (the two fields travel together). + +What stays freely mutable on a personal workspace: `bundles`, `name`, `about`, `customInstructions`. Those are workspace-content edits, not identity edits. + +The HTTP layer maps `PersonalWorkspaceInvariantError` to `422 personal_workspace_invariant` with `{ workspaceId, reason }` details (same shape as `ConversationCorruptedError → 422`). The workspace-mgmt tool handlers encode the error into `structuredContent` so it survives the in-process MCP serialization boundary; `handleToolCall` decodes and emits the 422. + +Pre-Stage-1.1 data (multi-admin personal workspaces seen on hq production) is repaired retroactively by `scripts/cleanup-personal-workspace-members.ts` (alias: `bun run cleanup:personal-workspace-members`, or `make cleanup-personal-workspace-members CLIENT= ENV=` for in-cluster runs). Idempotent; dry-run by default, `--apply` to write. A personal workspace missing `ownerUserId` is a hard-error — operator must triage. + ## Debug Logging Hot-path diagnostics are gated behind namespace flags so they're available when you need them without editing source. Use for tracing across the runtime ↔ SSE ↔ browser ↔ iframe chain. diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f08e227..b5f29129 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,8 @@ - Top-level `/profile` route. Identity isn't a setting; un-nested from `/settings/*`. - Org-admin gate on `set_model_config` — backend now refuses non-org-admin writes (was UI-only via RouteGuard). Distinguishes "no identity" (cron, automations) from "wrong role" so debug logs make non-user code paths obvious. - HTTP proxy primitive (`_meta["ai.nimblebrain/http-proxy"]`). Bundles can expose a loopback HTTP server (e.g. `astro preview`, Jupyter kernel) through the platform at `/v1/ws//apps///*`. Loopback-only target, credentials and `Accept-Encoding` stripped on forward, `Set-Cookie`/CSP/X-Frame-Options stripped on response, per-workspace kill switch via `Workspace.allowHttpProxy`. Bundles get `NB_WORKSPACE_ID`, `NB_PROXY_PREFIX`, `NB_PUBLIC_ORIGIN` in their env at spawn ([docs](https://docs.nimblebrain.ai/apps/http-proxy/)). +- `PersonalWorkspaceInvariantError` typed error (`src/workspace/errors.ts`) → HTTP 422 `personal_workspace_invariant` with structured `{ workspaceId, reason }` body. Mirrors the `ConversationCorruptedError` → 422 precedent; raised by `WorkspaceStore` on attempts to mutate the locked members / `isPersonal` / `ownerUserId` fields on personal workspaces. +- `scripts/cleanup-personal-workspace-members.ts` (also `bun run cleanup:personal-workspace-members` and `make cleanup-personal-workspace-members`) — one-off retroactive cleanup that converges pre-Stage-1.1 personal workspaces to the sole-owner-admin shape. Idempotent, dry-run by default. ### Changed @@ -85,6 +87,7 @@ - **Removed `manage_conversation` actions:** `shareConversation`, `unshareConversation`, `addParticipant`, `removeParticipant` are gone. Single-owner semantics; sharing returns in a future stage with policy gates. External callers that previously invoked these actions get an `unknown action` error. - **`Conversation.visibility` and `Conversation.participants` removed from the schema.** Reads of pre-migration files that still carry these fields skip them at parse time; writes never produce them. `ownerId` is now required on every conversation file — pre-migration files without one fail to load with a clear "run the migration" hint. - **`/v1/conversations/:id/events` no longer requires `X-Workspace-Id`.** The header is still honored when sent (validated for format + membership); absent → 200, present + malformed → 400, present + non-member → 403. Foreign-owner conversation → 403 `conversation_access_denied`; non-existent → 404 `not_found`. Web clients that already send the header keep working unchanged. +- **Personal workspaces enforce sole-owner-admin membership and freeze `isPersonal` / `ownerUserId` post-create.** `WorkspaceStore.update` / `addMember` / `removeMember` / `updateMemberRole` throw `PersonalWorkspaceInvariantError` on any member or identity-field mutation against a personal workspace; `WorkspaceStore.create` rejects a personal workspace whose initial members aren't exactly `[{ userId: ownerUserId, role: "admin" }]`. Operators with pre-existing multi-admin personal workspaces (observed on hq production) must run `bun run cleanup:personal-workspace-members --apply` to converge — `bundles`, `name`, `about`, `customInstructions` remain freely mutable. ### Fixed diff --git a/package.json b/package.json index e3eeccb6..514b316a 100644 --- a/package.json +++ b/package.json @@ -57,6 +57,7 @@ "smoke": "bun test test/smoke/", "migrate:personal-workspaces": "bun run scripts/migrate-personal-workspaces.ts", "migrate:conversations-to-top-level": "bun run scripts/migrate-conversations-to-top-level.ts", + "cleanup:personal-workspace-members": "bun run scripts/cleanup-personal-workspace-members.ts", "check:cycles": "bun run scripts/check-cycles.ts", "check:bundle-transport": "bun run scripts/check-bundle-transport.ts", "check:workspace-paths": "bun run scripts/check-workspace-paths.ts", diff --git a/scripts/cleanup-personal-workspace-members.ts b/scripts/cleanup-personal-workspace-members.ts new file mode 100644 index 00000000..954b85bf --- /dev/null +++ b/scripts/cleanup-personal-workspace-members.ts @@ -0,0 +1,256 @@ +#!/usr/bin/env bun +/** + * Stage 1.1 cleanup: enforce sole-owner-admin membership on existing + * personal workspaces. + * + * Stage 1 introduced personal workspaces (`isPersonal: true`, + * `ownerUserId: `) but didn't enforce that their `members` array + * matched the canonical sole-owner shape. The hq production tenant + * surfaced a personal workspace with three admins, which Stage 1.1 + * now disallows at the store layer. Operators with pre-existing data + * shaped by the looser invariant run this script once to converge. + * + * For each workspace where `isPersonal === true`: + * - If `members` is already `[{ userId: ownerUserId, role: "admin" }]` + * → no-op (idempotent — running twice produces no changes). + * - If `members` contains non-owner entries OR the owner's role is + * not "admin" → rewrite to the canonical shape: + * `[{ userId: ownerUserId, role: "admin" }]`. Any non-owner + * members are dropped from the personal workspace; they retain + * their own personal workspaces and any shared-workspace + * memberships elsewhere. + * - If `ownerUserId` is missing on a personal workspace → hard-error. + * We can't safely guess the owner; an operator must repair + * manually (or delete the workspace if it's an orphan). + * + * Non-personal workspaces are untouched. + * + * Usage: + * bun run scripts/cleanup-personal-workspace-members.ts [--work-dir ] [--dry-run | --apply] + * + * Default is dry-run. Use `--apply` to actually write. + */ + +import { existsSync } from "node:fs"; +import { homedir } from "node:os"; +import { join } from "node:path"; +import { writeJsonAtomic } from "../src/util/atomic-json.ts"; +import type { Workspace } from "../src/workspace/types.ts"; +import { WorkspaceStore } from "../src/workspace/workspace-store.ts"; +import { acquireMigrationLock } from "./lib/migration-lock.ts"; + +interface Args { + workDir: string; + apply: boolean; +} + +interface Stats { + workspacesScanned: number; + personalScanned: number; + alreadyClean: number; + cleaned: number; + nonPersonalSkipped: number; + errors: { ctx: string; message: string }[]; +} + +function parseArgs(): Args { + const argv = process.argv.slice(2); + let workDir = process.env.NB_WORK_DIR ?? join(homedir(), ".nimblebrain"); + // Default to dry-run — operator opts into writes via --apply. This + // mirrors the Stage 1 migration ergonomics; the cleanup is + // destructive (drops non-owner members) so the bias is toward + // visibility-first. + let apply = false; + + for (let i = 0; i < argv.length; i++) { + const arg = argv[i]; + if (arg === "--help" || arg === "-h") { + printHelp(); + process.exit(0); + } else if (arg === "--dry-run") { + apply = false; + } else if (arg === "--apply") { + apply = true; + } else if (arg === "--work-dir") { + workDir = argv[++i] ?? ""; + } else if (arg?.startsWith("--work-dir=")) { + workDir = arg.slice("--work-dir=".length); + } else { + console.error(`[cleanup] unknown argument: ${arg}`); + printHelp(); + process.exit(2); + } + } + + if (!workDir) { + console.error("[cleanup] --work-dir is required (or set NB_WORK_DIR)"); + process.exit(2); + } + return { workDir, apply }; +} + +function printHelp(): void { + console.log(` +cleanup-personal-workspace-members — Stage 1.1 follow-up + +Enforces sole-owner-admin membership on every personal workspace. +Non-owner members on personal workspaces are dropped; the owner's +role is forced to "admin". + +Usage: + bun run scripts/cleanup-personal-workspace-members.ts [options] + +Options: + --work-dir Override the work directory. + Defaults to $NB_WORK_DIR or ~/.nimblebrain. + --dry-run Report planned changes without writing (default). + --apply Actually write changes. + -h, --help This message. + +Idempotent: running twice produces no changes the second time. +Run with --dry-run first to verify the plan. +`); +} + +/** + * Compute the canonical sole-owner-admin members array for a personal + * workspace. Single source of truth — both the planner and the writer + * use it so the dry-run reflects the same end state as `--apply`. + */ +function canonicalMembers(ownerUserId: string): Workspace["members"] { + return [{ userId: ownerUserId, role: "admin" }]; +} + +/** + * Decide whether `ws` already matches the canonical sole-owner-admin + * shape. A personal workspace passes iff: + * - exactly one member, + * - that member's `userId` is the workspace's `ownerUserId`, + * - that member's `role` is "admin". + */ +function isCanonical(ws: Workspace): boolean { + if (!ws.ownerUserId) return false; + if (ws.members.length !== 1) return false; + const sole = ws.members[0]; + if (!sole) return false; + return sole.userId === ws.ownerUserId && sole.role === "admin"; +} + +async function main(): Promise { + const args = parseArgs(); + console.error( + `[cleanup] workDir=${args.workDir}${args.apply ? " (apply)" : " (dry-run)"}`, + ); + + const workspacesDir = join(args.workDir, "workspaces"); + if (!existsSync(workspacesDir)) { + console.error(`[cleanup] no workspaces dir at ${workspacesDir} — nothing to do`); + return; + } + + // Hold the same lock the Stage 1 migrations use. Even a dry-run + // reading state concurrently with a real migration could surface + // inconsistent results. + acquireMigrationLock(args.workDir, "cleanup-personal-workspace-members"); + + const wsStore = new WorkspaceStore(args.workDir); + const stats: Stats = { + workspacesScanned: 0, + personalScanned: 0, + alreadyClean: 0, + cleaned: 0, + nonPersonalSkipped: 0, + errors: [], + }; + + const workspaces = await wsStore.list(); + for (const ws of workspaces) { + stats.workspacesScanned++; + + if (ws.isPersonal !== true) { + stats.nonPersonalSkipped++; + continue; + } + + stats.personalScanned++; + + if (!ws.ownerUserId) { + // Hard-error rather than guess. A personal workspace with no + // owner is data corruption; running the Stage 1 migration + // (`migrate:personal-workspaces`) should have stamped one, or + // the workspace is an orphan that needs operator triage. + const message = `personal workspace ${ws.id} has isPersonal:true but no ownerUserId — operator action required (stamp manually or delete)`; + console.error(`[cleanup] ERROR: ${message}`); + stats.errors.push({ ctx: ws.id, message }); + continue; + } + + if (isCanonical(ws)) { + stats.alreadyClean++; + continue; + } + + // Report the diff so operators have a clear before/after in the + // dry-run output. Non-owner members are listed by id so the log + // doubles as a record of what got dropped. + const currentSummary = ws.members + .map((m) => `${m.userId}:${m.role}`) + .join(", "); + const dropped = ws.members + .filter((m) => m.userId !== ws.ownerUserId) + .map((m) => m.userId); + const targetSummary = `${ws.ownerUserId}:admin`; + console.error( + `[cleanup] ${ws.id} (owner=${ws.ownerUserId}): members [${currentSummary}] → [${targetSummary}]` + + (dropped.length > 0 ? ` (dropped: ${dropped.join(", ")})` : ""), + ); + + if (!args.apply) { + stats.cleaned++; + continue; + } + + try { + // Write the full record directly — `WorkspaceStore.update` now + // rejects member mutations on personal workspaces (by design), + // and the same is true at the addMember/removeMember layer. + // The cleanup script's purpose is precisely to repair the state + // that the store no longer permits to be reached through normal + // mutation, so it writes around the guard at the filesystem + // layer using the same precedent set by Stage 1's + // `stampNonPersonal` (which also bypassed `update`). + const updated: Workspace = { + ...ws, + members: canonicalMembers(ws.ownerUserId), + updatedAt: new Date().toISOString(), + }; + const wsPath = join(workspacesDir, ws.id, "workspace.json"); + await writeJsonAtomic(wsPath, updated); + stats.cleaned++; + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + console.error(`[cleanup] ERROR writing ${ws.id}: ${message}`); + stats.errors.push({ ctx: ws.id, message }); + } + } + + console.error(""); + console.error(`[cleanup] summary${args.apply ? "" : " (dry-run)"}:`); + console.error(`[cleanup] workspaces scanned: ${stats.workspacesScanned}`); + console.error(`[cleanup] non-personal skipped: ${stats.nonPersonalSkipped}`); + console.error(`[cleanup] personal scanned: ${stats.personalScanned}`); + console.error(`[cleanup] already canonical: ${stats.alreadyClean}`); + console.error(`[cleanup] ${args.apply ? "cleaned" : "would clean"}:${ + args.apply ? " " : " " + } ${stats.cleaned}`); + console.error(`[cleanup] errors: ${stats.errors.length}`); + if (stats.errors.length > 0) { + for (const e of stats.errors) console.error(`[cleanup] [error] ${e.ctx}: ${e.message}`); + process.exit(1); + } +} + +main().catch((err: unknown) => { + console.error(err); + process.exit(1); +}); diff --git a/scripts/migrate-personal-workspaces.ts b/scripts/migrate-personal-workspaces.ts index e29ba263..4c4b5812 100644 --- a/scripts/migrate-personal-workspaces.ts +++ b/scripts/migrate-personal-workspaces.ts @@ -233,9 +233,11 @@ async function stampNonPersonal( if (!wantsPersonalStamp && !wantsAboutStamp) return false; if (dryRun) return true; - // Write directly — `store.update` strips isPersonal/ownerUserId by - // design (Task 001). For the migration we DO need to write isPersonal, - // so we bypass the patch shape and write the full record. + // Write directly — `store.update` throws PersonalWorkspaceInvariantError + // when an isPersonal/ownerUserId patch is attempted (Stage 1.1). The + // migration legitimately needs to write isPersonal on workspaces that + // don't carry it yet, so we bypass the patch shape and write the full + // record. Same precedent the Stage 1.1 cleanup script follows. const updated: Workspace = { ...ws, isPersonal: false, diff --git a/src/api/handlers.ts b/src/api/handlers.ts index cc05232a..1988d0fa 100644 --- a/src/api/handlers.ts +++ b/src/api/handlers.ts @@ -22,6 +22,7 @@ import type { ResourceData } from "../tools/types.ts"; import { validateToolInput } from "../tools/validate-input.ts"; import { estimateCost } from "../usage/cost.ts"; import { bytesToBase64 } from "../util/base64.ts"; +import { PersonalWorkspaceInvariantError } from "../workspace/errors.ts"; import type { ConversationEventManager } from "./conversation-events.ts"; import type { SseEventManager } from "./events.ts"; import { ChatRequestBody, ToolCallRequestEnvelope } from "./schemas/rest.ts"; @@ -161,6 +162,44 @@ function conversationCorruptedResponse(err: ConversationCorruptedError): Respons }); } +/** + * Map `PersonalWorkspaceInvariantError` to a structured 422 response. + * Mirrors `conversationCorruptedResponse` — 422 over 500 because the + * request is well-formed; the state it would produce isn't. The + * `reason` field is the structured handle clients use to react (e.g. + * surface "cannot remove members from a personal workspace" without + * parsing the human message). + */ +function personalWorkspaceInvariantResponse(err: PersonalWorkspaceInvariantError): Response { + return apiError(422, "personal_workspace_invariant", err.message, { + workspaceId: err.workspaceId, + reason: err.reason, + }); +} + +/** + * Recognize the structuredContent shape that the workspace-mgmt tool + * handlers emit when they catch `PersonalWorkspaceInvariantError`. + * `structuredContent` rides through the in-process MCP serialization + * intact, so we can re-detect the original invariant violation from the + * tool result on the HTTP side without preserving the typed class + * across the boundary. See `workspace-mgmt-tools.ts::personalWorkspaceInvariantToolResult`. + */ +function isPersonalWorkspaceInvariantToolResult(structured: unknown): structured is { + error: "personal_workspace_invariant"; + workspaceId: string; + reason: string; + message?: string; +} { + if (!structured || typeof structured !== "object") return false; + const obj = structured as Record; + return ( + obj.error === "personal_workspace_invariant" && + typeof obj.workspaceId === "string" && + typeof obj.reason === "string" + ); +} + /** Handle POST /v1/chat/stream — SSE streaming chat request. */ export async function handleChatStream( request: Request, @@ -770,9 +809,40 @@ export async function handleToolCall( }; sseManager?.emit(failEvent); eventSink?.emit(failEvent); + // Typed invariant errors get mapped to clean HTTP status codes + // (mirrors how /v1/chat handles ConversationCorruptedError). The + // direct-throw path (in-process tool that bubbles up to here without + // crossing the MCP serialization boundary) preserves the typed + // class. The structuredContent-marker path below handles the case + // where the error already became a ToolResult inside an in-process + // MCP source. + if (err instanceof PersonalWorkspaceInvariantError) { + return personalWorkspaceInvariantResponse(err); + } throw err; } + // Recognize a PersonalWorkspaceInvariantError encoded in the tool + // result. The error class doesn't survive the in-process MCP + // serialization boundary (handler throws → SDK catches → JSON-RPC + // error → SDK client throws a generic McpError), so workspace-mgmt + // tool handlers encode it as `structuredContent.error === "personal_ + // workspace_invariant"` with `reason` + `workspaceId`. We unwrap that + // here so callers (web shell, external MCP clients) see a clean 422 + // with the same structured body as the direct-throw path above. + if (result.isError && isPersonalWorkspaceInvariantToolResult(result.structuredContent)) { + const sc = result.structuredContent; + return apiError( + 422, + "personal_workspace_invariant", + typeof sc.message === "string" ? sc.message : "Personal-workspace invariant violated", + { + workspaceId: sc.workspaceId, + reason: sc.reason, + }, + ); + } + const ms = Math.round(performance.now() - t0); // Emit bridge.tool.done after execution (ephemeral SSE + durable event sink) const doneEvent = { diff --git a/src/tools/workspace-mgmt-tools.ts b/src/tools/workspace-mgmt-tools.ts index 3312639f..bb438b4a 100644 --- a/src/tools/workspace-mgmt-tools.ts +++ b/src/tools/workspace-mgmt-tools.ts @@ -3,6 +3,7 @@ import type { ToolResult } from "../engine/types.ts"; import type { UserIdentity } from "../identity/provider.ts"; import { ORG_ADMIN_ROLES } from "../identity/types.ts"; import type { UserStore } from "../identity/user.ts"; +import { PersonalWorkspaceInvariantError } from "../workspace/errors.ts"; import type { WorkspaceStore } from "../workspace/workspace-store.ts"; import type { InProcessTool } from "./in-process-app.ts"; @@ -187,6 +188,12 @@ async function handleCreate( isError: false, }; } catch (err) { + // PersonalWorkspaceInvariantError propagates to the HTTP layer + // (mapped to 422). Swallowing it here would degrade a sharp + // identity-boundary violation into a soft 200 + isError:true. + if (err instanceof PersonalWorkspaceInvariantError) { + return personalWorkspaceInvariantToolResult(err); + } return { content: textContent( `Failed to create workspace: ${err instanceof Error ? err.message : String(err)}`, @@ -250,6 +257,9 @@ async function handleUpdate( isError: false, }; } catch (err) { + if (err instanceof PersonalWorkspaceInvariantError) { + return personalWorkspaceInvariantToolResult(err); + } return { content: textContent( `Failed to update workspace: ${err instanceof Error ? err.message : String(err)}`, @@ -366,6 +376,28 @@ function memberPermissionDenied(): ToolResult { }; } +/** + * Encode `PersonalWorkspaceInvariantError` into the ToolResult so the + * HTTP layer (`handleToolCall`) can recognize it and map to a 422 with + * a structured body — the typed error class itself is lost across the + * in-process MCP serialization boundary. The marker is the `error` + * field on `structuredContent`; consumers outside the HTTP layer (the + * agent loop, external MCP clients) see a regular `isError: true` + * result and can read the same `structuredContent` if they care. + */ +function personalWorkspaceInvariantToolResult(err: PersonalWorkspaceInvariantError): ToolResult { + return { + content: textContent(err.message), + structuredContent: { + error: "personal_workspace_invariant", + workspaceId: err.workspaceId, + reason: err.reason, + message: err.message, + }, + isError: true, + }; +} + /** @deprecated Member management is now handled by manage_workspaces. Kept for test coverage of handler logic. */ export function createManageMembersTool(ctx: ManageMembersContext): InProcessTool { return { @@ -475,6 +507,9 @@ async function handleAddMember( isError: false, }; } catch (err) { + if (err instanceof PersonalWorkspaceInvariantError) { + return personalWorkspaceInvariantToolResult(err); + } return { content: textContent( `Failed to add member: ${err instanceof Error ? err.message : String(err)}`, @@ -536,6 +571,9 @@ async function handleRemoveMember( isError: false, }; } catch (err) { + if (err instanceof PersonalWorkspaceInvariantError) { + return personalWorkspaceInvariantToolResult(err); + } return { content: textContent( `Failed to remove member: ${err instanceof Error ? err.message : String(err)}`, @@ -613,6 +651,9 @@ async function handleUpdateMember( isError: false, }; } catch (err) { + if (err instanceof PersonalWorkspaceInvariantError) { + return personalWorkspaceInvariantToolResult(err); + } return { content: textContent( `Failed to update member: ${err instanceof Error ? err.message : String(err)}`, diff --git a/src/workspace/errors.ts b/src/workspace/errors.ts new file mode 100644 index 00000000..56e5e320 --- /dev/null +++ b/src/workspace/errors.ts @@ -0,0 +1,69 @@ +/** + * Workspace-store invariant errors. + * + * The data layer is the source of truth for workspace invariants. The + * store throws typed errors on violation; the HTTP layer maps each one to + * a clean status code with a structured body. Mirrors the precedent set + * by `ConversationCorruptedError` → 422 in `src/runtime/errors.ts`. + */ + +/** + * Why the personal-workspace invariant was violated. Each value names + * exactly one rule, so the HTTP body's `reason` field is enough for + * operators / clients to act without parsing the human message. + * + * - `members_mutation` — attempted to add/remove members or + * change the owner's role on a + * personal workspace. Members are + * locked to `[{ userId: ownerUserId, + * role: "admin" }]`. + * - `is_personal_frozen` — attempted to flip the `isPersonal` + * flag after create (in either + * direction). Identity-bound at create + * time. + * - `owner_user_id_frozen` — attempted to change `ownerUserId` + * on a personal workspace. The + * owner-of-record cannot be reassigned + * through a patch. + * - `owner_user_id_on_non_personal` — attempted to set `ownerUserId` on + * a workspace where `isPersonal !== + * true`. The two fields travel + * together; one without the other is + * forbidden. + */ +export type PersonalWorkspaceInvariantReason = + | "members_mutation" + | "is_personal_frozen" + | "owner_user_id_frozen" + | "owner_user_id_on_non_personal"; + +/** + * Thrown when a `WorkspaceStore.create` or `WorkspaceStore.update` call + * violates one of the personal-workspace invariants. + * + * Personal workspaces (`isPersonal === true`) are sole-owner by design: + * the `members` array MUST be exactly `[{ userId: ownerUserId, role: + * "admin" }]`, and the identity fields (`isPersonal`, `ownerUserId`) are + * frozen at create time. These rules turn personal workspaces into a + * stable per-user namespace that the rest of the platform (conversation + * ownership, credential isolation, agent delegation) can reason about + * without re-checking on every read. + * + * The HTTP handler maps this to `422 personal_workspace_invariant` with + * a structured `{ error, reason, workspaceId }` body — same shape as + * `ConversationCorruptedError`. + */ +export class PersonalWorkspaceInvariantError extends Error { + readonly code = "personal_workspace_invariant"; + constructor( + public readonly workspaceId: string, + public readonly reason: PersonalWorkspaceInvariantReason, + detail?: string, + ) { + super( + `Personal-workspace invariant violated on ${workspaceId} (${reason})` + + (detail ? `: ${detail}` : ""), + ); + this.name = "PersonalWorkspaceInvariantError"; + } +} diff --git a/src/workspace/provisioning.ts b/src/workspace/provisioning.ts index 10504ee4..2d514d36 100644 --- a/src/workspace/provisioning.ts +++ b/src/workspace/provisioning.ts @@ -1,6 +1,6 @@ +import { PersonalWorkspaceInvariantError } from "./errors.ts"; import type { Workspace } from "./types.ts"; import { - MemberConflictError, personalWorkspaceIdFor, personalWorkspaceSlugFor, WorkspaceConflictError, @@ -25,16 +25,26 @@ export interface ProvisioningIdentity { * helper does not touch those. * * Providers call this on every successful verifyRequest so the invariant is - * self-healing — any state drift (admin deletion, partial failure, users - * migrated from a prior build) is corrected on next login. + * self-healing — any state drift (workspace missing, partial-applied + * migration) is corrected on next login. * * Behavior: - * - Personal workspace exists at the canonical id, user is a member → no writes, return it. - * - Personal workspace exists at the canonical id, user is NOT a member → add as admin, return. - * - Personal workspace does not exist → create with `isPersonal: true` + `ownerUserId`, add user as admin. - * - Concurrent first-login race → one winner creates, losers detect the conflict and re-read. + * - Personal workspace exists at the canonical id → return it. The store's + * create-time invariant guarantees the owner is the sole admin member; + * we don't second-guess that here. + * - Personal workspace does not exist → create with `isPersonal: true` + + * `ownerUserId`, which `WorkspaceStore.create` populates with the + * owner-admin member. + * - Concurrent first-login race → one winner creates, losers detect the + * conflict and re-read. * * Returns the user's personal workspace (always — never a shared one). + * + * Pre-Stage-1.1 state (the user exists but their personal workspace's + * member list isn't the canonical sole-owner-admin) is NOT auto-healed + * here. The membership invariant is now enforced by the store; bumping + * it from a login hot-path would silently mutate identity-bound state. + * Operators recover via `scripts/cleanup-personal-workspace-members.ts`. */ export async function ensureUserWorkspace( store: WorkspaceStore, @@ -43,81 +53,42 @@ export async function ensureUserWorkspace( const wsId = personalWorkspaceIdFor(identity.id); const existing = await store.get(wsId); - if (existing) { - const isMember = existing.members.some((m) => m.userId === identity.id); - if (isMember) return existing; - // The workspace exists but the user isn't a member — defensive - // self-heal. Should be rare: typically an admin accidentally removed - // them, or a migration partial-applied. Re-add and continue. - try { - return await store.addMember(wsId, identity.id, "admin"); - } catch (err) { - if (err instanceof MemberConflictError) { - return (await store.get(wsId)) ?? existing; - } - throw err; - } - } + if (existing) return existing; const name = identity.displayName ? `${identity.displayName}'s Workspace` : "Workspace"; const slug = personalWorkspaceSlugFor(identity.id); try { - const ws = await store.create(name, slug, { + return await store.create(name, slug, { isPersonal: true, ownerUserId: identity.id, }); - try { - return await store.addMember(ws.id, identity.id, "admin"); - } catch (err) { - // A loser of the create race can reach reconcileConflict and call - // addMember before we do. Tolerate it: re-read and return. - if (err instanceof MemberConflictError) { - return (await store.get(ws.id)) ?? ws; - } - throw err; - } } catch (err) { - if (!(err instanceof WorkspaceConflictError)) throw err; - return reconcileConflict(store, identity, wsId); + if (err instanceof WorkspaceConflictError) { + return reconcileConflict(store, wsId); + } + // A `PersonalWorkspaceInvariantError` from create() here would mean + // the helper itself produced a bad-shape personal workspace — bug, + // let it surface. + if (err instanceof PersonalWorkspaceInvariantError) throw err; + throw err; } } /** * A `create()` collision on the canonical personal-workspace id means - * another concurrent call won the race. Recover by re-reading and - * ensuring membership. Never create a second workspace with a different - * slug — two personal workspaces per user is exactly the bug the - * canonical-id model exists to prevent. + * another concurrent call won the race. Re-read and return. Never create + * a second workspace with a different slug — two personal workspaces per + * user is exactly the bug the canonical-id model exists to prevent. */ -async function reconcileConflict( - store: WorkspaceStore, - identity: ProvisioningIdentity, - wsId: string, -): Promise { +async function reconcileConflict(store: WorkspaceStore, wsId: string): Promise { const existing = await store.get(wsId); - if (!existing) { - // WorkspaceConflictError fires only when store.get() returned non-null - // inside create() — so reaching here means the workspace existed at - // throw time and was deleted before our re-read (concurrent delete, - // rare). Recreate it. - const ws = await store.create( - identity.displayName ? `${identity.displayName}'s Workspace` : "Workspace", - personalWorkspaceSlugFor(identity.id), - { isPersonal: true, ownerUserId: identity.id }, - ); - return await store.addMember(ws.id, identity.id, "admin"); - } - - const isMember = existing.members.some((m) => m.userId === identity.id); - if (isMember) return existing; - - try { - return await store.addMember(existing.id, identity.id, "admin"); - } catch (err) { - if (err instanceof MemberConflictError) { - return (await store.get(existing.id)) ?? existing; - } - throw err; - } + if (existing) return existing; + // WorkspaceConflictError fires only when store.get() returned non-null + // inside create() — so reaching here means the workspace existed at + // throw time and was deleted before our re-read (concurrent delete, + // rare). Surface the inconsistency; callers retry. + throw new Error( + `[provisioning] personal workspace ${wsId} disappeared between create-conflict and re-read`, + ); } diff --git a/src/workspace/workspace-store.ts b/src/workspace/workspace-store.ts index 6b918b92..75c8d5bb 100644 --- a/src/workspace/workspace-store.ts +++ b/src/workspace/workspace-store.ts @@ -2,6 +2,7 @@ import { existsSync, mkdirSync } from "node:fs"; import { readdir, readFile, rm } from "node:fs/promises"; import { join } from "node:path"; import { writeJsonAtomic } from "../util/atomic-json.ts"; +import { PersonalWorkspaceInvariantError } from "./errors.ts"; import { scaffoldWorkspace } from "./scaffold.ts"; import type { Workspace, WorkspaceMember, WorkspaceRole } from "./types.ts"; @@ -152,6 +153,14 @@ export class WorkspaceStore { ownerUserId?: string; /** Short human-readable description; defaults to `null`. */ about?: string | null; + /** + * Initial members. Personal workspaces force this to + * `[{ userId: ownerUserId, role: "admin" }]` — supplying anything + * else throws `PersonalWorkspaceInvariantError`. Shared workspaces + * default to `[]` (the caller invokes `addMember` afterwards to + * populate). + */ + members?: WorkspaceMember[]; }, ): Promise { const derivedSlug = slug ?? slugify(name); @@ -172,6 +181,37 @@ export class WorkspaceStore { throw new Error("[workspace-store] create: ownerUserId is only valid with isPersonal=true"); } + // Personal-workspace member shape: sole-owner-admin only. This is + // the create-time enforcement of the invariant that `update` / + // `addMember` / `removeMember` / `updateMemberRole` also defend at + // mutation time. A caller that supplies any other shape is making a + // claim the type system can't catch (member arrays carry no + // identity binding) — surface it loudly here so the next operator + // doesn't inherit a multi-admin personal workspace. + let members: WorkspaceMember[] = opts?.members ?? []; + if (isPersonal) { + const ownerUserId = opts?.ownerUserId; + // Unreachable in practice (the co-required check above already + // threw), but narrows the type for the assignment below. + if (!ownerUserId) { + throw new Error("[workspace-store] create: isPersonal=true requires ownerUserId"); + } + if (opts?.members !== undefined) { + const ok = + opts.members.length === 1 && + opts.members[0]?.userId === ownerUserId && + opts.members[0]?.role === "admin"; + if (!ok) { + throw new PersonalWorkspaceInvariantError( + id, + "members_mutation", + "personal workspace initial members must be exactly [{ userId: ownerUserId, role: 'admin' }]", + ); + } + } + members = [{ userId: ownerUserId, role: "admin" }]; + } + // Slug collision detection const existing = await this.get(id); if (existing) { @@ -182,7 +222,7 @@ export class WorkspaceStore { const workspace: Workspace = { id, name, - members: [], + members, bundles: [], createdAt: now, updatedAt: now, @@ -218,16 +258,89 @@ export class WorkspaceStore { const ws = await this.get(id); if (!ws) return null; - // Runtime guard for the type-level Pick: `isPersonal` and `ownerUserId` - // are identity-bound at create time and not patchable. A caller that - // casts through the type system (`as unknown as { name: string }`) - // would otherwise bypass the Pick — strip the disallowed keys - // explicitly so the invariant holds at runtime too. + // Runtime guard for the type-level Pick: `isPersonal`, `ownerUserId`, + // and `members` are identity-bound at create time and not patchable + // here. The Pick<> excludes them at the type level, but a caller can + // cast through the type system (`as unknown as { name: string }`), + // and historic callers did exactly that. Detect the attempt and + // throw a typed error instead of silently stripping — the silent + // strip is the failure mode that produced multi-admin personal + // workspaces on hq production. + // + // Casts here are scoped to read-only inspection of the widened patch + // shape. We do NOT widen the spread that builds `updated` — only + // fields in the Pick can land on disk. + const widePatch = patch as Partial; + const wantsIsPersonal = "isPersonal" in widePatch; + const wantsOwnerUserId = "ownerUserId" in widePatch; + const wantsMembers = "members" in widePatch; + + if (wantsIsPersonal) { + // Frozen post-create in both directions: a workspace's + // "personal-ness" is part of its identity. + if (widePatch.isPersonal !== ws.isPersonal) { + throw new PersonalWorkspaceInvariantError( + id, + "is_personal_frozen", + `cannot change isPersonal from ${String(ws.isPersonal === true)} to ${String(widePatch.isPersonal === true)}`, + ); + } + } + + if (wantsOwnerUserId) { + if (ws.isPersonal === true) { + if (widePatch.ownerUserId !== ws.ownerUserId) { + throw new PersonalWorkspaceInvariantError( + id, + "owner_user_id_frozen", + `cannot change ownerUserId from ${ws.ownerUserId ?? "(unset)"} to ${ + widePatch.ownerUserId ?? "(unset)" + }`, + ); + } + } else { + // Non-personal workspaces MUST NOT carry an ownerUserId — the + // two fields travel together (see `Workspace.ownerUserId`). + if (widePatch.ownerUserId !== undefined) { + throw new PersonalWorkspaceInvariantError( + id, + "owner_user_id_on_non_personal", + "ownerUserId can only be set on a workspace where isPersonal === true", + ); + } + } + } + + if (wantsMembers && ws.isPersonal === true) { + // Personal-workspace members are locked to sole-owner-admin. + // Membership changes go through `addMember` / `removeMember` / + // `updateMemberRole`, which carry the same guard. + const proposed = widePatch.members ?? []; + const ownerUserId = ws.ownerUserId; + const ok = + proposed.length === 1 && + proposed[0]?.userId === ownerUserId && + proposed[0]?.role === "admin"; + if (!ok) { + throw new PersonalWorkspaceInvariantError( + id, + "members_mutation", + "personal workspace members are locked to [{ userId: ownerUserId, role: 'admin' }]", + ); + } + } + + // Build the safe patch from the type-level Pick only. We strip the + // identity-bound keys (`isPersonal`, `ownerUserId`, `members`) from + // the spread — the guards above have already validated they're + // either absent, equal to the current value, or rejected — so the + // record on disk never gains a field outside the Pick. const { isPersonal: _isPersonal, ownerUserId: _ownerUserId, + members: _members, ...safePatch - } = patch as Partial; + } = widePatch; const updated: Workspace = { ...ws, @@ -252,6 +365,17 @@ export class WorkspaceStore { const ws = await this.get(wsId); if (!ws) throw new WorkspaceNotFoundError(wsId); + // Personal workspaces are sole-owner. Any addMember call against + // one violates the invariant — even adding the owner again, which + // would shadow the create-time entry. + if (ws.isPersonal === true) { + throw new PersonalWorkspaceInvariantError( + wsId, + "members_mutation", + `cannot add member ${userId} to a personal workspace; membership is locked to the owner`, + ); + } + const existing = ws.members.find((m) => m.userId === userId); if (existing) throw new MemberConflictError(wsId, userId); @@ -270,6 +394,14 @@ export class WorkspaceStore { const ws = await this.get(wsId); if (!ws) throw new WorkspaceNotFoundError(wsId); + if (ws.isPersonal === true) { + throw new PersonalWorkspaceInvariantError( + wsId, + "members_mutation", + `cannot remove member ${userId} from a personal workspace; membership is locked to the owner`, + ); + } + const updated: Workspace = { ...ws, members: ws.members.filter((m) => m.userId !== userId), @@ -284,6 +416,14 @@ export class WorkspaceStore { const ws = await this.get(wsId); if (!ws) throw new WorkspaceNotFoundError(wsId); + if (ws.isPersonal === true) { + throw new PersonalWorkspaceInvariantError( + wsId, + "members_mutation", + `cannot change role for ${userId} on a personal workspace; the owner is admin and the membership list is frozen`, + ); + } + const updated: Workspace = { ...ws, members: ws.members.map((m) => (m.userId === userId ? { ...m, role } : m)), diff --git a/test/integration/api/personal-workspace-invariants.test.ts b/test/integration/api/personal-workspace-invariants.test.ts new file mode 100644 index 00000000..3f6b5962 --- /dev/null +++ b/test/integration/api/personal-workspace-invariants.test.ts @@ -0,0 +1,172 @@ +/** + * End-to-end coverage: a mutation that violates the personal-workspace + * invariants returns HTTP 422 with the structured body the spec + * defines, through the real `/v1/tools/call` surface and the real + * `manage_workspaces` tool. + * + * The unit suite (`test/unit/workspace/personal-workspace-invariants.test.ts`) + * exhaustively covers the four invariants at the store layer; this file + * only proves the typed-error → 422 mapping survives the in-process MCP + * serialization boundary. + */ + +import { mkdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterAll, beforeAll, describe, expect, it } from "bun:test"; +import { startServer, type ServerHandle } from "../../../src/api/server.ts"; +import { Runtime } from "../../../src/runtime/runtime.ts"; +import { personalWorkspaceIdFor } from "../../../src/workspace/workspace-store.ts"; +import { createEchoModel } from "../../helpers/echo-model.ts"; +import { createTestAuthAdapter, TEST_IDENTITY } from "../../helpers/test-auth-adapter.ts"; + +const API_KEY = "personal-invariant-test-key"; + +describe("POST /v1/tools/call manage_workspaces — personal workspace invariants → 422", () => { + let runtime: Runtime; + let handle: ServerHandle; + let baseUrl: string; + let personalWsId: string; + const workDir = join(tmpdir(), `nimblebrain-personal-invariants-${Date.now()}`); + + beforeAll(async () => { + mkdirSync(workDir, { recursive: true }); + runtime = await Runtime.start({ + model: { provider: "custom", adapter: createEchoModel() }, + noDefaultBundles: true, + logging: { disabled: true }, + workDir, + }); + + // The test adapter provisions a personal workspace for TEST_IDENTITY + // on first auth — but that's lazy, and we want the row before the + // tool call runs. Create it explicitly so the route resolves to a + // real workspace and we have a known target for the invariant + // violation. + personalWsId = personalWorkspaceIdFor(TEST_IDENTITY.id); + const wsStore = runtime.getWorkspaceStore(); + if (!(await wsStore.get(personalWsId))) { + await wsStore.create("Test User's Workspace", personalWsId.slice(3), { + isPersonal: true, + ownerUserId: TEST_IDENTITY.id, + }); + } + await runtime.ensureWorkspaceRegistry(personalWsId); + + // Seed a second user so `add_member` reaches the store's + // PersonalWorkspaceInvariantError path instead of bouncing on the + // earlier "user not found" check inside the tool handler. + const userStore = runtime.getUserStore(); + if (!(await userStore.getByEmail("other@example.com"))) { + await userStore.create({ + id: "usr_other", + email: "other@example.com", + displayName: "Other", + orgRole: "member", + }); + } + + handle = startServer({ + runtime, + port: 0, + provider: createTestAuthAdapter(API_KEY, runtime), + }); + baseUrl = `http://localhost:${handle.port}`; + }); + + afterAll(async () => { + handle.stop(true); + await runtime.shutdown(); + rmSync(workDir, { recursive: true, force: true }); + }); + + function callToolHeaders(): Record { + return { + "Content-Type": "application/json", + Authorization: `Bearer ${API_KEY}`, + "X-Workspace-Id": personalWsId, + }; + } + + it("add_member on a personal workspace returns 422 personal_workspace_invariant", async () => { + const res = await fetch(`${baseUrl}/v1/tools/call`, { + method: "POST", + headers: callToolHeaders(), + body: JSON.stringify({ + server: "nb", + tool: "manage_workspaces", + arguments: { + action: "add_member", + workspaceId: personalWsId, + userId: "usr_other", + role: "admin", + }, + }), + }); + + expect(res.status).toBe(422); + const body = (await res.json()) as { + error: string; + message: string; + details?: { workspaceId?: string; reason?: string }; + }; + expect(body.error).toBe("personal_workspace_invariant"); + expect(body.details?.reason).toBe("members_mutation"); + expect(body.details?.workspaceId).toBe(personalWsId); + // The human message includes context the operator can act on. + expect(body.message).toContain(personalWsId); + }); + + it("update_member on the owner of a personal workspace returns 422 members_mutation", async () => { + // role="admin" (a no-op role-wise) bypasses the "cannot demote last + // admin" pre-check inside the tool handler so the call actually + // reaches `WorkspaceStore.updateMemberRole`, which is the layer + // that enforces the invariant. The point of the test is that the + // store-layer rejection survives the in-process MCP boundary as + // structuredContent + becomes a 422 at the HTTP boundary. + const res = await fetch(`${baseUrl}/v1/tools/call`, { + method: "POST", + headers: callToolHeaders(), + body: JSON.stringify({ + server: "nb", + tool: "manage_workspaces", + arguments: { + action: "update_member", + workspaceId: personalWsId, + userId: TEST_IDENTITY.id, + role: "admin", + }, + }), + }); + + expect(res.status).toBe(422); + const body = (await res.json()) as { + error: string; + details?: { reason?: string }; + }; + expect(body.error).toBe("personal_workspace_invariant"); + expect(body.details?.reason).toBe("members_mutation"); + }); + + it("normal update (name) on a personal workspace still succeeds — invariant scoped to identity fields", async () => { + // Topology adversarial: confirm we didn't over-lock. A name update + // on a personal workspace must continue to work. + const res = await fetch(`${baseUrl}/v1/tools/call`, { + method: "POST", + headers: callToolHeaders(), + body: JSON.stringify({ + server: "nb", + tool: "manage_workspaces", + arguments: { + action: "update", + workspaceId: personalWsId, + name: "Renamed Personal Workspace", + }, + }), + }); + + expect(res.status).toBe(200); + const body = (await res.json()) as { isError: boolean }; + expect(body.isError).toBe(false); + }); +}); diff --git a/test/integration/scripts/cleanup-personal-workspace-members.test.ts b/test/integration/scripts/cleanup-personal-workspace-members.test.ts new file mode 100644 index 00000000..3153912c --- /dev/null +++ b/test/integration/scripts/cleanup-personal-workspace-members.test.ts @@ -0,0 +1,208 @@ +/** + * Exercises scripts/cleanup-personal-workspace-members.ts against a fake + * work tree. Classified as integration because it spawns `bun` on the + * script. + * + * Covers: + * - Multi-admin personal workspace gets cleaned to sole-owner-admin. + * - Workspace already in desired state is a no-op (idempotency). + * - Missing `ownerUserId` on a personal workspace → hard-error. + * - Non-personal workspace is untouched. + * - Dry-run is the default; --apply is required to write. + */ + +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; + +const SCRIPT = join( + import.meta.dirname ?? __dirname, + "..", + "..", + "..", + "scripts", + "cleanup-personal-workspace-members.ts", +); + +let workDir: string; + +beforeEach(async () => { + workDir = await mkdtemp(join(tmpdir(), "nb-cleanup-pwm-")); + // Cleanup script walks `workspaces/*/workspace.json`; the dir must exist. + await mkdir(join(workDir, "workspaces"), { recursive: true }); +}); + +afterEach(async () => { + await rm(workDir, { recursive: true, force: true }); +}); + +interface SeedOpts { + id: string; + name?: string; + members: Array<{ userId: string; role: "admin" | "member" }>; + isPersonal?: boolean; + ownerUserId?: string; +} + +async function seedWorkspace(opts: SeedOpts): Promise { + const wsDir = join(workDir, "workspaces", opts.id); + await mkdir(wsDir, { recursive: true, mode: 0o700 }); + const now = new Date().toISOString(); + const ws: Record = { + id: opts.id, + name: opts.name ?? opts.id, + members: opts.members, + bundles: [], + createdAt: now, + updatedAt: now, + }; + if (opts.isPersonal !== undefined) ws.isPersonal = opts.isPersonal; + if (opts.ownerUserId !== undefined) ws.ownerUserId = opts.ownerUserId; + await writeFile(join(wsDir, "workspace.json"), `${JSON.stringify(ws, null, 2)}\n`); +} + +async function readWorkspace(id: string): Promise> { + const raw = await readFile(join(workDir, "workspaces", id, "workspace.json"), "utf-8"); + return JSON.parse(raw) as Record; +} + +async function runCleanup(args: string[] = []): Promise<{ exitCode: number; stderr: string }> { + const proc = Bun.spawn({ + cmd: ["bun", "run", SCRIPT, "--work-dir", workDir, ...args], + stdout: "pipe", + stderr: "pipe", + }); + const [stderr, exitCode] = await Promise.all([ + new Response(proc.stderr).text(), + proc.exited, + ]); + return { exitCode, stderr }; +} + +describe("cleanup-personal-workspace-members", () => { + test("multi-admin personal workspace gets cleaned to sole-owner-admin", async () => { + // The exact failure mode that motivated the script: an + // hq-production personal workspace with three admins. + await seedWorkspace({ + id: "ws_user_user_alice", + isPersonal: true, + ownerUserId: "user_alice", + members: [ + { userId: "user_alice", role: "admin" }, + { userId: "user_mat", role: "admin" }, + { userId: "user_mario", role: "admin" }, + ], + }); + + const { exitCode, stderr } = await runCleanup(["--apply"]); + expect(exitCode).toBe(0); + expect(stderr).toContain("ws_user_user_alice"); + // The log should name the dropped members so the operator can + // verify what was removed. + expect(stderr).toContain("user_mat"); + expect(stderr).toContain("user_mario"); + + const ws = await readWorkspace("ws_user_user_alice"); + expect(ws.members).toEqual([{ userId: "user_alice", role: "admin" }]); + expect(ws.isPersonal).toBe(true); + expect(ws.ownerUserId).toBe("user_alice"); + }); + + test("personal workspace with the owner-only-member but non-admin role gets fixed to admin", async () => { + await seedWorkspace({ + id: "ws_user_user_bob", + isPersonal: true, + ownerUserId: "user_bob", + members: [{ userId: "user_bob", role: "member" }], + }); + + const { exitCode } = await runCleanup(["--apply"]); + expect(exitCode).toBe(0); + + const ws = await readWorkspace("ws_user_user_bob"); + expect(ws.members).toEqual([{ userId: "user_bob", role: "admin" }]); + }); + + test("workspace already in canonical shape is a no-op (idempotency)", async () => { + await seedWorkspace({ + id: "ws_user_user_alice", + isPersonal: true, + ownerUserId: "user_alice", + members: [{ userId: "user_alice", role: "admin" }], + }); + + const before = await readWorkspace("ws_user_user_alice"); + + const { exitCode, stderr } = await runCleanup(["--apply"]); + expect(exitCode).toBe(0); + expect(stderr).toContain("already canonical: 1"); + + const after = await readWorkspace("ws_user_user_alice"); + // Idempotent: even `updatedAt` is untouched because we don't write. + expect(after.updatedAt).toBe(before.updatedAt); + }); + + test("missing ownerUserId on a personal workspace is a hard-error", async () => { + await seedWorkspace({ + id: "ws_user_orphan", + isPersonal: true, + // ownerUserId intentionally omitted — simulates pre-Stage-1 + // tenants where the migration's stamp didn't reach this row. + members: [{ userId: "user_x", role: "admin" }], + }); + + const { exitCode, stderr } = await runCleanup(["--apply"]); + expect(exitCode).toBe(1); + expect(stderr).toContain("no ownerUserId"); + + // The bad-state workspace is left alone so the operator can triage. + const ws = await readWorkspace("ws_user_orphan"); + expect(ws.members).toEqual([{ userId: "user_x", role: "admin" }]); + }); + + test("non-personal workspace is untouched even when it has many admins", async () => { + await seedWorkspace({ + id: "ws_team_alpha", + isPersonal: false, + members: [ + { userId: "user_a", role: "admin" }, + { userId: "user_b", role: "admin" }, + { userId: "user_c", role: "member" }, + ], + }); + + const { exitCode, stderr } = await runCleanup(["--apply"]); + expect(exitCode).toBe(0); + expect(stderr).toContain("non-personal skipped: 1"); + + const ws = await readWorkspace("ws_team_alpha"); + expect(ws.members).toEqual([ + { userId: "user_a", role: "admin" }, + { userId: "user_b", role: "admin" }, + { userId: "user_c", role: "member" }, + ]); + }); + + test("dry-run is the default — without --apply, no writes happen", async () => { + await seedWorkspace({ + id: "ws_user_user_alice", + isPersonal: true, + ownerUserId: "user_alice", + members: [ + { userId: "user_alice", role: "admin" }, + { userId: "user_evil", role: "admin" }, + ], + }); + + const before = await readWorkspace("ws_user_user_alice"); + + const { exitCode, stderr } = await runCleanup(); + expect(exitCode).toBe(0); + expect(stderr).toContain("would clean"); + + // Disk state unchanged. + const after = await readWorkspace("ws_user_user_alice"); + expect(after).toEqual(before); + }); +}); diff --git a/test/unit/workspace/personal-workspace-invariants.test.ts b/test/unit/workspace/personal-workspace-invariants.test.ts new file mode 100644 index 00000000..ce4100e9 --- /dev/null +++ b/test/unit/workspace/personal-workspace-invariants.test.ts @@ -0,0 +1,272 @@ +/** + * Personal-workspace invariants enforced by `WorkspaceStore` (Stage 1.1). + * + * The store is the source of truth for four rules: + * 1. Personal workspaces' members are locked to + * `[{ userId: ownerUserId, role: "admin" }]`. + * 2. `isPersonal` is frozen post-create (both directions). + * 3. `ownerUserId` is frozen on personal workspaces. + * 4. `ownerUserId` MUST NOT be set on non-personal workspaces. + * + * Each rule is exercised positively (it throws) and the topology + * adversarial cases (`bundles`, `name` updates on a personal workspace) + * confirm we didn't over-lock — those still succeed. + */ + +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { PersonalWorkspaceInvariantError } from "../../../src/workspace/errors.ts"; +import { WorkspaceStore } from "../../../src/workspace/workspace-store.ts"; + +let workDir: string; +let store: WorkspaceStore; + +beforeEach(async () => { + workDir = await mkdtemp(join(tmpdir(), "ws-invariants-")); + store = new WorkspaceStore(workDir); +}); + +afterEach(async () => { + await rm(workDir, { recursive: true, force: true }); +}); + +async function createPersonal(userId = "user_alice"): Promise<{ wsId: string }> { + const ws = await store.create("Alice", `user_${userId}`, { + isPersonal: true, + ownerUserId: userId, + }); + return { wsId: ws.id }; +} + +async function createShared(slug = "team"): Promise<{ wsId: string }> { + const ws = await store.create("Team", slug); + return { wsId: ws.id }; +} + +// ── Invariant 1: members locked on personal workspaces ─────────────── + +describe("members locked on personal workspaces", () => { + test("addMember on personal workspace throws members_mutation", async () => { + const { wsId } = await createPersonal(); + let captured: PersonalWorkspaceInvariantError | null = null; + try { + await store.addMember(wsId, "user_bob", "admin"); + } catch (err) { + if (err instanceof PersonalWorkspaceInvariantError) captured = err; + else throw err; + } + expect(captured).not.toBeNull(); + expect(captured?.reason).toBe("members_mutation"); + expect(captured?.workspaceId).toBe(wsId); + }); + + test("removeMember on personal workspace (removing the owner) throws", async () => { + const { wsId } = await createPersonal(); + await expect(store.removeMember(wsId, "user_alice")).rejects.toBeInstanceOf( + PersonalWorkspaceInvariantError, + ); + }); + + test("updateMemberRole on personal workspace throws", async () => { + const { wsId } = await createPersonal(); + let captured: PersonalWorkspaceInvariantError | null = null; + try { + await store.updateMemberRole(wsId, "user_alice", "member"); + } catch (err) { + if (err instanceof PersonalWorkspaceInvariantError) captured = err; + else throw err; + } + expect(captured?.reason).toBe("members_mutation"); + }); + + test("update() with a members patch that adds a non-owner throws", async () => { + const { wsId } = await createPersonal(); + await expect( + // Cast through the type system: `members` is not in `update`'s + // Pick — runtime must still reject. + store.update(wsId, { + members: [ + { userId: "user_alice", role: "admin" }, + { userId: "user_bob", role: "admin" }, + ], + } as unknown as { name: string }), + ).rejects.toBeInstanceOf(PersonalWorkspaceInvariantError); + }); + + test("update() that removes the owner from members throws", async () => { + const { wsId } = await createPersonal(); + await expect( + store.update(wsId, { members: [] } as unknown as { name: string }), + ).rejects.toBeInstanceOf(PersonalWorkspaceInvariantError); + }); + + test("update() that demotes the owner from admin to member throws", async () => { + const { wsId } = await createPersonal(); + await expect( + store.update(wsId, { + members: [{ userId: "user_alice", role: "member" }], + } as unknown as { name: string }), + ).rejects.toBeInstanceOf(PersonalWorkspaceInvariantError); + }); +}); + +// ── Invariant 2: isPersonal frozen post-create ─────────────────────── + +describe("isPersonal frozen post-create", () => { + test("flipping isPersonal: true → false throws is_personal_frozen", async () => { + const { wsId } = await createPersonal(); + let captured: PersonalWorkspaceInvariantError | null = null; + try { + await store.update(wsId, { isPersonal: false } as unknown as { name: string }); + } catch (err) { + if (err instanceof PersonalWorkspaceInvariantError) captured = err; + else throw err; + } + expect(captured?.reason).toBe("is_personal_frozen"); + }); + + test("flipping isPersonal: false → true throws is_personal_frozen", async () => { + const { wsId } = await createShared(); + let captured: PersonalWorkspaceInvariantError | null = null; + try { + await store.update(wsId, { isPersonal: true } as unknown as { name: string }); + } catch (err) { + if (err instanceof PersonalWorkspaceInvariantError) captured = err; + else throw err; + } + expect(captured?.reason).toBe("is_personal_frozen"); + }); +}); + +// ── Invariant 3: ownerUserId frozen on personal workspaces ─────────── + +describe("ownerUserId frozen on personal workspaces", () => { + test("changing ownerUserId on a personal workspace throws", async () => { + const { wsId } = await createPersonal("user_alice"); + let captured: PersonalWorkspaceInvariantError | null = null; + try { + await store.update(wsId, { ownerUserId: "user_evil" } as unknown as { name: string }); + } catch (err) { + if (err instanceof PersonalWorkspaceInvariantError) captured = err; + else throw err; + } + expect(captured?.reason).toBe("owner_user_id_frozen"); + }); + + test("setting the same ownerUserId on a personal workspace is a no-op (idempotent)", async () => { + // Passing the existing value through `update` is allowed because + // it's not a mutation — important so callers that round-trip the + // full workspace shape don't trip the invariant. + const { wsId } = await createPersonal("user_alice"); + const updated = await store.update(wsId, { + ownerUserId: "user_alice", + } as unknown as { name: string }); + expect(updated?.ownerUserId).toBe("user_alice"); + }); +}); + +// ── Invariant 4: ownerUserId forbidden on non-personal workspaces ──── + +describe("ownerUserId forbidden on non-personal workspaces", () => { + test("setting ownerUserId on a non-personal workspace throws owner_user_id_on_non_personal", async () => { + const { wsId } = await createShared(); + let captured: PersonalWorkspaceInvariantError | null = null; + try { + await store.update(wsId, { ownerUserId: "user_bob" } as unknown as { name: string }); + } catch (err) { + if (err instanceof PersonalWorkspaceInvariantError) captured = err; + else throw err; + } + expect(captured?.reason).toBe("owner_user_id_on_non_personal"); + }); +}); + +// ── Topology adversarial: mutable fields still work ────────────────── + +describe("topology — non-identity fields stay freely mutable on personal workspaces", () => { + test("update() of bundles on a personal workspace succeeds", async () => { + const { wsId } = await createPersonal(); + const updated = await store.update(wsId, { bundles: [{ name: "echo" }] }); + expect(updated?.bundles).toEqual([{ name: "echo" }]); + // Identity fields stay intact. + expect(updated?.isPersonal).toBe(true); + expect(updated?.ownerUserId).toBe("user_alice"); + expect(updated?.members).toEqual([{ userId: "user_alice", role: "admin" }]); + }); + + test("update() of name on a personal workspace succeeds", async () => { + const { wsId } = await createPersonal(); + const updated = await store.update(wsId, { name: "Renamed" }); + expect(updated?.name).toBe("Renamed"); + expect(updated?.isPersonal).toBe(true); + }); + + test("update() of about on a personal workspace succeeds", async () => { + const { wsId } = await createPersonal(); + const updated = await store.update(wsId, { about: "my space" }); + expect(updated?.about).toBe("my space"); + }); +}); + +// ── Create-time invariants ─────────────────────────────────────────── + +describe("create() enforces the personal-workspace shape", () => { + test("personal workspace gets the owner as the sole admin by default", async () => { + const ws = await store.create("Alice", "user_user_alice", { + isPersonal: true, + ownerUserId: "user_alice", + }); + expect(ws.members).toEqual([{ userId: "user_alice", role: "admin" }]); + }); + + test("create() rejects a personal workspace with extra members", async () => { + let captured: PersonalWorkspaceInvariantError | null = null; + try { + await store.create("Alice", "user_user_alice", { + isPersonal: true, + ownerUserId: "user_alice", + members: [ + { userId: "user_alice", role: "admin" }, + { userId: "user_bob", role: "admin" }, + ], + }); + } catch (err) { + if (err instanceof PersonalWorkspaceInvariantError) captured = err; + else throw err; + } + expect(captured?.reason).toBe("members_mutation"); + }); + + test("create() rejects a personal workspace whose initial member's userId doesn't match ownerUserId", async () => { + await expect( + store.create("Alice", "user_user_alice", { + isPersonal: true, + ownerUserId: "user_alice", + members: [{ userId: "user_bob", role: "admin" }], + }), + ).rejects.toBeInstanceOf(PersonalWorkspaceInvariantError); + }); + + test("create() rejects a personal workspace whose initial member's role isn't admin", async () => { + await expect( + store.create("Alice", "user_user_alice", { + isPersonal: true, + ownerUserId: "user_alice", + members: [{ userId: "user_alice", role: "member" }], + }), + ).rejects.toBeInstanceOf(PersonalWorkspaceInvariantError); + }); + + test("create() accepts an explicit owner-admin members array (idempotent shape)", async () => { + // A caller that provides the canonical shape explicitly should + // succeed — that's normal for round-trip workflows. + const ws = await store.create("Alice", "user_user_alice", { + isPersonal: true, + ownerUserId: "user_alice", + members: [{ userId: "user_alice", role: "admin" }], + }); + expect(ws.members).toEqual([{ userId: "user_alice", role: "admin" }]); + }); +}); diff --git a/test/unit/workspace/provisioning.test.ts b/test/unit/workspace/provisioning.test.ts index 6b0f44c4..897b6217 100644 --- a/test/unit/workspace/provisioning.test.ts +++ b/test/unit/workspace/provisioning.test.ts @@ -112,23 +112,24 @@ describe("ensureUserWorkspace", () => { expect(sharedAfter?.members).toEqual([{ userId: "user_alice", role: "member" }]); }); - test("self-heals when the canonical workspace exists but the user is not a member", async () => { - // Arrange: pre-create the personal workspace WITHOUT the user as a - // member (simulates admin error or partial migration). + test("create populates the owner as sole admin so ensureUserWorkspace is a pure read on the second login (Stage 1.1)", async () => { + // Stage 1.1 invariant: `WorkspaceStore.create` produces a personal + // workspace whose `members` is already `[{ userId: ownerUserId, + // role: "admin" }]`. The earlier "personal workspace exists with + // zero members" state can no longer be reached through the + // canonical create path — and `addMember` on a personal workspace + // is now rejected by the store. So ensureUserWorkspace becomes a + // pure read on every login after the first. Operators with + // pre-Stage-1.1 data converge via + // `scripts/cleanup-personal-workspace-members.ts`. const wsId = personalWorkspaceIdFor("user_alice"); - await store.create("Alice's Workspace", wsId.slice(3), { + const pre = await store.create("Alice's Workspace", wsId.slice(3), { isPersonal: true, ownerUserId: "user_alice", }); + expect(pre.members).toEqual([{ userId: "user_alice", role: "admin" }]); - // Pre-condition: workspace exists with zero members. - const pre = await store.get(wsId); - expect(pre?.members).toEqual([]); - - // Act: provisioning runs. const ws = await ensureUserWorkspace(store, { id: "user_alice", displayName: "Alice" }); - - // The workspace is the same one; the user is now a member. expect(ws.id).toBe(wsId); expect(ws.members).toEqual([{ userId: "user_alice", role: "admin" }]); }); diff --git a/test/unit/workspace/workspace-store.test.ts b/test/unit/workspace/workspace-store.test.ts index 6c8428f3..f003c58b 100644 --- a/test/unit/workspace/workspace-store.test.ts +++ b/test/unit/workspace/workspace-store.test.ts @@ -349,20 +349,22 @@ describe("WorkspaceStore.update", () => { expect(updated?.about).toBe("new description"); }); - test("ignores attempted writes to isPersonal and ownerUserId (Pick excludes them)", async () => { + test("throws PersonalWorkspaceInvariantError on attempted writes to isPersonal/ownerUserId (Stage 1.1)", async () => { const ws = await store.create("Alice", "user_user_alice", { isPersonal: true, ownerUserId: "user_alice", }); - // Cast to bypass the Pick<> at the type level — runtime should still - // preserve the original fields because update spreads only the allowed - // keys it accepted in the Pick. (TypeScript catches misuse at compile; - // this asserts the runtime contract too.) - const updated = await store.update(ws.id, { - isPersonal: false, - ownerUserId: "user_evil", - } as unknown as { name: string }); - expect(updated?.isPersonal).toBe(true); - expect(updated?.ownerUserId).toBe("user_alice"); + // Cast to bypass the Pick<> at the type level — runtime must throw + // loudly instead of silently stripping the disallowed keys. The + // silent-strip behavior is what produced multi-admin personal + // workspaces on hq production; Stage 1.1 replaces it with a typed + // error. Exhaustive invariant coverage lives in + // `personal-workspace-invariants.test.ts`. + await expect( + store.update(ws.id, { + isPersonal: false, + ownerUserId: "user_evil", + } as unknown as { name: string }), + ).rejects.toThrow(/personal-workspace invariant/i); }); }); From 065a56787115beacba65776bbafa4505bc98cb90 Mon Sep 17 00:00:00 2001 From: Mathew Goldsborough <1759329+mgoldsborough@users.noreply.github.com> Date: Thu, 21 May 2026 16:05:17 -1000 Subject: [PATCH 2/3] =?UTF-8?q?fix(workspace):=20round-1=20QA=20=E2=80=94?= =?UTF-8?q?=20provisioning=20self-heal=20+=20OSS=20hygiene=20+=20doc=20dri?= =?UTF-8?q?ft?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the three Critical findings on PR #266's QA review. **Provisioning regression (C2).** The original PR replaced `ensureUserWorkspace`'s self-healing recreate with a throw — under a concurrent create-then-delete race, `reconcileConflict` would surface "workspace disappeared between create-conflict and re-read" and bubble out of every auth provider (oidc/workos/dev), producing a 500 on login. The "callers retry" doc claim wasn't true. Folded `reconcileConflict` back into `ensureUserWorkspace` as a bounded retry loop (3 attempts). Body is shorter than the prior code AND covers the race the original self-healed against. The loop shapes: attempt 1: read → null → create → conflict from another caller attempt 2: read → null (third caller deleted) → create succeeds attempt 3: only reached under pathological create+delete churn Two new unit tests pin the race in `provisioning.test.ts` using a stub store with controlled get/create call counts. Pathological case surfaces a diagnosable error message naming the attempt count. **OSS hygiene (C1).** The repo is public OSS; internal tenant context and seed-user names that named specific people don't belong in commit history. Six sites neutralized: - src/workspace/workspace-store.ts:268 — "on hq production" → "in production" - test/unit/workspace/workspace-store.test.ts:360 — same - scripts/cleanup-personal-workspace-members.ts:8 — "the hq production tenant" → "a production tenant" - test/integration/scripts/cleanup-personal-workspace-members.test.ts — "user_mat"/"user_mario" → "user_b"/"user_c"; drop "hq-production" comment - AGENTS.md:153 — "seen on hq production" → "observed in production" - CHANGELOG.md:90 — drop "(observed on hq production)" parenthetical **Documentation drift (C3).** AGENTS.md previously documented `make cleanup-personal-workspace-members CLIENT= ENV=` as a real operator entry point, but this PR doesn't ship that Makefile addition (the deployments submodule has unrelated in-progress work). Operators reading the docs would reach for a target that doesn't exist. Replaced with the real entry point: `bun run cleanup:personal- workspace-members`. PR body refreshed to match. Verify: 2993 unit / 0 fail (+2 race tests), 258 web / 0, 551 integration / 0 (12 skip), 17 smoke / 0. All static checks green. --- AGENTS.md | 2 +- CHANGELOG.md | 2 +- scripts/cleanup-personal-workspace-members.ts | 2 +- src/workspace/provisioning.ts | 56 +++++++---------- src/workspace/workspace-store.ts | 2 +- ...cleanup-personal-workspace-members.test.ts | 12 ++-- test/unit/workspace/provisioning.test.ts | 62 +++++++++++++++++++ test/unit/workspace/workspace-store.test.ts | 2 +- 8 files changed, 97 insertions(+), 43 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 66f95bf9..6fc08afa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -150,7 +150,7 @@ What stays freely mutable on a personal workspace: `bundles`, `name`, `about`, ` The HTTP layer maps `PersonalWorkspaceInvariantError` to `422 personal_workspace_invariant` with `{ workspaceId, reason }` details (same shape as `ConversationCorruptedError → 422`). The workspace-mgmt tool handlers encode the error into `structuredContent` so it survives the in-process MCP serialization boundary; `handleToolCall` decodes and emits the 422. -Pre-Stage-1.1 data (multi-admin personal workspaces seen on hq production) is repaired retroactively by `scripts/cleanup-personal-workspace-members.ts` (alias: `bun run cleanup:personal-workspace-members`, or `make cleanup-personal-workspace-members CLIENT= ENV=` for in-cluster runs). Idempotent; dry-run by default, `--apply` to write. A personal workspace missing `ownerUserId` is a hard-error — operator must triage. +Pre-Stage-1.1 data (multi-admin personal workspaces observed in production) is repaired retroactively by `scripts/cleanup-personal-workspace-members.ts` (alias: `bun run cleanup:personal-workspace-members`). Idempotent; dry-run by default, `--apply` to write. A personal workspace missing `ownerUserId` is a hard-error — operator must triage. ## Debug Logging diff --git a/CHANGELOG.md b/CHANGELOG.md index b5f29129..2148f76d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -87,7 +87,7 @@ - **Removed `manage_conversation` actions:** `shareConversation`, `unshareConversation`, `addParticipant`, `removeParticipant` are gone. Single-owner semantics; sharing returns in a future stage with policy gates. External callers that previously invoked these actions get an `unknown action` error. - **`Conversation.visibility` and `Conversation.participants` removed from the schema.** Reads of pre-migration files that still carry these fields skip them at parse time; writes never produce them. `ownerId` is now required on every conversation file — pre-migration files without one fail to load with a clear "run the migration" hint. - **`/v1/conversations/:id/events` no longer requires `X-Workspace-Id`.** The header is still honored when sent (validated for format + membership); absent → 200, present + malformed → 400, present + non-member → 403. Foreign-owner conversation → 403 `conversation_access_denied`; non-existent → 404 `not_found`. Web clients that already send the header keep working unchanged. -- **Personal workspaces enforce sole-owner-admin membership and freeze `isPersonal` / `ownerUserId` post-create.** `WorkspaceStore.update` / `addMember` / `removeMember` / `updateMemberRole` throw `PersonalWorkspaceInvariantError` on any member or identity-field mutation against a personal workspace; `WorkspaceStore.create` rejects a personal workspace whose initial members aren't exactly `[{ userId: ownerUserId, role: "admin" }]`. Operators with pre-existing multi-admin personal workspaces (observed on hq production) must run `bun run cleanup:personal-workspace-members --apply` to converge — `bundles`, `name`, `about`, `customInstructions` remain freely mutable. +- **Personal workspaces enforce sole-owner-admin membership and freeze `isPersonal` / `ownerUserId` post-create.** `WorkspaceStore.update` / `addMember` / `removeMember` / `updateMemberRole` throw `PersonalWorkspaceInvariantError` on any member or identity-field mutation against a personal workspace; `WorkspaceStore.create` rejects a personal workspace whose initial members aren't exactly `[{ userId: ownerUserId, role: "admin" }]`. Operators with pre-existing multi-admin personal workspaces must run `bun run cleanup:personal-workspace-members --apply` to converge — `bundles`, `name`, `about`, `customInstructions` remain freely mutable. ### Fixed diff --git a/scripts/cleanup-personal-workspace-members.ts b/scripts/cleanup-personal-workspace-members.ts index 954b85bf..b4097007 100644 --- a/scripts/cleanup-personal-workspace-members.ts +++ b/scripts/cleanup-personal-workspace-members.ts @@ -5,7 +5,7 @@ * * Stage 1 introduced personal workspaces (`isPersonal: true`, * `ownerUserId: `) but didn't enforce that their `members` array - * matched the canonical sole-owner shape. The hq production tenant + * matched the canonical sole-owner shape. A production tenant * surfaced a personal workspace with three admins, which Stage 1.1 * now disallows at the store layer. Operators with pre-existing data * shaped by the looser invariant run this script once to converge. diff --git a/src/workspace/provisioning.ts b/src/workspace/provisioning.ts index 2d514d36..3abc7464 100644 --- a/src/workspace/provisioning.ts +++ b/src/workspace/provisioning.ts @@ -51,44 +51,36 @@ export async function ensureUserWorkspace( identity: ProvisioningIdentity, ): Promise { const wsId = personalWorkspaceIdFor(identity.id); - - const existing = await store.get(wsId); - if (existing) return existing; - const name = identity.displayName ? `${identity.displayName}'s Workspace` : "Workspace"; const slug = personalWorkspaceSlugFor(identity.id); - try { - return await store.create(name, slug, { - isPersonal: true, - ownerUserId: identity.id, - }); - } catch (err) { - if (err instanceof WorkspaceConflictError) { - return reconcileConflict(store, wsId); + // Self-healing read-then-create loop. The body covers three race + // shapes around the canonical id: (a) another caller already created + // it (read wins); (b) we lose a create-conflict and the workspace + // exists by the time we re-read (loop returns it); (c) we lose a + // create-conflict but the workspace was deleted before re-read (loop + // recreates). 3 attempts is plenty — (c) twice in a row would + // require pathological concurrent create+delete churn on one user. + const MAX_ATTEMPTS = 3; + for (let attempt = 0; attempt < MAX_ATTEMPTS; attempt++) { + const existing = await store.get(wsId); + if (existing) return existing; + try { + return await store.create(name, slug, { + isPersonal: true, + ownerUserId: identity.id, + }); + } catch (err) { + if (err instanceof WorkspaceConflictError) continue; + // `PersonalWorkspaceInvariantError` here would mean this helper + // built a bad-shape personal workspace — a bug, not a race. + // Anything else: surface unchanged. + if (err instanceof PersonalWorkspaceInvariantError) throw err; + throw err; } - // A `PersonalWorkspaceInvariantError` from create() here would mean - // the helper itself produced a bad-shape personal workspace — bug, - // let it surface. - if (err instanceof PersonalWorkspaceInvariantError) throw err; - throw err; } -} -/** - * A `create()` collision on the canonical personal-workspace id means - * another concurrent call won the race. Re-read and return. Never create - * a second workspace with a different slug — two personal workspaces per - * user is exactly the bug the canonical-id model exists to prevent. - */ -async function reconcileConflict(store: WorkspaceStore, wsId: string): Promise { - const existing = await store.get(wsId); - if (existing) return existing; - // WorkspaceConflictError fires only when store.get() returned non-null - // inside create() — so reaching here means the workspace existed at - // throw time and was deleted before our re-read (concurrent delete, - // rare). Surface the inconsistency; callers retry. throw new Error( - `[provisioning] personal workspace ${wsId} disappeared between create-conflict and re-read`, + `[provisioning] personal workspace ${wsId} couldn't be reconciled after ${MAX_ATTEMPTS} attempts — investigate concurrent create/delete activity`, ); } diff --git a/src/workspace/workspace-store.ts b/src/workspace/workspace-store.ts index 75c8d5bb..3e9f700c 100644 --- a/src/workspace/workspace-store.ts +++ b/src/workspace/workspace-store.ts @@ -265,7 +265,7 @@ export class WorkspaceStore { // and historic callers did exactly that. Detect the attempt and // throw a typed error instead of silently stripping — the silent // strip is the failure mode that produced multi-admin personal - // workspaces on hq production. + // workspaces in production. // // Casts here are scoped to read-only inspection of the widened patch // shape. We do NOT widen the spread that builds `updated` — only diff --git a/test/integration/scripts/cleanup-personal-workspace-members.test.ts b/test/integration/scripts/cleanup-personal-workspace-members.test.ts index 3153912c..2dac98a6 100644 --- a/test/integration/scripts/cleanup-personal-workspace-members.test.ts +++ b/test/integration/scripts/cleanup-personal-workspace-members.test.ts @@ -82,16 +82,16 @@ async function runCleanup(args: string[] = []): Promise<{ exitCode: number; stde describe("cleanup-personal-workspace-members", () => { test("multi-admin personal workspace gets cleaned to sole-owner-admin", async () => { - // The exact failure mode that motivated the script: an - // hq-production personal workspace with three admins. + // The exact failure mode that motivated the script: a personal + // workspace with three admins. await seedWorkspace({ id: "ws_user_user_alice", isPersonal: true, ownerUserId: "user_alice", members: [ { userId: "user_alice", role: "admin" }, - { userId: "user_mat", role: "admin" }, - { userId: "user_mario", role: "admin" }, + { userId: "user_b", role: "admin" }, + { userId: "user_c", role: "admin" }, ], }); @@ -100,8 +100,8 @@ describe("cleanup-personal-workspace-members", () => { expect(stderr).toContain("ws_user_user_alice"); // The log should name the dropped members so the operator can // verify what was removed. - expect(stderr).toContain("user_mat"); - expect(stderr).toContain("user_mario"); + expect(stderr).toContain("user_b"); + expect(stderr).toContain("user_c"); const ws = await readWorkspace("ws_user_user_alice"); expect(ws.members).toEqual([{ userId: "user_alice", role: "admin" }]); diff --git a/test/unit/workspace/provisioning.test.ts b/test/unit/workspace/provisioning.test.ts index 897b6217..ab5ed2c5 100644 --- a/test/unit/workspace/provisioning.test.ts +++ b/test/unit/workspace/provisioning.test.ts @@ -3,8 +3,10 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { ensureUserWorkspace } from "../../../src/workspace/provisioning.ts"; +import type { Workspace } from "../../../src/workspace/types.ts"; import { personalWorkspaceIdFor, + WorkspaceConflictError, WorkspaceStore, } from "../../../src/workspace/workspace-store.ts"; @@ -81,6 +83,66 @@ describe("ensureUserWorkspace", () => { expect(list[0]!.isPersonal).toBe(true); }); + test("self-heals when canonical is deleted between create-conflict and re-read", async () => { + // Race shape (pinning what an earlier version of reconcileConflict + // would have 500'd on): caller A's `store.get` returns null, A's + // `store.create` loses the race to caller B → throws + // WorkspaceConflictError, but C deletes the workspace before A + // re-reads. The bounded retry loop must recreate, not throw. + let getCallNo = 0; + let createCallNo = 0; + const recreated: Workspace = { + id: personalWorkspaceIdFor("user_alice"), + name: "Alice's Workspace", + members: [{ userId: "user_alice", role: "admin" }], + bundles: [], + isPersonal: true, + ownerUserId: "user_alice", + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }; + const stub = { + async get(_id: string): Promise { + getCallNo++; + return null; // both reads see the deleted state + }, + async create(_name: string, _slug: string): Promise { + createCallNo++; + if (createCallNo === 1) { + throw new WorkspaceConflictError(personalWorkspaceIdFor("user_alice")); + } + return recreated; + }, + } as unknown as WorkspaceStore; + + const result = await ensureUserWorkspace(stub, { + id: "user_alice", + displayName: "Alice", + }); + + expect(result).toBe(recreated); + expect(getCallNo).toBe(2); // initial read + post-conflict re-read + expect(createCallNo).toBe(2); // first lost, second won + }); + + test("gives up after 3 attempts under pathological create/delete churn", async () => { + // If every attempt loses to a concurrent creator AND every + // re-read sees the workspace already deleted again, surface a + // diagnosable error instead of looping forever. + const stub = { + async get(): Promise { + return null; + }, + async create(): Promise { + throw new WorkspaceConflictError(personalWorkspaceIdFor("user_alice")); + }, + } as unknown as WorkspaceStore; + + await expect( + ensureUserWorkspace(stub, { id: "user_alice", displayName: "Alice" }), + ).rejects.toThrow(/couldn't be reconciled after 3 attempts/); + }); + test("different users get different personal workspaces", async () => { const a = await ensureUserWorkspace(store, { id: "user_alice", displayName: "Alice" }); const b = await ensureUserWorkspace(store, { id: "user_bob", displayName: "Bob" }); diff --git a/test/unit/workspace/workspace-store.test.ts b/test/unit/workspace/workspace-store.test.ts index f003c58b..b48d8968 100644 --- a/test/unit/workspace/workspace-store.test.ts +++ b/test/unit/workspace/workspace-store.test.ts @@ -357,7 +357,7 @@ describe("WorkspaceStore.update", () => { // Cast to bypass the Pick<> at the type level — runtime must throw // loudly instead of silently stripping the disallowed keys. The // silent-strip behavior is what produced multi-admin personal - // workspaces on hq production; Stage 1.1 replaces it with a typed + // workspaces in production; Stage 1.1 replaces it with a typed // error. Exhaustive invariant coverage lives in // `personal-workspace-invariants.test.ts`. await expect( From af38c43b0f016ebeaf1592535854433a54705d2a Mon Sep 17 00:00:00 2001 From: Mathew Goldsborough <1759329+mgoldsborough@users.noreply.github.com> Date: Fri, 22 May 2026 06:28:30 -1000 Subject: [PATCH 3/3] fix(changelog): drop non-existent make cleanup target reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 QA flagged the same drift in AGENTS.md, fixed there but missed the parallel site in CHANGELOG. Symmetric-defense miss on my part — the new AGENTS.md convention says to find parallel sites when fixing a check, and I didn't grep for sibling `make cleanup-personal-workspace-members` references before pushing round-1. This is the operator-facing surface for "you must run this after deploying", so a wrong command here is materially worse than in AGENTS.md. Confirmed no other stray make-target references remain in the diff via `grep -nE 'make (cleanup|heal|migrate)-...'`. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2148f76d..baa8208c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,7 +43,7 @@ - Org-admin gate on `set_model_config` — backend now refuses non-org-admin writes (was UI-only via RouteGuard). Distinguishes "no identity" (cron, automations) from "wrong role" so debug logs make non-user code paths obvious. - HTTP proxy primitive (`_meta["ai.nimblebrain/http-proxy"]`). Bundles can expose a loopback HTTP server (e.g. `astro preview`, Jupyter kernel) through the platform at `/v1/ws//apps///*`. Loopback-only target, credentials and `Accept-Encoding` stripped on forward, `Set-Cookie`/CSP/X-Frame-Options stripped on response, per-workspace kill switch via `Workspace.allowHttpProxy`. Bundles get `NB_WORKSPACE_ID`, `NB_PROXY_PREFIX`, `NB_PUBLIC_ORIGIN` in their env at spawn ([docs](https://docs.nimblebrain.ai/apps/http-proxy/)). - `PersonalWorkspaceInvariantError` typed error (`src/workspace/errors.ts`) → HTTP 422 `personal_workspace_invariant` with structured `{ workspaceId, reason }` body. Mirrors the `ConversationCorruptedError` → 422 precedent; raised by `WorkspaceStore` on attempts to mutate the locked members / `isPersonal` / `ownerUserId` fields on personal workspaces. -- `scripts/cleanup-personal-workspace-members.ts` (also `bun run cleanup:personal-workspace-members` and `make cleanup-personal-workspace-members`) — one-off retroactive cleanup that converges pre-Stage-1.1 personal workspaces to the sole-owner-admin shape. Idempotent, dry-run by default. +- `scripts/cleanup-personal-workspace-members.ts` (alias: `bun run cleanup:personal-workspace-members`) — one-off retroactive cleanup that converges pre-Stage-1.1 personal workspaces to the sole-owner-admin shape. Idempotent, dry-run by default. ### Changed