You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Deferred follow-ups from the pr-review-toolkit pass on #2941 / PR #2988 (restore-into-scratch-DB backup verification). None block #2988; they're polish/consistency items intentionally kept out of that PR.
Items
Single-source the VerifyLevel union."full-restore" | "header-only" is currently declared in ~5 places (wire type in @useatlas/types, the zod schema, ee/src/backups/verify.ts, the BackupsManager Tag literal in services.ts, and the route's z.enum). Mirror the existing BACKUP_STATUSES pattern: export VERIFY_LEVELS = ["full-restore","header-only"] as const from @useatlas/types and derive everything from it. Note the ordering trap: a new value export in @useatlas/types breaks Scaffold CI until types is published — do the publish-then-bump-refs dance (see CLAUDE.md 'Publishing @useatlas/* packages' + the scaffold-symbols gate).
Tagged errors for the verify route's 404/400 mapping.packages/api/src/api/routes/platform-backups.ts classifies verify failures via message.includes(...) to pick 404 vs 400 vs 500 — the same string-matching anti-pattern the codebase bans for EnterpriseError. Introduce tagged errors (e.g. BackupNotFound / BackupInvalidState) so the mapping is structural. (Pre-existing; surfaced during the fix(ee/backups): restore-into-scratch-DB backup verification (#2941) #2988 review.)
DB CHECK constraint on verify_level — CHECK (verify_level IN ('full-restore','header-only') OR verify_level IS NULL) to push the enum invariant into storage. Optional given the ad-hoc-bootstrap table design.
Render verifyLevel in the platform backups UI.packages/web/.../platform/backups/page.tsx parses the new field but the table doesn't display it. Surface a 'Verified (full-restore)' vs 'Verified (header-only)' badge so admins can see verification depth.
Fail-fast at backup creation (optional).createBackup marks completed on pg_dump exit 0 + non-empty file with no inline content sanity check. The scheduler now verifies right after, so it's covered operationally, but consider a minimal create-time sanity check (e.g. non-zero expected rows/tables) so creation fails fast independent of verify.
(Conditional) Expected-table-count comparison. IF PR fix(ee/backups): restore-into-scratch-DB backup verification (#2941) #2988 ships only the BASE TABLE count filter (not the record-expected-count-at-backup-and-compare approach), track the stronger restorability proof here: persist the source DB's BASE TABLE count at createBackup and assert restored >= expected at verify, to catch clean-statement-boundary truncation that exits psql 0 with a non-zero-but-incomplete count.
Context
Deferred follow-ups from the pr-review-toolkit pass on #2941 / PR #2988 (restore-into-scratch-DB backup verification). None block #2988; they're polish/consistency items intentionally kept out of that PR.
Items
Single-source the
VerifyLevelunion."full-restore" | "header-only"is currently declared in ~5 places (wire type in@useatlas/types, the zod schema,ee/src/backups/verify.ts, theBackupsManagerTag literal inservices.ts, and the route'sz.enum). Mirror the existingBACKUP_STATUSESpattern: exportVERIFY_LEVELS = ["full-restore","header-only"] as constfrom@useatlas/typesand derive everything from it. Note the ordering trap: a new value export in@useatlas/typesbreaks Scaffold CI until types is published — do the publish-then-bump-refs dance (see CLAUDE.md 'Publishing @useatlas/* packages' + the scaffold-symbols gate).Tagged errors for the verify route's 404/400 mapping.
packages/api/src/api/routes/platform-backups.tsclassifies verify failures viamessage.includes(...)to pick 404 vs 400 vs 500 — the same string-matching anti-pattern the codebase bans forEnterpriseError. Introduce tagged errors (e.g.BackupNotFound/BackupInvalidState) so the mapping is structural. (Pre-existing; surfaced during the fix(ee/backups): restore-into-scratch-DB backup verification (#2941) #2988 review.)DB CHECK constraint on
verify_level—CHECK (verify_level IN ('full-restore','header-only') OR verify_level IS NULL)to push the enum invariant into storage. Optional given the ad-hoc-bootstrap table design.Render
verifyLevelin the platform backups UI.packages/web/.../platform/backups/page.tsxparses the new field but the table doesn't display it. Surface a 'Verified (full-restore)' vs 'Verified (header-only)' badge so admins can see verification depth.Fail-fast at backup creation (optional).
createBackupmarkscompletedon pg_dump exit 0 + non-empty file with no inline content sanity check. The scheduler now verifies right after, so it's covered operationally, but consider a minimal create-time sanity check (e.g. non-zero expected rows/tables) so creation fails fast independent of verify.(Conditional) Expected-table-count comparison. IF PR fix(ee/backups): restore-into-scratch-DB backup verification (#2941) #2988 ships only the
BASE TABLEcount filter (not the record-expected-count-at-backup-and-compare approach), track the stronger restorability proof here: persist the source DB's BASE TABLE count atcreateBackupand assertrestored >= expectedat verify, to catch clean-statement-boundary truncation that exits psql 0 with a non-zero-but-incomplete count.Dependencies