Skip to content

fix(recovery): rehearse every protected-main migration, not a frozen 0001-0009 list - #298

Draft
seonghobae wants to merge 2 commits into
developfrom
fix/recovery-rehearsal-full-migration-sequence
Draft

fix(recovery): rehearse every protected-main migration, not a frozen 0001-0009 list#298
seonghobae wants to merge 2 commits into
developfrom
fix/recovery-rehearsal-full-migration-sequence

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Summary

The DR restore rehearsal (.github/scripts/restore-rehearsal-postgres.sh) had frozen its applied migrations and could silently omit newer protected-main schema. This PR binds recovery evidence to the checked-in migration set and fails closed when that set cannot be discovered unambiguously.

Repair lineage

The first repair replaced the frozen migration list with ordered discovery from database/migrations and added executable/documented completeness evidence.

Fresh review then found a shell failure-mode gap: for migration in $(find ... | sort) could collapse discovery failure into an empty iteration under the compound command, so a missing/unreadable migration directory could skip migration application rather than fail closed.

9b6eca428c9b500158e3428b9cea461483ee4bcd is the ordinary-forward repair:

  • capture and explicitly check migration discovery before any cluster connection or destructive DDL;
  • reject empty discovery;
  • accept only canonical ^[0-9]{4}_[a-z0-9_]+\.sql$ basenames;
  • reject duplicate numeric prefixes;
  • iterate the validated set with while IFS= read -r instead of word splitting;
  • add missing/empty/non-canonical/duplicate-prefix regressions proving discovery fails before psql -f migration application;
  • update restore traceability and reseal recovery-manifest.json.

Exact-head authority

  • protected base: develop@eb9757f8649aaad026a9865508d9aad50c1a7a4f
  • exact head: 9b6eca428c9b500158e3428b9cea461483ee4bcd
  • GitHub state: open · Draft · mechanically mergeable
  • Foundation 34570990728: SUCCESS
  • Recovery Rehearsal Quality 34570990707: SUCCESS
  • SAST 34570990699: SUCCESS
  • Security 34570990713: SUCCESS
  • CodeQL 34570990718: FAILURE in the central producer/consumer settlement lane, not a demonstrated recovery-source finding. Python consumer 103173279931 failed enforcement at 2026-09-11T06:47:20Z and completed FAILURE 06:47:22Z; Actions consumer 103173279976 failed enforcement at 06:52:35Z and completed FAILURE 06:52:36Z; authoritative dispatch 103175360364 did not start until 06:56:50Z and completed SUCCESS 06:56:57Z. Fresh canary is handed to ContextualWisdomLab/.github#2040 in comment 5630795137.
  • Devin Review and CodeRabbit commit statuses are SUCCESS, but they are supplemental; no qualifying current-head protected APPROVED is claimed.
  • predecessor Noema/OpenCode/model evidence is not transferred to this head.

Acceptance

Protected integration requires the current exact head to keep all repository-owned recovery checks GREEN and prove that every canonical migration is discovered exactly once in deterministic order, discovery ambiguity/missing state fails before connection or DDL, the real cross-cluster dump/restore rehearsal succeeds, restored business/audit/outbox invariants remain intact, and all central required governance is admissible.

Keep Draft until those current-head gates and independent review are satisfied. No self/model approval, administrator bypass, gate weakening, no-op retrigger, predecessor-evidence transfer, force-push, destructive rebase, or simple Close is authorized.

…0001-0009 list

The DR rehearsal hardcoded migrations 0001-0009, so migrations 0010-0013
(including the MIGRATION_BACKED job_analysis_snapshot) were never exercised
by recovery evidence, while the traceability doc claimed the rehearsal
applies the protected-main migration sequence.

Enumerate database/migrations in numeric order instead, and bind the
behavior with a recovery test that fails closed on a hardcoded/frozen list,
a skipped migration, or a reordering hazard. Reseal recovery-manifest.json.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Sep 11, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: c479ad4e-8cdf-48ef-9037-8cffe6f9ac9f

📥 Commits

Reviewing files that changed from the base of the PR and between eb9757f and 161468e.

📒 Files selected for processing (4)
  • .github/scripts/restore-rehearsal-postgres.sh
  • docs/traceability/restore-rehearsal.md
  • recovery-manifest.json
  • tests/recovery-rehearsal.test.mjs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

