Skip to content

feat(workspace): add member and deletion management - #147

Merged
yetone merged 3 commits into
yetone:mainfrom
bingqilinweimaotai:feat/workspace-management
Sep 3, 2026
Merged

feat(workspace): add member and deletion management#147
yetone merged 3 commits into
yetone:mainfrom
bingqilinweimaotai:feat/workspace-management

Conversation

@bingqilinweimaotai

Copy link
Copy Markdown
Collaborator

Summary

  • add workspace member management APIs and settings UI
  • allow owners to change member/admin roles
  • allow owners and admins to remove members within role boundaries
  • add owner-only workspace deletion with exact-name confirmation
  • revoke removed members' tenant and conversation access atomically
  • restore participant state when a removed member accepts a new invitation
  • broadcast membership changes so affected clients refresh immediately
  • reset tenant-scoped client stores when workspace access changes
  • add an empty-workspace recovery screen
  • add English and Simplified Chinese translations

Permissions

  • owners can change members between member and admin
  • admins can remove regular members, but cannot manage admins or owners
  • only owners can invite another admin
  • workspace ownership cannot be changed or removed through these endpoints
  • only owners can delete a workspace
  • deleting the user's only workspace is currently blocked

Workspace deletion

Workspace deletion runs tenant database cleanup in a transaction, covering both
FK-backed records and legacy soft-scoped tables. Referenced storage objects and
agent runtime resources are cleaned up after the transaction commits.

Testing

  • npm run build
  • npm run typecheck
  • npm run server:typecheck
  • npm run lint
  • node --import tsx --test server/src/__tests__/db-schema-sentinels.test.ts

Added PostgreSQL integration coverage for:

  • member listing and role changes
  • admin permission boundaries
  • atomic member removal
  • re-inviting removed members
  • only-workspace deletion protection
  • workspace tenant-data cleanup

@yetone

yetone commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Reviewed this in full even though it is still a draft, so the feedback is waiting for you rather than the other way round. Not merging yet: it is a draft, and there are two data-integrity problems below that need fixing first. The authorization design itself is sound — more on that at the end.

Blocking

1. The user_preferences / agent_autonomy deletes are not actually tenant-scoped, and will wipe global user state for legacy rows.
user_preferences is keyed by user_id alone (migrate.ts:118), agent_autonomy by (user_id, agent_id). The runtime writes prefs by user_id and never sets company_id (router.ts INSERT INTO user_preferences (user_id, prefs, updated_at)), so company_id on those rows is either NULL or the 'personal' backfill from migrate.ts:554. They are per-user global state that happens to carry a vestigial column.
Failure: a legacy user is removed from the 'personal' workspace, or its owner deletes it → DELETE FROM user_preferences WHERE company_id = $1 AND user_id = $2 (removal) and the softScopedTables sweep (deletion) erase that user's — or on deletion, every legacy user's — preferences for all workspaces. Drop these two tables from both paths until they are genuinely tenant-keyed.

2. Post-commit storage and runtime cleanup is fire-and-forget with nothing to retry from.
void cleanupDeletedWorkspaceResources(...) after COMMIT, failures only console.warn. If S3 blips or the pod restarts mid-cleanup, the objects and agent pods/PVCs are orphaned forever — the rows that listed the keys and agent ids are already gone, so there is nothing left to retry against. Persist the pending keys and agent ids before COMMIT (a workspace_cleanup row is enough) and drain them from a retried job. Also: if (/test/i.test(env.DATABASE_URL)) return silently skips pod/PVC deletion on any production database whose URL happens to contain test (latest-db, contest…). Gate on an explicit env flag, not the URL.

3. The integration tests do not cover the permission matrix the description claims.
workspace-management.test.ts has two negative cases (admin → admin PATCH/DELETE). Missing: a plain member hitting each of the three endpoints (403); an admin successfully removing a member; admin blocked from inviting an admin; the owner as target of PATCH/DELETE; self-removal 409; a non-owner calling DELETE /companies/:id; wrong or absent confirmation; and cross-tenant — an admin of A calling /companies/B/.... The boundary is the whole value of this PR; each of those is one short test.

