feat(db): audited, idempotent per-tenant data deletion

[Antonio-RiveroMartnez]

Linked issue

None.

Summary

• Adds agor tenant delete --tenant-id <id> (with --dry-run): an audited, idempotent admin command that permanently deletes all data for exactly one tenant from a multi-tenant (PostgreSQL) deployment.
• Introduces a runtime-owned, schema-derived tenant table manifest (@agor/core/db) with an exhaustiveness test, so no future table can silently escape tenant deletion.
• Emits a stable machine-readable JSON contract on stdout for automation; all human/audit logging goes to stderr.
• Hardened through adversarial review into a fail-closed tool: it refuses to run unless the binary's schema is provably in lockstep with the database, aborts loudly on unclassified/transitively-scoped tables rather than deleting partially, and cannot emit a truncated contract.

Why / context

Operators of multi-tenant deployments need a safe, verifiable way to fully remove one tenant — offboarding, data-removal requests, GDPR-style erasure. Hand-written SQL is error-prone: it can miss a table, trip a foreign key, or leave orphaned rows and still look like it worked. This command makes tenant erasure exhaustive, FK-ordered, transactional, and self-verifying, and returns a result automation can assert on.

Implementation notes

Manifest (tenant-deletion-manifest.ts) — derived directly from the PostgreSQL Drizzle schema (not a hand-maintained list). Every table is classified direct (tenant_id column), transitive (reaches a scoped table via an FK chain), or explicitly global. Deletion order is a topological sort over blocking foreign keys (children before parents). buildTenantDeletionManifest() throws at runtime if any table is unclassified, or if a transitive table is present — the transitive delete/verify path is not yet proven, so it fails loud instead of deleting partially.

Deletion (tenant-deletion.ts) — deleteTenantData():

• Rejects empty / blank / whitespace-padded / wildcard tenant ids before opening a connection.
• Refuses non-PostgreSQL databases (multi-tenancy is PostgreSQL-only).
• Fails closed on schema skew: refuses unless the running binary's migrations match the database's applied set in both directions (hasPending, or a database migrated by a newer release) — a mismatched manifest could silently miss a renamed/dropped/added tenant table.
• Refuses to run inside an ambient tenant/system DB scope (which would collapse its delete and verify phases into one uncommitted transaction).
• Deletes all tenant rows inside a single tenant-scoped transaction. RLS (FORCE ROW LEVEL SECURITY) is pinned to the target via the agor.tenant_id GUC, and every statement also carries an explicit tenant_id predicate — defense-in-depth that holds whether or not the maintenance role bypasses RLS.
• Snapshots per-table counts before deleting, so ON DELETE CASCADE can't undercount.
• Re-scans the whole manifest in a fresh post-commit transaction and fails (non-zero) unless zero rows remain. Idempotent: a second run deletes nothing and still succeeds.

CLI (commands/tenant/delete.ts) — resolves the database exactly like the daemon (env/config); prints one JSON object to stdout — { tenantDataDeleted, schemaVersion, rowCounts } (dry-run adds dryRun: true), values limited to booleans, numbers, and identifier strings; the payload is flushed before exit so a piped consumer can't get truncated output. Human logs and errors (credentials redacted) go to stderr. Exit codes: 0 verified success, 2 invalid input, 1 runtime failure.

This PR was hardened through an adversarial advocate/skeptic review on two independent model families. Fixes landed from that review: the fail-closed schema-lockstep guard, the ambient-scope guard, runtime unclassified/transitive guards, and the stdout-flush fix.

Validation / test plan

• pnpm --filter @agor/core typecheck — clean
• pnpm --filter @agor/cli typecheck — clean
• pnpm --filter @agor/core test — 2906 passed, 5 skipped
• biome check — clean on all changed files
• PostgreSQL integration suite (tenant-deletion.postgres.test.ts) run against a real PostgreSQL: scoped deletion, cross-tenant isolation, idempotency, frozen output contract, dry-run — all pass. Gated on AGOR_TEST_POSTGRES_URL + AGOR_DB_DIALECT=postgresql, mirroring the existing *.postgres.test.ts convention; skipped in the SQLite fast lane.
• CI does not yet run the PostgreSQL suite — see Out of scope.

