P7-C REBUILD: portable persistence, backup & PostgreSQL DR - #274
Conversation
P7-C1 rebuild: portable persistence + cold filesystem backup/recovery + first-install Launchpad, starting from origin/master (baseline 2a1a9eb). Not derived from PR #273. Compose + isolation (C1.1-C1.3): - docker-compose.yml: authoritative state moved from named volumes to operator-visible host bind mounts under ${EXAM_DATA_ROOT:-./data} (postgres + redis). Top-level named-volume declarations removed. The deployment-topology-contract guard still passes (it pins service presence + security invariants, not volume type). - .gitignore: /data/ added so runtime data is never committed. - p6-corr1-compose-smoke.sh: sets EXAM_DATA_ROOT to a per-run mktemp directory with trap cleanup (path-guarded rm). All 16 checks still pass. Persistence + relocation proofs (C1.4-C1.5): - scripts/deployment/p7-c1-persistence-smoke.sh: down/up restart persistence + cold directory relocation (container-assisted cp -a of the COMPLETE data root while PostgreSQL is stopped) into a new project with identical business invariants. PASS. - Documents the PGDATA-files-owned-by-uid-999 reality: cold copy uses a helper container (host cp -a fails with EACCES). Cold backup/restore (C1.6-C1.8): - scripts/backup/cold-filesystem-backup.sh + cold-filesystem-restore.sh: operator helpers that refuse unsafe/empty paths, validate PGDATA shape, preserve ownership/mode/symlinks, and require explicit confirmation. Backup layout mirrors the deployment data root. - scripts/deployment/p7-c1-cold-backup-restore-drill.sh: permanent automated drill proving a fresh working Exam deployment with identical authoritative state is produced from a cold backup. Closes P2-2/P2-3 for the cold-filesystem path. PASS. Launchpad first-install (C1.9): - runtimeConfig.ts: LaunchpadConfig { setupToken } resolved from LAUNCHPAD_SETUP_TOKEN (unset/empty = disabled; NOT fail-fast). - packages/contracts: launchpad.ts (status + bootstrap request/response schemas); messageRegistry error codes LAUNCHPAD_ALREADY_INITIALIZED / LAUNCHPAD_INVALID_SETUP_TOKEN. - apps/api/src/routes/launchpad.ts: GET /api/launchpad/status (public, init-state only, no token oracle) + POST /api/launchpad/bootstrap (public, rate-limited, init-gate FIRST then constant-time token check, delegates to the canonical bootstrapAdminOnFreshDb atomic mutation — no duplicated irreversible logic). Role is not selectable (Admin only). - packages/db organizationRepo.defaultOrganizationExists(): the init gate (NOT activeAdminCount==0; removing last Admin never reopens launchpad). - Frontend: /launchpad public route (LaunchpadPage), redirect to /login when initialized, success redirect to /login, full i18n (zh-CN). - Tests: 6 route integration tests (init gate, token disabled/mismatch, atomic success + no token leak, second-bootstrap refused) + 5 page tests (redirect-when-initialized, form render, validation, submit, error banner). Docs (C1.10): docs/deployment/backup-and-recovery.md (new canonical guide), README + runbook updated with the bind-mount layout, Launchpad option, and the validated cold-backup/restore path. Gates: format/lint/arch/copy/ui-gates/eslint/typecheck/openapi all PASS; p6-corr1 smoke (16), persistence smoke, cold-backup drill PASS. C2 (logical pg_dump) and C3 (physical pg_basebackup + PITR) follow in separate commits on this branch.
Routine online pg_dump logical backup + clean-target restore producing a fresh working Exam with the expected state. Closes P7-C0 P2-2 (logical backup/restore UNVALIDATED) and P2-3 (exact historical replacement UNPROVEN). Backup (C2.1-C2.4): - postgres-logical-backup.sh: pg_dump -Fc custom format, --no-owner, via the db container (PostgreSQL stays ONLINE; API may be down). Fails non-zero on error; password never on argv (PGPASSWORD env). Refuses empty/partial artifact: non-empty + PGDMP magic + pg_restore --list OK (-i so stdin reaches the container). Clean restore (C2.5-C2.6): - postgres-logical-restore.sh: clean-target contract — DROP target DB + CREATE DATABASE ... TEMPLATE template0 (truly empty), then pg_restore --no-owner --exit-on-error. EXACT match of the dump, NOT a merge, fixing the --clean --if-exists-into-dirty-target gap. Requires typing the target DB name to confirm; refuses system DBs; operator-only. Drill (C2.5 — the program's central property): - p7-c2-logical-restore-drill.sh: isolated deploy -> bootstrap + State-A marker -> pg_dump -Fc -> mutate to State B (marker B) -> clean-restore State A -> assert A present, B ABSENT, business invariants (org/admin/ audit) restored, restored Admin row + password hash survived. ALL CHECKS PASSED. Proves a backup creates a fresh working Exam with the expected state and dump-absent objects are removed. Cluster globals (C2.3): - Audited: bundled Compose db service creates role/db at image init from POSTGRES_USER/POSTGRES_DB/POSTGRES_PASSWORD, so they are recreated by Docker/bootstrap config and NOT required in the dump. No cluster roles/tablespaces/globals are application-defined. pg_dumpall --globals-only NOT included for the bundled path (documented §7.3). Docs (C2.7): backup-and-recovery.md §7 (logical backup + clean restore + cluster-globals decision); decision tree updated; runbook §17 cross-refs the validated C2 path and demotes the old one-liner to reference. Gates: format/lint/arch/stale-ui-docs PASS; C2 drill PASS.
PostgreSQL-native physical backup, WAL archiving, and point-in-time
recovery (P7-C phase 3):
- scripts/backup/pg-basebackup.sh: online physical base backup via
pg_basebackup (-X stream -c fast -Fp --manifest-checksums SHA256), then
pg_verifybackup on the manifest. Required WAL is streamed at backup time
so the base backup is internally consistent on its own; the manifest
gives backup-integrity evidence (NOT a business-restore proof; a drill
is still required).
- docker-compose.pitr.yml + docker/pitr/wal-archive.conf: optional
PostgreSQL-native continuous archiving override. archive_mode=on with a
non-overwriting archive_command ('test ! -f /wal-archive/%f && cp ...')
so a WAL filename collision fails VISIBLY rather than silently
overwriting. wal_level stays at replica (sufficient for PITR).
- scripts/deployment/p7-c3-pitr-drill.sh: deterministic PITR happy-path.
base backup -> marker A -> marker B (capture LSN) -> destructive marker
C -> recover to the captured LSN -> assert A present, B present, C
absent. LSN-based target is clock-skew-independent. PASS.
- scripts/deployment/p7-c3-pitr-failure-drill.sh: three failure modes
that MUST fail loudly:
F1 missing WAL segment -> recovery surfaces the missing segment;
F2 corrupt base backup -> pg_verifybackup rejects the tampered file;
F3 invalid recovery_target_lsn -> recovery cluster refuses to start.
All three PASS.
- docs/deployment/backup-and-recovery.md: full decision tree (C1+C2+C3)
with comparison table; new section 8 documents pg_basebackup, WAL
archive, PITR procedure (incl. recovery_target_lsn/time/xid guidance),
what C3 does NOT do, minimal operator-owned retention, and drill
evidence; new section 10 codifies the ADR-016 boundary
(relocation = same history; C2/C3 restore = history replacement; no
schema change).
- README + runbook cross-references updated; the runbook's stale "C3
forthcoming" note is replaced with the shipped procedures.
C3 scope discipline: no PG18 incremental base backups, no retention
engine, no desktop recoveryEpoch, no schema change, no PITR automation in
the base compose (the override is opt-in).
Tests: route registry conformance anchor updated (+2 public launchpad
routes registered in C1; total now 115 = 99 protected + 16 non-protected).
All 2019 vitest tests pass; pnpm verify:static + pnpm build pass.
P7-C rebuild closeout. Adds the authoritative closeout document and repairs the roadmap to the rebuilt C0/C1/C2/C3 shape. - docs/audits/P7-C-PORTABLE-BACKUP-RECOVERY-CLOSEOUT.md: single source of truth for the rebuild. Records baseline SHA, final head SHA, per-phase evidence (C1/C2/C3), the full backup/restore matrix, downtime requirements, version compatibility, the off-host requirement, drill evidence (5/5 drills PASS on final head), the §7 adversarial matrix, scope discipline (what was NOT built), the ADR-016 boundary, known limitations, P0-P3 (none outstanding), and the P7-E handoff. Ends with the program verdict. - docs/roadmap/current.md: P7 row updated (P7-C rebuilt & shipped); execution-order block records the rebuilt phase sequence and the operator-owned-restore scope discipline; Workstream-4 backup note carries the post-rebuild status and the P7-E deferral. - docs/roadmap/P7-system-readiness-and-exam-modes.md: Workstream C re-scoped to the shipped portable-persistence + backup + PostgreSQL DR program (C0 closed; C1 portable + cold + Launchpad; C2 logical; C3 physical + PITR). The pre-rebuild C1=config-taxonomy / C2=settings- service / C3=settings-UI framing is explicitly superseded; those config-control-plane items now live under Workstream E and are NOT started. The sequence block records P7-C as shipped and P7-E (RPO/RTO + retention + Admin visibility) as the future control plane. PR #273 remains historical/reference only. No ceremony was reintroduced. pnpm verify:static PASS.
📝 WalkthroughWalkthroughThis change adds a token-protected first-install Launchpad flow, atomic Admin bootstrap handling, host-mounted PostgreSQL persistence, PostgreSQL backup and recovery scripts, PITR deployment drills, and updated deployment documentation. ChangesLaunchpad bootstrap
PostgreSQL persistence and recovery
Estimated code review effort: 5 (Critical) | ~120 minutes 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
docs/deployment/mvp-deployment-runbook.md-179-183 (1)
179-183: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the Launchpad cross-reference.
backup-and-recovery.md §8documents physical backup and PITR. The Launchpad procedure is in §11. Point this instruction to §11 so operators do not open the wrong procedure.🤖 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 `@docs/deployment/mvp-deployment-runbook.md` around lines 179 - 183, Update the backup-and-recovery cross-reference in the Launchpad first-install instructions to point to §11 instead of §8, leaving the rest of the setup procedure unchanged.docs/audits/P7-C-PORTABLE-BACKUP-RECOVERY-CLOSEOUT.md-140-146 (1)
140-146: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComplete the required closeout report fields.
The closeout reports
pnpm verify:static, test totals, build status, and limitations. It does not report modified files, new-test results as a dedicated field, coverage, orpnpm verify. Add those results, or state explicitly that each item was not collected or not run.As per coding guidelines, every completed job must report modified files, new tests, coverage,
pnpm verifyresults, and known limitations.🤖 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 `@docs/audits/P7-C-PORTABLE-BACKUP-RECOVERY-CLOSEOUT.md` around lines 140 - 146, Update the final closeout report fields around the existing “Final gates on the final head” section to explicitly report modified files, new-test results, coverage, and `pnpm verify` results, stating when any item was not collected or not run. Preserve the existing gate results and limitations.Source: Coding guidelines
docs/deployment/backup-and-recovery.md-267-276 (1)
267-276: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDo not describe a logical restore as byte-for-byte identical.
pg_restorerecreates the dump’s logical database state. Physical page layout, relation files, and other storage details can differ. State that the clean restore matches the dump’s logical contents and validated business invariants.🤖 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 `@docs/deployment/backup-and-recovery.md` around lines 267 - 276, Update the clean-target contract description to remove “byte-for-byte” language and state that the C2 restore via DROP DATABASE, CREATE DATABASE ... TEMPLATE template0, and pg_restore reproduces the dump’s logical contents and validated business invariants. Preserve the explanation that State-B-only data is absent and adjust the automated drill wording accordingly.docs/roadmap/current.md-15-15 (1)
15-15: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the configuration-control-plane ownership.
The companion roadmap moves the pre-rebuild configuration-control-plane work to Workstream E. P7-E covers RPO/RTO profiles, retention automation, and the Admin backup surface. Remove
(P7-E)fromconfig-control-planeor replace it with the correct Workstream E reference.🤖 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 `@docs/roadmap/current.md` at line 15, Update the P7 roadmap entry’s config-control-plane reference to use the correct Workstream E designation, removing the incorrect “(P7-E)” association while preserving the rest of the status and workstream text.docs/deployment/backup-and-recovery.md-44-51 (1)
44-51: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDistinguish the host data root from PGDATA.
The Compose contract mounts
${EXAM_DATA_ROOT}/postgresat/var/lib/postgresql, while the image’s actual PGDATA is under.../postgres/18/docker. Calldata/postgresthe host PostgreSQL data root, and reservePGDATAfor the nested directory.Proposed wording change
- ├── postgres/ ← authoritative: the PostgreSQL data directory (PGDATA) + ├── postgres/ ← authoritative host PostgreSQL data root ... -- `data/postgres/` is **required**. Deleting it destroys authoritative Exam +- `data/postgres/` is the **required host bind mount**. Deleting it destroys authoritative Exam state.🤖 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 `@docs/deployment/backup-and-recovery.md` around lines 44 - 51, Update the deployment documentation to distinguish the host PostgreSQL data root from the image-managed PGDATA directory: describe data/postgres as the host mount parent, and reserve PGDATA for the nested postgres/18/docker directory used by the PostgreSQL image. Preserve the warning that deleting the host data root destroys authoritative Exam state.scripts/deployment/p7-c3-pitr-failure-drill.sh-232-245 (1)
232-245: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTighten the F3 fallback pattern.
Line 238 matches
error,fatal, andstopped. Container logs contain these tokens in unrelated messages, so the branch can pass without the malformedrecovery_target_lsnbeing the cause. Restrict the fallback to aFATALorPANICline that names the parameter or the configuration file.Also rename
INVALID_OK. The variable is set toyeswhen the cluster becomes ready, which is the failure condition for F3. A name such asREC_BECAME_READYstates the meaning.🤖 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/deployment/p7-c3-pitr-failure-drill.sh` around lines 232 - 245, In the F3 validation block, rename INVALID_OK to REC_BECAME_READY consistently and update the condition so readiness remains the failure case. Tighten the fallback grep in the docker compose logs check to accept only FATAL or PANIC lines that also identify recovery_target_lsn or the relevant configuration file, removing broad matches for error and stopped.scripts/deployment/p7-c3-pitr-drill.sh-200-207 (1)
200-207: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDisable archiving in the recovered cluster.
The base backup carries the source
postgresql.auto.conf, which containsarchive_mode = 'on'and thearchive_commandthat writes to/wal-archive. The recovery override mounts/wal-archiveread-only at Line 217. After promotion, everyarchive_commandinvocation fails, WAL accumulates inpg_wal, and the archiver retries forever.Append an explicit override so the recovered cluster does not archive.
🛠️ Proposed fix
cat >> /pg/postgresql.auto.conf <<CONF restore_command = 'cp /wal-archive/%f %p' recovery_target_lsn = '${RECOVERY_LSN}' recovery_target_inclusive = on recovery_target_action = 'promote' +archive_mode = 'off' CONF🤖 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/deployment/p7-c3-pitr-drill.sh` around lines 200 - 207, Update the recovery configuration block that appends to postgresql.auto.conf to explicitly disable archiving in the recovered cluster, overriding the inherited archive_mode/archive_command settings before promotion. Preserve the existing restore_command and recovery target settings.apps/web/src/pages/LaunchpadPage.tsx-75-88 (1)
75-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClient validation does not check the length rules the API enforces.
The bootstrap contract requires
adminUsernamewith at least 3 characters andadminPasswordwith at least 8 characters.validateonly checks for non-empty values. A short password therefore reaches the API, Fastify rejects it with a schema validation error, andgetApiErrorMessageshows the genericlaunchpad.errors.bootstrapFailedbanner. The operator receives no field-level reason on the only path that installs the product.Add the length checks and matching zh-CN messages.
🛡️ Proposed fix
if (!adminUsername.trim()) errors.adminUsername = t("launchpad.adminUsernameRequired"); + else if (adminUsername.trim().length < 3) + errors.adminUsername = t("launchpad.adminUsernameMinLength"); if (!adminPassword.trim()) errors.adminPassword = t("launchpad.adminPasswordRequired"); + else if (adminPassword.length < 8) + errors.adminPassword = t("launchpad.adminPasswordMinLength");Add to
apps/web/src/i18n/locales/zh-CN.tsunderlaunchpad:adminUsernameMinLength: "管理员用户名至少 3 位", adminPasswordMinLength: "管理员密码至少 8 位",🤖 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/pages/LaunchpadPage.tsx` around lines 75 - 88, Update the validate function to add field-level minimum-length checks for adminUsername (3 characters) and adminPassword (8 characters), while preserving the existing required checks. Add the corresponding launchpad.adminUsernameMinLength and launchpad.adminPasswordMinLength translations to zh-CN.ts and use them for the validation errors.apps/api/src/routes/launchpad.ts-128-177 (1)
128-177: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMap a lost bootstrap race to 409 instead of 500.
The initialized gate at Line 132 and the mutation at Line 163 are separate steps. Two concurrent bootstrap requests can both read
initialized === false. The database constraints serialize the writes, so the second transaction fails.bootstrapAdminOnFreshDbthrows a plainErrorin that path, which the error handler reports as 500. The operator then sees a server error for a state that is really "already initialized".Re-check the gate after a failure and return 409.
🛡️ Proposed fix
- const result = await bootstrapAdminOnFreshDb( - fastify.db, - { - username: data.adminUsername, - password: data.adminPassword, - name: data.adminName, - }, - orgOptions, - ); + let result; + try { + result = await bootstrapAdminOnFreshDb( + fastify.db, + { + username: data.adminUsername, + password: data.adminPassword, + name: data.adminName, + }, + orgOptions, + ); + } catch (err) { + // A concurrent bootstrap may have won the race. Re-read the gate so + // the loser reports the real state instead of a 500. + if (await isInstallationInitialized()) { + return reply + .code(409) + .send( + buildErrorResponse(request.id, "LAUNCHPAD_ALREADY_INITIALIZED"), + ); + } + throw err; + }🤖 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/api/src/routes/launchpad.ts` around lines 128 - 177, Wrap the bootstrapAdminOnFreshDb call in the route handler with failure handling that re-checks isInstallationInitialized() after an error; when the installation is now initialized, return the existing 409 LAUNCHPAD_ALREADY_INITIALIZED response, otherwise rethrow the original error so unrelated failures remain 500.apps/api/src/routes/launchpad.ts-73-75 (1)
73-75: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign
defaultOrganizationExists()with the repository ctx-first rule.
packages/db/src/repository/organizationRepo.tsdocumentsdefaultOrganizationExists()as a first-install probe and does not record it as an accepted ctx-less exception, while the repo contract requiresctxas the first argument. Keep this route path using the repository, but add a ctx parameter/acceptance if this read is intentionally pre-tenant.🤖 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/api/src/routes/launchpad.ts` around lines 73 - 75, Update isInstallationInitialized and the organization repository call so defaultOrganizationExists follows the ctx-first repository contract: pass an appropriate pre-tenant context or explicitly accept the ctx-less first-install probe in the repository API if that behavior is intentional. Keep the route using createOrganizationRepo and preserve its boolean initialization result.Source: Coding guidelines
scripts/deployment/p7-c1-persistence-smoke.sh-92-97 (1)
92-97: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCleanup guards assume
mktempreturns a/tmp/...path.mktemp -d -t <prefix>creates the directory under$TMPDIRwhen that variable is set (this is the default on macOS and in many CI images). The/tmp/...pattern then never matches, cleanup silently skips, and every run leaks a full PostgreSQL data directory. Match against the prefix the script itself used instead of a hard-coded/tmp, for example by comparing$(dirname "${d}")with themktempparent and checking the basename prefix.
scripts/deployment/p7-c1-persistence-smoke.sh#L92-L97: replace thegrep -Eq '/tmp/p7c1-persist-[AB]-...'test with a basename-prefix check against the directories recorded inCREATED_DIRS.scripts/deployment/p6-corr1-compose-smoke.sh#L120-L124: replace the^/tmp/p6corr1-smoke-data-test with the same basename-prefix check onEXAM_DATA_ROOT.scripts/deployment/p7-c1-cold-backup-restore-drill.sh#L63-L72: replace the/tmp/p7c1-colddrill-...test with the same basename-prefix check.scripts/deployment/p7-c2-logical-restore-drill.sh#L59-L66: replace the/tmp/p7c2-drill-...test with the same basename-prefix check.🤖 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/deployment/p7-c1-persistence-smoke.sh` around lines 92 - 97, Cleanup guards incorrectly hard-code /tmp, so they skip directories created under TMPDIR. In scripts/deployment/p7-c1-persistence-smoke.sh lines 92-97, scripts/deployment/p6-corr1-compose-smoke.sh lines 120-124, scripts/deployment/p7-c1-cold-backup-restore-drill.sh lines 63-72, and scripts/deployment/p7-c2-logical-restore-drill.sh lines 59-66, replace the absolute-path regexes with basename-prefix validation using each script’s recorded directory variable, while retaining the existing directory and cleanup safety checks.scripts/backup/postgres-logical-backup.sh-108-113 (1)
108-113: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove the partial artifact when
pg_dumpfails.The redirect at Line 113 creates
${DEST}beforepg_dumpwrites anything. Ifpg_dumpexits non-zero,set -eaborts the script immediately. The empty or truncated file remains, and the next run stops at the "destination already exists" check (Lines 72-75). Install a trap before the redirect.
TS(Line 108) is assigned but never used, and the header at Line 17 promises a timestamped dump. Either useTSin the artifact name or delete the variable.🐛 Proposed fix
-TS="$(date -u +%Y%m%dT%H%M%SZ)" +cleanup_partial() { rm -f "${DEST}"; } +trap cleanup_partial EXIT docker exec \ -e PGPASSWORD="${PGPASSWORD:-}" \ "${DB_CONTAINER}" \ sh -c 'pg_dump -U "$POSTGRES_USER" -d "$POSTGRES_DB" -Fc --no-owner' \ > "${DEST}"Clear the trap (
trap - EXIT) after all verification steps succeed.🤖 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/backup/postgres-logical-backup.sh` around lines 108 - 113, Update the backup flow around TS and the pg_dump redirect: use TS in the destination artifact name to preserve the promised timestamped naming, then install an EXIT trap before pg_dump that removes DEST when the command fails. Clear the trap with trap - EXIT only after all verification steps succeed, while preserving the existing destination-exists checks.Source: Linters/SAST tools
🧹 Nitpick comments (19)
scripts/deployment/p7-c3-pitr-drill.sh (3)
131-134: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFail explicitly when the app never becomes healthy.
The loop exits after 90 attempts without a status check. The script then runs
bootstrap-admin.jsagainst an unhealthy container. The resulting error does not identify the real cause.♻️ Proposed change
-for i in $(seq 1 90); do +APP_READY="no" +for _ in $(seq 1 90); do docker exec "${PROJECT_SRC}-app-1" node -e "fetch('http://127.0.0.1:3000/api/health').then(r=>process.exit(r.ok?0:1)).catch(()=>process.exit(1))" >/dev/null 2>&1 && break sleep 1 done +docker exec "${PROJECT_SRC}-app-1" node -e "fetch('http://127.0.0.1:3000/api/health').then(r=>process.exit(r.ok?0:1)).catch(()=>process.exit(1))" >/dev/null 2>&1 || { + echo "FAIL: app never became healthy." >&2 + exit 1 +}🤖 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/deployment/p7-c3-pitr-drill.sh` around lines 131 - 134, Track whether the health-check loop in the deployment script succeeded, and after the loop explicitly fail with a clear error if the app never becomes healthy within 90 attempts. Only run bootstrap-admin.js after a successful health check.
61-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit declaration and assignment.
export VAR="$(cmd)"masks the exit status ofopenssl. Shellcheck reports SC2155 on both lines.♻️ Proposed change
-export POSTGRES_PASSWORD="p7c3-src-pg-$(openssl rand -hex 6)" +POSTGRES_PASSWORD="p7c3-src-pg-$(openssl rand -hex 6)" +export POSTGRES_PASSWORD SHARED_PG_PASSWORD="${POSTGRES_PASSWORD}" -export JWT_SECRET="p7c3-src-jwt-$(openssl rand -hex 16)" +JWT_SECRET="p7c3-src-jwt-$(openssl rand -hex 16)" +export JWT_SECRET🤖 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/deployment/p7-c3-pitr-drill.sh` around lines 61 - 63, Split the declarations and command substitutions for POSTGRES_PASSWORD and JWT_SECRET in the deployment script: assign each generated value first, then export the variable separately. Preserve the existing generated prefixes and openssl arguments while ensuring openssl failures are not masked.Source: Linters/SAST tools
227-232: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
_for the unused loop variable.Shellcheck reports SC2034 because
iis never read. The loop at Line 80 already uses_.♻️ Proposed change
-for i in $(seq 1 90); do +for _ in $(seq 1 90); do🤖 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/deployment/p7-c3-pitr-drill.sh` around lines 227 - 232, Replace the unused loop variable i in the pg_isready retry loop with _, matching the existing loop convention elsewhere in the script and eliminating ShellCheck SC2034.Source: Linters/SAST tools
scripts/deployment/p7-c3-pitr-failure-drill.sh (2)
278-290: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd
PROJECT_MISSto the cleanup trap.The trap at Line 64 tears down only
PROJECT_SRCandPROJECT_REC.PROJECT_MISSis torn down at Line 312 and again at Lines 315-318. If the script exits between Line 290 and Line 312, the F1 containers stay running. DeclarePROJECT_MISSbefore the trap, add it to the loop incleanup, and delete the redundantPROJECTS_EXTRAblock.🤖 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/deployment/p7-c3-pitr-failure-drill.sh` around lines 278 - 290, Declare PROJECT_MISS before the cleanup trap so cleanup can access it, add PROJECT_MISS to the project loop in cleanup alongside PROJECT_SRC and PROJECT_REC, and remove the later PROJECTS_EXTRA cleanup block and its duplicate PROJECT_MISS teardown.
162-174: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRun
pg_verifybackuponce.Lines 162 and 165 start the same container with the same arguments. The second run only re-derives the exit status. Capture the output and the status in a single invocation.
♻️ Proposed change
- VRF_OUT="$(docker run --rm -v "${CORRUPT_DIR}:/d:ro" \ - postgres:18.4-bookworm pg_verifybackup /d 2>&1 || true)" - # pg_verifybackup exits non-zero and emits a message about the mismatch. - if docker run --rm -v "${CORRUPT_DIR}:/d:ro" \ - postgres:18.4-bookworm pg_verifybackup /d >/dev/null 2>&1; then + VRF_STATUS=0 + VRF_OUT="$(docker run --rm -v "${CORRUPT_DIR}:/d:ro" \ + postgres:18.4-bookworm pg_verifybackup /d 2>&1)" || VRF_STATUS=$? + if [ "${VRF_STATUS}" -eq 0 ]; then fail "F2 corrupt-backup detection — pg_verifybackup accepted a tampered file"🤖 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/deployment/p7-c3-pitr-failure-drill.sh` around lines 162 - 174, Update the F2 corrupt-backup verification flow to invoke pg_verifybackup only once, capturing both its combined output and exit status from that invocation. Use the captured status for the acceptance/failure check while preserving the existing diagnostic-message matching and pass messages.docker-compose.pitr.yml (1)
31-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the empty
POSTGRES_INITDB_ARGSoverride and the misleading comment.The comment states that this block enables continuous archiving. Setting
POSTGRES_INITDB_ARGSto an empty string does not enable archiving. It only overwrites any value inherited from the base compose file or the environment.♻️ Proposed cleanup
db: - environment: - # Enable continuous archiving. archive_mode is a postmaster-level - # setting (requires restart, not just reload). - POSTGRES_INITDB_ARGS: ""🤖 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 `@docker-compose.pitr.yml` around lines 31 - 34, Remove the empty POSTGRES_INITDB_ARGS entry and its misleading continuous-archiving comment from the environment block; leave inherited or externally provided configuration untouched.apps/web/src/pages/LaunchpadPage.tsx (1)
117-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared page shell.
The loading branch at Lines 120-134 and the main return at Lines 138-146 repeat the same
main,PageContainer,Card,CardHeader, andBrandHeaderstructure. A later change to the shell must be applied twice.Extract a local
Shellwrapper and render the loading state as<Shell />and the form as<Shell>{form}</Shell>.🤖 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/pages/LaunchpadPage.tsx` around lines 117 - 146, In the LaunchpadPage component, extract the duplicated main/PageContainer/Card/CardHeader/BrandHeader structure into a local Shell wrapper. Replace the loading branch with <Shell /> and wrap the existing form content with <Shell>{form}</Shell>, preserving the current loading and form behavior.apps/api/src/routes/launchpad.test.ts (1)
36-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe hardcoded table list will drift from the schema.
The
TRUNCATEstatement names 28 tables literally. A new table added later is not truncated, so a stale row can leave the installation in an initialized state and make the bootstrap tests fail in a way that is hard to diagnose.Derive the list from
information_schema.tablesand exclude the Drizzle metadata tables, or reuse the existingtruncateBusinessTableshelper referenced in the doc comment.♻️ Proposed refactor
async function resetToUninitialized(ctx: TestContext): Promise<void> { - await ctx.db.execute( - sql.raw(` - TRUNCATE - organizations, - ... - RESTART IDENTITY CASCADE - `), - ); + await ctx.db.execute( + sql.raw(` + DO $$ + DECLARE stmt text; + BEGIN + SELECT string_agg(format('%I.%I', schemaname, tablename), ', ') + INTO stmt + FROM pg_tables + WHERE schemaname = 'public' + AND tablename NOT LIKE '__drizzle%'; + IF stmt IS NOT NULL THEN + EXECUTE 'TRUNCATE ' || stmt || ' RESTART IDENTITY CASCADE'; + END IF; + END $$; + `), + ); }🤖 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/api/src/routes/launchpad.test.ts` around lines 36 - 72, Update resetToUninitialized to avoid its hardcoded table list: reuse the existing truncateBusinessTables helper if available, or derive tables from information_schema.tables while excluding Drizzle metadata tables, then truncate the resulting business-table list with identity reset and cascade.apps/api/src/routes/launchpad.ts (1)
24-33: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCompare fixed-length digests instead of raw buffers.
The length-mismatch branch returns before the real comparison. The self-compare uses
bBuf, so its duration follows the configured token length and not the candidate length. The comment claims wall-clock independence from length, which the current code does not provide.A SHA-256 digest comparison produces two 32-byte buffers, so
timingSafeEqualalways runs on equal lengths and no length branch is needed. The practical risk here is low because the token is high entropy and the route is rate limited, but the simpler form also matches the documented intent.♻️ Proposed refactor
-import { timingSafeEqual } from "node:crypto"; +import { createHash, timingSafeEqual } from "node:crypto"; @@ function constantTimeEqual(a: string, b: string): boolean { - const aBuf = Buffer.from(a, "utf8"); - const bBuf = Buffer.from(b, "utf8"); - if (aBuf.length !== bBuf.length) { - // Still do a comparison to keep wall-clock time independent of length. - timingSafeEqual(bBuf, bBuf); - return false; - } - return timingSafeEqual(aBuf, bBuf); + // Digests are always 32 bytes, so the comparison never branches on the + // secret length. + const aDigest = createHash("sha256").update(a, "utf8").digest(); + const bDigest = createHash("sha256").update(b, "utf8").digest(); + return timingSafeEqual(aDigest, bDigest); }🤖 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/api/src/routes/launchpad.ts` around lines 24 - 33, Update constantTimeEqual to hash both input strings with SHA-256 and pass the resulting fixed-length digests to timingSafeEqual. Remove the raw-buffer length check and self-comparison so every call performs one equal-length digest comparison.scripts/deployment/p7-c1-cold-backup-restore-drill.sh (1)
128-133: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the application starts against the restored data root.
The drill only waits for the database after the restore. The stated goal (Lines 19-21) is a fresh working deployment. Call
wait_for_appafterwait_for_dbso a failing application start fails the drill.🤖 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/deployment/p7-c1-cold-backup-restore-drill.sh` around lines 128 - 133, After wait_for_db in the RESTORE_ROOT startup flow, call wait_for_app before capture so the drill fails when the application cannot start against restored data.scripts/backup/cold-filesystem-backup.sh (3)
119-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCreate the destination after the confirmation prompt.
mkdir -p "${DEST}/postgres"runs at Line 127, before the prompt at Line 136. If the operator aborts with Ctrl-C,${DEST}remains. The next attempt then fails at the "destination already exists" check (Lines 96-100) and the operator must delete the directory by hand. Move themkdircall below the prompt.🤖 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/backup/cold-filesystem-backup.sh` around lines 119 - 136, Move the mkdir -p "${DEST}/postgres" call in the cold-filesystem backup flow to after the read -r _confirm prompt, leaving the destination-parent validation and informational output before confirmation. Ensure aborting before confirmation does not create or leave the destination directory.
144-148: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the helper image and pre-load it.
alpine:latestis unpinned and is pulled from Docker Hub on first use. See the consolidated comment for the required change.🤖 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/backup/cold-filesystem-backup.sh` around lines 144 - 148, Update the Docker invocation in the cold filesystem backup flow to use the pinned Alpine helper image version specified by the consolidated review guidance instead of alpine:latest, and add the required pre-load step before docker run so the image is available locally without an implicit Docker Hub pull.Source: Coding guidelines
102-117: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDetect a running or uncleanly stopped cluster before copying.
The script documents that PostgreSQL must be stopped but never checks. The validation container already reads PGDATA, so add a
postmaster.pidcheck there. Ifpostmaster.pidexists, the cluster is running or was not shut down cleanly, and the resulting copy is not a trustworthy cold backup.Also, the header comment at Lines 70-72 states that
validate_pathrejects..traversal. No such check exists. Add the check or correct the comment.🛡️ Proposed guard
PGDATA_DIR="$(dirname "${PGDATA_SUBDIR}")" +if docker run --rm -v "${SRC_PG}:/pg:ro" alpine:latest \ + sh -c "test -f '${PGDATA_DIR}/postmaster.pid'" 2>/dev/null; then + echo "FAIL: ${PGDATA_DIR}/postmaster.pid exists." >&2 + echo " PostgreSQL is running or was not stopped cleanly. Stop it first." >&2 + exit 2 +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/backup/cold-filesystem-backup.sh` around lines 102 - 117, Add a cold-backup guard to the PGDATA validation flow: use the existing Docker helper container and resolved PGDATA_DIR to fail when postmaster.pid exists, before copying begins. Also reconcile the validate_path documentation with its implementation by either adding the promised .. traversal rejection or correcting the header comment if that validation is intentionally absent.scripts/backup/pg-basebackup.sh (2)
137-146: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the images used by the backup path.
pg_verifybackuprunspostgres:18.4-bookwormand the size report runsalpine:latest. Both are pulled from Docker Hub when absent. See the consolidated comment.🤖 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/backup/pg-basebackup.sh` around lines 137 - 146, Pin the Docker images used in the backup verification and size-report commands instead of using mutable tags. Update the postgres image in the pg_verifybackup invocation and the alpine image in the SIZE calculation to immutable version or digest references, preserving their existing command behavior.Source: Coding guidelines
116-128: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAlign the pg_basebackup docs with the actual host-based replication path.
The script starts a separate backup container and connects with
-h 127.0.0.1 -U "${PGUSER:-exam}", so this is TCP replication authentication, not a container-localpeerpath. Update the L19-23/L95-100 comments to match the implementation: the bundled single-node path uses the db container loopback namespace with the configuredexamsuperuser, while external deployments should use a scope-limited replication role.🤖 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/backup/pg-basebackup.sh` around lines 116 - 128, Update the comments around the bundled and external backup authentication paths to match the pg_basebackup invocation: document that the bundled single-node flow uses the backup container’s database loopback namespace and configured exam superuser over TCP, while external deployments should use a scope-limited replication role rather than container-local peer authentication.scripts/deployment/p7-c1-persistence-smoke.sh (1)
88-97: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHost-side
rm -rfcannot delete the uid-999 PGDATA files.The script correctly uses a container to copy and inspect these files (Lines 217-221, Lines 242-243), but cleanup removes them from the host. A non-root host user gets EACCES, and
|| truehides it, so each run leaks a full data directory. Use the same container-assisted removal asscripts/deployment/p7-c1-cold-backup-restore-drill.sh(Lines 66-70).The
/tmp/...guard also assumesTMPDIRis unset; see the consolidated comment.♻️ Container-assisted removal
for d in "${CREATED_DIRS[@]}"; do if [ -n "${d}" ] && [ -d "${d}" ] \ && printf '%s\n' "${d}" | grep -Eq '/tmp/p7c1-persist-[AB]-[A-Za-z0-9_-]+$'; then + docker run --rm -v "${d}:/d" alpine:latest \ + sh -c 'rm -rf /d/* /d/.[!.]* 2>/dev/null || true' > /dev/null 2>&1 || true rm -rf "${d}" > /dev/null 2>&1 || true fi done🤖 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/deployment/p7-c1-persistence-smoke.sh` around lines 88 - 97, Update the cleanup loop over CREATED_DIRS to remove validated temporary directories through the same container-assisted removal mechanism used by p7-c1-cold-backup-restore-drill.sh, so uid-999 PGDATA files are deleted without relying on host permissions. Preserve the existing non-empty, directory, and script-created-path safety checks, and ensure the validation supports the configured TMPDIR rather than assuming /tmp.scripts/deployment/p7-c2-logical-restore-drill.sh (2)
172-181: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCompare the restored state against the captured State A.
The drill captures
STATE_Aat Line 130 andSTATE_Bat Line 144, then asserts hard-coded counts of1. Both variables are otherwise unused. Comparecapture_stateoutput againstSTATE_A(with the B marker expected absent) so the assertion follows the recorded baseline instead of constants that drift when bootstrap behavior changes.🤖 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/deployment/p7-c2-logical-restore-drill.sh` around lines 172 - 181, Update the restored-state validation after the RESTORED_ORGS, RESTORED_ADMINS, and RESTORED_AUDIT queries to compare the capture_state output against the recorded STATE_A baseline instead of hard-coded counts. Reuse STATE_A for the organization, admin, and audit expectations, while separately verifying the STATE_B marker is absent; remove the unused-state issue and preserve failure diagnostics for mismatches.
149-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDelete the no-op line.
ADMIN_USERis already used at Line 124 and Line 192, so this statement suppresses nothing. It is a debug artifact.♻️ Proposed removal
-echo "${ADMIN_USER%%_*}" >/dev/null # no-op to keep shell lint happy🤖 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/deployment/p7-c2-logical-restore-drill.sh` at line 149, Remove the no-op echo statement involving ADMIN_USER. Keep the existing ADMIN_USER usages elsewhere unchanged.scripts/deployment/p6-corr1-compose-smoke.sh (1)
113-124: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCleanup cannot remove the PGDATA files it created.
The bind-mounted PostgreSQL files are owned by the container postgres user (uid 999). A host-side
rm -rfby a non-root user fails with EACCES, and|| truehides the failure. Each smoke run then leaks a full data directory.scripts/deployment/p7-c1-cold-backup-restore-drill.sh(Lines 66-70) already uses a container-assisted removal; apply the same approach here.The
^/tmp/guard is also unreliable whenTMPDIRis set; see the consolidated comment.♻️ Container-assisted removal
if [ -n "${EXAM_DATA_ROOT:-}" ] \ && [ -d "${EXAM_DATA_ROOT}" ] \ && printf '%s\n' "${EXAM_DATA_ROOT}" | grep -q '^/tmp/p6corr1-smoke-data-'; then + docker run --rm -v "${EXAM_DATA_ROOT}:/d" "${HELPER_IMAGE}" \ + sh -c 'rm -rf /d/* /d/.[!.]* 2>/dev/null || true' > /dev/null 2>&1 || true rm -rf "${EXAM_DATA_ROOT}" > /dev/null 2>&1 || true 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/deployment/p6-corr1-compose-smoke.sh` around lines 113 - 124, Update the cleanup block in scripts/deployment/p6-corr1-compose-smoke.sh to remove EXAM_DATA_ROOT through a temporary container, matching the container-assisted removal approach used by p7-c1-cold-backup-restore-drill.sh so root-owned PostgreSQL files are deleted. Replace the hard-coded ^/tmp/p6corr1-smoke-data- validation with a guard that validates the actual mktemp-created path under the configured TMPDIR and prevents empty, repository-root, or unowned paths from being removed.
🤖 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 `@docker-compose.pitr.yml`:
- Line 45: Rename the WAL archive seed from docker/pitr/wal-archive.conf to
docker/pitr/wal-archive.sql, convert its leading # comments to SQL -- comments,
and update the mount target at docker-compose.pitr.yml lines 45-45 to
/docker-entrypoint-initdb.d/99-pitr-wal-archive.sql. Verify end to end with a
fresh PITR cluster that archive_mode is on and a WAL segment is actually
archived; do not rely on file contents alone.
In `@docker-compose.yml`:
- Around line 100-103: Document the existing-install migration in the operator
runbook: before starting the updated stack, stop PostgreSQL, copy the contents
of the existing pgdata named volume into ${EXAM_DATA_ROOT}/postgres, and then
start the stack with the bind mount. Ensure the runbook clearly warns that
skipping this preserves data only in the orphaned volume while initializing an
empty cluster.
In `@docker/pitr/wal-archive.conf`:
- Around line 26-28: Document the host-side preparation required for
/wal-archive in both this configuration and
docs/deployment/backup-and-recovery.md: ensure the bind-mounted directory is
created with ownership by PostgreSQL uid 999 (or an equivalently restrictive
writable setup) before starting the container, rather than relying on 0777
permissions. Keep the existing archive_command and ALTER SYSTEM settings
unchanged.
In `@docs/deployment/backup-and-recovery.md`:
- Line 183: Update the cold backup command in
docs/deployment/backup-and-recovery.md lines 183-183 to pass
"${EXAM_DATA_ROOT:-./data}" instead of ./data; make the same replacement in
docs/deployment/mvp-deployment-runbook.md lines 804-809 so both examples use the
resolved EXAM_DATA_ROOT with the existing fallback.
- Around line 539-551: Remove administrator passwords from command-line
arguments in the bootstrap and reset procedures:
docs/deployment/backup-and-recovery.md lines 539-551 must use an interactive
prompt or protected stdin/file input for both commands, and
docs/deployment/mvp-deployment-runbook.md lines 174-177 must update its
bootstrap fallback to the same safe input method.
In `@packages/db/src/repository/organizationRepo.ts`:
- Around line 118-124: Add ctx: RequestContext as the first parameter of
defaultOrganizationExists, preserving its existing query behavior. In
apps/api/src/routes/launchpad.ts, create the request context and pass it to
defaultOrganizationExists(ctx), ensuring the repository call follows the
standard contract.
In `@scripts/backup/cold-filesystem-restore.sh`:
- Around line 127-131: Introduce one overridable EXAM_BACKUP_HELPER_IMAGE
variable with a digest-pinned default and use it in cold-filesystem-restore.sh
lines 127-131, cold-filesystem-backup.sh lines 144-148 and its validation
containers at lines 106 and 113, and the du container in pg-basebackup.sh lines
137-146; keep the postgres image for pg_verifybackup digest-pinned separately.
Before any helper image use, run docker image inspect and fail clearly if it is
unavailable locally, then document docker save/docker load preloading in
docs/deployment/backup-and-recovery.md so recovery remains offline-capable.
In `@scripts/backup/postgres-logical-backup.sh`:
- Line 110: Update the Docker invocation around the backup commands to avoid
expanding PGPASSWORD into docker exec arguments. Pass the variable by name
through the existing environment or use a securely permissioned temporary
env-file when value expansion is required, and apply the same fix to both
affected invocations.
In `@scripts/backup/postgres-logical-restore.sh`:
- Around line 112-126: Validate TARGET_DB before any SQL or shell interpolation,
allowing only a plain PostgreSQL identifier with an appropriate safe character
pattern and rejecting all other values; retain the existing reserved-name
checks. Update the DROP DATABASE statement in the restore flow to use WITH
(FORCE), preserving the recreate-and-restore sequence and preventing connected
clients from interrupting it.
In `@scripts/deployment/p7-c1-cold-backup-restore-drill.sh`:
- Around line 98-102: Update the capture function to invoke psql with
ON_ERROR_STOP=1 and validate that its result is non-empty, failing the drill
when the query errors or returns no evidence. Ensure the before/after comparison
and PASS path around INV_BEFORE and INV_AFTER cannot treat two empty captures as
a successful restore.
In `@scripts/deployment/p7-c3-pitr-failure-drill.sh`:
- Around line 297-311: Tighten the F1 validation around MISS_LOGS and pg_isready
so it requires evidence of an actual restore_command/WAL retrieval failure
rather than generic recovery terms. Record or match the concrete missing-segment
failure message, and assert that the recovery node remains unavailable; do not
pass solely because broad log patterns appear or because the node starts with
non-specific restore errors.
---
Minor comments:
In `@apps/api/src/routes/launchpad.ts`:
- Around line 128-177: Wrap the bootstrapAdminOnFreshDb call in the route
handler with failure handling that re-checks isInstallationInitialized() after
an error; when the installation is now initialized, return the existing 409
LAUNCHPAD_ALREADY_INITIALIZED response, otherwise rethrow the original error so
unrelated failures remain 500.
- Around line 73-75: Update isInstallationInitialized and the organization
repository call so defaultOrganizationExists follows the ctx-first repository
contract: pass an appropriate pre-tenant context or explicitly accept the
ctx-less first-install probe in the repository API if that behavior is
intentional. Keep the route using createOrganizationRepo and preserve its
boolean initialization result.
In `@apps/web/src/pages/LaunchpadPage.tsx`:
- Around line 75-88: Update the validate function to add field-level
minimum-length checks for adminUsername (3 characters) and adminPassword (8
characters), while preserving the existing required checks. Add the
corresponding launchpad.adminUsernameMinLength and
launchpad.adminPasswordMinLength translations to zh-CN.ts and use them for the
validation errors.
In `@docs/audits/P7-C-PORTABLE-BACKUP-RECOVERY-CLOSEOUT.md`:
- Around line 140-146: Update the final closeout report fields around the
existing “Final gates on the final head” section to explicitly report modified
files, new-test results, coverage, and `pnpm verify` results, stating when any
item was not collected or not run. Preserve the existing gate results and
limitations.
In `@docs/deployment/backup-and-recovery.md`:
- Around line 267-276: Update the clean-target contract description to remove
“byte-for-byte” language and state that the C2 restore via DROP DATABASE, CREATE
DATABASE ... TEMPLATE template0, and pg_restore reproduces the dump’s logical
contents and validated business invariants. Preserve the explanation that
State-B-only data is absent and adjust the automated drill wording accordingly.
- Around line 44-51: Update the deployment documentation to distinguish the host
PostgreSQL data root from the image-managed PGDATA directory: describe
data/postgres as the host mount parent, and reserve PGDATA for the nested
postgres/18/docker directory used by the PostgreSQL image. Preserve the warning
that deleting the host data root destroys authoritative Exam state.
In `@docs/deployment/mvp-deployment-runbook.md`:
- Around line 179-183: Update the backup-and-recovery cross-reference in the
Launchpad first-install instructions to point to §11 instead of §8, leaving the
rest of the setup procedure unchanged.
In `@docs/roadmap/current.md`:
- Line 15: Update the P7 roadmap entry’s config-control-plane reference to use
the correct Workstream E designation, removing the incorrect “(P7-E)”
association while preserving the rest of the status and workstream text.
In `@scripts/backup/postgres-logical-backup.sh`:
- Around line 108-113: Update the backup flow around TS and the pg_dump
redirect: use TS in the destination artifact name to preserve the promised
timestamped naming, then install an EXIT trap before pg_dump that removes DEST
when the command fails. Clear the trap with trap - EXIT only after all
verification steps succeed, while preserving the existing destination-exists
checks.
In `@scripts/deployment/p7-c1-persistence-smoke.sh`:
- Around line 92-97: Cleanup guards incorrectly hard-code /tmp, so they skip
directories created under TMPDIR. In
scripts/deployment/p7-c1-persistence-smoke.sh lines 92-97,
scripts/deployment/p6-corr1-compose-smoke.sh lines 120-124,
scripts/deployment/p7-c1-cold-backup-restore-drill.sh lines 63-72, and
scripts/deployment/p7-c2-logical-restore-drill.sh lines 59-66, replace the
absolute-path regexes with basename-prefix validation using each script’s
recorded directory variable, while retaining the existing directory and cleanup
safety checks.
In `@scripts/deployment/p7-c3-pitr-drill.sh`:
- Around line 200-207: Update the recovery configuration block that appends to
postgresql.auto.conf to explicitly disable archiving in the recovered cluster,
overriding the inherited archive_mode/archive_command settings before promotion.
Preserve the existing restore_command and recovery target settings.
In `@scripts/deployment/p7-c3-pitr-failure-drill.sh`:
- Around line 232-245: In the F3 validation block, rename INVALID_OK to
REC_BECAME_READY consistently and update the condition so readiness remains the
failure case. Tighten the fallback grep in the docker compose logs check to
accept only FATAL or PANIC lines that also identify recovery_target_lsn or the
relevant configuration file, removing broad matches for error and stopped.
---
Nitpick comments:
In `@apps/api/src/routes/launchpad.test.ts`:
- Around line 36-72: Update resetToUninitialized to avoid its hardcoded table
list: reuse the existing truncateBusinessTables helper if available, or derive
tables from information_schema.tables while excluding Drizzle metadata tables,
then truncate the resulting business-table list with identity reset and cascade.
In `@apps/api/src/routes/launchpad.ts`:
- Around line 24-33: Update constantTimeEqual to hash both input strings with
SHA-256 and pass the resulting fixed-length digests to timingSafeEqual. Remove
the raw-buffer length check and self-comparison so every call performs one
equal-length digest comparison.
In `@apps/web/src/pages/LaunchpadPage.tsx`:
- Around line 117-146: In the LaunchpadPage component, extract the duplicated
main/PageContainer/Card/CardHeader/BrandHeader structure into a local Shell
wrapper. Replace the loading branch with <Shell /> and wrap the existing form
content with <Shell>{form}</Shell>, preserving the current loading and form
behavior.
In `@docker-compose.pitr.yml`:
- Around line 31-34: Remove the empty POSTGRES_INITDB_ARGS entry and its
misleading continuous-archiving comment from the environment block; leave
inherited or externally provided configuration untouched.
In `@scripts/backup/cold-filesystem-backup.sh`:
- Around line 119-136: Move the mkdir -p "${DEST}/postgres" call in the
cold-filesystem backup flow to after the read -r _confirm prompt, leaving the
destination-parent validation and informational output before confirmation.
Ensure aborting before confirmation does not create or leave the destination
directory.
- Around line 144-148: Update the Docker invocation in the cold filesystem
backup flow to use the pinned Alpine helper image version specified by the
consolidated review guidance instead of alpine:latest, and add the required
pre-load step before docker run so the image is available locally without an
implicit Docker Hub pull.
- Around line 102-117: Add a cold-backup guard to the PGDATA validation flow:
use the existing Docker helper container and resolved PGDATA_DIR to fail when
postmaster.pid exists, before copying begins. Also reconcile the validate_path
documentation with its implementation by either adding the promised .. traversal
rejection or correcting the header comment if that validation is intentionally
absent.
In `@scripts/backup/pg-basebackup.sh`:
- Around line 137-146: Pin the Docker images used in the backup verification and
size-report commands instead of using mutable tags. Update the postgres image in
the pg_verifybackup invocation and the alpine image in the SIZE calculation to
immutable version or digest references, preserving their existing command
behavior.
- Around line 116-128: Update the comments around the bundled and external
backup authentication paths to match the pg_basebackup invocation: document that
the bundled single-node flow uses the backup container’s database loopback
namespace and configured exam superuser over TCP, while external deployments
should use a scope-limited replication role rather than container-local peer
authentication.
In `@scripts/deployment/p6-corr1-compose-smoke.sh`:
- Around line 113-124: Update the cleanup block in
scripts/deployment/p6-corr1-compose-smoke.sh to remove EXAM_DATA_ROOT through a
temporary container, matching the container-assisted removal approach used by
p7-c1-cold-backup-restore-drill.sh so root-owned PostgreSQL files are deleted.
Replace the hard-coded ^/tmp/p6corr1-smoke-data- validation with a guard that
validates the actual mktemp-created path under the configured TMPDIR and
prevents empty, repository-root, or unowned paths from being removed.
In `@scripts/deployment/p7-c1-cold-backup-restore-drill.sh`:
- Around line 128-133: After wait_for_db in the RESTORE_ROOT startup flow, call
wait_for_app before capture so the drill fails when the application cannot start
against restored data.
In `@scripts/deployment/p7-c1-persistence-smoke.sh`:
- Around line 88-97: Update the cleanup loop over CREATED_DIRS to remove
validated temporary directories through the same container-assisted removal
mechanism used by p7-c1-cold-backup-restore-drill.sh, so uid-999 PGDATA files
are deleted without relying on host permissions. Preserve the existing
non-empty, directory, and script-created-path safety checks, and ensure the
validation supports the configured TMPDIR rather than assuming /tmp.
In `@scripts/deployment/p7-c2-logical-restore-drill.sh`:
- Around line 172-181: Update the restored-state validation after the
RESTORED_ORGS, RESTORED_ADMINS, and RESTORED_AUDIT queries to compare the
capture_state output against the recorded STATE_A baseline instead of hard-coded
counts. Reuse STATE_A for the organization, admin, and audit expectations, while
separately verifying the STATE_B marker is absent; remove the unused-state issue
and preserve failure diagnostics for mismatches.
- Line 149: Remove the no-op echo statement involving ADMIN_USER. Keep the
existing ADMIN_USER usages elsewhere unchanged.
In `@scripts/deployment/p7-c3-pitr-drill.sh`:
- Around line 131-134: Track whether the health-check loop in the deployment
script succeeded, and after the loop explicitly fail with a clear error if the
app never becomes healthy within 90 attempts. Only run bootstrap-admin.js after
a successful health check.
- Around line 61-63: Split the declarations and command substitutions for
POSTGRES_PASSWORD and JWT_SECRET in the deployment script: assign each generated
value first, then export the variable separately. Preserve the existing
generated prefixes and openssl arguments while ensuring openssl failures are not
masked.
- Around line 227-232: Replace the unused loop variable i in the pg_isready
retry loop with _, matching the existing loop convention elsewhere in the script
and eliminating ShellCheck SC2034.
In `@scripts/deployment/p7-c3-pitr-failure-drill.sh`:
- Around line 278-290: Declare PROJECT_MISS before the cleanup trap so cleanup
can access it, add PROJECT_MISS to the project loop in cleanup alongside
PROJECT_SRC and PROJECT_REC, and remove the later PROJECTS_EXTRA cleanup block
and its duplicate PROJECT_MISS teardown.
- Around line 162-174: Update the F2 corrupt-backup verification flow to invoke
pg_verifybackup only once, capturing both its combined output and exit status
from that invocation. Use the captured status for the acceptance/failure check
while preserving the existing diagnostic-message matching and pass messages.
🪄 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: b4d3bc05-2535-4ed9-973c-ce486b646f80
📒 Files selected for processing (37)
.gitignoreREADME.mdapps/api/openapi.jsonapps/api/src/authz/routeRegistryConformanceWholeApp.test.tsapps/api/src/config/runtimeConfig.tsapps/api/src/routes/launchpad.test.tsapps/api/src/routes/launchpad.tsapps/api/src/routes/registerApiRoutes.tsapps/web/src/App.tsxapps/web/src/i18n/locales/zh-CN.tsapps/web/src/lib/pageMeta.tsapps/web/src/lib/routes.tsapps/web/src/pages/LaunchpadPage.test.tsxapps/web/src/pages/LaunchpadPage.tsxdocker-compose.pitr.ymldocker-compose.ymldocker/pitr/wal-archive.confdocs/audits/P7-C-PORTABLE-BACKUP-RECOVERY-CLOSEOUT.mddocs/deployment/backup-and-recovery.mddocs/deployment/mvp-deployment-runbook.mddocs/roadmap/P7-system-readiness-and-exam-modes.mddocs/roadmap/current.mdpackages/contracts/src/index.tspackages/contracts/src/launchpad.tspackages/contracts/src/messageRegistry.tspackages/db/src/repository/organizationRepo.tsscripts/backup/cold-filesystem-backup.shscripts/backup/cold-filesystem-restore.shscripts/backup/pg-basebackup.shscripts/backup/postgres-logical-backup.shscripts/backup/postgres-logical-restore.shscripts/deployment/p6-corr1-compose-smoke.shscripts/deployment/p7-c1-cold-backup-restore-drill.shscripts/deployment/p7-c1-persistence-smoke.shscripts/deployment/p7-c2-logical-restore-drill.shscripts/deployment/p7-c3-pitr-drill.shscripts/deployment/p7-c3-pitr-failure-drill.sh
| # P7-C1: authoritative state is an operator-visible host bind mount. | ||
| # Containers are disposable; this host directory is not. The official | ||
| # postgres image owns its internal PGDATA layout under this path. | ||
| - ${EXAM_DATA_ROOT:-./data}/postgres:/var/lib/postgresql |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Document the named-volume to bind-mount migration for existing deployments.
Existing production stacks keep authoritative data in the pgdata named volume. After this change, Compose mounts an empty host directory and the postgres image initializes a new cluster. The old data remains in the orphaned volume and the deployment looks empty. Confirm that the upgrade procedure (copy pgdata contents into ${EXAM_DATA_ROOT}/postgres while PostgreSQL is stopped) is documented in the operator runbook before release.
#!/bin/bash
# Verify the volume -> bind-mount migration is documented for existing installs.
fd -t f -e md . docs README.md 2>/dev/null | xargs rg -n -C3 -i 'pgdata|named volume|docker volume|EXAM_DATA_ROOT' || true🤖 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 `@docker-compose.yml` around lines 100 - 103, Document the existing-install
migration in the operator runbook: before starting the updated stack, stop
PostgreSQL, copy the contents of the existing pgdata named volume into
${EXAM_DATA_ROOT}/postgres, and then start the stack with the bind mount. Ensure
the runbook clearly warns that skipping this preserves data only in the orphaned
volume while initializing an empty cluster.
| ```bash | ||
| docker compose exec app \ | ||
| node dist/scripts/bootstrap-admin.js \ | ||
| --username admin --password '<STRONG_OPERATOR_PASSWORD>' \ | ||
| --name 'System Admin' --organization-name 'My Organization' | ||
| ``` | ||
|
|
||
| ### 11.3 Reset an Admin's password | ||
|
|
||
| ```bash | ||
| docker compose exec app \ | ||
| node dist/scripts/reset-admin-password.js \ | ||
| --username admin --password '<NEW_STRONG_PASSWORD>' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Remove administrator passwords from command-line arguments. Both procedures expose credentials through shell history and process inspection.
docs/deployment/backup-and-recovery.md#L539-L551: update bootstrap and password-reset procedures to use an interactive prompt or protected stdin/file input.docs/deployment/mvp-deployment-runbook.md#L174-L177: update the bootstrap fallback to use the same safe input method.
📍 Affects 2 files
docs/deployment/backup-and-recovery.md#L539-L551(this comment)docs/deployment/mvp-deployment-runbook.md#L174-L177
🤖 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 `@docs/deployment/backup-and-recovery.md` around lines 539 - 551, Remove
administrator passwords from command-line arguments in the bootstrap and reset
procedures: docs/deployment/backup-and-recovery.md lines 539-551 must use an
interactive prompt or protected stdin/file input for both commands, and
docs/deployment/mvp-deployment-runbook.md lines 174-177 must update its
bootstrap fallback to the same safe input method.
| docker run --rm \ | ||
| -v "${SRC_PG}:/from:ro" \ | ||
| -v "${DEST_PG}:/to" \ | ||
| alpine:latest \ | ||
| sh -c 'cp -a /from/. /to/' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Backup and recovery scripts pull unpinned images from Docker Hub. All three helpers call docker run with alpine:latest or postgres:18.4-bookworm. Docker pulls the image when it is not cached locally, so the procedures require internet access at the moment of recovery and produce non-reproducible results. Introduce a single overridable variable (for example EXAM_BACKUP_HELPER_IMAGE) with a digest-pinned default, verify the image exists locally with docker image inspect before use, and document the pre-load step (docker save / docker load) in docs/deployment/backup-and-recovery.md.
scripts/backup/cold-filesystem-restore.sh#L127-L131: replacealpine:latestwith the pinned helper image variable and fail early with a clear message when the image is absent locally.scripts/backup/cold-filesystem-backup.sh#L144-L148: use the same pinned helper image variable for the copy step and for the validation containers at Lines 106 and 113.scripts/backup/pg-basebackup.sh#L137-L146: pin thepostgresimage used forpg_verifybackupby digest and use the shared helper image variable for theducontainer.
As per coding guidelines: "The product must remain LAN/on-premise and offline-capable; do not add cloud services, CDNs, external APIs, telemetry, or other runtime internet dependencies."
📍 Affects 3 files
scripts/backup/cold-filesystem-restore.sh#L127-L131(this comment)scripts/backup/cold-filesystem-backup.sh#L144-L148scripts/backup/pg-basebackup.sh#L137-L146
🤖 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/backup/cold-filesystem-restore.sh` around lines 127 - 131, Introduce
one overridable EXAM_BACKUP_HELPER_IMAGE variable with a digest-pinned default
and use it in cold-filesystem-restore.sh lines 127-131,
cold-filesystem-backup.sh lines 144-148 and its validation containers at lines
106 and 113, and the du container in pg-basebackup.sh lines 137-146; keep the
postgres image for pg_verifybackup digest-pinned separately. Before any helper
image use, run docker image inspect and fail clearly if it is unavailable
locally, then document docker save/docker load preloading in
docs/deployment/backup-and-recovery.md so recovery remains offline-capable.
Source: Coding guidelines
| docker exec -i -e PGPASSWORD="${PGPASSWORD:-}" "${DB_CONTAINER}" \ | ||
| sh -c 'psql -U "$POSTGRES_USER" -d postgres -v ON_ERROR_STOP=1' <<SQL | ||
| DROP DATABASE IF EXISTS "${TARGET_DB}"; | ||
| CREATE DATABASE "${TARGET_DB}" TEMPLATE template0; | ||
| SQL | ||
|
|
||
| echo "Restoring dump into clean '${TARGET_DB}'..." | ||
| # pg_restore into the recreated database. --no-owner (matches the dump) so the | ||
| # restore is portable across the role that owns objects. --exit-on-error makes | ||
| # any restore error fail the script immediately (no partial silent restore). | ||
| docker exec \ | ||
| -e PGPASSWORD="${PGPASSWORD:-}" \ | ||
| -i "${DB_CONTAINER}" \ | ||
| sh -c 'pg_restore -U "$POSTGRES_USER" -d '"${TARGET_DB}"' --no-owner --exit-on-error' \ | ||
| < "${DUMP}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Validate TARGET_DB before it reaches SQL and the container shell.
TARGET_DB is spliced into an unquoted heredoc at Lines 114-115 and into the sh -c command string at Line 125. Only three system names are rejected (Lines 63-68). A value containing ;, ", backticks, or $(...) becomes executable SQL or shell code inside the db container, in a script that already runs destructive statements as the PostgreSQL superuser. Restrict the argument to a plain identifier.
DROP DATABASE also fails while any session is connected. app and email-worker use restart: unless-stopped, so one reconnecting client makes the restore fail midway. Use WITH (FORCE) (PostgreSQL 13+).
🔒 Proposed fix
case "${TARGET_DB}" in
postgres|template0|template1)
echo "FAIL: refusing to DROP the system database '${TARGET_DB}'." >&2
exit 2
;;
esac
+if ! printf '%s' "${TARGET_DB}" | grep -Eq '^[A-Za-z_][A-Za-z0-9_]{0,62}$'; then
+ echo "FAIL: TARGET_DB '${TARGET_DB}' is not a plain SQL identifier." >&2
+ exit 2
+fi-DROP DATABASE IF EXISTS "${TARGET_DB}";
+DROP DATABASE IF EXISTS "${TARGET_DB}" WITH (FORCE);
CREATE DATABASE "${TARGET_DB}" TEMPLATE template0;🤖 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/backup/postgres-logical-restore.sh` around lines 112 - 126, Validate
TARGET_DB before any SQL or shell interpolation, allowing only a plain
PostgreSQL identifier with an appropriate safe character pattern and rejecting
all other values; retain the existing reserved-name checks. Update the DROP
DATABASE statement in the restore flow to use WITH (FORCE), preserving the
recreate-and-restore sequence and preventing connected clients from interrupting
it.
| capture() { | ||
| # orgs : admin_bootstrap_audit : probe label | ||
| docker exec "${PROJECT}-db-1" psql -U exam -d exam -tAc \ | ||
| "SELECT count(*)||':'||(SELECT count(*) FROM audit_logs WHERE action='admin.bootstrap')||':'||(SELECT label FROM p7c1_probe.marker WHERE id=1) FROM organizations;" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The drill can report PASS on two empty results.
capture() runs psql without ON_ERROR_STOP=1 and without a non-empty assertion. If the probe schema or a table is missing after restore, both INV_BEFORE and INV_AFTER can be empty, the comparison at Line 135 succeeds, and the drill reports PASS without evidence. Add ON_ERROR_STOP=1 and reject an empty capture.
💚 Proposed fix
capture() {
# orgs : admin_bootstrap_audit : probe label
- docker exec "${PROJECT}-db-1" psql -U exam -d exam -tAc \
+ local out
+ out="$(docker exec "${PROJECT}-db-1" psql -v ON_ERROR_STOP=1 -U exam -d exam -tAc \
"SELECT count(*)||':'||(SELECT count(*) FROM audit_logs WHERE action='admin.bootstrap')||':'||(SELECT label FROM p7c1_probe.marker WHERE id=1) FROM organizations;"
+ )"
+ if [ -z "${out}" ]; then
+ echo "FAIL: invariant capture returned no rows." >&2
+ exit 1
+ fi
+ printf '%s\n' "${out}"
}Also applies to: 135-138
🤖 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/deployment/p7-c1-cold-backup-restore-drill.sh` around lines 98 - 102,
Update the capture function to invoke psql with ON_ERROR_STOP=1 and validate
that its result is non-empty, failing the drill when the query errors or returns
no evidence. Ensure the before/after comparison and PASS path around INV_BEFORE
and INV_AFTER cannot treat two empty captures as a successful restore.
| MISS_LOGS="$(docker compose -p "${PROJECT_MISS}" -f "${COMPOSE_FILE}" logs --tail=200 db 2>&1 || true)" | ||
| if printf '%s' "${MISS_LOGS}" | grep -Eqi 'could not receive data|not found|restore_command failed|could not open file|record|WAL segment|invalid record|unexpected|could not locate| Consistency state not yet reached'; then | ||
| pass "F1 missing WAL segment — recovery surfaced the missing segment loudly" | ||
| elif ! docker exec "${PROJECT_MISS}-db-1" pg_isready -U exam -d exam >/dev/null 2>&1; then | ||
| pass "F1 missing WAL segment — recovery cluster refused to serve (loud failure)" | ||
| else | ||
| # The cluster did start. Check whether it reached a consistent state. If | ||
| # pg_isready says yes but the log still shows repeated restore_command | ||
| # failures, the recovery is at least NOT silent. Otherwise treat as fail. | ||
| if printf '%s' "${MISS_LOGS}" | grep -Eqi 'restore_command|cp:.*No such|archive'; then | ||
| pass "F1 missing WAL segment — restore_command failures visible in logs (loud)" | ||
| else | ||
| fail "F1 missing WAL segment — recovery silently started with incomplete history" | ||
| fi | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== script excerpt == "
sed -n '270,320p' scripts/deployment/p7-c3-pitr-failure-drill.sh || true
echo
echo "== related pg/docker compose references == "
rg -n "pg (18|16|15)|postgres|COMPOSE_FILE|PROJECT_MISS|project_miss|archive recovery|restore_command|F1" docs scripts package.json pnpm-lock.yaml docker-compose.yml docker-compose.*.ya* 2>/dev/null || true
echo
echo "== postgres image versions from project config/files == "
rg -n "postgres:|postgres:" package.json pnpm-lock.yaml *docker*.* 2>/dev/null || trueRepository: jnhu76/exam
Length of output: 50367
🌐 Web query:
PostgreSQL 18 archive recovery "starting archive recovery" "could not restore file" "restore_command" recovery log
💡 Result:
You’re seeing archive recovery start, but PostgreSQL cannot restore the next required file from the WAL archive (“could not restore file … from archive …”), and/or the log shows issues while replaying WAL (e.g., failures related to restoring timeline history such as “00000004.history” / “RECOVERYHISTORY”). In PostgreSQL, archive recovery depends entirely on restore_command to fetch WAL segments and related timeline/history files; misconfiguration or wrong failure semantics in restore_command will stop startup. What the error means 1) In archive recovery (PITR / replica catch-up from archive), PostgreSQL repeatedly requests specific WAL segment and/or timeline history files it needs, and it expects restore_command to retrieve them into the server’s pg_wal/ directory path (the %p destination). The docs state restore_command is “absolutely must specify” and it tells PostgreSQL how to retrieve archived WAL segments, using %f (archived file name) and %p (local destination path) [1]. 2) PostgreSQL determines success/failure by the shell command’s exit status. The docs specify: the command should return a zero exit status only if it succeeded; it “will be asked for file names that are not present in the archive” and must return nonzero in that case; but if the command is “terminated by a signal” or “an error by the shell (such as command not found), then recovery will abort and the server will not start up” [1]. Common root causes for “archive recovery could not restore file …” A) restore_command points to a tool that isn’t installed / not on PATH - A typical log is “could not restore file …: command not found”, which happens if your restore_command references pgbackrest/another tool but that tool isn’t available where PostgreSQL runs (or it needs an absolute path). A real example shows exactly this for “00000004.history” and pgbackrest: the fix was to install it on the standby or use the correct absolute path, or remove stale recovery parameters [2]. B) restore_command returns 0 even though it failed to place the file on disk - Sometimes you can see behavior like “restore_command returned a zero exit status, but stat failed” and then PostgreSQL may later keep trying. One example explains that PostgreSQL tries to restore the timeline history file (e.g., .history) and names it RECOVERYHISTORY locally; if restore_command returns 0 but it doesn’t actually create the destination file in pg_wal, recovery will fail in an unusual way [3]. - So your restore_command must both (1) succeed with exit code 0 only when it really copied/created the requested file at %p, and (2) return nonzero when the requested archived file is not available. C) Wrong/insufficient archive contents (missing WAL timeline/history files) - Archive recovery requires a continuous sequence of archived WAL files that extends back far enough for the requested recovery start. The docs emphasize you need the archive to contain the needed WAL history back to at least the recovery start time, otherwise recovery cannot proceed [1]. - Additionally, for timeline history, PostgreSQL requests the next “.history” file. If it’s removed by retention (or never archived), restore_command cannot restore it. D) Archive cleanup / retention removed needed files - For standby/log shipping scenarios, the warm-standby docs caution about archive_cleanup_command: you must retain files needed to recover (e.g., at least as far back as required by the latest base backup / to support restarting) [4]. If your cleanup policy is too aggressive, you’ll observe missing files during archive recovery. E) restore_command uses a script/remote copy that fails with an exit code that PostgreSQL treats as fatal - Because PostgreSQL treats “error by the shell (such as command not found)” as abort-worthy, and because exit-status behavior matters, you must ensure your restore_command’s failure mode matches PostgreSQL expectations. Discussion notes that nonzero exit codes in general are used for “not present in archive,” but shell termination/error can abort startup; ensure the command properly handles connection failures/availability and returns a nonzero value rather than triggering shell-level errors [5][6]. What to do (practical checks) 1) Verify restore_command syntax and placeholders - Ensure restore_command uses %f and %p correctly (and %p is where PostgreSQL expects the file to appear, relative to the data directory) [1]. 2) Ensure restore_command can run in the PostgreSQL server runtime environment - Confirm the referenced command exists (PATH/absolute paths) and required credentials/network access are available for the PostgreSQL process user. 3) Ensure restore_command return codes are correct - Return 0 only if the requested file was successfully placed at the requested destination (%p) [1]. - Return nonzero if the file is not present in the archive or otherwise cannot be restored (as PostgreSQL may ask for files that aren’t present) [1]. 4) Confirm the requested files exist in the archive storage - Look at the log line(s) mentioning the specific file name PostgreSQL cannot restore (e.g., a WAL segment like 0000000X... or a timeline history like 00000004.history). Then check your archive directory/object store for that exact file. 5) Check archive retention/cleanup - If you use archive_cleanup_command or archive retention policies, ensure you didn’t delete required WAL/history files for the recovery point you’re trying to reach [4]. If you paste the exact PostgreSQL log lines (especially the “could not restore file … from archive: …” portion and the restore_command you’re using), I can map it to the most likely bucket above and suggest a corrected restore_command pattern. Key references - PostgreSQL 18 docs: continuous archiving/PITR and restore_command semantics including exit codes and required %f/%p behavior [1]. - PostgreSQL 18 warm-standby docs: archive cleanup/retention guidance [4]. - Example failure modes involving “command not found” for timeline history restores [2]. - Example failure mode where restore_command returns 0 but file isn’t actually created at pg_wal destination [3].
Citations:
- 1: https://www.postgresql.org/docs/current/continuous-archiving.html
- 2: https://stackoverflow.com/questions/77462971/postgresql-history-error-could-not-restore-file-00000004-history-from-archive
- 3: https://www.postgresql.org/message-id/4e68824dfa5242fd0db8a856d47b2411046f6df2.camel%40cybertec.at
- 4: https://www.postgresql.org/docs/18/warm-standby.html
- 5: https://www.postgresql.org/message-id/CAHZmTm2xstmCjhMSH65iJeuV%3DjZjLScVrpHGgqYzyoqsm7UdqA%40mail.gmail.com
- 6: https://www.postgresql.org/message-id/57177C46.6040604%40elster.de
🌐 Web query:
PostgreSQL source could not restore file \"%s\" from archive
💡 Result:
PostgreSQL error “FATAL: could not restore file "%s" from archive …” almost always means archive recovery (WAL/history restore) called restore_command to fetch a needed WAL segment (or timeline history file) and the restore_command failed or produced an unusable result. Most common causes and what to check 1) restore_command uses a tool that isn’t installed (or isn’t found in PATH) If PostgreSQL logs show something like “command not found” for the invoked program, recovery aborts when PostgreSQL cannot execute the restore command. For example, one case shows the log line “pgbackrest: not found” alongside “could not restore file "00000004.history" from archive: command not found” [1]. Fix: ensure the command exists on the standby (or recovery host), and use the absolute path if the tool is installed in a non-standard location [1]. 2) restore_command exit status / behavior doesn’t match what PostgreSQL expects Even if the restore_command returns success (exit code 0), PostgreSQL may still fail if the file it expects was not actually created in the target location. A 2025 report shows PostgreSQL logging: “restore_command returned a zero exit status, but stat failed” while it was trying to restore “pg_wal/RECOVERYHISTORY” [2]. This indicates the script returned 0 but did not leave the required file where PostgreSQL expects it. Fix: make sure your restore_command creates (or copies) the file to “%p” successfully whenever it returns 0; if the file doesn’t exist, return a non-zero status (or otherwise ensure correctness for the specific placeholder) so PostgreSQL retries/fails in a controlled way [2]. 3) archive recovery requires timeline history files (.history); they must be retrievable The error often references a “.history” file (timeline history) such as “00000001.history” [3] or “00000004.history” [1]. If your restore_command only knows how to fetch WAL segments but not timeline history files, startup will fail. Fix: ensure restore_command handles both WAL segments (%f like 000000…/000000…) and the corresponding “.history” requests, and that the archive storage actually contains those files (or are fetchable from your archive manager) [3][1]. 4) external archive transport failures (ssh, barman, network partitions, etc.) If restore_command depends on network access and the network is down, the child process can exit and recovery will abort. A report describes a replica crashing with “could not restore file … from archive: child process exited with exit code 255” during network maintenance; after the network returned, recovery/restore_command worked again [4]. Fix: ensure restore_command is robust to transient failures, and (if using ssh) consider wrapping the command so that failure modes don’t behave like fatal signals in a way that kills recovery; also verify connectivity and authentication [4]. 5) Not a pg_dump/pg_restore error (important distinction) The message above is for archive recovery / restore_command during WAL replay, not for restoring a logical dump (pg_restore) [5]. If you are using pg_restore and see different errors, they may be unrelated. If you share your exact log lines (especially the full “could not restore file …” line plus the preceding restore_command-related output) and your restore_command, I can pinpoint which of the above categories applies and suggest a specific corrected restore_command.
Citations:
- 1: https://stackoverflow.com/questions/77462971/postgresql-history-error-could-not-restore-file-00000004-history-from-archive
- 2: https://www.postgresql.org/message-id/4e68824dfa5242fd0db8a856d47b2411046f6df2.camel%40cybertec.at
- 3: https://www.postgresql.org/message-id/158182AF0720CA4FBB0E7DC47600D5A81F397C0B%40winxbede14.exchange.xchg
- 4: https://groups.google.com/g/comp.databases.postgresql/c/r6uqQKj2jWk
- 5: https://www.postgresql.org/docs/19/app-pgrestore.html
🌐 Web query:
PostgreSQL wal_retrieve retry archive recovery waiting for archive segment log line
💡 Result:
The log message you are referring to occurs when a PostgreSQL server is in recovery mode (e.g., a standby server) and is unable to immediately locate the required Write-Ahead Log (WAL) segment from any of its configured sources [1]. In this state, the PostgreSQL recovery process searches for the next needed WAL segment from available sources, typically in this order: 1. Streaming replication (if configured) [1] 2. The local pg_wal directory 3. The WAL archive (via restore_command or recovery_target_timeline) [1][2] When the requested WAL data is not available in any of these locations, the recovery process pauses to wait for the data to become available [1]. The duration of this pause is controlled by the wal_retrieve_retry_interval parameter [1]. Once this interval expires, the system retries the retrieval process [1]. Key points regarding this behavior: - Configuration: The wal_retrieve_retry_interval parameter (default: 5000 milliseconds, or 5 seconds) determines how long the server waits between these retry attempts [1][3]. - Optimization: In environments where you want faster detection of new WAL files during archive recovery, you can reduce this value [1]. Conversely, in cloud environments where infrastructure access is metered or charged, you might increase this value to reduce the frequency of archive requests during periods of low activity [1]. - Context: This behavior is normal during archive recovery or standby operations when the primary server has not yet generated or shipped the next WAL segment [1]. It simply indicates that the standby is currently "caught up" and is waiting for more data from the primary [1]. If you see these messages frequently and the system is not making progress, it often indicates an issue with the connectivity between the standby and the primary, or a failure in the archiving process (e.g., the primary is failing to archive segments to the location the standby is checking) [1][4].
Citations:
- 1: https://postgresqlco.nf/doc/en/param/wal_retrieve_retry_interval/
- 2: https://postgrespro.com/docs/postgresql/11/archive-recovery-settings.html
- 3: https://github.com/postgres/postgres/blob/af9f8b7ca343eefa33b693d7919d8f945aeee3e7/src/backend/access/transam/xlog.c
- 4: Causes of "WAL segment _ was not archived before the _ms timeout" ? pgbackrest/pgbackrest#1059
🌐 Web query:
PostgreSQL 18 could not restore file \"000000010000000000000016\" from archive "record"
💡 Result:
The error "could not restore file from archive" in PostgreSQL, including references to specific Write-Ahead Log (WAL) files like "000000010000000000000016," generally indicates that the server's recovery process is attempting to fetch a file from the configured archive location via the restore_command and is failing to do so [1][2]. This is a normal part of the recovery process when it reaches the end of available WAL files in the archive. However, if the server considers the failure fatal (aborting startup), it is usually due to one of the following reasons: 1. External Command Failure: If the restore_command itself fails for a reason other than the file being missing (e.g., the command is not found, or there is a permission issue), PostgreSQL will abort the recovery [3][1][2]. For example, a "command not found" error often occurs if a backup tool (like pgbackrest) is configured in the restore_command but is not installed or available in the system's path [4][5]. 2. Improper Exit Status: PostgreSQL expects the restore_command to return a non-zero exit status when a requested file is genuinely missing from the archive (which is normal and not an error) [1][2]. If your script returns a non-zero status for reasons other than a missing file (or if it crashes), the database may interpret this as a fatal failure [3]. 3. Misconfigured Recovery Targets: If you have recovery parameters set (e.g., recovery_target_time, recovery_target_lsn), the server will continue to look for WAL files until it reaches that target [6][1]. If those files are not in the archive, or if there is a mismatch in timelines, the process will fail [7][8]. 4. Stale Configuration: If you are performing a standard startup but left restore_command or other recovery settings in your postgresql.conf or postgresql.auto.conf from a previous recovery attempt, the server will continue to try to enter recovery mode [4][5]. To troubleshoot this: * Check your logs for the specific exit code or error message accompanying the failure [3]. * Verify that your restore_command script is executable, installed correctly, and returns the correct exit codes [4][1]. * Ensure you have cleared out old recovery configuration settings if you are no longer attempting a point-in-time recovery [4][5]. * If you are intentionally performing recovery, ensure the required WAL segments are actually present in the specified archive directory [9][8].
Citations:
- 1: https://www.postgresql.org/docs/18/continuous-archiving.html
- 2: https://www.postgresql.org/docs/current/continuous-archiving.html
- 3: https://groups.google.com/g/comp.databases.postgresql/c/r6uqQKj2jWk
- 4: https://stackoverflow.com/questions/77462971/postgresql-history-error-could-not-restore-file-00000004-history-from-archive
- 5: https://www.postgresql.org/message-id/CAJZxWENFPuDko%2BQbCj5Kh6CHUy%3DpauUsDvOA9Zj6-P9-V4mH6A%40mail.gmail.com
- 6: https://pgdash.io/blog/postgres-incremental-backup-recovery.html
- 7: https://dba.stackexchange.com/questions/334466/postgresql-replication-failed-with-no-such-file-or-directory-for-wal-file
- 8: https://dba.stackexchange.com/questions/336191/point-in-time-postgres-restore
- 9: https://www.postgresql.org/docs/18/warm-standby.html
Tighten F1 so the failure assertion is reachable.
The current F1 patterns accept generic terms that can appear in recovery logs, so they do not prove the missing WAL segment was surfaced as a failure. Match or record an actual recovery failure message, such as the restore_command retrieval failure, and assert the node remained unavailable.
🤖 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/deployment/p7-c3-pitr-failure-drill.sh` around lines 297 - 311,
Tighten the F1 validation around MISS_LOGS and pg_isready so it requires
evidence of an actual restore_command/WAL retrieval failure rather than generic
recovery terms. Record or match the concrete missing-segment failure message,
and assert that the recovery node remains unavailable; do not pass solely
because broad log patterns appear or because the node starts with non-specific
restore errors.
P7-C Rebuild — Adversarial Audit: PG Backup Simplicity & Config SyncRepository: Audit date: 2026-08-10 Scope note: this is not a re-run of the earlier PR #273 audit 1. Executive verdictRequest changes — the backup/restore mechanics are sound and follow
The backup scripts themselves (C1 cold copy, C2 2. PostgreSQL backup simplicity & PG-convention review2.1 What is genuinely good (and simple)
The program is appropriately simple: it uses PG-native tools, adds no 2.2 PG-convention issues (ordered by severity)(Required) — The comment is wrong. The official postgres image's default
Note: (Nit) — (Nit) — cosmetic (FYI) — cold backup does not explicitly check 3. Critical config-sync defect: PITR initdb seed is silently ignored3.1 The defect
- ./docker/pitr/wal-archive.conf:/docker-entrypoint-initdb.d/99-pitr-wal-archive.conf:ro
case "$f" in
*.sh) ... ;;
*.sql) ... docker_process_sql -f "$f" ... ;;
*.sql.gz) ... gunzip -c "$f" | docker_process_sql ... ;;
*.sql.xz) ... xzcat "$f" | docker_process_sql ... ;;
*.sql.zst) ... zstd -dc "$f" | docker_process_sql ... ;;
*) printf '%s: ignoring %s\n' "$0" "$f" ;;
esacA 3.2 Why the drills do not catch itBoth PITR drills ( psql_src -c "ALTER SYSTEM SET archive_mode = 'on';"
psql_src -c "ALTER SYSTEM SET archive_command = 'test ! -f /wal-archive/%f && cp %p /wal-archive/%f';"So the documented operator path ( 3.3 Impact (operator-visible)
3.4 Fix (smallest acceptable)Rename the seed file so the extension is processed: git mv docker/pitr/wal-archive.conf docker/pitr/wal-archive.sql
# update the mount in docker-compose.pitr.yml:
# ./docker/pitr/wal-archive.sql:/docker-entrypoint-initdb.d/99-pitr-wal-archive.sql:ro
4. Critical config-sync defect: Launchpad token never reaches the container4.1 The defect (P2-1, carried over and still present)
Compose reads 4.2 Runtime consequence
The documented first-install UX cannot succeed. The operator must either 4.3 Fix (one line + contract awareness)In LAUNCHPAD_SETUP_TOKEN: ${LAUNCHPAD_SETUP_TOKEN:-}The empty default keeps launchpad disabled for a bare Check the topology contract ( 5. Other config-sync & doc-consistency findings(Required) — (Nit) — (Nit) — PITR drills hardcode (Nit) — (FYI) — 6. Drill-vs-documented-path gap (process finding)The five drills are well-constructed and path-isolated, but none of them
This is why §3.1 went undetected. The closeout's "5/5 drills PASS" is 7. Summary table
8. Required corrective actions
Nits (§5) are optional cleanup; address at author discretion. 9. Verification notes
|
One production Compose entry point: delete docker-compose.pitr.yml and docker/pitr/wal-archive.conf. PITR is a database capability enabled by the canonical scripts/backup/postgres-enable-pitr.sh (ALTER SYSTEM persisted in postgresql.auto.conf), not an alternate Docker topology. WAL archive mount is always present but inert until enabled. - idempotent archive_command (absent->copy, identical->cmp, collision->fail) - pg_basebackup derives deployment PGUSER/PGPASSWORD from the db container env; auth comment matches reality (loopback TCP + scram-sha-256, password via PGPASSWORD, never argv) - cold backup refuses an obviously running source (postmaster.pid) - logical restore reworded to clean logical reconstruction (no byte-identity overclaim) - C3 drills now exercise the canonical enable script (product path == test path); new archive-idempotency drill - topology guard: forbids production Compose variants, requires LAUNCHPAD_SETUP_TOKEN wiring in the app environment
- bootstrapAdminOnFreshDb takes a transaction-scoped pg_advisory_xact_lock at entry, shared by the HTTP launchpad adapter and the bootstrap-admin CLI, so a concurrent first-install has exactly one winner on a fresh DB - new p7-c1-launchpad-compose-drill.sh proves the production Compose path end to end: fresh status uninitialized; wrong token 403; correct token creates the first Admin; correct token again 409 (installation never reopens and is not a token oracle); Admin login; DB invariants (1 Admin, 0 Candidates, default org, bootstrap audit); register disabled (API 403 stub + no client route); unset token disables launchpad entirely
- backup-and-recovery.md: One-Compose model read-first section; PITR enabled via scripts/backup/postgres-enable-pitr.sh with restrictive permissions and pg_stat_archiver proof; idempotent archive_command contract with three-case table; corrected retention rule (base history must precede T + all WAL through the window, no automatic pruning); base-backup-before-WAL ordering; WAL-G/pgBackRest future boundary; cold/logical terminology without byte-identity overclaim - mvp-deployment-runbook.md: replace docker-compose.pitr.yml references with the canonical enable script - README: one-compose invariant, advisory-lock first-install note - closeout: corrective-pass section with correction table, drill and static-gate evidence
|
AI review done up to commit: 567e0ad AI Review Summary:The pull request introduces a new Launchpad feature for initial system setup and significantly overhauls the deployment, backup, and recovery mechanisms for PostgreSQL and Redis. This includes implementing portable persistence using host bind mounts, cold filesystem backup/restore, logical online backup with clean restore, and physical backup with WAL archiving and point-in-time recovery (PITR). Many of these changes directly address critical findings from a comprehensive adversarial audit. The quality of the changes is high. The new features and infrastructure are well-tested with a combination of unit tests, integration tests, and deterministic shell drills, covering a wide range of scenarios including happy paths, edge cases, and failure modes. Security considerations, such as constant-time token comparisons, prevention of token oracles, and secure handling of database credentials, have been diligently addressed. The documentation has been thoroughly updated with detailed guides, closeout reports, and adversarial audit records, ensuring transparency and providing clear, actionable instructions for operators. A key improvement for deployment simplicity and reliability is the adoption of a "One-Compose model" and explicit forwarding of environment variables in One minor observation:
Overall, the changes represent a significant improvement in the system's robustness, deployability, and recoverability, with thorough validation and documentation. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/deployment/backup-and-recovery.md (1)
54-61: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSeparate authoritative state from deployment identity.
data/postgresis the authoritative durable Exam state. It is not the only material required to restore the same running deployment. Preserve.envsecrets, Compose configuration, and the exact application/database image identity. Replace “the only bytes you must preserve” with “the only durable Exam state you must preserve.”🤖 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 `@docs/deployment/backup-and-recovery.md` around lines 54 - 61, Update the backup-and-recovery documentation to distinguish authoritative durable Exam state from deployment identity: revise the claim that the PostgreSQL data directory is the only bytes required, stating instead that it is the only durable Exam state to preserve. Also document preserving .env secrets, Compose configuration, and exact application/database image identity for restoring the same deployment.
🧹 Nitpick comments (7)
scripts/deployment/p7-c3-archive-idempotency-drill.sh (3)
96-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not discard the output of the canonical enable-PITR script.
Line 96 redirects both stdout and stderr to
/dev/null. Ifpostgres-enable-pitr.shfails,set -eaborts the drill with no diagnostic text. The operator then sees an exit code and nothing else.p7-c3-pitr-drill.shpipes the same call throughsed 's/^/ /'; match that behavior.♻️ Proposed change to keep the diagnostics
-bash "${ENABLE_PITR_SH}" "${PROJECT}" "${COMPOSE_FILE}" >/dev/null 2>&1 +bash "${ENABLE_PITR_SH}" "${PROJECT}" "${COMPOSE_FILE}" 2>&1 | sed 's/^/ /'🤖 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/deployment/p7-c3-archive-idempotency-drill.sh` around lines 96 - 97, Update the canonical enable-PITR invocation in the archive idempotency drill to preserve and indent its stdout and stderr, matching the p7-c3-pitr-drill.sh behavior with sed. Remove the /dev/null redirection while keeping the existing wait_db sequence unchanged.
62-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit the declaration and the assignment.
export VAR="$(openssl ...)"returns the exit status ofexport, not ofopenssl. Ifopensslfails, the drill continues with a truncated secret instead of stopping. Shellcheck reports SC2155 on both lines.♻️ Proposed change
-export POSTGRES_PASSWORD="p7c3idem-pg-$(openssl rand -hex 6)" -export JWT_SECRET="p7c3idem-jwt-$(openssl rand -hex 16)" +POSTGRES_PASSWORD="p7c3idem-pg-$(openssl rand -hex 6)" +JWT_SECRET="p7c3idem-jwt-$(openssl rand -hex 16)" +export POSTGRES_PASSWORD JWT_SECRET🤖 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/deployment/p7-c3-archive-idempotency-drill.sh` around lines 62 - 63, Split the POSTGRES_PASSWORD and JWT_SECRET declarations from their command substitutions in the deployment drill: assign each generated value to the variable first, then export it separately. Preserve the existing OpenSSL commands and variable values while ensuring command-substitution failures propagate under the script’s error handling.Source: Linters/SAST tools
149-163: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider asserting that the target bytes stay unchanged in CASE 3.
The drill checks only the exit status. The stated risk is a silent overwrite, so the strongest assertion is that
/wal-archive/IDEM-CASE1still matches/tmp/idem-sourceafter the collision attempt. Add acmp -s /tmp/idem-source /wal-archive/IDEM-CASE1check after the case runs.🤖 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/deployment/p7-c3-archive-idempotency-drill.sh` around lines 149 - 163, The CASE 3 validation currently checks only that the archive command fails, not that it preserves existing data. Update the CASE3 flow in scripts/deployment/p7-c3-archive-idempotency-drill.sh to run cmp -s between /tmp/idem-source and /wal-archive/IDEM-CASE1 after the collision attempt, and pass only when the command fails and the target bytes remain unchanged.scripts/backup/postgres-enable-pitr.sh (2)
180-209: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider failing when the archiver reports failures.
The loop accepts the presence of
/wal-archive/${SWITCHED_FILE}as proof. That file can exist from an earlier successful archive attempt while the currentarchive_commandfails repeatedly. Readingfailed_countandlast_failed_errorfrompg_stat_archivermakes the enablement report trustworthy.Line 205 also recomputes
AFTER_COUNTafter the segment-existence path succeeds. The report can then print0 → 0, which contradicts the "archived_count" wording.🤖 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/backup/postgres-enable-pitr.sh` around lines 180 - 209, Update the archiver verification loop to read and track pg_stat_archiver failed_count and last_failed_error, and fail instead of accepting stale segment evidence when the current archive attempt reports failures. Also preserve the count captured during verification when the segment-existence path succeeds, rather than recomputing AFTER_COUNT afterward, so the final archived_count report reflects the evidence used.
62-75: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winResolve the db container through Compose instead of building its name.
Line 70 builds
${PROJECT}-db-1by string concatenation. Acontainer_nameuses its exact value, and a scaleddbservice uses container names such asdb_1ordb_2, so this script may fail with "db container not found" even after the stack is running. Build names may also fail when the Compose-project name is invalid.Use
docker compose -p "${PROJECT}" -f "${COMPOSE_FILE}" ps -q dbto resolve the container id.🤖 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/backup/postgres-enable-pitr.sh` around lines 62 - 75, Replace the hard-coded DB_CONTAINER name construction with Compose-based resolution using `docker compose -p "${PROJECT}" -f "${COMPOSE_FILE}" ps -q db`. Validate that the returned container ID is non-empty, then use it in the existing `docker inspect` check and preserve the current failure messages and exit behavior.docs/audits/P7-C1-ADVERSARIAL-PORTABLE-DEPLOYMENT-AUDIT.md (1)
385-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a language identifier to the final fenced block.
The static check reports MD040 for the block starting at Line 385. Mark it
textor the correct language so the audit passes the repository Markdown checks.🤖 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 `@docs/audits/P7-C1-ADVERSARIAL-PORTABLE-DEPLOYMENT-AUDIT.md` around lines 385 - 421, Update the final fenced code block in the audit document to include a `text` language identifier, preserving its contents unchanged so the MD040 Markdown check passes.Source: Linters/SAST tools
docs/audits/P7-C-REBUILD-ADVERSARIAL-PG-BACKUP-CONFIG-AUDIT.md (1)
8-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language identifiers to the fenced blocks.
The static check reports MD040 for these blocks. Mark the commit list as
textand the shell example asbash, or use the correct language for each block.Also applies to: 99-109
🤖 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 `@docs/audits/P7-C-REBUILD-ADVERSARIAL-PG-BACKUP-CONFIG-AUDIT.md` around lines 8 - 13, Add language identifiers to every fenced code block in the audit document, including the commit list near the shown diff and the shell example around the additionally referenced section; mark the commit list as text and the shell example as bash to resolve MD040.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 @.env.example:
- Around line 150-152: Update the Launchpad comment in .env.example to qualify
that it never reopens after supported Admin-removal operations, while noting
that destructive database edits or history-replacing restores may alter the
freshness evidence. Keep the existing operator CLI recovery guidance unchanged.
In `@docs/audits/P7-C-PORTABLE-BACKUP-RECOVERY-CLOSEOUT.md`:
- Around line 364-370: Add the missing release-evidence fields to the static
gates report: modified files, new tests, coverage, the `pnpm verify` result, and
known limitations. Alternatively, link this table to an authoritative report
containing all required fields, while preserving the existing gate results.
In `@docs/audits/P7-C-REBUILD-ADVERSARIAL-PG-BACKUP-CONFIG-AUDIT.md`:
- Line 85: Update the WAL archive_command verdict in the audit table to indicate
that `test ! -f /wal-archive/%f && cp %p /wal-archive/%f` prevents overwrites
but is not idempotent, aligning it with the corrective finding and replacement
documented near line 337.
- Around line 82-83: Revise the audit descriptions for
postgres-logical-backup.sh and postgres-logical-restore.sh to use precise
PostgreSQL terminology: describe pg_dump -Fc using its actual consistent
snapshot and custom archive semantics without mentioning -X stream, and replace
“exact match” with wording that limits the claim to dumped database objects
while acknowledging configuration objects may differ.
In `@docs/audits/P7-C1-ADVERSARIAL-PORTABLE-DEPLOYMENT-AUDIT.md`:
- Line 225: Reconcile the HYP-8 severity classification across the summary,
findings around HYP-8, and merge recommendation. Either use one consistent
severity everywhere or explicitly distinguish P3 as the clean-host coverage debt
and P2-2 as the inaccurate “permanently guarded” claim, including that
distinction in the relevant sections.
---
Outside diff comments:
In `@docs/deployment/backup-and-recovery.md`:
- Around line 54-61: Update the backup-and-recovery documentation to distinguish
authoritative durable Exam state from deployment identity: revise the claim that
the PostgreSQL data directory is the only bytes required, stating instead that
it is the only durable Exam state to preserve. Also document preserving .env
secrets, Compose configuration, and exact application/database image identity
for restoring the same deployment.
---
Nitpick comments:
In `@docs/audits/P7-C-REBUILD-ADVERSARIAL-PG-BACKUP-CONFIG-AUDIT.md`:
- Around line 8-13: Add language identifiers to every fenced code block in the
audit document, including the commit list near the shown diff and the shell
example around the additionally referenced section; mark the commit list as text
and the shell example as bash to resolve MD040.
In `@docs/audits/P7-C1-ADVERSARIAL-PORTABLE-DEPLOYMENT-AUDIT.md`:
- Around line 385-421: Update the final fenced code block in the audit document
to include a `text` language identifier, preserving its contents unchanged so
the MD040 Markdown check passes.
In `@scripts/backup/postgres-enable-pitr.sh`:
- Around line 180-209: Update the archiver verification loop to read and track
pg_stat_archiver failed_count and last_failed_error, and fail instead of
accepting stale segment evidence when the current archive attempt reports
failures. Also preserve the count captured during verification when the
segment-existence path succeeds, rather than recomputing AFTER_COUNT afterward,
so the final archived_count report reflects the evidence used.
- Around line 62-75: Replace the hard-coded DB_CONTAINER name construction with
Compose-based resolution using `docker compose -p "${PROJECT}" -f
"${COMPOSE_FILE}" ps -q db`. Validate that the returned container ID is
non-empty, then use it in the existing `docker inspect` check and preserve the
current failure messages and exit behavior.
In `@scripts/deployment/p7-c3-archive-idempotency-drill.sh`:
- Around line 96-97: Update the canonical enable-PITR invocation in the archive
idempotency drill to preserve and indent its stdout and stderr, matching the
p7-c3-pitr-drill.sh behavior with sed. Remove the /dev/null redirection while
keeping the existing wait_db sequence unchanged.
- Around line 62-63: Split the POSTGRES_PASSWORD and JWT_SECRET declarations
from their command substitutions in the deployment drill: assign each generated
value to the variable first, then export it separately. Preserve the existing
OpenSSL commands and variable values while ensuring command-substitution
failures propagate under the script’s error handling.
- Around line 149-163: The CASE 3 validation currently checks only that the
archive command fails, not that it preserves existing data. Update the CASE3
flow in scripts/deployment/p7-c3-archive-idempotency-drill.sh to run cmp -s
between /tmp/idem-source and /wal-archive/IDEM-CASE1 after the collision
attempt, and pass only when the command fails and the target bytes remain
unchanged.
🪄 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: 53eb5997-89a7-4e92-9709-96cc8386337a
📒 Files selected for processing (18)
.env.exampleREADME.mdapps/api/src/scripts/bootstrap-admin.tsdocker-compose.ymldocs/audits/P7-C-PORTABLE-BACKUP-RECOVERY-CLOSEOUT.mddocs/audits/P7-C-REBUILD-ADVERSARIAL-PG-BACKUP-CONFIG-AUDIT.mddocs/audits/P7-C1-ADVERSARIAL-PORTABLE-DEPLOYMENT-AUDIT.mddocs/deployment/backup-and-recovery.mddocs/deployment/mvp-deployment-runbook.mdscripts/backup/cold-filesystem-backup.shscripts/backup/pg-basebackup.shscripts/backup/postgres-enable-pitr.shscripts/backup/postgres-logical-restore.shscripts/deployment/p7-c1-launchpad-compose-drill.shscripts/deployment/p7-c3-archive-idempotency-drill.shscripts/deployment/p7-c3-pitr-drill.shscripts/deployment/p7-c3-pitr-failure-drill.shscripts/repository-contract/deployment-topology-contract.mjs
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/deployment/mvp-deployment-runbook.md
- README.md
- scripts/backup/postgres-logical-restore.sh
- scripts/backup/pg-basebackup.sh
- scripts/deployment/p7-c3-pitr-failure-drill.sh
| # Once the installation is initialized, /launchpad redirects to /login and | ||
| # never reopens — removing the last Admin does NOT reopen it (Admin recovery | ||
| # is operator CLI: bootstrap-admin / reset-admin-password). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Qualify the “never reopens” guarantee.
The supported removal flow keeps Launchpad completed, but destructive database edits or history-replacing restores can change the freshness evidence. State that this guarantee applies to supported Admin-removal operations.
🤖 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 @.env.example around lines 150 - 152, Update the Launchpad comment in
.env.example to qualify that it never reopens after supported Admin-removal
operations, while noting that destructive database edits or history-replacing
restores may alter the freshness evidence. Keep the existing operator CLI
recovery guidance unchanged.
| | `postgres-logical-backup.sh` (`pg_dump -Fc`) | Correct | `-Fc` (custom format) + `-X stream`-equivalent self-consistency + `--no-owner`; never puts the password on argv; verifies artifact with non-empty + `PGDMP` magic + `pg_restore --list`. This is the routine online backup PG documents. | | ||
| | `postgres-logical-restore.sh` | Correct & notably well-reasoned | `DROP DATABASE` + `CREATE DATABASE ... TEMPLATE template0` before `pg_restore` is the PG-documented way to get an **exact** match of the dump (avoids the `--clean --if-exists` merge trap where dump-absent objects survive). `--exit-on-error` fails fast. Refuses `postgres`/`template0`/`template1`. | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
PostgreSQL pg_dump -Fc custom format -X stream option pg_basebackup documentation
💡 Result:
There is no "stream" option for the custom format in pg_dump, nor is there a "custom format" in pg_basebackup. The two utilities serve different purposes and operate in distinct ways [1][2]. pg_dump The pg_dump utility performs a logical backup of a single database [2][3]. When using the custom format (-Fc), it creates a compressed, archive-style file intended for use with pg_restore [4][5]. This format is not a stream in the sense of continuous data flow for replication; rather, it is a structured archive that allows for selective restoration of objects and parallel restoration [4][6]. The custom format is compressed by default [4][7]. pg_basebackup The pg_basebackup utility performs a physical, binary backup of an entire PostgreSQL database cluster [8][9][2]. It does not have a "custom format" like pg_dump. Instead, it typically outputs files in the format of the database cluster itself (or in a tar archive format using the -Ft option) [9][10]. Regarding "streaming": While pg_dump does not have a "stream" option, pg_basebackup includes an option called -X (or --wal-method), which can be set to stream [9][10]. This option controls how Write-Ahead Log (WAL) files are handled during the backup. When -X stream is used, pg_basebackup streams the required WAL files along with the data files during the backup process, which helps ensure the resulting backup is consistent and ready for recovery without needing external WAL archives [9][1]. Summary of Differences pg_dump: Logical backup, database-level, portable across versions/architectures, slower for massive datasets [1][2][3]. pg_basebackup: Physical backup, cluster-level, requires identical major version/platform for restore, faster for large datasets, essential for Point-in-Time Recovery (PITR) and replication [2][11][3].
Citations:
- 1: https://stackoverflow.com/questions/62653659/what-is-recommended-way-to-do-a-postgresql-database-backup-pg-dump-or-pg-baseba
- 2: https://www.percona.com/blog/postgresql-backup-strategy-enterprise-grade-environment/
- 3: https://blog.worlber.sa/postgresql-backup-strategies-pg_dump-vs-pg_basebackup-vs-barman/
- 4: https://www.postgresql.org/docs/current/app-pgdump.html
- 5: https://www.postgresql.org/docs/12/backup-dump.html
- 6: https://github.com/postgres/postgres/blob/e18b0cb7/doc/src/sgml/ref/pg_dump.sgml
- 7: https://github.com/postgres/postgres/blob/207cb2ab/doc/src/sgml/ref/pg_dump.sgml
- 8: https://www.postgresql.org/docs/current/app-pgbasebackup.html
- 9: https://www.postgresql.org/docs/16/app-pgbasebackup.html
- 10: https://www.postgresql.org/docs/14/app-pgbasebackup.html
- 11: https://www.crunchydata.com/blog/introduction-to-postgres-backups
Use precise PostgreSQL backup terminology.
pg_dump -Fc does not have a -X stream mode; use the actual consistency/archive semantics instead, and avoid “exact match” for logical restores because database configuration objects may differ.
🤖 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 `@docs/audits/P7-C-REBUILD-ADVERSARIAL-PG-BACKUP-CONFIG-AUDIT.md` around lines
82 - 83, Revise the audit descriptions for postgres-logical-backup.sh and
postgres-logical-restore.sh to use precise PostgreSQL terminology: describe
pg_dump -Fc using its actual consistent snapshot and custom archive semantics
without mentioning -X stream, and replace “exact match” with wording that limits
the claim to dumped database objects while acknowledging configuration objects
may differ.
| | `postgres-logical-backup.sh` (`pg_dump -Fc`) | Correct | `-Fc` (custom format) + `-X stream`-equivalent self-consistency + `--no-owner`; never puts the password on argv; verifies artifact with non-empty + `PGDMP` magic + `pg_restore --list`. This is the routine online backup PG documents. | | ||
| | `postgres-logical-restore.sh` | Correct & notably well-reasoned | `DROP DATABASE` + `CREATE DATABASE ... TEMPLATE template0` before `pg_restore` is the PG-documented way to get an **exact** match of the dump (avoids the `--clean --if-exists` merge trap where dump-absent objects survive). `--exit-on-error` fails fast. Refuses `postgres`/`template0`/`template1`. | | ||
| | `pg-basebackup.sh` | Correct | `-X stream -c fast -Fp --manifest-checksums SHA256`; `pg_verifybackup` on the manifest before success; `--no-sync` deliberately not used. | | ||
| | WAL `archive_command` | Correct | `test ! -f /wal-archive/%f && cp %p /wal-archive/%f` is the PG-documented idiom that **refuses to silently overwrite** a colliding segment. | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Align the WAL verdict with the corrective finding.
Line 85 labels test ! -f /wal-archive/%f && cp %p /wal-archive/%f as correct. Line 337 states that this command fails on an identical retry and was replaced. Mark the old form as non-overwriting but not idempotent.
🤖 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 `@docs/audits/P7-C-REBUILD-ADVERSARIAL-PG-BACKUP-CONFIG-AUDIT.md` at line 85,
Update the WAL archive_command verdict in the audit table to indicate that `test
! -f /wal-archive/%f && cp %p /wal-archive/%f` prevents overwrites but is not
idempotent, aligning it with the corrective finding and replacement documented
near line 337.
| - Static guards: 7 of 8 mutations caught (§6); M4 (PG 18→19) not caught statically — P3. | ||
| - `verify:static` wiring: `verify-static-includes-guards.mjs` asserts `lint:repo-contract` ∈ `verify:static`; `package.json` chains both contract scripts into `lint:repo-contract`. Verified. | ||
| - Unit tests: `preflight.test.ts` (12 cases), `launchpad.test.ts` (8 + concurrency). CI API coverage SUCCESS on final head. | ||
| - Clean-host workflow triggers: `workflow_dispatch` + push to `feat/p7-c1-portable-single-node-deployment` only — **after merge the clean-host proof never runs automatically (HYP-8 TRUE)**; the permanent gate is the local clean-root drill (`pnpm drill:p7-c1-relocation`, manual) + static contracts. Classified: P3 coverage debt (deliberate, documented; but note the workflow required 3 commits to pass, so its "manual drill" value depends on it being run — recommend at least a scheduled/manual-on-master trigger). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the HYP-8 severity distinction explicit.
Line 225 classifies the post-merge clean-host gate as P3. Lines 275-276 list the same issue as P2-2. Use one severity in the summary, findings, and merge recommendation, or explain that P3 describes coverage debt while P2 describes the inaccurate “permanently guarded” claim.
Also applies to: 275-276
🤖 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 `@docs/audits/P7-C1-ADVERSARIAL-PORTABLE-DEPLOYMENT-AUDIT.md` at line 225,
Reconcile the HYP-8 severity classification across the summary, findings around
HYP-8, and merge recommendation. Either use one consistent severity everywhere
or explicitly distinguish P3 as the clean-host coverage debt and P2-2 as the
inaccurate “permanently guarded” claim, including that distinction in the
relevant sections.
…lity-named deployment tests Corrective pass per PR #274 final review: - Fix the invalid F1 'missing WAL' proof: the old test deleted pg_wal inside the base backup and treated normal end-of-archive file-not-found as failure. The corrected test removes ONE archived WAL segment required to reach an explicit recovery_target_lsn (untouched base backup) and asserts the target is UNREACHABLE: cluster stays in archive recovery (pg_controldata), restore_command failures for the missing segment are visible, and no promotion within a bounded window. - Remove scripts/deployment/ as a permanent surface: phase-named drills moved to tests/deployment/ with capability names (compose-smoke, launchpad-bootstrap, persistence-and-cold-restore, logical-backup-restore, pitr) + shared lib.sh helpers; p6-corr1 compose smoke renamed and kept as the compose capability suite. - No temporary Compose overrides in any test: recovery clusters start from the canonical docker-compose.yml with EXAM_DATA_ROOT / EXAM_WAL_ARCHIVE_HOST_PATH / POSTGRES_PASSWORD / COMPOSE_PROJECT_NAME. - Fixed sleeps replaced with bounded polling (pg_isready, /api/health, archived-segment presence, log evidence, pg_controldata state). - Operator scripts: derive POSTGRES_USER/POSTGRES_DB from the deployment (logical backup/restore), safe psql-variable TARGET_DB handling with a conservative name contract, removed the redundant backup prompt and numeric-UID claims, corrected pg_verifybackup 'manifest signature' wording (checksums, not a signature) and the false docker-inspect claim. - Launchpad backend: canonical bootstrap accepts an explicit audit source (local_script | launchpad); the HTTP race loser maps to 409 LAUNCHPAD_ALREADY_INITIALIZED via a typed AdminAlreadyExistsError (never 500); deterministic concurrency tests (DB-level + HTTP-level). - Closeout rewritten to describe ONE final architecture (no stale docker-compose.pitr.yml / wal-archive.conf claims, no self-referential final-head SHA); backup-and-recovery.md, runbook, roadmap, compose and env comments aligned to final reality. - package.json gains test:deployment[:compose|launchpad|persistence|logical|pitr]; deployment-topology-contract.mjs now patterns docker-compose.test*.yml and checks tests/deployment/ for developer-path leakage.
|
AI review done up to commit: a8767df AI Review Summary:The pull request significantly refactors the deployment test suite, moving individual drill scripts into a consolidated and well-structured set of suites under Key logical changes include:
Key security and robustness improvements include:
The documentation in Overall, the changes are of high quality, addressing potential race conditions, improving auditability, enhancing security, and significantly improving the testing infrastructure. No major logical errors, bugs, or security vulnerabilities were found in the revised code. |
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/routes/launchpad.ts (1)
74-76: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winPass context to
defaultOrganizationExists.
isInstallationInitializedinvokes repository data access withoutctx. This bypasses the route-to-repository context contract. Add an explicit context parameter todefaultOrganizationExistsand pass the applicable public or system context from this route.As per coding guidelines, all route data access must go through
repo.method(ctx, ...).🤖 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/api/src/routes/launchpad.ts` around lines 74 - 76, Update isInstallationInitialized to accept the applicable route context and pass it to createOrganizationRepo(fastify.db).defaultOrganizationExists(ctx). Ensure the route’s repository access follows the repo.method(ctx, ...) contract, using the established public or system context for this route.Source: Coding guidelines
♻️ Duplicate comments (1)
scripts/backup/postgres-logical-restore.sh (1)
143-150: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
DROP DATABASEstill fails if any session is connected.The identifier-injection half of the earlier review is resolved: Lines 79-84 bound
TARGET_DBto a plain identifier, and Lines 145-149 pass it as apsqlvariable with:"target_db"quoted-identifier interpolation.The second half is not addressed.
DROP DATABASE IF EXISTSat Line 148 aborts when any backend is connected to the target. Theappandemail-workerservices userestart: unless-stopped, so one reconnecting client fails the restore after the operator already typed the destructive confirmation. AddWITH (FORCE).🐛 Proposed fix
-DROP DATABASE IF EXISTS :"target_db"; +DROP DATABASE IF EXISTS :"target_db" WITH (FORCE); CREATE DATABASE :"target_db" TEMPLATE template0;PostgreSQL DROP DATABASE WITH FORCE supported version🤖 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/backup/postgres-logical-restore.sh` around lines 143 - 150, Update the DROP DATABASE statement in the restore heredoc to use PostgreSQL’s WITH (FORCE) option, preserving the existing :"target_db" identifier interpolation and CREATE DATABASE flow so connected clients are terminated before recreation.
🧹 Nitpick comments (5)
tests/deployment/lib.sh (1)
137-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving the PostgreSQL role and database from the running container.
psql_execandwait_for_postgreshard-code-U exam -d exam.scripts/backup/postgres-logical-backup.sh(Lines 91-99) andscripts/backup/postgres-logical-restore.sh(Lines 102-109) derivePOSTGRES_USERandPOSTGRES_DBfrom the container environment with anexamfallback. If an operator customizes those values, the backup scripts adapt and the drills do not. The drills set the environment themselves, so this is not a current defect; aligning the two paths removes a future divergence.🤖 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 `@tests/deployment/lib.sh` around lines 137 - 145, Update psql_exec and wait_for_postgres to derive POSTGRES_USER and POSTGRES_DB from the running database container, using the same exam fallback behavior as the backup scripts, and pass those values to psql instead of hard-coding exam.tests/deployment/persistence-and-cold-restore.sh (1)
155-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe PostgreSQL major-version path is hard-coded.
Lines 157-158 assert
/pg/18/docker/PG_VERSION.docker-compose.ymlpinspostgres:18.4-bookworm, so the path is correct today. A major-version bump silently breaks this assertion.tests/deployment/pitr.shrepeats the samepostgres/18/dockerliteral at Lines 205, 208, 223, 224, and 330, and repeats thepostgres:18.4-bookwormimage tag at Lines 331 and 366.Discover the directory instead of naming the version, or derive the version from the Compose image tag once and share it through
tests/deployment/lib.sh.♻️ Proposed refactor
-if ! docker run --rm -v "${ROOT_C}/postgres:/pg:ro" alpine:latest \ - sh -c 'test -f /pg/18/docker/PG_VERSION && cat /pg/18/docker/PG_VERSION'; then +if ! docker run --rm -v "${ROOT_C}/postgres:/pg:ro" alpine:latest \ + sh -c 'f=$(find /pg -maxdepth 3 -name PG_VERSION -print -quit); test -n "$f" && cat "$f"'; then🤖 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 `@tests/deployment/persistence-and-cold-restore.sh` around lines 155 - 162, Remove the hard-coded PostgreSQL major-version directory from the PGDATA visibility check and related PITR paths. Add shared version discovery or derivation in tests/deployment/lib.sh, then reuse that symbol in persistence-and-cold-restore.sh and pitr.sh for PG_VERSION paths and PostgreSQL image references so version bumps update the tests automatically.scripts/repository-contract/deployment-topology-contract.mjs (1)
110-134: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider failing when
tests/deployment/is missing.The
catchblock treats a missing directory as "no tests to validate". If the directory is deleted or renamed, the contract passes silently. The suite is now referenced bypackage.jsonscripts (test:deployment:*), so its absence is a real regression signal. DistinguishENOENTfrom other read errors, or assert the directory exists.🤖 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/repository-contract/deployment-topology-contract.mjs` around lines 110 - 134, Update the deployment test discovery around deploymentTestsDir so a missing tests/deployment directory records a contract error instead of being treated as valid. Distinguish ENOENT from other filesystem failures, preserving the existing validation for present directories and propagating unexpected read errors.tests/deployment/compose-smoke.sh (1)
191-204: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
run_composefor the remaining direct Compose invocations.Lines 191, 202, 359, and 366 still build
docker compose -p "${PROJECT}" -f "${COMPOSE_FILE}" ps -q email-workerby hand.lib.shprovidesrun_composefor exactly this. The behavior is identical today, but two invocation styles in one file make future Compose-flag changes easy to miss.♻️ Proposed refactor
-WORKER_STATE=$(docker inspect "$(docker compose -p "${PROJECT}" -f "${COMPOSE_FILE}" ps -q email-worker 2>/dev/null)" --format '{{.State.Status}}' 2>/dev/null || echo "missing") +WORKER_STATE=$(docker inspect "$(run_compose "${PROJECT}" ps -q email-worker 2>/dev/null)" --format '{{.State.Status}}' 2>/dev/null || echo "missing")Also applies to: 359-366
🤖 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 `@tests/deployment/compose-smoke.sh` around lines 191 - 204, Replace the remaining hand-built docker compose ps invocations in the worker-state checks and the Test 5b setup around WORKER_CONTAINER with the existing run_compose helper, passing the same project, compose file, and ps -q email-worker arguments. Apply the same change to the corresponding invocations near the later referenced checks, preserving all existing output and error-handling behavior.tests/deployment/pitr.sh (1)
306-326: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe F1 window costs up to 150 seconds on the success path.
The first loop polls up to 60 seconds for restore-failure evidence and breaks early. The second loop polls up to 90 seconds for the promotion message and breaks only when promotion occurs. When the test passes, promotion never occurs, so the second loop always runs its full 90 seconds and issues 90
docker compose logscalls. Confirm this budget is acceptable forpnpm test:deployment. A shorter window that still exceeds the observed happy-path promotion time gives the same proof.🤖 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 `@tests/deployment/pitr.sh` around lines 306 - 326, Shorten the second polling window in the F1 recovery check around PROMOTED_EARLY so the successful required-WAL-missing path does not spend the full 90 seconds polling. Retain enough iterations to exceed the observed time for an incorrect promotion, preserve detection of the “database system is ready to accept connections” message, and keep the existing failure handling intact.
🤖 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 `@docs/audits/P7-C-PORTABLE-BACKUP-RECOVERY-CLOSEOUT.md`:
- Around line 165-167: Rewrite the PITR statement in the closeout document to
describe the 60-second archive_timeout as an archival-freshness delay bound for
active workloads, not a recovery-precision or target-granularity limit. State
that recovery depends on the available archived WAL and the selected recovery
target, such as recovery_target_time, recovery_target_lsn, recovery_target_name,
or recovery_target_xid.
- Around line 101-103: Update the statement in the audit closeout to scope
runtime credential derivation to “Online PostgreSQL scripts” instead of “All
scripts.” Add documentation for the cold-filesystem backup and restore scripts
describing their path and ownership checks, without implying they use a running
database container.
In `@docs/deployment/backup-and-recovery.md`:
- Line 574: Update the F1 heading text to use a single modal by replacing
“missing REQUIRED archived WAL” with “missing required WAL” or equivalent
wording such as “WAL required for recovery is missing,” while preserving the
rest of the heading.
- Around line 373-378: Update the pg_verifybackup description in the backup
verification section to remove file modification-time validation, stating that
verification covers file checksums and the manifest checksum. Retain SHA256 as
the checksum algorithm because the backup script passes --manifest-checksums
SHA256.
In `@tests/deployment/launchpad-bootstrap.sh`:
- Around line 109-110: Replace the direct exit after compose_logs in the
health-timeout path with a call to fail(), preserving log collection first so
timeout failures update FAIL_COUNT and use the standard failure summary.
- Around line 250-252: After Stack B becomes healthy, add a check against the
running app container’s environment to verify LAUNCHPAD_SETUP_TOKEN is unset or
empty, failing the test if a non-empty value is present. Keep the existing unset
LAUNCHPAD_SETUP_TOKEN setup and use the Stack B container-health flow to perform
this assertion.
- Around line 33-37: Update the run identifier initialization around TS,
PROJECT_A, and PROJECT_B to use a per-run unique value rather than a
seconds-only timestamp, and derive both Compose project names from that value.
Preserve the existing project-specific naming and ensure the identifier is
available before Compose launches so compose_down_best_effort receives unique
names.
In `@tests/deployment/lib.sh`:
- Around line 117-135: Update tests/deployment/lib.sh lines 117-135 so
safe_temp_root records or exposes each TMPDIR-aware path it creates, and
cleanup_temp_root safely removes only those roots. In
tests/deployment/compose-smoke.sh lines 106-109, replace the hard-coded /tmp
guard and host rm -rf with comparison to the returned safe_temp_root path
followed by cleanup_temp_root. Apply the same recorded-path comparisons in
tests/deployment/logical-backup-restore.sh lines 56-64,
tests/deployment/persistence-and-cold-restore.sh lines 63-73, and
tests/deployment/pitr.sh lines 68-79; in pitr.sh also remove the inert
CORRUPT_DIR entry added at line 350.
- Around line 24-39: Update run_compose so an empty first argument is consumed
before forwarding arguments to docker compose, while keeping project empty so
COMPOSE_PROJECT_NAME remains the fallback. Preserve the existing behavior for
non-empty project names and flag-first arguments, and ensure helpers such as
db_container and psql_exec no longer pass an empty argument through.
In `@tests/deployment/logical-backup-restore.sh`:
- Around line 42-43: Update the cleanup guards in the deployment test scripts
around ROOT, BACKUP_DIR_PARENT, and their corresponding cleanup blocks to
compare paths against the values returned by safe_temp_root rather than assuming
a /tmp prefix. Apply the same change to persistence-and-cold-restore.sh and
pitr.sh, ensuring cleanup_temp_root runs when TMPDIR points elsewhere.
In `@tests/deployment/persistence-and-cold-restore.sh`:
- Around line 63-73: Update cleanup() to validate created directories against
the actual paths returned by safe_temp_root, rather than requiring a hard-coded
/tmp prefix in its regex. Preserve the existing name-pattern restrictions for
persist roots and backups, while allowing any TMPDIR location so
cleanup_temp_root runs for all created directories.
In `@tests/deployment/pitr.sh`:
- Around line 68-79: Update cleanup() to validate registered directories against
the actual paths returned by safe_temp_root, without assuming a /tmp prefix,
while preserving the allowed PITR root-name pattern. Remove the CREATED_DIRS
registration of CORRUPT_DIR near its creation, since that path cannot satisfy
the cleanup guard.
- Around line 131-151: Update run_idem_case to accept an explicit case-name
argument in addition to src, tgt, and expect, and use that name in both pass and
fail messages instead of ${3}. Update each run_idem_case invocation to provide a
distinct label for the absent-target, identical-retry, and byte-collision cases
so their results remain distinguishable.
---
Outside diff comments:
In `@apps/api/src/routes/launchpad.ts`:
- Around line 74-76: Update isInstallationInitialized to accept the applicable
route context and pass it to
createOrganizationRepo(fastify.db).defaultOrganizationExists(ctx). Ensure the
route’s repository access follows the repo.method(ctx, ...) contract, using the
established public or system context for this route.
---
Duplicate comments:
In `@scripts/backup/postgres-logical-restore.sh`:
- Around line 143-150: Update the DROP DATABASE statement in the restore heredoc
to use PostgreSQL’s WITH (FORCE) option, preserving the existing :"target_db"
identifier interpolation and CREATE DATABASE flow so connected clients are
terminated before recreation.
---
Nitpick comments:
In `@scripts/repository-contract/deployment-topology-contract.mjs`:
- Around line 110-134: Update the deployment test discovery around
deploymentTestsDir so a missing tests/deployment directory records a contract
error instead of being treated as valid. Distinguish ENOENT from other
filesystem failures, preserving the existing validation for present directories
and propagating unexpected read errors.
In `@tests/deployment/compose-smoke.sh`:
- Around line 191-204: Replace the remaining hand-built docker compose ps
invocations in the worker-state checks and the Test 5b setup around
WORKER_CONTAINER with the existing run_compose helper, passing the same project,
compose file, and ps -q email-worker arguments. Apply the same change to the
corresponding invocations near the later referenced checks, preserving all
existing output and error-handling behavior.
In `@tests/deployment/lib.sh`:
- Around line 137-145: Update psql_exec and wait_for_postgres to derive
POSTGRES_USER and POSTGRES_DB from the running database container, using the
same exam fallback behavior as the backup scripts, and pass those values to psql
instead of hard-coding exam.
In `@tests/deployment/persistence-and-cold-restore.sh`:
- Around line 155-162: Remove the hard-coded PostgreSQL major-version directory
from the PGDATA visibility check and related PITR paths. Add shared version
discovery or derivation in tests/deployment/lib.sh, then reuse that symbol in
persistence-and-cold-restore.sh and pitr.sh for PG_VERSION paths and PostgreSQL
image references so version bumps update the tests automatically.
In `@tests/deployment/pitr.sh`:
- Around line 306-326: Shorten the second polling window in the F1 recovery
check around PROMOTED_EARLY so the successful required-WAL-missing path does not
spend the full 90 seconds polling. Retain enough iterations to exceed the
observed time for an incorrect promotion, preserve detection of the “database
system is ready to accept connections” message, and keep the existing failure
handling intact.
🪄 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: ce4ead88-87f3-4a64-bbae-ad470f582ac1
📒 Files selected for processing (28)
.env.exampleREADME.mdapps/api/src/audit/auditPolicy.tsapps/api/src/routes/launchpad.test.tsapps/api/src/routes/launchpad.tsapps/api/src/scripts/bootstrap-admin.test.tsapps/api/src/scripts/bootstrap-admin.tsdocker-compose.ymldocs/audits/P7-C-PORTABLE-BACKUP-RECOVERY-CLOSEOUT.mddocs/deployment/backup-and-recovery.mddocs/deployment/mvp-deployment-runbook.mddocs/roadmap/P7-system-readiness-and-exam-modes.mdpackage.jsonpackages/contracts/src/messageRegistry.tspackages/domain/src/errors.tsscripts/backup/cold-filesystem-backup.shscripts/backup/cold-filesystem-restore.shscripts/backup/pg-basebackup.shscripts/backup/postgres-enable-pitr.shscripts/backup/postgres-logical-backup.shscripts/backup/postgres-logical-restore.shscripts/repository-contract/deployment-topology-contract.mjstests/deployment/compose-smoke.shtests/deployment/launchpad-bootstrap.shtests/deployment/lib.shtests/deployment/logical-backup-restore.shtests/deployment/persistence-and-cold-restore.shtests/deployment/pitr.sh
🚧 Files skipped from review as they are similar to previous changes (11)
- .env.example
- docs/deployment/mvp-deployment-runbook.md
- README.md
- packages/contracts/src/messageRegistry.ts
- docs/roadmap/P7-system-readiness-and-exam-modes.md
- scripts/backup/postgres-logical-backup.sh
- scripts/backup/pg-basebackup.sh
- docker-compose.yml
- scripts/backup/postgres-enable-pitr.sh
- apps/api/src/routes/launchpad.test.ts
- scripts/backup/cold-filesystem-restore.sh
…per, ctx, DROP FORCE
|
AI review done up to commit: fab5b7f AI Review Summary:The changes in this pull request primarily focus on improving the robustness and clarity of deployment scripts and documentation, alongside a minor API modification to pass context. Key changes include:
Overall quality assessment: The changes are well-intentioned, improving the robustness of backup/restore procedures and updating documentation for clarity. The introduction of |
… cleanup, PG version derivation Review follow-up on PR #274: - lib.sh: safe_temp_root now records every created path in a registry and cleanup_temp_root removes ONLY recorded roots (exact match), so TMPDIR location never matters and arbitrary paths can never be removed. run_compose consumes an explicit empty first argument (helpers no longer forward an empty arg). psql_exec / wait_for_postgres derive POSTGRES_USER/POSTGRES_DB from the running db container (exam fallback). PG_IMAGE / PG_MAJOR are derived from docker-compose.yml so a version bump updates the tests automatically. - compose-smoke / launchpad-bootstrap / persistence-and-cold-restore / logical-backup-restore / pitr: cleanup guards now compare against the safe_temp_root registry instead of hard-coded /tmp regexes; the compose smoke data root is removed container-assisted (host rm -rf could not delete postgres-owned files). pitr drops the inert CORRUPT_DIR registration (nested under a registered root), names the archive idempotency cases explicitly (absent-target / identical-retry / byte-collision), shortens the F1 no-promotion window to 30s (exceeds any incorrect-promotion time), and uses the derived PG_MAJOR/PG_IMAGE in PGDATA paths and postgres image references. - launchpad-bootstrap: per-run unique RUN_ID for Compose project names (seconds-only timestamp collides across runs), health-timeout path uses fail() so FAIL_COUNT/summary stay consistent, and Stack B now asserts the running app container's environment carries no LAUNCHPAD_SETUP_TOKEN. - compose-smoke: remaining hand-built docker compose ps invocations routed through run_compose. - deployment-topology-contract.mjs: a missing tests/deployment/ directory is now a contract error (ENOENT distinguished; other errors propagate). - Docs: closeout scopes runtime credential derivation to the online PostgreSQL scripts and documents the cold scripts' path/ownership checks; archive_timeout described as an archival-freshness bound for active workloads (recovery depends on available WAL + the selected target type), not a target-granularity limit. backup-and-recovery.md F1 heading reads 'missing required WAL'; pg_verifybackup wording drops the inaccurate mtime validation (checksums + manifest checksum only), also fixed in pg-basebackup.sh's comment.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| fi | ||
| compose_down_best_effort "${PROJECT_REC}-miss" | ||
|
|
||
| # ── 6. F2: corrupt base backup → pg_verifybackup rejects ───────────────── |
There was a problem hiding this comment.
The removal of this line is correct because CORRUPT_DIR is not created via safe_temp_root (and thus should not be managed by cleanup_temp_root). However, CORRUPT_DIR is created via mkdir -p at line 347 and is not explicitly cleaned up later in the script's cleanup function. This will lead to CORRUPT_DIR being left behind after the test run, which is a minor resource leak. Please add rm -rf "${CORRUPT_DIR}" to the cleanup function or ensure it is otherwise removed.
|
AI review done up to commit: d15702f AI Review Summary:The pull request introduces significant improvements to the deployment test suite and related documentation. Key changes include:
Overall, the quality of the changes is high. The refactoring leads to a more robust, secure, and flexible test suite. The new safety mechanisms for temporary directory cleanup are particularly noteworthy. I've only identified one minor resource leak where a temporary directory created in |
The temp-root registry introduced in d15702f was INERT: safe_temp_root was always called via command substitution (VAR="$(safe_temp_root ...)"), which runs the function in a subshell, so the SAFE_TEMP_ROOTS append was lost. cleanup_temp_root's registry check therefore always bailed (found != yes) and NO suite ever cleaned up its temp roots — every deployment-test run leaked gigabytes under /tmp. The suites passed because cleanup failure is silent. Fix: safe_temp_root now takes the caller's variable NAME and assigns via printf -v in the caller's scope, so both the mktemp path and the registry append survive. All 14 call sites across the five suites are converted from command substitution to the variable-name convention (the function fails fast on a missing name, so a future command-substitution call is a hard error, not a silent leak). Also: - launchpad-bootstrap.sh was the ONE suite d15702f missed: its cleanup still used the old '/tmp/launchpad-' regex guard + host rm -rf (which cannot delete postgres-owned PGDATA files). Converted to cleanup_temp_root, matching the other four suites. - cleanup_temp_root now WARNs to stderr when a root survives removal, so a future cleanup regression is visible instead of silent. Verified: full pnpm test:deployment (compose-smoke, launchpad-bootstrap 13/13, persistence, logical, pitr 7/7) passes AND leaves zero temp roots; launchpad re-run after conversion cleans its WORK root. All pre-fix leftovers removed manually.
|
AI review done up to commit: 5c4fc99 AI Review Summary:The pull request refactors the temporary directory management in the deployment test suite. The |
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 `@tests/deployment/lib.sh`:
- Around line 160-190: Update cleanup_temp_root to use the pinned ${PG_IMAGE}
container image instead of alpine:latest, and add Docker’s --pull=never option
so cleanup only uses an image already present locally without contacting a
registry.
🪄 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: cc06233f-ba5a-4135-b463-3e3f7a6469c7
📒 Files selected for processing (16)
apps/api/src/routes/launchpad.tsdocs/audits/P7-C-PORTABLE-BACKUP-RECOVERY-CLOSEOUT.mddocs/deployment/backup-and-recovery.mddocs/deployment/mvp-deployment-runbook.mdpackages/db/src/repository/organizationRepo.tsscripts/backup/cold-filesystem-backup.shscripts/backup/cold-filesystem-restore.shscripts/backup/pg-basebackup.shscripts/backup/postgres-logical-restore.shscripts/repository-contract/deployment-topology-contract.mjstests/deployment/compose-smoke.shtests/deployment/launchpad-bootstrap.shtests/deployment/lib.shtests/deployment/logical-backup-restore.shtests/deployment/persistence-and-cold-restore.shtests/deployment/pitr.sh
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/api/src/routes/launchpad.ts
- tests/deployment/logical-backup-restore.sh
- tests/deployment/persistence-and-cold-restore.sh
- scripts/repository-contract/deployment-topology-contract.mjs
- docs/deployment/backup-and-recovery.md
- scripts/backup/postgres-logical-restore.sh
- scripts/backup/cold-filesystem-restore.sh
- packages/db/src/repository/organizationRepo.ts
- scripts/backup/pg-basebackup.sh
- docs/deployment/mvp-deployment-runbook.md
- tests/deployment/compose-smoke.sh
- scripts/backup/cold-filesystem-backup.sh
- docs/audits/P7-C-PORTABLE-BACKUP-RECOVERY-CLOSEOUT.md
| # Remove a temp root created by safe_temp_root. Files inside may be owned | ||
| # by the container postgres user (not host-readable), so removal is | ||
| # container-assisted. Paths NOT recorded by safe_temp_root are never | ||
| # touched. | ||
| cleanup_temp_root() { | ||
| local d="$1" | ||
| local i | ||
| local remaining=() | ||
| local found="no" | ||
| for i in "${SAFE_TEMP_ROOTS[@]}"; do | ||
| if [ "${i}" = "${d}" ]; then | ||
| found="yes" | ||
| else | ||
| remaining+=("${i}") | ||
| fi | ||
| done | ||
| if [ "${found}" != "yes" ]; then | ||
| return 0 | ||
| fi | ||
| SAFE_TEMP_ROOTS=("${remaining[@]}") | ||
| if [ ! -d "${d}" ]; then | ||
| return 0 | ||
| fi | ||
| docker run --rm -v "${d}:/d" alpine:latest \ | ||
| sh -c 'rm -rf /d/* /d/.[!.]* 2>/dev/null || true' \ | ||
| >/dev/null 2>&1 || true | ||
| rmdir "${d}" 2>/dev/null || rm -rf "${d}" 2>/dev/null || true | ||
| if [ -d "${d}" ]; then | ||
| echo "WARN: cleanup_temp_root could not fully remove ${d} (left in place)." >&2 | ||
| fi | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "files:"
fd -a 'lib\.sh$|compose\.ya?ml$|Dockerfile$|docs/SPEC\.md|docs/roadmap/phase-roadmap\.md' . | sed 's#^\./##'
echo
echo "tests/deployment/lib.sh occurrences around safe_temp_root/cleanup_temp_root/PG_IMAGE:"
python3 - <<'PY'
from pathlib import Path
p=Path('tests/deployment/lib.sh')
lines=p.read_text(errors='replace').splitlines()
for i,l in enumerate(lines,1):
if any(x in l for x in ['safe_temp_root','cleanup_temp_root','PG_IMAGE','docker run','alpine:latest','pull=never','alpine']):
print(f"{i}: {l}")
PY
echo
echo "docker references in deployment scripts:"
rg -n "docker (run|build|compose)|pull=never|alpine:latest|PG_IMAGE|safe_temp_root" tests -S || true
echo
echo "compose image names mentioning postgres:"
rg -n "postres|postgres|image:|version:" -S compose* docker-compose* .github 2>/dev/null || trueRepository: jnhu76/exam
Length of output: 11939
🌐 Web query:
Docker run default pull policy missing image docker.com
💡 Result:
When using the docker run command, the default pull policy for a missing image is missing [1][2][3]. This means that Docker will only attempt to pull the image from the registry if it is not already present in the local image cache [1]. If the image is found locally, Docker will use the cached version instead of pulling it again [1]. This behavior can be controlled using the --pull flag, which supports the following options [1][3]: - missing (default): Pull the image only if it is not found in the local cache [1]. - always: Always pull the image from the registry before creating the container, even if it already exists locally [1]. - never: Never attempt to pull the image; the command will fail if the image is not found in the local cache [1]. It is important to distinguish this from Kubernetes, which has different default behaviors for image pull policies depending on the image tag (e.g.,:latest or missing tags default to Always) [4]. In contrast, the standard Docker CLI command consistently defaults to missing [1].
Citations:
- 1: https://docs.docker.com/reference/cli/docker/container/run/
- 2: https://github.com/docker/cli/blob/master/docs/reference/commandline/run.md
- 3: https://man.archlinux.org/man/docker-container-run.1.en.txt
- 4: https://kubernetes.io/docs/concepts/containers/images/
Use a local-only image during temp-root cleanup.
cleanup_temp_root() calls Docker at line 183. If alpine:latest is not present locally, Docker pulls it by default, and latest can change. This adds a runtime registry dependency during offline-required deployment steps. Use a fully pinned image present on the deployment host, such as the pinned ${PG_IMAGE}, and pass --pull=never.
🤖 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 `@tests/deployment/lib.sh` around lines 160 - 190, Update cleanup_temp_root to
use the pinned ${PG_IMAGE} container image instead of alpine:latest, and add
Docker’s --pull=never option so cleanup only uses an image already present
locally without contacting a registry.
Source: Coding guidelines
1 participant
P7-C REBUILD — Portable Persistence, Backup & PostgreSQL Disaster Recovery
Full rebuild of P7-C off
origin/master(baseline2a1a9eb). PR #273 istreated as historical/reference only — it is NOT the baseline and was not
repaired, cherry-picked, or simplified. Executed C1 → C2 → C3 serially in
this branch, one commit per phase, then a corrective pass that collapsed
PITR into the canonical Compose and renamed the deployment verification to
capability-named suites.
Verdict:
P7-C PORTABLE PERSISTENCE / BACKUP / POSTGRESQL RECOVERY READY FOR HUMAN REVIEWFull evidence:
docs/audits/P7-C-PORTABLE-BACKUP-RECOVERY-CLOSEOUT.md.Authority stores (reconfirmed at C0)
${EXAM_DATA_ROOT}/postgres, operator-visible)Final structure — one canonical Compose
Phase summary
df2d07dfbootstrapAdminOnFreshDb)e164fd30pg_dump -Fconline backup; clean restore (DROP DATABASE … WITH (FORCE)+template0+pg_restore --no-owner --exit-on-error) — NOT--clean --if-existsinto a dirty DB0df4ba72pg_basebackup -X stream+pg_verifybackup;postgres-enable-pitr.shWAL continuous archiving (idempotent non-overwritingarchive_command,archive_timeout 60s, persisted viaALTER SYSTEM); PITR torecovery_target_lsn/time/xid0ebdf6ebfbea2a55…a8767dfefab5b7f5…5c4fc997Review closeout (
fab5b7f5…5c4fc997)"${EXAM_DATA_ROOT:-./data}"instead of a hardcoded./data(a hardcoded path could silently back up the wrong directory on a relocated deployment).alpine:latesthelper-image dependency from the backup scripts: the cold backup/restore scripts reuse the deployment's own pinnedpostgres:18.4-bookworm(which shipssh/cp/find), andpg-basebackup.shdropped its cosmeticdusize display.defaultOrganizationExists(ctx)now takes ctx like every other repo method.postgres-logical-restore.shDROP now usesWITH (FORCE)(terminates lingering connections; robustness only — the stop-API-first contract is unchanged).d15702f9):safe_temp_rootnow records every created path in a registry andcleanup_temp_rootremoves ONLY recorded roots — TMPDIR location never matters and arbitrary paths can never be removed;run_composeconsumes an explicit empty first argument;wait_for_postgres/psql_execderive POSTGRES_USER/POSTGRES_DB from the running db container; PG_IMAGE/PG_MAJOR are derived from docker-compose.yml so a postgres version bump updates the tests automatically.launchpad-bootstrap.sh: per-run unique RUN_ID for Compose project names (seconds-only timestamps collided across runs); health-timeout path usesfail()so the summary stays consistent; Stack B asserts the running app container carries no LAUNCHPAD_SETUP_TOKEN env.pitr.sh: archive-idempotency cases named explicitly (absent-target / identical-retry / byte-collision); F1 no-promotion window shortened to 30s (exceeds any incorrect-promotion time); PGDATA paths and postgres image references use the derived PG_MAJOR/PG_IMAGE.compose-smoke.shdata root is cleaned container-assisted (hostrm -rfcannot delete postgres-owned files).deployment-topology-contract.mjs: a missingtests/deployment/directory is now a contract error (ENOENT); other errors propagate.5c4fc997): the registry introduced ind15702f9was INERT —VAR="$(safe_temp_root …)"command substitution runs the function in a subshell, so the registry append was lost andcleanup_temp_rootalways bailed; every suite leaked its temp roots (verified: a full suite run left all ~25 roots behind).safe_temp_rootnow takes the caller's variable NAME and assigns viaprintf -vin the caller's scope (all 14 call sites converted; a missing name is a hard error, so a future command-substitution call fails instead of leaking).launchpad-bootstrap.shwas the one suited15702f9missed — it still used the old/tmp/…regex guard + hostrm -rf(which cannot delete postgres-owned PGDATA files) — and is converted tocleanup_temp_rootlike the other four suites.cleanup_temp_rootnow WARNs when a root survives. Verified: fullpnpm test:deploymentpasses with ZERO temp-root leftovers.archive_timeoutis described as an archival-freshness bound for active workloads (recovery depends on available archived WAL + the selected target type), not a target-granularity limit; F1 heading reads "missing required WAL";pg_verifybackupwording drops the inaccurate mtime validation (checksums + manifest checksum only), also fixed in thepg-basebackup.shcomment.Verification suites (deterministic, isolated, all PASS on final head)
compose-smoke.sh— production Compose parses, starts, and honors the environment contract (isolated project + temp data root).launchpad-bootstrap.sh— first-install contract A1–A7 + B1: fresh stack uninitialized; wrong token 403; correct token creates first Admin + default org; never reopens (409); login works; exactly 1 Admin / 0 Candidates /admin.bootstrapaudit evidence; register disabled; no-token disables launchpad.persistence-and-cold-restore.sh— container-recreation persistence, stopped-directory relocation, cold backup/restore round-trip (identical invariants after each step).logical-backup-restore.sh— A-present / B-absent clean logical restore.pitr.sh— archive idempotency (absent / identical retry / byte collision); happy PITR (A + A1 + B present, C absent, promoted); F1 missing REQUIRED archived WAL (loud — stays in archive recovery, no promotion); F2 corrupt base backup (pg_verifybackuprejects); F3 invalid recovery target (refuses to start).Gates
pnpm verify✅ (format:check, lint family, copy/arch/env-contract lint, typecheck, openapi check, coverage, build)Scope discipline (what was NOT built — "Delete ceremony, not safety")
NO custom relocation manifest protocol · NO Docker image identity framework · NO OCI digest authority subsystem · NO cross-runner docker-save bundle · NO migration-history preflight framework · NO historical-migration omission allowlist · NO custom recoveryEpoch · NO generic reconciler/job engine · NO Kubernetes/Patroni/HA · NO Admin restore button · NO Desktop · NO PG18 incremental backups · NO retention engine · NO PITR automation in base compose · NO P7-E control plane. No backup-helper image management framework — the scripts reuse the deployment's own pinned postgres image.
PR #273 is NOT recreated under different names.
ADR-016 boundary
Container restart / C1 relocation = same authoritative history. C2 logical restore / C3 physical restore / PITR = authoritative history replacement. No schema change marks these events. The exam system's authoritative state is whatever PostgreSQL currently holds.
P7-E handoff (deferred, not started)
RPO/RTO profile automation · retention automation · Admin backup visibility (read-only; restore stays operator-owned) · files/settings backup beyond PostgreSQL · cross-PG-major upgrade playbook.
P0–P3
None outstanding.
Do NOT merge automatically. Do NOT start P7-E.
Summary by CodeRabbit
New Features
Documentation
Tests