feat(admin): move a workspace between organizations - #7243
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@greptile review @cursor review Reviewers: the two areas most worth your attention are the lock ordering in |
Greptile SummaryThe PR enables organization-to-organization workspace moves and adds transactional cleanup, concurrency fencing, preflight impact reporting, and source-organization auditing. The transactional entitlement recheck remains broader than the Enterprise gate it is intended to enforce.
Confidence Score: 4/5The PR is not yet safe to merge because an Enterprise-source workspace can still be moved into a Team-plan destination that lacks the required Enterprise entitlement. The prior reply reports the entitlement fence as fixed, but the replacement uses isOrgPlan, so a Team subscription passes the transactional check while the capability-facing Enterprise resolver rejects the same destination. Files Needing Attention: apps/sim/lib/workspaces/admin-move.ts
|
| Filename | Overview |
|---|---|
| apps/sim/lib/workspaces/admin-move.ts | Adds the organization-to-organization move orchestration, locking, blockers, cleanup, durable replay, and audits; its transactional entitlement fence incorrectly treats Team plans as satisfying the Enterprise gate. |
| apps/sim/lib/workspaces/admin-move-source-impact.ts | Collects bounded preflight impact details and performs transaction-aware cleanup of source-organization artifacts. |
| apps/sim/ee/workspace-forking/lib/create-fork.ts | Revalidates the fork parent organization while holding a conflicting row lock, closing the previously reported move race. |
| apps/sim/lib/workflows/custom-blocks/operations.ts | Serializes custom-block validation and publication with workspace moves through the organization mutation lock. |
| apps/sim/lib/billing/core/subscription.ts | Exposes whether organization entitlement is subscription-backed so transactional callers can avoid rejecting deployment-configured entitlement modes. |
| apps/sim/lib/api/contracts/v1/admin/dashboard-workspaces.ts | Expands the admin move contract with source impact, credentials, entitlement, blocker, notice, and truncation details. |
Sequence Diagram
sequenceDiagram
participant Admin
participant Move as Workspace move
participant Source as Source organization
participant Destination as Destination organization
participant Workspace
Admin->>Move: Confirm organization transfer
Move->>Source: Acquire organization lock
Move->>Destination: Acquire organization lock
Move->>Workspace: Lock row and verify source
Move->>Move: Recheck forks, invitations, entitlements
alt blocker found
Move-->>Admin: Refuse move
else checks pass
Move->>Source: Remove source-owned artifacts
Move->>Workspace: Change organization and payer
Move-->>Admin: Return applied summary
end
Reviews (10): Last reviewed commit: "feat(admin): move a workspace between or..." | Re-trigger Greptile
There was a problem hiding this comment.
8 issues found across 6 files
Confidence score: 2/5
apps/sim/lib/workspaces/admin-move.tsdoes not consistently serialize fork creation and custom-block publication with organization mutation locks, allowing concurrent writers to commit after the scans and violate the cross-organization invariant—acquire the same locks before inserting.resolveMoveEntitlementsandgetCustomBlockUsageCountsuse the global pool instead of the move transaction executor, while personal-workspace fork edges can be skipped whensourceOrganizationIdis null; thread the executor through these calls and check fork edges for every move.apps/sim/lib/api/contracts/v1/admin/dashboard-workspaces.tscan return an unbounded list when multiple collection caps are exceeded, causingrequestJsonclients to reject the preflight response—apply explicit bounds consistently.- Move review data can be incomplete or inaccurate:
getWorkspaceMoveOperationloses source context after completion,findRetainedCollaboratorCapsomits external collaborators, andsourceOrgElsewhereUsage.liveplus the LIKE-only block query can overcount usage inapps/sim/lib/workspaces/admin-move-source-impact.ts; persist or reconstruct context and use the exact type/placement predicates when calculating impact.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/api/contracts/v1/admin/dashboard-workspaces.ts">
<violation number="1" location="apps/sim/lib/api/contracts/v1/admin/dashboard-workspaces.ts:82">
P2: When a workspace has more than one of these collection caps, the preflight returns the complete unbounded list and the contract rejects the response in clients using `requestJson`. Enforce the same bounds with explicit overflow handling, or remove the caps so the contract can represent every returned row.</violation>
</file>
<file name="apps/sim/lib/workspaces/admin-move.ts">
<violation number="1" location="apps/sim/lib/workspaces/admin-move.ts:959">
P1: When a personal workspace has a fork edge, preflight reports a blocker but the move transaction skips the fork check because `sourceOrganizationId` is null. Check fork edges for every move, not only organization-owned sources, before mutating the workspace.</violation>
<violation number="2" location="apps/sim/lib/workspaces/admin-move.ts:961">
P1: Thread the transaction executor through `resolveMoveEntitlements` and `getCustomBlockUsageCounts`; these calls currently use the global pool inside the move transaction, triggering the tripwire and escaping the transaction's consistency boundary.</violation>
<violation number="3" location="apps/sim/lib/workspaces/admin-move.ts:962">
P0: Make fork creation and custom-block publication acquire the same organization mutation locks before inserting. Otherwise either writer can commit after these scans and leave a cross-organization invariant.</violation>
<violation number="4" location="apps/sim/lib/workspaces/admin-move.ts:2349">
P2: When an already-completed move is reloaded through `getWorkspaceMoveOperation`, the response loses the source organization and impact details added by this change. Persist or reconstruct the applied move context before building the operation summary.</violation>
</file>
<file name="apps/sim/lib/workspaces/admin-move-source-impact.ts">
<violation number="1" location="apps/sim/lib/workspaces/admin-move-source-impact.ts:192">
P2: When a custom block is present only in the moving workspace's active deployment, `sourceOrgElsewhereUsage.live` reports it as source-org collateral. Subtract a moving-workspace union count, or calculate source-org live usage with the same live-only scope as the displayed field.</violation>
<violation number="2" location="apps/sim/lib/workspaces/admin-move-source-impact.ts:228">
P3: When a deployment state contains the block slug outside a block's `type`, this LIKE-only query counts a false placement. Add the same `jsonb_each(... -> 'blocks')` exact-type predicate used by `getCustomBlockUsageCounts`.</violation>
<violation number="3" location="apps/sim/lib/workspaces/admin-move-source-impact.ts:377">
P2: When an external collaborator has a source-organization usage cap, this inner join excludes them from `findRetainedCollaboratorCaps`, so the move review fails to disclose the cap that will stop applying. Use an outer join or remove the membership join; the limit itself already identifies the source organization.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
867f481 to
2fa8a9e
Compare
2fa8a9e to
7ceebd1
Compare
7ceebd1 to
b9007ab
Compare
b9007ab to
42dd04d
Compare
|
@cubic review |
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
7 issues found across 7 files
Confidence score: 2/5
apps/sim/lib/workspaces/admin-move.tsdoes not coordinate the personal-workspace move with a concurrent fork when the source organization is null, so the move can commit before the fork insert is observed and violate the intended concurrency guarantee — use a shared lock or equivalent transactional coordination.- In
apps/sim/lib/workspaces/admin-move.ts, entitlement state can become stale between the initial read and lock acquisition, allowing a destination that lost Enterprise status to bypass the downgrade blocker — re-resolve entitlements after acquiring the lock. apps/sim/lib/workspaces/admin-move.tscan commit the move beforerecordSourceOrganizationMoveAuditfinishes, leaving a durable retry unable to restore the source-loss audit after a process failure — record the audit transactionally with the move or make retry recovery complete.- The remaining correctness and scalability gaps need follow-up:
admin-move-source-impact.tscan issue four unbounded concurrent queries per block,dashboard-workspaces.tsrejects preflight responses with more than 500 fork edges, and the durable-reload parser dropssourceOrganizationId; cap/batch the queries, bound the response, preserve the field, and add the missingSourceOrganizationChangedErrorretry test inadmin-move.test.ts.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/workspaces/admin-move-source-impact.ts">
<violation number="1" location="apps/sim/lib/workspaces/admin-move-source-impact.ts:222">
P2: When a workspace has many source custom blocks, this launches four database queries per block concurrently because the unbounded result is passed to `Promise.all`. Batch the usage counts or cap/enrich rows safely before this fan-out so an admin preflight cannot exhaust the database pool.</violation>
</file>
<file name="apps/sim/lib/api/contracts/v1/admin/dashboard-workspaces.ts">
<violation number="1" location="apps/sim/lib/api/contracts/v1/admin/dashboard-workspaces.ts:94">
P2: When a workspace has more than 500 fork edges, the preflight response fails contract validation because the move code returns all edges but this field enforces a 500-item maximum. Bound or truncate the fork-edge result with an omission indicator before returning it, or remove this limit from the contract.</violation>
</file>
<file name="apps/sim/lib/workspaces/admin-move.ts">
<violation number="1" location="apps/sim/lib/workspaces/admin-move.ts:367">
P2: After a durable move reload, the parser drops the newly persisted `sourceOrganizationId`, so the operation view reports that the source was not persisted and omits the source organization. Preserve `sourceOrganizationId` in the parsed audit object.</violation>
<violation number="2" location="apps/sim/lib/workspaces/admin-move.ts:1035">
P1: When moving a personal workspace while a fork is created concurrently, this check can run before the fork insert because neither path shares a lock for a null source organization. The move can then commit with the parent in the destination while the new child remains personal, violating the parent/child organization invariant; serialize the fork edge or workspace mutation and revalidate before commit.</violation>
<violation number="3" location="apps/sim/lib/workspaces/admin-move.ts:1059">
P1: If the destination loses Enterprise between the pre-transaction entitlement read and lock acquisition, this stale check allows the move despite the entitlement-downgrade blocker. Re-resolve entitlements after acquiring the organization locks using a transaction-safe query, or otherwise fence the subscription state under the same lock.</violation>
<violation number="4" location="apps/sim/lib/workspaces/admin-move.ts:1296">
P2: If the process fails after the move commits but before `recordSourceOrganizationMoveAudit` completes, the durable retry path cannot restore the source organization's loss audit. Record the source audit transactionally or persist enough source outcome data for the operation-recovery path to replay it.</violation>
</file>
<file name="apps/sim/lib/workspaces/admin-move.test.ts">
<violation number="1" location="apps/sim/lib/workspaces/admin-move.test.ts:812">
P2: The new 'fences the payer transfer' test only asserts the happy-path fence value; no test exercises the SourceOrganizationChangedError retry that the PR flags as the critical concurrency guarantee. In admin-move.ts the optimistic source-org read (line 825) is re-verified under the locks (throw SourceOrganizationChangedError at line 936) and the loop re-reads the org and re-fences the payer. A stale fence is precisely the case where an inter-org move could transfer the payer to the wrong organization, so a test should queue the workspace read to return a different org on a later attempt and assert the retry passes the corrected expectedCurrentPayer.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
42dd04d to
2ebee1c
Compare
|
@cubic review |
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
|
@cubic review |
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 existing issues remain and 1 new issue found across 7 files
Confidence score: 2/5
apps/sim/lib/workspaces/admin-move.tscan skip Enterprise validation if the source organization upgrades after the initial read, allowing an invalid move under a stale entitlement snapshot — re-read source and destination entitlements before applying the guard.apps/sim/lib/workspaces/admin-move.tsincorrectly requires a subscription row for self-hosted deployments where billing and access control are deployment-configured, potentially rejecting valid workspace moves — bypass the subscription requirement for deployment-configured entitlements.apps/sim/lib/api/contracts/v1/admin/dashboard-workspaces.tscan return more than 500 deleted custom blocks even though the move response contract does not allow that size, causing contract violations for large workspaces — bound the post-move transaction summary.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/workspaces/admin-move.ts">
<violation number="1" location="apps/sim/lib/workspaces/admin-move.ts:1137">
P1: On self-hosted deployments with billing enabled and access control enabled, entitlement is deployment-configured rather than subscription-backed, but this guard still requires a subscription row. Skip this subscription fence for deployment-configured entitlement modes so inter-organization moves remain usable.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Fix all with cubic | Re-trigger cubic
7fad82e to
621689d
Compare
|
@cubic review |
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 existing issue remains and 2 new issues found across 8 files
Confidence score: 2/5
apps/sim/lib/workspaces/admin-move.tscan skip the Enterprise fence if the source organization changes after the initial entitlement read, silently downgrading the workspace in a non-Enterprise destination; re-evaluate the source and destination entitlements before completing the move.apps/sim/ee/workspace-forking/lib/create-fork.tsholds the organization lock across resource-heavy forks, so work taking longer than five seconds can cause concurrent admin moves to fail with a lock-timeout error; avoid holding this short-timeout lock across the whole operation.apps/sim/lib/api/contracts/v1/admin/dashboard-workspaces.tscan reject move responses containing more than 500 post-move custom blocks, turning an otherwise successful move into a contract failure; bound the summary or use a response contract that permits the full list.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/ee/workspace-forking/lib/create-fork.ts">
<violation number="1" location="apps/sim/ee/workspace-forking/lib/create-fork.ts:175">
P2: When a resource-heavy organization fork runs longer than five seconds, a concurrent admin move waiting on this lock fails with a lock-timeout error. Do not hold this short-timeout organization lock across the whole copy transaction, or add retry handling for this contention.</violation>
</file>
<file name="apps/sim/lib/workspaces/admin-move.ts">
<violation number="1" location="apps/sim/lib/workspaces/admin-move.ts:1137">
P1: When the source organization becomes Enterprise after the initial entitlement read, this fence is skipped and the move can silently downgrade the workspace in a non-Enterprise destination. Re-evaluate the source and destination entitlements under the organization locks, or use a transaction-aware optimistic fence instead of gating on the stale pre-transaction source result.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
6 issues found across 8 files
Confidence score: 2/5
apps/sim/ee/workspace-forking/lib/create-fork.tsacquires locks in the reverse order used by invitation acceptance, which can deadlock concurrent organization-backed forks and acceptances; it also overwrites the fork’s 10-second timeout with 5 seconds, so preserve lock ordering and timeout semantics.apps/sim/lib/api/contracts/v1/admin/dashboard-workspaces.tscan return contract-invalid responses after a move by omitting required credential truncation fields or emitting oversized summaries for unpublished blocks and detached permission groups; populate the default fields and bound/report truncation.apps/sim/lib/workspaces/admin-move.tsrelies on a stale Enterprise-entitlement read, allowing capability loss when the source organization changes during the move; re-read or validate the entitlement inside the transaction before applying the fence.apps/sim/lib/workspaces/admin-move.tsdiscards persisted source context on retries of completed durable moves, causing the response to mark the source unrecoverable and skip source-audit replay; reuse the persisted durable-move context.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/ee/workspace-forking/lib/create-fork.ts">
<violation number="1" location="apps/sim/ee/workspace-forking/lib/create-fork.ts:175">
P2: Organization-backed forks overwrite the fork transaction's 10-second lock timeout with the helper's 5-second timeout. Reapply the fork timeout after acquiring the organization lock, or use a lock helper that preserves the caller's timeout, so row contention does not abort twice as early as the fork contract specifies.</violation>
<violation number="2" location="apps/sim/ee/workspace-forking/lib/create-fork.ts:175">
P1: When an organization-backed fork overlaps invitation acceptance, this call acquires the organization lock before the source workspace row. Acceptance takes that row first and then the organization lock, so the transactions can wait on each other until one hits `lock_timeout`; acquire the shared workspace-invitation advisory lock before this organization lock to preserve the established order.</violation>
</file>
<file name="apps/sim/lib/api/contracts/v1/admin/dashboard-workspaces.ts">
<violation number="1" location="apps/sim/lib/api/contracts/v1/admin/dashboard-workspaces.ts:82">
P2: When a workspace has over 500 unpublished blocks or detached permission groups, the move commits but its response fails contract validation. Bound the post-move summary too and report omitted rows through `truncated`, or raise the contract limit consistently.</violation>
<violation number="2" location="apps/sim/lib/api/contracts/v1/admin/dashboard-workspaces.ts:154">
P1: Every successful move response omits the newly required credential truncation fields, so clients fail contract validation after the move commits. Add zero-valued fields to `EMPTY_CREDENTIAL_SUMMARY` and update the credential summary type.</violation>
</file>
<file name="apps/sim/lib/workspaces/admin-move.ts">
<violation number="1" location="apps/sim/lib/workspaces/admin-move.ts:1019">
P2: When a completed durable move is retried through the move endpoint, this branch discards the persisted source context, so the response claims the source is unrecoverable and skips source-audit replay. Reuse `durableAudit` to reconstruct the applied context and replay the source audit before returning the idempotent summary.</violation>
<violation number="2" location="apps/sim/lib/workspaces/admin-move.ts:1137">
P1: When the source organization gains Enterprise entitlement after the pre-transaction read, this stale `sourceIsEnterprise` value skips the fence and allows capability loss into a non-Enterprise destination. Re-read both organizations' entitlements under the acquired locks before allowing the transfer.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
621689d to
d967e00
Compare
|
@cubic review |
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 existing issue remains and 2 new issues found across 8 files
Confidence score: 2/5
- In
apps/sim/lib/billing/core/subscription.ts, billing-blocked destinations with active or past-due organization subscriptions can be treated as entitled, bypassing the preflight downgrade blocker; ensure billing-blocked status cannot enable the transaction fence entitlement. - In
apps/sim/lib/workspaces/admin-move.ts, non-Enterprise destination plans are treated as entitled despiteresolveMoveEntitlementsidentifying an Enterprise downgrade, which could allow invalid workspace moves; reuse the same Enterprise predicate. - In
apps/sim/ee/workspace-forking/lib/create-fork.ts, concurrent personal-workspace forking and organization attachment can violate the parent/child organization invariant because the row lock is insufficient; coordinate with attachment or revalidate and handle the race.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/billing/core/subscription.ts">
<violation number="1" location="apps/sim/lib/billing/core/subscription.ts:446">
P1: When the destination is billing-blocked but still has an active or past-due organization subscription, this helper enables the transaction fence to treat it as entitled. That bypasses the preflight downgrade blocker and can move an Enterprise workspace into a destination whose capabilities are disabled; include each organization's effective billing-blocked state when revalidating entitlement.</violation>
</file>
<file name="apps/sim/lib/workspaces/admin-move.ts">
<violation number="1" location="apps/sim/lib/workspaces/admin-move.ts:1162">
P1: When the destination has a non-Enterprise organization plan, this check treats it as entitled even though `resolveMoveEntitlements` marks it as an Enterprise downgrade. Use the same Enterprise predicate here so moving from Enterprise cannot silently lose gated capabilities.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
2 existing issues remain and 3 new issues found across 8 files
Confidence score: 2/5
apps/sim/ee/workspace-forking/lib/create-fork.tscan let a fork commit before a waiting organization re-home operation moves the parent without revalidating lineage, risking inconsistent parent/child relationships—revalidate lineage while holding or reacquiring the lock.apps/sim/lib/billing/core/subscription.tscan bypassresolveMoveEntitlementswhen billing is enabled outside self-hosted access-control mode, allowing an active but billing-blocked source to pass move checks—ensure the locked check honors entitlement results.apps/sim/lib/workspaces/admin-move.tstreats Team as entitled throughisOrgPlan, so Enterprise-to-Team moves can bypass downgrade protection—compare the actual required entitlement levels before allowing the move.apps/sim/lib/workspaces/admin-move-source-impact.tsmay issue up to 2,000 concurrent database queries during source custom-block enrichment, risking database saturation during preflight—limit concurrency or batch the usage-count query.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/billing/core/subscription.ts">
<violation number="1" location="apps/sim/lib/billing/core/subscription.ts:446">
P1: When billing is enabled outside self-hosted access-control mode, this helper makes the locked move check bypass the entitlement result from `resolveMoveEntitlements`. An active-but-billing-blocked source can therefore be rejected as entitled, while a destination that is only `past_due` can be accepted as entitled despite the preflight resolver treating it as unavailable; make the locked recheck use the same billing-block and usable-status semantics as `resolveOrganizationEnterprisePlan`.</violation>
</file>
<file name="apps/sim/lib/workspaces/admin-move.ts">
<violation number="1" location="apps/sim/lib/workspaces/admin-move.ts:1162">
P1: When the source has Enterprise entitlements and the destination has a Team subscription, this recheck treats both organizations as entitled because `isOrgPlan` includes Team. The move therefore bypasses the downgrade blocker and silently drops Enterprise capabilities; check Enterprise specifically here.</violation>
</file>
<file name="apps/sim/lib/api/contracts/v1/admin/dashboard-workspaces.ts">
<violation number="1" location="apps/sim/lib/api/contracts/v1/admin/dashboard-workspaces.ts:199">
P2: After a completed move is reloaded through the move-operation endpoint, these required entitlement fields report both organizations as non-Enterprise unconditionally. Persist or reconstruct the actual entitlement values before exposing them in the operation response.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Fix all with cubic | Re-trigger cubic
| * Exported so those callers cannot drift from the short-circuits below. | ||
| */ | ||
| export function isSubscriptionBackedEntitlement(): boolean { | ||
| return isBillingEnabled && !(isAccessControlEnabled && !isHosted) |
There was a problem hiding this comment.
P1: When billing is enabled outside self-hosted access-control mode, this helper makes the locked move check bypass the entitlement result from resolveMoveEntitlements. An active-but-billing-blocked source can therefore be rejected as entitled, while a destination that is only past_due can be accepted as entitled despite the preflight resolver treating it as unavailable; make the locked recheck use the same billing-block and usable-status semantics as resolveOrganizationEnterprisePlan.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/sim/lib/billing/core/subscription.ts, line 446:
<comment>When billing is enabled outside self-hosted access-control mode, this helper makes the locked move check bypass the entitlement result from `resolveMoveEntitlements`. An active-but-billing-blocked source can therefore be rejected as entitled, while a destination that is only `past_due` can be accepted as entitled despite the preflight resolver treating it as unavailable; make the locked recheck use the same billing-block and usable-status semantics as `resolveOrganizationEnterprisePlan`.</comment>
<file context>
@@ -431,6 +431,21 @@ export async function isEnterpriseOrgAdminOrOwner(userId: string): Promise<boole
+ * Exported so those callers cannot drift from the short-circuits below.
+ */
+export function isSubscriptionBackedEntitlement(): boolean {
+ return isBillingEnabled && !(isAccessControlEnabled && !isHosted)
+}
+
</file context>
| ) | ||
| ) | ||
| const entitledOrganizationIds = new Set( | ||
| entitledRows.filter((row) => isOrgPlan(row.plan)).map((row) => row.referenceId) |
There was a problem hiding this comment.
P1: When the source has Enterprise entitlements and the destination has a Team subscription, this recheck treats both organizations as entitled because isOrgPlan includes Team. The move therefore bypasses the downgrade blocker and silently drops Enterprise capabilities; check Enterprise specifically here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/sim/lib/workspaces/admin-move.ts, line 1162:
<comment>When the source has Enterprise entitlements and the destination has a Team subscription, this recheck treats both organizations as entitled because `isOrgPlan` includes Team. The move therefore bypasses the downgrade blocker and silently drops Enterprise capabilities; check Enterprise specifically here.</comment>
<file context>
@@ -601,6 +1093,99 @@ export async function moveWorkspaceToOrganization(params: {
+ )
+ )
+ const entitledOrganizationIds = new Set(
+ entitledRows.filter((row) => isOrgPlan(row.plan)).map((row) => row.referenceId)
+ )
+ if (
</file context>
| entitledRows.filter((row) => isOrgPlan(row.plan)).map((row) => row.referenceId) | |
| entitledRows.filter((row) => row.plan === 'enterprise').map((row) => row.referenceId) |
| ), | ||
| sourceOrganizationImpact: adminDashboardWorkspaceSourceImpactSchema, | ||
| credentials: adminDashboardWorkspaceCredentialsSchema, | ||
| entitlements: z.object({ |
There was a problem hiding this comment.
P2: After a completed move is reloaded through the move-operation endpoint, these required entitlement fields report both organizations as non-Enterprise unconditionally. Persist or reconstruct the actual entitlement values before exposing them in the operation response.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/sim/lib/api/contracts/v1/admin/dashboard-workspaces.ts, line 199:
<comment>After a completed move is reloaded through the move-operation endpoint, these required entitlement fields report both organizations as non-Enterprise unconditionally. Persist or reconstruct the actual entitlement values before exposing them in the operation response.</comment>
<file context>
@@ -91,6 +194,17 @@ const adminDashboardWorkspacePreflightSchema = z.object({
),
+ sourceOrganizationImpact: adminDashboardWorkspaceSourceImpactSchema,
+ credentials: adminDashboardWorkspaceCredentialsSchema,
+ entitlements: z.object({
+ sourceIsEnterprise: z.boolean(),
+ destinationIsEnterprise: z.boolean(),
</file context>
The admin workspace move was restricted to personal/grandfathered sources; `assertWorkspaceMovable` refused anything already owned by an organization, so support could only re-home a workspace with manual SQL. Relax that guard to a drift-only check and handle the source organization. `changeWorkspaceStoragePayerInTx` already accepted an arbitrary source payer, so the storage-ledger rebalance needed no change. Moving a workspace between organizations is the first operation capable of separating an artifact from the organization that owns it, so two invariants nothing has ever had to defend are enforced here: - A custom block and its bound workflow always share an organization. `getCustomBlockAuthority` resolves by the consumer's org and `admitCustomBlockChildExecution` skips its concurrency reservation on the strength of that, so a stranded row would run a foreign tenant's workflow under its owner's credentials, billed to the wrong payer. The move unpublishes those blocks through the product's own `deleteCustomBlock` and records the loss in the source organization's audit view. - A fork parent and child always share an organization. `resolveForkEdge` has no org check at all, so the move refuses while a cross-org edge would result. Enforcing them at move time is not enough on its own: `publishCustomBlock` and fork creation wrote without the organization mutation lock, so either could commit after the move's scans and produce exactly the artifact the move refused to create. Both now take that lock, which is what actually makes the invariants hold under concurrency. Pending invitations block too. Re-stamping an org-scoped invitation would convert a pending membership in the source org into one in the destination, consuming a seat for an invitation the destination never issued. An entitlement downgrade blocks: `isOrganizationOnEnterprisePlan` gates permission groups, SSO domains, data retention, session revocation, forking, and custom blocks, and losing them silently is not recoverable. That check resolves before the transaction — it reads through the global client with no executor seam, and a plan lapsing in the intervening seconds is recoverable by moving the workspace back, unlike a cross-organization artifact. Both organizations are locked, ascending by id, mirroring `acquireOrganizationUserMutationLocks`. The source id is read optimistically before the transaction and re-verified under the locks, retrying through the existing loop when it moved.
d967e00 to
2ac4d8a
Compare
|
@cubic review |
@mzxchandra I have started the AI code review. It will take a few minutes to complete. |
| const blockedOrganizationIds = new Set(blockedRows.map((row) => row.organizationId)) | ||
| const entitledOrganizationIds = new Set( | ||
| entitledRows | ||
| .filter((row) => isOrgPlan(row.plan)) |
There was a problem hiding this comment.
When an Enterprise-entitled source workspace moves to a destination on a Team plan, isOrgPlan includes the destination in entitledOrganizationIds even though isOrganizationOnEnterprisePlan rejects Team plans. The transactional check therefore allows the move to commit, leaving the workspace without the Enterprise capabilities that preflight identified as lost.
Knowledge Base Used: Database schema and migrations
There was a problem hiding this comment.
1 existing issue remains and 1 new issue found across 8 files
Confidence score: 2/5
apps/sim/lib/workspaces/admin-move.tscan allow an Enterprise source to move into an active Team destination, silently losing Enterprise capabilities; require an active Enterprise subscription on the destination.apps/sim/lib/workspaces/admin-move-source-impact.tscan populateretainedCollaboratorCapswith uncapped collaborators and omit actual cap holders when many collaborators exist, making the review inaccurate; join directly to source cap rows.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/workspaces/admin-move-source-impact.ts">
<violation number="1" location="apps/sim/lib/workspaces/admin-move-source-impact.ts:410">
P2: When a workspace has many uncapped collaborators, this left join fills `retainedCollaboratorCaps` with rows that have no cap and can truncate actual cap holders from the review. Join the source cap rows directly so the list contains only limits that will stop applying.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Fix all with cubic | Re-trigger cubic
| * which is the opposite of the field's purpose: disclosing every cap that | ||
| * stops applying. The cap row itself already scopes to the source org. | ||
| */ | ||
| .leftJoin( |
There was a problem hiding this comment.
P2: When a workspace has many uncapped collaborators, this left join fills retainedCollaboratorCaps with rows that have no cap and can truncate actual cap holders from the review. Join the source cap rows directly so the list contains only limits that will stop applying.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/sim/lib/workspaces/admin-move-source-impact.ts, line 410:
<comment>When a workspace has many uncapped collaborators, this left join fills `retainedCollaboratorCaps` with rows that have no cap and can truncate actual cap holders from the review. Join the source cap rows directly so the list contains only limits that will stop applying.</comment>
<file context>
@@ -0,0 +1,536 @@
+ * which is the opposite of the field's purpose: disclosing every cap that
+ * stops applying. The cap row itself already scopes to the source org.
+ */
+ .leftJoin(
+ organizationMemberUsageLimit,
+ and(
</file context>
There was a problem hiding this comment.
3 existing issues remain and no new issues found across 8 files
Confidence score: 2/5
apps/sim/lib/workspaces/admin-move.tscan allow an Enterprise workspace to move to Team without triggering the downgrade guard becauseisOrgPlantreats Team as entitled; use an Enterprise-specific entitlement check in the transactional fence.apps/sim/ee/workspace-forking/lib/create-fork.tshas a lock-ordering race where a fork can commit before organization attachment observes it, leaving the parent and child in different organizational states; serialize these operations or recheck the newly created child before rehoming.apps/sim/lib/workspaces/admin-move-source-impact.tscan launch roughly 2,000 database queries concurrently for 500 source blocks, potentially saturating the primary pool and causing preflight failures; bound concurrency or aggregate the usage query.
Requires human review: Auto-approval blocked because this review re-detected 3 unresolved issues already reported by Cubic.
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
|
Closing in favour of #7254, which carries the identical file state on a fresh branch off current staging. This PR accumulated ~45 review threads across 10 rounds. Every substantive finding in it was fixed — 23 in total, all real bugs bar one false positive — and those fixes are all present in #7254. But the review bots had begun re-reviewing the accumulated thread history rather than the diff: re-raising findings already resolved on the head commit, and regressing the confidence score on code that had not changed between rounds. That is churn, not signal. #7254 is the same eight files, byte-identical, with the full review history summarised in its description so none of the context is lost. Verified before opening: 37,455 tests pass across 2,670 files, |
Summary
Lets the admin panel move a workspace from one organization to another. The move was restricted to personal/grandfathered sources —
assertWorkspaceMovablethrew "Inter-organization workspace transfers are not supported" — so support could only re-home a workspace with manual SQL.Most of the machinery already existed: durable operation IDs, seat-capacity checks, and
changeWorkspaceStoragePayerInTx, which already accepted an arbitrary source payer, so the storage-ledger rebalance between two orgs needed no change. The work is a guard relaxation plus handling what the source organization loses.Two invariants this feature could break, now enforced here
Moving a workspace between organizations is the first operation in the product capable of separating an artifact from the org that owns it. Two invariants nothing has ever had to defend:
A custom block and its bound workflow always share an organization.
publishCustomBlockrefuses a workflow outside the target org, so the pair has always been co-located.getCustomBlockAuthorityresolves by the consumer's org, andadmitCustomBlockChildExecutiondeliberately skips its concurrency reservation because "the consumer and source workspaces are always in the same organization". A stranded row would run a foreign tenant's workflow under its owner's credentials, billed to the wrong payer. The move unpublishes those blocks through the product's owndeleteCustomBlock, and recordsCUSTOM_BLOCK_DELETEDin the source org's audit view.A fork parent and child always share an organization.
assertCanForkpins the child to the source's org andresolveForkEdgehas no org check at all. The move refuses while a cross-org edge would result; the fork must be disconnected first.Three blockers
Each is evaluated in preflight and re-checked inside the transaction, because a subscription can lapse, a fork can be created, and an invitation can arrive in between.
fork-lineage-conflictpending-invitations-presentdestination-entitlement-downgradeBlocking on invitations deleted the most intricate part of the design — no cross-org invitation migration, no intent resolver, no sibling-invitation creation.
invitation-migration-plan.tsis untouched and the shipped personal→org invitation path is not modified at all.Concurrency
Both organizations are now locked, ascending by id, mirroring
acquireOrganizationUserMutationLocks. The source id is read optimistically before the transaction and re-verified under the locks, retrying through the existing loop viaSourceOrganizationChangedError. Lock order is unchanged: invitation/workspace advisory locks → org locks → workspace row.Audit
The workspace-scoped move entry resolves to the destination after the move, because
buildOrgScopeConditionscopes org audit reads by the org's current workspaces. Without more, the org that lost the workspace would have no record. Adds an org-level entry (workspaceId: null+metadata.organizationId, that condition's other branch) so the loss is diagnosable from the side that incurred it.Type of Change
Testing
Automated: full
apps/simsuite — 37,306 tests across 2,669 files, zero failures.turbo run type-checkclean across all 26 workspaces.check:api-validation:strict,check:boundaries,check:client-boundary,check:route-verbs,check:api-contract-routes,check:utils,check:import-specifiers, and the 39-auditcheck:auditsbundle all pass.11 new tests cover the org→org path: both org locks acquired in sorted order after the invitation locks and before the row lock; the payer transfer fenced on the source org read under those locks (the assertion that makes the whole move safe); each blocker refusing without mutating anything; custom blocks unpublished; the source-org audit entry landing in the source and not the destination; and the personal source path unchanged, as a regression fence.
End to end against a real database, two orgs and a workspace with content on both sides. Verified: org-owned workspaces appear as candidates with their source org named; every blocker refuses and mutates nothing; the happy path re-homes org, payer, and
organizationAssignedAt; the move is reversible; replaying anoperationIdis idempotent; and both invariant post-conditions hold (zero cross-org custom blocks, zero cross-org fork edges).E2E surfaced two defects the unit tests missed, both now with regression tests:
getMovedWorkspaceSummarytook anappliedContextparameter no call site passed, so every applied move reportedsourceOrganization: null; and the source-org audit entries were specified but never implemented.Reviewers should focus on the lock ordering in
moveWorkspaceToOrganizationand theexpectedCurrentPayerfence — that optimistic check is what makes the payer transfer safe under concurrency.Checklist
Notes for reviewers
Not verifiable on a local deployment.
resolveOrganizationEnterprisePlanshort-circuits onisAccessControlEnabled && !isHosted, so every org resolves as entitled locally and the downgrade blocker cannot fire. That is correct — the gates it protects also all-pass there, so nothing is lost — but it means the blocker needs a hosted deployment to exercise. Related:isOrganizationOnEnterprisePlanis really "is on a paid org plan" (isOrgPlan = isTeam || isEnterprise), so Team is not a downgrade from Enterprise for these capabilities. The blocker reuses that same predicate deliberately, so it can never disagree with the gates it protects.Companion PR in
simstudioai/adminadds the review modal that presents all of this before the admin confirms.