Example success output (stdout):

{"tenantDataDeleted":true,"schemaVersion":"0066_gateway_listener_discovery","rowCounts":{"sessions":3,"tasks":5,"messages":42,"branches":1,"repos":1}}

Risks / rollout / rollback

• Destructive by design. Mitigations: strict id validation, schema-lockstep refusal, single atomic transaction (partial failure rolls back), and post-commit verification that fails loudly if anything survives. --dry-run previews counts without deleting.
• Operational precondition: the operator must quiesce/disable the tenant (stop new tenant-scoped work at the control/auth layer) before running. The command attests to state at scan time; it does not by itself prevent a concurrent writer from recreating rows after verification. This is documented in the CLI help and module doc.
• Purely additive: no schema migration, no change to existing runtime paths beyond three barrel exports and one additive field on checkMigrationStatus. Blast radius on existing deployments is zero until the command is invoked.

Out of scope / follow-ups

• Non-database tenant state (per-branch worktrees, caches on disk) — keyed by branch, not tenant; safe reclamation is deployment-specific. This command is DB-only. Follow-up: enumerate a tenant's branches before deletion and reclaim their filesystem state with the same verify/report rigor.
• Tenant admission/lifecycle fence — a durable "tenant disabled" state that stops new tenant-scoped work belongs at the control/auth layer and is a separate change. This PR documents the precondition; it does not build the fence.
• PostgreSQL CI job — the integration suite (this one and the pre-existing tasks.postgres.test.ts) is env-gated and not exercised by current CI. Follow-up: add a Postgres-service job so the deletion path runs under FORCE RLS in CI.
• Transitive-scope deletion — currently refused at runtime; if a future table needs it, extend the engine with orphan-safe (post-parent-deletion) verification before enabling.

[kgabryje]

Review — verdict: not mergeable as-is (fixable, not architectural)

AI-assisted review (Codex gpt-5.6-sol correctness pass, spot-verified against source). An architectural-lens pass is running separately; I'll follow up if it surfaces anything material.

The core design is sound. runWithTenantDatabaseScope opens its own db.transaction() per call (sets agor.tenant_id transaction-locally, commits on success, rolls back on throw), and the ambient-scope guard forces phase-1 to commit before phase-2 verifies committed state — so the atomicity claim holds. RLS + the explicit tenant_id = $1 predicate on directly-issued statements is correct.

Blocking

1. kb_unit_embeddings escapes the manifest entirely. The manifest enumerates only Drizzle PgTable exports from schema.postgres.ts. But kb_unit_embeddings is created imperatively in apps/agor-daemon/src/knowledge/pgvector.ts (~L160) — a real physical table with a mandatory tenant_id, FORCE ROW LEVEL SECURITY, and a tenant_isolation_kb_unit_embeddings policy — and it is not a Drizzle export. So it is absent from delete, from post-commit verification, and from the exhaustiveness test. Its two ON DELETE CASCADE parents (kb_document_units, kb_embedding_spaces) do cascade-delete its rows in the happy path, but the verification pass never inspects it — so a row whose tenant_id disagrees with its parent survives and the command still certifies "deleted and verified." For a tool whose thesis is exhaustive, self-verifying erasure, that's a real hole. Fix: source the tenant-table set from the live catalog (or explicitly enumerate runtime-created tenant tables) into the manifest + verification, or fail closed when such a table exists but isn't represented.