Should fix, not blocking

  • Orphaned DMs. Removal strips the user from every conversation, leaving kind='direct' rooms with one member. On re-invite a new DM is created because dedup requires exactly two members (private_chat.ts:29), so the remaining member keeps a dead DM they cannot leave ("cannot leave a direct conversation") plus a fresh one. Exclude direct rooms from the strip, or mark them.
  • The storage-key re-check runs attachment::text LIKE '%'||key||'%' over all of messages, once per key — a cross-tenant full scan × N after commit. One query with ANY($1) or the server-side key helper.
  • Deleting llm_calls / llm_calls_rollup erases the cost ledger CONTRIBUTING.md treats as a correctness invariant. If that is intended, say so in a comment; otherwise keep them and null the tenant.
  • GET /companies/:id/members reuses requireCompanyAdmin, whose 403 text says "manage invitations".
  • Frontend: the owner gets authMe() + ws.reconnect() twice on delete (once from the modal, once from WorkspaceSessionBridge, since the owner is in recipientUserIds); a pure role change also forces a socket reconnect it does not need; the members list does not refresh on role_changed from another admin.
  • The import reordering in router.ts / ws.ts / App.tsx inflates the diff for no behavioural reason; please revert it so the review is one logical change.

What checks out

Role checks are right on every endpoint: PATCH owner-only with the owner immutable; DELETE owner/admin with admin → member only, owner unremovable, self-removal 409; admin cannot invite admin; workspace delete is owner-only with an owner_user_id cross-check and the exact-name confirmation enforced server-side; every query is company_id-scoped; FOR UPDATE serialises deletion against removal. Deletion coverage is complete against the schema — everything tenant-scoped is hit explicitly or cascades, audit_events is rightly kept, and no FK ordering bites. The WS branch is additive: recipients already resolve against live membership, so a removed user's open socket can neither read nor write after commit. Re-invite correctly clears departed_at and restores availability. All 21 i18n keys are in both locales; every button has a type.

Not verified: in-flight daemon runs holding a runtime token for a deleted workspace, and Redis-held state (status leases, kanban timers, calendar dispatcher) keyed by the deleted company. Worth a paragraph in the description on what you expect there.

Fix 1–3, mark it ready, and ping me.

@bingqilinweimaotai
bingqilinweimaotai marked this pull request as ready for review September 3, 2026 09:22
Resolves the import-list conflict in src/api/client.ts with yetone#175
(DetectedEngine joins the sorted type import).

Claude-Session: https://claude.ai/code/session_01SevbW9qCBbzrjfLMy14A31
@yetone

yetone commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Re-checked the fix commit against both blocking findings before merging:

  1. Global user stateuser_preferences / agent_autonomy are out of both the removal path and the softScopedTables sweep (and llm_calls* stays too). The integration tests now seed 'personal'-scoped rows and assert they survive removal and deletion. ✔
  2. Cleanup durabilityenqueueWorkspaceCleanup runs inside the deletion transaction ahead of DELETE FROM companies, in a table with no company FK; the worker claims with FOR UPDATE SKIP LOCKED, guards completion on locked_by, backs off exponentially, and is started/stopped from index.ts. The DATABASE_URL =~ /test/ heuristic is gone in favour of WORKSPACE_RUNTIME_CLEANUP_ENABLED. ✔

Two things worth a follow-up, neither blocking:

  • The 60s lease is never renewed while storage keys are deleted one by one, so a large workspace can outlive it and get re-claimed by a second replica. Work is idempotent, so the cost is duplicated deletes and an undercounted attempts, not lost data — but a locked_until bump per batch would close it.
  • When WORKSPACE_RUNTIME_CLEANUP_ENABLED is off, the job is marked complete with the pods/PVCs untouched, so enabling the flag later never retro-cleans. Note for operators: the production Deployment is patched by image only, so the "true" you added to the manifests does not reach it until someone applies it. I have called that out in the release notes.

Merging; ships in v0.14.0.

@yetone
yetone merged commit ddbbfed into yetone:main Sep 3, 2026
7 checks passed
@yetone yetone mentioned this pull request Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants