Uh oh!
There was an error while loading. Please reload this page.
feat(promotion): one-command staging→prod promotion runner (#516) - #517
Merged
Conversation
Add backend/promotion/ package with preflight.py: project_ref, ledger_diff, staging_gap, scan_destructive (comment-stripping so the repo's explanatory migration headers don't trip false positives) and evaluate, which aggregates all guards into blocking findings before production is ever touched. The plan's original test_evaluate_reports_nothing_to_promote fixture left a migration pending while asserting the "nothing to promote" case, contradicting evaluate()'s own guard (commits_ahead == 0 and not pending). Fixed the test fixture rather than the implementation: "nothing to promote" must mean no commits AND no pending migrations, and a migration-only promotion (code level, schema behind) is real, distinct, and must still proceed. Added test_evaluate_allows_a_migration_only_promotion to pin that distinction.
Line-oriented matching evaded detection whenever a destructive DDL statement's keywords were split across lines — this repo's own house style wraps ALTER TABLE/ALTER COLUMN clauses (see db/migrations/0012_gradebook.sql), so a wrapped ALTER COLUMN ... TYPE ... USING ... would have passed the guard silently. scan_destructive now splits comment-stripped text on ';' and whitespace-collapses each statement before matching, so wrapped and single-line statements match identically. Findings still report the line where the statement begins and the original (uncollapsed) source line. Added tests for a wrapped ALTER COLUMN ... TYPE, a wrapped DROP COLUMN, the real benign wrapped DROP NOT NULL shape from 0012_gradebook.sql (must stay clean — this is what keeps the guard from crying wolf), and that a wrapped statement's finding reports its starting line. Re-ran the migration sanity check against 0012/0013/0021/0030 — unchanged results plus 0012 now clean.
…#516) test_scan_destructive_flags_wrapped_drop_column previously wrapped "ALTER TABLE t" and "DROP COLUMN old;" onto separate lines, but DROP COLUMN itself sat wholly on one line — the pre-fix line-oriented scanner would have matched it directly, so the test didn't actually exercise the statement-split fix. Changed the fixture so DROP and COLUMN straddle the line break themselves ("ALTER TABLE t\n DROP\n COLUMN old;"), which the line-oriented scanner cannot match. Verified by hand: reverting scan_destructive to its pre-fix line-oriented form makes this test fail; restoring the statement-based version makes it pass again.
Adds capture()/diff()/format_diff() so an operator confirming a staging→production promotion sees exactly what a migration changed (new/dropped tables, row-count deltas, newly-applied ledger entries) instead of terminal scrollback. SELECTs only, nothing writes.
ruff check . flagged F401 — tests get their Path objects from pytest's tmp_path fixture, so the explicit import was dead weight. Scoping ruff to promotion/ during earlier fix passes never caught this since it never lints the test file.
Adds promotion/runner.py (Ports/Options + run(): preflight -> snapshot -> migrate -> snapshot -> ensure_pr -> the one confirm prompt -> merge with 502-retry -> wait-for-deploy -> smoke) and promotion/__main__.py (the real psycopg/git/gh/httpx ports). Test suite is fully hermetic: every side effect is an injected port.
…heck lie (#516) Code review on Task 5 found defects in the brief it was transcribed from: - ensure_pr queried --state all, so it could find a PREVIOUS promotion's already-merged PR and skip the confirmation prompt entirely. Now queries --state open only, creates + re-queries once (no recursion) if none found. - The deploy wait compared /api/health's reported commit against origin/main's tip, but `gh pr merge --merge` creates a merge commit ON production and Railway deploys production — every successful promotion would time out. Now re-fetches and waits on origin/production's tip. - A missing STAGING_SUPABASE_DB_URL (the default: no .env* ships it) made _staging_recorded() return an empty set, which preflight read as "staging ran nothing" and flagged every pending migration as a staging gap. Now returns None ("unknown") and preflight emits one honest staging-unknown finding instead of guessing. - subprocess calls swallowed real git/gh stderr behind "non-zero exit status"; added a _run() helper that surfaces it, and wrapped main()'s run() call so a RuntimeError prints cleanly instead of a traceback. - The "already-merged PR resumes at wait+smoke" resume path was unreachable (a real re-run sees commits_ahead == 0 and exits via nothing-to-promote first). Replaced with an explicit --verify-only flag that skips straight to wait+smoke against production's current tip. Also: FakeGit now returns distinct SHAs per ref (was returning the same SHA for every ref, which is why the tests didn't catch the wrong-SHA bug); FakeGh gained a revert() the runner must never call, making test_smoke_failure_does_not_revert_anything a real guard instead of an assertion that could never fail.
…eeds no DB (#516) Re-review of the prior fix wave found one new Important defect it introduced, plus three small follow-ups: - The merge-retry loop's new state-read except couldn't distinguish "the read failed" from "the read succeeded and said not-merged", so 5 consecutive gh pr view failures (a real GitHub API flake mid-promotion) would exhaust the loop and print "Production code unchanged" even though gh.merge may have landed on one of the attempts. Track whether any read actually succeeded; only claim "unchanged" when one did, otherwise print an explicit UNKNOWN-outcome message that tells the operator to check `gh pr view <N>` by hand and that the migrations are already applied either way. - --verify-only touches no database (it skips preflight/snapshot/migrate entirely) but main() was still rejecting it without SUPABASE_DB_URL. Scoped the credential check to the paths that actually need it. - _staging_recorded() could leak a raw psycopg traceback past main()'s RuntimeError handler on a bad STAGING_SUPABASE_DB_URL; wrapped it the same way _run() already wraps subprocess failures. - Strengthened the --verify-only test to assert the deploy wait and smoke checks actually executed (fetch called for /api/health and for a smoke-only path), not just that connect/migrate/gh were untouched.
…#516) Second re-review found the previous fix's state_confirmed flag was sticky: set True by ANY successful post-merge gh pr view across the 5 retry attempts and never reset, so "read 1 succeeds (OPEN), reads 2-5 all fail" still printed "Production code unchanged" even though 4 more merge() attempts happened after the last confirmed state. That's the same false-claim class the round was meant to close; all-five-failed was just its narrowest instance. Replaced the sticky flag with the single current_state variable, which is reassigned every loop iteration (None on a failed read) and therefore reflects only the MOST RECENT read by the time the loop exhausts. Added test_merge_state_stale_confirmation_does_not_claim_production_unchanged for the mixed case, verified live to fail against the reintroduced sticky implementation and pass against the fix. Also strengthened test_verify_only_skips_promotion_and_just_waits_and_smokes: /api/health is also one of smoke's own CHECKS, so asserting it was fetched didn't prove the wait loop ran (a short-circuit straight to _run_smoke would pass too). Now forces the wait to iterate via a non-matching first health poll and spies on sleep(), asserting it was called at least once. Verified live to fail when the wait is bypassed and pass when restored.
…nup (#516) Wires backend/promotion (Task 5's __main__) to a `make promote` target, documents the CLI in a runbook and CLAUDE.md, and deletes the untracked hand-run #515 promotion artifacts (prod_snapshot.py, prod_db_check.py, smoke_prod_app.sh, smoke_prod_promotion.sh, prod_snapshot_*.json) now superseded by the package. apply_graph_edges_fix.py and graph_edges_fix.sql are a separate, unrelated production reconciliation and are left untouched.
… path (#516) Whole-branch review found one more Critical (read staleness, not read failure) plus four Important gaps, two Minors, and the previously-deferred __main__.py coverage item. Critical: the merge-retry exhaustion message asserted "Production code unchanged" as fact even when every gh pr view read succeeded and said OPEN — but gh's own documented squash/merge 502 wedge is "error returned, merge lands anyway", so a merge triggered by this run can land seconds after the last successful read. Reworded to report what was observed ("as of the last check, it was OPEN... may still be landing... check gh pr view by hand") instead of asserting a fact this run cannot know. Verified live: reverting to the old wording makes the new test fail, restoring it passes. Important 1: a mid-migration failure (apply_migration commits per file, so partial progress is real and durable) used to propagate as a raw traceback, skipping the after-snapshot and partial-state warning entirely. Now catches it, reopens a fresh connection (the failed one may be left with an aborted transaction), re-captures, and reports exactly how many migrations landed and which one failed via the ledger diff. main()'s handler broadened from except RuntimeError to except Exception so no path exits as a stack trace. Important 2: prints "Target: project <ref> (<host>)" before the migrate stage — SUPABASE_DB_URL/SUPABASE_URL matching each other proves nothing if a whole .env file points at the wrong project. Important 3: commits_ahead == 0 with pending migrations used to apply the migrations successfully then die on `gh pr create` ("No commits between production and main"), reporting the whole run as failed. Now skips PR/merge/deploy-wait and goes straight to smoke (a migration that broke the running app is exactly what that stage exists to catch). Important 4: README step 5 called the prompt "the only irreversible step" when the actual irreversible step (the migration, step 3) already happened by then, contradicting the README's own line 44. Minors: __main__.py's _preflight_data now strips SUPABASE_DB_URL/ SUPABASE_URL the same way main() does before connecting (unstripped values silently no-op the target-mismatch guard); the dead already_merged branch (a real gh call that could abort the run before the operator ever saw the confirm prompt) is deleted now that ensure_pr is --state open-only. Coverage: new tests/test_promotion_main.py locks ensure_pr's `--state open` argv shape down with a faked _run, modeling this repo's real regression (a MERGED PR #515 must never come back as the PR to merge). Verified live: reverting to --state all makes the test fail, restoring it passes.
Final round on Task 5: the migrations-only path (commits_ahead == 0 with pending migrations, added last round) routed smoke failures through the shared _run_smoke, whose default text tells the operator to `git revert -m 1 HEAD` on production. On that path nothing was merged this run, so production's HEAD is a PREVIOUS promotion's merge commit — following that instruction would revert and force-push away an unrelated, previously working deploy. _run_smoke now takes merged_this_run: bool = True; the migrations-only call site passes False and gets its own honest failure text (schema moved, code untouched, no code revert applies, inspect the migration that landed). The normal post-merge path and --verify-only are unchanged and keep the original text, where it is correct. Per the coordinator, the same recipe reachable under --verify-only after a migrations-only promotion is parked, not fixed, this round. Also: the migration-failure message no longer says "PARTIALLY migrated" when 0 of N landed (says schema is UNCHANGED instead), and no longer blames pending[0] by name when nothing landed at all — db.migrate.run()'s prologue (SET maintenance_work_mem / ensure_tracking_table) can fail before touching any file, indistinguishable from a first-file failure from the runner's side, so it now says the run failed before applying anything and points at the Error line instead of guessing a filename.
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthroughThe pull request adds a tested staging-to-production promotion workflow. It includes preflight guards, production snapshots, migrations, gated pull-request merges, deployment verification, smoke checks, build-commit health reporting, a CLI, a Make target, and runbook documentation. ChangesPromotion workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant PromotionCLI
participant PromotionRunner
participant ProductionDB
participant GitHub
participant DeployedServices
Operator->>PromotionCLI: Start promotion command
PromotionCLI->>PromotionRunner: Configure ports and options
PromotionRunner->>ProductionDB: Validate, snapshot, and migrate
PromotionRunner->>Operator: Request merge confirmation
PromotionRunner->>GitHub: Create or reuse and merge pull request
PromotionRunner->>DeployedServices: Wait for deployed commit
PromotionRunner->>DeployedServices: Run smoke checks
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 443d711 | Commit Preview URL Branch Preview URL | Aug 12 2026, 07:36 AM |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
backend/promotion/snapshot.py (1)
41-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
psycopg.sql.Identifierfor the table name.The identifier comes from
information_schema, so injection from user input is not the risk here. The remaining gap is quoting: a table name that contains a double quote breaks the manual"{name}"quoting.psycopg.sql.Identifierquotes correctly and also silences the static-analysis finding on Line 44.♻️ Proposed refactor
+from psycopg import sql+ ... for name in names: - # Identifier comes from information_schema, never from user input,- # and count(*) takes no bindable parameter for a table name.- cur.execute(f'SELECT count(*) FROM public."{name}"')+ # Identifier comes from information_schema, never from user input;+ # count(*) takes no bindable parameter for a table name, so compose+ # the identifier instead of interpolating it.+ cur.execute(+ sql.SQL("SELECT count(*) FROM public.{}").format(sql.Identifier(name))+ ) counts[name] = cur.fetchone()[0]🤖 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 `@backend/promotion/snapshot.py` around lines 41 - 46, Update the table-count query in the names loop to use psycopg.sql.Identifier for name and compose the SQL statement through psycopg.sql.SQL rather than manually interpolating double quotes. Preserve the existing counts[name] assignment and fetch behavior.Source: Linters/SAST tools
backend/promotion/__main__.py (1)
106-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNote the psycopg use against the connection guideline.
The guideline names
db/migrate.pyas the single exception that may use psycopg directly. This module connects with psycopg to staging and to production. The use is consistent in domain — it reads and writes the migration ledger, which isdb/migrate.py's concern, and the module already reusesdb.migrate.discover_migrationsanddb.migrate.run.Either extend the documented exception to cover the promotion CLI, or move the ledger read into a helper in
db/migrate.pythat both callers share.As per coding guidelines: "Route all Supabase access through
db/connection.py::table(); do not instantiatehttpxclients or importsupabaseelsewhere, exceptdb/migrate.pyusing psycopg for DDL."🤖 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 `@backend/promotion/__main__.py` around lines 106 - 108, Move the schema_migrations ledger read from the promotion CLI’s psycopg connection block into a shared helper in db/migrate.py, alongside discover_migrations and run. Update the promotion flow to call that helper for staging and production, preserving the existing set-of-filenames result while keeping direct psycopg usage confined to db/migrate.py.Source: Coding guidelines
backend/promotion/README.md (1)
69-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a language to the fenced code block.
markdownlint reports MD040 on Line 69.
📝 Proposed fix
-```+```bash make promote ARGS="--verify-only" ```🤖 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 `@backend/promotion/README.md` around lines 69 - 71, Update the fenced code block containing the make promote command to declare the bash language, resolving the markdownlint MD040 violation while preserving the command unchanged.Source: Linters/SAST tools
🤖 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 `@backend/config.py`:
- Around line 117-118: Normalize the Railway and generic commit environment
values independently before selecting the first non-empty value in the
commit-resolution logic, preserving the existing seven-character lowercase
fallback to “unknown”; update backend/config.py lines 117-118. Add a test
covering whitespace-only RAILWAY_GIT_COMMIT_SHA with a valid GIT_COMMIT_SHA and
assert the expected shortened SHA in backend/tests/test_health_build_commit.py
lines 35-38.
In `@backend/promotion/__main__.py`:
- Around line 31-42: Update the subprocess invocation in _run to pass an
explicit finite timeout, using the package’s existing external-call timeout
convention where appropriate. Preserve the current stdout/stderr capture and
RuntimeError handling, and ensure a stalled git or gh command raises rather than
blocking indefinitely.
- Around line 169-172: Update the confirm function to catch EOFError from input
and treat it as a declined confirmation by returning False. Preserve the
existing --yes behavior and affirmative response handling so main() emits the
normal pre-merge abort report and EXIT_ABORTED instead of an empty error
message.
- Around line 105-113: Update _staging_recorded to catch connection and query
failures without raising RuntimeError; print the failure reason and return None
so preflight.evaluate emits the existing staging-unknown finding and its
--skip-staging-check guidance. Preserve the current set return value for
successful reads and the existing None behavior when the staging URL is unset.
In `@backend/promotion/runner.py`:
- Around line 118-121: Update _wait_then_smoke to accept and forward the
caller’s merged_this_run value when invoking _run_smoke, preserving the default
for normal promotions and passing false for verify-only runs. Add a regression
test beside test_migrations_only_promotion_fails_if_smoke_fails that simulates a
failed smoke with verify_only=True and asserts the output excludes “git revert”.
In `@backend/promotion/snapshot.py`:
- Around line 37-46: Update capture’s table-count logic in the snapshot flow to
avoid executing exact count(*) scans for every public table. Prefer
pg_class.reltuples estimates, clearly mark the reported counts as approximate,
and preserve the existing per-table comparison structure used by
snapshot["tables"].
In `@CLAUDE.md`:
- Line 63: Update the fenced command block in CLAUDE.md to specify the bash
language identifier, changing the opening fence to bash while preserving the
block’s contents.
---
Nitpick comments:
In `@backend/promotion/__main__.py`:
- Around line 106-108: Move the schema_migrations ledger read from the promotion
CLI’s psycopg connection block into a shared helper in db/migrate.py, alongside
discover_migrations and run. Update the promotion flow to call that helper for
staging and production, preserving the existing set-of-filenames result while
keeping direct psycopg usage confined to db/migrate.py.
In `@backend/promotion/README.md`:
- Around line 69-71: Update the fenced code block containing the make promote
command to declare the bash language, resolving the markdownlint MD040 violation
while preserving the command unchanged.
In `@backend/promotion/snapshot.py`:
- Around line 41-46: Update the table-count query in the names loop to use
psycopg.sql.Identifier for name and compose the SQL statement through
psycopg.sql.SQL rather than manually interpolating double quotes. Preserve the
existing counts[name] assignment and fetch behavior.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c5652c9-769e-4cb1-beef-08007e69e4ac
📒 Files selected for processing (19)
CLAUDE.mdMakefilebackend/.env.examplebackend/config.pybackend/main.pybackend/promotion/README.mdbackend/promotion/__init__.pybackend/promotion/__main__.pybackend/promotion/preflight.pybackend/promotion/runner.pybackend/promotion/smoke.pybackend/promotion/snapshot.pybackend/tests/test_health_build_commit.pybackend/tests/test_promotion_main.pybackend/tests/test_promotion_preflight.pybackend/tests/test_promotion_runner.pybackend/tests/test_promotion_smoke.pybackend/tests/test_promotion_snapshot.pydocs/superpowers/plans/2026-08-02-staging-to-prod-promotion.md
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+37
to
+46
| cur.execute(TABLES_SQL) | ||
| names = [row[0] for row in cur.fetchall()] | ||
| counts: dict[str, int] = {} | ||
| for name in names: | ||
| # Identifier comes from information_schema, never from user input, | ||
| # and count(*) takes no bindable parameter for a table name. | ||
| cur.execute(f'SELECT count(*) FROM public."{name}"') | ||
| counts[name] = cur.fetchone()[0] | ||
| snapshot["tables"] = counts |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
capture runs count(*) over every public table, twice per promotion.
count(*) in PostgreSQL is a full table scan. capture executes it for every base table in public, and the runner calls capture before and after the migration. On a production database with large tables this makes the promotion slow and holds one connection busy during the whole scan. The migration failure path adds a third full pass.
Consider one of these options:
- Use
pg_class.reltuplesfor an estimate and mark the numbers as approximate in the report. - Add a statement timeout, or limit exact counts to a small allowlist of tables the diff must be precise about.
The diff only reports which counts changed, so estimates are enough for that purpose.
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 44-44: SQL query built via f-string passed to execute()/executemany(). Use parameterized queries with placeholders instead.
(coderabbit.sql-injection.python-fstring-execute)
🤖 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 `@backend/promotion/snapshot.py` around lines 37 - 46, Update capture’s
table-count logic in the snapshot flow to avoid executing exact count(*) scans
for every public table. Prefer pg_class.reltuples estimates, clearly mark the
reported counts as approximate, and preserve the existing per-table comparison
structure used by snapshot["tables"].
| Promotion (repo root; full runbook backend/promotion/README.md): | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the command block.
Markdownlint reports MD040 for this fence. Mark the block as bash.
Proposed fix
-```+```bash📝 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.
Suggested change
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 63-63: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@CLAUDE.md` at line 63, Update the fenced command block in CLAUDE.md to
specify the bash language identifier, changing the opening fence to bash while
preserving the block’s contents.
Source: Linters/SAST tools
…egrade staging reads (#516) PR #517 CodeRabbit review, including one item I'd previously parked on a factually wrong premise: --verify-only does not predate this diff (added in f3ab42f, which is part of this PR), so it needed the same fix as the migrations-only path. - runner.py: _wait_then_smoke now forwards merged_this_run to _run_smoke; --verify-only passes False, so a smoke failure there no longer hands out the git-revert-production's-HEAD recipe for a merge this invocation never made. _run_smoke's False-branch message generalized to cover both the migrations-only path and --verify-only accurately. - __main__.py: _run gains a timeout (120s default) so a stalled git/gh network call can no longer hang forever, possibly after the migration has already applied; a timeout now surfaces as a clean RuntimeError naming the command. _staging_recorded no longer re-raises on a staging connection/query failure (stale URI, paused project, transient fault) — it prints a warning and returns None so preflight produces its documented staging-unknown finding instead of aborting the whole run before the operator sees any report. _confirm is now a module-level, directly-testable function that treats EOFError from input() (no controlling terminal / CI without a tty) as a declined confirmation instead of letting main() print a blank "ERROR: " right after the migration landed. - config.py: build_commit() now strips RAILWAY_GIT_COMMIT_SHA and GIT_COMMIT_SHA independently before choosing between them — `A or B` picked a whitespace-only A over a valid B, since whitespace is truthy, silently reporting "unknown" and disabling deploy verification. Also folds in two CodeRabbit nits: test_promotion_main.py's duplicate promotion.__main__ import, and a lambda in test_promotion_runner.py that only wrapped a zero-arg callable. Declined per the coordinator: snapshot's count(*) full scans (already triaged) and CLAUDE.md's bare fence blocks (matches the file's own convention).
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 `@backend/promotion/__main__.py`:
- Around line 120-129: Update _staging_recorded to read the staging migration
ledger through backend/db/connection.py::table() instead of psycopg.connect.
Preserve the existing query result handling and warning/None fallback, and do
not add a direct psycopg access path outside the migration module.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 40306ec2-fdb6-4963-98c5-a15a4beca014
📒 Files selected for processing (6)
backend/config.pybackend/promotion/__main__.pybackend/promotion/runner.pybackend/tests/test_health_build_commit.pybackend/tests/test_promotion_main.pybackend/tests/test_promotion_runner.py
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/config.py
- backend/tests/test_health_build_commit.py
- backend/promotion/runner.py
Comment on lines
+120
to
+129
| # NOT re-raised: a stale URI, a paused staging project, or a | ||
| # transient network fault here must not abort the whole run before | ||
| # the operator ever sees a preflight report — that would deny them | ||
| # the existence of --skip-staging-check at the exact moment they'd | ||
| # want it. Print the reason (so the degradation isn't silent) and | ||
| # return None, same as "the var is unset": preflight.evaluate turns | ||
| # that into its documented `staging-unknown` finding, whose own text | ||
| # says exactly this ("could not read staging's migration ledger"). | ||
| print(f"WARNING: could not read staging's migration ledger ({exc})", file=sys.stderr) | ||
| return None |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Route the staging ledger read through the database access boundary.
_staging_recorded accesses Supabase through psycopg.connect at Line 116. This query is not DDL, and this module is not backend/db/migrate.py. Route this access through backend/db/connection.py::table().
As per coding guidelines, “Route all Supabase access through backend/db/connection.py::table(); … except backend/db/migrate.py for psycopg-based DDL.”
🤖 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 `@backend/promotion/__main__.py` around lines 120 - 129, Update
_staging_recorded to read the staging migration ledger through
backend/db/connection.py::table() instead of psycopg.connect. Preserve the
existing query result handling and warning/None fallback, and do not add a
direct psycopg access path outside the migration module.
Source: Coding guidelines
…corded's degrade path (#516) The coordinator rejected "real IO, no dedicated test" for these two Major fixes from the PR #517 review — both are reachable hermetically by monkeypatching the one library call at the boundary, same pattern already used for Gh.ensure_pr's --state open test: - test_run_passes_a_timeout_to_subprocess / test_run_converts_timeout_expired_to_a_clean_runtime_error: fake promotion_main.subprocess.run, no process spawned. Verified live that dropping _run's timeout kwarg fails both (missing kwarg; a raw TimeoutExpired escaping uncaught). - test_staging_recorded_degrades_to_none_on_connection_failure: fake promotion_main.psycopg.connect to raise OperationalError, no database contacted. Verified live that reverting the except branch to re-raise fails it (RuntimeError propagates instead of None being returned). No production code changed — _run and _staging_recorded were already correct; only their test coverage was missing.
…nned merge, honest partial-state reporting (#516) Preflight now fails closed: unparseable project refs block instead of skipping the target-mismatch guard; the destructive-DDL scan gains ALTER TABLE ... RENAME and DROP VIEW, string-literal-aware comment stripping, and a TYPE pattern that no longer false-positives on columns named 'type'; new blocking guards require the local migrations dir to match origin/main and origin/production to be an ancestor of origin/main. The merge is pinned to the SHA preflight audited (--match-head-commit), with a deterministic fail-fast when main moved instead of burning the transient-502 retry loop. The deploy wait is tri-state — 'unknown' never satisfies it on first poll; an all-unknown window degrades explicitly to an UNVERIFIED report — and its timeout budgets wall-clock time via an injected monotonic port. Every post-migrate failure path (gh failure at ensure_pr, dead-DB recovery, post-merge git blip, Ctrl-C at the confirm prompt) now emits the 'migrations ALREADY APPLIED / schema is ahead of code' partial-state report instead of a bare traceback, and ensure_pr parses the PR number from gh pr create's own stdout with the list re-query as a bounded fallback. Ledger-diff primitives are extracted to promotion.preflight and consumed by scripts/migration_drift_report.py (byte-identical output) so the two can no longer drift apart. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/promotion/runner.py (1)
404-447: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winGate the moved-head verdict on a failed merge attempt.
The moved-
origin/mainbranch runs whenever the PR state is readable and is notMERGED. It does not check whetherports.gh.mergeraised. If the pinned merge actually landed but thegh pr viewread lags and returnsOPEN, andorigin/mainadvances in the same iteration (an unrelated PR merging to main), the runner prints "MERGE REJECTED ... were NOT promoted" and exits 1. That is a false statement about production, which is the failure class the rest of this loop is written to avoid.Record whether the merge call raised, and enter the moved-head branch only in that case. A successful merge call with a stale
OPENread then keeps the existing retry and "may still be landing" reporting.🐛 Proposed fix
current_state = None for attempt in range(5): + merge_raised = False try: # Pinned to `head`, the SHA stage-1 preflight audited: GitHub # itself (gh --match-head-commit) rejects the merge if main's tip # moved while the operator sat at the confirm prompt — commits in # that window were never destructive-scanned and their migrations # never applied, so they must not ride along. ports.gh.merge(number, head) except Exception as exc: # noqa: BLE001 — any gh failure gets re-checked + merge_raised = True out(f" merge attempt {attempt + 1} errored ({exc}); re-checking PR state") @@ - if current_state is not None:+ if merge_raised and current_state is not None: try: ports.git.fetch() main_now = ports.git.head_sha("origin/main")Note that
test_moved_main_fails_fast_without_burning_the_retry_loopinbackend/tests/test_promotion_runner.pystill passes with this change, because itsRejectingGh.mergeraises.🤖 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 `@backend/promotion/runner.py` around lines 404 - 447, Track whether ports.gh.merge(number, head) raises during each retry iteration, and gate the moved-origin/main rejection branch on that failed-merge flag in addition to a readable non-MERGED state. Preserve the existing retry and “may still be landing” behavior when the merge call succeeds, even if the subsequent PR state read is stale.backend/promotion/__main__.py (1)
187-192: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a 10-second connection timeout.
If the staging database does not respond,
psycopg.connect(url)can block without a bound. Passconnect_timeout=10so the preflight reaches its fallback path.🤖 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 `@backend/promotion/__main__.py` around lines 187 - 192, Update the staging database connection in the preflight flow to pass a 10-second connection timeout to psycopg.connect. Preserve the existing URL validation, recorded_filenames call, and fallback behavior while ensuring an unresponsive database reaches that fallback after the timeout.
🧹 Nitpick comments (1)
backend/promotion/preflight.py (1)
125-186: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe scanner does not handle
E'...'backslash escapes.The literal state machine treats only
''as an escape. In a Postgres escape-string literal,E'a\'b'closes the literal at the backslash-quote, so the scanner leaves literal state early. Text after it is then scanned as SQL. This produces a false positive finding, not a missed one, so the guard stays fail-closed. No migration in this repo appears to useE'...', so this is a note for future migrations rather than a defect now.Add
standard_conforming_strings-style handling only if such a literal appears.🤖 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 `@backend/promotion/preflight.py` around lines 125 - 186, The _strip_comments scanner currently handles only doubled-quote escapes; if an E-prefixed string literal is introduced, add backslash-escape handling within its in_literal state so a backslash-quote does not terminate the literal. Do not change the scanner for standard string literals or add this handling proactively unless an E'...' literal appears.
🔇 Additional comments (23)
backend/promotion/runner.py (8)
11-44: LGTM!
63-108: LGTM!
111-181: LGTM!
183-251: LGTM!
254-316: LGTM!
318-353: LGTM!
355-385: LGTM!
449-496: LGTM!backend/promotion/README.md (2)
44-50: 📐 Maintainability & Code Quality
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the documented 10-minute default wait timeout.
The runbook states a 10 min timeout for the deploy wait.
Options.wait_timeoutis defined inbackend/promotion/runner.pyoutside the reviewed range, so the default value cannot be confirmed here. Confirm the two match, otherwise the runbook misstates how longmake promoteblocks.
17-25: LGTM!Also applies to: 27-43, 51-102
backend/tests/test_promotion_runner.py (6)
8-28: LGTM!
50-85: LGTM!
141-180: LGTM!
207-258: LGTM!Also applies to: 324-409
419-511: LGTM!Also applies to: 514-603
677-743: LGTM!Also applies to: 886-993
backend/promotion/__main__.py (2)
180-203: The staging ledger read still usespsycopgdirectly.This concern was raised on a previous commit and the code path is unchanged in substance:
_staging_recordedopens its ownpsycopgconnection instead of going through the sanctioned database boundary.As per coding guidelines, "All Supabase access goes through
db/connection.py::table(). Do not instantiatehttpxclients or importsupabasedirectly elsewhere."
67-105: LGTM!Also applies to: 108-177, 206-228, 231-256
backend/promotion/preflight.py (1)
6-13: LGTM!Also applies to: 28-48, 91-114, 245-303
backend/scripts/migration_drift_report.py (1)
39-44: LGTM!Also applies to: 55-65, 101-107
backend/tests/test_promotion_preflight.py (1)
79-145: LGTM!Also applies to: 160-195, 242-258, 265-328
backend/tests/test_promotion_main.py (1)
3-38: LGTM!Also applies to: 81-201, 218-232, 247-304, 307-366, 369-436
backend/tests/test_migration_drift_report.py (1)
1-27: LGTM!Also applies to: 30-92, 95-161
🤖 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.
Outside diff comments:
In `@backend/promotion/__main__.py`:
- Around line 187-192: Update the staging database connection in the preflight
flow to pass a 10-second connection timeout to psycopg.connect. Preserve the
existing URL validation, recorded_filenames call, and fallback behavior while
ensuring an unresponsive database reaches that fallback after the timeout.
In `@backend/promotion/runner.py`:
- Around line 404-447: Track whether ports.gh.merge(number, head) raises during
each retry iteration, and gate the moved-origin/main rejection branch on that
failed-merge flag in addition to a readable non-MERGED state. Preserve the
existing retry and “may still be landing” behavior when the merge call succeeds,
even if the subsequent PR state read is stale.
---
Nitpick comments:
In `@backend/promotion/preflight.py`:
- Around line 125-186: The _strip_comments scanner currently handles only
doubled-quote escapes; if an E-prefixed string literal is introduced, add
backslash-escape handling within its in_literal state so a backslash-quote does
not terminate the literal. Do not change the scanner for standard string
literals or add this handling proactively unless an E'...' literal appears.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4365bf03-a8d0-4a70-9da8-6caf89a22ffb
📒 Files selected for processing (9)
backend/promotion/README.mdbackend/promotion/__main__.pybackend/promotion/preflight.pybackend/promotion/runner.pybackend/scripts/migration_drift_report.pybackend/tests/test_migration_drift_report.pybackend/tests/test_promotion_main.pybackend/tests/test_promotion_preflight.pybackend/tests/test_promotion_runner.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#516.
Replaces the hand-run promotion sequence from #515 with
make promote.What lands
backend/promotion/— four single-responsibility units plus a runner:preflight.py— read-only guards: target identity, ledger exists, no orphans, staging-ran-it-first, no destructive DDL, something to promotesnapshot.py— before/after prod capture + diff (SELECT-only)smoke.py— durable post-deploy checks as data, injected fetcherrunner.py/__main__.py— stage sequencing, the single confirmation prompt, real ports/api/healthreports the build commit (config.build_commit()), so the deploy wait is deterministic instead of a sleepmake promote+ runbook atbackend/promotion/README.md+ CLAUDE.md entryprod_snapshot.py,prod_db_check.py,smoke_prod_*.sh,prod_snapshot_*.json)Design decisions
--allow-destructiveis an explicit decision rather than a default.fall-2026and a start date; those pass today and rot next term).Verification
All new tests are hermetic — every side effect is an injected port, so the full promotion sequence runs in-process with no database, network, subprocess or
gh. Suite: 1638 passed, 38 skipped;ruff check .fully clean.Rehearsed read-only against live production:
Review history worth knowing
The branch went through per-task review plus a whole-branch review. Two Critical defects were caught before merge, both in the original plan rather than the implementation:
ensure_prqueried--state all, which returns PR Promote staging to production — 285 commits (DB reconciled first) #515 (MERGED) — the runner would have read that as "already merged", skipped both the prompt and the merge, and exited 1 blaming the deploy, after having already migrated production./api/healthagainstorigin/main's tip, butgh pr merge --mergecreates a merge commit onproduction, which is what gets deployed. Those SHAs can never match.Beyond those, five separate defects were of one kind: the tool printing something that could be false about production state — claiming "production code unchanged" when a 502'd merge may have landed, claiming "partially migrated" when zero migrations landed, and handing the operator a
git revert -m 1 HEADrecipe on a path where nothing was merged (which would have reverted an unrelated, working deploy). Each is now framed as an observation or scoped to the path where it is true.Known and deferred
--verify-only; that path predates this work and merges nothing either way.main → productiononly; production may carry extra commits.capture()doescount(*)per table — twice per promotion, fine at current prod size._strip_commentssplits on the first--without respecting string literals; no current migration contains such a literal, and the guard backstops human review rather than replacing it.Follow-up (not this PR)
A
workflow_dispatchwrapper with a protectedproductionenvironment.runner.run(ports, options)takes every side effect as a port, so the wrapper supplies a non-interactiveconfirmand CI secrets without touching the runner.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
make promote.Documentation
Bug Fixes