2. Topo-sort ignores CASCADE / SET NULL edges (tenant-deletion-manifest.ts, isBlockingOnDelete). A parent can be deleted before a cascading child, and the DB cascade that then fires is not constrained by the explicit tenant_id = $1 predicate. Crossing tenants requires a pre-existing cross-tenant FK inconsistency (FKs reference bare object IDs, not (tenant_id, id), so it isn't structurally prevented), so this is lower-likelihood than #1 — but it falsifies the "every affected row is covered by the explicit predicate" defense-in-depth claim. Cheap fix: order children-first for all row-affecting FK actions, not just blocking ones.

3. "Provably in lockstep" is overstated (resolveSchemaVersion). dbAheadOfBinary = maxAppliedMillis > journalMaxWhen is correct for the monotonic case, but a max-timestamp watermark doesn't prove exact schema equivalence: a divergent DB migration at/below the watermark reads as "known applied," equal watermarks can differ in content, applied/schemaVersion is inferred from the binary journal rather than read from the DB, and out-of-band drop/rename is invisible. Either validate the live catalog against the manifest immediately before deleting, or soften the "provably" language. For a destructive command I'd want the catalog check.

4. Empty manifest → false success. If schema discovery ever yields zero tables, both phases no-op and it returns {"tenantDataDeleted":true,"rowCounts":{}}. Add a reject-empty / assert-minimum-fingerprint guard.

Non-blocking hardening

• Tenant-id validation permits newlines / control / ANSI chars → audit-log/terminal injection; reject control chars or validate against the canonical tenant-id format.
• redactSecrets is skipped when a non-Error value is thrown — apply it after stringifying either branch.
• stderr writes aren't awaited before process.exit, so trailing audit/error lines can theoretically truncate (stdout is already handled correctly).
• Integration test doesn't cover kb_unit_embeddings, mid-delete rollback, or a seeded cross-tenant FK mismatch.

Advisory (over-engineering, does not affect the verdict)

The transitive-scope machinery (~85 lines: the recursive buildTenantScopeCondition subquery path + buildParentLink) is dead today — any transitive table is rejected at runtime before it's used. It's fail-closed scaffolding for a deliberately-unsupported path; reasonable to keep, but it could be deleted and re-added with orphan-safe verification when transitive deletion is actually implemented.

Bottom line: #1 is the one true blocker — the "nothing can silently escape" guarantee doesn't hold while the tenant-table set is derived purely from Drizzle exports. #2–#4 are correctness-hardening a destructive erasure command should carry. None require rethinking the approach.

[kgabryje]

Architectural note (approach-level — advisory, distinct from the correctness blockers above)

A separate architectural-lens pass landed, and it makes a case worth putting in front of the team: the manifest models the binary's schema, not the live database — which is the wrong source of truth for a tool whose defining requirement is exhaustive erasure.

discoverTableMetas() enumerates Object.values(postgresSchema) and accepts only exported PgTable objects, so the "exhaustiveness" invariant really means "every table known to this version of schema.postgres.ts was classified" — not "every physical table holding tenant data was classified." kb_unit_embeddings isn't a one-off miss; it's proof that Agor already has a second schema lifecycle (imperative CREATE TABLE + tenant_id + FORCE RLS + tenant_isolation_* policy in pgvector.ts) that is structurally invisible to an ORM-export manifest, its CI guard, and the migration-lockstep check. Any extension, optional subsystem, partition, or future imperative storage falls in the same blind spot.

The suggested direction: derive the deletion plan from the live PostgreSQL catalog rather than Drizzle exports. Postgres already owns every input the plan needs:

• tenant-scoped tables/columns → pg_class / pg_attribute
• RLS / FORCE-RLS status → pg_class
• tenant policies → pg_policy / pg_policies
• FK edges + delete actions (for child-first ordering) → pg_constraint

Discovery would select application-schema tables carrying tenant_id, assert each satisfies the tenant contract (column type/nullability, RLS enabled+forced, expected isolation policy referencing agor.tenant_id), and run the inverse check (every FORCE-RLS/tenant_isolation_* table must expose a supported discriminator) so unusual cases fail closed. Keep the explicit tenant_id = $1 predicate as defense-in-depth (and fail closed / document if the execution role has BYPASSRLS). This naturally covers kb_unit_embeddings and anything else actually present after operational changes — the migration lockstep check then becomes a conservative compatibility guard rather than the thing pretending to guarantee exhaustiveness.

Two smaller structural notes from the same pass:

• The direct / transitive / global model is premature — the code builds transitive classification + recursive predicate machinery, then refuses every transitive table at runtime. The current invariant is simply "tenant-owned tables carry tenant_id"; enforce that and fail on exceptions rather than carrying an unused ownership model (~ the same ~85 lines the advisory note flagged).
• "Deleted and verified" via a second post-commit transaction proves only "no matching rows during the rescan" — a writer can insert between/after, and a verification failure can't roll back the already-committed delete. Genuinely durable erasure needs a lifecycle fence (a durable "tenant disabled" state / advisory lock all writers honor) — which is the same quiescence precondition the PR documents but can't self-enforce. Consider collapsing to a single count→delete→verify transaction and treating any post-commit recheck as an audit, not a stronger guarantee. (Naming nit: this is an offline DB-owner op — agor db tenant-delete may read truer than agor tenant delete.)

Tradeoff: this is a material rewrite of the ~400-line manifest layer, but not a bigger system — a catalog query + plan builder should be smaller than the current Drizzle classification/transitive machinery. The payoff is that the command certifies the live database rather than the binary's partial view of it, which for a regulatory/offboarding tool is the correct boundary.

Flagging as approach-level input for you to weigh against a narrower fix (e.g. a catalog cross-check bolted onto the existing manifest just to close the exhaustiveness gap) — the correctness items above stand regardless of which route you take.

[Antonio-RiveroMartnez]

Addressed (pushed)

• Live-DB exhaustiveness (the real blocker): deletion/verification now reconcile against the live PostgreSQL catalog before deleting and fail closed if any tenant-contract table (a tenant_id column and/or FORCE-RLS + tenant_isolation_* policy) isn't in the plan — so kb_unit_embeddings and any future imperatively-created tenant table can no longer silently escape. The Drizzle manifest remains the typed execution plan; the catalog is the auditor.
• kb_unit_embeddings explicitly covered (delete + verify when present).
• Empty-plan → fail closed (no more vacuous success).
• Ordering: cascade FK edges added to children-first ordering (defense-in-depth for same-tenant cascades).
• Wording: “lockstep” reframed as a conservative migration-compatibility guard; exhaustiveness now comes from the catalog reconciliation.
• Hardening: reject control/newline chars in tenant id; redact secrets on non-Error failures; flush stderr before exit.
• Cleanup: removed the unreachable transitive deletion machinery (kept the classifier that fails closed on any transitive table).

Intentionally not changed (rationale)

• “Order children-first for all FK actions”: infeasible as stated — the schema has SET NULL FK cycles (boards↔branches, sessions↔schedules) that would fabricate an unsolvable ordering cycle; we added the feasible cascade-edge subset and rely on the explicit tenant_id = $1 predicate + catalog reconciliation for the rest.
• Cross-tenant cascade collateral: requires a pre-existing cross-tenant FK reference, which row-level security prevents at insert time (a scoped write can't reference an invisible cross-tenant parent). Documented as a known limitation; a fail-closed cross-tenant-consistency pre-check is a reasonable future follow-up.
• Full catalog-derived plan rewrite: deferred — the reconciliation check delivers the same fail-closed safety invariant with far less risk in the most destructive command in the tree; a full rewrite is a sensible follow-up if imperative tables proliferate.
• Post-commit verification / single-transaction collapse: kept two-phase. Verifying inside one transaction would check uncommitted state; post-commit re-scan verifies committed state and is treated as an audit. Durable erasure requires quiescing the tenant first — a documented operator precondition, not something this offline DB operation can self-enforce.
• Command rename to agor db tenant-delete: declined; tenant/ matches the CLI's resource-topic convention and the stable contract is the stdout JSON, not the path.
• CI running the Postgres suite: pre-existing repo-wide *.postgres.test.ts gating; tracked as a separate follow-up (the suite was run against a real Postgres for this change).

[kgabryje]

Re-review after your push — big improvement; one safety-critical item left

The catalog-as-auditor + typed-manifest-as-executor split is a good resolution, and the fail-closed reconciliation genuinely closes the exhaustiveness gap (verified: tenant_id-without-RLS, forced-RLS-without-tenant_id, malformed column, non-public schema, and partitioning all abort before the delete loop; empty plan fails closed; kb_unit_embeddings is counted/deleted/verified when present, omitted when not; transitive classifier still aborts; hardening nits all landed). Thanks for the thorough turnaround.

One correctness item I'd hold the merge on, plus two cheap hardening items:

Must-fix: the RLS argument against cross-tenant cascade is unsound

"requires a pre-existing cross-tenant FK reference, which RLS prevents at insert time"

It doesn't. PostgreSQL referential-integrity checks bypass row-level security by design — the FK lookup on the parent runs with RI-trigger privileges, not the caller's RLS-scoped view. So a write carrying tenant_id = B can reference a tenant-A parent (the FK check will find A's row even though a normal scoped SELECT wouldn't), and privileged/BYPASSRLS/system writes and pre-existing rows are additional paths. The FK doesn't include tenant_id to prevent it — e.g. branches.repo_id → repos.repo_id ON DELETE CASCADE (schema.postgres.ts:670), FK on repo_id only.

Concretely during deletion: explicit tenant_id = A deletes A's branches, then A's repos; the DB cascade from the repo delete then removes any branch referencing that repo — including a tenant_id = B branch — and children-first ordering can't prevent it because that B-row was deliberately excluded from the explicit delete. For the most destructive command in the tree, "can delete another tenant's row" is worth closing rather than documenting.

Reachability is admittedly an anomalous cross-tenant reference (shouldn't arise in normal scoped operation), so if you'd rather scope it as a follow-up, the honest framing is "cross-tenant FK references are an unhandled integrity anomaly," not "RLS prevents them." Cleanest real fix: a pre-delete pg_constraint reconciliation that enumerates cascade edges and rejects cross-tenant references (which also closes the caveat below). Note the catalog audit currently reads pg_class/pg_namespace/pg_attribute/pg_policy/pg_inherits but not pg_constraint — so it proves relation-level tenant markers, not the live FK/cascade graph.

Cheap hardening

• Catalog reconciliation is outside the deletion transaction (auditLiveTenantCatalog(db, …) on the base connection, steps frozen before runWithTenantDatabaseScope opens). A lazily-created tenant table (kb_unit_embeddings is created on demand) appearing between audit and delete escapes, and phase-2 reuses the pre-audit steps rather than re-reading the catalog — so it can still certify success with data left behind. The quiescence precondition mostly covers this, but reconciling inside the same transaction (or re-auditing post-delete and failing if the live set changed) makes the guarantee real rather than precondition-dependent.
• Unqualified executed identifiers. The audit certifies public.kb_unit_embeddings but buildImperativeStep executes sql.identifier('kb_unit_embeddings') (and the Drizzle tables are likewise unqualified). A different earlier search_path schema with a same-named shadow relation would make count/delete/verify hit a different table than the one audited. Schema-qualify to public.<table>, or SET LOCAL search_path = pg_catalog, public inside the transaction.

Nit

cascadeParents in tenant-imperative-tables.ts is documentation-only — not validated against the live FK graph or used to derive ordering (imperative entries are placed first on the asserted-leaf claim). It's correct today (kb_unit_embeddings is a genuine leaf), but if you add the pg_constraint check above, wire this through it so the leaf assertion is verified rather than trusted.

Net: the exhaustiveness blocker is resolved. The cross-tenant cascade is the one I'd resolve-or-consciously-accept before merge; the TOCTOU + search_path items are cheap and worth folding in.

[Antonio-RiveroMartnez]

Thanks for the follow-up. You are correct that PostgreSQL referential-integrity checks and referential actions bypass RLS. I withdraw the earlier statement that RLS prevents a cross-tenant foreign-key reference from being created.

I pushed b653d4440cee52e4eb227839ffd7e8e64c11ceb6 with the changes that can be made safely and locally in this command:

Addressed

• Audited relation identity / search-path shadowing: all tenant-deletion count, delete, lock, and verification operations now target the audited public relations explicitly. Catalog access is qualified through pg_catalog, and the command uses a hardened transaction-local search path. A PostgreSQL regression test covers temporary-table shadowing.
• Catalog timing: phase two now performs an independent live-catalog reconciliation and rebuilds the verification plan. Relation, policy, and FK fingerprints must match phase one; catalog drift fails closed rather than allowing a stale plan to certify success. A deterministic PostgreSQL test creates a tenant-contract table between the phases and confirms failure.
• Policy semantics: reconciliation now validates the supported isolation-policy command, role, permissiveness, and normalized tenant-equality semantics, and rejects unexpected restrictive policies that could hide rows from deletion or verification. Tests cover a restrictive USING (false) policy and malformed/inverted policy semantics.
• Imperative-table metadata: removed the unused cascadeParents field rather than presenting a partial metadata check as proof of FK safety.
• Contract wording: CLI/module documentation now states the actual preconditions: the tenant must remain quiesced for the full operation, and the existing FK graph must be tenant-consistent. The wording no longer relies on RLS to establish FK consistency.

Validation at this commit: core and CLI typechecks passed; full SQLite suites passed (core 2912, CLI 21); the PostgreSQL 16 gated suite passed 11/11; Biome passed; the throwaway database containers were removed.

Not implemented: a deletion-local row-level pg_constraint scanner

The cross-tenant referential-action risk is real, but pg_constraint alone cannot detect it: the catalog describes FK columns and actions, not whether each existing parent/child pair has matching tenant ownership. A correct row-level preflight would have to:

1. inspect every row-affecting action (CASCADE, SET NULL, and SET DEFAULT), including runtime-created relations and composite keys;
2. read across tenant scopes despite forced RLS;
3. lock all participating relations in a deterministic order so a mismatched reference cannot be inserted after the scan; and
4. preserve those guarantees for every ordinary parent deletion path, since this is a database-wide referential-integrity property rather than a tenant-deletion ordering property.

Granting this runtime command cross-tenant/RLS-bypass visibility, or generating dynamic DML from arbitrary live constraints, would expand privilege and risk in the most destructive operation without establishing a durable invariant. A metadata-only scanner would be worse: it could appear to prove safety while missing the row-level mismatch.

Accordingly, this command now fails closed on catalog/contract drift and explicitly requires a tenant-consistent FK graph as an operator precondition. The durable structural remedy is tenant-inclusive foreign keys, accompanied by write-path validation and a migration of existing constraints/data. That is a database-wide schema change and is intentionally not being approximated with a partial scanner in this PR.

Phase separation

The two-phase design remains intentional. Phase one commits the deletion; phase two independently audits the live catalog and verifies committed state. Combining them would only verify uncommitted state. Concurrent target writes or schema/backfill activity remain outside the command contract and are why quiescence is required; the new phase-two fingerprint comparison adds defense in depth without claiming that an advisory convention can eliminate uncoordinated DDL.

[kgabryje]

Good to merge from my side

Verified this push: catalog access is fully pg_catalog-qualified and count/delete/verify now target the audited public relations explicitly (search-path shadowing closed); phase two performs an independent live-catalog reconciliation and fails closed if the relation/policy/FK fingerprint drifts from phase one (TOCTOU closed); policy-semantics validation now covers command/role/permissiveness + normalized tenant-equality and rejects restrictive policies that could hide rows; unused cascadeParents dropped; docs no longer lean on RLS for FK consistency. Tests cover the temp-table shadow, between-phase table creation, and restrictive/inverted policies. Thanks for the careful iteration.

On the cross-tenant referential-action risk — I agree with your call to not build a deletion-local row-level scanner, and your reasoning is more precise than my suggestion: pg_constraint gives the FK edges/actions (which you now fingerprint for drift), but detecting an actual cross-tenant row reference needs cross-scope row reads — i.e. RLS bypass — which is exactly the privilege you shouldn't grant the most destructive command in the tree, and a metadata-only scanner would falsely imply safety. Failing closed on catalog/contract drift + documenting a tenant-consistent FK graph as an operator precondition is the right boundary; the durable fix (tenant-inclusive FKs + write-path validation + constraint/data migration) is correctly scoped as a separate DB-wide change.

Residual, now honestly documented rather than claimed away: this command assumes (a) the tenant stays quiesced for the full operation and (b) the existing FK graph is tenant-consistent. Both are the right preconditions for an offline DB-owner erasure tool. Worth a tracked follow-up for the tenant-inclusive-FK structural remedy.

No blocking items left from the review. 👍

[kgabryje]

Lets goo!