feat(workspace): enforce personal-workspace immutability invariants#266
Merged
Conversation
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.
…oc drift 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=<x> ENV=<y>` 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.
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)-...'`.
…04636eac9f1 # Conflicts: # AGENTS.md # package.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Personal workspaces (
isPersonal === true) become immutable from a user/permission standpoint. Motivated by a production observation: one user's personal workspace had three admins. Stage 1 introduced personal workspaces but didn't enforce the invariants that follow from "personal" — Stage 1.1 makes the store the source of truth.Four invariants enforced by
WorkspaceStore[{ userId: ownerUserId, role: "admin" }].addMember/removeMember/updateMemberRoleandupdate({ members })patches all reject mutations against personal workspaces.isPersonalfrozen post-create, both directions.ownerUserIdfrozen on personal workspaces.ownerUserIdforbidden on non-personal workspaces (the two fields travel together).bundles,name,about,customInstructionsstay freely mutable — these are workspace-content edits, not identity edits.Typed error → HTTP 422
PersonalWorkspaceInvariantError(new,src/workspace/errors.ts) carries{ workspaceId, reason }wherereason ∈ { members_mutation, is_personal_frozen, owner_user_id_frozen, owner_user_id_on_non_personal }. The HTTP handler maps it to422 personal_workspace_invariantwith the same structured body — mirrors theConversationCorruptedError → 422precedent from Stage 1.The typed class doesn't survive the in-process MCP serialization boundary, so the workspace-mgmt tool handlers encode it into
structuredContentandhandleToolCallre-detects → 422. External MCP clients see the sameisError: trueresult with the structured payload.Retroactive cleanup
scripts/cleanup-personal-workspace-members.ts(alias:bun run cleanup:personal-workspace-members) repairs pre-Stage-1.1 data. Idempotent; dry-run by default,--applyto write. A personal workspace missingownerUserIdis a hard-error — operator must triage.Operators must run this after deploying to converge any multi-admin personal workspaces.
A wrapping Makefile target was scoped for
deployments/nimblebrain/Makefilebut isn't part of this PR — that submodule has unrelated in-progress work. Will land as a separate submodule commit.Test plan
test/unit/workspace/personal-workspace-invariants.test.ts.ensureUserWorkspacerecreates when canonical is deleted between create-conflict and re-read; gives up after 3 attempts under pathological churn).test/unit/workspace/provisioning.test.ts./v1/tools/call manage_workspacesreturns 422 with structured body onadd_memberandupdate_memberagainst a personal workspace; non-identity updates still succeed.test/integration/api/personal-workspace-invariants.test.ts.ownerUserId, untouched on non-personal, dry-run is default.test/integration/scripts/cleanup-personal-workspace-members.test.ts.bun run verify: static green, 2993 unit / 0 fail, 258 web / 0, 551 integration / 0 (12 skip), 17 smoke / 0.Changes since initial PR
Round 1 of QA review landed on this branch. Three Critical findings addressed:
ensureUserWorkspace's self-healing recreate path with a throw, which would have 500'd login under a concurrent create-delete race. FoldreconcileConflictback intoensureUserWorkspaceas a bounded retry loop (3 attempts) — shorter code than the original AND restores the self-heal. Two new unit tests pin the race shape.make cleanup-personal-workspace-memberstarget that this PR doesn't deliver. Replaced with the real entry point (bun run cleanup:personal-workspace-members).