복구 리허설이 고정 목록 대신 마이그레이션 디렉터리의 SQL 파일을 숫자 순서로 검색하고 실행합니다. 테스트와 실행 계약은 누락, 중복, 비순차 적용을 검증합니다. 무결성 매니페스트도 갱신되었습니다.

Changes

복구 마이그레이션 검색 및 검증

Layer / File(s) Summary
동적 마이그레이션 검색 및 실행
.github/scripts/restore-rehearsal-postgres.sh, tests/recovery-rehearsal.test.mjs
복구 스크립트가 database/migrations에서 숫자 접두사가 있는 SQL 파일을 검색하고 정렬합니다. 테스트가 고정 목록 사용, 빈 디렉터리, 중복 번호, 누락 파일을 검증합니다.
실행 계약 및 무결성 기록
docs/traceability/restore-rehearsal.md, recovery-manifest.json
실행 계약이 전체 마이그레이션의 중복, 누락, 비순차 적용을 실패 조건으로 정의합니다. 변경된 파일의 해시, 바이트 수, 줄 수를 매니페스트에 반영합니다.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 16146

The recovery rehearsal now applies the complete checked-in migration chain in order, with tests guarding against omissions and duplicate prefixes. No actionable merge risk remains.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed PR 제목은 하드코딩된 0001~0009 목록 대신 보호된 메인 마이그레이션 전체를 복구 리허설에 적용하는 핵심 변경을 정확하고 간결하게 설명합니다.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/recovery-rehearsal-full-migration-sequence

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copy link
Copy Markdown
Contributor Author

Recovery rehearsal audit found one still-valid fail-closed defect in the new migration enumeration. The current exact head 161468e3d17b3e1964f570ec9bf286ec4045bc8d uses for migration in $(find ... | sort); do .... Under the script's own set -euo pipefail, a failure inside that command substitution does not make the for compound command fail: set -euo pipefail; for x in $(false | sort); do :; done; echo after reaches after with status 0. That means a missing/unreadable migration directory or discovery failure can collapse to an empty iteration and the rehearsal proceeds past migration application instead of failing closed. Word splitting also makes the loop depend on filename whitespace behavior rather than a bounded migration-name contract.

Please repair this on the canonical #298 branch rather than weakening the test/gate: capture discovery through a simple assignment whose exit status is checked under set -e/pipefail, reject an empty result, validate every discovered .sql basename against the canonical ^[0-9]{4}_[a-z0-9_]+\.sql$ form (and duplicate numeric prefixes) at runtime, then iterate with while IFS= read -r or an equivalently non-word-splitting path. Add a regression that proves discovery failure/empty discovery cannot reach the first psql -f application loop. The existing static directory-completeness test is useful but is not a substitute for the executable itself failing closed when run outside the preceding npm run validate step.

Current hosted evidence on this head is otherwise strong: Recovery Rehearsal Quality, Foundation CI, SAST, and Security are SUCCESS. CodeQL remains a separate central settlement-order failure, not evidence against this recovery source.

@seonghobae
seonghobae marked this pull request as draft September 11, 2026 02:15

Copy link
Copy Markdown
Contributor Author

I moved #298 back to Draft without changing the source head/base. The executable fail-closed finding above is still valid on exact 161468e3d17b3e1964f570ec9bf286ec4045bc8d, so keeping the PR Ready would advertise merge admission before the recovery executable itself proves discovery failure/empty discovery cannot silently skip migrations. This is a repair state, not a Close: preserve the existing valid recovery delta/evidence, ordinary-forward the minimum discovery/iteration fix and regression on this canonical branch, then reacquire exact-head recovery/Foundation/security evidence before returning Ready.

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

OpenCode could not approve from deterministic current-head evidence because GitHub Checks have failed.

Findings

1. HIGH Current-head GitHub Checks - Fix failed required checks before approval

  • Problem: Failed same-head checks remain for 161468e3d17b3e1964f570ec9bf286ec4045bc8d.
  • Root cause: The model-unavailable evidence fallback is allowed only when peer GitHub Checks are complete and clean.
  • Fix: Read and fix the failed check logs below, then rerun the current-head checks.
  • Regression test: Keep the model-unavailable fallback gated on an empty failed-check rollup.

