Repository navigation
fix(orgs): make organization deletion temporarily unavailable (#6827) - #7560
Conversation
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean defensive containment slice for #6827.
Both organization-deletion entry points (self-service DELETE /api/organizations/:orgId and admin DELETE /api/admin/accounts/:orgId) now return an unconditional 503 organization_deletion_unavailable before authentication, authority resolution, billing/provider reads, audit writes, and cascades, with the authorization observer correctly excluded. Reviewer verified return-before-auth (per-route guards removed, no dangling imports), effect-free proof in the 299-line integration test, app-only scope (no protocol surface, no changeset required or present), and the merge-path writer as genuinely pre-existing.
No blocking, High, or Medium code defects. Two documented non-blocking follow-ups noted (503-driven 5xx dashboard inflation, stale admin-UI delete copy) — neither actionable here.
Decision path: no critical/high findings (row 1 n/a); gated_paths false (row 2 n/a); high-risk reasons are (modified) not (deleted) (row 3 n/a); no medium findings at all (rows 4/5/8 n/a); no prior decision (row 6 n/a); no author team gate (row 7 n/a). The high_risk flag is a heuristic on modified server/src/routes/** files with zero medium-or-higher concerns, so the modification is presumed safe. Falls through to row 9 → approve.
97652f8 to
468adfa
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — clean pass, no blocking or medium findings.
This PR is a patch-identical restack of a previously clean-approved tree (prior decision: approve). Both DELETE org-deletion handlers (organizations.ts:503, accounts-billing.ts:362) now return an unconditional 503 before any auth/authority/billing/provider/audit work, with the observer exclusion applied. Route matching is not broadened and no org data reaches the wire. Mounted Postgres integration and unit suites confirm zero DB/provider/audit/cache/notification effect across sibling substitution, all roles, platform admin, selectors, subscription states, replay, and CSRF ordering.
No protocol surface (static/schemas/source/, docs/reference/, dist/**), changeset, or dist artifact is touched — this is an operational/app change, so changeset, schema↔docs, oneOf, and immutability gates do not apply. The only operational cost (503 inflating /api 5xx metrics) is documented with mitigation.
Decision-table walk: no critical/high (row 1 no), gated_paths false (row 2 no), no deletions in high-risk reasons — both are (modified) (row 3 no), no medium findings (rows 4/5/8 no), prior decision was approve not escalate (row 6 no), no no-auto-approve team match (row 7 no). Falls through to row 9 → approve. high_risk is true only via (modified) route files with zero medium-or-higher findings, so the modifications are presumed safe per the high-risk-flag guidance. review_decision is APPROVED.
3085c09 to
acad5ed
Compare
The base branch was changed.
468adfa to
2a97bee
Compare
Return a stable 503 from the self-service and administrative organization deletion routes before route authentication, provider calls, or local effects. Exclude these contained requests from post-response authorization observation. Preserve the landed membership and authentication correction. Keep organization merge and operator bypass containment in the separate dependent change. Document the durable lifecycle requirements for re-enabling deletion without claiming a journal, compensation, or rollback guarantee. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2a97bee to
95f9e0f
Compare
There was a problem hiding this comment.
Ladon verdict: Approve
Approve — metadata-only follow-up to a previously clean approval.
The only change since the prior clean approve (head 468adfa) is three frontmatter lines added to docs/contributing/organization-deletion-containment.md (description + AdCP-prefixed og:title) to satisfy the SEO gate. Naming is correct ("AdCP"), and no wire/schema/changeset/normative reference surface is touched — this is a contributing doc, not docs/reference/** or static/schemas/source/**. The containment code and test suites are identical to the prior approved candidate.
Checks applied:
- No critical/high/medium findings this run (rows 1, 4, 8 do not fire).
- gated_paths: false (row 2 does not fire).
- high_risk is true only via
server/src/routes/**files that are (modified) but carry no findings — under the flag-reading rules a modified high-risk file with no medium-or-higher concern is presumed safe; rows 3 and 5 require a deletion or a medium finding respectively, neither present. - Prior decision was
approve, so sticky escalation (row 6) does not apply. - No no-auto-approve team match (row 7 does not fire).
None of rows 1–8 fire, so the outcome is row 9: approve.
Organization merge deleted the secondary organization as its final step. mergeOrganizations removed the local organization row inside its transaction, committed, and only then attempted workos.organizations.deleteOrganization, downgrading a provider failure to a warning while still reporting success. The Addie path attempted the provider delete twice — once inside mergeOrganizations and again at step 4 of the tool handler — with WorkOS membership creation interleaved between the local commit and that second attempt. That is the split provider/local state commit 97652f8 closed for the two DELETE endpoints, so merge is now unavailable on the same terms and with the same stable shape. Three layers, each refusing before any merge-specific effect: - POST /api/admin/cleanup/merge returns a stable 503 organization_merge_unavailable before body validation and before any merge-specific transaction, WorkOS membership write, provider organization deletion, Stripe operation, audit row, cache invalidation or notification. Administrative gating is deliberately UNCHANGED — requireGlobalAdmin still authenticates and authorizes, unlike the contained deletion routes, because nothing here needs to run before authentication. - The Addie merge_organizations tool refuses execution before it reads provider memberships and no longer acquires a WorkOS client on that path; the client moved into the read-only preview branch. - mergeOrganizations itself throws OrganizationMergeUnavailableError before getPool(), pool.connect() and BEGIN, so an overlooked caller, a new caller or a script bypassing both entry points cannot execute the sequence either. The unsafe body is removed rather than left unreachable behind the guard: that sequence is the hazard, so re-enabling merge must be a new reviewed implementation meeting the lifecycle contract in docs/contributing/organization-deletion-containment.md, not the deletion of a guard. After this change no production call site of workos.organizations.deleteOrganization remains in server/src. A containment that only covers routes and services is one an operator can walk around, and two checked-in surfaces handed them the prohibited operation: - scripts/incidents/2026-05-cleanup-duplicate-prospect-stubs.ts deleted organization rows with direct SQL, reasoning from the absence of an admin delete endpoint that a script was the sanctioned route. Its destructive path is removed, not gated: no database client, no writes, and --execute hard-refuses above every other statement. Read-only analysis is preserved. - The unique-org-per-email-domain invariant emitted a ready-to-paste raw delete with the organization ID pre-filled, and pointed at a merge endpoint that does not exist. Its remediation_hint now routes to read-only preview-merge inspection plus escalation and names both containment codes. server/tests/unit/organization-delete-statement-guard.test.ts is the static guard: it scans the JavaScript, TypeScript and SQL files under server/src, server/scripts and scripts for a raw organization delete and fails on anything outside an allowlist recording why each remaining occurrence is not operator-reachable. SQL is covered because a .sql runbook is the likeliest shape for the next bypass. server/scripts/setup-sandbox.ts is documented and allowlisted rather than changed: it is sk_test-gated and ID-bounded. previewMerge and GET /api/admin/cleanup/preview-merge are untouched and remain read-only (SELECT/COUNT only). The WorkOS preview section describes provider effects conditionally and the tool footer states that execution refuses; the existing Stripe and data-movement warnings remain unchanged. An existing assertion in server/tests/unit/integrity/invariants.test.ts pinned the raw delete this change removes; it now pins the inverse contract. The three org-merge suites keep their fixtures and prove the contained service performs no work on them, and a pre-existing identities cleanup leak in the primary-organization suite is fixed deterministically by id. No migration, lifecycle journal, compensation, partial merge or rollback fiction is introduced. The merge_organizations tool's declared description and usage hints are intentionally left alone because the Addie tool-surface budget pins a reviewed profile-contract hash across every declared tool profile; that re-blessing is recorded as its own follow-up. Reconstructed directly over landed #7560 at 82b429a. Preserve main's authorization correction and SEO metadata. Bump Addie CODE_VERSION to 2026.09.60 as required for this tool implementation change. Correct the containment document to acknowledge the identity consolidation refusal already on main, and narrow its conditional-wording claim to the WorkOS preview section. Original rationale and co-author attribution are retained; the original public commit and description remain archived. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… (#7561) Organization merge deleted the secondary organization as its final step. mergeOrganizations removed the local organization row inside its transaction, committed, and only then attempted workos.organizations.deleteOrganization, downgrading a provider failure to a warning while still reporting success. The Addie path attempted the provider delete twice — once inside mergeOrganizations and again at step 4 of the tool handler — with WorkOS membership creation interleaved between the local commit and that second attempt. That is the split provider/local state commit 97652f8 closed for the two DELETE endpoints, so merge is now unavailable on the same terms and with the same stable shape. Three layers, each refusing before any merge-specific effect: - POST /api/admin/cleanup/merge returns a stable 503 organization_merge_unavailable before body validation and before any merge-specific transaction, WorkOS membership write, provider organization deletion, Stripe operation, audit row, cache invalidation or notification. Administrative gating is deliberately UNCHANGED — requireGlobalAdmin still authenticates and authorizes, unlike the contained deletion routes, because nothing here needs to run before authentication. - The Addie merge_organizations tool refuses execution before it reads provider memberships and no longer acquires a WorkOS client on that path; the client moved into the read-only preview branch. - mergeOrganizations itself throws OrganizationMergeUnavailableError before getPool(), pool.connect() and BEGIN, so an overlooked caller, a new caller or a script bypassing both entry points cannot execute the sequence either. The unsafe body is removed rather than left unreachable behind the guard: that sequence is the hazard, so re-enabling merge must be a new reviewed implementation meeting the lifecycle contract in docs/contributing/organization-deletion-containment.md, not the deletion of a guard. After this change no production call site of workos.organizations.deleteOrganization remains in server/src. A containment that only covers routes and services is one an operator can walk around, and two checked-in surfaces handed them the prohibited operation: - scripts/incidents/2026-05-cleanup-duplicate-prospect-stubs.ts deleted organization rows with direct SQL, reasoning from the absence of an admin delete endpoint that a script was the sanctioned route. Its destructive path is removed, not gated: no database client, no writes, and --execute hard-refuses above every other statement. Read-only analysis is preserved. - The unique-org-per-email-domain invariant emitted a ready-to-paste raw delete with the organization ID pre-filled, and pointed at a merge endpoint that does not exist. Its remediation_hint now routes to read-only preview-merge inspection plus escalation and names both containment codes. server/tests/unit/organization-delete-statement-guard.test.ts is the static guard: it scans the JavaScript, TypeScript and SQL files under server/src, server/scripts and scripts for a raw organization delete and fails on anything outside an allowlist recording why each remaining occurrence is not operator-reachable. SQL is covered because a .sql runbook is the likeliest shape for the next bypass. server/scripts/setup-sandbox.ts is documented and allowlisted rather than changed: it is sk_test-gated and ID-bounded. previewMerge and GET /api/admin/cleanup/preview-merge are untouched and remain read-only (SELECT/COUNT only). The WorkOS preview section describes provider effects conditionally and the tool footer states that execution refuses; the existing Stripe and data-movement warnings remain unchanged. An existing assertion in server/tests/unit/integrity/invariants.test.ts pinned the raw delete this change removes; it now pins the inverse contract. The three org-merge suites keep their fixtures and prove the contained service performs no work on them, and a pre-existing identities cleanup leak in the primary-organization suite is fixed deterministically by id. No migration, lifecycle journal, compensation, partial merge or rollback fiction is introduced. The merge_organizations tool's declared description and usage hints are intentionally left alone because the Addie tool-surface budget pins a reviewed profile-contract hash across every declared tool profile; that re-blessing is recorded as its own follow-up. Reconstructed directly over landed #7560 at 82b429a. Preserve main's authorization correction and SEO metadata. Bump Addie CODE_VERSION to 2026.09.60 as required for this tool implementation change. Correct the containment document to acknowledge the identity consolidation refusal already on main, and narrow its conditional-wording claim to the WorkOS preview section. Original rationale and co-author attribution are retained; the original public commit and description remain archived. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The self-service and administrative organization-deletion routes now return the generic
503 organization_deletion_unavailableresponse before route authentication or identity hydration, provider calls, billing reads, and deletion work. Their authorization observer is excluded so the response cannot trigger provider reads afterward. Global parsing and CSRF behavior remain in place.Mounted tests cover credentials, linked identities, roles, selectors, replay, observer behavior, and unchanged PostgreSQL state. The containment document records the remaining limitations and the durable lifecycle requirements for re-enabling deletion.
This eight-file change preserves the membership and authorization correction landed in #7538. Merge and operator bypass containment remains the separate follow-on in #7561. No deletion journal, reconciliation implementation, or rollback guarantee is added.
The document now includes a unique
descriptionand AdCP-prefixedog:title, fixing the SEO failure on the first published candidate. These five frontmatter lines are the only addition to the original #7560 delta: the containment source, tests, and document body remain unchanged. The document is therefore no longer byte-identical to the original #7560 blob.Validation on the amended candidate:
Candidate commit:
95f9e0f22bfa24650b3ec805b3150213439cae42Sole parent:
9d247db2e6e2af755951cba268b060425f246e2fReviewed tree:
8cfdad362eab77f39ece3e515f18be647d40f88c