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 7 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).
📝 WalkthroughWalkthroughThe change adds destructive-migration checks to CI, centralizes SHA-based Fly deployments and migrations, introduces staged promotion with smoke testing, and debounces health endpoint failures with a three-failure threshold. ChangesRelease safety and deployment flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DockerImagesWorkflow
participant run-fly-migration.sh
participant FlyMachinesAPI
participant smoke-test.sh
participant deploy-fly-service.sh
DockerImagesWorkflow->>run-fly-migration.sh: Run staging migration
run-fly-migration.sh->>FlyMachinesAPI: Start and poll migration machine
FlyMachinesAPI-->>run-fly-migration.sh: Return migration state and exit code
DockerImagesWorkflow->>smoke-test.sh: Check staging health
smoke-test.sh-->>DockerImagesWorkflow: Return smoke-test status
DockerImagesWorkflow->>deploy-fly-service.sh: Deploy production SHA image
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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.
|
# Conflicts: # .github/workflows/ci.yml # .github/workflows/docker-images.yml # scripts/vitest.config.ts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/app/api/health/route.ts (1)
110-137: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe catch block reuses the database counter for non-database errors and can throw again.
Two problems in this handler:
Line 112 increments
consecutiveDbFailuresfor any unexpected error, not only database errors.checkDatabase()at line 51 may have already succeeded and reset the counter to0in the same request. IfgetPoolStats()orgetMonitoringIngestStatus()then throws, the counter climbs, and after three such requests the route returns 503 while the database is connected. Fly cycles a healthy machine as a result. Track handler errors in a separate counter and gate the status on either counter.Lines 121 and 128 call
getMonitoringIngestStatus()andgetPoolStats()inside the catch. If the original throw came fromgetPoolStats()at line 79, the same call throws again here. The second throw escapes the handler, so the route returns a framework 500 with a non-JSON body.scripts/deploy/smoke-test.shreads.statusand.checks.databasefrom that body and finds neither, so the staging gate fails without a useful signal. Wrap both calls in a safe fallback.🐛 Proposed fix
+let consecutiveHandlerFailures = 0; + const checkDatabase = async (): Promise<boolean> => {} catch (error) { loggers.api.error('Health check failed', error as Error); - consecutiveDbFailures += 1; + consecutiveHandlerFailures += 1; + + let monitoring: HealthResponse['checks']['monitoring'] = 'misconfigured'; + try { + monitoring = getMonitoringIngestStatus(); + } catch { + // getMonitoringIngestStatus itself may be the failing call. + } + + let pool: HealthResponse['pool'] = { total: 0, idle: 0, waiting: 0 }; + try { + pool = getPoolStats(); + } catch { + // getPoolStats itself may be the failing call. + } const response: HealthResponse = { status: 'degraded', service: 'pagespace-web', version: process.env.npm_package_version || '0.0.0', timestamp: new Date().toISOString(), checks: { database: 'disconnected', - monitoring: getMonitoringIngestStatus(), + monitoring, }, memory: { heapUsed: 0, heapTotal: 0, rss: 0, }, - pool: getPoolStats(), + pool, error: 'Health check failed unexpectedly', }; return Response.json(response, { - status: consecutiveDbFailures >= CONSECUTIVE_FAILURE_THRESHOLD ? 503 : 200, + status: consecutiveHandlerFailures >= CONSECUTIVE_FAILURE_THRESHOLD ? 503 : 200, headers: { 'Cache-Control': 'no-store, no-cache, must-revalidate', }, }); }Reset
consecutiveHandlerFailuresto0on the success path, next to theResponse.jsoncall at line 104.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/api/health/route.ts` around lines 110 - 137, Update the health handler catch path to track unexpected handler failures with a separate consecutiveHandlerFailures counter, incrementing it only for non-database errors and resetting it on the success path near the existing Response.json return. Gate the degraded response status on either consecutiveDbFailures or consecutiveHandlerFailures reaching the threshold. Wrap getMonitoringIngestStatus and getPoolStats in safe fallbacks within the catch so a secondary throw cannot escape and the response remains valid JSON.
🧹 Nitpick comments (2)
apps/web/src/app/api/health/__tests__/route.test.ts (1)
136-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the unexpected-error path.
These tests pin the database-failure threshold well. The
catchblock inapps/web/src/app/api/health/route.tsat lines 110-137 also gained threshold logic, and no test exercises it. A test that makes a non-database call throw would pin the counter semantics and the JSON shape of the fallback response. It would also lock in the fix for the separate handler-error counter raised onroute.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/api/health/__tests__/route.test.ts` around lines 136 - 171, Add a health-route test covering an unexpected non-database error by making the relevant handler dependency throw, then verify the fallback response JSON shape and status across the threshold, including that the separate handler-error counter resets or recovers as intended. Anchor the test alongside the existing GET failure tests and exercise the catch path in GET rather than the database-failure path.scripts/check-destructive-migrations.ts (1)
135-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
import.meta.mainfor the direct-execution guard.This script runs with Bun in CI.
import.meta.mainis Bun’s entrypoint check and avoids false matches from the current filename suffix check.♻️ Proposed refactor
-// Only run if executed directly (not imported by tests) -if (typeof process !== 'undefined' && process.argv[1]?.endsWith('check-destructive-migrations.ts')) { - main(); -} +// Only run if executed directly (not imported by tests) +if (import.meta.main) { + main(); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-destructive-migrations.ts` around lines 135 - 138, Replace the filename-based direct-execution guard around main with Bun’s import.meta.main check, while preserving the existing behavior of invoking main only when the script is executed directly and not when imported.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/docker-images.yml:
- Around line 143-148: Add explicit least-privilege permissions to the
deploy-staging and smoke-test jobs: configure deploy-staging with contents read
and the package scope required to pull from GHCR, and configure smoke-test with
contents read only.
In `@scripts/deploy/run-fly-migration.sh`:
- Around line 72-106: Update the migration polling loop around the STATE
handling to read events[].request.exit_event.exit_code from the successful API
response. Treat stopped as successful only when the exit code is exactly 0; when
it is missing or non-zero, print the machine logs, destroy it, and exit 1. Keep
the existing handling for failed/destroyed states, and emit “Migrations
complete” only after a verified zero exit code.
- Around line 49-70: Update the machine-ID failure branch after the `MACHINE_ID`
lookup to query the Fly Machines API by `$MACHINE_NAME` and obtain the created
machine’s ID, then destroy that machine with `flyctl machine destroy` before
exiting. Preserve the existing diagnostic output and exit behavior, using the
API fallback only when parsing `Machine ID` from `$OUTPUT_FILE` fails.
In `@scripts/deploy/smoke-test.sh`:
- Around line 29-35: Update the smoke-test health condition to require a
successful HTTP response and DATABASE=connected without requiring
STATUS=healthy, so monitoring misconfiguration does not block deployment.
Preserve the existing passed output and successful exit behavior, and optionally
report the non-healthy status as a warning.
---
Outside diff comments:
In `@apps/web/src/app/api/health/route.ts`:
- Around line 110-137: Update the health handler catch path to track unexpected
handler failures with a separate consecutiveHandlerFailures counter,
incrementing it only for non-database errors and resetting it on the success
path near the existing Response.json return. Gate the degraded response status
on either consecutiveDbFailures or consecutiveHandlerFailures reaching the
threshold. Wrap getMonitoringIngestStatus and getPoolStats in safe fallbacks
within the catch so a secondary throw cannot escape and the response remains
valid JSON.
---
Nitpick comments:
In `@apps/web/src/app/api/health/__tests__/route.test.ts`:
- Around line 136-171: Add a health-route test covering an unexpected
non-database error by making the relevant handler dependency throw, then verify
the fallback response JSON shape and status across the threshold, including that
the separate handler-error counter resets or recovers as intended. Anchor the
test alongside the existing GET failure tests and exercise the catch path in GET
rather than the database-failure path.
In `@scripts/check-destructive-migrations.ts`:
- Around line 135-138: Replace the filename-based direct-execution guard around
main with Bun’s import.meta.main check, while preserving the existing behavior
of invoking main only when the script is executed directly and not when
imported.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f749412-6b56-4b78-ac44-257dac183447
📒 Files selected for processing (10)
.github/workflows/ci.yml.github/workflows/docker-images.ymlapps/web/src/app/api/health/__tests__/route.test.tsapps/web/src/app/api/health/route.tspackages/lib/src/realtime/conversation-event-names.tsscripts/__tests__/check-destructive-migrations.test.tsscripts/check-destructive-migrations.tsscripts/deploy/deploy-fly-service.shscripts/deploy/run-fly-migration.shscripts/deploy/smoke-test.sh
💤 Files with no reviewable changes (1)
- packages/lib/src/realtime/conversation-event-names.ts
| deploy-staging: | ||
| name: Deploy to Staging | ||
| needs: [build-and-push, ci] | ||
| runs-on: ubuntu-latest | ||
| environment: fly-production | ||
| environment: fly-staging | ||
| if: github.ref == 'refs/heads/master' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Add an explicit permissions: block to deploy-staging.
The job has no permissions: block, so it inherits the repository default token scope. The job only needs to read the repository and pull from GHCR. smoke-test needs even less. Set least privilege explicitly on both new jobs.
🔒️ Proposed change
deploy-staging:
name: Deploy to Staging
needs: [build-and-push, ci]
runs-on: ubuntu-latest
environment: fly-staging
+ permissions:
+ contents: read
+ packages: read
if: github.ref == 'refs/heads/master'Apply the same pattern to smoke-test with contents: read only.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deploy-staging: | |
| name: Deploy to Staging | |
| needs: [build-and-push, ci] | |
| runs-on: ubuntu-latest | |
| environment: fly-production | |
| environment: fly-staging | |
| if: github.ref == 'refs/heads/master' | |
| deploy-staging: | |
| name: Deploy to Staging | |
| needs: [build-and-push, ci] | |
| runs-on: ubuntu-latest | |
| environment: fly-staging | |
| permissions: | |
| contents: read | |
| packages: read | |
| if: github.ref == 'refs/heads/master' |
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 143-185: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/docker-images.yml around lines 143 - 148, Add explicit
least-privilege permissions to the deploy-staging and smoke-test jobs: configure
deploy-staging with contents read and the package scope required to pull from
GHCR, and configure smoke-test with contents read only.
Source: Linters/SAST tools
| flyctl machine run "$FLY_IMAGE" \ | ||
| --app "$FLY_APP" \ | ||
| --name "$MACHINE_NAME" \ | ||
| --region iad \ | ||
| --restart no \ | ||
| "${EXTRA_ARGS[@]}" \ | ||
| 2>&1 | tee "$OUTPUT_FILE" || true | ||
|
|
||
| MACHINE_ID=$(grep -oP 'Machine ID: \K[0-9a-f]+' "$OUTPUT_FILE" | head -1) | ||
| if [ -z "$MACHINE_ID" ]; then | ||
| echo "ERROR: could not parse machine ID from flyctl output" >&2 | ||
| cat "$OUTPUT_FILE" >&2 | ||
| exit 1 | ||
| fi | ||
| echo "Migration machine: $MACHINE_ID" | ||
|
|
||
| if grep -q "Error:" "$OUTPUT_FILE" && ! grep -q "desired start state" "$OUTPUT_FILE"; then | ||
| echo "ERROR: unexpected flyctl error:" >&2 | ||
| grep "Error:" "$OUTPUT_FILE" >&2 | ||
| flyctl machine destroy "$MACHINE_ID" --app "$FLY_APP" --force 2>/dev/null || true | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
flyctl machine run --json output machine id
💡 Result:
To retrieve the Machine ID when running the flyctl machine run command, you should use the --json flag. While the command's primary purpose is to launch a new machine, adding --json will cause flyctl to output the machine's configuration and metadata in JSON format, which includes the machine's unique identifier (the id field) [1][2][3]. Example usage: fly machine run --json The command will output a JSON object upon success, where the machine ID can be parsed. For example, if you use a tool like jq to process the output, you can extract the ID specifically: fly machine run --json | jq -r '.id' Note that flyctl machine run does not provide a dedicated flag to output only the machine ID (unlike fly machine list, which has a --quiet or -q flag) [4]. The --json flag is the standard way to programmatically access machine details, including the ID, immediately after creation [2]. The internal flyctl logic explicitly prints the Machine ID to standard output upon success [3], and using --json ensures this data is structured for parsing [2].
Citations:
- 1: https://fly.io/docs/flyctl/integrating/
- 2: https://fly.io/blog/flyctl-meets-json/
- 3: https://github.com/superfly/flyctl/blob/master/internal/command/machine/run.go
- 4: https://fly.io/docs/flyctl/machine-list/
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | grep -E '(^|/)(run-fly-migration\.sh|.*fly.*ya?ml|.*workflow.*)$' | head -80
printf '%s\n' '--- script ---'
script=$(git ls-files | grep 'scripts/deploy/run-fly-migration\.sh$' | head -1)
if [ -n "$script" ]; then
cat -n "$script"
fi
printf '%s\n' '--- flyctl setup references ---'
rg -n -C 3 'setup-flyctl|flyctl machine run|MACHINE_NAME|OUTPUT_FILE|machine destroy' .github scripts 2>/dev/null | head -240Repository: 2witstudios/PageSpace
Length of output: 13876
🏁 Script executed:
#!/bin/bash
set -eu
tmp=$(mktemp)
trap 'rm -f "$tmp"' EXIT
curl -fsSL https://raw.githubusercontent.com/superfly/flyctl/master/internal/command/machine/run.go -o "$tmp"
printf '%s\n' '--- machine run JSON and ID handling ---'
rg -n -C 5 'json|Machine ID|machine ID|Output|Format' "$tmp" | head -240
printf '%s\n' '--- workflow invocation context ---'
rg -n -C 8 'run-fly-migration|Setup flyctl|setup-flyctl' .github/workflows scriptsRepository: 2witstudios/PageSpace
Length of output: 10776
🏁 Script executed:
#!/bin/bash
set -eu
tmp=$(mktemp)
trap 'rm -f "$tmp"' EXIT
curl -fsSL https://raw.githubusercontent.com/superfly/flyctl/master/internal/command/machine/run.go -o "$tmp"
printf '%s\n' '--- run command flags ---'
sed -n '1,220p' "$tmp"
printf '%s\n' '--- all JSON references and output paths ---'
rg -ni -C 4 'json|format|output|machine id' "$tmp" | head -300
printf '%s\n' '--- repository-wide JSON flag declarations in flyctl ---'
curl -fsSL 'https://api.github.com/repos/superfly/flyctl/git/trees/master?recursive=1' |
jq -r '.tree[] | select(.type=="blob" and (.path|test("\\.go$"))) | .path' |
while read -r path; do
body=$(curl -fsSL "https://raw.githubusercontent.com/superfly/flyctl/master/$path" 2>/dev/null || true)
if printf '%s' "$body" | rg -q 'Name:\s*["'\"'\"']json["'\"'\"']|--json'; then
printf '%s\n' "$path"
printf '%s' "$body" | rg -n -C 2 'Name:\s*["'\"'\"']json["'\"'\"']|--json' | head -40
fi
doneRepository: 2witstudios/PageSpace
Length of output: 9471
Handle machine-ID lookup failures before exiting.
flyctl machine run emits Machine ID as human-readable output, and flyctl is installed from the unpinned superfly/flyctl-actions/setup-flyctl@master. A future output change can leave a created machine orphaned because lines 58–61 exit without cleanup. Look up the machine by $MACHINE_NAME through the Machines API and destroy it when parsing fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/deploy/run-fly-migration.sh` around lines 49 - 70, Update the
machine-ID failure branch after the `MACHINE_ID` lookup to query the Fly
Machines API by `$MACHINE_NAME` and obtain the created machine’s ID, then
destroy that machine with `flyctl machine destroy` before exiting. Preserve the
existing diagnostic output and exit behavior, using the API fallback only when
parsing `Machine ID` from `$OUTPUT_FILE` fails.
| STATUS=$(jq -r '.status // ""' "$RESPONSE_FILE" 2>/dev/null || echo "") | ||
| DATABASE=$(jq -r '.checks.database // ""' "$RESPONSE_FILE" 2>/dev/null || echo "") | ||
|
|
||
| if [ "$STATUS" = "healthy" ] && [ "$DATABASE" = "connected" ]; then | ||
| echo "Smoke test PASSED: $HEALTH_URL -> 200, status=healthy, database=connected" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
status=healthy also requires monitoring to be configured, which can block production permanently.
In apps/web/src/app/api/health/route.ts line 63, isHealthy is dbHealthy && monitoringStatus !== 'misconfigured'. The route reports status: 'degraded' with HTTP 200 when MONITORING_INGEST_KEY is unset and MONITORING_INGEST_DISABLED is not 'true', even though the database is connected.
The new staging app is provisioned as part of this change. If its monitoring env vars are not set, this smoke test never observes status=healthy, smoke-test fails on every run, and deploy-fly is skipped for every merge to master.
Two options:
- Set
MONITORING_INGEST_KEYorMONITORING_INGEST_DISABLED=trueonpagespace-web-stagingas part of owner setup, and record it in the runbook. - Or gate the smoke test on the database check and the HTTP status only, and treat monitoring misconfiguration as a warning.
🛠️ Option 2 diff
if [ "$HTTP_CODE" = "200" ]; then
STATUS=$(jq -r '.status // ""' "$RESPONSE_FILE" 2>/dev/null || echo "")
DATABASE=$(jq -r '.checks.database // ""' "$RESPONSE_FILE" 2>/dev/null || echo "")
+ MONITORING=$(jq -r '.checks.monitoring // ""' "$RESPONSE_FILE" 2>/dev/null || echo "")
- if [ "$STATUS" = "healthy" ] && [ "$DATABASE" = "connected" ]; then
- echo "Smoke test PASSED: $HEALTH_URL -> 200, status=healthy, database=connected"
+ if [ "$DATABASE" = "connected" ]; then
+ if [ "$STATUS" != "healthy" ]; then
+ echo "WARNING: database connected but status=$STATUS (monitoring=$MONITORING)"
+ fi
+ echo "Smoke test PASSED: $HEALTH_URL -> 200, database=connected"
exit 0
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| STATUS=$(jq -r '.status // ""' "$RESPONSE_FILE" 2>/dev/null || echo "") | |
| DATABASE=$(jq -r '.checks.database // ""' "$RESPONSE_FILE" 2>/dev/null || echo "") | |
| if [ "$STATUS" = "healthy" ] && [ "$DATABASE" = "connected" ]; then | |
| echo "Smoke test PASSED: $HEALTH_URL -> 200, status=healthy, database=connected" | |
| exit 0 | |
| fi | |
| STATUS=$(jq -r '.status // ""' "$RESPONSE_FILE" 2>/dev/null || echo "") | |
| DATABASE=$(jq -r '.checks.database // ""' "$RESPONSE_FILE" 2>/dev/null || echo "") | |
| MONITORING=$(jq -r '.checks.monitoring // ""' "$RESPONSE_FILE" 2>/dev/null || echo "") | |
| if [ "$DATABASE" = "connected" ]; then | |
| if [ "$STATUS" != "healthy" ]; then | |
| echo "WARNING: database connected but status=$STATUS (monitoring=$MONITORING)" | |
| fi | |
| echo "Smoke test PASSED: $HEALTH_URL -> 200, database=connected" | |
| exit 0 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/deploy/smoke-test.sh` around lines 29 - 35, Update the smoke-test
health condition to require a successful HTTP response and DATABASE=connected
without requiring STATUS=healthy, so monitoring misconfiguration does not block
deployment. Preserve the existing passed output and successful exit behavior,
and optionally report the non-healthy status as a warning.
infrastructure/__tests__/ci-admin-migrate.test.ts pinned the admin migration step's OLD inline-bash shape (flyctl machine run inline, pagespace-migrate:latest, inline failed/destroyed polling) from before this branch extracted that logic into scripts/deploy/run-fly-migration.sh and moved off the mutable :latest tag. Master added this test after this branch diverged, so the conflict never surfaced as a textual merge conflict — it only showed up as a CI failure post-merge. Updated the assertions to verify the new architecture instead of reverting the sha-pinning fix that's the actual point of this PR: - both the main and admin migration steps delegate to the same scripts/deploy/run-fly-migration.sh (still asserts admin mirrors main's failure-handling, now by shared implementation rather than duplicated inline bash) - the shared script itself contains the flyctl machine run + failed/ destroyed polling this test cares about - both steps reference the same ghcr.io/2witstudios/pagespace-migrate image, and neither pins the mutable :latest tag
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@infrastructure/__tests__/ci-admin-migrate.test.ts`:
- Around line 60-62: Replace the source-text assertions around
RUN_FLY_MIGRATION_SCRIPT with an executable test that runs the script using
mocked Fly API responses for failed and destroyed machine states. Assert the
script exits non-zero and performs machine cleanup, so comments or log messages
cannot satisfy the test.
- Around line 64-73: Update the migration image assertions in the admin/main
migration test to extract and compare the full tag argument, not just the
pagespace-migrate image prefix. Assert both steps use sha-${GITHUB_SHA::7} and
reject the actual latest argument form, using the existing adminStep and
steps[mainIdx] run values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 62225ab2-054b-49c0-96d0-d73719656fc3
📒 Files selected for processing (1)
infrastructure/__tests__/ci-admin-migrate.test.ts
| it('given the shared one-shot-machine script, should run flyctl machine run', () => { | ||
| expect(RUN_FLY_MIGRATION_SCRIPT).toContain('flyctl machine run'); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Test executable failure behavior, not source-text presence.
The assertions only search the shell script text. The script comments and log messages already contain flyctl machine run, failed, and destroyed. The tests can pass if the actual command or exit 1 handling is removed. Add an executable test with mocked Fly API responses for failed and destroyed, and assert a non-zero exit and cleanup.
Also applies to: 80-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@infrastructure/__tests__/ci-admin-migrate.test.ts` around lines 60 - 62,
Replace the source-text assertions around RUN_FLY_MIGRATION_SCRIPT with an
executable test that runs the script using mocked Fly API responses for failed
and destroyed machine states. Assert the script exits non-zero and performs
machine cleanup, so comments or log messages cannot satisfy the test.
| it('given the admin migrations step, should use the same migrate image as the main step', () => { | ||
| expect(adminStep.run).toContain('pagespace-migrate:latest'); | ||
| const imageOf = (run?: string) => run?.match(/ghcr\.io\/2witstudios\/pagespace-migrate\b/)?.[0]; | ||
| expect(imageOf(adminStep.run)).toBeDefined(); | ||
| expect(imageOf(adminStep.run)).toBe(imageOf(steps[mainIdx].run)); | ||
| }); | ||
|
|
||
| it('given the admin and main migrations steps, should never pin the mutable :latest tag', () => { | ||
| expect(adminStep.run).not.toContain('pagespace-migrate:latest'); | ||
| expect(steps[mainIdx].run).not.toContain('pagespace-migrate:latest'); | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Compare the migration tag argument, not only the image prefix.
Line [65] stops at pagespace-migrate, so Line [67] cannot detect different tags. Lines [71-72] also miss the actual pagespace-migrate latest argument form. Extract the tag argument and assert that both steps use sha-${GITHUB_SHA::7} and never use latest.
Suggested fix
- const imageOf = (run?: string) => run?.match(/ghcr\.io\/2witstudios\/pagespace-migrate\b/)?.[0];
- expect(imageOf(adminStep.run)).toBeDefined();
- expect(imageOf(adminStep.run)).toBe(imageOf(steps[mainIdx].run));
+ const migrationTagOf = (run?: string) =>
+ run?.match(/ghcr\.io\/2witstudios\/pagespace-migrate\s+["']?([^"'\s]+)["']?/)?.[1];
+ expect(migrationTagOf(adminStep.run)).toBe('sha-${GITHUB_SHA::7}');
+ expect(migrationTagOf(adminStep.run)).toBe(migrationTagOf(steps[mainIdx].run));
- expect(adminStep.run).not.toContain('pagespace-migrate:latest');
- expect(steps[mainIdx].run).not.toContain('pagespace-migrate:latest');
+ expect(migrationTagOf(adminStep.run)).not.toBe('latest');
+ expect(migrationTagOf(steps[mainIdx].run)).not.toBe('latest');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('given the admin migrations step, should use the same migrate image as the main step', () => { | |
| expect(adminStep.run).toContain('pagespace-migrate:latest'); | |
| const imageOf = (run?: string) => run?.match(/ghcr\.io\/2witstudios\/pagespace-migrate\b/)?.[0]; | |
| expect(imageOf(adminStep.run)).toBeDefined(); | |
| expect(imageOf(adminStep.run)).toBe(imageOf(steps[mainIdx].run)); | |
| }); | |
| it('given the admin and main migrations steps, should never pin the mutable :latest tag', () => { | |
| expect(adminStep.run).not.toContain('pagespace-migrate:latest'); | |
| expect(steps[mainIdx].run).not.toContain('pagespace-migrate:latest'); | |
| }); | |
| it('given the admin migrations step, should use the same migrate image as the main step', () => { | |
| const migrationTagOf = (run?: string) => | |
| run?.match(/ghcr\.io\/2witstudios\/pagespace-migrate\s+["']?([^"'\s]+)["']?/)?.[1]; | |
| expect(migrationTagOf(adminStep.run)).toBe('sha-${GITHUB_SHA::7}'); | |
| expect(migrationTagOf(adminStep.run)).toBe(migrationTagOf(steps[mainIdx].run)); | |
| }); | |
| it('given the admin and main migrations steps, should never pin the mutable :latest tag', () => { | |
| expect(migrationTagOf(adminStep.run)).not.toBe('latest'); | |
| expect(migrationTagOf(steps[mainIdx].run)).not.toBe('latest'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@infrastructure/__tests__/ci-admin-migrate.test.ts` around lines 64 - 73,
Update the migration image assertions in the admin/main migration test to
extract and compare the full tag argument, not just the pagespace-migrate image
prefix. Assert both steps use sha-${GITHUB_SHA::7} and reject the actual latest
argument form, using the existing adminStep and steps[mainIdx] run values.
…cess run_fly_migration.sh polled Fly's Machines API for the one-shot migration machine's terminal state and treated `stopped` as unconditional success. With `--restart no`, a machine reaches `stopped` on ANY process exit — including a non-zero one — so a migration script that errored out was silently reported as "Migrations complete" and the deploy proceeded on a half-applied schema. Now reads `events[].request.exit_event.exit_code` from the last-polled response and fails closed (destroys the machine, prints logs, exits 1) unless it's exactly 0. Added an executable test (scripts/__tests__/run-fly-migration.test.ts) that runs the real script against fake flyctl/docker/curl executables on PATH, rather than asserting on script source text — proves actual exit-code behavior for success, non-zero, and missing-exit-code cases. Writing that test surfaced two portability bugs blocking it from running on macOS (works fine on the Ubuntu CI runner, GNU coreutils + bash 5): an empty EXTRA_ARGS array expansion is an unbound-variable error under old bash's `set -u`, and `grep -oP`/`\K` is GNU-only. Fixed both so the script (and its test) run the same everywhere. Found via CodeRabbit review on PR #2125 (Critical severity).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/__tests__/run-fly-migration.test.ts`:
- Around line 29-55: Update the fake machine fixture returned by the “machine
run” case to use an ID containing only hexadecimal characters, and modify the
fake curl executable to verify the requested URL includes that complete ID
before returning success. Reject or fail requests with a missing or partial
machine ID while preserving the existing response-file and HTTP-status behavior
for valid requests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6141d972-8841-48c8-a4ee-f676b71299da
📒 Files selected for processing (3)
scripts/__tests__/run-fly-migration.test.tsscripts/deploy/run-fly-migration.shtasks/deploy-safety-review-fixes-epic.md
| writeFakeExecutable( | ||
| 'flyctl', | ||
| ` | ||
| case "$*" in | ||
| "machine run"*) echo "Machine ID: fake0001234"; exit 0 ;; | ||
| "machine destroy"*) exit 0 ;; | ||
| "machine logs"*) exit 0 ;; | ||
| *) exit 0 ;; | ||
| esac | ||
| ` | ||
| ); | ||
|
|
||
| // Mimics `curl -s -o <file> -w "%{http_code}" <url>`: writes the canned Machines | ||
| // API response to the -o file and prints "200" to stdout (the polled HTTP code). | ||
| writeFakeExecutable( | ||
| 'curl', | ||
| ` | ||
| outfile="" | ||
| prev="" | ||
| for arg in "$@"; do | ||
| if [ "$prev" = "-o" ]; then outfile="$arg"; fi | ||
| prev="$arg" | ||
| done | ||
| cp "${stateResponsePath}" "$outfile" | ||
| printf '200' | ||
| ` | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the complete machine ID in the fake API call.
fake0001234 contains k, but the production parser accepts only [0-9a-f]. The script extracts fa, and this fake curl still returns success because it ignores the requested URL. Use a hexadecimal fixture ID and reject requests that do not contain the full ID.
Proposed test change
- "machine run"*) echo "Machine ID: fake0001234"; exit 0 ;;
+ "machine run"*) echo "Machine ID: fa1e0001234"; exit 0 ;;
@@
outfile=""
prev=""
+url=""
for arg in "$@"; do
if [ "$prev" = "-o" ]; then outfile="$arg"; fi
+ case "$arg" in
+ https://api.machines.dev/*) url="$arg" ;;
+ esac
prev="$arg"
done
+[ "$url" = "https://api.machines.dev/v1/apps/pagespace-web/machines/fa1e0001234" ] || exit 2
cp "${stateResponsePath}" "$outfile"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| writeFakeExecutable( | |
| 'flyctl', | |
| ` | |
| case "$*" in | |
| "machine run"*) echo "Machine ID: fake0001234"; exit 0 ;; | |
| "machine destroy"*) exit 0 ;; | |
| "machine logs"*) exit 0 ;; | |
| *) exit 0 ;; | |
| esac | |
| ` | |
| ); | |
| // Mimics `curl -s -o <file> -w "%{http_code}" <url>`: writes the canned Machines | |
| // API response to the -o file and prints "200" to stdout (the polled HTTP code). | |
| writeFakeExecutable( | |
| 'curl', | |
| ` | |
| outfile="" | |
| prev="" | |
| for arg in "$@"; do | |
| if [ "$prev" = "-o" ]; then outfile="$arg"; fi | |
| prev="$arg" | |
| done | |
| cp "${stateResponsePath}" "$outfile" | |
| printf '200' | |
| ` | |
| ); | |
| writeFakeExecutable( | |
| 'flyctl', | |
| ` | |
| case "$*" in | |
| "machine run"*) echo "Machine ID: fa1e0001234"; exit 0 ;; | |
| "machine destroy"*) exit 0 ;; | |
| "machine logs"*) exit 0 ;; | |
| *) exit 0 ;; | |
| esac | |
| ` | |
| ); | |
| // Mimics `curl -s -o <file> -w "%{http_code}" <url>`: writes the canned Machines | |
| // API response to the -o file and prints "200" to stdout (the polled HTTP code). | |
| writeFakeExecutable( | |
| 'curl', | |
| ` | |
| outfile="" | |
| prev="" | |
| url="" | |
| for arg in "$@"; do | |
| if [ "$prev" = "-o" ]; then outfile="$arg"; fi | |
| case "$arg" in | |
| https://api.machines.dev/*) url="$arg" ;; | |
| esac | |
| prev="$arg" | |
| done | |
| [ "$url" = "https://api.machines.dev/v1/apps/pagespace-web/machines/fa1e0001234" ] || exit 2 | |
| cp "${stateResponsePath}" "$outfile" | |
| printf '200' | |
| ` | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/__tests__/run-fly-migration.test.ts` around lines 29 - 55, Update the
fake machine fixture returned by the “machine run” case to use an ID containing
only hexadecimal characters, and modify the fake curl executable to verify the
requested URL includes that complete ID before returning success. Reject or fail
requests with a missing or partial machine ID while preserving the existing
response-file and HTTP-status behavior for valid requests.
#2125 (open) adds a migration-safety CI job that fails any newly-added packages/db/drizzle/*.sql containing DROP TABLE / DROP COLUMN without a leading `-- destructive-migration-ack: <reason>` comment. 0256 has both and had no ack, so whichever of the two merges second would have broken on the other. Verified against that PR's actual checker rather than by reading it, and mutation-checked in both directions: with ack: OK 0256_parched_bloodscream.sql (destructive: DROP TABLE, DROP COLUMN — acknowledged) exit 0 ack removed: FAIL … Destructive migration(s) added without an ack exit 1 The live SQL is byte-identical — this adds comments only, and `db:generate` still reports no drift. Hand-annotating a generated migration is established practice here: 0250 through 0254 all carry leading comment blocks, 0253 being the previous contract drop in this same epic. The reason text answers the checker's own prompt ("why this is safe, or what old code it may break") with both halves: the production pre-flight result, and the accepted deploy window the codex review surfaced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EzyBCcUBisXoraLveHXKDA
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
Summary by CodeRabbit
Bug Fixes
Deployment
Safety