Failed checks:

Changed-File Evidence Map

flowchart LR
  PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
  Evidence --> S1["Repository file: restore-rehearsal-postgres.sh"]
  S1 --> I1["repository behavior"]
  I1 --> R1["Review risk: Repository file: restore-rehearsal-postgres.sh"]
  R1 --> V1["required checks"]
  Evidence --> S2["Docs: restore-rehearsal.md"]
  S2 --> I2["operator or user guidance"]
  I2 --> R2["Review risk: Docs: restore-rehearsal.md"]
  R2 --> V2["docs review"]
  Evidence --> S3["Repository file: recovery-manifest.json"]
  S3 --> I3["repository behavior"]
  I3 --> R3["Review risk: Repository file: recovery-manifest.json"]
  R3 --> V3["required checks"]
  Evidence --> S4["Test: recovery-rehearsal.test.mjs"]
  S4 --> I4["regression suite"]
  I4 --> R4["Review risk: Test: recovery-rehearsal.test.mjs"]
  R4 --> V4["targeted test run"]
Loading

@opencode-agent

Copy link
Copy Markdown
Contributor

OpenCode Review Overview

…uous

The rehearsal enumerated migrations with `for migration in $(find ... | sort)`.
Under `set -euo pipefail` a failure inside that command substitution does not
fail the `for` compound command, so a missing or unreadable migration
directory collapsed to an empty iteration and the rehearsal silently skipped
migration application instead of failing closed.

Capture discovery through an assignment whose status is checked, reject an
empty result, validate every discovered basename against the canonical
`^[0-9]{4}_[a-z0-9_]+\.sql$` form, reject duplicate numeric prefixes, and
iterate with `while IFS= read -r` rather than word splitting. Discovery now
runs before any cluster connection or destructive DDL.

The executable recovery contract adds regressions proving that missing,
empty, non-canonical, and duplicate-prefix migration sets fail before the
first `psql -f` application is reached.
@seonghobae

Copy link
Copy Markdown
Contributor Author

Recovery discovery fail-closed repair is applied ordinary-forward on this canonical branch. Current exact head: 9b6eca428c9b500158e3428b9cea461483ee4bcd (protected base unchanged develop@eb9757f8649aaad026a9865508d9aad50c1a7a4f).

The prior head 161468e3... used for migration in $(find ... | sort). Under the script's own set -euo pipefail, a failure inside that command substitution does not fail the for compound command (set -euo pipefail; for x in $(false | sort); do :; done; echo after exits 0), so a missing or unreadable migration directory collapsed to an empty iteration and the rehearsal proceeded past migration application. Word splitting also made iteration depend on filename whitespace.

The repair:

  • captures discovery through an assignment whose exit status is checked, and fails closed on an empty result;
  • validates every discovered basename against the canonical ^[0-9]{4}_[a-z0-9_]+\.sql$ form and rejects duplicate numeric prefixes at runtime;
  • iterates with while IFS= read -r instead of word splitting;
  • runs discovery before any cluster connection or destructive DDL, so it fails closed ahead of the first psql -f application.

Regression evidence (test-first, RED then GREEN):

  • RED: the two new executable tests fail against the unchanged 161468e3 script — rehearsal fails closed before applying anything when migrations cannot be discovered and rehearsal fails closed on empty, non-canonical, and duplicate-prefix migration sets — while the pre-existing static directory test still passed, proving the static test is not a substitute for the executable failing closed.
  • GREEN: all 6 recovery-contract tests pass on 9b6eca42; npm run validate exits 0 (55/55).
  • Provenance: recovery-manifest.json re-bound to the exact new SHA-256/byte/line counts for the executable, test, traceability doc, and unchanged workflow.

The doc copy in docs/traceability/restore-rehearsal.md now states the pre-connection discovery contract and that discovery failure cannot reach the application loop.

This changes recovery executable behavior only; no migration source, workflow trigger, gate, or product semantics changed. Awaiting exact-head Recovery Rehearsal Quality / Foundation / SAST / Security evidence on 9b6eca42 before returning Ready. CodeQL remains the separate central settlement-order failure.

@seonghobae seonghobae added bug Something isn't working priority: high labels Sep 12, 2026 — with ChatGPT Codex Connector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority: high

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant