Deploy safety: staging tier, real smoke test, destructive-migration gate, failing health checks - #2125
Deploy safety: staging tier, real smoke test, destructive-migration gate, failing health checks#21252witstudios wants to merge 4 commits into
Conversation
Adds a migration-safety CI job that fails a PR when a newly added packages/db/drizzle/*.sql migration contains a destructive statement (DROP TABLE/COLUMN/TYPE, TRUNCATE, a column type change, an enum-swap rename, or a NOT NULL column added without a DEFAULT) and doesn't carry a leading `-- destructive-migration-ack: <reason>` comment. Scope is per-file, newly-added-files-only — existing migrations are never retroactively flagged. Part of the deploy-safety audit (tasks/solo-tells-audit-2026-07-17.md section 6): forward-only destructive migrations currently run before old app code retires with no expand/contract discipline enforced.
/api/health previously always returned HTTP 200 even when the database check failed — Fly's rolling-deploy health check and any external uptime monitor could never detect a degraded instance. Require 3 consecutive DB failures before flipping the HTTP status to 503, so one transient blip (e.g. a slow reconnect right as the grace period elapses) doesn't fail a healthy rolling deploy. Monitoring misconfiguration stays a 200-with-warning — it's a config issue, not a traffic-serving failure, and shouldn't cycle machines. Part of the deploy-safety audit (tasks/solo-tells-audit-2026-07-17.md section 6): "Health checks can't fail."
Replaces the direct build -> deploy-prod pipeline with:
build-and-push -> deploy-staging -> smoke-test -> deploy-fly (prod)
- Every deploy (staging and prod) now pulls an immutable sha-<short-sha>
tag instead of the mutable :latest — the artifact smoke-tested in
staging is bit-for-bit the one promoted to prod.
- deploy-staging deploys pagespace-web to a new pagespace-web-staging
Fly app (config in PageSpace-Deploy, fly.web.staging.toml) and runs
migrations against an isolated staging database first.
- smoke-test polls the deployed instance's /api/health until it
reports status=healthy AND checks.database=connected (not just "the
process answers HTTP") before prod is touched at all.
- Extracted the ~200 lines of inline migration-polling and
pull/tag/push/deploy bash that were duplicated per-service into
scripts/deploy/{run-fly-migration,deploy-fly-service,smoke-test}.sh,
parameterized by app/image/tag, so staging and prod share one
implementation instead of drifting.
Part of the deploy-safety audit (tasks/solo-tells-audit-2026-07-17.md
section 6): "no staging env at all... no post-deploy smoke test...
prod tracks mutable :latest."
Requires manual owner setup before this can pass — see the PR
description checklist (new Fly app, staging DB, fly-staging GitHub
environment).
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 903c28e073
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| !/\bDEFAULT\b/i.test(s) && | ||
| !/\b(BIG)?SERIAL\b/i.test(s), // serial/bigserial self-populate via an implicit sequence default |
There was a problem hiding this comment.
When a destructive migration includes an explanatory comment containing DEFAULT—for example, -- table is empty, so no DEFAULT is needed before ADD COLUMN ... NOT NULL—the check treats that comment as part of the statement, DEFAULT matches here, and the migration passes without an acknowledgement. Because the same problem applies to SERIAL, comments and ideally string literals must be removed before applying these exemption patterns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 15203ac: added stripSqlComments() (strips -- and /* */ comments) and now run it on each statement before testing the DEFAULT/SERIAL exemption regexes — a comment mentioning DEFAULT/SERIAL can no longer suppress a real destructive match. Added regression tests for both cases plus a comment-only-DROP-TABLE false-positive case.
| import { execFileSync } from 'node:child_process'; | ||
| import { readFileSync } from 'node:fs'; | ||
|
|
||
| const MIGRATIONS_DIR = 'packages/db/drizzle'; |
There was a problem hiding this comment.
The production workflow conditionally runs db:migrate:admin, whose migrations come from packages/db/drizzle-admin, but this gate searches only packages/db/drizzle. Once ADMIN_DB_MIGRATIONS_ENABLED=true, a newly added destructive admin migration can therefore reach the production trust-plane database without the acknowledgement required for equivalent main-database changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 15203ac: MIGRATIONS_DIRS now includes packages/db/drizzle-admin alongside packages/db/drizzle, and addedMigrationFiles() scans both. Verified against bun scripts/check-destructive-migrations.ts master locally.
| docker pull "$FULL_GHCR_IMAGE" | ||
| docker tag "$FULL_GHCR_IMAGE" "$FLY_IMAGE" | ||
| docker push "$FLY_IMAGE" | ||
| flyctl deploy --app "$FLY_APP" --image "$FLY_IMAGE" --wait-timeout "$WAIT_TIMEOUT" |
There was a problem hiding this comment.
These new helper scripts are executed directly from the checked-out commit, but .github/workflows/docker-images.yml limits push triggers to apps/**, packages/**, docker/**, bun.lock, and the workflow itself. Consequently, merging a fix that changes only scripts/deploy/** will not run the deployment pipeline, leaving the fix unapplied until an unrelated matching change is merged.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 15203ac: added scripts/deploy/** to the docker-images.yml push paths filter.
- P1: strip SQL comments before testing for a DEFAULT/SERIAL exemption. A migration whose explanatory comment happens to contain the word "DEFAULT" or "SERIAL" (e.g. "-- table is empty, no DEFAULT needed") was satisfying the regex and letting a genuinely destructive ADD COLUMN ... NOT NULL through unacknowledged. Added stripSqlComments() and apply it per-statement before running DESTRUCTIVE_CHECKS (comments still count for ACK_PATTERN, which needs the real ack comment). - P2: the gate only scanned packages/db/drizzle, missing the drizzle-admin trust-plane migrations that run via db:migrate:admin once ADMIN_DB_MIGRATIONS_ENABLED=true. Now scans both directories. - P2 (docker-images.yml): added scripts/deploy/** to the push path filter — those scripts run directly from the checked-out commit, not baked into any image, so a fix there previously wouldn't trigger the deploy pipeline until an unrelated apps/packages change also landed.
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Summary
Implements section 6 of the deploy-safety audit (
tasks/solo-tells-audit-2026-07-17.md):Four independent pieces, each committed separately:
/api/healthreturns 503 on sustained DB outage (fix(health)commit) — itpreviously always returned HTTP 200 even when the DB check failed, so Fly's rolling
deploy and any uptime monitor could never detect a degraded instance. Requires 3
consecutive failures before flipping to 503, so one transient blip doesn't fail a
healthy rolling deploy.
Destructive-migration CI gate (
feat(ci): destructive-migrationcommit) — a newmigration-safetyjob fails a PR when a newly-addedpackages/db/drizzle*/*.sqlmigration contains a destructive statement (DROP TABLE/COLUMN/TYPE, TRUNCATE, a
column type change, an enum-swap rename, or
NOT NULLadded without aDEFAULT)without a leading
-- destructive-migration-ack: <reason>comment. Per-file,newly-added-files-only — never retroactive. Covers both
packages/db/drizzleandthe
drizzle-admintrust-plane migrations.Staging tier + real post-deploy smoke test + sha-pinned deploys
(
feat(ci): staging tiercommit) —docker-images.ymlis nowbuild-and-push -> deploy-staging -> smoke-test -> deploy-fly (prod). Every deploypulls an immutable
sha-<short-sha>tag instead of:latest, so the exact artifactsmoke-tested in staging is the one promoted to prod.
smoke-testpolls/api/healthuntil it reportsstatus: healthyANDchecks.database: connected—if it doesn't, prod is never touched. The ~200 lines of duplicated
migration-polling/deploy bash were extracted into
scripts/deploy/{run-fly-migration,deploy-fly-service,smoke-test}.shso staging andprod share one implementation.
Fixed the silent migration-skip footgun — companion PR in
PageSpace-Deploy:2witstudios/PageSpace-Deploy#19.
deploy-fly.sh'srun_migrations()used to scrapeDATABASE_URLout offlyctl secrets list(which never prints values) and silentlyskip migrations on an empty scrape. Also adds
fly.web.staging.tomlfor the newstaging app.
Review round 1 (Codex) — fixed
check-destructive-migrations.tstreated SQL comments as live code, so acomment mentioning "DEFAULT" or "SERIAL" could suppress a real destructive match.
Fixed with
stripSqlComments()applied per-statement before the exemption checks;added regression tests.
packages/db/drizzle, missing thedrizzle-admintrust-plane migrations. Now scans both.
docker-images.yml's push-path filter didn't includescripts/deploy/**,so a fix to those helper scripts alone wouldn't trigger the pipeline. Added.
All three replied to inline and fixed in
15203ac80— see thread replies for detail.Owner action required (this PR's CI will not pass end-to-end until these are done)
flyctl apps create pagespace-web-stagingpagespace_stagingdatabase + role on the existingpagespace-dbmachine (exact commands inPageSpace-Deploy/fly/FLY.md→"Staging Environment")
flyctl secrets set --app pagespace-web-staging DATABASE_URL='...' CRON_SECRET='...'(mirror-then-deploy steps in
PageSpace-Deploy/fly/FLY.md)fly-stagingGitHub environment on this repo with aFLY_API_TOKENsecret (can reuse the
fly-productiontoken, or scope a tighter one to justpagespace-web-staging)Scope notes / deliberate non-goals
pagespace-web(+ its own DB). No stagingrealtime/processor/
admin/cron/marketing/proxy— those aren't on the migration hot paththis exists to protect; extending parity is a follow-up, not assumed here.
so
NEXT_PUBLIC_*values still point atpagespace.ai. This validates backend/DB/migration behavior, not full user-facing UI staging.
Test plan
scripts/check-destructive-migrations.ts— 24 unit tests, all passing (incl.comment-stripping and drizzle-admin regression cases added in review round 1)
apps/web/src/app/api/health/__tests__/route.test.ts— 13 unit tests, all passing(including the new debounce/recovery cases)
scripts/deploy/smoke-test.shmanually exercised against local fake HTTP servers:healthy response (pass), degraded response (fail + retries), connection-refused
(fail + retries)
.github/workflows/docker-images.ymlvalidated as well-formed YAML with theexpected job dependency graph (
ci,build-and-push→deploy-staging→smoke-test→deploy-fly)Migration Check, CodeQL, Secret Scanning, Dependency Audit, Static Security Analysis)
mastercanexercise
deploy-staging/smoke-test/deploy-flyfor real🤖 Generated with Claude Code
https://claude.ai/code/session_01RK2U2tc6buCvpJWCs49mtN